From 53a599e763eb8f105dd12c8e0a2a50e2a850e4d6 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Tue, 19 May 2026 14:18:43 +0300 Subject: Fix four defects flagged by S19/S20/S24; tighten scenarios MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ("-") 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 --- player-server/internal/api/handlers.go | 14 ++++ player-server/internal/repository/media.go | 4 +- player-server/internal/repository/repository.go | 6 ++ player-server/internal/scanner/scanner.go | 40 ++++++++- player-server/internal/service/tag.go | 12 ++- .../e2e-llm/scenarios/S19-permissions-matrix.md | 37 ++++----- .../test/e2e-llm/scenarios/S20-range-head.md | 24 +++--- .../e2e-llm/scenarios/S24-soft-delete-rescan.md | 97 +++++++++++----------- 8 files changed, 150 insertions(+), 84 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) diff --git a/player-server/test/e2e-llm/scenarios/S19-permissions-matrix.md b/player-server/test/e2e-llm/scenarios/S19-permissions-matrix.md index 95c8ca7..5b756d2 100644 --- a/player-server/test/e2e-llm/scenarios/S19-permissions-matrix.md +++ b/player-server/test/e2e-llm/scenarios/S19-permissions-matrix.md @@ -23,14 +23,15 @@ set, and the scenario then asserts visibility, read access and modify access from each user's perspective. Mark anything that deviates from the assertions below as a real authorization defect. -Note on viewer write surface: the codebase routes tag, favorite and note -mutations through `verifyAccess` (NOT `verifyModifyAccess`). That means a -viewer is currently permitted to add tags, favorites and notes on media they -can see, even though `model.RoleViewer` is documented as "browsing and -playback" only. The steps below capture the **actual** behaviour (200 for -tag-add by a viewer). If this scenario later starts returning 403 there, the -service has been tightened on purpose — update the step. If it ever flips -back to 200 after a deliberate tightening, that is a regression. +Note on viewer write surface: tag mutations go through +`verifyModifyAccess` (added with this scenario), so a viewer is forbidden +from adding or removing tags — tags are global state visible to every +other user with access to the media. Favorites and notes stay on +`verifyAccess` because they're per-user data (`favorites.user_id`, +`media_notes.user_id`) and don't affect what anyone else sees. Step 15 +asserts the 403 for tags; steps 16 and 17 assert the 200 for personal +favorites and notes. A viewer-tag-add returning 200 (or a viewer-favorite +returning 403) would be a regression. --- @@ -129,14 +130,14 @@ back to 200 after a deliberate tightening, that is a regression. ## D) U1 — viewer write surface (verify actual behaviour) -15. As `U1`, add a tag to a media item in `SET_A`: call +15. As `U1`, attempt to add a tag to a media item in `SET_A`: call `POST /api/v1/media/{MEDIA_A_ID}/tags` with `U1_COOKIE`, `Content-Type: application/json` and body `{"tag": "e2e-perm-test"}`. - Expected behaviour (per `tagService.AssignTag` which calls - `verifyAccess`, not `verifyModifyAccess`): HTTP 200. If the response is - 403, the service has been tightened to require owner role for tag - mutations — note this in the run output. If it is 200, leave the tag in - place for now; step 21 cleans it up. + Confirm the response is HTTP 403. Reasoning: tags are global shared + state (every viewer of this media sees the change), so + `tagService.AssignTag` calls `verifyModifyAccess`, which requires + `RoleOwner` (or admin) — `U1` is only a viewer. A 200 here would be a + regression of the tightening landed alongside this scenario. 16. As `U1`, mark the media item as a favorite: call `POST /api/v1/media/{MEDIA_A_ID}/favorite` with `U1_COOKIE` and an @@ -186,11 +187,9 @@ back to 200 after a deliberate tightening, that is a regression. `POST /api/v1/media/{MEDIA_B_ID}/restore` with `ADMIN_COOKIE`. Confirm the response is HTTP 200. -24. As admin, remove the tag added by `U1` in step 15 (only if step 15 - returned HTTP 200): call - `DELETE /api/v1/media/{MEDIA_A_ID}/tags/e2e-perm-test` with - `ADMIN_COOKIE`. Confirm the response is HTTP 200. If step 15 returned - 403 (viewer not allowed to tag), skip this step. +24. No tag cleanup needed: step 15 now returns 403, so no tag was created. + (Earlier versions of this scenario tolerated 200 here and cleaned up; + keep the slot to preserve step numbering across runs and history.) 25. As admin, revoke `U1`'s grant on `SET_A`: call `DELETE /api/v1/admin/permissions` with `ADMIN_COOKIE` and body diff --git a/player-server/test/e2e-llm/scenarios/S20-range-head.md b/player-server/test/e2e-llm/scenarios/S20-range-head.md index e9364f1..d4354f6 100644 --- a/player-server/test/e2e-llm/scenarios/S20-range-head.md +++ b/player-server/test/e2e-llm/scenarios/S20-range-head.md @@ -50,8 +50,9 @@ the Go standard library: no digits) is treated as "no Range" — the server returns the full body with status 200, NOT 416. This matches RFC 7233 §3.1. - `If-Modified-Since` matching the `Last-Modified` returns 304. The handler - does NOT set an explicit `ETag`, so `If-None-Match` cannot match and the - full body is returned with 200. + also emits a strong `ETag` header derived from file size and mtime, so + `If-None-Match` matching that ETag returns 304 too. Both conditional + paths must work — they're the basis of iOS/podcast-client revalidation. If any of those server-side facts have changed when this scenario runs, flag it: every one of them affects iOS / podcast-app playback. @@ -166,15 +167,16 @@ flag it: every one of them affects iOS / podcast-app playback. is empty. This proves the conditional-GET path through `http.ServeContent` works. -14. GET stream with `If-None-Match`: issue - `GET /api/v1/media/{media_id}/stream` with the session cookie and an - additional `If-None-Match: "any-tag"` request header. The handler - does NOT set an explicit `ETag` response header, so this conditional - cannot match — confirm the response is HTTP 200 and the - `Content-Length` response header equals the full `file_size`. Also - confirm the response has no `ETag` response header. If a future - change adds ETag support, this step should be updated to assert 304 - when the client sends back the server-emitted ETag. +14. GET stream with `If-None-Match`: first issue a plain + `GET /api/v1/media/{media_id}/stream` (or HEAD) with the session + cookie, read the `ETag` response header, and confirm it is a + quoted string of the form `"-"`. Then re-issue + `GET /api/v1/media/{media_id}/stream` with the session cookie and + `If-None-Match: ` and confirm the response is HTTP 304 + (Not Modified) with an empty body. A response missing the `ETag` + header or returning 200 with a body to the matching `If-None-Match` + request is a regression of the ETag support added with this + scenario. 15. Negative case — HEAD on a nonexistent media id: issue `HEAD /api/v1/media/999999999/stream` with the session cookie. diff --git a/player-server/test/e2e-llm/scenarios/S24-soft-delete-rescan.md b/player-server/test/e2e-llm/scenarios/S24-soft-delete-rescan.md index 1936f5c..66ecf0b 100644 --- a/player-server/test/e2e-llm/scenarios/S24-soft-delete-rescan.md +++ b/player-server/test/e2e-llm/scenarios/S24-soft-delete-rescan.md @@ -20,18 +20,24 @@ would be a surprising, silent "undelete". A secondary check probes what happens when the underlying file is removed from disk while a non-deleted media row points to it. -Reading `internal/scanner/scanner.go` shows that -`FSScanner.loadExistingMedia` uses `repository.ScannerStore.ListMedia` to -build its `existing` map, and `repository/media.go` always appends -`media.deleted_at IS NULL` to the `ListMedia` predicate. That means -soft-deleted rows are invisible to the scanner's dedup map, so a re-walk will -try to `CreateMedia` for the same `(set_id, rel_path)` pair — which the -schema constrains with `UNIQUE(set_id, rel_path)` (see -`internal/repository/schema.go`). The likely observable outcomes are -therefore: rescan fails with a UNIQUE constraint error and the soft-delete -remains (b with a noisy side-effect on `last_error`), or — if the harness -ever changes to upsert — the row is silently resurrected (a, a defect). -The scenario asserts (b) and treats (a) as a failure. +Two real defects in the scanner used to be locked in by earlier drafts of +this scenario; both are now fixed and asserted as regressions: + +1. **Soft-deleted rows tripped the rescan.** `FSScanner.loadExistingMedia` + used a `ListMedia` call that filtered `deleted_at IS NULL`, so a + re-walk of a soft-deleted file tried to `CreateMedia` for the same + `(set_id, rel_path)` pair and hit the schema's `UNIQUE` constraint. + `last_error` was set and the scan reported failure. Fixed by adding + `MediaFilter.IncludeDeleted` and using it in the scanner. + +2. **Files deleted from disk left orphan rows.** The scanner only walked + files that exist and never compared the resulting set against the DB, + so a removed file kept its row in `GET /api/v1/media` (where any + stream attempt would 404). Fixed by `reconcileOrphans` which soft- + deletes any active row whose `rel_path` was not seen during the walk. + +The scenario asserts both fixes (clean rescan in step 13; orphan +soft-delete in step 20). A regression in either fires this scenario. 1. Authenticate as an admin user: call `POST /api/v1/auth/login` with body `{"username": "admin", "password": "TestPassw0rd!"}`. Confirm the response @@ -99,22 +105,15 @@ The scenario asserts (b) and treats (a) as a failure. `running: false`. On each poll the response must be HTTP 200. Save the final JSON object as `final_progress` for the next step. -13. Inspect the scan outcome. Read `final_progress.last_error` (which may be - absent or empty when the scan succeeded). Record one of three observed - cases: - - **Case (a)** — `last_error` is empty/absent AND step 14 shows the - soft-deleted row resurfaced in `GET /api/v1/media`. This is a defect: - rescan silently undeleted media. Annotate task 89 with the observation - and fail the scenario at step 15. - - **Case (b-clean)** — `last_error` is empty/absent AND the soft-deleted - row stays out of `GET /api/v1/media`. This is the desired behaviour. - - **Case (b-noisy)** — `last_error` contains a UNIQUE constraint error - (text matching `UNIQUE constraint failed: media.set_id, media.rel_path` - or similar) AND the soft-deleted row stays out of - `GET /api/v1/media`. The soft-delete is preserved, but rescan reports a - failure caused by trashed entries. Annotate task 89 noting this as a - defect candidate (rescans should not fail because of soft-deleted - rows). +13. Inspect the scan outcome. Read `final_progress.last_error`: it MUST be + empty or absent — the scanner's dedup map now includes soft-deleted + rows (via `MediaFilter.IncludeDeleted`), so it skips re-inserting them + and never hits the `UNIQUE(set_id, rel_path)` constraint. A non-empty + `last_error` (especially text matching + `UNIQUE constraint failed: media.set_id, media.rel_path`) is a + regression of that fix and must fail the scenario. Combined with + step 14, only the clean case is acceptable: rescan succeeds AND the + soft-deleted row stays out of `GET /api/v1/media`. 14. Re-query the active list to verify the soft-delete persisted: call `GET /api/v1/media` with the `admin_session` cookie. Confirm the response @@ -151,28 +150,26 @@ The scenario asserts (b) and treats (a) as a failure. 19. Poll `GET /api/v1/admin/scan-progress` with the `admin_session` cookie every 1 s for up to 60 polls until `running: false`. -20. Verify what the rescan did to the orphaned row. Run - `db: SELECT count(*) FROM media WHERE id={media_id_2}` and record the - result. Reading `internal/scanner/scanner.go` shows the scanner only - walks files that exist and never reconciles disappeared files against - the DB, so the expected count is 1 (row unchanged). Also run - `db: SELECT deleted_at FROM media WHERE id={media_id_2}` and confirm - `deleted_at` is NULL — the scanner does NOT auto-soft-delete missing - files. If the count is 0 or `deleted_at` is non-NULL, that means the - scanner does prune orphans and the scenario should annotate task 89 with - the observed pruning behaviour, since it contradicts the current - implementation. - -21. Confirm the orphaned row still appears in `GET /api/v1/media`: call - `GET /api/v1/media` with the `admin_session` cookie and verify an entry - with `id == media_id_2` is present. (Streaming it would 404 because the - file is gone, but listing should not.) - -22. Cleanup — soft-delete then attempt hard cleanup of both rows. Call - `DELETE /api/v1/media/{media_id_2}` with the `admin_session` cookie and - confirm HTTP 200. The first test file at `{abs_path}` is still on disk; - remove it directly: run `rm -f {abs_path}`. Both DB rows now have - `deleted_at IS NOT NULL` and live in the trash. +20. Verify the rescan reconciled the orphaned row. The scanner now soft- + deletes media whose underlying file disappeared between scans (see + `reconcileOrphans` in `internal/scanner/scanner.go`). Run + `db: SELECT count(*) FROM media WHERE id={media_id_2}` and confirm the + count is exactly 1 — orphans are SOFT-deleted, not hard-deleted. Then + run `db: SELECT deleted_at FROM media WHERE id={media_id_2}` and + confirm `deleted_at` is NOT NULL (a recent timestamp). A NULL + `deleted_at` here means the orphan-reconcile pass failed to fire — + regression. + +21. Confirm the orphan no longer appears in the active media list: call + `GET /api/v1/media` with the `admin_session` cookie and verify NO + entry has `id == media_id_2`. It must instead appear in + `GET /api/v1/admin/trash` (alongside the row from step 7). + +22. Cleanup — `media_id_2` is already soft-deleted by the orphan-reconcile + pass in step 20, so no DELETE call is needed for it. Remove the first + test file from disk if it still exists: run `rm -f {abs_path}`. Both + rows now have `deleted_at IS NOT NULL` and live in the trash; subsequent + rescans will not re-import them because the dedup map sees them. 23. Revoke the API token: call `DELETE /api/v1/auth/tokens/{token_id}` with the `admin_session` cookie. Confirm the response is HTTP 200. -- cgit v1.2.3