Adversarial review of PR #479 found the stated fail-open contract was not what the code measured, plus four smaller gaps. All fixed here as a follow-up commit (no amend/force-push). High — a 404 from ErsatzTV's OWN endpoint was treated as "media gone". /media/{provider}/... is served by InternalController, which returns NotFound when the media source is unconfigured or momentarily missing (a media-source edit that deletes+reinserts connections, a restore, a partially-configured server). Probing for "any 404" therefore failed CLOSED for every item on that source -- exactly the case the fail-open contract exists to prevent. A media-server 404 always arrives after a redirect, so an un-redirected 404 is now treated as available. Medium — the new switch label was untested and its benefit overstated. maybeDuration/finish are computed before the switch, so `default:` already sized the error card to the next playout item; the label only changes the caption. The handler test asserted call counts only, so deleting the label still passed. It now asserts the error message, and removing the label fails the test (verified). Medium — Plex/Emby branches changed but had no coverage. Added an Emby handler test asserting the probe is called with the emby URL. Low — caller cancellation was swallowed and pinned as desired behaviour. A shutdown / client disconnect is a genuine signal, not a probe failure; it now propagates, and only the probe's own 2s timeout fails open. Low — the response stream was disposed unread, aborting the connection instead of returning it to the pool. The one requested byte is drained. Nit — fully-qualified RangeHeaderValue replaced with a using. docs/decisions.md corrected where it overstated: the switch label's role, the "fixes the class for all three media servers" claim (external-JSON channels bypass ValidatePlayoutItemPath entirely -- filed as #480), and the unmeasured latency assertion. Deferred HEAD-instead-of-GET recorded with its reason rather than silently dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
144 lines
5.4 KiB
C#
144 lines
5.4 KiB
C#
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();
|
|
}
|
|
|
|
[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<ILogger<HttpRemoteStreamProber>>());
|
|
|
|
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<ILogger<HttpRemoteStreamProber>>());
|
|
|
|
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<OperationCanceledException>(() => prober.IsAvailable(Url, cts.Token));
|
|
}
|
|
|
|
private static HttpRemoteStreamProber ProberReturning(HttpStatusCode statusCode, string finalUri = null) =>
|
|
new(
|
|
new StubHttpClientFactory(new StatusCodeHttpMessageHandler(statusCode, finalUri)),
|
|
Substitute.For<ILogger<HttpRemoteStreamProber>>());
|
|
|
|
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<HttpResponseMessage> 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 ThrowingHttpMessageHandler(Exception exception) : HttpMessageHandler
|
|
{
|
|
protected override Task<HttpResponseMessage> SendAsync(
|
|
HttpRequestMessage request,
|
|
CancellationToken cancellationToken) =>
|
|
Task.FromException<HttpResponseMessage>(exception);
|
|
}
|
|
}
|