From 3dbee7c7c80f5424fda85bb500cf51b6cd8f536e Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Wed, 23 Sep 2026 21:03:49 +0800 Subject: [PATCH] Keep a heartbeat loop alive when its error reporter throws HeartbeatLoop.RunAsync handed a failed ping to the caller's onPingError outside the ping's guard. A reporter that threw therefore faulted the loop, and every caller awaits the loop in the finally around the work it protects: AgentRunExecutor's launch and re-attach heartbeats, and the three supervisor grading call sites. The reporter's exception then replaced the protected result, and in AgentRunExecutor it also skipped the credential revoke and workspace cleanup that follow the await. A reporter that threw an OperationCanceledException was caught by the loop's own cancellation exit instead, ending the heartbeat early while the work ran on. Guard the report so nothing it throws escapes. The loop now completes only by its own cancellation, never faulted, however the caller's reporter is wired. --- .../Services/Agents/HeartbeatLoop.cs | 25 +++++++++-- .../Workflows/HeartbeatLoopTests.cs | 44 +++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) 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() {