From b2059bd2a6af00e17c332b72fad6ba0b2ba57615 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 17 Jul 2026 19:50:54 +0200 Subject: [PATCH 1/3] fix(320): break troubleshoot segment-wait loop on ffmpeg failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second (segment-readiness) poll in POST /api/v1/troubleshoot/playback/start only checked !hasSegments. If ffmpeg died after writing the HLS playlist but before the first segments appeared, the loop spun until the client cancelled — tying up the request thread and holding the troubleshooting lock (worse since #301 moved the manifest fetch server-side per #320's writeup). Mirror the first (playlist) wait loop's exit check so a dead ffmpeg or client cancellation breaks the wait; the existing !IsFailed guard then falls through to the terminal NotFoundProblem, releasing the thread + lock. Adds a non-vacuous regression test (verified it fails on the pre-fix spinning loop via a bounded cancellation deadline). fixes #320 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../TroubleshootControllerTests.cs | 79 +++++++++++++++++++ .../Controllers/Api/TroubleshootController.cs | 8 ++ 2 files changed, 87 insertions(+) diff --git a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs index e1314e7c6..0081de5c2 100644 --- a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs @@ -448,6 +448,85 @@ public class TroubleshootControllerTests } } + [Test] + public async Task TroubleshootPlayback_Should_Return_404_And_Not_Spin_When_Ffmpeg_Fails_Before_Segments() + { + // ersatztv#320: if ffmpeg dies after writing the playlist but before any segments appear, the + // segment-readiness poll must break on notifier.IsFailed instead of spinning until the client + // cancels (which would tie up the request thread + hold the troubleshooting lock). No segment + // files are seeded, so the only way this test completes without hitting the cancellation + // deadline is the IsFailed check. + _entityLocker.IsTroubleshootingPlaybackLocked().Returns(false); + + var playoutItemResult = new PlayoutItemResult( + new CliWrap.Command("ffmpeg"), + Option.None, + Option.Some(1)); + + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Either.Right(playoutItemResult)); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Either.Right(FakeMediaItemInfo())); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new TroubleshootingInfo( + "1.2.3", + new Dictionary(), + [], + [], + [], + new ErsatzTV.Application.FFmpegProfiles.FFmpegSettingsViewModel(), + [], + [], + [], + false, + false, + null, + null, + null, + null)); + + var fileSystem = Substitute.For(); + // playlist "exists" so the first wait loop exits immediately and we reach the segment poll + fileSystem.File.Exists(Arg.Any()).Returns(true); + + var configElementRepository = Substitute.For(); + configElementRepository + .GetValue(ConfigElementKey.FFmpegInitialSegmentCount, Arg.Any()) + .Returns(Option.Some(2)); + + var notifier = Substitute.For(); + notifier.IsFailed(Arg.Any()).Returns(true); + + var controller = new TroubleshootController( + System.Threading.Channels.Channel.CreateUnbounded().Writer, + fileSystem, + configElementRepository, + notifier, + _entityLocker, + _statusStore, + _mediator) + { + ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext { Request = { PathBase = "/etv" } } + } + }; + + // safety deadline so a regression (the loop spinning) fails the test in bounded time instead + // of hanging CI; the fixed code exits via IsFailed long before this fires + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + + IActionResult result = await controller.TroubleshootPlayback(DefaultPlaybackRequest(), cts.Token); + + var notFound = result.ShouldBeOfType(); + notFound.Value.ShouldBeOfType().Status.ShouldBe(404); + + // the loop must have exited via IsFailed, NOT by hitting the cancellation deadline — this is + // what makes the test non-vacuous (it fails on the pre-fix spinning loop) + cts.IsCancellationRequested.ShouldBeFalse(); + notifier.Received().RemoveSession(Arg.Any()); + } + [Test] public async Task GetPlaybackStatus_Should_Report_Idle_When_No_Result_And_Unlocked() { diff --git a/ErsatzTV/Controllers/Api/TroubleshootController.cs b/ErsatzTV/Controllers/Api/TroubleshootController.cs index 7fa49fb2c..67a16eea5 100644 --- a/ErsatzTV/Controllers/Api/TroubleshootController.cs +++ b/ErsatzTV/Controllers/Api/TroubleshootController.cs @@ -238,6 +238,14 @@ public class TroubleshootController( { await Task.Delay(TimeSpan.FromMilliseconds(100), cancellationToken); + // if ffmpeg died (or the client cancelled) before segments appeared, stop waiting + // instead of spinning until client-cancel; the !IsFailed guard below then falls + // through to the terminal NotFoundProblem, releasing the request thread + lock + if (cancellationToken.IsCancellationRequested || notifier.IsFailed(sessionId)) + { + break; + } + string[] segmentFiles = streamingMode switch { // StreamingMode.HttpLiveStreamingSegmenter => Directory.GetFiles( -- 2.47.3 From 5f8525eed76646d7cf90620ebf50119f60893603 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 17 Jul 2026 19:57:55 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(320):=20address=20cold-review=20nits=20?= =?UTF-8?q?=E2=80=94=20drop=20redundant=20cancel=20term,=20harden=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold review (PR #423) found two LOW nits: - The segment-wait break also checked cancellationToken.IsCancellationRequested, which could exit into the Ok gate and return a spurious 200 for a cancelled request with no segments. The term is redundant (Task.Delay already throws on cancel -> caught -> 404), so break on notifier.IsFailed only. - The regression test's non-vacuousness was filesystem-state dependent (absent troubleshooting folder -> Directory.GetFiles throws -> false pass pre-fix). Seed an empty folder like the sibling success test so the spin path is deterministic regardless of NUnit run order. Negative control re-verified: removing the IsFailed break fails the test in ~10s. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Controllers/TroubleshootControllerTests.cs | 16 +++++++++++++--- .../Controllers/Api/TroubleshootController.cs | 11 +++++++---- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs index 0081de5c2..bf0278db0 100644 --- a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs @@ -453,9 +453,19 @@ public class TroubleshootControllerTests { // ersatztv#320: if ffmpeg dies after writing the playlist but before any segments appear, the // segment-readiness poll must break on notifier.IsFailed instead of spinning until the client - // cancels (which would tie up the request thread + hold the troubleshooting lock). No segment - // files are seeded, so the only way this test completes without hitting the cancellation - // deadline is the IsFailed check. + // cancels (which would tie up the request thread + hold the troubleshooting lock). + // + // The segment scan uses the real Directory.GetFiles on the shared troubleshooting folder, so + // to make the pre-fix spin path deterministic (regardless of NUnit run order) we seed an EMPTY + // folder: it must exist (else Directory.GetFiles throws -> fast 404 -> false pass) and hold + // fewer than initialSegmentCount .ts files (so hasSegments never trips). With that guaranteed, + // the only thing that lets the fixed code complete before the cancellation deadline is IsFailed. + Directory.CreateDirectory(FileSystemLayout.TranscodeTroubleshootingFolder); + foreach (string stray in Directory.GetFiles(FileSystemLayout.TranscodeTroubleshootingFolder, "*.ts")) + { + File.Delete(stray); + } + _entityLocker.IsTroubleshootingPlaybackLocked().Returns(false); var playoutItemResult = new PlayoutItemResult( diff --git a/ErsatzTV/Controllers/Api/TroubleshootController.cs b/ErsatzTV/Controllers/Api/TroubleshootController.cs index 67a16eea5..cefeeee34 100644 --- a/ErsatzTV/Controllers/Api/TroubleshootController.cs +++ b/ErsatzTV/Controllers/Api/TroubleshootController.cs @@ -238,10 +238,13 @@ public class TroubleshootController( { await Task.Delay(TimeSpan.FromMilliseconds(100), cancellationToken); - // if ffmpeg died (or the client cancelled) before segments appeared, stop waiting - // instead of spinning until client-cancel; the !IsFailed guard below then falls - // through to the terminal NotFoundProblem, releasing the request thread + lock - if (cancellationToken.IsCancellationRequested || notifier.IsFailed(sessionId)) + // if ffmpeg died before segments appeared, stop waiting instead of spinning + // until client-cancel; the !IsFailed guard below then falls through to the + // terminal NotFoundProblem, releasing the request thread + lock. (Client + // cancellation is already handled by the Task.Delay above throwing; checking + // it here too would let a cancel exit into the Ok gate and return a spurious + // 200 for a request with no segments.) + if (notifier.IsFailed(sessionId)) { break; } -- 2.47.3 From de63603aab658e5d254bbcc0213f439d377e1a01 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 17 Jul 2026 20:03:10 +0200 Subject: [PATCH 3/3] test(320): don't blanket-delete foreign *.ts in the shared troubleshooting folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review nit: the regression test deleted every *.ts in the machine-global TranscodeTroubleshootingFolder, which could nuke a live troubleshooting session's segments on a dev machine (reaping files it didn't create). Drop the sweep and keep only Directory.CreateDirectory — the folder-exists guarantee is what closes the false-pass hole; NUnit runs serially and no test leaves >= 2 stray .ts, so determinism is unaffected (negative control re-verified: still fails in ~10s). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Controllers/TroubleshootControllerTests.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs index bf0278db0..0ceb3ce42 100644 --- a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs @@ -456,15 +456,14 @@ public class TroubleshootControllerTests // cancels (which would tie up the request thread + hold the troubleshooting lock). // // The segment scan uses the real Directory.GetFiles on the shared troubleshooting folder, so - // to make the pre-fix spin path deterministic (regardless of NUnit run order) we seed an EMPTY - // folder: it must exist (else Directory.GetFiles throws -> fast 404 -> false pass) and hold - // fewer than initialSegmentCount .ts files (so hasSegments never trips). With that guaranteed, - // the only thing that lets the fixed code complete before the cancellation deadline is IsFailed. + // ensure the folder EXISTS — otherwise Directory.GetFiles throws DirectoryNotFoundException, + // which the pre-fix loop would surface as a fast 404 (a false pass that hides the regression). + // We deliberately do NOT delete stray *.ts here: this folder is machine-global and may hold a + // live troubleshooting session's segments (don't reap files this test didn't create). It isn't + // needed for determinism — NUnit runs serially and the sibling Should_Return_200 test cleans up + // its own seg-test-*.ts in a finally, so no test leaves >= initialSegmentCount(2) files behind; + // and the fixed code breaks on IsFailed before the scan runs regardless of folder contents. Directory.CreateDirectory(FileSystemLayout.TranscodeTroubleshootingFolder); - foreach (string stray in Directory.GetFiles(FileSystemLayout.TranscodeTroubleshootingFolder, "*.ts")) - { - File.Delete(stray); - } _entityLocker.IsTroubleshootingPlaybackLocked().Returns(false); -- 2.47.3