fix(536): enforce workAheadSegmenterLimit with an atomic slot claim #538

Merged

timothy merged 2 commits from fix/536-workahead-slot-race into main 2026-07-21 18:44:12 +02:00

Conversation 1 Commits 2 Files Changed 5

+359 -9

timothy commented 2026-07-21 18:23:48 +02:00

Owner

Copy Link

Copy Source

fixes #536

Root cause

Not a missing Interlocked — a structural check-then-act. The write side already used Interlocked.Increment, which is why the code read as thread-safe. The read side was a separate, earlier load in Run:

bool initialWorkAhead = Volatile.Read(ref _workAheadCount) < await GetWorkAheadLimit(cancellationToken);

with the increment happening later inside Transcode, and at least one await (a DB-backed config read) in between. Every simultaneous tune-in observed 0 < limit and took the unthrottled path — three concurrent tunes on prod with workAheadSegmenterLimit: 1 all ran with no -readrate. Same class as #231/#250.

The guarantee failed in the one scenario it exists for, and the qsvExtraHardwareFrames floor from #529 is sized on the assumption that it holds.

Change

• New ErsatzTV.Application/Streaming/WorkAheadSlots.cs — TryAcquire(limit) claims via compare-exchange, Release() returns. Extracted rather than patched in place because the static int was untestable and the invariant deserves one documented home.
• Run claims; Transcode releases. Transcode(bool ownsWorkAheadSlot, …) derives realtime = !ownsWorkAheadSlot on its first line and releases in its existing finally, so acquire/release stay one-for-one with a single release site. Acquisition deliberately stays in the caller: Run sets _state from the outcome and Transcode reads that state on entry (wasSeekAndWorkAhead) to choose the item start time — folding acquisition inward would have silently flipped that branch. Inverting the parameter also removes the old Transcode(!initialWorkAhead, …) double negative.
• CAS rather than the issue's suggested increment-then-back-off. Both are correct on holder count, but increment-first transiently overshoots, so a concurrent reader can observe a count above the limit — undesirable when the QSV pool sizing is derived from that bound.
• Unbalanced Release clamps at zero and records UnbalancedReleases instead of throwing (the sole caller releases from a finally, where a throw would swallow the real exception). The pool is process-wide and never recreated, so a -1 would permanently admit an extra unthrottled transcode with nothing in the logs to find it by. Added from review.

Verification

• Full .NET suite green locally (all 7 projects) before the push; dotnet format --verify-no-changes exit 0; no BOMs.
• Negative control, per the #231/#250 lesson. A single Barrier(N) + Task.WhenAll round does not collide on this hardware, so the tests hammer 8 threads × 20 000 rounds with a per-round barrier. Reinstating the pre-fix check-then-act body fails TryAcquire_ParallelCallers_NeverExceedsLimit with 15 912 over-claiming rounds of 20 000 (4 of 9 tests red). Restored with an explicit touch + rebuild — a cp restore alone preserves the old mtime, MSBuild skips the rebuild, and the control then "passes" on a stale dll. Build output grepped for error CS, not just a green tail.
• Independent adversarial review (cold context, different model family) before the push — verdict MERGEABLE. It traced every return/throw/early-exit between acquisition and release, verified both call sites are exact negations of the previous booleans, confirmed HlsSessionWorker is the only IHlsSessionWorker implementation and the only consumer of the budget, and independently replicated the harness (8 966/20 000 bad rounds against the buggy shape — same order as ours). Its one substantive finding is the Release clamp, folded in above.

Docs

• New decisions.md record ffmpeg.work-ahead-slot-atomic; catalog regenerated, decisions_validate.py OK.
• Correction on the #350 record ([decisions-edit] in the first commit): its bullet "every concurrent tune-in falls back to the throttled path" described the intent, not the behaviour — the "one winner, two throttled" observation held only because those tunes were effectively staggered. The measurement and its conclusions stand; the guarantee simply wasn't enforced.

No API/route/SPA surface changed, so no OpenAPI regen or parity-doc update is owed.

Not in this PR

The issue's "verify with N simultaneous tune-ins" box cannot be satisfied before an image carrying this change is deployed, so it was split out of ## Done-when into a tracked post-deploy section rather than left as an unsatisfiable merge gate. It is still owed — #536 stays open until it's reported against ersatztv-test.

fixes #536 ## Root cause Not a missing `Interlocked` — a structural check-then-act. The write side already used `Interlocked.Increment`, which is why the code read as thread-safe. The read side was a separate, earlier load in `Run`: ```csharp bool initialWorkAhead = Volatile.Read(ref _workAheadCount) < await GetWorkAheadLimit(cancellationToken); ``` with the increment happening later inside `Transcode`, and at least one `await` (a DB-backed config read) in between. Every simultaneous tune-in observed `0 < limit` and took the unthrottled path — three concurrent tunes on prod with `workAheadSegmenterLimit: 1` all ran with no `-readrate`. Same class as #231/#250. The guarantee failed in the one scenario it exists for, and the `qsvExtraHardwareFrames` floor from #529 is sized on the assumption that it holds. ## Change - **New `ErsatzTV.Application/Streaming/WorkAheadSlots.cs`** — `TryAcquire(limit)` claims via compare-exchange, `Release()` returns. Extracted rather than patched in place because the static `int` was untestable and the invariant deserves one documented home. - **`Run` claims; `Transcode` releases.** `Transcode(bool ownsWorkAheadSlot, …)` derives `realtime = !ownsWorkAheadSlot` on its first line and releases in its existing `finally`, so acquire/release stay one-for-one with a single release site. Acquisition deliberately stays in the caller: `Run` sets `_state` from the outcome and `Transcode` reads that state on entry (`wasSeekAndWorkAhead`) to choose the item start time — folding acquisition inward would have silently flipped that branch. Inverting the parameter also removes the old `Transcode(!initialWorkAhead, …)` double negative. - **CAS rather than the issue's suggested increment-then-back-off.** Both are correct on holder count, but increment-first transiently overshoots, so a concurrent reader can observe a count above the limit — undesirable when the QSV pool sizing is derived from that bound. - **Unbalanced `Release` clamps at zero** and records `UnbalancedReleases` instead of throwing (the sole caller releases from a `finally`, where a throw would swallow the real exception). The pool is process-wide and never recreated, so a `-1` would permanently admit an extra unthrottled transcode with nothing in the logs to find it by. Added from review. ## Verification - Full .NET suite green locally (all 7 projects) before the push; `dotnet format --verify-no-changes` exit 0; no BOMs. - **Negative control, per the #231/#250 lesson.** A single `Barrier(N)` + `Task.WhenAll` round does not collide on this hardware, so the tests hammer 8 threads × 20 000 rounds with a per-round barrier. Reinstating the pre-fix check-then-act body fails `TryAcquire_ParallelCallers_NeverExceedsLimit` with **15 912 over-claiming rounds of 20 000** (4 of 9 tests red). Restored with an explicit `touch` + rebuild — a `cp` restore alone preserves the old mtime, MSBuild skips the rebuild, and the control then "passes" on a stale dll. Build output grepped for `error CS`, not just a green tail. - **Independent adversarial review (cold context, different model family) before the push — verdict MERGEABLE.** It traced every return/throw/early-exit between acquisition and release, verified both call sites are exact negations of the previous booleans, confirmed `HlsSessionWorker` is the only `IHlsSessionWorker` implementation and the only consumer of the budget, and independently replicated the harness (8 966/20 000 bad rounds against the buggy shape — same order as ours). Its one substantive finding is the `Release` clamp, folded in above. ## Docs - New `decisions.md` record `ffmpeg.work-ahead-slot-atomic`; catalog regenerated, `decisions_validate.py` OK. - **Correction on the #350 record** (`[decisions-edit]` in the first commit): its bullet "every concurrent tune-in falls back to the throttled path" described the intent, not the behaviour — the "one winner, two throttled" observation held only because those tunes were effectively staggered. The measurement and its conclusions stand; the guarantee simply wasn't enforced. No API/route/SPA surface changed, so no OpenAPI regen or parity-doc update is owed. ## Not in this PR The issue's "verify with N simultaneous tune-ins" box cannot be satisfied before an image carrying this change is deployed, so it was split out of `## Done-when` into a tracked post-deploy section rather than left as an unsatisfiable merge gate. It is still owed — #536 stays open until it's reported against `ersatztv-test`.

timothy added 2 commits 2026-07-21 18:23:49 +02:00

fix(536): enforce workAheadSegmenterLimit with an atomic slot claim [decisions-edit] ... c0d3dab190

The slot check and its increment straddled an await: `Run` compared
`Volatile.Read(ref _workAheadCount)` against a DB-backed limit, and the
increment happened later inside `Transcode`. Every simultaneous tune-in
therefore observed `0 < limit` and started unthrottled — three concurrent
tunes on prod with a limit of 1 all ran with no `-readrate`. `Interlocked`
on the write side alone buys nothing when the read side is a separate,
earlier load (same class as #231/#250).

Extract the counter into a `WorkAheadSlots` pool whose `TryAcquire(limit)`
claims via compare-exchange, so the count never even transiently exceeds
the limit that the QSV hardware-frame pool sizing (#529) is derived from.
`Run` claims the slot and passes ownership in; `Transcode(bool
ownsWorkAheadSlot, ...)` derives `realtime` from it and releases it in its
existing `finally`, keeping acquire/release one-for-one. Acquisition stays
in the caller because `Run` sets `_state` from the outcome and `Transcode`
reads that state on entry to pick the item start time.

Tests hammer 8 threads x 20k rounds (a single Barrier round does not
collide on this hardware); the documented negative control reinstates the
check-then-act body and produces 15912 over-claiming rounds of 20000.

Also annotates the #350 decision record, whose "every concurrent tune-in
falls back to the throttled path" bullet described the intent rather than
the behaviour.

fixes #536

fix(536): clamp an unbalanced work-ahead release instead of going negative ... 5db0836d41

Pre-push adversarial review, Low finding. The pool is process-wide and
lives for the life of the app, so a `Release()` not matched by a
successful `TryAcquire` would drive the count to -1 permanently: with a
limit of 1 that silently admits two unthrottled transcodes forever, which
is exactly the #529 QSV pool exhaustion with nothing in the logs to find
it by. Clamp at zero and record the breakage in `UnbalancedReleases`
rather than throwing — the sole caller releases from a `finally`, where a
throw would swallow the real exception.

The hammer tests now also assert `UnbalancedReleases == 0`, so the clamp
cannot mask drift it was added to survive.

Also moves the #536 index line to the end of the in-file decisions index
(it was inserted in the 2026-07-11 block while its body appends at the
end) — review nit, anchors were already correct.

refs #536

timothy referenced this pull request 2026-07-21 18:27:25 +02:00

`WorkAheadSlots.Release` hardening: never go negative, and surface `UnbalancedReleases` somewhere a human will see it #539

timothy commented 2026-07-21 18:27:45 +02:00

Author

Owner

Copy Link

Copy Source

Two independent review passes, both cold-context and review-only, and between them they cover the whole head — not just the initial diff (the #242 lesson: the fix commit is code no reviewer has seen).

Pass 1 — full diff at c0d3dab1: MERGEABLE. Traced every return/throw/early exit between acquisition and release across all of HlsSessionWorker (not just the diff) and confirmed exactly one release site, one-for-one with each true passed in — no leak, no double-release, no release-without-acquire, including the mediator-error, error-process, cancellation and catch-all paths. Verified both call sites pass the exact negation of the previous booleans and that realtime is used identically at every one of its five use sites, with the wasSeekAndWorkAhead capture order unchanged. Confirmed HlsSessionWorker is the only IHlsSessionWorker implementation and the only consumer of the budget (the other FFmpegWorkAheadSegmenters readers are settings + troubleshooting display, neither starts a transcode). Independently replicated the hammer harness and measured 8 966/20 000 bad rounds against the pre-fix shape versus 0 against the fix — so the harness is proven non-vacuous on a second machine-run, same order as the 15 912 reported in the commit. Its one substantive finding (a negative count would inflate the budget permanently) is fixed in 5db0836d.

Pass 2 — scoped re-review of 5db0836d: MERGEABLE. Confirmed the clamp loop cannot livelock and, critically, cannot destroy a concurrent legitimate acquire — the CAS only writes 0 when it still observes the negative value it read, so any interleaved TryAcquire makes it fail and re-read. Confirmed the clamp cannot mask a leak (leaks leave the count positive, still caught by Count.ShouldBe(0)), that UnbalancedReleases can never over-count, that the new tests fail against every plausible broken clamp variant, and that the decisions.md index move is inert.

Three Low findings deferred to #539 rather than pushed here: a residual transient over-admission window that a never-negative Release would close outright, the counter's floor-triggered under-count when an over-release cancels a leak, and the fact that UnbalancedReleases still isn't surfaced in any log. All three require an unbalanced release to reach, which the single guarded release site makes unreachable today; the reviewer's own framing is that the current shape is strictly better than what it replaced (same window, minus the permanence). Not worth orphaning an in-flight CI run — pushes can't be cancelled on this Gitea.

Review-verdict: MERGEABLE @ 5db0836d41

Two independent review passes, both cold-context and review-only, and between them they cover the whole head — not just the initial diff (the #242 lesson: the fix commit is code no reviewer has seen). **Pass 1 — full diff at `c0d3dab1`: MERGEABLE.** Traced every `return`/`throw`/early exit between acquisition and release across all of `HlsSessionWorker` (not just the diff) and confirmed exactly one release site, one-for-one with each `true` passed in — no leak, no double-release, no release-without-acquire, including the mediator-error, error-process, cancellation and catch-all paths. Verified both call sites pass the exact negation of the previous booleans and that `realtime` is used identically at every one of its five use sites, with the `wasSeekAndWorkAhead` capture order unchanged. Confirmed `HlsSessionWorker` is the only `IHlsSessionWorker` implementation and the only consumer of the budget (the other `FFmpegWorkAheadSegmenters` readers are settings + troubleshooting display, neither starts a transcode). Independently replicated the hammer harness and measured **8 966/20 000** bad rounds against the pre-fix shape versus 0 against the fix — so the harness is proven non-vacuous on a second machine-run, same order as the 15 912 reported in the commit. Its one substantive finding (a negative count would inflate the budget permanently) is fixed in `5db0836d`. **Pass 2 — scoped re-review of `5db0836d`: MERGEABLE.** Confirmed the clamp loop cannot livelock and, critically, **cannot destroy a concurrent legitimate acquire** — the CAS only writes 0 when it still observes the negative value it read, so any interleaved `TryAcquire` makes it fail and re-read. Confirmed the clamp cannot mask a *leak* (leaks leave the count positive, still caught by `Count.ShouldBe(0)`), that `UnbalancedReleases` can never over-count, that the new tests fail against every plausible broken clamp variant, and that the decisions.md index move is inert. Three **Low** findings deferred to **#539** rather than pushed here: a residual transient over-admission window that a never-negative `Release` would close outright, the counter's floor-triggered under-count when an over-release cancels a leak, and the fact that `UnbalancedReleases` still isn't surfaced in any log. All three require an unbalanced release to reach, which the single guarded release site makes unreachable today; the reviewer's own framing is that the current shape is strictly better than what it replaced (same window, minus the permanence). Not worth orphaning an in-flight CI run — pushes can't be cancelled on this Gitea. Review-verdict: MERGEABLE @ 5db0836d41714cef7e42080e9c31429d27a08546

timothy merged commit 5e5da9dbd1 into main 2026-07-21 18:44:12 +02:00

timothy deleted branch fix/536-workahead-slot-race 2026-07-21 18:44:13 +02:00

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

Merge pull request 'fix(536): enforce workAheadSegmenterLimit with an atomic slot claim' (#538)

timothy referenced this pull request 2026-07-21 19:09:43 +02:00

`workAheadSegmenterLimit` is not enforced: the slot check and its increment straddle an `await` (N concurrent tune-ins all run unthrottled) #536

timothy referenced this pull request 2026-07-21 19:21:36 +02:00

`workAheadSegmenterLimit` is not enforced: the slot check and its increment straddle an `await` (N concurrent tune-ins all run unthrottled) #536

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#538