fix(locking): EntityLocker atomicity + scan-lifecycle honesty (#231, #232) #241

Merged

timothy merged 4 commits from batch/231-232-lock-soundness into main 2026-07-11 13:05:16 +02:00

Conversation 0 Commits 4 Files Changed 21

+1252 -314

timothy commented 2026-07-11 12:42:15 +02:00

Owner

Copy Link

Copy Source

Reviewer#20 Blocker/High batch (arc item 2). Two coupled fixes landed together because #232's honest API contract depends on #231's now-trustworthy Lock* return.

#231 — EntityLocker soundness (F5)

• Atomicity: the six plain-bool lock flags (Plex, Trakt, Emby/Jellyfin/Plex collections, troubleshooting playback) used a non-atomic check-then-set → two concurrent Lock* callers could both win. Converted to int flags mutated only via Interlocked.CompareExchange; the CAS winner is the sole owner and the only one that fires the change event. The three ConcurrentDictionary-backed kinds (Library/Playout/RemoteMediaSource) were already atomic — redundant ContainsKey pre-checks dropped.
• Ownership model (tokenless single-owner discipline, no interface change): documented on IEntityLocker + docs/decisions.md. A true from Lock* confers ownership of exactly one release. Unlock* on an unlocked slot returns false, fires no event, logs a Warning (the double-release / non-owner tripwire). Owner-tokens rejected — the acquirer and releaser cross in-memory Channel<T> queues, so a token would ripple through ~8 message payloads for a defect discipline already prevents.
• Tests: EntityLockerTests (real locker, high-iteration parallel-acquire stress) proving exactly one winner per kind, one-releaser-per-slot, event-fires-once-per-transition. Verified non-vacuous via negative control — a single-round race did not catch a deliberately non-atomic flag; the stress loop does.

#232 — Scan lifecycle honesty (F1 + F2 + F6 + F5.3)

• Honest outcome: 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 the errors), with [ProducesResponseType] for each.
• No stranded locks: all 8 ScannerService unlock sites moved into try/finally (a handler exception previously leaked the lock until restart). Compensating unlock-on-enqueue-failure added to the lock→enqueue producers (QueueLibraryScanByLibraryIdHandler, CreateLocalLibraryHandler, UpdateLocalLibraryHandler, UpdateTraktListHandler) per the EnqueueWithTraktLock pattern.
• Batch release-safety (F5.3): the Plex-Shows one-lock/two-message batch now uses a tail-token (Unlock flag on the Plex messages; only the trailing SynchronizePlexNetworks releases), mirroring the scheduler Trakt list == last precedent — the lock now spans both units.
• SPA: removed PENDING_GRACE_TICKS/pruneGraceExpiredPending (they only existed to paper over the lying 200); triggerScan now handles 202 (pending+poll), 409 (already scanning → reconcile, no error toast), 404/422 (surface error).
• Docs: docs/api-conventions.md §3b (outcome-enum→status mapping + compensating-unlock rule). OpenAPI regenerated (v1.json); check:api in sync.

Verification (integrated branch)

• dotnet build ErsatzTV.sln: 0 errors
• ErsatzTV.Tests: 1051 passed · ErsatzTV.Infrastructure.Tests: 53 passed (incl. 37 new locker tests)
• web: 579 passed (65 files) · typecheck clean · check:api in sync

Deferred (noted, not in scope)

• BuildPlayoutHandler / ExtractEmbeddedSubtitlesHandler finally-gating on the captured acquire result → #234 (#231 only defines the contract they consume).
• Blazor scan producers (Libraries.razor, RemoteMediaSourceLibrariesEditor.razor) share the leak shape but are being retired in #91 phase (b); not the same trivial call structure — flagged, not expanded into scope.

fixes #231
fixes #232

🤖 Generated with Claude Code

Reviewer#20 Blocker/High batch (arc item 2). Two coupled fixes landed together because #232's honest API contract depends on #231's now-trustworthy `Lock*` return. ## #231 — EntityLocker soundness (F5) - **Atomicity**: the six plain-`bool` lock flags (Plex, Trakt, Emby/Jellyfin/Plex collections, troubleshooting playback) used a non-atomic check-then-set → two concurrent `Lock*` callers could both win. Converted to `int` flags mutated only via `Interlocked.CompareExchange`; the CAS winner is the sole owner and the only one that fires the change event. The three `ConcurrentDictionary`-backed kinds (Library/Playout/RemoteMediaSource) were already atomic — redundant `ContainsKey` pre-checks dropped. - **Ownership model** (tokenless single-owner discipline, **no interface change**): documented on `IEntityLocker` + `docs/decisions.md`. A `true` from `Lock*` confers ownership of exactly one release. `Unlock*` on an unlocked slot returns `false`, fires no event, logs a Warning (the double-release / non-owner tripwire). Owner-tokens rejected — the acquirer and releaser cross in-memory `Channel<T>` queues, so a token would ripple through ~8 message payloads for a defect discipline already prevents. - **Tests**: `EntityLockerTests` (real locker, high-iteration parallel-acquire stress) proving exactly one winner per kind, one-releaser-per-slot, event-fires-once-per-transition. **Verified non-vacuous** via negative control — a single-round race did not catch a deliberately non-atomic flag; the stress loop does. ## #232 — Scan lifecycle honesty (F1 + F2 + F6 + F5.3) - **Honest outcome**: `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 the errors), with `[ProducesResponseType]` for each. - **No stranded locks**: all 8 `ScannerService` unlock sites moved into `try/finally` (a handler exception previously leaked the lock until restart). Compensating unlock-on-enqueue-failure added to the lock→enqueue producers (`QueueLibraryScanByLibraryIdHandler`, `CreateLocalLibraryHandler`, `UpdateLocalLibraryHandler`, `UpdateTraktListHandler`) per the `EnqueueWithTraktLock` pattern. - **Batch release-safety (F5.3)**: the Plex-Shows one-lock/two-message batch now uses a tail-token (`Unlock` flag on the Plex messages; only the trailing `SynchronizePlexNetworks` releases), mirroring the scheduler Trakt `list == last` precedent — the lock now spans both units. - **SPA**: removed `PENDING_GRACE_TICKS`/`pruneGraceExpiredPending` (they only existed to paper over the lying 200); `triggerScan` now handles 202 (pending+poll), 409 (already scanning → reconcile, no error toast), 404/422 (surface error). - Docs: `docs/api-conventions.md` §3b (outcome-enum→status mapping + compensating-unlock rule). OpenAPI regenerated (`v1.json`); `check:api` in sync. ## Verification (integrated branch) - `dotnet build ErsatzTV.sln`: 0 errors - `ErsatzTV.Tests`: 1051 passed · `ErsatzTV.Infrastructure.Tests`: 53 passed (incl. 37 new locker tests) - `web`: 579 passed (65 files) · typecheck clean · `check:api` in sync ## Deferred (noted, not in scope) - `BuildPlayoutHandler` / `ExtractEmbeddedSubtitlesHandler` `finally`-gating on the captured acquire result → **#234** (#231 only defines the contract they consume). - Blazor scan producers (`Libraries.razor`, `RemoteMediaSourceLibrariesEditor.razor`) share the leak shape but are being retired in #91 phase (b); not the same trivial call structure — flagged, not expanded into scope. fixes #231 fixes #232 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 3 commits 2026-07-11 12:42:16 +02:00

fix(locking): atomic EntityLocker flags + single-owner release contract (#231) ... 275908ec11

The six plain-bool lock flags (Plex, Trakt, Emby/Jellyfin/Plex collections,
troubleshooting playback) used a non-atomic check-then-set, so two concurrent
Lock* callers could both win. Convert them to int flags mutated only via
Interlocked.CompareExchange, so the caller that wins the 0->1 transition is the
sole owner and the only one that fires the change event. The three
ConcurrentDictionary-backed kinds (Library/Playout/RemoteMediaSource) were
already atomic; drop their redundant ContainsKey pre-checks.

Define the ownership contract (tokenless single-owner discipline, no interface
change) on IEntityLocker and in docs/decisions.md: a true from Lock* confers
ownership of exactly one release; Unlock* on an unlocked slot returns false,
fires no event, and logs a warning (the double-release / non-owner tripwire).

Adds EntityLockerTests (real locker, parallel-caller races) proving exactly one
winner per kind, one-releaser-per-slot, and event-fires-once-per-transition.

Ref #231. Scan-lifecycle call-site fixes that consume this contract land in the
same PR (#232); the BuildPlayout/subtitle finally-gating is #234.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

feat(libraries): honest scan lifecycle + one-lock-one-release discipline (#232) ... 088644e8c0

Scan queue handler now returns a QueueLibraryScanResult enum
(Queued|NotFound|SyncDisabled|AlreadyScanning) instead of a lying bool;
LibrariesController.ScanLibrary maps them to 202/404/422/409 with ProblemDetails.
Guard the lock->enqueue with the EnqueueWithTraktLock compensating-unlock pattern.

ScannerService now releases every library/collection lock in a finally so a handler
exception can't leak the lock. Plex "Shows" scheduler batch (one lock, two messages)
now has only the trailing SynchronizePlexNetworks carry the single release
(Unlock flag), mirroring the scheduler Trakt tail-token precedent.

Guard the other lock->enqueue producers (Create/UpdateLocalLibrary, UpdateTraktList)
with compensating unlock. SPA drops the PENDING_GRACE_TICKS heuristic now that the
POST reports 202/409/404/422 directly: 202 -> pending+poll, 409 -> reconcile (no
error toast), 404/422 -> surface error.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Merge branch 'slice/232-scan' into batch/231-232-lock-soundness 78e4a3c738

timothy referenced this issue from a commit 2026-07-11 12:54:50 +02:00

fix(review): bounded scan-pending net + scheduler compensating unlock (#232)

timothy added 1 commit 2026-07-11 12:54:50 +02:00

fix(review): bounded scan-pending net + scheduler compensating unlock (#232) ... 702b0121cd

Adversarial review (fork + Codex, both flagged) of PR #241:

- SPA (both reviewers): removing PENDING_GRACE_TICKS wholesale reintroduced a
  stuck scan button. A 202'd scan that finishes between 10s polls (short/empty
  library) is never observed active, so its optimistic pending flag wedged the
  button disabled until reload. Restore a BOUNDED grace net (pruneGraceExpiredPending)
  — re-scoped honestly: it absorbs the inherent queue->observed-active lag and the
  fast-completion race, NOT the removed lying-200 compensation (the POST now returns
  409/404/422 honestly). Bounds pending to PENDING_GRACE_TICKS * pollMs (~30s).

- SPA 409 (Codex): on "already scanning" the button was cleared+reconciled, but a
  scan-status still lagging the in-progress scan re-enabled the button and let the
  user fire repeated 409s. Keep the pending flag on 409 (no toast) so the button
  stays disabled; polling promotes or expires it.

- Scheduler (Codex): the Plex-Shows tail-token batch and the local/Jellyfin/Emby
  scan enqueues had no compensating unlock — a WriteAsync failure after LockLibrary
  (cancellation on shutdown) stranded the library lock. Wrap each acquired-lock
  enqueue in try/catch → UnlockLibrary → rethrow (the Plex catch covers both writes,
  since the library message carries Unlock: false and the un-enqueued networks
  message was the sole releaser).

Tests: two new App.test.tsx cases — grace-window expiry re-enables the button, and
409 keeps it disabled through the queue->active lag.

Ref #232.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy merged commit c42928e891 into main 2026-07-11 13:05:16 +02:00

timothy deleted branch batch/231-232-lock-soundness 2026-07-11 13:05:17 +02:00

timothy referenced this issue from a commit 2026-07-11 13:05:18 +02:00

Merge pull request 'fix(locking): EntityLocker atomicity + scan-lifecycle honesty (#231, #232)' (#241) from batch/231-232-lock-soundness into main

timothy referenced this pull request 2026-07-11 13:05:54 +02:00

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

timothy referenced this pull request 2026-07-11 13:06:19 +02:00

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

timothy referenced this pull request 2026-07-11 13:06:50 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this pull request 2026-07-11 13:14:48 +02:00

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

timothy referenced this pull request 2026-07-11 13:40:27 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

Sign in to join this conversation.

Reviewers

No Reviewers

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

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#241