fix(media): use project logger and harden map cleanup

- Replace stdlib log.Printf with logger.InfoCF/WarnCF for consistency
  with the rest of the codebase (addresses @nikolasdehor review point #3)
- ReleaseAll: clean refToScope/refs mappings even if refs entry is missing
- CleanExpired: guard refToScope lookup before scope cleanup
- Add TestReleaseAllCleansMappingsIfRefsMissing for robustness

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
ex-takashima
2026-02-26 22:39:58 +09:00
committed by Hoshina
parent 61eae92b38
commit 94aa2b1788
2 changed files with 58 additions and 15 deletions
+28 -15
View File
@@ -2,12 +2,12 @@ package media
import (
"fmt"
"log"
"os"
"sync"
"time"
"github.com/google/uuid"
"github.com/sipeed/picoclaw/pkg/logger"
)
// MediaMeta holds metadata about a stored media file.
@@ -148,9 +148,9 @@ func (s *FileMediaStore) ReleaseAll(scope string) error {
for ref := range refs {
if entry, exists := s.refs[ref]; exists {
paths = append(paths, entry.path)
delete(s.refs, ref)
delete(s.refToScope, ref)
}
delete(s.refs, ref)
delete(s.refToScope, ref)
}
delete(s.scopeToRefs, scope)
s.mu.Unlock()
@@ -158,7 +158,10 @@ func (s *FileMediaStore) ReleaseAll(scope string) error {
// 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)
logger.WarnCF("media", "release: failed to remove file", map[string]any{
"path": p,
"error": err.Error(),
})
}
}
@@ -187,11 +190,12 @@ func (s *FileMediaStore) CleanExpired() int {
if entry.storedAt.Before(cutoff) {
expired = append(expired, expiredEntry{ref: ref, path: entry.path})
scope := s.refToScope[ref]
if scopeRefs, ok := s.scopeToRefs[scope]; ok {
delete(scopeRefs, ref)
if len(scopeRefs) == 0 {
delete(s.scopeToRefs, scope)
if scope, ok := s.refToScope[ref]; ok {
if scopeRefs, ok := s.scopeToRefs[scope]; ok {
delete(scopeRefs, ref)
if len(scopeRefs) == 0 {
delete(s.scopeToRefs, scope)
}
}
}
@@ -204,7 +208,10 @@ func (s *FileMediaStore) CleanExpired() int {
// 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)
logger.WarnCF("media", "cleanup: failed to remove file", map[string]any{
"path": e.path,
"error": err.Error(),
})
}
}
@@ -218,14 +225,18 @@ func (s *FileMediaStore) Start() {
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)
logger.WarnCF("media", "cleanup: skipped due to invalid config", map[string]any{
"interval": s.cleanerCfg.Interval.String(),
"max_age": s.cleanerCfg.MaxAge.String(),
})
return
}
s.startOnce.Do(func() {
log.Printf("[media] cleanup enabled: interval=%s, max_age=%s",
s.cleanerCfg.Interval, s.cleanerCfg.MaxAge)
logger.InfoCF("media", "cleanup enabled", map[string]any{
"interval": s.cleanerCfg.Interval.String(),
"max_age": s.cleanerCfg.MaxAge.String(),
})
go func() {
ticker := time.NewTicker(s.cleanerCfg.Interval)
@@ -235,7 +246,9 @@ func (s *FileMediaStore) Start() {
select {
case <-ticker.C:
if n := s.CleanExpired(); n > 0 {
log.Printf("[media] cleanup: removed %d expired entries", n)
logger.InfoCF("media", "cleanup: removed expired entries", map[string]any{
"count": n,
})
}
case <-s.stop:
return
+30
View File
@@ -134,6 +134,36 @@ func TestReleaseAllIdempotent(t *testing.T) {
}
}
func TestReleaseAllCleansMappingsIfRefsMissing(t *testing.T) {
dir := t.TempDir()
store := NewFileMediaStore()
path := createTempFile(t, dir, "file.jpg")
ref, err := store.Store(path, MediaMeta{Source: "test"}, "scope1")
if err != nil {
t.Fatalf("Store failed: %v", err)
}
// Simulate internal inconsistency: scopeToRefs/refToScope contains ref but refs map doesn't.
store.mu.Lock()
delete(store.refs, ref)
store.mu.Unlock()
if err := store.ReleaseAll("scope1"); err != nil {
t.Fatalf("ReleaseAll failed: %v", err)
}
// ReleaseAll should still clean mappings (even if it can't delete the file without the path).
store.mu.RLock()
defer store.mu.RUnlock()
if _, ok := store.refToScope[ref]; ok {
t.Error("refToScope should not contain ref after ReleaseAll")
}
if _, ok := store.scopeToRefs["scope1"]; ok {
t.Error("scopeToRefs should not contain scope1 after ReleaseAll")
}
}
func TestStoreNonexistentFile(t *testing.T) {
store := NewFileMediaStore()