timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

Scan lifecycle honesty: 200-on-dropped-scan, non-finally unlocks, unguarded enqueue (reviewer#20 F1+F2+F6) #232

New Issue
Closed
opened 2026-07-11 00:55:14 +02:00 by timothy · 3 comments
timothy commented 2026-07-11 00:55:14 +02:00
Owner
Copy Link
Copy Source

From audit adversarial-reviewer#20, findings F1 (Blocker) + F2 (Blocker) + F6 (Medium) + F5.3. Builds on the ownership model from #231.

Defects

F1 — API lies about acceptance. QueueLibraryScanByLibraryIdHandler.cs:46-82: return true sits outside if (locker.LockLibrary(...)) → a held lock skips all enqueueing and still returns true → LibrariesController.cs:24-27 maps to 200 OK. Sync-disabled returns the same false as missing → both 404. Repro: POST /api/libraries/{id}/scan twice quickly → both 200, second did nothing. The SPA papers over this with PENDING_GRACE_TICKS (web/src/api/libraries.ts:31-34).

F2 — ScannerService strands locks on handler exception. Every unlock in ErsatzTV/Services/ScannerService.cs sits after await mediator.Send(...), not in finally (:129-132, :179-182, :242-245, :292-295, :369-372 libraries; :208-211, :321-324, :398-401 collections). A thrown exception jumps to the loop catch (:89-92) — the lock leaks until restart; scheduler then skips that library every cycle and the API keeps answering 200.

F6 — unguarded lock→enqueue window. Only TraktController.EnqueueWithTraktLock (TraktController.cs:170-192) guards the enqueue. QueueLibraryScanByLibraryIdHandler:46-79, CreateLocalLibraryHandler:45, UpdateLocalLibraryHandler:97, Libraries.razor:222-273, RemoteMediaSourceLibrariesEditor.razor:129, UpdateTraktListHandler:56-58, SchedulerService scan loops all lock then WriteAsync bare — a cancelled token between lock and write leaks; for the Plex/Jellyfin/Emby two-message batches, cancellation between the writes half-enqueues and the lock is never released.

F5.3 — batch early-unlock. Scheduler Plex Shows: one lock, two messages (SchedulerService.cs:230-241); ScannerService unlocks after the first → networks-sync runs unlocked.

Task

  • Handler returns an outcome enum (Queued | NotFound | SyncDisabled | AlreadyScanning); controller maps 202 / 404 / 422 / 409 (ProblemDetails for 409/422).
  • try/finally around each mediator.Send + unlock in all eight ScannerService sites.
  • Guard every lock→enqueue with catch-unlock (copy the EnqueueWithTraktLock pattern).
  • Make multi-message scan batches release-safe: composite message, or unlock-on-last with loss handling.
  • Same PR: remove the SPA grace-tick workaround (web/src/api/libraries.ts), OpenAPI regen (./scripts/update-openapi.sh + npm run generate:api), OpenApiErrorResponseContractTests cases for the new 404/409/422.

Required tests (evidence, not optional)

  • Throwing mediator inside ScannerService message processing → assert lock released.
  • Pre-cancelled token after LockLibrary in the queue handler → assert released.
  • Two concurrent scan POSTs (mocked locker) → one 202, one 409.
From audit [adversarial-reviewer#20](https://gitea.tblindustries.be/timothy/adversarial-reviewer/issues/20#issuecomment-7588), findings **F1 (Blocker) + F2 (Blocker) + F6 (Medium) + F5.3**. Builds on the ownership model from #231. ## Defects **F1 — API lies about acceptance.** `QueueLibraryScanByLibraryIdHandler.cs:46-82`: `return true` sits outside `if (locker.LockLibrary(...))` → a held lock skips all enqueueing and still returns `true` → `LibrariesController.cs:24-27` maps to **200 OK**. Sync-disabled returns the same `false` as missing → both **404**. Repro: POST `/api/libraries/{id}/scan` twice quickly → both 200, second did nothing. The SPA papers over this with `PENDING_GRACE_TICKS` (`web/src/api/libraries.ts:31-34`). **F2 — ScannerService strands locks on handler exception.** Every unlock in `ErsatzTV/Services/ScannerService.cs` sits after `await mediator.Send(...)`, not in `finally` (`:129-132, :179-182, :242-245, :292-295, :369-372` libraries; `:208-211, :321-324, :398-401` collections). A thrown exception jumps to the loop catch (`:89-92`) — the lock leaks until restart; scheduler then skips that library every cycle and the API keeps answering 200. **F6 — unguarded lock→enqueue window.** Only `TraktController.EnqueueWithTraktLock` (`TraktController.cs:170-192`) guards the enqueue. `QueueLibraryScanByLibraryIdHandler:46-79`, `CreateLocalLibraryHandler:45`, `UpdateLocalLibraryHandler:97`, `Libraries.razor:222-273`, `RemoteMediaSourceLibrariesEditor.razor:129`, `UpdateTraktListHandler:56-58`, `SchedulerService` scan loops all lock then `WriteAsync` bare — a cancelled token between lock and write leaks; for the Plex/Jellyfin/Emby two-message batches, cancellation between the writes half-enqueues and the lock is never released. **F5.3 — batch early-unlock.** Scheduler Plex Shows: one lock, two messages (`SchedulerService.cs:230-241`); `ScannerService` unlocks after the first → networks-sync runs unlocked. ## Task - Handler returns an outcome enum (`Queued | NotFound | SyncDisabled | AlreadyScanning`); controller maps **202 / 404 / 422 / 409** (ProblemDetails for 409/422). - `try/finally` around each `mediator.Send` + unlock in all eight ScannerService sites. - Guard every lock→enqueue with catch-unlock (copy the `EnqueueWithTraktLock` pattern). - Make multi-message scan batches release-safe: composite message, or unlock-on-last with loss handling. - **Same PR**: remove the SPA grace-tick workaround (`web/src/api/libraries.ts`), OpenAPI regen (`./scripts/update-openapi.sh` + `npm run generate:api`), `OpenApiErrorResponseContractTests` cases for the new 404/409/422. ## Required tests (evidence, not optional) - Throwing mediator inside ScannerService message processing → assert lock released. - Pre-cancelled token after `LockLibrary` in the queue handler → assert released. - Two concurrent scan POSTs (mocked locker) → one 202, one 409.
timothy added the bugapipriority: highreview labels 2026-07-11 00:55:14 +02:00
timothy added the in-progress label 2026-07-11 12:08:34 +02:00
timothy commented 2026-07-11 12:08:37 +02:00
Author
Owner
Copy Link
Copy Source

claiming — session started 2026-07-11 (same session as #231; this lands on top of #231's ownership model).

claiming — session started 2026-07-11 (same session as #231; this lands on top of #231's ownership model).
timothy commented 2026-07-11 12:18:48 +02:00
Author
Owner
Copy Link
Copy Source

Working this as one PR with #231 (batch/231-232-lock-soundness). The ownership-model contract #232 implements is decided in #231's plan comment: tokenless atomic flag + single-owner release discipline (a true from Lock* = ownership of exactly one release; compensating unlock on enqueue failure per the EnqueueWithTraktLock pattern). #232 scope: outcome enum → 202/404/422/409, try/finally unlocks in all 8 ScannerService sites, guarded enqueues, batch release-safety, SPA PENDING_GRACE_TICKS removal, OpenAPI regen + contract tests.

Working this as one PR with #231 (`batch/231-232-lock-soundness`). The ownership-model contract #232 implements is decided in [#231's plan comment](http://192.168.1.95:3000/timothy/ersatztv/issues/231): tokenless atomic flag + single-owner release discipline (a `true` from `Lock*` = ownership of exactly one release; compensating unlock on enqueue failure per the `EnqueueWithTraktLock` pattern). #232 scope: outcome enum → 202/404/422/409, `try/finally` unlocks in all 8 ScannerService sites, guarded enqueues, batch release-safety, SPA `PENDING_GRACE_TICKS` removal, OpenAPI regen + contract tests.
timothy commented 2026-07-11 13:06:19 +02:00
Author
Owner
Copy Link
Copy Source

Closed via PR #241 (merged to main c42928e8)

What was done

  • Honest outcome (F1): QueueLibraryScanByLibraryIdHandler returns QueueLibraryScanResult { Queued | NotFound | SyncDisabled | AlreadyScanning } instead of a bool that returned true even when a held lock dropped the scan. LibrariesController maps → 202 / 404 / 422 / 409 (ProblemDetails on errors), with [ProducesResponseType] for each.
  • No stranded locks (F2): all 8 ScannerService unlock sites moved into try/finally; compensating unlock-on-enqueue-failure added to the lock→enqueue producers (queue handler, Create/UpdateLocalLibrary, UpdateTraktList) per the EnqueueWithTraktLock pattern.
  • Unguarded enqueue window (F6): the producers above now release the lock if the WriteAsync throws (cancellation) between lock and enqueue.
  • Batch release-safety (F5.3): the Plex-Shows one-lock/two-message batch uses a tail-token (Unlock flag; only the trailing SynchronizePlexNetworks releases), so the lock now spans both units.
  • SPA: the lying-200 workaround is gone; triggerScan handles 202/409/404/422 honestly.

Root cause — the handler's return true sat unconditionally inside the library-exists branch, collapsing not-found / sync-disabled / already-scanning / queued into a single truthy result; and every ScannerService unlock sat after await mediator.Send rather than in finally, so a handler exception stranded the lock until restart. The SPA PENDING_GRACE_TICKS existed only to paper over the resulting dishonest 200.

Files: QueueLibraryScanByLibraryId(.cs/Handler.cs), Create/UpdateLocalLibraryHandler.cs, UpdateTraktListHandler.cs, Plex/Commands/{SynchronizePlexLibraryById,SynchronizePlexNetworks}.cs, LibrariesController.cs, ScannerService.cs, SchedulerService.cs, web/src/api/libraries.ts, generated v1.json, docs api-conventions.md §3b; tests QueueLibraryScanByLibraryIdHandlerTests.cs, LibrariesControllerTests.cs, OpenApiErrorResponseContractTests.cs, web/src/App.test.tsx.

Adversarial review (fork + Codex) found 3 issues, all fixed in-PR (commit 702b0121):

  1. Removing the SPA grace-tick wholesale reintroduced a stuck scan button (a 202'd scan finishing between polls never observed active) → restored a bounded grace net, honestly re-scoped for the queue→active lag + fast-completion race (not the lying-200 compensation).
  2. 409 was clearing pending + reconciling, re-enabling the button while the lock was still held (scan-status lag) → 409 now keeps pending, no toast.
  3. Scheduler scan enqueues (Plex-Shows tail-token + local/Jellyfin/Emby) lacked a compensating unlock → wrapped each in try/catch → UnlockLibrary → rethrow.

Deferred: Blazor scan producers (Libraries.razor, RemoteMediaSourceLibrariesEditor.razor) share the leak shape but are being retired in #91 phase (b) — flagged, not expanded into scope. Docs updated: api-conventions.md §3b (outcome-enum→status mapping + compensating-unlock rule), OpenAPI regenerated.

Verification: solution 0 errors · ErsatzTV.Tests 1051 · ErsatzTV.Infrastructure.Tests 53 · web 581 · typecheck + check:api clean · CI green on 702b0121.

## Closed via PR #241 (merged to main `c42928e8`) **What was done** - **Honest outcome** (F1): `QueueLibraryScanByLibraryIdHandler` returns `QueueLibraryScanResult { Queued | NotFound | SyncDisabled | AlreadyScanning }` instead of a bool that returned `true` even when a held lock dropped the scan. `LibrariesController` maps → **202 / 404 / 422 / 409** (ProblemDetails on errors), with `[ProducesResponseType]` for each. - **No stranded locks** (F2): all 8 `ScannerService` unlock sites moved into `try/finally`; compensating unlock-on-enqueue-failure added to the lock→enqueue producers (queue handler, Create/UpdateLocalLibrary, UpdateTraktList) per the `EnqueueWithTraktLock` pattern. - **Unguarded enqueue window** (F6): the producers above now release the lock if the `WriteAsync` throws (cancellation) between lock and enqueue. - **Batch release-safety** (F5.3): the Plex-Shows one-lock/two-message batch uses a tail-token (`Unlock` flag; only the trailing `SynchronizePlexNetworks` releases), so the lock now spans both units. - **SPA**: the lying-200 workaround is gone; `triggerScan` handles 202/409/404/422 honestly. **Root cause** — the handler's `return true` sat unconditionally inside the library-exists branch, collapsing not-found / sync-disabled / already-scanning / queued into a single truthy result; and every `ScannerService` unlock sat *after* `await mediator.Send` rather than in `finally`, so a handler exception stranded the lock until restart. The SPA `PENDING_GRACE_TICKS` existed only to paper over the resulting dishonest 200. **Files**: `QueueLibraryScanByLibraryId(.cs/Handler.cs)`, `Create/UpdateLocalLibraryHandler.cs`, `UpdateTraktListHandler.cs`, `Plex/Commands/{SynchronizePlexLibraryById,SynchronizePlexNetworks}.cs`, `LibrariesController.cs`, `ScannerService.cs`, `SchedulerService.cs`, `web/src/api/libraries.ts`, generated `v1.json`, docs `api-conventions.md` §3b; tests `QueueLibraryScanByLibraryIdHandlerTests.cs`, `LibrariesControllerTests.cs`, `OpenApiErrorResponseContractTests.cs`, `web/src/App.test.tsx`. **Adversarial review (fork + Codex) found 3 issues, all fixed in-PR** (commit `702b0121`): 1. Removing the SPA grace-tick wholesale reintroduced a stuck scan button (a 202'd scan finishing between polls never observed active) → restored a **bounded** grace net, honestly re-scoped for the queue→active lag + fast-completion race (not the lying-200 compensation). 2. 409 was clearing pending + reconciling, re-enabling the button while the lock was still held (scan-status lag) → 409 now keeps pending, no toast. 3. Scheduler scan enqueues (Plex-Shows tail-token + local/Jellyfin/Emby) lacked a compensating unlock → wrapped each in `try/catch → UnlockLibrary → rethrow`. **Deferred**: Blazor scan producers (`Libraries.razor`, `RemoteMediaSourceLibrariesEditor.razor`) share the leak shape but are being retired in #91 phase (b) — flagged, not expanded into scope. **Docs updated**: `api-conventions.md` §3b (outcome-enum→status mapping + compensating-unlock rule), OpenAPI regenerated. **Verification**: solution 0 errors · `ErsatzTV.Tests` 1051 · `ErsatzTV.Infrastructure.Tests` 53 · web 581 · typecheck + `check:api` clean · CI green on `702b0121`.
timothy removed the in-progress label 2026-07-11 13:06:22 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels api bug priority: high review
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#232