Skip to content

Say what keeps a checkpoint and settle two flaky waits - #2010

Merged
ppXD merged 1 commit into
mainfrom
fix/say-what-keeps-a-checkpoint-and-drive-two-flaky-waits-by-the-clock
Sep 23, 2026
Merged

ppXD merged 1 commit into
mainfrom
fix/say-what-keeps-a-checkpoint-and-drive-two-flaky-waits-by-the-clock

Conversation

@ppXD

@ppXD ppXD commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Summary

  • Name both keepers of a run's session-checkpoint columns — the reconciler's abandon and its spool recovery, which land through the same terminal CAS (AgentRunReconcilerService.CasTerminalAsync) — in the cancel and resumable-lookup comments of AgentRunService.cs and in ArtifactRetentionPolicy.cs, whose 162-character doc line is also re-wrapped to the file's width.
  • Behaviour change (goal text every retry lane emits): AgentRetryContinuity.HonestNoContinuityHint now ends "(your prior attempt pushed no branch of its own)" instead of "(no pushed branch was found to continue from)", which contradicted a dependent supervisor unit's "Continue from this branch" handoff in the same goal. LostHostPreamble now says "the machine or the process running your previous attempt was lost mid-run", because the reconciler abandons the same way for ProcessConfirmedDead and for a lapsed lease whose orphan it kills; LostHostPublishedBranchHint says the unpublished remainder "was lost with that attempt" instead of "died with the machine".
  • SupervisorTurnService.RunGradingHeartbeatLoopAsync takes its TimeProvider as a required parameter (the three call sites pass TimeProvider.System; no constructor changes), matching HeartbeatLoop.RunAsync since Run every heartbeat on the clock its caller passed #2005. SupervisorGradingHeartbeatTests drive a FakeTimeProvider and assert exactly one heartbeat per elapsed interval. The cancel test records the loop's outcome directly: Should.NotThrowAsync passes a canceled task, so it could never catch the escaping OperationCanceledException the test is named for.
  • Production fix: LoopbackModelCredentialBroker.Install calls AcceptAsync directly instead of through Task.Run, so the loop's first accept wait is registered before Install returns. A close that raced that registration was never delivered by the managed HttpListener, leaving the loop waiting forever with the lease still claimed — the cause of the A_lease_whose_listener_died_stops_being_claimed flake (open-then-break against the real broker: 23 of 1500 leases stuck before, 0 of 1500 after). The test now wakes on the drop's warning, its last effect, instead of polling HasLease, and the raw NUL byte in the file's ThisHost constant is written as \0 so grep stops skipping the file as binary.

Test plan

  • dotnet build backend/CodeSpace.sln: 0 errors
  • Unit: full suite, 10931 passed, 0 failed, 1 skipped
  • Integration: AgentNodeFlowTests, SupervisorRetryWorldStateFlowTests, AgentRunSessionCheckpointFlowTests, SupervisorPreClaimCrashRecoveryFlowTests (57/57); AgentRunExecutorTests including the credential-broker partial (109/109); AgentRunReattachFlowTests (13/13); SupervisorAcceptanceGradeFlowTests, SupervisorAcceptanceFoldFlowTests, SupervisorUnitAcceptanceFoldFlowTests (116/116)
  • Mutations: a heartbeat that ignores the handed clock, stops after one tick, lets the cancel escape, or ignores the cancel mid-sleep turns SupervisorGradingHeartbeatTests red; a drop that keeps the table entry, a silent drop, or an accept loop that swallows the failure turns A_lease_whose_listener_died_stops_being_claimed red
  • Sandbox: ModelCredentialBrokerNetnsE2ETests degrade-skips on macOS; the privileged Linux job is authoritative

The reconciler's spool recovery keeps a run's session-checkpoint columns
exactly as its abandon does: both land through the same terminal CAS,
which never touches them. Three comments named only the abandon; they
now name both, as ArtifactRetention already did.

Two retry notes said things that are not always true. The honest-redo
line ended "(no pushed branch was found to continue from)", which a
dependent supervisor unit reads in the same goal as its producer's
"Continue from this branch"; it now says the prior attempt pushed no
branch of its own. The lost-host preamble said the machine was lost,
but the reconciler abandons the same way when only the process died
(ProcessConfirmedDead, or a lapsed lease whose orphan it kills), so it
now says "the machine or the process", and the published-branch hint
no longer says the unpublished work died with the machine.

The grading heartbeat slept on the wall clock, so its tests could only
race real milliseconds. It now takes its clock from the caller as a
required parameter, as HeartbeatLoop.RunAsync has since #2005, and the
tests drive a FakeTimeProvider: exactly one heartbeat per elapsed
interval. Converting them showed the cancel test could never fail on
the escape it is named for, because Should.NotThrowAsync passes a
canceled task; it records the outcome directly now.

The listener-death flake was not a slow wait but a lost one. Install
handed the accept loop to Task.Run, so a close that landed while a pool
thread was still registering the loop's first wait was never delivered
by the managed HttpListener, and the loop waited forever with the lease
still claimed. Against the real broker, opening and immediately
breaking a lease left 23 of 1500 leases claimed; calling the loop
directly, which registers its first wait before Install returns, left
none. The test now wakes on the drop's warning, its last effect,
instead of polling HasLease. The raw NUL byte in that file's ThisHost
constant is written as an escape, so grep stops skipping the file as
binary.
@ppXD
ppXD merged commit f0c586d into main Sep 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant