Closes the two F9 Libraries.razor parity gaps and normalizes scan-show error
mapping to ProblemDetails.
TASK 1 — library-wide deep scan:
- QueueLibraryScanByLibraryId gains optional `bool DeepScan = false`; handler
threads it into ForceSynchronize{Plex,Jellyfin,Emby}LibraryById.
- POST /api/libraries/{id}/scan?deep=false binds it via [FromQuery].
TASK 2 — external-collections scan (new endpoints):
- POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=false
acquires the per-source collections lock (§3b: lock IS the running scan → 409),
enqueues Synchronize{X}Collections(id, ForceScan:true, deep) to the scanner
channel, returns 202; compensating-unlock on enqueue throw.
TASK 3 — scan-show normalization:
- New QueueShowScanResult enum; handler returns it instead of bool.
- POST /api/libraries/{id}/scan-show now maps 202/404/409/422 (all errors
ProblemDetails) instead of 200/404/400-anonymous-object.
- Updated the lone Blazor caller (TelevisionSeasonList.razor).
Tests: LibrariesController (scan deep=true, scan-show enum→status), the three
media-source controllers (scan-collections route/404/409/202/compensating-unlock),
and handler tests for both changed handlers (deep threading + show-scan outcomes).
Docs: api-conventions §3b exemplar + blazor-route-parity §5 F9 gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Slice C of the async-op contract normalization:
- channel reset (POST /api/channels/{channelNumber}/playout/reset) now
returns 202 Accepted (was 200 Ok) — it only queues a background rebuild
- reset-all (POST /api/playouts/reset-all) still 202 but now returns a
ResetAllPlayoutsResponseModel body reporting QueuedPlayoutIds /
SkippedLocked / SkippedUnsupported instead of silently swallowing skips;
handler returns a new ResetAllPlayoutsResult record
- single-playout GET (GET /api/playouts/{id}) now exposes IsLocked on
PlayoutResponseModel, set from IEntityLocker.IsPlayoutLocked mirroring
the list projection — gives a polling client the lock flag
Tests: channel reset asserts 202; reset-all asserts 202 + skipped-body
shape; single GET asserts IsLocked; new ResetAllPlayoutsHandlerTests
(in-memory SQLite) asserts locked/ExternalJson/None land in skipped lists
and eligible playouts in queued. docs/api-conventions.md §3a updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The global Trakt lock is acquired by SchedulerService.RefreshTraktLists /
MatchTraktLists (and TraktController) and released only when the *terminal*
message of a batch — the one carrying Unlock: true (list == traktLists.Last())
— is processed by WorkerService, whose handler (AddTraktListHandler /
MatchTraktListItemsHandler) calls IEntityLocker.UnlockTrakt() in a finally.
WorkerService.ExecuteAsync breaks out of the read loop on
stoppingToken.IsCancellationRequested (and exits on channel completion /
reader cancellation) BEFORE processing the next message. If shutdown lands
after a batch is enqueued but before its terminal Unlock: true message is
handled, UnlockTrakt() never runs and the in-memory Trakt lock leaks for the
rest of the process lifetime (subsequent Trakt operations 409 forever).
Fix (option a): make the batch-release loss-tolerant with a compensating
release in a finally around the read loop — if the Trakt lock is still held
when the worker stops, release it. Chosen over tracking pending ownership
(b) because the lock is a global singleton and WorkerService is its sole
batch-release site, so "held at shutdown" unambiguously means "the terminal
release was lost"; covers all three exit paths (break / channel completion /
cancellation) in one place. Same lock-lifecycle class as #231/#233/#234.
Regression test: WorkerServiceTests gates the first (non-terminal) batch
message on the stopping token, then StopAsync-cancels so the worker breaks
before the terminal Unlock: true message — asserts the lock is released and
the terminal message was never processed. Proven non-vacuous: inverting the
finally condition fails the test.
Backend-only; no controller/DTO/SPA/OpenAPI impact.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Slice A of the async-op API contract normalization.
MaintenanceController:
- EmptyTrash error path: was 500 text/plain (error.ToString()); now maps the
BaseError Left through ApiResults.ToErrorResult() -> 404 (NotFoundError) / 422
ProblemDetails. Success stays 200 OkResult. Added ProducesResponseType 200 + 422.
- CleanArtwork: fire-and-forget enqueue of DeleteOrphanedArtwork was a silent 200;
now returns 202 Accepted (AcceptedResult) since it queues background work.
Added ProducesResponseType 202. (Controller does not derive from ControllerBase,
so results are built directly as before.)
TroubleshootController.TroubleshootPlayback (GET|HEAD /api/troubleshoot/playback.m3u8):
- Two bare body-less NotFound() call sites conflated "not found" with "prepare/
playback failure". Both now return a ProblemDetails body:
* prepare-failure (result.IsLeft): mapped through error.ToErrorResult() -> 404 for
NotFoundError (unknown media item/channel) else 422 for a validation BaseError.
* terminal fall-through (prepare ok but no playable output): kept 404 with a
distinguishing ApiResults.NotFoundProblem(...) detail.
- Added ProducesResponseType 404 + 422 (409 already present).
Consumer check: the SPA (PlaybackTroubleshootingScreen) feeds the playback.m3u8 URL
straight to hls.js via HlsPlayer, which never inspects the HTTP status code — playback
state is surfaced via the separate /api/troubleshoot/playback/status poll. So the
404->422 split for the validation subcase is safe; no player code branches on the
status code.
Tests: MaintenanceControllerTests (200/422/202 + enqueue assertion),
TroubleshootControllerTests (prepare 404 NotFoundError, 422 validation). All green;
Api error-metadata/contract/security scans still pass.
Note: OpenAPI artifacts (v1.json / v1.d.ts) intentionally NOT regenerated here — the
orchestrator regenerates once after all #235 slices merge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the two SHOULD-FIX gate findings from the #91 cold review by making the
removal plan address them explicitly instead of clearing the gate by omission.
Pages (verified in code, not assumed) — OIDC's AuthorizeFolder("/") gates only the
Blazor _Host Razor Page; /app (SPA) and /api/* were already unauthenticated since
phase (a); /iptv JWT + API-key filters are independent of Blazor and survive
removal. Sign-off: no capability lost, no NEW exposure beyond phase (a); real
SPA/API auth deferred to #197. Recorded in docs/decisions.md.
(cut at removal time on the pre-deletion main commit — not a v* tag, no release
build) + the restore path (checkout+build+pin test container, or revert the merge).
Recorded in docs/decisions.md.
Both fold into a new "Section 5 — Removal execution runbook" in blazor-route-parity.md
so the (gated) removal PR has an ordered checklist. Docs-only; no code change.
refs #205#206#91
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fork adversarial review nit: Map_Keys_Should_Not_Begin_With_Forbidden_Prefix
covered only Tier-1 Map keys, not the Tier-2 PatternRule templates. That guard
invariant is the load-bearing protection for the un-prefix-guarded /api|/artwork|
/docs|/openapi surface, so make it self-enforcing over ALL rules — a future
prefix-violating template now fails the test instead of slipping through.
Exposes internal LegacyUiRedirects.PatternTemplates (InternalsVisibleTo already
set for ErsatzTV.Tests); stores the raw template on PatternRule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend LegacyUiRedirects from an exact-match dictionary to a two-tier matcher:
Tier 1 keeps the exact Map (now 52 entries incl. the ?kind= browse roots),
Tier 2 adds 36 ordered segment-template PatternRules for id-carrying routes.
{id} is a strict positive integer (non-int/0/neg/overflow falls through), which
also makes the rule set collision-free by construction. New AppendQueryString
helper merges the incoming query into ?kind= targets with '&' (kills the
double-'?' bug); one-line Startup change keeps the redirect GET/HEAD-only 302
before UseRouting.
Completes phase-(a) Step 1 for every PARITY-OK route (#91 phase b); the
catch-all fallback replacing MapFallbackToPage stays with the removal PR.
/media/sources/* (#202) and /system/health remain deliberately un-redirected.
fixes#204
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build the Remote media-source SPA screens over the S5 foundation, replacing
the MediaSourceEditorPlaceholder for the plex/jellyfin/emby dispatch branches
only (Local branches left for S6a):
- PlexSourceScreen: pin-flow sign-in / fix-credentials / sign-out with the
§C1 poll state machine — polls GET /api/media-sources/plex every 2s up to
150s and keeps polling while authorized-but-locked ("finalizing"); the
terminal success is the lock releasing. Popup-blocked fallback link. Server
table (Refresh disabled while locked / Edit Libraries / Edit Path
Replacements) + sign-out content-removal confirm dialog.
- RemoteSourceScreen (shared Jellyfin/Emby): connect / edit-connection /
disconnect (warning dialog) + server table.
- RemoteConnectionEditScreen (shared): secure key affordance (§C3/finding 1)
— address prefilled, "leave blank to keep" when hasApiKey, required on first
connect; stored key never rendered or requested.
- RemoteLibrariesEditScreen (shared): client-side sortable Name + MediaKind
columns, per-library sync Switch, one Save; draft keyed by (name,mediaKind)
not id, refetch after save (ids change on disable, §C4a).
- PathReplacementsEditScreen (shared): row list + selected-row edit form,
add/remove, one Save; both fields required; family remote-path column label.
All editors use the ChannelEditScreen draft/save model + a shared useDirtyGuard
(registerNavigationGuard + beforeunload), Save gated !valid||!dirty||saving,
draft retained on 422/network, destructive actions gated on saving, 409 →
refetch. Colocated tests cover the poll (waiting→finalizing→success asserting
it does NOT stop at authorized&locked, timeout, budget-exhausted), the secure
key affordance, sortable columns, draft-retained-on-422, dirty-guard veto, and
the disconnect/sign-out dialogs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the Local library create/edit editor (create at /app/libraries/local/new,
edit at /app/libraries/local/{id}) wired into the S5-built LibrariesRouteScreen
dispatch switch, replacing MediaSourceEditorPlaceholder for the local-new and
local-edit sub-routes only. Remote (Plex/Jellyfin/Emby) branches are untouched
(S6b).
- Name (required) + Media Kind (create-only, disabled+annotated on edit)
- Add Path: path-exists pre-check (L7) + in-draft duplicate detection
(mediaSources/paths.ts normalizePath)
- Delete path: draft-local removal with a media-item-count confirm dialog
- Move path: dialog filtered to same-MediaKind libraries excluding the source,
including "(New Library)" which composes createLocalLibrary + moveLocalLibraryPath
(surfaces the error and leaves the new empty library on a failed move, matching
Blazor); gated on !dirty to avoid clobbering unsaved edits with the post-move
refetch
- Draft/saved model with explicit Save (POST L3 / PUT L4), draft retained on
422/network error, dirty-guard (registerNavigationGuard + beforeunload)
- Delete library (L5) with a media-item-count confirm; 409 refetches detail
Extended the existing App.test.tsx App-owned-popstate regression test (design
§D.2) to exercise the real screen's dirty guard instead of a manually-armed
stand-in, now that S6a has landed the editor it was stubbing out for.
Verification (web/): vitest (632 passed), eslint clean, tsc -b + vite build
clean, check:api reports no drift.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New JellyfinMediaSourcesController (/api/media-sources/jellyfin, J1-J9) and
EmbyMediaSourcesController (/api/media-sources/emby, E1-E9), wrapping the
existing Jellyfin/Emby MediatR commands per the #202 design doc §A.3/§A.4.
Secure connection contract (§C3/§B, finding 1): the connection GET returns
only { address, hasApiKey } — the API key never crosses the wire. The PUT
retains the existing key when the incoming key is blank, sets a new one when
non-blank, and 422s "API key is required" on a blank first connect.
Finding 7 (lock-release discipline): DisconnectJellyfinHandler and
DisconnectEmbyHandler now wrap their work in try/finally so a throw from any
awaited dependency (repo delete, search-index commit, secret store) still
releases the family lock instead of wedging every future disconnect at 409.
Findings 2c/8 (path-replacement cross-source guard): UpdateJellyfinPathReplacementsHandler
and UpdateEmbyPathReplacementsHandler now reject, before any write, an incoming
positive Id that isn't owned by the route's media source, a null item, or a
blank RemotePath/LocalPath — all 422 with no partial mutation. Defense-in-depth
repo fix: the Jellyfin/Emby path-replacement UPDATE SQL in MediaSourceRepository
now scopes by {Jellyfin,Emby}MediaSourceId (was previously unscoped by Id alone,
allowing a PUT to one source to silently overwrite another source's row). The
Plex path-replacement method (~line 397) is untouched — that's slice S2's file.
Library preferences (§C4a): the controller validates the incoming id set
against the source's known libraries (reject foreign ids, require full
coverage, no Id=0) before dispatch, then — for §C7 — LockLibrary + enqueues
the SynchronizeXLibraries/SynchronizeXLibraryByIdIfNeeded pair per enabled
library (compensating unlock if the enqueue throws), and returns the reloaded
list (ids are not stable across a disable).
404s on id-taking endpoints come from a controller pre-check (GetXMediaSourceById
is None), not a handler NotFoundError, since Either.Apply/ToEitherAsync join any
NotFoundError into a flat 422 (finding 9).
Tests: controller route/404/409/422 tests for both families; disconnect
fault-injection tests proving the lock releases even when a dependency throws;
path-replacement handler tests for cross-source-id/blank/null-item rejection
and correct add/update/delete merge; a repository-level test proving the SQL
fix stops a same-family cross-source path-replacement overwrite.
No new commands, no DB migration, no OpenAPI regen (gated until S1-S3 merge
per the design doc's build-slice plan).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds LocalLibrariesController (L1-L7: list/get/create/update/delete/move-path/
path-exists) wrapping the existing local-library MediatR commands, mapping to
the shared S0 response DTOs. Per design #202 §A.1/§C5/§C6:
- 404 for L4/L5/L6 comes from a controller pre-check (GetLocalLibraryById is
None), not the handler -- .Apply/.ToEitherAsync both .Join() a NotFoundError
into a plain 422, so relying on the handler would be dead code. This is
check-then-act; a delete racing the pre-check falls through to the handler's
422, documented in the controller.
- L4/L5 409 via IEntityLocker.IsLibraryLocked(id); L6 resolves the source
library from the path id (new ILibraryRepository.GetLibraryIdForPath) before
its own lock check.
- MoveLocalLibraryPathHandler gains same-MediaKind and different-library
validation (finding 3) -- Blazor only filtered these client-side in the move
dialog, so an API/MCP client could bypass them.
- CreateLocalLibraryHandler/UpdateLocalLibraryHandler gain a shared
NewPathsMustExist validation (LocalLibraryHandlerBase) that Directory.Exists-
checks only new paths (Id < 1); existing rows stay exempt so an unmounted
share doesn't block a rename. L7 (path-exists) is a controller-local
IFileSystem check with no command.
Tests: controller route/mediator-arg tests incl. 404-pre-check vs
fall-through-422 and 409-lock cases; handler tests for the move-path
cross-kind/same-library 422s, new-path 422 (missing/mixed), and a lossless
round-trip proving local paths are identified by normalized path string, not
id. Full solution test suite (Scanner/Core/Architecture/Tests/Infrastructure)
green, 0 regressions.
Deviations: none from the S1 slice description. Did not touch
MediaSourceRepository.cs or any Plex/Jellyfin/Emby file (S2/S3 scope). Did not
run the OpenAPI regen scripts (separate gate after S1-S3 merge per design §E).
New PlexMediaSourcesController (/api/media-sources/plex) P1-P8 wrapping
existing MediatR commands: state GET, pin-flow, sign-out, per-server
libraries/path-replacements GET+PUT, and refresh — VMs projected to the
S0 shared DTOs, ApiResults mapping, 404 controller pre-checks, #215-style
409 lock guards, [EndpointGroupName("general")].
Lock-lifecycle hardening (the tricky part):
- TryCompletePlexPinFlowHandler now releases the Plex lock ONLY on its
non-handoff exits (timeout-throw, poll exception, enqueue exception, the
dead return-false) via try/catch — NOT an unconditional finally. On
success the lock is handed off to SynchronizePlexMediaSources (the sole
releaser after discovery); a finally would double-release and release
before discovery, re-opening the finding-5 poll race. Fixes the latent
leak where an abandoned pin flow wedged Plex locked until restart.
- StartPlexPinFlow controller compensates UnlockPlex on the Left branch AND
any thrown dispatch/enqueue; only the Right/200 path holds the lock.
- SignOutOfPlexHandler wraps its work in try/finally { UnlockPlex() } — a
terminal handler with no handoff, so unconditional release is correct.
- Post-save library sync enqueues SynchronizePlexLibraryByIdIfNeeded
(Unlock:false) then SynchronizePlexNetworks (Unlock:true) — one lock, one
release on the last message, compensating-unlock if the 2nd enqueue throws
(corrects the Blazor Unlock-ordering bug, finding 6).
Data-integrity hardening:
- UpdatePlexPathReplacementsHandler rejects (422, no mutation) any positive
Id not owned by the route source, blank RemotePath/LocalPath, and null
list/items (findings 2c/8).
- MediaSourceRepository Plex path-replacement UPDATE gains
AND PlexMediaSourceId = @id (Jellyfin/Emby untouched — slice S3).
- ReplaceLibraryPreferences controller validates the id set against the
source's libraries (rejects unowned + Id=0), returns the reloaded list
(ids change on disable).
Tests (NUnit/Shouldly/NSubstitute), 34 new, all green: pin-flow lock
released on thrown-cancellation/poll-throw/enqueue-throw AND held on
success (no double-release); sign-out finally-release under a throwing
dependency; cross-source/nonblank/null path-replacement 422s; library-prefs
id-not-owned 422; post-save enqueue exact messages + Unlock flags; full
controller route/404/409/422 coverage.
Refs #202
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the Libraries domain verbatim out of web/src/App.tsx into
web/src/screens/LibrariesScreen.tsx (zero-prop, self-sufficient), mirroring the
ChannelsScreen extraction (#244). Pure structural move: no API, route, CSS, or
visual change. App.tsx retains only the import + the <LibrariesScreen /> dispatch.
- 10 symbols moved (LibrariesLoadingState -> sourceLastScanLabel); App.tsx's
formatDateTime is inlined into the moved screen so it has no import back into
App.tsx (behavior-identical), matching the Channels precedent.
- Libraries behavior tests moved to a colocated LibrariesScreen.test.tsx with its
own scoped fetch mock (renders <LibrariesScreen /> directly); App.test.tsx keeps
one nav-smoke test for the route.
- Pruned now-dead App.tsx imports (Server, MonitorPlay, Music, FileImage, Folder,
HardDrive icons; useLibrariesScreenQuery, LibraryScanStatus/MediaSource/
MediaSourceLibrary types) and the now-dead runPollTick test helper (its doc
comment named it Libraries-specific).
- Disabled "Add Source"/gear/"Scan All" affordances are unchanged (wired in later
S5/S6 slices, not here).
Verified: web vitest 584 passed, eslint clean, tsc/vite build clean, check:api no
drift.
refs #202
Response DTOs (ErsatzTV.Core/Api/MediaSources) and request DTOs
(ErsatzTV/Controllers/Api/Requests) per the #202 design doc §B — the
shared shapes that backend slices S1 (local libraries), S2 (Plex), and
S3 (Jellyfin/Emby) will consume. No controllers or handler changes;
DTOs are unused so far.
Notable: RemoteConnectionResponseModel deliberately never carries the
raw API key (secure connection contract); SaveRemoteConnectionRequest's
To{Jellyfin,Emby}Command(existingApiKey) retains the existing key when
the incoming ApiKey is blank/omitted.
No conventions changed; nothing to update in docs/api-conventions.md.
Independent Codex review of the fix diff surfaced two real findings the fork pass missed:
- Medium: the #251 affected-playout QUERIES in ReplaceDecoTemplateItemsHandler and
UpdateDecoHandler still ran on the request `cancellationToken`, so a cancellation
landing after SaveChanges committed but before those queries executed would throw
before the CancellationToken.None enqueue — the edit committed but no playout Reset,
re-opening the stale-content bug in that window. Run the entire post-commit
invalidation (queries + enqueue) on CancellationToken.None so the side effect can't
be half-aborted once the data has changed.
- Low: UpdateDefaultDecoHandler enqueued a Reset for request.PlayoutId even when
ExecuteUpdateAsync matched 0 rows (nonexistent playout), creating a background build
request for an id that isn't there. Guard the enqueue on rows-updated > 0 so the
enqueued set equals the affected set. Added a regression test.
Also corrected the ReplaceProgramScheduleItemsHandler comments: the schedule-item
hierarchy is TPT (table-per-type), not TPH — the SetValues reconcile is safe either way
(same-runtime-type guard; no discriminator to corrupt), Codex confirmed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#252: ReplaceProgramScheduleItems deleted and re-inserted every item on every
save (even a no-op PUT-back), and PlayoutScheduleItemFillGroupIndex.ProgramScheduleItemId
is OnDelete(Cascade) — so every schedule save silently wiped persisted
fill-group/shuffle enumerator progression for all playouts using the schedule.
Switch to a positional in-place reconcile: for a same-typed slot, copy scalars via
CurrentValues.SetValues (BuildItem stays the single source of item construction, so
no field is dropped) and rebuild the watermark/graphics join rows, keeping the item
id — and with it the fill-group index. Subtype change / surplus falls back to
delete+insert for that slot only. The request DTO carries no stable item id, so
position is the only key here; true content-aware stable identity is deferred to the
shared concurrency/round-trip contract in #253.
#251: deco / deco-template CONTENT edits (and default-deco assignment) only take
effect on a playout Reset build — deco/break/default-filler content is applied during
Reset, a Continue keeps the frozen filler items, and BlockKey change-detection has no
deco dimension to self-heal. The editors enqueued nothing (a commented-out TODO in
ReplaceDecoTemplateItemsHandler), so filler/break content stayed stale indefinitely
until a manual Reset. Enqueue BuildPlayout(Reset) for exactly the affected playouts:
- ReplaceDecoTemplateItemsHandler: playouts via PlayoutTemplate.DecoTemplateId
- UpdateDecoHandler: playouts via Playout.DecoId and via deco-template items
- UpdateDefaultDecoHandler: the reassigned playout (adjacent same-class fix)
Post-commit enqueues use CancellationToken.None (audit #22 policy).
Tests: DecoInvalidationTests + ReplaceProgramScheduleItemsReconcileTests, each proven
non-vacuous against a negative control (inverted the primitive, verified 0 CS errors so
the --no-build run used a fresh dll). Full ErsatzTV.Tests suite green (1067).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Independent Codex review of PR #250 caught a cross-release the fork missed: the
outer catch in PrepareTroubleshootingPlaybackHandler released the troubleshooting
lock unconditionally, so an exception BEFORE this caller acquired it (e.g. request
cancellation during validation, or a DB error) would release a lock held by another
session.
Fix: track ownership with a Handle-scoped `lockAcquired` flag and gate the catch on
it. Acquisition for the media-item path moves out of GetProcess up into Handle (after
validation succeeds), so one place owns the full lifecycle: acquire -> on Left release
-> on success hand off to StartTroubleshootingPlayback -> on any exception release only
if we own it. GetProcess is now lock-free.
Test: Handle_Should_Not_Release_Lock_It_Never_Acquired (exception before acquisition ->
no Unlock, no cross-release), verified non-vacuous against an inverted-condition
negative control. Strengthened the empty-path test to also assert acquisition happened.
Deferred (noted for close comment): worker-dispatch-failure lock leak and
BuildPlayout silent-skip observability are pre-existing / sanctioned -> #235.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Consume the EntityLocker ownership contract (#231/#241: Lock* returns true iff
this caller won the slot) at three lock-leak sites surfaced by adversarial-reviewer#20.
#233 (F3) — troubleshooting playback:
- PrepareTroubleshootingPlaybackHandler: both lock sites now acquire via
`if (!LockTroubleshootingPlayback())` (kills the check-then-set TOCTOU) and the
empty-media-path Left return releases the lock it acquired — previously it leaked,
wedging the status endpoint at "running" forever for a file gone from disk.
- TroubleshootController.TroubleshootPlayback: lock conflict is now 409 ProblemDetails
(was a bare 404, indistinguishable from a bad id); the Prepare-success -> enqueue
window releases the lock if we never hand off to StartTroubleshootingPlayback.
#234 (F4 + F5.2) — playout builds:
- ExtractEmbeddedSubtitlesHandler: try/finally releases exactly the playouts it
locked, on every terminal path (cancellation early-return, swallowed cancellation,
any exception) — no more permanent leaks after cancelled mid-extraction, and no
cross-release of playouts held by someone else.
- BuildPlayoutHandler: skips (logs, returns Right) when LockPlayout returns false
instead of building unlocked and cross-releasing the other owner's lock in finally.
Tests: handler-level release-discipline tests (Prepare empty-path, Extract
cancellation + no-cross-release, BuildPlayout skip + finally-release) via the
InMemoryTvContext harness, a TroubleshootController 409 test, and OpenApi contract
cases for the m3u8 endpoint's 409. OpenAPI regenerated. All non-vacuous (F3 verified
against a negative control).
Fixes#233, #234
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the Channels domain verbatim out of web/src/App.tsx into
web/src/screens/ChannelsScreen.tsx (zero-prop, self-sufficient, mirroring the
SchedulesScreen extraction). Pure structural move: no API, route, CSS, or
visual change. App.tsx retains only the import + the <ChannelsScreen /> dispatch.
- 14 symbols moved (ChannelViewFilter → ChannelTableRow); the Dashboard-owned
progressFromNowPlaying is inlined into the moved progressFromChannelState so
the screen has no import back into App.tsx (behavior-identical).
- 12 Channels behavior tests moved to a colocated ChannelsScreen.test.tsx with
its own scoped fetch mock (renders <ChannelsScreen /> directly, no
mockDashboardApi); App.test.tsx keeps one nav-smoke test for the route.
- Pruned 12 now-dead App.tsx imports; shared symbols (ChannelState,
messageFromError, ApiError, useChannelsQuery) verified still used and kept.
- Docs: spa-conventions §6 (extracted-screen own-fetch-mock convention),
decisions.md (single-file rationale; no web/src/channels/ sibling dir, unlike
Schedules; inlined helper; #238 deferral).
Verified: web vitest 587 passed, eslint clean, tsc/vite build clean,
check:api no drift. #212 empty-lineup bare-create success+failure coverage
preserved. #238 TopBar dead-button left as-is (its owned bug; shell redesign
is epic phase 4 / #247).
refs #244#243
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex re-review of the prior fix commit found the Edit gate closed the exact
repro but two paths remained. One is reachable: Delete was the only
schedule-switch path that guardedSwitch's `saving` guard didn't cover — deleting
mid-save runs applySwitch to the next schedule while the in-flight items PUT is
still outstanding, and that PUT's completion handler then overwrites the next
schedule's draft with the deleted schedule's response. Delete is now
`disabled={saving}`, consistent with the Select, Edit, and guardedSwitch.
Regression: the deferred-PUT test now also asserts Delete is disabled in-flight
and re-enables after the save settles.
Also widens the test mock's onRequest return type to `Response | Promise<Response>
| null` (removes the `as unknown as Response` cast — a test-only type hole the
re-review flagged).
Deferred to #248: the other residual path (properties dialog not focus-trapped, so
keyboard focus can escape to underlying Add/Save mid-save) is a pre-existing,
cross-cutting overlay.tsx a11y gap affecting all dialogs — out of scope for this
targeted blocker fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two findings from the PR #242 adversarial review (Codex + Claude fork):
1. ChannelsEmptyState swallowed mutationError: on the fresh-install path #212
targets, a bare-create can 4xx (e.g. no default ffmpeg profile), but the empty
branch never rendered the error alert the non-empty screen shows — the user saw
only a spinner re-enable. The empty state now renders the same ctv-channels-error
alert. Regression: App.test.tsx asserts the error surfaces + no navigation on an
empty lineup.
2. SchedulesScreen Edit button was not gated on `saving` (Codex): during an items
save PUT the draft is still dirty, so a discard-to-open → shuffle-flip could let
the in-flight PUT resolve AFTER the shuffle reload and clobber the normalized
draft with the pre-shuffle body. Edit is now disabled while saving (consistent
with the schedule Select). Regression: a deferred PUT proves Edit is disabled
in-flight and re-enables once the save settles.
Deferred as nits (both reviews rate low): vetoed navigateToPath leaves two stray
history entries (cosmetic, rare — fixing means refactoring central nav); rapid
double-Back is best-effort (inherent popstate non-cancellability).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
Editing a schedule's properties to change shuffleScheduleItems for the active
schedule left the open items draft stale: hidden Fixed/Flood start values could
be saved back and the inspector kept offering start-type controls the schedule
no longer supports. onScheduleSaved now detects a shuffle flip on the active
schedule and reloads the items via GET so the server's EnforceProperties
re-normalizes the draft. The schedule-level flags already refresh from the save
response (setBoot maps `saved` into the list).
Decision: chose "block opening the edit dialog while the item draft is dirty"
(confirm-to-discard, guardedSwitch semantics) over confirm-at-reload — the
smaller fully-consistent change. It guarantees the properties editor only ever
opens over a clean baseline draft, so the post-save reload is lossless and
avoids the awkward state where a cancelled discard leaves a now-shuffled
schedule holding Fixed values.
Regression: SchedulesScreen.test.tsx — Fixed item on a non-shuffled schedule →
edit properties to shuffle=true → a fresh items GET fires, the Fixed option is
gone, and a subsequent Save's PUT carries no Fixed startType.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
timothy
