From d804f9cb3fde9b213647e4b4217f6441469c6823 Mon Sep 17 00:00:00 2001
From: ex-takashima <takajyun00@gmail.com>
Date: Thu, 26 Feb 2026 16:33:32 +0900
Subject: [PATCH] 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>
---
 pkg/media/store.go      | 27 +++++++++++++++++++++------
 pkg/media/store_test.go | 21 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/pkg/media/store.go b/pkg/media/store.go
index c6a6502a1..c4cd816d2 100644
--- a/pkg/media/store.go
+++ b/pkg/media/store.go
@@ -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.
+// Phase 1 (under lock): remove entries from maps.
+// Phase 2 (no lock): delete files from disk.
 func (s *FileMediaStore) ReleaseAll(scope string) error {
-	s.mu.Lock()
-	defer s.mu.Unlock()
+	// Phase 1: collect paths and remove from maps under lock
+	var paths []string
 
+	s.mu.Lock()
 	refs, ok := s.scopeToRefs[scope]
 	if !ok {
+		s.mu.Unlock()
 		return nil
 	}
 
 	for ref := range refs {
 		if entry, exists := s.refs[ref]; exists {
-			if err := os.Remove(entry.path); err != nil && !os.IsNotExist(err) {
-				// Log but continue — best effort cleanup
-			}
+			paths = append(paths, entry.path)
 			delete(s.refs, ref)
 			delete(s.refToScope, ref)
 		}
 	}
-
 	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
 }
 
@@ -207,6 +217,11 @@ func (s *FileMediaStore) Start() {
 	if !s.cleanerCfg.Enabled || s.stop == nil {
 		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() {
 		log.Printf("[media] cleanup enabled: interval=%s, max_age=%s",
diff --git a/pkg/media/store_test.go b/pkg/media/store_test.go
index e45212671..b14e1ff9a 100644
--- a/pkg/media/store_test.go
+++ b/pkg/media/store_test.go
@@ -386,6 +386,27 @@ func TestStartDisabledIsNoop(t *testing.T) {
 	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) {
 	dir := t.TempDir()
 	store := newTestStoreWithCleanup(50 * time.Millisecond)