diff --git a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs index 8c2e68381..43cdb7d0b 100644 --- a/ErsatzTV.Application/Streaming/HlsSessionWorker.cs +++ b/ErsatzTV.Application/Streaming/HlsSessionWorker.cs @@ -673,6 +673,20 @@ public class HlsSessionWorker : IHlsSessionWorker } } } + 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. 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; + } catch (Exception ex) { _logger.LogError(ex, "Error transcoding channel {Channel} - {Message}", _channelNumber, ex.Message); 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..a936fd9d6 --- /dev/null +++ b/ErsatzTV.Core/Interfaces/Streaming/IRemoteStreamProber.cs @@ -0,0 +1,28 @@ +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 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. + /// + /// + /// 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/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs new file mode 100644 index 000000000..563e09446 --- /dev/null +++ b/ErsatzTV.Infrastructure.Tests/Streaming/HttpRemoteStreamProberTests.cs @@ -0,0 +1,247 @@ +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_From_The_Media_Server() + { + // 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(); + } + + // 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)] + 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(); + } + + // 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() + { + 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(); + } + + // 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_Propagate_Caller_Cancellation() + { + HttpRemoteStreamProber prober = ProberReturning(HttpStatusCode.OK); + + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + await Should.ThrowAsync(() => prober.IsAvailable(Url, cts.Token)); + } + + private static HttpRemoteStreamProber ProberReturning(HttpStatusCode statusCode, string finalUri = null) => + new( + new StubHttpClientFactory(new StatusCodeHttpMessageHandler(statusCode, finalUri)), + Substitute.For>()); + + private sealed class StubHttpClientFactory(HttpMessageHandler handler) : IHttpClientFactory + { + public HttpClient CreateClient(string name) => new(handler, disposeHandler: false); + } + + private sealed class StatusCodeHttpMessageHandler(HttpStatusCode statusCode, string finalUri = null) + : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + + // 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 }); + } + } + + 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( + 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..b10c8d99b --- /dev/null +++ b/ErsatzTV.Infrastructure/Streaming/HttpRemoteStreamProber.cs @@ -0,0 +1,109 @@ +using System.Net; +using System.Net.Http.Headers; +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: 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, + 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 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) + { + // 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; + } + + // 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; + } + 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 + logger.LogDebug(ex, "Unable to probe remote stream {Url}; assuming it is available", url); + return true; + } + } + + private static bool WasRedirected(HttpResponseMessage response, string probeUrl) + { + Uri finalUri = response.RequestMessage?.RequestUri; + 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); + } +} diff --git a/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs new file mode 100644 index 000000000..e573d6768 --- /dev/null +++ b/ErsatzTV.Tests/Application/Streaming/GetPlayoutItemProcessByChannelNumberHandlerTests.cs @@ -0,0 +1,278 @@ +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 const string EmbyItemId = "def456"; + + 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()); + + 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] + 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}"); + } + + // 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) + .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 — or, when + /// is set, an emby episode. + /// + /// The "now" the request should use. + private async Task SeedAll(bool emby = false) + { + 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); + + 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(); + + 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..5ea8707c8 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2081,3 +2081,62 @@ 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: 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 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** — 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 + `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. +- **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. 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