fix(473): gate the cancellation filter on the caller's token
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 5s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 6s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 5s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 16s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m17s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m10s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 18m28s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 5s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 6s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 5s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 16s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m17s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m10s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 18m28s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Third review pass: MERGEABLE WITH NITS. Taking the one finding it asked for before merge, plus a doc nit. The cancellation filter added last commit had no token check, and it spans the whole Transcode body -- including every mediator send (ffprobe via CliWrap, media-server API calls, subtitle extraction, song-video generation). TaskCanceledException is also what HttpClient throws on its OWN timeout, so a real timeout in any of those was being downgraded from an ERROR with a stack trace to a routine "Terminating HLS session" Information line. Behaviour was unchanged (both arms return false) but the fault signal was lost, and this repo has been bitten before by "empty log != the event didn't happen". Now filters on cancellationToken.IsCancellationRequested, so only genuine caller cancellation is treated as a graceful teardown. Doc nit: the <exception> block said cancellation "is thrown"; it is only thrown when the token trips while the probe is in flight -- cancelling after it completes returns normally. Now says "may propagate". Declined the reviewer's optional suggestion to drain until a 0-return instead of reading exactly one byte: reading exactly one byte is what makes the guard safe BY CONSTRUCTION, since a server or proxy that answers 206 with a wider range than requested still cannot be drained unboundedly. 206-only was confirmed correct rather than extended to short 200s, since deciding "short" from Content-Length would reopen the unbounded path for a chunked or Content-Length-less response. Also records the operator's standing rule in the handoff lore: a lone `decisions.md append-only` red is a known infra flake -- do not investigate, rebase, amend or push to clear it; the operator reruns that job from the UI. I violated this earlier in this PR with a tidy-but-wrong "my entry is no longer at EOF" theory, and the rebase did not fix it -- the job went red again on a verified pure-append diff, which is the proof the red was never about the diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -673,12 +673,16 @@ public class HlsSessionWorker : IHlsSessionWorker
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
catch (Exception ex) when (ex is TaskCanceledException or OperationCanceledException)
|
catch (Exception ex) when (ex is TaskCanceledException or OperationCanceledException
|
||||||
|
&& cancellationToken.IsCancellationRequested)
|
||||||
{
|
{
|
||||||
// a cancellation anywhere in this method (including inside the mediator sends, which sit
|
// a cancellation anywhere in this method (including inside the mediator sends, which sit
|
||||||
// outside the inner ffmpeg try below) is a shutdown or a client disconnect, not a fault.
|
// outside the inner ffmpeg try below) is a shutdown or a client disconnect, not a fault.
|
||||||
// Without this it reaches the catch-all and logs a channel-level ERROR with a stack
|
// Without this it reaches the catch-all and logs a channel-level ERROR with a stack
|
||||||
// trace on every graceful teardown. (ersatztv#473 review)
|
// trace on every graceful teardown. The token check is load-bearing: TaskCanceledException
|
||||||
|
// is also what HttpClient throws on ITS OWN timeout, and a real timeout inside ffprobe, a
|
||||||
|
// media-server call or subtitle extraction must keep its ERROR-level signal rather than
|
||||||
|
// being downgraded to a routine teardown. (ersatztv#473 review)
|
||||||
_logger.LogInformation("Terminating HLS session for channel {Channel}", _channelNumber);
|
_logger.LogInformation("Terminating HLS session for channel {Channel}", _channelNumber);
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
|
|||||||
@@ -18,10 +18,11 @@ public interface IRemoteStreamProber
|
|||||||
/// <c>true</c>, so a probe that cannot answer never prevents a tune that would have worked.
|
/// <c>true</c>, so a probe that cannot answer never prevents a tune that would have worked.
|
||||||
/// </returns>
|
/// </returns>
|
||||||
/// <exception cref="OperationCanceledException">
|
/// <exception cref="OperationCanceledException">
|
||||||
/// Thrown when <paramref name="cancellationToken" /> is cancelled. Caller cancellation is a
|
/// May propagate when <paramref name="cancellationToken" /> is cancelled while the probe is
|
||||||
/// genuine signal (shutdown / client disconnect), not a probe failure, so it propagates
|
/// in flight. Caller cancellation is a genuine signal (shutdown / client disconnect), not a
|
||||||
/// rather than being absorbed by the fail-open behaviour above. The prober's own internal
|
/// probe failure, so it is not absorbed by the fail-open behaviour above. Cancelling after
|
||||||
/// timeout does <em>not</em> throw — it fails open.
|
/// the probe has already completed returns normally. The prober's own internal timeout does
|
||||||
|
/// <em>not</em> throw — it fails open.
|
||||||
/// </exception>
|
/// </exception>
|
||||||
Task<bool> IsAvailable(string url, CancellationToken cancellationToken);
|
Task<bool> IsAvailable(string url, CancellationToken cancellationToken);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -249,6 +249,18 @@ HARD CONSTRAINTS:
|
|||||||
must supersede a live run, SAY SO explicitly instead of leaving it burning. (`dispatch_workflow` is
|
must supersede a live run, SAY SO explicitly instead of leaving it burning. (`dispatch_workflow` is
|
||||||
a different route and still works for re-triggering a **main** run.) Corrects the older "superseded
|
a different route and still works for re-triggering a **main** run.) Corrects the older "superseded
|
||||||
runs drain on their own" note below: they do finish, but they hold a slot while doing it.
|
runs drain on their own" note below: they do finish, but they hold a slot while doing it.
|
||||||
|
- **A lone `decisions.md append-only` red is a KNOWN INFRA FLAKE — do NOTHING** (operator-stated,
|
||||||
|
2026-07-19). When it is the **only** red: do not investigate it, and do **not** create a new run or
|
||||||
|
commit to clear it — no rebase, no `--amend`, no no-op push. **The operator reruns that single job
|
||||||
|
from the Gitea UI.** Report it as a known flake and carry on; only if *other* jobs are red too does
|
||||||
|
the run deserve diagnosis. Same family as the killed-job rule below (a spurious single-job red is
|
||||||
|
cleared by the operator's selective rerun, never by pushing — and pushes can't be cancelled, see the
|
||||||
|
batching constraint above). **The trap is that a convincing local explanation is always available.**
|
||||||
|
On #473/PR #479 the job went red just after `main` landed its own `decisions.md` entry, so "mine is
|
||||||
|
no longer at EOF, I must rebase" looked airtight — I rebased, and it went red **again** on a head
|
||||||
|
whose diff was a verified pure EOF append with zero deleted lines. A rebase that provably satisfied
|
||||||
|
the gate's stated rule did not turn it green, which is the proof the red was never about the diff.
|
||||||
|
**Check this rule before theorising about the cause.**
|
||||||
- **Do NOT gate or throttle your push on host health — trust the Gitea build queue.** The runners were
|
- **Do NOT gate or throttle your push on host health — trust the Gitea build queue.** The runners were
|
||||||
retuned for stability (operator, 2026-07-17); queueing is the queue's job, not yours. Don't SSH to
|
retuned for stability (operator, 2026-07-17); queueing is the queue's job, not yours. Don't SSH to
|
||||||
bumblebee to sample load/RAM before pushing, and don't hand-schedule around other sessions' runs. Batch
|
bumblebee to sample load/RAM before pushing, and don't hand-schedule around other sessions' runs. Batch
|
||||||
|
|||||||
Reference in New Issue
Block a user