fix(473): probe media-server remote streams before handing the URL to ffmpeg #479

Merged

timothy merged 5 commits from fix/473-missing-media-resilience into main 2026-07-19 22:24:59 +02:00

Conversation 1 Commits 5 Files Changed 10

+787 -10

timothy commented 2026-07-19 21:06:35 +02:00

Owner

Copy Link

Copy Source

fixes #473

The bug

Tuning a channel intermittently hard-failed with ffmpeg exit 8 and Server returned 404 Not Found on /media/jellyfin/{itemId}.

Root cause

GetPlayoutItemProcessByChannelNumberHandler.ValidatePlayoutItemPath checked _fileSystem.File.Exists(path) 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 mattered because it bypassed the good error path the handler already had: PlayoutItemDoesNotExistOnDisk renders an error card sized to run until the next playout item, i.e. the dead item is skipped and playback resumes. Instead the failure 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 entire slot (~22 min for an episode). To a viewer that is a dead channel, which is what was reported.

The fix

Restore the method's own invariant — every PlayoutItemWithPath it returns has been checked for existence — via a new IRemoteStreamProber seam (interface in Core, HttpRemoteStreamProber in Infrastructure). A definitive 404 returns the new PlayoutItemNotAvailableFromMediaServer error, handled in the same switch arm as PlayoutItemDoesNotExistOnDisk. Applied to all three remote branches (Plex, Jellyfin, Emby), so this fixes the class rather than the reported instance.

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 otherwise have worked. This is the load-bearing property and is pinned by tests (Should_Fail_Open_*) precisely so a later refactor cannot silently invert it. GET with Range: bytes=0-0 + ResponseHeadersRead, 2s timeout, redirects followed so the probe exercises the exact chain ffmpeg will.

No config toggle: the behaviour strictly dominates the status quo, and one LAN round trip bounded at 2s is noise against the ~7s cold start #350 is about.

Rejected alternatives

Recorded in docs/decisions.md with reasons:

• Resizing the HlsSessionWorker retry loop to skip the item on any ffmpeg failure — it cannot distinguish "this item is dead" from "this transcoder hiccupped", and prod has live transient VAAPI hwupload -22 failures (now #478) that must keep retrying. Converting those into whole-item blackouts is a regression. The retry loop is left intact and remains the backstop for the probe's TOCTOU window.
• Writing MediaItemState from the streaming path — breaks scanner ownership of State, and would not have fixed this anyway: the failing item is RemoteOnly, which PlayoutBuilder's PlayoutSkipMissingItems skip does not exclude. On Jellyfin, RemoteOnly is the normal state for every item when path replacement isn't configured, so it carries no signal about playability.

Scope split

Recon showed part (a) of the issue is not content hygiene as originally triaged — it is a scanner cascade bug, filed as #476 (a show deleted upstream is flagged FileNotFound but its seasons/episodes are never swept, so playouts keep scheduling them — evidenced on prod: show row State=1, all 110 of its episodes State=3). Also filed: #477 (no anti-nuke guard on the reconciliation sweep) and #478 (prod VAAPI failures). None are in scope here.

Verification

• Full .NET gate green: 3,885 passed / 0 failed across all six test projects (FFmpeg 20, Architecture 5, Scanner 1473, Core 589, Infrastructure 64, Tests 1734).
• Negative controls run, both non-vacuous. Inverting the 404 check in the prober fails exactly Should_Report_Unavailable_On_404; inverting the Jellyfin availability condition in the handler fails both handler tests. Both controls verified against a clean Build succeeded (no error CS), and re-verified after restore — a first attempt produced a false pass from a stale dll because a restore preserved the old mtime.
• dotnet format whitespace . --folder --verify-no-changes exits 0; the legacy UTF-8 BOM on the touched handler was stripped.

Live-E2E was not run: this changes a server-side validation path with no UI surface, and the behaviour it produces (the error card) is the pre-existing, already-exercised PlayoutItemDoesNotExistOnDisk path. The new logic is covered by handler-level tests that assert ForError vs ForPlayoutItem dispatch.

Files

File	Change
ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs	new seam
ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs	new error
ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs	fail-open HTTP probe
ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs	probe all 3 remote branches; handle the new error
ErsatzTV/Startup.cs	DI registration
ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs	11 tests, incl. the fail-open contract
ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs	2 tests, error-card vs remote-stream dispatch
docs/decisions.md	decision + rejected alternatives

🤖 Generated with Claude Code

fixes #473 ## The bug Tuning a channel intermittently hard-failed with ffmpeg **exit 8** and `Server returned 404 Not Found` on `/media/jellyfin/{itemId}`. ## Root cause `GetPlayoutItemProcessByChannelNumberHandler.ValidatePlayoutItemPath` checked `_fileSystem.File.Exists(path)` 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 mattered because it bypassed the good error path the handler **already had**: `PlayoutItemDoesNotExistOnDisk` renders an error card sized to run until the **next playout item**, i.e. the dead item is skipped and playback resumes. Instead the failure 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 entire slot (~22 min for an episode). To a viewer that is a dead channel, which is what was reported. ## The fix Restore the method's own invariant — every `PlayoutItemWithPath` it returns has been checked for existence — via a new `IRemoteStreamProber` seam (interface in Core, `HttpRemoteStreamProber` in Infrastructure). A definitive **404** returns the new `PlayoutItemNotAvailableFromMediaServer` error, handled in the same `switch` arm as `PlayoutItemDoesNotExistOnDisk`. Applied to **all three** remote branches (Plex, Jellyfin, Emby), so this fixes the class rather than the reported instance. **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 otherwise have worked. This is the load-bearing property and is pinned by tests (`Should_Fail_Open_*`) precisely so a later refactor cannot silently invert it. `GET` with `Range: bytes=0-0` + `ResponseHeadersRead`, 2s timeout, redirects followed so the probe exercises the exact chain ffmpeg will. No config toggle: the behaviour strictly dominates the status quo, and one LAN round trip bounded at 2s is noise against the ~7s cold start #350 is about. ## Rejected alternatives Recorded in `docs/decisions.md` with reasons: - **Resizing the `HlsSessionWorker` retry loop** to skip the item on any ffmpeg failure — it cannot distinguish "this item is dead" from "this transcoder hiccupped", and prod has live *transient* VAAPI `hwupload -22` failures (now #478) that must keep retrying. Converting those into whole-item blackouts is a regression. The retry loop is left intact and remains the backstop for the probe's TOCTOU window. - **Writing `MediaItemState` from the streaming path** — breaks scanner ownership of `State`, and would not have fixed this anyway: the failing item is `RemoteOnly`, which `PlayoutBuilder`'s `PlayoutSkipMissingItems` skip does not exclude. On Jellyfin, `RemoteOnly` is the *normal* state for every item when path replacement isn't configured, so it carries no signal about playability. ## Scope split Recon showed part (a) of the issue is **not** content hygiene as originally triaged — it is a scanner cascade bug, filed as **#476** (a show deleted upstream is flagged `FileNotFound` but its seasons/episodes are never swept, so playouts keep scheduling them — evidenced on prod: show row `State=1`, all 110 of its episodes `State=3`). Also filed: **#477** (no anti-nuke guard on the reconciliation sweep) and **#478** (prod VAAPI failures). None are in scope here. ## Verification - Full .NET gate green: **3,885 passed / 0 failed** across all six test projects (FFmpeg 20, Architecture 5, Scanner 1473, Core 589, Infrastructure 64, Tests 1734). - **Negative controls run, both non-vacuous.** Inverting the 404 check in the prober fails exactly `Should_Report_Unavailable_On_404`; inverting the Jellyfin availability condition in the handler fails both handler tests. Both controls verified against a clean `Build succeeded` (no `error CS`), and re-verified after restore — a first attempt produced a false pass from a stale dll because a restore preserved the old mtime. - `dotnet format whitespace . --folder --verify-no-changes` exits 0; the legacy UTF-8 BOM on the touched handler was stripped. Live-E2E was not run: this changes a server-side validation path with no UI surface, and the behaviour it produces (the error card) is the pre-existing, already-exercised `PlayoutItemDoesNotExistOnDisk` path. The new logic is covered by handler-level tests that assert `ForError` vs `ForPlayoutItem` dispatch. ## Files | File | Change | |---|---| | `ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs` | new seam | | `ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs` | new error | | `ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs` | fail-open HTTP probe | | `ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs` | probe all 3 remote branches; handle the new error | | `ErsatzTV/Startup.cs` | DI registration | | `ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs` | 11 tests, incl. the fail-open contract | | `ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs` | 2 tests, error-card vs remote-stream dispatch | | `docs/decisions.md` | decision + rejected alternatives | 🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

External-JSON playout channels still hand ffmpeg an unprobed remote-stream URL (#473 class survives there) #480

timothy referenced this issue from a commit 2026-07-19 21:27:17 +02:00

fix(473): review fixes — only a redirected 404 fails closed

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

Tune-in hard-fails (ffmpeg exit 8) when a playout item's media file no longer exists — 717 stale episodes across 10 removed shows on prod #473

timothy force-pushed fix/473-missing-media-resilience from 02e748e080 to dc5ceb5a14 2026-07-19 21:47:58 +02:00 Compare

timothy referenced this issue from a commit 2026-07-19 21:47:59 +02:00

fix(473): review fixes — only a redirected 404 fails closed

timothy added 1 commit 2026-07-19 22:02:20 +02:00

fix(473): gate the cancellation filter on the caller's token ... 1eff0b39b7

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>

timothy commented 2026-07-19 22:12:40 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 1eff0b39b7

Cold-context adversarial review, three rounds (the first two returned findings; round two was BLOCKED). Each round re-reviewed the fix commit, not just the original diff.

Round 1 — 1 High, 4 Medium/Low

• High: the "fail-open" contract was false. Probing for any 404 failed closed when ErsatzTV's own InternalController returned NotFound (unconfigured / momentarily missing media source), blanking every item on that source. Now only a redirected 404 counts.
• Scope claim corrected — ExternalJsonPlayoutItemProvider bypasses ValidatePlayoutItemPath entirely → filed #480 rather than claiming a fix not delivered.
• The new case label was dead weight: default: already sized the error card to the next playout item, so the label only changes the caption — and the test asserted call counts, so deleting it still passed. Test now asserts the message; verified that removing the label fails it.
• Caller cancellation was swallowed and pinned by a test as desired; now propagates. Added Emby-branch coverage. Response body drained. Nits.

Round 2 — BLOCKED, both findings introduced by my own round-1 fixes

• Blocker: the drain added for "return the connection to the pool" used ReadAsByteArrayAsync, which buffers the whole body. A server ignoring Range: bytes=0-0 answers 200 with the entire file → download at line rate into memory on the streaming hot path, defeating ResponseHeadersRead. Strictly worse than the abort it replaced. Now reads exactly one byte, on 206 only; pinned by a 64 MB TrackingStream test asserting BytesRead == 0, verified non-vacuous.
• Blocker: IRemoteStreamProber's contract still documented pre-fix behaviour — I had reported it updated when only the implementation's <remarks> was. Both false clauses fixed; throwing contract documented.
• Its own round-1 rethrow request reached HlsSessionWorker's catch-all → spurious channel-level ERROR on every graceful teardown. Fixed.

Round 3 — MERGEABLE WITH NITS → MERGEABLE

• Took finding A: the cancellation filter lacked a token check, so HttpClient's own-timeout TaskCanceledException (from ffprobe, media-server calls, subtitle extraction) was downgraded from ERROR+stack-trace to a routine Information line. Behaviour unchanged, but fault signal lost. Now gated on cancellationToken.IsCancellationRequested; reviewer explicitly parsed the mixed pattern-or / && precedence rather than trusting compilation.
• Declined its optional "drain to EOF" suggestion — reading exactly one byte is what makes the bound hold by construction against a 206 wider than requested. Reviewer withdrew the nit and agreed: the invariant "never reads more than one byte, ever" survives future edits better than "drain, but only when small".

Verification

• Full .NET gate: 3,889 passed / 0 failed across all six projects.
• Negative controls, all confirmed non-vacuous: inverting the prober's 404 check; inverting the handler's availability condition; deleting the case label; restoring the unbounded drain. Each verified against a clean Build succeeded — one earlier control produced a false pass from a stale dll (mtime-preserving restore), which is why each is re-verified after restore.
• Format gate exit 0; no BOMs on the 8 touched .cs.

Open follow-ups, both deliberately scoped out: #480 (external-JSON path) and Plex-branch handler coverage.

Review-verdict: MERGEABLE @ 1eff0b39b74efbd3e24f8a79998211feea035da4 Cold-context adversarial review, **three rounds** (the first two returned findings; round two was `BLOCKED`). Each round re-reviewed the *fix commit*, not just the original diff. ### Round 1 — 1 High, 4 Medium/Low - **High**: the "fail-open" contract was false. Probing for *any* 404 failed **closed** when ErsatzTV's own `InternalController` returned `NotFound` (unconfigured / momentarily missing media source), blanking every item on that source. Now only a **redirected** 404 counts. - Scope claim corrected — `ExternalJsonPlayoutItemProvider` bypasses `ValidatePlayoutItemPath` entirely → filed **#480** rather than claiming a fix not delivered. - The new `case` label was dead weight: `default:` already sized the error card to the next playout item, so the label only changes the caption — and the test asserted call counts, so deleting it still passed. Test now asserts the message; verified that removing the label fails it. - Caller cancellation was swallowed *and pinned by a test as desired*; now propagates. Added Emby-branch coverage. Response body drained. Nits. ### Round 2 — BLOCKED, both findings introduced by my own round-1 fixes - **Blocker**: the drain added for "return the connection to the pool" used `ReadAsByteArrayAsync`, which buffers the **whole body**. A server ignoring `Range: bytes=0-0` answers 200 with the entire file → download at line rate into memory on the streaming hot path, defeating `ResponseHeadersRead`. Strictly worse than the abort it replaced. Now reads **exactly one byte, on 206 only**; pinned by a 64 MB `TrackingStream` test asserting `BytesRead == 0`, verified non-vacuous. - **Blocker**: `IRemoteStreamProber`'s contract still documented pre-fix behaviour — I had reported it updated when only the *implementation's* `<remarks>` was. Both false clauses fixed; throwing contract documented. - Its own round-1 rethrow request reached `HlsSessionWorker`'s catch-all → spurious channel-level ERROR on every graceful teardown. Fixed. ### Round 3 — MERGEABLE WITH NITS → MERGEABLE - Took finding A: the cancellation filter lacked a token check, so `HttpClient`'s **own-timeout** `TaskCanceledException` (from ffprobe, media-server calls, subtitle extraction) was downgraded from ERROR+stack-trace to a routine Information line. Behaviour unchanged, but fault signal lost. Now gated on `cancellationToken.IsCancellationRequested`; reviewer explicitly parsed the mixed pattern-`or` / `&&` precedence rather than trusting compilation. - Declined its optional "drain to EOF" suggestion — reading *exactly one byte* is what makes the bound hold **by construction** against a 206 wider than requested. Reviewer withdrew the nit and agreed: the invariant "never reads more than one byte, ever" survives future edits better than "drain, but only when small". ### Verification - Full .NET gate: **3,889 passed / 0 failed** across all six projects. - **Negative controls, all confirmed non-vacuous**: inverting the prober's 404 check; inverting the handler's availability condition; deleting the `case` label; restoring the unbounded drain. Each verified against a clean `Build succeeded` — one earlier control produced a false pass from a stale dll (mtime-preserving restore), which is why each is re-verified after restore. - Format gate exit 0; no BOMs on the 8 touched `.cs`. Open follow-ups, both deliberately scoped out: **#480** (external-JSON path) and Plex-branch handler coverage.

timothy merged commit de11eb01b3 into main 2026-07-19 22:24:59 +02:00

timothy deleted branch fix/473-missing-media-resilience 2026-07-19 22:25:00 +02:00

timothy referenced this issue from a commit 2026-07-19 22:25:00 +02:00

Merge pull request 'fix(473): probe media-server remote streams before handing the URL to ffmpeg' (#479) from fix/473-missing-media-resilience into main

timothy referenced this pull request 2026-07-19 22:25:30 +02:00

Tune-in hard-fails (ffmpeg exit 8) when a playout item's media file no longer exists — 717 stale episodes across 10 removed shows on prod #473

timothy referenced this pull request 2026-07-19 23:54:42 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this pull request 2026-07-20 00:04:07 +02:00

External-JSON playout channels still hand ffmpeg an unprobed remote-stream URL (#473 class survives there) #480

timothy referenced this issue from a commit 2026-07-20 00:17:17 +02:00

fix(480): probe external-JSON remote-stream URLs before handing them to ffmpeg

timothy referenced this pull request 2026-07-20 00:17:32 +02:00

fix(480): probe external-JSON remote-stream URLs before ffmpeg #486

timothy referenced this pull request 2026-07-20 00:37:58 +02:00

External-JSON playout channels still hand ffmpeg an unprobed remote-stream URL (#473 class survives there) #480

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