From d91d6ee1ed0c36c654e77c251b3becd235187578 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 22:49:07 +0200 Subject: [PATCH 1/4] feat(472): sub-split the HLS cold-start startup phase #350's measurement showed `startup` is 81% of tune-in latency and carries 100% of its variance, while remaining one opaque bucket spanning FFmpeg spawn -> input open/probe -> encoder init -> first GOP. Two hypotheses survive that measurement (NFS input open vs VAAPI init under contention) and they need opposite fixes, so split before optimizing. Adds `prep` (ErsatzTV-side work before FFmpeg exists) + `ffmpegInit` (launch -> first `-progress` output) + `firstGop` (-> live.m3u8 exists) to the existing Information-level cold-start line. The pipeline runs `-loglevel error -nostats -hide_banner`, so a healthy FFmpeg writes nothing to stderr; the `-progress` stream is the only zero-cost milestone available and `ffmpegInit` therefore still lumps input-open with encoder-init. That limit is documented rather than papered over, and the split degrades to the two-way form #472 accepts when no progress arrives before the playlist. Log-only: no transcode behavior change, no new endpoint or config knob. fixes #472 --- .../Commands/StartFFmpegSessionHandler.cs | 10 ++ .../Streaming/HlsSessionWorker.cs | 39 ++++++- .../FFmpeg/ColdStartStartupSplitTests.cs | 107 ++++++++++++++++++ ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs | 101 +++++++++++++++++ .../FFmpeg/PlaylistSegmentsResult.cs | 4 +- 5 files changed, 258 insertions(+), 3 deletions(-) create mode 100644 ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs create mode 100644 ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs diff --git a/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs b/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs index 6cc07bb99..d155510dc 100644 --- a/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs +++ b/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs @@ -131,9 +131,15 @@ public class StartFFmpegSessionHandler : IRequestHandler + Base + (long)(milliseconds / 1000.0 * Stopwatch.Frequency); + + [Test] + public void Should_Split_Three_Ways_When_All_Milestones_Present() + { + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(0), + At(150), + At(1200), + At(1600)); + + split.Kind.ShouldBe(ColdStartStartupSplitKind.ThreeWay); + split.Prep.TotalMilliseconds.ShouldBe(150, 1); + split.FFmpegInit.TotalMilliseconds.ShouldBe(1050, 1); + split.FirstGop.TotalMilliseconds.ShouldBe(400, 1); + } + + [Test] + public void Sub_Phases_Should_Sum_To_The_Whole_Startup_Phase() + { + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(0), + At(150), + At(1200), + At(1600)); + + (split.Prep + split.FFmpegInit + split.FirstGop).TotalMilliseconds.ShouldBe(1600, 1); + } + + [Test] + public void Should_Fall_Back_To_Two_Way_Split_When_FFmpeg_Never_Reported_Progress() + { + // no -progress output before the playlist appeared: ffmpegInit must absorb the remainder + // rather than the split inventing a firstGop boundary that was never observed + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(0), + At(150), + 0, + At(1600)); + + split.Kind.ShouldBe(ColdStartStartupSplitKind.TwoWay); + split.Prep.TotalMilliseconds.ShouldBe(150, 1); + split.FFmpegInit.TotalMilliseconds.ShouldBe(1450, 1); + split.FirstGop.ShouldBe(TimeSpan.Zero); + } + + [Test] + public void Should_Fall_Back_To_Two_Way_Split_When_Progress_Arrived_After_The_Playlist() + { + // the playlist is observed on the request thread while progress is recorded on the worker + // thread; a progress milestone outside the phase must not produce a negative bucket + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(0), + At(150), + At(1800), + At(1600)); + + split.Kind.ShouldBe(ColdStartStartupSplitKind.TwoWay); + split.FFmpegInit.TotalMilliseconds.ShouldBe(1450, 1); + split.FirstGop.ShouldBe(TimeSpan.Zero); + } + + [TestCase(0L, 150L, 1200L, 1600L, TestName = "Run never started")] + [TestCase(100L, 0L, 0L, 1600L, TestName = "Process never launched")] + [TestCase(100L, 150L, 1200L, 0L, TestName = "Playlist never appeared")] + public void Should_Be_Unavailable_When_A_Required_Milestone_Is_Missing( + long runStarted, + long processLaunched, + long firstProgress, + long playlistExists) + { + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + runStarted == 0 ? 0 : At(runStarted), + processLaunched == 0 ? 0 : At(processLaunched), + firstProgress == 0 ? 0 : At(firstProgress), + playlistExists == 0 ? 0 : At(playlistExists)); + + split.ShouldBe(ColdStartStartupSplit.Unavailable); + } + + [Test] + public void Should_Clamp_Rather_Than_Report_A_Negative_Prep() + { + // defensive: launch cannot precede Run entry, but telemetry must never show a negative + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(500), + At(150), + At(1200), + At(1600)); + + split.Prep.ShouldBe(TimeSpan.Zero); + } +} diff --git a/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs new file mode 100644 index 000000000..f1dfc7454 --- /dev/null +++ b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs @@ -0,0 +1,101 @@ +using System.Diagnostics; + +namespace ErsatzTV.Core.FFmpeg; + +/// +/// How finely a cold-start's startup phase could be broken down (#472). +/// +public enum ColdStartStartupSplitKind +{ + /// No split available (the FFmpeg process was never launched, or the playlist never appeared). + Unavailable = 0, + + /// + /// Two-way split: prep + ffmpegInit, where ffmpegInit runs all the way to the + /// playlist appearing because FFmpeg emitted no progress output before then. + /// + TwoWay = 1, + + /// Three-way split: prep + ffmpegInit + firstGop. + ThreeWay = 2 +} + +/// +/// Sub-split of the HLS cold-start startup phase (#472), which #350's measurement showed to be +/// 81% of tune-in latency and to carry 100% of its variance while remaining a single opaque bucket. +/// +/// is ErsatzTV-side work before FFmpeg exists: playout-item resolution, pipeline +/// build, graphics-engine spawn. is FFmpeg from launch until it first reports +/// progress — input open + probe (the NFS hypothesis) plus decoder/encoder init (the VAAPI-contention +/// hypothesis). is from that first progress report until live.m3u8 exists. +/// +/// +/// Because the pipeline runs -loglevel error -nostats -hide_banner, a healthy FFmpeg writes +/// nothing to stderr, so input-open and encoder-init cannot be separated from each other without +/// changing the FFmpeg command — which this instrumentation deliberately does not do. The +/// -progress stream on stdout is therefore the only zero-cost milestone available, and +/// necessarily lumps those two candidates together. #472 accepts this: a +/// large vs a large is itself the first discrimination, +/// and it is honest about what it cannot yet see. +/// +/// +public readonly record struct ColdStartStartupSplit( + TimeSpan Prep, + TimeSpan FFmpegInit, + TimeSpan FirstGop, + ColdStartStartupSplitKind Kind) +{ + public static ColdStartStartupSplit Unavailable => + new(TimeSpan.Zero, TimeSpan.Zero, TimeSpan.Zero, ColdStartStartupSplitKind.Unavailable); + + /// + /// Builds the split from four milestones; 0 means the + /// milestone never happened. Milestones are recorded on different threads (the session worker + /// records the launch and progress ones; the request thread observes the playlist), so ordering is + /// validated rather than assumed: any out-of-order or missing milestone degrades the result to a + /// coarser instead of producing a negative or invented bucket. + /// + public static ColdStartStartupSplit FromTimestamps( + long runStarted, + long processLaunched, + long firstProgress, + long playlistExists) + { + if (runStarted <= 0 || processLaunched <= 0 || playlistExists <= 0) + { + return Unavailable; + } + + // the worker's Run entry strictly precedes every later milestone; clamp anyway so a clock + // oddity can never surface as a negative duration in telemetry + TimeSpan prep = Elapsed(runStarted, processLaunched); + + bool hasProgressMilestone = firstProgress > 0 + && firstProgress >= processLaunched + && firstProgress <= playlistExists; + + if (!hasProgressMilestone) + { + // FFmpeg never reported progress before the playlist appeared: fall back to the two-way + // split #472 explicitly accepts, rather than attributing the whole phase to a boundary + // that was never observed + return new ColdStartStartupSplit( + prep, + Elapsed(processLaunched, playlistExists), + TimeSpan.Zero, + ColdStartStartupSplitKind.TwoWay); + } + + return new ColdStartStartupSplit( + prep, + Elapsed(processLaunched, firstProgress), + Elapsed(firstProgress, playlistExists), + ColdStartStartupSplitKind.ThreeWay); + } + + private static TimeSpan Elapsed(long from, long to) + { + TimeSpan elapsed = Stopwatch.GetElapsedTime(from, to); + return elapsed < TimeSpan.Zero ? TimeSpan.Zero : elapsed; + } +} diff --git a/ErsatzTV.Core/FFmpeg/PlaylistSegmentsResult.cs b/ErsatzTV.Core/FFmpeg/PlaylistSegmentsResult.cs index 83964430a..aeb30799b 100644 --- a/ErsatzTV.Core/FFmpeg/PlaylistSegmentsResult.cs +++ b/ErsatzTV.Core/FFmpeg/PlaylistSegmentsResult.cs @@ -6,6 +6,7 @@ namespace ErsatzTV.Core.FFmpeg; /// FFmpeg process spawn + probe + libass/encoder init + first GOP, since the wait begins right /// after the fire-and-forget worker is launched); is Phase B (playlist /// exists -> the requested number of segments are present, or the 8s deadline). +/// breaks Phase A down further (#472). /// public readonly record struct PlaylistSegmentsResult( TimeSpan ProcessStartup, @@ -13,4 +14,5 @@ public readonly record struct PlaylistSegmentsResult( int SegmentsReached, int InitialSegmentCount, bool DeadlineExpired, - ColdStartFeatures Features); + ColdStartFeatures Features, + ColdStartStartupSplit StartupSplit); -- 2.47.3 From 757fb761511ff2127d0b899778e05f1591660288 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 23:05:14 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(472):=20review=20fixes=20=E2=80=94=20ho?= =?UTF-8?q?nest=20bucket=20boundaries,=20stale-playlist=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the cold adversarial review of the initial diff. No blockers were found; these address what the numbers MEAN, which is the whole point of an instrumentation change. - The buckets span the worker's Run entry, not the startup stopwatch, so prep overlaps the tail of `setup`. Rather than let the log imply an invariant it does not satisfy, say "spans runEntry" in the line, spell it out in the doc comment, and rename the test that had codified the false `sum == startup` claim. - Guard `processLaunched > playlistExists` -> Unavailable: a stale live.m3u8 from a previous session (Run warns about a non-empty transcode folder but does not delete it) would otherwise yield a plausible-looking sample whose prep exceeds the measured phase. - Split the two-way fallback into TwoWay vs TwoWayLateProgress. They are different stories about the pipeline and discriminating stories is what this issue is for. - Document the 100ms playlist-poll quantization (it lands entirely in firstGop, the smallest bucket) and the first-process-failed case where ffmpegInit spans a retry. - Short-circuit the per-line timestamp call; static readonly Unavailable. - Tests for the new guard, progress-before-launch, and boundary equality (so tightening >= to > later cannot pass silently). --- .../Commands/StartFFmpegSessionHandler.cs | 14 ++-- .../Streaming/HlsSessionWorker.cs | 8 ++- .../FFmpeg/ColdStartStartupSplitTests.cs | 55 ++++++++++++++- ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs | 70 ++++++++++++++----- 4 files changed, 122 insertions(+), 25 deletions(-) diff --git a/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs b/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs index d155510dc..abe01b94a 100644 --- a/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs +++ b/ErsatzTV.Application/Streaming/Commands/StartFFmpegSessionHandler.cs @@ -131,15 +131,19 @@ public class StartFFmpegSessionHandler : IRequestHandler -/// How finely a cold-start's startup phase could be broken down (#472). +/// How finely a cold-start's startup work could be broken down (#472). /// public enum ColdStartStartupSplitKind { - /// No split available (the FFmpeg process was never launched, or the playlist never appeared). + /// + /// No split available: the FFmpeg process was never launched, the playlist never appeared, or the + /// playlist was observed before FFmpeg was launched (a stale playlist from a previous session). + /// Unavailable = 0, /// - /// Two-way split: prep + ffmpegInit, where ffmpegInit runs all the way to the - /// playlist appearing because FFmpeg emitted no progress output before then. + /// Two-way split: prep + ffmpegInit, because FFmpeg emitted no progress output at all + /// before the playlist appeared. ffmpegInit therefore runs to the playlist. /// TwoWay = 1, + /// + /// Two-way split, distinguished because FFmpeg did report progress but only after the + /// playlist was observed. Same buckets as ; kept separate because it means the + /// playlist appeared before the first progress report rather than FFmpeg being silent, which is a + /// different story about the pipeline (and is also what the 100ms playlist poll can manufacture). + /// + TwoWayLateProgress = 2, + /// Three-way split: prep + ffmpegInit + firstGop. - ThreeWay = 2 + ThreeWay = 3 } /// -/// Sub-split of the HLS cold-start startup phase (#472), which #350's measurement showed to be -/// 81% of tune-in latency and to carry 100% of its variance while remaining a single opaque bucket. +/// Sub-split of the HLS cold-start startup work (#472), which #350's measurement showed to be 81% of +/// tune-in latency and to carry 100% of its variance while remaining a single opaque bucket. /// /// is ErsatzTV-side work before FFmpeg exists: playout-item resolution, pipeline /// build, graphics-engine spawn. is FFmpeg from launch until it first reports @@ -30,6 +41,13 @@ public enum ColdStartStartupSplitKind /// hypothesis). is from that first progress report until live.m3u8 exists. /// /// +/// These buckets span the session worker's Run entry to the playlist appearing, which is NOT +/// exactly the logged startup phase: the worker is launched fire-and-forget slightly before +/// the request thread starts the startup stopwatch, so overlaps the tail of +/// the logged setup bucket (in practice one config read). Do not expect +/// prep + ffmpegInit + firstGop to equal startup — it is a superset by that overlap. +/// +/// /// Because the pipeline runs -loglevel error -nostats -hide_banner, a healthy FFmpeg writes /// nothing to stderr, so input-open and encoder-init cannot be separated from each other without /// changing the FFmpeg command — which this instrumentation deliberately does not do. The @@ -38,6 +56,14 @@ public enum ColdStartStartupSplitKind /// large vs a large is itself the first discrimination, /// and it is honest about what it cannot yet see. /// +/// +/// Two further caveats when reading these numbers. The playlist is detected by a 100ms poll, so its +/// timestamp is up to 100ms late and that error lands entirely in — the +/// smallest bucket — and can also flip a sample between +/// and . And if the session's first FFmpeg +/// process fails and a second one produces the playlist, spans the first +/// process's whole lifetime plus the retry while still being labelled as one process's init. +/// /// public readonly record struct ColdStartStartupSplit( TimeSpan Prep, @@ -45,7 +71,7 @@ public readonly record struct ColdStartStartupSplit( TimeSpan FirstGop, ColdStartStartupSplitKind Kind) { - public static ColdStartStartupSplit Unavailable => + public static readonly ColdStartStartupSplit Unavailable = new(TimeSpan.Zero, TimeSpan.Zero, TimeSpan.Zero, ColdStartStartupSplitKind.Unavailable); /// @@ -66,19 +92,22 @@ public readonly record struct ColdStartStartupSplit( return Unavailable; } + if (processLaunched > playlistExists) + { + // the playlist was observed before FFmpeg was even launched, so it is a stale file from a + // previous session (Run warns about a non-empty transcode folder but does not delete it). + // Every bucket would be meaningless; report nothing rather than a plausible-looking sample + return Unavailable; + } + // the worker's Run entry strictly precedes every later milestone; clamp anyway so a clock // oddity can never surface as a negative duration in telemetry TimeSpan prep = Elapsed(runStarted, processLaunched); - bool hasProgressMilestone = firstProgress > 0 - && firstProgress >= processLaunched - && firstProgress <= playlistExists; - - if (!hasProgressMilestone) + if (firstProgress <= 0 || firstProgress < processLaunched) { - // FFmpeg never reported progress before the playlist appeared: fall back to the two-way - // split #472 explicitly accepts, rather than attributing the whole phase to a boundary - // that was never observed + // FFmpeg reported no usable progress before the playlist appeared: fall back to the two-way + // split #472 explicitly accepts, rather than inventing a boundary that was never observed return new ColdStartStartupSplit( prep, Elapsed(processLaunched, playlistExists), @@ -86,6 +115,15 @@ public readonly record struct ColdStartStartupSplit( ColdStartStartupSplitKind.TwoWay); } + if (firstProgress > playlistExists) + { + return new ColdStartStartupSplit( + prep, + Elapsed(processLaunched, playlistExists), + TimeSpan.Zero, + ColdStartStartupSplitKind.TwoWayLateProgress); + } + return new ColdStartStartupSplit( prep, Elapsed(processLaunched, firstProgress), -- 2.47.3 From 58b93d3e3dd8038f75e701d9428270bb9605a77d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 23:21:03 +0200 Subject: [PATCH 3/4] docs(472): correct the stale-playlist rationale; pin guard precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the fix commit returned MERGEABLE with one Medium: the comment justifying the stale-playlist guard misstated the mechanism, in three places. It claimed Run "warns about a non-empty transcode folder but does not delete it" — but StartFFmpegSessionHandler.FolderMustBeEmpty calls EmptyFolder BEFORE the worker spawns, and Run's finally empties it again. Verified directly rather than taken on the reviewer's word. The real residual path is EmptyFolder FAILING: it swallows every exception into a LogWarning and continues. Say that instead. On a PR whose entire value is that the numbers mean what they say, a rationale comment that misstates the mechanism is the same class of defect the PR exists to prevent, so it does not get to ship as a nit. Also documents that the guard is best-effort rather than a proof (if the wipe failed, before-or-after-launch is a scheduling race, so an unlucky sample can still slip through as an implausibly fast ThreeWay), and adds the test the reviewer noted was missing: guard precedence over an otherwise-valid ThreeWay. Comments and one test only; no logic change. --- .../FFmpeg/ColdStartStartupSplitTests.cs | 20 ++++++++++++++++--- ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs | 16 ++++++++++----- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs b/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs index d04261b54..7e3f61482 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs @@ -46,9 +46,9 @@ public class ColdStartStartupSplitTests [Test] public void Should_Be_Unavailable_When_The_Playlist_Predates_The_Process_Launch() { - // a stale live.m3u8 from a previous session: Run warns about a non-empty transcode folder but - // does not delete it. Every bucket would be meaningless, so report nothing rather than a - // plausible-looking sample with a prep that exceeds the whole measured phase + // a stale live.m3u8 survives when the handler's pre-session folder wipe fails (EmptyFolder + // swallows the failure into a warning). Every bucket would be meaningless, so report nothing + // rather than a plausible-looking sample with a prep that exceeds the whole measured phase ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( At(0), At(1600), @@ -58,6 +58,20 @@ public class ColdStartStartupSplitTests split.ShouldBe(ColdStartStartupSplit.Unavailable); } + [Test] + public void Stale_Playlist_Guard_Should_Take_Precedence_Over_An_Otherwise_Valid_Three_Way() + { + // the guard must win even when the progress milestone would otherwise satisfy ThreeWay — + // that precedence is the ordering that actually matters + ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( + At(0), + At(1600), + At(1700), + At(150)); + + split.ShouldBe(ColdStartStartupSplit.Unavailable); + } + [Test] public void Should_Fall_Back_To_Two_Way_Split_When_Progress_Predates_The_Process_Launch() { diff --git a/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs index 4a1477a48..9b94f287f 100644 --- a/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs +++ b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs @@ -9,7 +9,8 @@ public enum ColdStartStartupSplitKind { /// /// No split available: the FFmpeg process was never launched, the playlist never appeared, or the - /// playlist was observed before FFmpeg was launched (a stale playlist from a previous session). + /// playlist was observed before FFmpeg was launched (a stale playlist left behind because the + /// pre-session transcode-folder wipe failed — it logs a warning and continues). /// Unavailable = 0, @@ -57,12 +58,16 @@ public enum ColdStartStartupSplitKind /// and it is honest about what it cannot yet see. /// /// -/// Two further caveats when reading these numbers. The playlist is detected by a 100ms poll, so its +/// Three further caveats when reading these numbers. The playlist is detected by a 100ms poll, so its /// timestamp is up to 100ms late and that error lands entirely in — the /// smallest bucket — and can also flip a sample between /// and . And if the session's first FFmpeg /// process fails and a second one produces the playlist, spans the first -/// process's whole lifetime plus the retry while still being labelled as one process's init. +/// process's whole lifetime plus the retry while still being labelled as one process's init. And the +/// stale-playlist guard below is best-effort rather than a proof: if the folder wipe failed, whether +/// the stale playlist is observed before or after the launch milestone is a scheduling race, so an +/// unlucky sample could still slip through as an implausibly fast +/// . /// /// public readonly record struct ColdStartStartupSplit( @@ -94,8 +99,9 @@ public readonly record struct ColdStartStartupSplit( if (processLaunched > playlistExists) { - // the playlist was observed before FFmpeg was even launched, so it is a stale file from a - // previous session (Run warns about a non-empty transcode folder but does not delete it). + // the playlist was observed before FFmpeg was even launched, so it is a stale file: the + // handler wipes the transcode folder before starting the session, but that wipe swallows + // its failures into a warning (LocalFileSystem.EmptyFolder) and continues. // Every bucket would be meaningless; report nothing rather than a plausible-looking sample return Unavailable; } -- 2.47.3 From 37674d6519d9655204a7def06fbac7466075a30d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 23:33:28 +0200 Subject: [PATCH 4/4] test(472): name the stale-playlist test for what it actually pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-round review caught that Stale_Playlist_Guard_Should_Take_Precedence_Over_An_Otherwise_Valid_Three_Way describes an impossible case: ThreeWay requires processLaunched <= playlistExists, which is exactly the negation of the guard condition, so the guard can never preempt a ThreeWay. What the test really pins is precedence over the progress branches (TwoWayLateProgress) — still the ordering that matters. That is the same "rationale misstates the mechanism" defect the previous commit existed to fix, landed inside the fix itself. Renaming rather than leaving a test whose name teaches the next reader something false. Also broadens the escape-hatch caveat: a stale playlist that slips past the guard lands as TwoWay more often than ThreeWay, since FFmpeg has usually not reported progress that early. Test name and comments only; no logic change. --- ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs | 7 ++++--- ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs b/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs index 7e3f61482..ab9f189ae 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/ColdStartStartupSplitTests.cs @@ -59,10 +59,11 @@ public class ColdStartStartupSplitTests } [Test] - public void Stale_Playlist_Guard_Should_Take_Precedence_Over_An_Otherwise_Valid_Three_Way() + public void Stale_Playlist_Guard_Should_Take_Precedence_Over_The_Progress_Branches() { - // the guard must win even when the progress milestone would otherwise satisfy ThreeWay — - // that precedence is the ordering that actually matters + // without the guard, this input would be classified TwoWayLateProgress; the guard must be + // evaluated first. (It can never preempt a ThreeWay: that requires processLaunched <= + // playlistExists, which is exactly the negation of the guard condition.) ColdStartStartupSplit split = ColdStartStartupSplit.FromTimestamps( At(0), At(1600), diff --git a/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs index 9b94f287f..a94df97cd 100644 --- a/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs +++ b/ErsatzTV.Core/FFmpeg/ColdStartStartupSplit.cs @@ -66,8 +66,9 @@ public enum ColdStartStartupSplitKind /// process's whole lifetime plus the retry while still being labelled as one process's init. And the /// stale-playlist guard below is best-effort rather than a proof: if the folder wipe failed, whether /// the stale playlist is observed before or after the launch milestone is a scheduling race, so an -/// unlucky sample could still slip through as an implausibly fast -/// . +/// unlucky sample could still slip through as an implausibly fast one (most often +/// , since FFmpeg has usually not reported progress +/// that early). /// /// public readonly record struct ColdStartStartupSplit( -- 2.47.3