diff --git a/backend/src/CodeSpace.Core/Services/Agents/HeartbeatLoop.cs b/backend/src/CodeSpace.Core/Services/Agents/HeartbeatLoop.cs index dc61e8271..f9d734a77 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/HeartbeatLoop.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/HeartbeatLoop.cs @@ -12,8 +12,9 @@ public static class HeartbeatLoop /// /// Wait , then invoke ; repeat until /// fires. A ping that throws (a transient DB blip) is reported to - /// and the loop continues — a missed heartbeat must never kill liveness. - /// Returns cleanly when cancelled; never surfaces to the caller. + /// and the loop continues — a missed heartbeat must never kill liveness, and neither + /// may a reporter that throws in turn. So the loop completes only by its own cancellation, and then cleanly: never + /// faulted, never surfacing to the caller. /// The first ping is deferred by one interval because the claim already stamped an initial heartbeat. /// /// exists so the cadence can be driven deterministically in a test instead @@ -45,7 +46,7 @@ public static async Task RunAsync(Func ping, TimeSpan i } catch (Exception ex) { - onPingError(ex); + ReportQuietly(onPingError, ex); } } } @@ -54,4 +55,22 @@ public static async Task RunAsync(Func ping, TimeSpan i // Expected: the harness finished or the worker is stopping. Not an error. } } + + /// + /// Hands a failed ping to the caller's reporter and lets nothing the reporter throws escape. Every caller awaits the + /// loop in the finally around the work it protects, so a reporter's fault that faulted the loop would replace + /// that work's result with the error of a log line, and a reporter's caught + /// by the loop's own cancellation exit would end it early, stopping liveness while the work ran on. + /// + private static void ReportQuietly(Action onPingError, Exception exception) + { + try + { + onPingError(exception); + } + catch (Exception) + { + // The reporter was the one place to say the ping failed, and it failed too; there is nowhere left to say it. + } + } } diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/HeartbeatLoopTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/HeartbeatLoopTests.cs index fb02e182a..d83241714 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/HeartbeatLoopTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/HeartbeatLoopTests.cs @@ -108,6 +108,50 @@ public async Task A_failing_ping_is_reported_but_does_not_kill_the_loop() await loop; // a loop whose every ping threw still returns cleanly on cancel, never surfacing the failure } + /// + /// The reporter is the caller's code, and every caller awaits this loop in the finally around the work it + /// protects, so a reporter that throws may end the loop no more than a failing ping may. It did: the report ran outside + /// the ping's guard, so a reporter's fault faulted the loop — and the awaiting finally surfaced it IN PLACE of + /// the result the loop was keeping alive — while a reporter's ended the loop + /// early and without a word, stopping liveness while the work ran on. Both are a loop that completed by something other + /// than its own cancellation, and both red here as a loop that never arms its next beat. + /// + [Theory] + [InlineData(typeof(InvalidOperationException))] + [InlineData(typeof(OperationCanceledException))] + public async Task A_reporter_that_throws_neither_faults_nor_ends_the_loop(Type faultType) + { + var time = new HeartbeatClock(); + var interval = TimeSpan.FromSeconds(30); + var reported = new SemaphoreSlim(0); + var pings = 0; + using var cts = new CancellationTokenSource(); + + // Released BEFORE the reporter throws, since nothing after the throw runs — and after the ping has counted, so + // each signal reads a settled count. + var loop = HeartbeatLoop.RunAsync( + _ => { Interlocked.Increment(ref pings); throw new InvalidOperationException("transient db blip"); }, + interval, + _ => { reported.Release(); throw (Exception)Activator.CreateInstance(faultType, "the reporter itself failed")!; }, + cts.Token, + time); + + for (var i = 1; i <= 3; i++) + { + await AdvanceOneIntervalAsync(time, reported, interval, i); + + Volatile.Read(ref pings).ShouldBe(i, "a reporter that threw must not stop, skip, or double the cadence"); + } + + cts.Cancel(); + + // Recorded rather than asserted with Should.NotThrowAsync, which passes a CANCELED task without a word. Bounded, so + // a loop that ignores the cancel fails instead of hanging. + var escaped = await Record.ExceptionAsync(() => loop.WaitAsync(TimeSpan.FromSeconds(10))); + + escaped.ShouldBeNull("the loop completes only by its own cancellation, and quietly — whatever escapes it is what the awaiting finally surfaces in place of the result it protects"); + } + [Fact] public async Task Returns_without_pinging_when_already_cancelled() {