From 90dc864ce5e71ed89118d07bbde7d4411eee16a2 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 21:05:56 +0200 Subject: [PATCH 1/5] fix(473): probe media-server remote streams before handing the URL to ffmpeg 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) --- ...layoutItemProcessByChannelNumberHandler.cs | 40 ++- .../PlayoutItemNotAvailableFromMediaServer.cs | 9 + .../Streaming/IRemoteStreamProber.cs | 17 ++ .../Streaming/HttpRemoteStreamProberTests.cs | 117 +++++++++ .../Streaming/HttpRemoteStreamProber.cs | 55 ++++ ...tItemProcessByChannelNumberHandlerTests.cs | 244 ++++++++++++++++++ ErsatzTV/Startup.cs | 1 + docs/decisions.md | 39 +++ 8 files changed, 512 insertions(+), 10 deletions(-) create mode 100644 ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs create mode 100644 ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs create mode 100644 ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs create mode 100644 ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs create mode 100644 ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs diff --git a/ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs b/ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs index b6184f4f1..10cebdbc2 100644 --- a/ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs +++ b/ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs @@ -1,4 +1,4 @@ -using System.IO.Abstractions; +using System.IO.Abstractions; using CliWrap; using Dapper; using ErsatzTV.Application.Playouts; @@ -42,6 +42,7 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< private readonly IGraphicsElementSelector _graphicsElementSelector; private readonly IDecoSelector _decoSelector; private readonly IPlexPathReplacementService _plexPathReplacementService; + private readonly IRemoteStreamProber _remoteStreamProber; private readonly ISongVideoGenerator _songVideoGenerator; private readonly ITelevisionRepository _televisionRepository; private readonly bool _isDebugNoSync; @@ -62,9 +63,11 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< IWatermarkSelector watermarkSelector, IGraphicsElementSelector graphicsElementSelector, IDecoSelector decoSelector, + IRemoteStreamProber remoteStreamProber, ILogger logger) : base(dbContextFactory) { + _remoteStreamProber = remoteStreamProber; _ffmpegProcessService = ffmpegProcessService; _fileSystem = fileSystem; _externalJsonPlayoutItemProvider = externalJsonPlayoutItemProvider; @@ -549,6 +552,7 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< Optional(channel.PlayoutOffset), !request.HlsRealtime); case PlayoutItemDoesNotExistOnDisk: + case PlayoutItemNotAvailableFromMediaServer: Command doesNotExistProcess = await _ffmpegProcessService.ForError( ffmpegPath, channel, @@ -850,9 +854,15 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< pmf.Path, pmf.Key); - return new PlayoutItemWithPath( - playoutItem, - $"http://localhost:{Settings.StreamingPort}/media/plex/{plexMediaSourceId}/{pmf.Key}"); + var plexUrl = + $"http://localhost:{Settings.StreamingPort}/media/plex/{plexMediaSourceId}/{pmf.Key}"; + + if (!await _remoteStreamProber.IsAvailable(plexUrl, cancellationToken)) + { + return new PlayoutItemNotAvailableFromMediaServer(plexUrl); + } + + return new PlayoutItemWithPath(playoutItem, plexUrl); } break; @@ -868,9 +878,14 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< foreach (string itemId in jellyfinItemId) { - return new PlayoutItemWithPath( - playoutItem, - $"http://localhost:{Settings.StreamingPort}/media/jellyfin/{itemId}"); + var jellyfinUrl = $"http://localhost:{Settings.StreamingPort}/media/jellyfin/{itemId}"; + + if (!await _remoteStreamProber.IsAvailable(jellyfinUrl, cancellationToken)) + { + return new PlayoutItemNotAvailableFromMediaServer(jellyfinUrl); + } + + return new PlayoutItemWithPath(playoutItem, jellyfinUrl); } // attempt to remotely stream emby @@ -883,9 +898,14 @@ public class GetPlayoutItemProcessByChannelNumberHandler : FFmpegProcessHandler< foreach (string itemId in embyItemId) { - return new PlayoutItemWithPath( - playoutItem, - $"http://localhost:{Settings.StreamingPort}/media/emby/{itemId}"); + var embyUrl = $"http://localhost:{Settings.StreamingPort}/media/emby/{itemId}"; + + if (!await _remoteStreamProber.IsAvailable(embyUrl, cancellationToken)) + { + return new PlayoutItemNotAvailableFromMediaServer(embyUrl); + } + + return new PlayoutItemWithPath(playoutItem, embyUrl); } return new PlayoutItemDoesNotExistOnDisk(path); diff --git a/ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs b/ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs new file mode 100644 index 000000000..40021e11f --- /dev/null +++ b/ErsatzTV.Core/Errors/PlayoutItemNotAvailableFromMediaServer.cs @@ -0,0 +1,9 @@ +namespace ErsatzTV.Core.Errors; + +public class PlayoutItemNotAvailableFromMediaServer : BaseError +{ + public PlayoutItemNotAvailableFromMediaServer(string url) : base( + $"Playout item is not available from media server\n{url}") + { + } +} diff --git a/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs new file mode 100644 index 000000000..3ba2cb94e --- /dev/null +++ b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs @@ -0,0 +1,17 @@ +namespace ErsatzTV.Core.Interfaces.Streaming; + +/// +/// Checks whether a media-server remote-stream URL still resolves to playable media. +/// +public interface IRemoteStreamProber +{ + /// + /// Probes (following redirects, as ffmpeg would). + /// + /// + /// false only when the media is definitively gone (HTTP 404). Every other outcome — + /// any other status code, a timeout, or a transport failure — returns true so that a + /// probe problem can never prevent a tune that would otherwise have succeeded (fail-open). + /// + Task IsAvailable(string url, CancellationToken cancellationToken); +} diff --git a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs new file mode 100644 index 000000000..49e769e79 --- /dev/null +++ b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs @@ -0,0 +1,117 @@ +using System.Net; +using ErsatzTV.Infrastructure.Streaming; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Infrastructure.Tests.Streaming; + +[TestFixture] +public class HttpRemoteStreamProberTests +{ + private const string Url = "http://localhost:8409/media/jellyfin/abc123"; + + [Test] + public async Task Should_Report_Unavailable_On_404() + { + HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.NotFound); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeFalse(); + } + + [TestCase(HttpStatusCode.OK)] + [TestCase(HttpStatusCode.PartialContent)] + [TestCase(HttpStatusCode.NoContent)] + public async Task Should_Report_Available_On_Success(HttpStatusCode statusCode) + { + HttpRemoteStreamProber prober = ProberReturning(statusCode); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + } + + // the fail-open contract: a probe that cannot answer must never block a tune that would + // otherwise have worked. these cases exist so a future refactor can't silently invert it. + [TestCase(HttpStatusCode.InternalServerError)] + [TestCase(HttpStatusCode.BadGateway)] + [TestCase(HttpStatusCode.Unauthorized)] + [TestCase(HttpStatusCode.Forbidden)] + public async Task Should_Fail_Open_On_Other_Status_Codes(HttpStatusCode statusCode) + { + HttpRemoteStreamProber prober = ProberReturning(statusCode); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + } + + [Test] + public async Task Should_Fail_Open_On_Transport_Failure() + { + var prober = new HttpRemoteStreamProber( + new StubHttpClientFactory(new ThrowingHttpMessageHandler(new HttpRequestException("no route to host"))), + Substitute.For>()); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + } + + [Test] + public async Task Should_Fail_Open_On_Timeout() + { + var prober = new HttpRemoteStreamProber( + new StubHttpClientFactory(new ThrowingHttpMessageHandler(new TaskCanceledException("timed out"))), + Substitute.For>()); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + } + + [Test] + public async Task Should_Fail_Open_When_Caller_Cancels() + { + HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.OK); + + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + bool result = await prober.IsAvailable(Url, cts.Token); + + result.ShouldBeTrue(); + } + + private static HttpRemoteStreamProber ProberReturning(HttpStatusCode statusCode) => + new( + new StubHttpClientFactory(new StatusCodeHttpMessageHandler(statusCode)), + Substitute.For>()); + + private sealed class StubHttpClientFactory(HttpMessageHandler handler) : IHttpClientFactory + { + public HttpClient CreateClient(string name) => new(handler, disposeHandler: false); + } + + private sealed class StatusCodeHttpMessageHandler(HttpStatusCode statusCode) : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + return Task.FromResult(new HttpResponseMessage(statusCode)); + } + } + + private sealed class ThrowingHttpMessageHandler(Exception exception) : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) => + Task.FromException(exception); + } +} diff --git a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs new file mode 100644 index 000000000..6ef74ec0e --- /dev/null +++ b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs @@ -0,0 +1,55 @@ +using System.Net; +using ErsatzTV.Core.Interfaces.Streaming; +using Microsoft.Extensions.Logging; + +namespace ErsatzTV.Infrastructure.Streaming; + +/// +/// Probes a media-server remote-stream URL over HTTP. +/// +/// +/// Deliberately fail-open: only a definitive HTTP 404 reports the media as gone. A probe that +/// times out, errors, or returns any other status must never turn a tune that would have worked +/// into an error card, so every other outcome reports available. (ersatztv#473) +/// +public class HttpRemoteStreamProber( + IHttpClientFactory httpClientFactory, + ILogger logger) : IRemoteStreamProber +{ + private static readonly TimeSpan ProbeTimeout = TimeSpan.FromSeconds(2); + + public async Task IsAvailable(string url, CancellationToken cancellationToken) + { + try + { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(ProbeTimeout); + + using var request = new HttpRequestMessage(HttpMethod.Get, url); + + // ask for a single byte; media servers vary in their HEAD support, and this exercises the + // same redirect chain ffmpeg will follow + request.Headers.Range = new System.Net.Http.Headers.RangeHeaderValue(0, 0); + + using HttpClient client = httpClientFactory.CreateClient(); + using HttpResponseMessage response = await client.SendAsync( + request, + HttpCompletionOption.ResponseHeadersRead, + timeoutCts.Token); + + if (response.StatusCode is HttpStatusCode.NotFound) + { + logger.LogWarning("Media server reported 404 for remote stream {Url}", url); + return false; + } + + return true; + } + catch (Exception ex) + { + // fail open - a probe failure is not evidence that the media is gone + logger.LogDebug(ex, "Unable to probe remote stream {Url}; assuming it is available", url); + return true; + } + } +} diff --git a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs new file mode 100644 index 000000000..7e25874e0 --- /dev/null +++ b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs @@ -0,0 +1,244 @@ +using System.IO.Abstractions; +using CliWrap; +using ErsatzTV.Application.Streaming; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Emby; +using ErsatzTV.Core.Interfaces.FFmpeg; +using ErsatzTV.Core.Interfaces.Jellyfin; +using ErsatzTV.Core.Interfaces.Plex; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Interfaces.Streaming; +using ErsatzTV.FFmpeg; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NSubstitute.Core; +using NSubstitute.Extensions; +using NUnit.Framework; +using Shouldly; +using DomainChannel = ErsatzTV.Core.Domain.Channel; + +namespace ErsatzTV.Tests.Application.Streaming; + +/// +/// ersatztv#473: a media-server item that is missing from disk used to fall back to the +/// remote-stream URL unconditionally. It must now be probed first, and an unavailable +/// remote stream must render an error card instead of a real playout process. +/// +[TestFixture] +public class GetPlayoutItemProcessByChannelNumberHandlerTests +{ + private const string ChannelNumber = "1"; + private const string JellyfinItemId = "abc123"; + + private readonly List _tempFiles = []; + private InMemoryTvContext _db = null!; + private IFFmpegProcessService _ffmpegProcessService = null!; + private IRemoteStreamProber _remoteStreamProber = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _remoteStreamProber = Substitute.For(); + + _ffmpegProcessService = Substitute.For(); + + // both ForError and ForPlayoutItem must hand back a real value; the handler dereferences them + _ffmpegProcessService.ReturnsForAll(Task.FromResult(Cli.Wrap("ffmpeg"))); + _ffmpegProcessService.ReturnsForAll( + Task.FromResult( + new PlayoutItemResult( + Cli.Wrap("ffmpeg"), + Option.None, + Option.None))); + } + + [TearDown] + public async Task TearDown() + { + await _db.DisposeAsync(); + foreach (string tempFile in _tempFiles) + { + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + } + } + + [Test] + public async Task Should_Render_Error_Card_When_Remote_Stream_Is_Unavailable() + { + DateTimeOffset now = await SeedAll(); + _remoteStreamProber.IsAvailable(Arg.Any(), Arg.Any()).Returns(false); + + Either result = + await CreateHandler().Handle(Request(now), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await _remoteStreamProber.Received(1) + .IsAvailable($"http://localhost:{Settings.StreamingPort}/media/jellyfin/{JellyfinItemId}", Arg.Any()); + + CallsTo(nameof(IFFmpegProcessService.ForError)).Count.ShouldBe(1); + CallsTo(nameof(IFFmpegProcessService.ForPlayoutItem)).Count.ShouldBe(0); + } + + [Test] + public async Task Should_Stream_Remotely_When_Remote_Stream_Is_Available() + { + DateTimeOffset now = await SeedAll(); + _remoteStreamProber.IsAvailable(Arg.Any(), Arg.Any()).Returns(true); + + Either result = + await CreateHandler().Handle(Request(now), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + CallsTo(nameof(IFFmpegProcessService.ForError)).Count.ShouldBe(0); + + List playoutCalls = CallsTo(nameof(IFFmpegProcessService.ForPlayoutItem)); + playoutCalls.Count.ShouldBe(1); + + // videoPath is the 7th parameter of ForPlayoutItem + object?[] arguments = playoutCalls[0].GetArguments(); + arguments[6].ShouldBe($"http://localhost:{Settings.StreamingPort}/media/jellyfin/{JellyfinItemId}"); + } + + private List CallsTo(string methodName) => + _ffmpegProcessService.ReceivedCalls() + .Where(c => c.GetMethodInfo().Name == methodName) + .ToList(); + + private static GetPlayoutItemProcessByChannelNumber Request(DateTimeOffset now) => + new( + ChannelNumber, + StreamingMode.TransportStream, + now, + StartAtZero: false, + HlsRealtime: true, + ChannelStart: now, + PtsOffset: TimeSpan.Zero, + TargetFramerate: Option.None, + IsTroubleshooting: false, + FFmpegProfileId: Option.None); + + private GetPlayoutItemProcessByChannelNumberHandler CreateHandler() + { + var fileSystem = Substitute.For(); + fileSystem.File.Exists(Arg.Any()).Returns(false); + + return new GetPlayoutItemProcessByChannelNumberHandler( + _db.Factory, + _ffmpegProcessService, + fileSystem, + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + _remoteStreamProber, + NullLogger.Instance); + } + + /// + /// Seeds ffmpeg/ffprobe config, an ffmpeg profile, a channel, a playout and a playout item + /// covering "now" whose media item is a jellyfin episode missing from disk. + /// + /// The "now" the request should use. + private async Task SeedAll() + { + string ffmpeg = Path.GetTempFileName(); + string ffprobe = Path.GetTempFileName(); + _tempFiles.Add(ffmpeg); + _tempFiles.Add(ffprobe); + + var now = new DateTimeOffset(2026, 1, 1, 12, 0, 0, TimeSpan.Zero); + + await using TvContext context = _db.CreateContext(); + + context.ConfigElements.Add(new ConfigElement { Key = ConfigElementKey.FFmpegPath.Key, Value = ffmpeg }); + context.ConfigElements.Add(new ConfigElement { Key = ConfigElementKey.FFprobePath.Key, Value = ffprobe }); + + var profile = new FFmpegProfile + { + Name = "Test", + Resolution = new Resolution { Name = "1080p", Width = 1920, Height = 1080 } + }; + context.FFmpegProfiles.Add(profile); + await context.SaveChangesAsync(); + + var channel = new DomainChannel(Guid.NewGuid()) + { + Number = ChannelNumber, + Name = "Test", + Group = "ErsatzTV", + Categories = string.Empty, + FFmpegProfileId = profile.Id, + StreamSelector = string.Empty, + PreferredAudioLanguageCode = string.Empty, + PreferredAudioTitle = string.Empty, + PreferredSubtitleLanguageCode = string.Empty, + MusicVideoCreditsTemplate = string.Empty, + StreamingMode = StreamingMode.TransportStream, + PlayoutSource = ChannelPlayoutSource.Generated, + PlayoutMode = ChannelPlayoutMode.Continuous + }; + context.Channels.Add(channel); + + var episode = new JellyfinEpisode + { + ItemId = JellyfinItemId, + EpisodeMetadata = [new EpisodeMetadata { Title = "Missing", SortTitle = "Missing", Subtitles = [] }], + MediaVersions = + [ + new MediaVersion + { + Name = "Main", + Duration = TimeSpan.FromMinutes(30), + MediaFiles = [new MediaFile { Path = "/gone/episode.mkv", PathHash = "gone" }], + Streams = [] + } + ] + }; + context.AddRange(episode); + await context.SaveChangesAsync(); + + var playout = new Playout + { + ChannelId = channel.Id, + ScheduleKind = PlayoutScheduleKind.Classic + }; + context.Playouts.Add(playout); + await context.SaveChangesAsync(); + + context.PlayoutItems.Add( + new PlayoutItem + { + PlayoutId = playout.Id, + MediaItemId = episode.Id, + Start = now.AddMinutes(-5).UtcDateTime, + Finish = now.AddMinutes(25).UtcDateTime, + InPoint = TimeSpan.FromMinutes(5), + OutPoint = TimeSpan.FromMinutes(30), + Watermarks = [], + PlayoutItemWatermarks = [], + GraphicsElements = [], + PlayoutItemGraphicsElements = [] + }); + await context.SaveChangesAsync(); + + return now; + } +} diff --git a/ErsatzTV/Startup.cs b/ErsatzTV/Startup.cs index c147544ca..a26e1fe5a 100644 --- a/ErsatzTV/Startup.cs +++ b/ErsatzTV/Startup.cs @@ -1084,6 +1084,7 @@ public class Startup services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/docs/decisions.md b/docs/decisions.md index 4e7ba3687..8a0cf6d1f 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2081,3 +2081,42 @@ Disabling the servers is worth it (consistent reduction, no resident server), bu off the live test-job peak-anon this instrument now reports, not off the build-only A/B. The older #411 probe (`anon 7134 MiB`) read higher than these swap-off sampled numbers and is superseded (swap/read-method move the figure >1 GiB). +## 2026-07-19 — Media-server remote-stream URLs are probed before use: 404 fails closed, everything else fails open, no toggle (#473) + +`GetPlayoutItemProcessByChannelNumberHandler.ValidatePlayoutItemPath` now probes the Plex/Jellyfin/Emby +remote-stream URL via the new `IRemoteStreamProber` seam before returning it, and on a definitive HTTP +**404** returns the new `PlayoutItemNotAvailableFromMediaServer` error instead of a playable path. + +- **The bug this closes.** The method's local branch checked `_fileSystem.File.Exists(path)`, but the + three 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", ffmpeg was + launched against a URL that 404s and died with **exit 8**. That bypassed the good error path the handler + already had (`PlayoutItemDoesNotExistOnDisk` → an error card sized to run until the **next** playout item, + i.e. 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 for an episode). Restoring the method's + own invariant — every `PlayoutItemWithPath` it returns has been checked for existence — fixes the class + for all three media servers, not just the reported Jellyfin instance. +- **404-only, fail-open on everything else.** A timeout, a 5xx, an auth error or a transport failure + returns *available*, so a probe that cannot answer can never turn a tune that would have worked into an + error card. This is the load-bearing property and is pinned by tests + (`HttpRemoteStreamProberTests.Should_Fail_Open_*`) precisely so a later refactor can't quietly invert it. +- **No `ConfigElement` toggle.** The behaviour strictly dominates the status quo (fail-open means it can + only ever convert an ffmpeg-404 into a better-labelled error card), and the cost is one LAN round trip + bounded by a 2s timeout — noise against the ~7s cold start that #350 is about. A toggle would be surface + for a switch nobody has a reason to flip. +- **Rejected — resizing the `HlsSessionWorker` retry loop to skip the item on any ffmpeg failure.** It buys + the viewer nothing the above doesn't (the slot is dead either way) and cannot distinguish "this item is + dead" from "this transcoder hiccupped". Prod shows real *transient* channel-wide failures (VAAPI + `hwupload -22` / exit 234), and retrying those is correct; converting them into whole-item blackouts is a + regression. The retry loop is deliberately left intact — it also remains the backstop for the probe's + TOCTOU window (media can vanish between a 200 probe and ffmpeg's own request). +- **Rejected — writing `MediaItemState` from the streaming path.** Only the scanner writes `State`, and + breaking that ownership wouldn't even have fixed this: the failing item is `RemoteOnly`, which + `PlayoutBuilder`'s `PlayoutSkipMissingItems` skip does not exclude. On Jellyfin (`ServerSupportsRemoteStreaming`) + `RemoteOnly` is the *normal* state for every item when path replacement isn't configured, so it carries no + signal about playability — never key availability decisions off it. +- **Known consequence.** A removed/misconfigured media source makes `InternalController` return 404 itself, + so every item on it now probes 404 and renders an error card. Same viewer outcome as before (the tune was + failing anyway), better message — but worth knowing before reading "all my channels show error cards" as + a regression. -- 2.47.3 From bad19f8d26fd866ddfb713059b8d4d1f0ce41754 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 21:27:09 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(473):=20review=20fixes=20=E2=80=94=20on?= =?UTF-8?q?ly=20a=20redirected=20404=20fails=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../Streaming/HttpRemoteStreamProberTests.cs | 46 ++++++++++--- .../Streaming/HttpRemoteStreamProber.cs | 47 +++++++++++-- ...tItemProcessByChannelNumberHandlerTests.cs | 67 ++++++++++++++----- docs/decisions.md | 64 ++++++++++++------ 4 files changed, 169 insertions(+), 55 deletions(-) diff --git a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs index 49e769e79..0f1ab7f49 100644 --- a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs +++ b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs @@ -13,15 +13,32 @@ public class HttpRemoteStreamProberTests private const string Url = "http://localhost:8409/media/jellyfin/abc123"; [Test] - public async Task Should_Report_Unavailable_On_404() + public async Task Should_Report_Unavailable_On_404_From_The_Media_Server() { - HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.NotFound); + // a media-server 404 arrives after our /media/... endpoint redirected, so the response's + // final request uri is the media server's, not the probe url + HttpRemoteStreamProber prober = ProberReturning( + HttpStatusCode.NotFound, + finalUri: "http://jellyfin:8096/Videos/abc123/stream?static=true"); bool result = await prober.IsAvailable(Url, CancellationToken.None); result.ShouldBeFalse(); } + // ersatztv#473 review finding: our OWN /media/{provider}/... endpoint 404s when the media source + // is unconfigured or momentarily missing. Failing closed there would blank every item on that + // source, which is exactly what the fail-open contract exists to prevent. + [Test] + public async Task Should_Fail_Open_On_404_That_Was_Not_Redirected() + { + HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.NotFound, finalUri: Url); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + } + [TestCase(HttpStatusCode.OK)] [TestCase(HttpStatusCode.PartialContent)] [TestCase(HttpStatusCode.NoContent)] @@ -73,22 +90,22 @@ public class HttpRemoteStreamProberTests result.ShouldBeTrue(); } + // caller cancellation (shutdown / client disconnect) is a genuine signal, NOT a probe failure -- + // swallowing it would let the handler go on building an ffmpeg command on a dead token. [Test] - public async Task Should_Fail_Open_When_Caller_Cancels() + public async Task Should_Propagate_Caller_Cancellation() { HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.OK); using var cts = new CancellationTokenSource(); await cts.CancelAsync(); - bool result = await prober.IsAvailable(Url, cts.Token); - - result.ShouldBeTrue(); + await Should.ThrowAsync(() => prober.IsAvailable(Url, cts.Token)); } - private static HttpRemoteStreamProber ProberReturning(HttpStatusCode statusCode) => + private static HttpRemoteStreamProber ProberReturning(HttpStatusCode statusCode, string finalUri = null) => new( - new StubHttpClientFactory(new StatusCodeHttpMessageHandler(statusCode)), + new StubHttpClientFactory(new StatusCodeHttpMessageHandler(statusCode, finalUri)), Substitute.For>()); private sealed class StubHttpClientFactory(HttpMessageHandler handler) : IHttpClientFactory @@ -96,14 +113,23 @@ public class HttpRemoteStreamProberTests public HttpClient CreateClient(string name) => new(handler, disposeHandler: false); } - private sealed class StatusCodeHttpMessageHandler(HttpStatusCode statusCode) : HttpMessageHandler + private sealed class StatusCodeHttpMessageHandler(HttpStatusCode statusCode, string finalUri = null) + : HttpMessageHandler { protected override Task SendAsync( HttpRequestMessage request, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); - return Task.FromResult(new HttpResponseMessage(statusCode)); + + // HttpClient rewrites RequestMessage.RequestUri to the final hop when it follows a + // redirect; finalUri lets a test stand in for "the media server answered this". + if (finalUri is not null) + { + request.RequestUri = new Uri(finalUri); + } + + return Task.FromResult(new HttpResponseMessage(statusCode) { RequestMessage = request }); } } diff --git a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs index 6ef74ec0e..ed4bd698a 100644 --- a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs +++ b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs @@ -1,4 +1,5 @@ using System.Net; +using System.Net.Http.Headers; using ErsatzTV.Core.Interfaces.Streaming; using Microsoft.Extensions.Logging; @@ -8,9 +9,11 @@ namespace ErsatzTV.Infrastructure.Streaming; /// Probes a media-server remote-stream URL over HTTP. /// /// -/// Deliberately fail-open: only a definitive HTTP 404 reports the media as gone. A probe that -/// times out, errors, or returns any other status must never turn a tune that would have worked -/// into an error card, so every other outcome reports available. (ersatztv#473) +/// Deliberately fail-open: the only outcome that reports the media as gone is a 404 that came +/// from the media server itself (i.e. arrived after our /media/{provider}/... endpoint +/// redirected). A timeout, a transport failure, any other status, or a 404 raised by ErsatzTV's +/// own endpoint all report available, so a probe that cannot answer never turns a tune that +/// would have worked into an error card. (ersatztv#473) /// public class HttpRemoteStreamProber( IHttpClientFactory httpClientFactory, @@ -29,7 +32,7 @@ public class HttpRemoteStreamProber( // ask for a single byte; media servers vary in their HEAD support, and this exercises the // same redirect chain ffmpeg will follow - request.Headers.Range = new System.Net.Http.Headers.RangeHeaderValue(0, 0); + request.Headers.Range = new RangeHeaderValue(0, 0); using HttpClient client = httpClientFactory.CreateClient(); using HttpResponseMessage response = await client.SendAsync( @@ -39,12 +42,38 @@ public class HttpRemoteStreamProber( if (response.StatusCode is HttpStatusCode.NotFound) { - logger.LogWarning("Media server reported 404 for remote stream {Url}", url); - return false; + // only the MEDIA SERVER's 404 is evidence that the item is gone. our own + // /media/{provider}/... endpoint also returns 404 when the media source is + // unconfigured or momentarily missing (InternalController maps a failed + // connection-parameter lookup to NotFound), and treating that as "gone" would fail + // CLOSED for every item on that source. A media-server 404 always arrives after a + // redirect, so an un-redirected 404 came from us and must fail open. + if (WasRedirected(response, url)) + { + logger.LogWarning("Media server reported 404 for remote stream {Url}", url); + return false; + } + + logger.LogDebug( + "Probe of {Url} returned 404 without redirecting to a media server; assuming the " + + "item is available rather than failing closed on our own endpoint", + url); + + return true; } + // drain the single byte we asked for so the connection goes back to the pool instead of + // being aborted when the unread response stream is disposed + await response.Content.ReadAsByteArrayAsync(timeoutCts.Token); + return true; } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // the CALLER cancelled (shutdown / client disconnect). that is a genuine signal, not a + // probe failure, so it must propagate rather than be swallowed as fail-open. + throw; + } catch (Exception ex) { // fail open - a probe failure is not evidence that the media is gone @@ -52,4 +81,10 @@ public class HttpRemoteStreamProber( return true; } } + + private static bool WasRedirected(HttpResponseMessage response, string probeUrl) + { + Uri finalUri = response.RequestMessage?.RequestUri; + return finalUri is not null && !string.Equals(finalUri.ToString(), probeUrl, StringComparison.Ordinal); + } } diff --git a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs index 7e25874e0..9c4598238 100644 --- a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs @@ -33,6 +33,7 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests { private const string ChannelNumber = "1"; private const string JellyfinItemId = "abc123"; + private const string EmbyItemId = "def456"; private readonly List _tempFiles = []; private InMemoryTvContext _db = null!; @@ -84,8 +85,16 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests await _remoteStreamProber.Received(1) .IsAvailable($"http://localhost:{Settings.StreamingPort}/media/jellyfin/{JellyfinItemId}", Arg.Any()); - CallsTo(nameof(IFFmpegProcessService.ForError)).Count.ShouldBe(1); + List errorCalls = CallsTo(nameof(IFFmpegProcessService.ForError)); + errorCalls.Count.ShouldBe(1); CallsTo(nameof(IFFmpegProcessService.ForPlayoutItem)).Count.ShouldBe(0); + + // the `case PlayoutItemNotAvailableFromMediaServer:` arm exists to surface the real error + // text; without it the error falls to `default:` and the card says "Channel is Offline". + // Assert the message, or the case label is untested dead weight. + object?[] arguments = errorCalls[0].GetArguments(); + string errorMessage = arguments.OfType().Single(a => a.Contains("not available")); + errorMessage.ShouldContain($"/media/jellyfin/{JellyfinItemId}"); } [Test] @@ -109,6 +118,28 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests arguments[6].ShouldBe($"http://localhost:{Settings.StreamingPort}/media/jellyfin/{JellyfinItemId}"); } + // the fix changed all three remote-stream branches; Jellyfin above covers one, this pins that a + // second provider is probed too rather than the fix being Jellyfin-only (ersatztv#473 review) + [Test] + public async Task Should_Render_Error_Card_When_Emby_Remote_Stream_Is_Unavailable() + { + DateTimeOffset now = await SeedAll(emby: true); + _remoteStreamProber.IsAvailable(Arg.Any(), Arg.Any()).Returns(false); + + Either result = + await CreateHandler().Handle(Request(now), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await _remoteStreamProber.Received(1) + .IsAvailable( + $"http://localhost:{Settings.StreamingPort}/media/emby/{EmbyItemId}", + Arg.Any()); + + CallsTo(nameof(IFFmpegProcessService.ForError)).Count.ShouldBe(1); + CallsTo(nameof(IFFmpegProcessService.ForPlayoutItem)).Count.ShouldBe(0); + } + private List CallsTo(string methodName) => _ffmpegProcessService.ReceivedCalls() .Where(c => c.GetMethodInfo().Name == methodName) @@ -157,7 +188,7 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests /// covering "now" whose media item is a jellyfin episode missing from disk. /// /// The "now" the request should use. - private async Task SeedAll() + private async Task SeedAll(bool emby = false) { string ffmpeg = Path.GetTempFileName(); string ffprobe = Path.GetTempFileName(); @@ -197,21 +228,23 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests }; context.Channels.Add(channel); - var episode = new JellyfinEpisode - { - ItemId = JellyfinItemId, - EpisodeMetadata = [new EpisodeMetadata { Title = "Missing", SortTitle = "Missing", Subtitles = [] }], - MediaVersions = - [ - new MediaVersion - { - Name = "Main", - Duration = TimeSpan.FromMinutes(30), - MediaFiles = [new MediaFile { Path = "/gone/episode.mkv", PathHash = "gone" }], - Streams = [] - } - ] - }; + List metadata = + [new EpisodeMetadata { Title = "Missing", SortTitle = "Missing", Subtitles = [] }]; + List versions = + [ + new MediaVersion + { + Name = "Main", + Duration = TimeSpan.FromMinutes(30), + MediaFiles = [new MediaFile { Path = "/gone/episode.mkv", PathHash = "gone" }], + Streams = [] + } + ]; + + Episode episode = emby + ? new EmbyEpisode { ItemId = EmbyItemId, EpisodeMetadata = metadata, MediaVersions = versions } + : new JellyfinEpisode { ItemId = JellyfinItemId, EpisodeMetadata = metadata, MediaVersions = versions }; + context.AddRange(episode); await context.SaveChangesAsync(); diff --git a/docs/decisions.md b/docs/decisions.md index 8a0cf6d1f..5ea8707c8 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2081,30 +2081,48 @@ Disabling the servers is worth it (consistent reduction, no resident server), bu off the live test-job peak-anon this instrument now reports, not off the build-only A/B. The older #411 probe (`anon 7134 MiB`) read higher than these swap-off sampled numbers and is superseded (swap/read-method move the figure >1 GiB). -## 2026-07-19 — Media-server remote-stream URLs are probed before use: 404 fails closed, everything else fails open, no toggle (#473) +## 2026-07-19 — Media-server remote-stream URLs are probed before use: a redirected 404 fails closed, everything else fails open, no toggle (#473) `GetPlayoutItemProcessByChannelNumberHandler.ValidatePlayoutItemPath` now probes the Plex/Jellyfin/Emby -remote-stream URL via the new `IRemoteStreamProber` seam before returning it, and on a definitive HTTP -**404** returns the new `PlayoutItemNotAvailableFromMediaServer` error instead of a playable path. +remote-stream URL via the new `IRemoteStreamProber` seam before returning it, and on a 404 **from the media +server** returns the new `PlayoutItemNotAvailableFromMediaServer` error instead of a playable path. - **The bug this closes.** The method's local branch checked `_fileSystem.File.Exists(path)`, but the three 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", ffmpeg was - launched against a URL that 404s and died with **exit 8**. That bypassed the good error path the handler - already had (`PlayoutItemDoesNotExistOnDisk` → an error card sized to run until the **next** playout item, - i.e. 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 for an episode). Restoring the method's - own invariant — every `PlayoutItemWithPath` it returns has been checked for existence — fixes the class - for all three media servers, not just the reported Jellyfin instance. -- **404-only, fail-open on everything else.** A timeout, a 5xx, an auth error or a transport failure - returns *available*, so a probe that cannot answer can never turn a tune that would have worked into an - error card. This is the load-bearing property and is pinned by tests - (`HttpRemoteStreamProberTests.Should_Fail_Open_*`) precisely so a later refactor can't quietly invert it. -- **No `ConfigElement` toggle.** The behaviour strictly dominates the status quo (fail-open means it can - only ever convert an ffmpeg-404 into a better-labelled error card), and the cost is one LAN round trip - bounded by a 2s timeout — noise against the ~7s cold start that #350 is about. A toggle would be surface - for a switch nobody has a reason to flip. + launched against a URL that 404s and died with **exit 8** — landing 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, so a dead item produces a repeating error card for its whole slot + (~22 min for an episode). The fix restores the method's own invariant — every `PlayoutItemWithPath` it + returns has been checked for existence — so the failure now returns an **error** rather than a playable + URL, and the handler's error path sizes the card to run until the **next** playout item. +- **What the new `case` label does and does not do** (a review corrected an earlier draft of this entry): + the skip-to-next-item sizing comes from `maybeDuration`/`finish`, computed *before* the switch — the + `default:` arm already had it. Adding `case PlayoutItemNotAvailableFromMediaServer:` alongside + `PlayoutItemDoesNotExistOnDisk` changes only the **caption** on the card (the real error text instead of + "Channel is Offline"). Worth having, but not where the fix lives. A handler test asserts that message + specifically, so the label cannot silently decay into dead weight. +- **Scope: the generated-playout path only.** All three remote branches of `ValidatePlayoutItemPath` are + covered. `ExternalJsonPlayoutItemProvider` builds its own `/media/plex/...` URL and its result is + assigned *without* passing through `ValidatePlayoutItemPath`, so external-JSON channels keep the old + behaviour — tracked as #480 rather than silently claimed as fixed. The troubleshooting and + subtitle-extraction paths build the same URLs and are deliberately **left unprobed**: troubleshooting + should surface the raw ffmpeg failure, and a subtitle-extraction miss is not a channel outage. +- **Fail-open on everything except a *redirected* 404.** A timeout, a 5xx, an auth error or a transport + failure returns *available* — and so does a **404 that arrived without a redirect**. ErsatzTV's own + `InternalController` returns `NotFound` when the media source is unconfigured or momentarily missing, so + honouring that would fail *closed* for every item on that source; the first draft probed for "any 404", + which made this contract untrue. Only a 404 reached *after* the redirect to the media server is evidence + the item is gone. Pinned by tests (`Should_Fail_Open_*`, + `Should_Fail_Open_On_404_That_Was_Not_Redirected`) so a later refactor can't quietly invert it. Caller + cancellation is **not** swallowed — it propagates, because a shutdown is a genuine signal, not a probe + failure. +- **No `ConfigElement` toggle.** The behaviour strictly dominates the status quo, and the probe is bounded + by a 2s linked-CTS timeout (not `HttpClient.Timeout`, which defaults to 100s). A toggle would be surface + for a switch nobody has a reason to flip. **Not yet measured:** on an install without path replacement + every item selection pays this probe, and a slow media server could cost up to the full 2s. That is a + worst case rather than a typical one, but it has not been measured against #350's cold-start budget — + measure before assuming it is noise. - **Rejected — resizing the `HlsSessionWorker` retry loop to skip the item on any ffmpeg failure.** It buys the viewer nothing the above doesn't (the slot is dead either way) and cannot distinguish "this item is dead" from "this transcoder hiccupped". Prod shows real *transient* channel-wide failures (VAAPI @@ -2116,7 +2134,9 @@ remote-stream URL via the new `IRemoteStreamProber` seam before returning it, an `PlayoutBuilder`'s `PlayoutSkipMissingItems` skip does not exclude. On Jellyfin (`ServerSupportsRemoteStreaming`) `RemoteOnly` is the *normal* state for every item when path replacement isn't configured, so it carries no signal about playability — never key availability decisions off it. -- **Known consequence.** A removed/misconfigured media source makes `InternalController` return 404 itself, - so every item on it now probes 404 and renders an error card. Same viewer outcome as before (the tune was - failing anyway), better message — but worth knowing before reading "all my channels show error cards" as - a regression. +- **Deferred — HEAD instead of GET.** The probe sends `GET` with `Range: bytes=0-0`, which on + `/Videos/{id}/stream?static=true` is a real (if minimal) playback request and may register a session or + touch play-state on some media-server versions. `GET` was chosen because HEAD support varies across + media servers — that is a reasonable prior but an *unverified* one. A HEAD-with-GET-fallback would avoid + the side effect; deferred rather than guessed at, since it trades a known-working request for an + untested one. -- 2.47.3 From a10c0325eb0961c2f6bd3ecb143142d7ea4ac981 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 21:41:25 +0200 Subject: [PATCH 3/5] fix(473): compare parsed Uris in the redirect check 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) --- .../Streaming/HttpRemoteStreamProberTests.cs | 14 ++++++++++++++ .../Streaming/HttpRemoteStreamProber.cs | 13 ++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs index 0f1ab7f49..d9d5e7740 100644 --- a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs +++ b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs @@ -39,6 +39,20 @@ public class HttpRemoteStreamProberTests result.ShouldBeTrue(); } + // a plex key can contain spaces/unicode; pin that an un-redirected 404 on such a url still fails + // OPEN. (This passes against a naive string comparison too - Uri.ToString() unescapes - so it + // guards the behaviour, not the implementation choice.) + [Test] + public async Task Should_Fail_Open_On_404_For_An_Unredirected_Url_Needing_Escaping() + { + const string plexUrl = "http://localhost:8409/media/plex/1/library/parts/1/a file.mkv"; + HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.NotFound, finalUri: plexUrl); + + bool result = await prober.IsAvailable(plexUrl, CancellationToken.None); + + result.ShouldBeTrue(); + } + [TestCase(HttpStatusCode.OK)] [TestCase(HttpStatusCode.PartialContent)] [TestCase(HttpStatusCode.NoContent)] diff --git a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs index ed4bd698a..8ec9f8bb1 100644 --- a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs +++ b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs @@ -85,6 +85,17 @@ public class HttpRemoteStreamProber( private static bool WasRedirected(HttpResponseMessage response, string probeUrl) { Uri finalUri = response.RequestMessage?.RequestUri; - return finalUri is not null && !string.Equals(finalUri.ToString(), probeUrl, StringComparison.Ordinal); + if (finalUri is null || !Uri.TryCreate(probeUrl, UriKind.Absolute, out Uri requestedUri)) + { + // can't tell where the 404 came from; fail open rather than guess + return false; + } + + // compare parsed Uris rather than strings. Uri.Equals compares normalized components, so it + // can't mistake an escaping/casing difference for a redirect and fail CLOSED - the exact + // failure this check exists to prevent. (A string compare on Uri.ToString() happens to agree + // for our machine-generated URLs, since ToString unescapes; this is defense in depth, not a + // fix for an observed bug.) + return !Uri.Equals(finalUri, requestedUri); } } -- 2.47.3 From dc5ceb5a140f80bf2396622fea0b2f0d71652b58 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 21:47:50 +0200 Subject: [PATCH 4/5] fix(473): bound the drain, correct the interface contract, quiet graceful cancels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review pass returned BLOCKED on two findings introduced by the first fix commit. Both were right. BLOCKER 1 — the drain added for "return the connection to the pool" was unbounded. `response.Content.ReadAsByteArrayAsync()` buffers the WHOLE body, and it ran for every non-404 response. A server that ignores `Range: bytes=0-0` answers 200 with the entire file, so this would download at line rate into a byte[] on the streaming hot path for up to the 2s timeout -- strictly worse than the aborted socket it replaced, and it defeated the ResponseHeadersRead the probe deliberately uses. Now the single byte is read only on 206 (where the server honoured the range and the body really is one byte); any other status aborts the socket, which is much the cheaper evil. Two tests pin both directions; verified non-vacuous (restoring the unbounded drain fails the 200-with-body test). BLOCKER 2 — IRemoteStreamProber's doc-comment still described pre-fix behaviour. I had told the reviewer it was updated; it was not -- only the implementation's had been. It claimed `false` on any 404 (now only a redirected one) and that every other outcome returns `true` (caller cancellation throws). Both clauses corrected, and the throwing contract is now documented with . Also fixed the reviewer's own follow-on finding: the cancellation rethrow it asked for reached HlsSessionWorker's catch-all, which logs a channel-level ERROR with a stack trace. The graceful TaskCanceledException/OperationCanceledException handler at :662 wraps only the inner ffmpeg block, not the mediator sends, so every client disconnect on a remote-streaming channel would have produced a spurious ERROR -- in exactly the logs a #350 cold-start investigation reads. Added a cancellation filter on the outer try that logs Information instead. Nit: stale SeedAll doc-comment now mentions the emby case. Deferred, per reviewer's explicit agreement: Plex-branch handler coverage (follow-up), and HEAD-with-GET-fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Streaming/HlsSessionWorker.cs | 10 +++ .../Streaming/IRemoteStreamProber.cs | 18 +++- .../Streaming/HttpRemoteStreamProberTests.cs | 90 +++++++++++++++++++ .../Streaming/HttpRemoteStreamProber.cs | 14 ++- ...tItemProcessByChannelNumberHandlerTests.cs | 3 +- 5 files changed, 127 insertions(+), 8 deletions(-) diff --git a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs index 8c2e68381..b4c133f53 100644 --- a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs +++ b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs @@ -673,6 +673,16 @@ public class HlsSessionWorker : IHlsSessionWorker } } } + catch (Exception ex) when (ex is TaskCanceledException or OperationCanceledException) + { + // 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. + // Without this it reaches the catch-all and logs a channel-level ERROR with a stack + // trace on every graceful teardown. (ersatztv#473 review) + _logger.LogInformation("Terminating HLS session for channel {Channel}", _channelNumber); + + return false; + } catch (Exception ex) { _logger.LogError(ex, "Error transcoding channel {Channel} - {Message}", _channelNumber, ex.Message); diff --git a/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs index 3ba2cb94e..c3b20e434 100644 --- a/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs +++ b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs @@ -6,12 +6,22 @@ namespace ErsatzTV.Core.Interfaces.Streaming; public interface IRemoteStreamProber { /// - /// Probes (following redirects, as ffmpeg would). + /// Probes , following redirects as ffmpeg would. /// /// - /// false only when the media is definitively gone (HTTP 404). Every other outcome — - /// any other status code, a timeout, or a transport failure — returns true so that a - /// probe problem can never prevent a tune that would otherwise have succeeded (fail-open). + /// false only when the media server itself reported the media gone — i.e. a 404 that + /// arrived after ErsatzTV's own /media/{provider}/... endpoint redirected. + /// Every other outcome returns true (fail-open), including an un-redirected 404: that + /// one came from ErsatzTV's own endpoint, which also 404s when the media source is + /// unconfigured or momentarily missing, and honouring it would blank every item on that + /// source. Timeouts, transport failures and all other status codes likewise return + /// true, so a probe that cannot answer never prevents a tune that would have worked. /// + /// + /// Thrown when is cancelled. Caller cancellation is a + /// genuine signal (shutdown / client disconnect), not a probe failure, so it propagates + /// rather than being absorbed by the fail-open behaviour above. The prober's own internal + /// timeout does not throw — it fails open. + /// Task IsAvailable(string url, CancellationToken cancellationToken); } diff --git a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs index d9d5e7740..563e09446 100644 --- a/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs +++ b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs @@ -80,6 +80,46 @@ public class HttpRemoteStreamProberTests result.ShouldBeTrue(); } + // a server that ignores `Range: bytes=0-0` answers 200 with the WHOLE FILE. The probe must not + // read it -- buffering a video on the streaming hot path would be far worse than the aborted + // socket the drain was added to avoid. (Review finding against the first fix commit.) + [Test] + public async Task Should_Not_Read_The_Body_When_The_Server_Ignores_The_Range_Request() + { + var body = new TrackingStream(64 * 1024 * 1024); + var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StreamContent(body) }; + + var prober = new HttpRemoteStreamProber( + new StubHttpClientFactory(new FixedResponseHttpMessageHandler(response)), + Substitute.For>()); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + body.BytesRead.ShouldBe(0); + } + + // the counterpart: when the server DID honour the range, the one byte is read so the connection + // goes back to the pool rather than being aborted + [Test] + public async Task Should_Drain_The_Single_Byte_When_The_Server_Honours_The_Range_Request() + { + var body = new TrackingStream(1); + var response = new HttpResponseMessage(HttpStatusCode.PartialContent) + { + Content = new StreamContent(body) + }; + + var prober = new HttpRemoteStreamProber( + new StubHttpClientFactory(new FixedResponseHttpMessageHandler(response)), + Substitute.For>()); + + bool result = await prober.IsAvailable(Url, CancellationToken.None); + + result.ShouldBeTrue(); + body.BytesRead.ShouldBe(1); + } + [Test] public async Task Should_Fail_Open_On_Transport_Failure() { @@ -147,6 +187,56 @@ public class HttpRemoteStreamProberTests } } + private sealed class FixedResponseHttpMessageHandler(HttpResponseMessage response) : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + response.RequestMessage = request; + return Task.FromResult(response); + } + } + + /// A readable stream that records how many bytes were actually pulled from it. + private sealed class TrackingStream(long length) : Stream + { + public int BytesRead { get; private set; } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => length; + + public override long Position + { + get => BytesRead; + set => throw new NotSupportedException(); + } + + public override int Read(byte[] buffer, int offset, int count) + { + if (BytesRead >= length) + { + return 0; + } + + int toRead = (int)Math.Min(count, length - BytesRead); + Array.Clear(buffer, offset, toRead); + BytesRead += toRead; + return toRead; + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + private sealed class ThrowingHttpMessageHandler(Exception exception) : HttpMessageHandler { protected override Task SendAsync( diff --git a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs index 8ec9f8bb1..b10c8d99b 100644 --- a/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs +++ b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs @@ -62,9 +62,17 @@ public class HttpRemoteStreamProber( return true; } - // drain the single byte we asked for so the connection goes back to the pool instead of - // being aborted when the unread response stream is disposed - await response.Content.ReadAsByteArrayAsync(timeoutCts.Token); + // return the connection to the pool instead of aborting it by disposing an unread + // stream - but ONLY where the server honoured the range, i.e. the body really is one + // byte. A server that ignores `Range` answers 200 with the WHOLE FILE, and draining that + // would download at line rate into memory on the streaming hot path, defeating the + // ResponseHeadersRead above. There, abort the socket - much the cheaper evil. + if (response.StatusCode is HttpStatusCode.PartialContent) + { + var singleByte = new byte[1]; + Stream body = await response.Content.ReadAsStreamAsync(timeoutCts.Token); + await body.ReadAsync(singleByte, timeoutCts.Token); + } return true; } diff --git a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs index 9c4598238..e573d6768 100644 --- a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs @@ -185,7 +185,8 @@ public class GetPlayoutItemProcessByChannelNumberHandlerTests /// /// Seeds ffmpeg/ffprobe config, an ffmpeg profile, a channel, a playout and a playout item - /// covering "now" whose media item is a jellyfin episode missing from disk. + /// covering "now" whose media item is a jellyfin episode missing from disk — or, when + /// is set, an emby episode. /// /// The "now" the request should use. private async Task SeedAll(bool emby = false) -- 2.47.3 From 1eff0b39b74efbd3e24f8a79998211feea035da4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 22:02:15 +0200 Subject: [PATCH 5/5] fix(473): gate the cancellation filter on the caller's token 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 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) --- ErsatzTV.Application/Streaming/HlsSessionWorker.cs | 8 ++++++-- .../Interfaces/Streaming/IRemoteStreamProber.cs | 9 +++++---- docs/handoffs/chicorytv-issue-queue.md | 12 ++++++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs index b4c133f53..43cdb7d0b 100644 --- a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs +++ b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs @@ -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 // 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 - // 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); return false; diff --git a/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs index c3b20e434..a936fd9d6 100644 --- a/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs +++ b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs @@ -18,10 +18,11 @@ public interface IRemoteStreamProber /// true, so a probe that cannot answer never prevents a tune that would have worked. /// /// - /// Thrown when is cancelled. Caller cancellation is a - /// genuine signal (shutdown / client disconnect), not a probe failure, so it propagates - /// rather than being absorbed by the fail-open behaviour above. The prober's own internal - /// timeout does not throw — it fails open. + /// May propagate when is cancelled while the probe is + /// in flight. Caller cancellation is a genuine signal (shutdown / client disconnect), not a + /// probe failure, so it is not absorbed by the fail-open behaviour above. Cancelling after + /// the probe has already completed returns normally. The prober's own internal timeout does + /// not throw — it fails open. /// Task IsAvailable(string url, CancellationToken cancellationToken); } diff --git a/docs/handoffs/chicorytv-issue-queue.md b/docs/handoffs/chicorytv-issue-queue.md index 0355ff869..77cab0503 100644 --- a/docs/handoffs/chicorytv-issue-queue.md +++ b/docs/handoffs/chicorytv-issue-queue.md @@ -249,6 +249,18 @@ HARD CONSTRAINTS: 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 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 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 -- 2.47.3