Wire the frozen ETag/If-Match/412 recipe (Block reference implementation)
onto the Template and DecoTemplate aggregates:
- ReplaceTemplateItems / ReplaceDecoTemplateItems commands gain
Option<int> ExpectedVersion; ToCommand() on the request DTOs threads it
through from If-Match.
- Handlers introduce the version check as a standalone Either after
validation (never via Apply), bump Version unconditionally before
saving, and persist through SaveChangesWithConcurrencyGuard so a losing
writer maps to 412 instead of 500. DecoTemplate's post-commit playout
Reset enqueue now only runs after a successful save.
- TemplateViewModel / DecoTemplateViewModel carry Version (header-only,
not echoed in the response body), populated in Mapper.
- TemplateController / DecoTemplateController: GET items emits a strong
ETag of the root's version; PUT parses If-Match (400 on malformed),
threads the expected version into the command, and returns the new
ETag from the refreshed root on success. Both PUT actions now use the
handler's returned item list directly instead of re-querying items.
- SPA: templates.ts / decoTemplates.ts gain getXItemsWithMeta and an
If-Match-aware replaceX; TemplateEditor / DecoTemplateEditor hold the
ETag in a ref, read items-with-meta first on load, and open a
"changed elsewhere" ConfirmDialog on a 412 instead of navigating away.
Tests: new ReplaceTemplateItemsHandlerConcurrencyTests /
ReplaceDecoTemplateItemsHandlerConcurrencyTests mirror the Block
concurrency contract tests (stale/matching/absent If-Match, no-op bump,
racing-save 412, non-vacuous backstop). TemplateControllerTests /
DecoTemplateControllerTests gain ETag/If-Match/412 coverage.
TemplatesScreen.test.tsx / DecoTemplatesScreen.test.tsx gain a 412
conflict-dialog test mirroring BlocksScreen's.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fans the frozen ETag/If-Match/412 recipe (Block reference, #253) onto the
Playlist aggregate:
- ReplacePlaylistItems command carries ExpectedVersion; the handler runs
CheckVersion as a standalone Either after validation (so a stale write
survives as 412, not flattened to 422 by Apply/Join), bumps Version
unconditionally before saving, and persists via
SaveChangesWithConcurrencyGuard (EF concurrency-token backstop).
- PlaylistViewModel carries Version; the items GET sets a strong ETag and
the PUT parses If-Match, threads it into the command, and returns the
refreshed ETag on success (400 on a malformed If-Match).
- Sibling item-adding handlers (AddItemsToPlaylist, AddMovie/Episode/
Season/ShowToPlaylist) bump Version too, since they mutate the same
editor-visible item list.
- SPA: playlists.ts exposes getPlaylistItemsWithMeta and an
If-Match-aware updatePlaylist; PlaylistEditor holds the ETag in a ref,
round-trips it on save, and opens a "changed elsewhere" ConfirmDialog on
412 (mirrors BlockEditor).
Tests: new ReplacePlaylistItemsHandlerConcurrencyTests (stale/match/
force-write/no-op-bump/racing-save), new PlaylistController tests
(ETag on GET items, 400/412/thread-version/force-write on PUT), and a
vitest 412-conflict-dialog test for PlaylistsScreen. dotnet test:
1304/1304 green. web: npm run typecheck clean, npm run build clean,
vitest 664/664 green.
Ref #253 PR2.
Codex re-review of the fix commit confirmed both prior findings resolved and raised one
new Medium: RFC 7232 would 412 (not 400) a syntactically-valid but non-matching If-Match
(non-canonical "03", weak W/"3", tag lists, empty, overflow). Deferred to #197 (cold
contract pass) as #265 — fail-safe today (the mutation is rejected, never applied) and no
first-party client is affected. Records the deferral where the #253 fan-out will copy the
parser: a code comment in ConcurrencyHeaders + a note in api-conventions §7a.
Refs #253#265
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex independent review of #263 surfaced two defects the fork review missed:
- High — client load TOCTOU: BlockEditor read root metadata (getBlock) and items+ETag
(getBlockItemsWithMeta) concurrently, so a concurrent write landing between them (with
the items read resolving last) left a stale root paired with a current ETag → the save
silently overwrote the concurrent change with no 412. Fix: read items+ETag FIRST, then
the root metadata, so the captured ETag is never newer than the root version and any
inconsistency fails safe (save 412s → conflict dialog → reload).
- Medium — `ParseIfMatch` accepted non-canonical strong tags ("03", "+3", " 3 ") as
version 3. An ETag is opaque; only the exact emitted form is valid. Fix: canonical
decimal only (`NumberStyles.None` + no leading zeros) → else 400.
Tests: new `ConcurrencyHeadersTests` (canonical parse + padded/signed/whitespace/weak/
unquoted/list/overflow/empty → malformed); `ApiResultsTests` gains the 412 mapping case.
Existing BlocksScreen tests still green (load reordering is behavior-preserving for the
non-concurrent path).
Refs #253
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>
Adds the shared optimistic-concurrency contract so a stale second tab can no longer
silently overwrite a fresher edit. PR1 lands the infra + the Block reference aggregate;
PRs 2–4 fan the same recipe across the other 8 roots (design: #253#issuecomment-8472).
Contract
- `IVersionedAggregate` (`int Version`) on all 9 replace-all roots (ProgramSchedule,
Block, Template, DecoTemplate, Playlist, Collection, Playout, MultiCollection,
RerunCollection), EF-mapped `.IsConcurrencyToken()`; one dual-provider migration
`AddAggregateVersions` (nullable:false, default 0).
- Strong `ETag` of `Version` on the aggregate GET; `If-Match` on the PUT; mismatch →
412 (distinct from the §3a 409 build-lock guard). Successful PUT returns the new ETag.
- `PreconditionFailedError : BaseError` → 412 in `ApiResults.ToErrorResult`;
`ConcurrencyHeaders.ParseIfMatch/SetETag`; malformed If-Match → 400; `*`/absent =
Phase-1 force-write.
Block reference wiring
- Handler: standalone `Either` via `CheckVersion` AFTER validation (never through
`Apply`, which Join()-flattens the subtype to 422), unconditional `Version++`,
`SaveChangesWithConcurrencyGuard` backstop (DbUpdateConcurrencyException → 412).
- `BlockViewModel.Version` (header-only, not echoed in the body); controller sets the
ETag on GET items and on the successful PUT.
- SPA: `client.requestWithMeta` seam; `blocks.getBlockItemsWithMeta` + `replaceBlock`
If-Match/ETag round-trip; `BlockEditor` holds the ETag, sends If-Match, and on 412
opens a blocking "changed elsewhere — reload" dialog.
Tests
- Handler contract tests: stale-If-Match → 412 (no mutation), matching/absent → success
+ bump, no-op save still bumps, and a two-context racing save → 412; proven
non-vacuous (drop `.IsConcurrencyToken()` → the race test fails).
- Controller tests: malformed If-Match → 400, If-Match threaded to the command, ETag on
GET/PUT, 412 passthrough. SPA: requestWithMeta ETag, replaceBlock If-Match, 412 dialog.
Docs: api-conventions §7a, spa-conventions §4a, domain-model glossary, decisions log.
Refs #253
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>
timothy
