Defense in depth on the redirect detector: Uri.Equals compares normalized
components, so an escaping/casing difference can't be mistaken for a
redirect and fail CLOSED -- the exact failure the check exists to prevent.
A plex key can contain spaces or unicode.
Honest note: this is NOT a fix for an observed bug. I wrote a test claiming
to pin it, then ran the negative control and the test passed against the
string comparison too -- Uri.ToString() unescapes, so both forms agree for
our machine-generated URLs. The test was vacuous as written. It is kept,
retitled and re-commented to describe what it actually guards (an
un-redirected 404 on an escaping-sensitive url fails open), and the code
comment says plainly that this is defense in depth rather than a repair.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of PR #479 found the stated fail-open contract was not
what the code measured, plus four smaller gaps. All fixed here as a
follow-up commit (no amend/force-push).
High — a 404 from ErsatzTV's OWN endpoint was treated as "media gone".
/media/{provider}/... is served by InternalController, which returns
NotFound when the media source is unconfigured or momentarily missing
(a media-source edit that deletes+reinserts connections, a restore, a
partially-configured server). Probing for "any 404" therefore failed
CLOSED for every item on that source -- exactly the case the fail-open
contract exists to prevent. A media-server 404 always arrives after a
redirect, so an un-redirected 404 is now treated as available.
Medium — the new switch label was untested and its benefit overstated.
maybeDuration/finish are computed before the switch, so `default:`
already sized the error card to the next playout item; the label only
changes the caption. The handler test asserted call counts only, so
deleting the label still passed. It now asserts the error message, and
removing the label fails the test (verified).
Medium — Plex/Emby branches changed but had no coverage. Added an Emby
handler test asserting the probe is called with the emby URL.
Low — caller cancellation was swallowed and pinned as desired behaviour.
A shutdown / client disconnect is a genuine signal, not a probe failure;
it now propagates, and only the probe's own 2s timeout fails open.
Low — the response stream was disposed unread, aborting the connection
instead of returning it to the pool. The one requested byte is drained.
Nit — fully-qualified RangeHeaderValue replaced with a using.
docs/decisions.md corrected where it overstated: the switch label's role,
the "fixes the class for all three media servers" claim (external-JSON
channels bypass ValidatePlayoutItemPath entirely -- filed as #480), and
the unmeasured latency assertion. Deferred HEAD-instead-of-GET recorded
with its reason rather than silently dropped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tuning a channel intermittently hard-failed with ffmpeg exit 8 and
`Server returned 404 Not Found` on /media/jellyfin/{itemId}.
Root cause: ValidatePlayoutItemPath checked `File.Exists` on the local
branch, but the three media-server remote-stream branches returned
`http://localhost:{port}/media/{plex,jellyfin,emby}/{id}` unconditionally.
When the media was gone from the media server too, validation "succeeded"
and ffmpeg was launched against a URL that 404s.
That bypassed the good error path the handler already had
(PlayoutItemDoesNotExistOnDisk renders an error card sized to run until
the NEXT playout item, so the dead item is skipped) and instead landed in
HlsSessionWorker's generic ffmpeg-failure path, which sizes its error card
to the failed 44s work-ahead chunk and then re-selects the SAME broken
item -- a repeating error card for the item's whole slot (~22 min).
Restore the method's own invariant: every PlayoutItemWithPath it returns
has been checked for existence. A definitive 404 now returns the new
PlayoutItemNotAvailableFromMediaServer error, handled in the same switch
arm as PlayoutItemDoesNotExistOnDisk.
The probe is deliberately fail-open: only a 404 reports the media gone.
A timeout, 5xx, auth error or transport failure reports available, so a
probe that cannot answer can never break a tune that would have worked.
That contract is pinned by tests so a later refactor cannot invert it.
Rejected alternatives (see docs/decisions.md): resizing the
HlsSessionWorker retry loop (cannot distinguish a dead item from a
transient transcoder failure -- prod has live VAAPI hwupload -22 failures
that must keep retrying), and writing MediaItemState from the streaming
path (breaks scanner ownership, and would not have fixed this: the item
is RemoteOnly, which PlayoutBuilder's skip does not exclude).
Scanner-side follow-ups filed separately: #476 (FileNotFound does not
cascade show -> episodes, the reason dead items keep being scheduled),
fixes#473
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
timothyandClaude Opus 4.8