Audit: async lock ownership and background-job acceptance in ErsatzTV #20

New Issue

2026-07-11T00:23:28+02:00

timothy commented

2026-07-11 00:23:28 +02:00

Context

Findings from adversarial-reviewer #18/#19 exposed a recurring temporal-ownership failure class: asynchronous work is safe only when the code makes clear which request/job owns state, completion, and cleanup.

ErsatzTV's library scanner shows evidence that the same class exists server-side:

QueueLibraryScanByLibraryIdHandler acquires LockLibrary and then performs one or more ChannelWriter.WriteAsync calls.
It returns true even when LockLibrary returns false, so the API can report success for work it did not accept.
Cancellation/failure between lock acquisition and successful enqueue can strand the lock.
ScannerService unlocks after mediator.Send; an exception is caught by the outer worker loop before the unlock is reached.
Scheduler-driven library scans and Trakt batches use similar acquire/enqueue/unlock-by-last-item patterns.

Relevant source:

ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs
ErsatzTV/Services/ScannerService.cs
ErsatzTV/Services/SchedulerService.cs
ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs
ErsatzTV/Controllers/Api/LibrariesController.cs

Audit scope

Inventory every IEntityLocker.Lock* call and trace:

Who owns the lock immediately after acquisition?
At what exact point is ownership transferred to queued work?
What releases it on validation failure, queue rejection, cancellation, handler exception, worker shutdown, or partial multi-message enqueue?
Can unrelated work unlock a lock it did not acquire?
Does the API distinguish missing, disabled, unsupported, already-running, accepted, and completed/failed outcomes?
Is active/terminal job state observable without client-side timing guesses?

Include library scans, external collection sync, remote-media-source operations, Trakt refresh/match, playout builds, subtitle extraction, and troubleshooting playback.

Evidence required

Lock lifecycle matrix covering every Lock* call.
Failure-injection tests for cancellation, queue-write failure, handler exception, and partial multi-message enqueue.
Concurrent-request tests proving one request is accepted and the other receives an honest conflict outcome.
Proof that every acquired lock is released exactly once on all terminal paths.
API contract recommendation: normally 202 accepted, 404 missing, 409 already running, and 422 disabled/unsupported, unless the audit justifies another mapping.
Implementer prompt separating immediate defects from any larger durable-job-status design.

Relationship to existing work

Coordinate with ErsatzTV #202 for media-source lifecycle changes.
Coordinate with ErsatzTV #215 for playout mutation gating.
Do not fold this into ErsatzTV #197: #197 covers API contract/security broadly, while this audit owns asynchronous lock and job lifecycle semantics.

Deliverable

Post a severity-ranked finding set, concrete reproduction paths, proposed issue split, and rollback/test requirements.

## Context Findings from adversarial-reviewer #18/#19 exposed a recurring temporal-ownership failure class: asynchronous work is safe only when the code makes clear which request/job owns state, completion, and cleanup. ErsatzTV's library scanner shows evidence that the same class exists server-side: - `QueueLibraryScanByLibraryIdHandler` acquires `LockLibrary` and then performs one or more `ChannelWriter.WriteAsync` calls. - It returns `true` even when `LockLibrary` returns false, so the API can report success for work it did not accept. - Cancellation/failure between lock acquisition and successful enqueue can strand the lock. - `ScannerService` unlocks after `mediator.Send`; an exception is caught by the outer worker loop before the unlock is reached. - Scheduler-driven library scans and Trakt batches use similar acquire/enqueue/unlock-by-last-item patterns. Relevant source: - `ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs` - `ErsatzTV/Services/ScannerService.cs` - `ErsatzTV/Services/SchedulerService.cs` - `ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs` - `ErsatzTV/Controllers/Api/LibrariesController.cs` ## Audit scope Inventory every `IEntityLocker.Lock*` call and trace: 1. Who owns the lock immediately after acquisition? 2. At what exact point is ownership transferred to queued work? 3. What releases it on validation failure, queue rejection, cancellation, handler exception, worker shutdown, or partial multi-message enqueue? 4. Can unrelated work unlock a lock it did not acquire? 5. Does the API distinguish missing, disabled, unsupported, already-running, accepted, and completed/failed outcomes? 6. Is active/terminal job state observable without client-side timing guesses? Include library scans, external collection sync, remote-media-source operations, Trakt refresh/match, playout builds, subtitle extraction, and troubleshooting playback. ## Evidence required - Lock lifecycle matrix covering every `Lock*` call. - Failure-injection tests for cancellation, queue-write failure, handler exception, and partial multi-message enqueue. - Concurrent-request tests proving one request is accepted and the other receives an honest conflict outcome. - Proof that every acquired lock is released exactly once on all terminal paths. - API contract recommendation: normally `202` accepted, `404` missing, `409` already running, and `422` disabled/unsupported, unless the audit justifies another mapping. - Implementer prompt separating immediate defects from any larger durable-job-status design. ## Relationship to existing work - Coordinate with ErsatzTV #202 for media-source lifecycle changes. - Coordinate with ErsatzTV #215 for playout mutation gating. - Do not fold this into ErsatzTV #197: #197 covers API contract/security broadly, while this audit owns asynchronous lock and job lifecycle semantics. ## Deliverable Post a severity-ranked finding set, concrete reproduction paths, proposed issue split, and rollback/test requirements.

timothy added the audit ersatztv labels 2026-07-11 00:23:28 +02:00

timothy commented

2026-07-11 00:40:03 +02:00

Picking this up (main session, ersatztv checkout, read-only audit — no worktree collision with #18/#19 agents). Approach: full inventory of IEntityLocker call sites + lifecycle trace per the audit scope, then severity-ranked findings, lock lifecycle matrix, API contract recommendation, proposed issue split, and an implementer prompt posted here.

Picking this up (main session, ersatztv checkout, read-only audit — no worktree collision with #18/#19 agents). Approach: full inventory of `IEntityLocker` call sites + lifecycle trace per the audit scope, then severity-ranked findings, lock lifecycle matrix, API contract recommendation, proposed issue split, and an implementer prompt posted here.

timothy commented

2026-07-11 00:48:13 +02:00

Audit: async lock ownership & background-job acceptance — findings

Audited at main @ ef8915f1 (2026-07-11). All file:line refs verified against source, not inferred. Every IEntityLocker call site in the main tree was inventoried (worktree copies and test mocks excluded).

Architecture facts the findings rest on

EntityLocker is a singleton (Startup.cs:836). Library/playout/remote-media-source locks are ConcurrentDictionary; the other six kinds (Plex, Trakt, Emby/Jellyfin/Plex collections, troubleshooting playback) are plain non-volatile bool fields with non-atomic check-then-set.
All seven work channels are unbounded, single-reader (Startup.cs:1014-1020) — writes never block; they only throw on a cancelled token. Nothing drains or completes channels at shutdown.
The dominant pattern is split lock ownership: a producer (handler / Blazor page / scheduler) acquires the lock, enqueues one or more messages, and a consumer (ScannerService / worker handler) releases it after processing. Locks are in-memory, so every leak below persists until process restart.
Locks are not owned, counted, or re-entrant: any code can release any lock (Unlock* takes no token), and several sites ignore the Lock* return value.

Blockers

F1 — `POST /api/libraries/{id}/scan` reports success for work it silently dropped

QueueLibraryScanByLibraryIdHandler.cs:46-82: return true sits outside if (locker.LockLibrary(...)), so a held lock skips all enqueueing and still returns true → LibrariesController.cs:24-27 maps it to 200 OK. A sync-disabled library returns the same false as a missing id → both surface as 404.
Repro: POST /api/libraries/{id}/scan twice in quick succession → both 200; the second queued nothing. Disable ShouldSyncItems on a Plex library → scan returns 404 as if the library doesn't exist.
Evidence it already bites: the SPA carries a workaround — web/src/api/libraries.ts:31-34 holds a triggered scan "pending" for PENDING_GRACE_TICKS = 3 polls and silently drops it if scan-status never shows it ("the scanner never picked it up / it failed silently").
Fix shape: handler returns an outcome enum (Queued | NotFound | SyncDisabled | AlreadyScanning), controller maps 202 / 404 / 422 / 409; delete the SPA grace-tick heuristic in the same PR.

F2 — ScannerService strands library/collection locks on any handler exception

Every unlock in ScannerService.cs sits after await mediator.Send(...) and is not in a finally (:129-132 local, :179-182 Plex, :242-245 Plex networks, :292-295 Jellyfin, :369-372 Emby, :208-211/:321-324/:398-401 collections). A thrown exception (DB unavailable, scanner-process fault — anything not returned as Either.Left) jumps to the loop's catch (:89-92), which logs and continues — the unlock is never reached.
Consequence: that library is permanently locked — scheduler skips it every cycle (SchedulerService LockLibrary returns false → no enqueue), Blazor scan buttons stay disabled, and per F1 the API keeps answering 200 while doing nothing. Same for collections locks.
Repro: NSubstitute mediator that throws from SynchronizePlexLibraryByIdHandler; or stop the DB mid-scan. Assert IsLibraryLocked afterwards.
Fix shape: try/finally per message around the Send, unlock in the finally.

Issues

F3 — Troubleshooting playback lock leaks on "media not on disk"; lock conflict surfaces as 404 (High)

PrepareTroubleshootingPlaybackHandler.GetProcess: locks at :171, then if (string.IsNullOrEmpty(mediaPath)) return BaseError.New(...) at :182-186 — a Left return, not an exception, so the catch at :142-150 (which does unlock) never fires and nothing ever releases the lock. GET /api/troubleshoot/playback/status then reports "running" forever and every subsequent troubleshoot attempt fails with "Troubleshooting playback is locked" — which TroubleshootController.cs:113 maps to a bare 404, indistinguishable from a bad id.
Repro: troubleshoot any media item whose file has been deleted/unmounted → troubleshooting permanently unavailable until restart.
Secondary: the controller enqueues StartTroubleshootingPlayback (whose handler's finally at StartTroubleshootingPlaybackHandler.cs:203 is the normal release) only after Prepare succeeds — a cancelled request in that window also leaks. And the two lock sites (:67-72, :166-171) are check-then-set on a plain bool → two concurrent Prepare requests can both pass (see F5).

F4 — Subtitle extraction leaks every playout lock on cancellation or exception (High)

ExtractEmbeddedSubtitlesHandler.cs: locks all checked playouts (:125-128); the unlock loop (:183-186) is skipped by (a) the cancellation early-return at :171-174, (b) the catch (TaskCanceled/OperationCanceled) at :188-191 that swallows and returns, and (c) any other exception (no finally). This runs after every playout build and on the hourly schedule, so shutdown/cancellation mid-extraction is routine.
Consequence: leaked playout locks make ResetAllPlayoutsHandler.cs:25,36 silently skip those playouts on every future reset, and disable all Blazor playout actions. Partially masked by F5: the next BuildPlayout for that playout unconditionally unlocks it in its finally — i.e. one bug papers over another.

F5 — The lock primitive itself is unsound: races, no ownership, cross-release (High, class-level)

Non-atomic bool locks: two concurrent LockTrakt() / LockTroubleshootingPlayback() / Lock*Collections() / LockPlex() calls (HTTP threads + scheduler + Blazor circuits genuinely race) can both return true — the mutual exclusion these exist for is not guaranteed. EntityLocker.cs:54-64,110-120,136-146,162-172,188-198,238-248.
No ownership: BuildPlayoutHandler.cs:67 ignores LockPlayout's return value and unconditionally unlocks in finally (:127-130) — it releases whoever's lock is present (e.g. one held by ExtractEmbeddedSubtitlesHandler, or any direct-Send caller — relevant to in-flight ersatztv#215).
Concrete cross-release: scheduler Plex Shows path takes one library lock and enqueues two messages (SchedulerService.cs:230-241: SynchronizePlexLibraryByIdIfNeeded + SynchronizePlexNetworks). ScannerService unlocks after the first completes (:179-182), so networks-sync runs unlocked; if a user queues a manual scan in that window, networks-sync's completion (:242-245) releases the manual scan's lock → UI shows idle while a scan is queued/running, and a second concurrent scan of the same library becomes possible.

F6 — Unguarded lock→enqueue window; partial multi-message enqueue (Medium)

Only TraktController.EnqueueWithTraktLock (TraktController.cs:170-192) guards the enqueue (catch { UnlockTrakt(); throw; }). Every other producer — QueueLibraryScanByLibraryIdHandler:46-79, CreateLocalLibraryHandler:45, UpdateLocalLibraryHandler:97, Libraries.razor:222-273, RemoteMediaSourceLibrariesEditor.razor:129, UpdateTraktListHandler:56-58, SchedulerService scan loops — locks then WriteAsyncs bare. A cancelled token (client abort) between lock and write leaks the lock; for the Plex/Jellyfin/Emby two-message batches, cancellation between the writes half-enqueues: the source-level sync runs but the library-scan message that would release the lock never exists → permanent leak.

F7 — Trakt unlock-by-last-item is fragile by construction (Medium-low)

Scheduler batches take one global Trakt lock and thread Unlock = (list == last) through the messages (SchedulerService.cs:319-328, 343-352); only the last message's handler releases (AddTraktListHandler.cs:44-50, MatchTraktListItemsHandler.cs:56-62). If the last message is never processed — WorkerService's shutdown break (WorkerService.cs:42-45) with the process staying up in a graceful-shutdown window, or any future channel-drop — the global lock leaks and all Trakt operations 409 until restart. Handler exceptions are safe (unlock is in finally); message loss is not. (MatchTraktListItems defaults Unlock = true, so the UpdateTraktListHandler single-shot path is correct.)

F8 — Accepted-async semantics are inconsistent across the API; playout builds are unobservable (Medium)

Endpoint	Accepted	Lock conflict	Missing	Disabled/unsupported
`POST /api/libraries/{id}/scan`	200	200 silent no-op	404	404
`POST /api/libraries/{id}/scan-show`	200	400 (conflated msg)	400 (conflated)	400 (conflated)
`POST /api/trakt/*`	202	409	404	422 (bad URL)
`POST /api/channels/{n}/playout/reset`	200	no check (queues behind lock)	404	—
`POST /api/playouts/reset-all`	202	silent skip of locked + ExternalJson	—	—
`POST /api/maintenance/clean_artwork`	200 fire-and-forget	—	—	—
`POST /api/maintenance/empty_trash`	200	—	—	500 text/plain
`GET /api/troubleshoot/playback.m3u8`	long-block → 302	404	404	404

Observability: scans have GET /api/libraries/scan-status (in-memory ScannerProxyService registry, with progress), Trakt has GET /api/trakt/status (busy bool), troubleshooting has a status endpoint (state/exitCode/log tail). Playout builds have nothing — IsPlayoutLocked is not exposed over HTTP; the SPA fires reset and does a single blind refresh (web/src/App.tsx:3064-3109), with only the after-the-fact warnings-count badge.

Suggestions

F9 — Parity + hygiene notes (Low)

Deep scan is not reachable via the API: QueueLibraryScanByLibraryIdHandler hardcodes ForceSynchronize*ById(library.Id, false); Blazor Libraries.razor passes deepScan: true. External-collections scan (Synchronize*Collections) likewise has no API trigger. Both are #91 phase (b) gates for deleting the Blazor Libraries page — add to the gate list.
ResetAllPlayoutsHandler should report skipped playouts (count/ids) instead of silently dropping them.
Unbounded channels + no shutdown drain: acceptable today (locks are in-memory and die with the process), but worth a one-line decision record so nobody "fixes" it into a persistence trap.

Lock lifecycle matrix

#	Lock	Acquired by	Released by	Release on failure?	Verdict
1	Library	`QueueLibraryScanByLibraryIdHandler:46`	`ScannerService` after scan	✗ not-finally (F2); ✗ enqueue window (F6); lies on conflict (F1)	LEAKS
2	Library	`QueueShowScanByLibraryIdHandler:49`	same handler `finally:102`	✓ try/finally	OK (the model to copy)
3	Library	`Create/UpdateLocalLibraryHandler:45/97`	`ScannerService`	✗ F2/F6	LEAKS
4	Library	`SchedulerService:212/230/264/291`	`ScannerService`	✗ F2; Plex Shows 2-msg batch unlocks after 1st (F5.3)	LEAKS + cross-release
5	Library	`Libraries.razor:222`, `RemoteMediaSourceLibrariesEditor.razor:129`	`ScannerService`	✗ F2/F6	LEAKS
6	Collections ×3	`Libraries.razor:256/263/270`	`ScannerService`	✗ F2; bool race (F5.1)	LEAKS
7	Playout	`BuildPlayoutHandler:67` (return ignored)	same handler `finally:129`	✓ finally, but releases non-owned locks (F5.2)	cross-release
8	Playout ×N	`ExtractEmbeddedSubtitlesHandler:127`	same handler `:185`	✗ cancellation early-return + swallowing catch, no finally (F4)	LEAKS
9	Trakt	`TraktController:174`	worker handler `finally`	✓ guarded enqueue	OK
10	Trakt	`SchedulerService:319/343` (batch)	last message's handler	✗ lost-last-message (F7)	fragile
11	Trakt	`TraktLists.razor:130/138/177`, `UpdateTraktListHandler:56`	worker handler `finally`	dialog-cancel unlocks ✓; enqueue unguarded (F6)	mostly OK
12	Plex	`PlexMediaSources.razor:117/127`	`SignOutOfPlexHandler:34` / `SynchronizePlexMediaSourcesHandler:74`	error branch unlocks `:144`; consumer path not-finally	fragile
13	RemoteMediaSource	`RemoteMediaSources.razor:162`	`DisconnectEmby/JellyfinHandler:37`	✗ handler throw → leak	fragile
14	Troubleshooting	`PrepareTroubleshootingPlaybackHandler:72/171`	error paths `:110/121/144` or `StartTroubleshootingPlaybackHandler` `finally:203`	✗ `GetProcess` media-path return `:182-186` (F3); bool race (F5.1)	LEAKS

API contract recommendation

Adopt uniformly for queue-triggering endpoints (matches the issue's proposal; Trakt already implements it):

202 Accepted — work queued (change library scan / playout reset / clean-artwork from 200).
404 — entity missing (real pre-check, per the existing controller-pre-check convention).
409 ProblemDetails — lock already held (library scan, scan-show, troubleshoot; reset-all returns 202 + a body listing skipped playouts).
422 ProblemDetails — disabled (ShouldSyncItems=false) / unsupported (local-library show scan, ExternalJson reset).
Every 202 must have a pollable status surface. Scans and Trakt and troubleshooting have one; add playout build status (expose lock/PlayoutBuildStatus — coordinate with ersatztv#215, in flight right now).
empty_trash 500 text/plain → ProblemDetails.

Proposed issue split (ersatztv)

Lock-soundness core [PLAN-MODE] — make EntityLocker atomic (Interlocked/lock for the six bools) and decide the ownership model (token/count vs. documented single-owner + assert). Smallest primitive change that makes F5 impossible; everything else builds on it. Includes concurrency unit tests.
Scan lifecycle honesty — F1 + F2 + F6(scan paths) + F5.3: outcome enum from queue handlers, 202/404/409/422 mapping, finally unlocks in ScannerService, guarded enqueue, make multi-message batches release-safe (composite message or last-message-unlock with loss handling). Same PR: delete SPA grace-tick workaround, OpenAPI regen, contract tests.
Troubleshooting lock leak — F3: try/finally (or using-style scope) in GetProcess, guard the controller's enqueue, map lock-conflict to 409 instead of 404.
Subtitle-extraction / playout lock leak — F4 + F5.2: finally unlock, respect LockPlayout return. Must coordinate with #215 (fix-215 in flight — playout mutation gating touches the same call sites).
Async contract normalization + playout build observability — F8 + F9 (reset-all skip reporting, empty_trash ProblemDetails, deep-scan + collections-scan API parity for #91b). Explicitly not folded into #197; #197 should reference the resulting contract table.

Suggested order: 1 → 2 → 3/4 (parallel) → 5. Items 2–5 are each shippable independently once 1 lands.

Test & rollback requirements

Failure-injection (NUnit + NSubstitute, existing ErsatzTV.Tests harness): throwing mediator inside ScannerService message processing → assert lock released; pre-cancelled token after LockLibrary in QueueLibraryScanByLibraryIdHandler → assert released; cancellation mid-ExtractEmbeddedSubtitles → assert all playout locks released; GetProcess with empty media path → assert troubleshooting lock released.
Concurrency: parallel LockTrakt/LockTroubleshootingPlayback (Task.WhenAll ×N) → exactly one true. Two concurrent POST /api/libraries/{id}/scan (controller test, mocked locker) → one 202, one 409.
Exactly-once release: for each terminal path per matrix row, assert Unlock* called exactly once (ownership assert makes double-release loud instead of silent).
Contract: OpenApiErrorResponseContractTests cases for the new 404/409/422s; regenerate v1.json + endpoint index per api-conventions checklist.
Rollback: all changes are in-memory semantics — no DB migrations, no data risk. The status-code changes are SPA-breaking → each API-changing PR must update web/src consumers in the same PR (libraries.ts polling, TraktListsScreen untouched, playout reset paths). Nothing here needs a feature flag; revert = git revert.

Implementer prompt

Context: adversarial-reviewer#20 audit of ErsatzTV async lock ownership, conducted at main @ ef8915f1. Findings F1–F9 above; lifecycle matrix and file:line refs verified. Coordinate: ersatztv#215 (playout gating, IN FLIGHT), #202 (media-source lifecycle), #197 (broad API contract — do not fold this work into it). Read docs/api-conventions.md + docs/contributing.md first per repo rules.

Work order (file as five ersatztv issues per the split above, label review):

[PLAN-MODE] EntityLocker soundness: replace the six racy bool locks with atomic operations; pick and document an ownership model (decisions.md entry). Add concurrency unit tests (parallel lock → exactly one winner; unlock-by-non-owner behavior defined and tested).

Scan lifecycle: outcome enum from QueueLibraryScanByLibraryIdHandler (fix return true on lock-conflict at :82); controller 202/404/409/422; try/finally unlocks in all eight ScannerService sites; guard every lock→enqueue with catch-unlock (copy TraktController.EnqueueWithTraktLock); fix the scheduler Plex Shows two-message batch so the lock survives until the LAST message (or use one composite message). Same PR: remove PENDING_GRACE_TICKS workaround in web/src/api/libraries.ts, OpenAPI regen, contract tests.

Troubleshooting: finally-scope the lock in PrepareTroubleshootingPlaybackHandler.GetProcess (leak at :182-186); 409 for lock-conflict instead of 404; guard the StartTroubleshootingPlayback enqueue.

Playout locks: finally unlock in ExtractEmbeddedSubtitlesHandler (leak at :171-191); stop ignoring LockPlayout's return in BuildPlayoutHandler:67 per the ownership model from item 1. Rebase over #215's merge; do not duplicate its gating work.

Contract + observability: normalize per the recommendation table; expose playout build status over HTTP; reset-all reports skips; empty_trash → ProblemDetails; file the deep-scan + collections-scan API parity gaps against the #91b gate list.

Per-issue: failure-injection tests listed in the audit are REQUIRED evidence, not optional. Follow the repo's Task Completion Protocol (comment as you work, push, structured close, reply to reviewer).

Deliverable complete. Next step: /dispatch to file the five ersatztv issues, or fold items into the existing queue (#215 overlap noted). Leaving this issue open until dispatch.

# Audit: async lock ownership & background-job acceptance — findings Audited at `main @ ef8915f1` (2026-07-11). All file:line refs verified against source, not inferred. Every `IEntityLocker` call site in the main tree was inventoried (worktree copies and test mocks excluded). ## Architecture facts the findings rest on - `EntityLocker` is a **singleton** (`Startup.cs:836`). Library/playout/remote-media-source locks are `ConcurrentDictionary`; the other six kinds (Plex, Trakt, Emby/Jellyfin/Plex collections, troubleshooting playback) are **plain non-volatile `bool` fields with non-atomic check-then-set**. - All seven work channels are **unbounded, single-reader** (`Startup.cs:1014-1020`) — writes never block; they only throw on a cancelled token. Nothing drains or completes channels at shutdown. - The dominant pattern is **split lock ownership**: a producer (handler / Blazor page / scheduler) acquires the lock, enqueues one or more messages, and a *consumer* (ScannerService / worker handler) releases it after processing. Locks are in-memory, so every leak below persists **until process restart**. - Locks are not owned, counted, or re-entrant: any code can release any lock (`Unlock*` takes no token), and several sites ignore the `Lock*` return value. --- ## Blockers ### F1 — `POST /api/libraries/{id}/scan` reports success for work it silently dropped `QueueLibraryScanByLibraryIdHandler.cs:46-82`: `return true` sits **outside** `if (locker.LockLibrary(...))`, so a held lock skips all enqueueing and still returns `true` → `LibrariesController.cs:24-27` maps it to **200 OK**. A sync-disabled library returns the same `false` as a missing id → both surface as **404**. **Repro**: `POST /api/libraries/{id}/scan` twice in quick succession → both 200; the second queued nothing. Disable `ShouldSyncItems` on a Plex library → scan returns 404 as if the library doesn't exist. **Evidence it already bites**: the SPA carries a workaround — `web/src/api/libraries.ts:31-34` holds a triggered scan "pending" for `PENDING_GRACE_TICKS = 3` polls and silently drops it if scan-status never shows it ("the scanner never picked it up / it failed silently"). **Fix shape**: handler returns an outcome enum (`Queued | NotFound | SyncDisabled | AlreadyScanning`), controller maps 202 / 404 / 422 / 409; delete the SPA grace-tick heuristic in the same PR. ### F2 — ScannerService strands library/collection locks on any handler exception Every unlock in `ScannerService.cs` sits **after** `await mediator.Send(...)` and is **not in a `finally`** (`:129-132` local, `:179-182` Plex, `:242-245` Plex networks, `:292-295` Jellyfin, `:369-372` Emby, `:208-211`/`:321-324`/`:398-401` collections). A thrown exception (DB unavailable, scanner-process fault — anything not returned as `Either.Left`) jumps to the loop's catch (`:89-92`), which logs and continues — the unlock is never reached. **Consequence**: that library is permanently locked — scheduler skips it every cycle (`SchedulerService` `LockLibrary` returns false → no enqueue), Blazor scan buttons stay disabled, and per F1 the API keeps answering 200 while doing nothing. Same for collections locks. **Repro**: NSubstitute mediator that throws from `SynchronizePlexLibraryByIdHandler`; or stop the DB mid-scan. Assert `IsLibraryLocked` afterwards. **Fix shape**: `try/finally` per message around the `Send`, unlock in the `finally`. --- ## Issues ### F3 — Troubleshooting playback lock leaks on "media not on disk"; lock conflict surfaces as 404 (High) `PrepareTroubleshootingPlaybackHandler.GetProcess`: locks at `:171`, then `if (string.IsNullOrEmpty(mediaPath)) return BaseError.New(...)` at `:182-186` — a `Left` return, not an exception, so the `catch` at `:142-150` (which does unlock) never fires and **nothing ever releases the lock**. `GET /api/troubleshoot/playback/status` then reports `"running"` forever and every subsequent troubleshoot attempt fails with "Troubleshooting playback is locked" — which `TroubleshootController.cs:113` maps to a bare **404**, indistinguishable from a bad id. **Repro**: troubleshoot any media item whose file has been deleted/unmounted → troubleshooting permanently unavailable until restart. Secondary: the controller enqueues `StartTroubleshootingPlayback` (whose handler's `finally` at `StartTroubleshootingPlaybackHandler.cs:203` is the normal release) only after Prepare succeeds — a cancelled request in that window also leaks. And the two lock sites (`:67-72`, `:166-171`) are check-then-set on a plain bool → two concurrent Prepare requests can both pass (see F5). ### F4 — Subtitle extraction leaks every playout lock on cancellation or exception (High) `ExtractEmbeddedSubtitlesHandler.cs`: locks **all** checked playouts (`:125-128`); the unlock loop (`:183-186`) is skipped by (a) the cancellation early-return at `:171-174`, (b) the `catch (TaskCanceled/OperationCanceled)` at `:188-191` that swallows and returns, and (c) any other exception (no `finally`). This runs after every playout build and on the hourly schedule, so shutdown/cancellation mid-extraction is routine. **Consequence**: leaked playout locks make `ResetAllPlayoutsHandler.cs:25,36` silently skip those playouts on every future reset, and disable all Blazor playout actions. Partially masked by F5: the *next* `BuildPlayout` for that playout unconditionally unlocks it in its `finally` — i.e. one bug papers over another. ### F5 — The lock primitive itself is unsound: races, no ownership, cross-release (High, class-level) 1. **Non-atomic bool locks**: two concurrent `LockTrakt()` / `LockTroubleshootingPlayback()` / `Lock*Collections()` / `LockPlex()` calls (HTTP threads + scheduler + Blazor circuits genuinely race) can both return `true` — the mutual exclusion these exist for is not guaranteed. `EntityLocker.cs:54-64,110-120,136-146,162-172,188-198,238-248`. 2. **No ownership**: `BuildPlayoutHandler.cs:67` ignores `LockPlayout`'s return value and unconditionally unlocks in `finally` (`:127-130`) — it releases whoever's lock is present (e.g. one held by `ExtractEmbeddedSubtitlesHandler`, or any direct-`Send` caller — relevant to in-flight ersatztv#215). 3. **Concrete cross-release**: scheduler Plex Shows path takes **one** library lock and enqueues **two** messages (`SchedulerService.cs:230-241`: `SynchronizePlexLibraryByIdIfNeeded` + `SynchronizePlexNetworks`). `ScannerService` unlocks after the **first** completes (`:179-182`), so networks-sync runs unlocked; if a user queues a manual scan in that window, networks-sync's completion (`:242-245`) **releases the manual scan's lock** → UI shows idle while a scan is queued/running, and a second concurrent scan of the same library becomes possible. ### F6 — Unguarded lock→enqueue window; partial multi-message enqueue (Medium) Only `TraktController.EnqueueWithTraktLock` (`TraktController.cs:170-192`) guards the enqueue (`catch { UnlockTrakt(); throw; }`). Every other producer — `QueueLibraryScanByLibraryIdHandler:46-79`, `CreateLocalLibraryHandler:45`, `UpdateLocalLibraryHandler:97`, `Libraries.razor:222-273`, `RemoteMediaSourceLibrariesEditor.razor:129`, `UpdateTraktListHandler:56-58`, `SchedulerService` scan loops — locks then `WriteAsync`s bare. A cancelled token (client abort) between lock and write leaks the lock; for the Plex/Jellyfin/Emby **two-message** batches, cancellation between the writes half-enqueues: the source-level sync runs but the library-scan message that would release the lock never exists → permanent leak. ### F7 — Trakt unlock-by-last-item is fragile by construction (Medium-low) Scheduler batches take one global Trakt lock and thread `Unlock = (list == last)` through the messages (`SchedulerService.cs:319-328, 343-352`); only the last message's handler releases (`AddTraktListHandler.cs:44-50`, `MatchTraktListItemsHandler.cs:56-62`). If the last message is never processed — `WorkerService`'s shutdown `break` (`WorkerService.cs:42-45`) with the process staying up in a graceful-shutdown window, or any future channel-drop — the global lock leaks and **all** Trakt operations 409 until restart. Handler exceptions are safe (unlock is in `finally`); message loss is not. (`MatchTraktListItems` defaults `Unlock = true`, so the `UpdateTraktListHandler` single-shot path is correct.) ### F8 — Accepted-async semantics are inconsistent across the API; playout builds are unobservable (Medium) | Endpoint | Accepted | Lock conflict | Missing | Disabled/unsupported | |---|---|---|---|---| | `POST /api/libraries/{id}/scan` | 200 | **200 silent no-op** | 404 | **404** | | `POST /api/libraries/{id}/scan-show` | 200 | 400 (conflated msg) | 400 (conflated) | 400 (conflated) | | `POST /api/trakt/*` | **202** | **409** | 404 | 422 (bad URL) | | `POST /api/channels/{n}/playout/reset` | 200 | no check (queues behind lock) | 404 | — | | `POST /api/playouts/reset-all` | 202 | **silent skip** of locked + ExternalJson | — | — | | `POST /api/maintenance/clean_artwork` | 200 fire-and-forget | — | — | — | | `POST /api/maintenance/empty_trash` | 200 | — | — | **500 text/plain** | | `GET /api/troubleshoot/playback.m3u8` | long-block → 302 | **404** | 404 | 404 | Observability: scans have `GET /api/libraries/scan-status` (in-memory `ScannerProxyService` registry, with progress), Trakt has `GET /api/trakt/status` (busy bool), troubleshooting has a status endpoint (state/exitCode/log tail). **Playout builds have nothing** — `IsPlayoutLocked` is not exposed over HTTP; the SPA fires reset and does a single blind refresh (`web/src/App.tsx:3064-3109`), with only the after-the-fact warnings-count badge. ## Suggestions ### F9 — Parity + hygiene notes (Low) - **Deep scan is not reachable via the API**: `QueueLibraryScanByLibraryIdHandler` hardcodes `ForceSynchronize*ById(library.Id, false)`; Blazor `Libraries.razor` passes `deepScan: true`. External-collections scan (`Synchronize*Collections`) likewise has no API trigger. Both are **#91 phase (b) gates** for deleting the Blazor Libraries page — add to the gate list. - `ResetAllPlayoutsHandler` should report skipped playouts (count/ids) instead of silently dropping them. - Unbounded channels + no shutdown drain: acceptable today (locks are in-memory and die with the process), but worth a one-line decision record so nobody "fixes" it into a persistence trap. --- ## Lock lifecycle matrix | # | Lock | Acquired by | Released by | Release on failure? | Verdict | |---|---|---|---|---|---| | 1 | Library | `QueueLibraryScanByLibraryIdHandler:46` | `ScannerService` after scan | ✗ not-finally (F2); ✗ enqueue window (F6); lies on conflict (F1) | **LEAKS** | | 2 | Library | `QueueShowScanByLibraryIdHandler:49` | same handler `finally:102` | ✓ try/finally | OK (the model to copy) | | 3 | Library | `Create/UpdateLocalLibraryHandler:45/97` | `ScannerService` | ✗ F2/F6 | LEAKS | | 4 | Library | `SchedulerService:212/230/264/291` | `ScannerService` | ✗ F2; Plex Shows 2-msg batch unlocks after 1st (F5.3) | LEAKS + cross-release | | 5 | Library | `Libraries.razor:222`, `RemoteMediaSourceLibrariesEditor.razor:129` | `ScannerService` | ✗ F2/F6 | LEAKS | | 6 | Collections ×3 | `Libraries.razor:256/263/270` | `ScannerService` | ✗ F2; bool race (F5.1) | LEAKS | | 7 | Playout | `BuildPlayoutHandler:67` (return ignored) | same handler `finally:129` | ✓ finally, but releases non-owned locks (F5.2) | cross-release | | 8 | Playout ×N | `ExtractEmbeddedSubtitlesHandler:127` | same handler `:185` | ✗ cancellation early-return + swallowing catch, no finally (F4) | **LEAKS** | | 9 | Trakt | `TraktController:174` | worker handler `finally` | ✓ guarded enqueue | OK | | 10 | Trakt | `SchedulerService:319/343` (batch) | last message's handler | ✗ lost-last-message (F7) | fragile | | 11 | Trakt | `TraktLists.razor:130/138/177`, `UpdateTraktListHandler:56` | worker handler `finally` | dialog-cancel unlocks ✓; enqueue unguarded (F6) | mostly OK | | 12 | Plex | `PlexMediaSources.razor:117/127` | `SignOutOfPlexHandler:34` / `SynchronizePlexMediaSourcesHandler:74` | error branch unlocks `:144`; consumer path not-finally | fragile | | 13 | RemoteMediaSource | `RemoteMediaSources.razor:162` | `DisconnectEmby/JellyfinHandler:37` | ✗ handler throw → leak | fragile | | 14 | Troubleshooting | `PrepareTroubleshootingPlaybackHandler:72/171` | error paths `:110/121/144` or `StartTroubleshootingPlaybackHandler` `finally:203` | ✗ `GetProcess` media-path return `:182-186` (F3); bool race (F5.1) | **LEAKS** | ## API contract recommendation Adopt uniformly for queue-triggering endpoints (matches the issue's proposal; Trakt already implements it): - **202 Accepted** — work queued (change library scan / playout reset / clean-artwork from 200). - **404** — entity missing (real pre-check, per the existing controller-pre-check convention). - **409 ProblemDetails** — lock already held (library scan, scan-show, troubleshoot; reset-all returns 202 + a body listing skipped playouts). - **422 ProblemDetails** — disabled (`ShouldSyncItems=false`) / unsupported (local-library show scan, ExternalJson reset). - Every 202 must have a pollable status surface. Scans and Trakt and troubleshooting have one; **add playout build status** (expose lock/`PlayoutBuildStatus` — coordinate with ersatztv#215, in flight right now). - `empty_trash` 500 text/plain → ProblemDetails. ## Proposed issue split (ersatztv) 1. **Lock-soundness core** `[PLAN-MODE]` — make `EntityLocker` atomic (Interlocked/`lock` for the six bools) and decide the ownership model (token/count vs. documented single-owner + assert). Smallest primitive change that makes F5 impossible; everything else builds on it. Includes concurrency unit tests. 2. **Scan lifecycle honesty** — F1 + F2 + F6(scan paths) + F5.3: outcome enum from queue handlers, 202/404/409/422 mapping, `finally` unlocks in ScannerService, guarded enqueue, make multi-message batches release-safe (composite message or last-message-unlock with loss handling). Same PR: delete SPA grace-tick workaround, OpenAPI regen, contract tests. 3. **Troubleshooting lock leak** — F3: `try/finally` (or `using`-style scope) in `GetProcess`, guard the controller's enqueue, map lock-conflict to 409 instead of 404. 4. **Subtitle-extraction / playout lock leak** — F4 + F5.2: `finally` unlock, respect `LockPlayout` return. **Must coordinate with #215** (fix-215 in flight — playout mutation gating touches the same call sites). 5. **Async contract normalization + playout build observability** — F8 + F9 (reset-all skip reporting, empty_trash ProblemDetails, deep-scan + collections-scan API parity for #91b). Explicitly **not** folded into #197; #197 should reference the resulting contract table. Suggested order: 1 → 2 → 3/4 (parallel) → 5. Items 2–5 are each shippable independently once 1 lands. ## Test & rollback requirements - **Failure-injection (NUnit + NSubstitute, existing `ErsatzTV.Tests` harness)**: throwing mediator inside ScannerService message processing → assert lock released; pre-cancelled token after `LockLibrary` in `QueueLibraryScanByLibraryIdHandler` → assert released; cancellation mid-`ExtractEmbeddedSubtitles` → assert all playout locks released; `GetProcess` with empty media path → assert troubleshooting lock released. - **Concurrency**: parallel `LockTrakt`/`LockTroubleshootingPlayback` (Task.WhenAll ×N) → exactly one `true`. Two concurrent `POST /api/libraries/{id}/scan` (controller test, mocked locker) → one 202, one 409. - **Exactly-once release**: for each terminal path per matrix row, assert `Unlock*` called exactly once (ownership assert makes double-release loud instead of silent). - **Contract**: `OpenApiErrorResponseContractTests` cases for the new 404/409/422s; regenerate `v1.json` + endpoint index per api-conventions checklist. - **Rollback**: all changes are in-memory semantics — no DB migrations, no data risk. The status-code changes are SPA-breaking → each API-changing PR must update `web/src` consumers in the same PR (`libraries.ts` polling, TraktListsScreen untouched, playout reset paths). Nothing here needs a feature flag; revert = git revert. --- # Implementer prompt > **Context**: adversarial-reviewer#20 audit of ErsatzTV async lock ownership, conducted at `main @ ef8915f1`. Findings F1–F9 above; lifecycle matrix and file:line refs verified. Coordinate: ersatztv#215 (playout gating, IN FLIGHT), #202 (media-source lifecycle), #197 (broad API contract — do not fold this work into it). Read `docs/api-conventions.md` + `docs/contributing.md` first per repo rules. > > **Work order** (file as five ersatztv issues per the split above, label `review`): > 1. `[PLAN-MODE]` **EntityLocker soundness**: replace the six racy bool locks with atomic operations; pick and document an ownership model (decisions.md entry). Add concurrency unit tests (parallel lock → exactly one winner; unlock-by-non-owner behavior defined and tested). > 2. **Scan lifecycle**: outcome enum from `QueueLibraryScanByLibraryIdHandler` (fix `return true` on lock-conflict at `:82`); controller 202/404/409/422; `try/finally` unlocks in all eight ScannerService sites; guard every lock→enqueue with catch-unlock (copy `TraktController.EnqueueWithTraktLock`); fix the scheduler Plex Shows two-message batch so the lock survives until the LAST message (or use one composite message). Same PR: remove `PENDING_GRACE_TICKS` workaround in `web/src/api/libraries.ts`, OpenAPI regen, contract tests. > 3. **Troubleshooting**: `finally`-scope the lock in `PrepareTroubleshootingPlaybackHandler.GetProcess` (leak at `:182-186`); 409 for lock-conflict instead of 404; guard the `StartTroubleshootingPlayback` enqueue. > 4. **Playout locks**: `finally` unlock in `ExtractEmbeddedSubtitlesHandler` (leak at `:171-191`); stop ignoring `LockPlayout`'s return in `BuildPlayoutHandler:67` per the ownership model from item 1. Rebase over #215's merge; do not duplicate its gating work. > 5. **Contract + observability**: normalize per the recommendation table; expose playout build status over HTTP; reset-all reports skips; `empty_trash` → ProblemDetails; file the deep-scan + collections-scan API parity gaps against the #91b gate list. > > Per-issue: failure-injection tests listed in the audit are REQUIRED evidence, not optional. Follow the repo's Task Completion Protocol (comment as you work, push, structured close, reply to reviewer). Deliverable complete. Next step: `/dispatch` to file the five ersatztv issues, or fold items into the existing queue (#215 overlap noted). Leaving this issue open until dispatch.

timothy referenced this issue from timothy/ersatztv

2026-07-11 00:54:49 +02:00

[PLAN-MODE] EntityLocker soundness: atomic bool locks + ownership model (reviewer#20 F5) #231

timothy referenced this issue from timothy/ersatztv

2026-07-11 00:55:14 +02:00

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

timothy referenced this issue from timothy/ersatztv

2026-07-11 00:55:25 +02:00

Troubleshooting playback lock leaks permanently on "media not on disk"; lock conflict = 404 (reviewer#20 F3) #233

timothy referenced this issue from timothy/ersatztv

2026-07-11 00:55:33 +02:00

Subtitle extraction leaks all playout locks on cancellation/exception; BuildPlayout ignores lock result (reviewer#20 F4+F5.2) #234

timothy referenced this issue from timothy/ersatztv

2026-07-11 00:55:51 +02:00

Async-op API contract normalization + playout build observability (reviewer#20 F7+F8+F9) #235

timothy commented

2026-07-11 00:56:08 +02:00

Dispatched

All findings filed as review-labeled ersatztv issues:

Issue	Findings	Priority
ersatztv#231 — `[PLAN-MODE]` EntityLocker soundness (atomic bools + ownership model)	F5	high — foundation, land first
ersatztv#232 — scan lifecycle honesty	F1, F2, F6, F5.3	high (both Blockers live here)
ersatztv#233 — troubleshooting playback lock leak + 409	F3	medium
ersatztv#234 — subtitle-extraction/playout lock leaks	F4, F5.2	medium — coordinate with ersatztv#215 (in flight)
ersatztv#235 — async contract normalization + playout build observability	F7, F8, F9	medium — land last; #197 references its contract table

Order: 231 → 232 → 233/234 (parallel) → 235. Each issue carries its own repro paths, fix shape, and required failure-injection tests from the audit above.

Leaving this issue open until the implementer replies per contract (comment-as-you-work → push → close → reply here), which triggers the verification pass.

## Dispatched All findings filed as `review`-labeled ersatztv issues: | Issue | Findings | Priority | |---|---|---| | [ersatztv#231](https://gitea.tblindustries.be/timothy/ersatztv/issues/231) — `[PLAN-MODE]` EntityLocker soundness (atomic bools + ownership model) | F5 | high — **foundation, land first** | | [ersatztv#232](https://gitea.tblindustries.be/timothy/ersatztv/issues/232) — scan lifecycle honesty | F1, F2, F6, F5.3 | high (both Blockers live here) | | [ersatztv#233](https://gitea.tblindustries.be/timothy/ersatztv/issues/233) — troubleshooting playback lock leak + 409 | F3 | medium | | [ersatztv#234](https://gitea.tblindustries.be/timothy/ersatztv/issues/234) — subtitle-extraction/playout lock leaks | F4, F5.2 | medium — **coordinate with ersatztv#215 (in flight)** | | [ersatztv#235](https://gitea.tblindustries.be/timothy/ersatztv/issues/235) — async contract normalization + playout build observability | F7, F8, F9 | medium — land last; #197 references its contract table | Order: 231 → 232 → 233/234 (parallel) → 235. Each issue carries its own repro paths, fix shape, and required failure-injection tests from the audit above. Leaving this issue open until the implementer replies per contract (comment-as-you-work → push → close → reply here), which triggers the verification pass.

timothy referenced this issue

2026-07-11 11:39:01 +02:00

Retro review: ersatztv mutation-parity milestone (#210/#211 PR #214, #208/#209 PR #216) #18

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/adversarial-reviewer#20

Audit: async lock ownership and background-job acceptance in ErsatzTV #20

Context

Audit scope

Evidence required

Relationship to existing work

Deliverable

Audit: async lock ownership & background-job acceptance — findings

Architecture facts the findings rest on

Blockers

F1 — POST /api/libraries/{id}/scan reports success for work it silently dropped

F2 — ScannerService strands library/collection locks on any handler exception

Issues

F3 — Troubleshooting playback lock leaks on "media not on disk"; lock conflict surfaces as 404 (High)

F4 — Subtitle extraction leaks every playout lock on cancellation or exception (High)

F5 — The lock primitive itself is unsound: races, no ownership, cross-release (High, class-level)

F6 — Unguarded lock→enqueue window; partial multi-message enqueue (Medium)

F7 — Trakt unlock-by-last-item is fragile by construction (Medium-low)

F8 — Accepted-async semantics are inconsistent across the API; playout builds are unobservable (Medium)

Suggestions

F9 — Parity + hygiene notes (Low)

Lock lifecycle matrix

API contract recommendation

Proposed issue split (ersatztv)

Test & rollback requirements

Implementer prompt

Dispatched

F1 — `POST /api/libraries/{id}/scan` reports success for work it silently dropped