diff options
| author | Paul Buetow <paul@buetow.org> | 2026-05-19 14:18:43 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-05-19 14:18:43 +0300 |
| commit | 53a599e763eb8f105dd12c8e0a2a50e2a850e4d6 (patch) | |
| tree | 02bb08debc8844b77b65a32c8448044bc21a419b /player-server/internal | |
| parent | 212e849475701d91d5f173c3638af540fab796dd (diff) | |
Fix four defects flagged by S19/S20/S24; tighten scenarios
1. tagService.AssignTag and RemoveTag now use verifyModifyAccess
(owner role required) instead of verifyAccess. Tags are global
state visible to every user with access to a media item, so a
viewer must not be able to add or remove them. Favorites and
notes stay on verifyAccess because they're per-user data
(favorites.user_id, media_notes.user_id) and don't affect anyone
else. Verified via curl: viewer POST /media/{id}/tags now 403,
admin still 200.
2. serveFileResult now emits a strong ETag header
("<size>-<mtime-nanos>") before calling http.ServeContent. Go's
ServeContent honours If-None-Match when ETag is set, so iOS
audio clients and podcast apps can revalidate cached downloads
with conditional GETs. Verified via curl: ETag present on
/stream; If-None-Match matching the ETag returns 304.
3. MediaFilter gains IncludeDeleted flag; ListMedia skips the
implicit `deleted_at IS NULL` predicate when it is set.
FSScanner.loadExistingMedia now passes IncludeDeleted=true so
the dedup map includes soft-deleted rows. Previously a re-scan
of a soft-deleted file tried to CreateMedia and hit the
UNIQUE(set_id, rel_path) constraint, failing the whole scan and
setting progress.last_error. Now the rescan skips the row
cleanly; soft-delete sticks.
4. FSScanner.reconcileOrphans soft-deletes media rows whose
underlying file disappeared between scans. The scanner used to
only walk files that exist and never compare against the DB,
leaving phantom rows in GET /api/v1/media that 404'd on stream.
Verified via curl: rm /testdata/.../orphan.mp3, rescan, row
now has deleted_at != NULL.
Scenarios updated to lock in the fixed behaviour:
S19 step 15 — viewer tag-add now asserts 403, not 200.
S20 step 14 — asserts ETag is present and If-None-Match → 304.
S24 step 13 — asserts clean rescan (no last_error from UNIQUE).
S24 step 20 — asserts orphan rows are soft-deleted by rescan.
Verified: full Go unit suite passes; 25/25 LLM e2e scenarios;
22/22 Playwright e2e-web.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Diffstat (limited to 'player-server/internal')
| -rw-r--r-- | player-server/internal/api/handlers.go | 14 | ||||
| -rw-r--r-- | player-server/internal/repository/media.go | 4 | ||||
| -rw-r--r-- | player-server/internal/repository/repository.go | 6 | ||||
| -rw-r--r-- | player-server/internal/scanner/scanner.go | 40 | ||||
| -rw-r--r-- | player-server/internal/service/tag.go | 12 |
5 files changed, 72 insertions, 4 deletions
diff --git a/player-server/internal/api/handlers.go b/player-server/internal/api/handlers.go index 1423a84..a9c832f 100644 --- a/player-server/internal/api/handlers.go +++ b/player-server/internal/api/handlers.go @@ -8,11 +8,20 @@ import ( "log/slog" "net/http" "strconv" + "time" "codeberg.org/snonux/player/internal/model" "codeberg.org/snonux/player/internal/service" ) +// fileETag returns a strong ETag value (without surrounding quotes) for a +// file of the given size and modification time. Combining size with mtime +// nanoseconds is enough to detect any in-place rewrite or replacement — +// callers wrap the result in quotes when emitting the header. +func fileETag(size int64, modTime time.Time) string { + return fmt.Sprintf("%d-%d", size, modTime.UnixNano()) +} + // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------ @@ -177,6 +186,11 @@ func (s *Server) serveFileResult(w http.ResponseWriter, r *http.Request, res *se } w.Header().Set("Content-Type", stream.ContentType) w.Header().Set("Accept-Ranges", "bytes") + // Strong ETag derived from size and mtime nanoseconds. http.ServeContent + // reads If-None-Match / If-Match from the request once ETag is set, so + // clients (iOS audio player, podcast clients) can revalidate cached + // downloads without re-fetching the full body. + w.Header().Set("ETag", fmt.Sprintf("%q", fileETag(stream.Size, stream.ModTime))) s.logger.Info("api stream file", "file", stream.FileName, "size", stream.Size, "range", r.Header.Get("Range")) http.ServeContent(w, r, stream.FileName, stream.ModTime, stream.File) } diff --git a/player-server/internal/repository/media.go b/player-server/internal/repository/media.go index 60a611d..c06d257 100644 --- a/player-server/internal/repository/media.go +++ b/player-server/internal/repository/media.go @@ -231,7 +231,9 @@ func (s *SQLite) ListMedia(ctx context.Context, filter MediaFilter) ([]model.Med conds = append(conds, `media.duration <= ?`) args = append(args, *filter.MaxDuration) } - conds = append(conds, `media.deleted_at IS NULL`) + if !filter.IncludeDeleted { + conds = append(conds, `media.deleted_at IS NULL`) + } query += joins if len(conds) > 0 { diff --git a/player-server/internal/repository/repository.go b/player-server/internal/repository/repository.go index 487921f..e4b28c7 100644 --- a/player-server/internal/repository/repository.go +++ b/player-server/internal/repository/repository.go @@ -214,6 +214,12 @@ type MediaFilter struct { Sort string // Sort chooses the order: name, date, duration, play_count, or random. Limit int // Limit caps the number of returned rows. Offset int // Offset skips rows before returning results. + + // IncludeDeleted disables the implicit `deleted_at IS NULL` filter. + // Required for scanner dedup loads so soft-deleted rows are visible to + // the dedup map; without it a re-scan of a soft-deleted file would + // reinsert and hit the UNIQUE(set_id, rel_path) constraint. + IncludeDeleted bool } // MediaRepo manages media items. diff --git a/player-server/internal/scanner/scanner.go b/player-server/internal/scanner/scanner.go index 15a4949..9eb5b1e 100644 --- a/player-server/internal/scanner/scanner.go +++ b/player-server/internal/scanner/scanner.go @@ -144,9 +144,15 @@ func isPodcastRoot(rootPath string) bool { } // loadExistingMedia builds a lookup map of existing media keyed by relPath. +// IncludeDeleted = true so soft-deleted rows show up in the dedup map; if we +// omitted them, probeFile would treat the file as new and the writer would +// hit the UNIQUE(set_id, rel_path) constraint, failing the whole scan. func (s *FSScanner) loadExistingMedia(ctx context.Context, setID int64, setName string) (map[string]model.Media, error) { existing := make(map[string]model.Media) - mediaList, err := s.store.ListMedia(ctx, repository.MediaFilter{SetID: &setID}) + mediaList, err := s.store.ListMedia(ctx, repository.MediaFilter{ + SetID: &setID, + IncludeDeleted: true, + }) if err != nil { return nil, fmt.Errorf("list media for set %q: %w", setName, err) } @@ -156,6 +162,28 @@ func (s *FSScanner) loadExistingMedia(ctx context.Context, setID int64, setName return existing, nil } +// reconcileOrphans soft-deletes media rows whose underlying file is no +// longer present on disk. seenRel is the set of relPaths produced by the +// current scan; any active media row in existing whose key is NOT in +// seenRel had its file deleted between scans. Soft-deleted rows are left +// alone so the soft-delete state survives the rescan. +func (s *FSScanner) reconcileOrphans(ctx context.Context, existing map[string]model.Media, seenRel map[string]struct{}, setName string) { + for relPath, media := range existing { + if _, ok := seenRel[relPath]; ok { + continue + } + if media.DeletedAt != nil { + // Already soft-deleted; nothing to reconcile. + continue + } + if err := s.store.SoftDeleteMedia(ctx, media.ID); err != nil { + s.log().Warn("scanner orphan soft-delete failed", "set", setName, "rel_path", relPath, "id", media.ID, "err", err) + continue + } + s.log().Info("scanner soft-deleted orphan", "set", setName, "rel_path", relPath, "id", media.ID) + } +} + // gatherCoverImages walks the set and records the first cover image per directory. func (s *FSScanner) gatherCoverImages(setPath string) map[string]string { coverImages := make(map[string]string) @@ -346,6 +374,16 @@ func (s *FSScanner) scanSet(ctx context.Context, root, setPath string, progress return fmt.Errorf("scan set %q: %w", setName, err) } + // Build the set of relPaths we just saw on disk so reconcileOrphans + // can soft-delete media rows whose files disappeared between scans. + seenRel := make(map[string]struct{}, len(files)) + for _, p := range files { + if rel, relErr := filepath.Rel(setPath, p); relErr == nil { + seenRel[filepath.ToSlash(rel)] = struct{}{} + } + } + s.reconcileOrphans(ctx, existing, seenRel, setName) + if progress != nil { progress.AddFilesTotal(len(files)) } diff --git a/player-server/internal/service/tag.go b/player-server/internal/service/tag.go index c2a9a7a..b859b93 100644 --- a/player-server/internal/service/tag.go +++ b/player-server/internal/service/tag.go @@ -31,7 +31,11 @@ func (s *tagService) ListTags(ctx context.Context, userID int64) ([]model.Tag, e } func (s *tagService) AssignTag(ctx context.Context, mediaID, userID int64, tagName string) error { - if _, err := s.helper.verifyAccess(ctx, mediaID, userID); err != nil { + // Tags are global state: every other user with access to this media + // sees the change. Require owner-level access so a viewer cannot + // mutate shared metadata. Personal annotations (favorites, notes) + // stay on verifyAccess because they're per-user. + if _, err := s.helper.verifyModifyAccess(ctx, mediaID, userID); err != nil { return err } tag, err := s.store.GetTagByName(ctx, tagName) @@ -49,7 +53,11 @@ func (s *tagService) AssignTag(ctx context.Context, mediaID, userID int64, tagNa } func (s *tagService) RemoveTag(ctx context.Context, mediaID, userID int64, tagName string) error { - if _, err := s.helper.verifyAccess(ctx, mediaID, userID); err != nil { + // Tags are global state: every other user with access to this media + // sees the change. Require owner-level access so a viewer cannot + // mutate shared metadata. Personal annotations (favorites, notes) + // stay on verifyAccess because they're per-user. + if _, err := s.helper.verifyModifyAccess(ctx, mediaID, userID); err != nil { return err } tag, err := s.store.GetTagByName(ctx, tagName) |
