From b705e58528cb85e15e4ce80468684f56a2dfe160 Mon Sep 17 00:00:00 2001 From: ex-takashima Date: Thu, 26 Feb 2026 16:22:49 +0900 Subject: [PATCH] fix(media): address review comments on TTL cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CleanExpired: split into two phases — collect expired entries under lock, then delete files after releasing the lock to minimize contention - CleanExpired: guard against zero MaxAge (no-op if unconfigured) - CleanExpired: log file removal errors instead of silently ignoring - Start: protect with startOnce to prevent multiple goroutines - Stop: rename once -> stopOnce for clarity - cmd_gateway: call mediaStore.Stop() on error path after Start() - Add TestCleanExpiredZeroMaxAge and double-Start test Co-Authored-By: Claude Opus 4.6 --- cmd/picoclaw/internal/gateway/helpers.go | 1 + pkg/media/store.go | 70 +++++++++++++++--------- pkg/media/store_test.go | 23 ++++++++ 3 files changed, 68 insertions(+), 26 deletions(-) diff --git a/cmd/picoclaw/internal/gateway/helpers.go b/cmd/picoclaw/internal/gateway/helpers.go index 8255fb6e8..360073f01 100644 --- a/cmd/picoclaw/internal/gateway/helpers.go +++ b/cmd/picoclaw/internal/gateway/helpers.go @@ -124,6 +124,7 @@ func gatewayCmd(debug bool) error { channelManager, err := channels.NewManager(cfg, msgBus, mediaStore) if err != nil { + mediaStore.Stop() return fmt.Errorf("error creating channel manager: %w", err) } diff --git a/pkg/media/store.go b/pkg/media/store.go index 6851f9512..c6a6502a1 100644 --- a/pkg/media/store.go +++ b/pkg/media/store.go @@ -59,7 +59,8 @@ type FileMediaStore struct { cleanerCfg MediaCleanerConfig stop chan struct{} - once sync.Once + startOnce sync.Once + stopOnce sync.Once nowFunc func() time.Time // for testing } @@ -155,20 +156,26 @@ func (s *FileMediaStore) ReleaseAll(scope string) error { } // CleanExpired removes all entries older than MaxAge. -// Both the file on disk and the in-memory references are deleted atomically -// under the same mutex, preventing dangling references. +// Phase 1 (under lock): identify expired entries and remove from maps. +// Phase 2 (no lock): delete files from disk to minimize lock contention. func (s *FileMediaStore) CleanExpired() int { - s.mu.Lock() - defer s.mu.Unlock() + if s.cleanerCfg.MaxAge <= 0 { + return 0 + } + // Phase 1: collect expired entries under lock + type expiredEntry struct { + ref string + path string + } + + s.mu.Lock() cutoff := s.nowFunc().Add(-s.cleanerCfg.MaxAge) - removed := 0 + var expired []expiredEntry for ref, entry := range s.refs { if entry.storedAt.Before(cutoff) { - if err := os.Remove(entry.path); err != nil && !os.IsNotExist(err) { - // Log but continue — best effort cleanup - } + expired = append(expired, expiredEntry{ref: ref, path: entry.path}) scope := s.refToScope[ref] if scopeRefs, ok := s.scopeToRefs[scope]; ok { @@ -180,45 +187,56 @@ func (s *FileMediaStore) CleanExpired() int { delete(s.refs, ref) delete(s.refToScope, ref) - removed++ + } + } + s.mu.Unlock() + + // Phase 2: delete files without holding the lock + for _, e := range expired { + if err := os.Remove(e.path); err != nil && !os.IsNotExist(err) { + log.Printf("[media] cleanup: failed to remove %s: %v", e.path, err) } } - return removed + return len(expired) } // Start begins the background cleanup goroutine if cleanup is enabled. +// Safe to call multiple times; only the first call starts the goroutine. func (s *FileMediaStore) Start() { if !s.cleanerCfg.Enabled || s.stop == nil { return } - log.Printf("[media] cleanup enabled: interval=%s, max_age=%s", - s.cleanerCfg.Interval, s.cleanerCfg.MaxAge) + s.startOnce.Do(func() { + log.Printf("[media] cleanup enabled: interval=%s, max_age=%s", + s.cleanerCfg.Interval, s.cleanerCfg.MaxAge) - go func() { - ticker := time.NewTicker(s.cleanerCfg.Interval) - defer ticker.Stop() + go func() { + ticker := time.NewTicker(s.cleanerCfg.Interval) + defer ticker.Stop() - for { - select { - case <-ticker.C: - if n := s.CleanExpired(); n > 0 { - log.Printf("[media] cleanup: removed %d expired entries", n) + for { + select { + case <-ticker.C: + if n := s.CleanExpired(); n > 0 { + log.Printf("[media] cleanup: removed %d expired entries", n) + } + case <-s.stop: + return } - case <-s.stop: - return } - } - }() + }() + }) } // Stop terminates the background cleanup goroutine. +// Safe to call multiple times; only the first call closes the channel. func (s *FileMediaStore) Stop() { if s.stop == nil { return } - s.once.Do(func() { + s.stopOnce.Do(func() { close(s.stop) }) } diff --git a/pkg/media/store_test.go b/pkg/media/store_test.go index e934bf9c0..e45212671 100644 --- a/pkg/media/store_test.go +++ b/pkg/media/store_test.go @@ -345,6 +345,8 @@ func TestStartStopLifecycle(t *testing.T) { // Start and stop should not panic store.Start() + // Double start should not spawn a second goroutine + store.Start() time.Sleep(100 * time.Millisecond) store.Stop() @@ -352,6 +354,27 @@ func TestStartStopLifecycle(t *testing.T) { store.Stop() } +func TestCleanExpiredZeroMaxAge(t *testing.T) { + store := NewFileMediaStoreWithCleanup(MediaCleanerConfig{ + Enabled: true, + MaxAge: 0, + Interval: time.Hour, + }) + + dir := t.TempDir() + path := createTempFile(t, dir, "file.jpg") + ref, _ := store.Store(path, MediaMeta{Source: "test"}, "scope1") + + // Zero MaxAge should be a no-op + removed := store.CleanExpired() + if removed != 0 { + t.Errorf("expected 0 removed with zero MaxAge, got %d", removed) + } + if _, err := store.Resolve(ref); err != nil { + t.Errorf("ref should still resolve: %v", err) + } +} + func TestStartDisabledIsNoop(t *testing.T) { store := NewFileMediaStoreWithCleanup(MediaCleanerConfig{ Enabled: false,