fix(media): guard Interval<=0 panic, two-phase ReleaseAll
Address Codex (GPT-5.2) review feedback: - Start: guard against Interval<=0 or MaxAge<=0 to prevent time.NewTicker panic on misconfiguration - ReleaseAll: split into two phases (collect under lock, delete after unlock) matching CleanExpired pattern - ReleaseAll: log file removal errors - Add TestStartZeroIntervalNoPanic and TestStartZeroMaxAgeNoPanic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
b705e58528
commit
d804f9cb3f
2 changed files with 42 additions and 6 deletions
|
|
@ -132,26 +132,36 @@ func (s *FileMediaStore) ResolveWithMeta(ref string) (string, MediaMeta, error)
|
||||||
}
|
}
|
||||||
|
|
||||||
// ReleaseAll removes all files under the given scope and cleans up mappings.
|
// ReleaseAll removes all files under the given scope and cleans up mappings.
|
||||||
|
// Phase 1 (under lock): remove entries from maps.
|
||||||
|
// Phase 2 (no lock): delete files from disk.
|
||||||
func (s *FileMediaStore) ReleaseAll(scope string) error {
|
func (s *FileMediaStore) ReleaseAll(scope string) error {
|
||||||
s.mu.Lock()
|
// Phase 1: collect paths and remove from maps under lock
|
||||||
defer s.mu.Unlock()
|
var paths []string
|
||||||
|
|
||||||
|
s.mu.Lock()
|
||||||
refs, ok := s.scopeToRefs[scope]
|
refs, ok := s.scopeToRefs[scope]
|
||||||
if !ok {
|
if !ok {
|
||||||
|
s.mu.Unlock()
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
for ref := range refs {
|
for ref := range refs {
|
||||||
if entry, exists := s.refs[ref]; exists {
|
if entry, exists := s.refs[ref]; exists {
|
||||||
if err := os.Remove(entry.path); err != nil && !os.IsNotExist(err) {
|
paths = append(paths, entry.path)
|
||||||
// Log but continue — best effort cleanup
|
|
||||||
}
|
|
||||||
delete(s.refs, ref)
|
delete(s.refs, ref)
|
||||||
delete(s.refToScope, ref)
|
delete(s.refToScope, ref)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
delete(s.scopeToRefs, scope)
|
delete(s.scopeToRefs, scope)
|
||||||
|
s.mu.Unlock()
|
||||||
|
|
||||||
|
// Phase 2: delete files without holding the lock
|
||||||
|
for _, p := range paths {
|
||||||
|
if err := os.Remove(p); err != nil && !os.IsNotExist(err) {
|
||||||
|
log.Printf("[media] release: failed to remove %s: %v", p, err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -207,6 +217,11 @@ func (s *FileMediaStore) Start() {
|
||||||
if !s.cleanerCfg.Enabled || s.stop == nil {
|
if !s.cleanerCfg.Enabled || s.stop == nil {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if s.cleanerCfg.Interval <= 0 || s.cleanerCfg.MaxAge <= 0 {
|
||||||
|
log.Printf("[media] cleanup: skipped (interval=%s, max_age=%s)",
|
||||||
|
s.cleanerCfg.Interval, s.cleanerCfg.MaxAge)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
s.startOnce.Do(func() {
|
s.startOnce.Do(func() {
|
||||||
log.Printf("[media] cleanup enabled: interval=%s, max_age=%s",
|
log.Printf("[media] cleanup enabled: interval=%s, max_age=%s",
|
||||||
|
|
|
||||||
|
|
@ -386,6 +386,27 @@ func TestStartDisabledIsNoop(t *testing.T) {
|
||||||
store.Stop()
|
store.Stop()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestStartZeroIntervalNoPanic(t *testing.T) {
|
||||||
|
store := NewFileMediaStoreWithCleanup(MediaCleanerConfig{
|
||||||
|
Enabled: true,
|
||||||
|
MaxAge: time.Minute,
|
||||||
|
Interval: 0,
|
||||||
|
})
|
||||||
|
// Zero interval should not panic (time.NewTicker panics on <= 0)
|
||||||
|
store.Start()
|
||||||
|
store.Stop()
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestStartZeroMaxAgeNoPanic(t *testing.T) {
|
||||||
|
store := NewFileMediaStoreWithCleanup(MediaCleanerConfig{
|
||||||
|
Enabled: true,
|
||||||
|
MaxAge: 0,
|
||||||
|
Interval: time.Minute,
|
||||||
|
})
|
||||||
|
store.Start()
|
||||||
|
store.Stop()
|
||||||
|
}
|
||||||
|
|
||||||
func TestConcurrentCleanupSafety(t *testing.T) {
|
func TestConcurrentCleanupSafety(t *testing.T) {
|
||||||
dir := t.TempDir()
|
dir := t.TempDir()
|
||||||
store := newTestStoreWithCleanup(50 * time.Millisecond)
|
store := newTestStoreWithCleanup(50 * time.Millisecond)
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue