test(startup): order the recovery-latch mockClear against startup recovery (BLO-34702) - #1938
Conversation
…overy (BLO-34702) The `mockClear()` pair in the `heartbeatRecoveryChainInFlight` regression guard was sequenced against the startup recovery chain only by `startServer()` happening to have more `await`s after the fire-and-forget IIFE (index.ts:1173) than the chain has before `reconcileStrandedAssignedIssues()` (:1273). Nothing ordered it. Wait on that pass explicitly instead. One wait orders both clears: `resumeQueuedRuns` (:1272) is awaited immediately before it in the same chain. The comment now records that a tick returns early at :1435 while `heartbeatStartupRecoveryPending` is true, which is why a lost race here shows up as a `vi.waitFor` timeout rather than a latch failure. Test-only; no behaviour change. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review at head 4973a42 — test-only ordering fix in |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4973a42
Test-only, one file, +15/-1. All five index.ts citations in the new comment were verified against this head and are exact: the fire-and-forget IIFE (:1173), resumeQueuedRuns (:1272) awaited immediately before reconcileStrandedAssignedIssues (:1273), trackHeartbeatSchedulerWork (:1351), and the early return under heartbeatStartupRecoveryPending (:1435). The "one wait orders both clears" claim holds — :1272 is awaited before :1273, so a wait on the latter proves the former already landed.
The change is strictly stronger than the status quo: it proceeds at max(startServer() return, :1273 called) where the old code proceeded at startServer() return alone, so it cannot sequence anything earlier than before.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[native-codex]
server/src/__tests__/server-startup-feedback-export.test.ts:1085— the new wait establishes that:1273was called, not that the recovery chain finished. Seven more awaited passes run between:1273and the.finallythat clearsheartbeatStartupRecoveryPendingat:1349. The un-retried tick two statements later (:1097) needs that flag false, per this diff's own comment — so the tick is still ordered only by the chain happening to drain insideawait startServer(), which is the implicit coupling this PR removes for the clears.- Low priority and not a defect today: the chain does drain there (that is why the test was green before this change), and the failure direction stays safe — a lost race times the
releaseTailwait out rather than green-lighting a dead latch. Worth a line in the comment noting the tick retains that dependency, so a future reader does not read the new wait as covering it.tickUntilTailRunsis not a drop-in here — the comment at:1060rules it out for this tick because a retried tick makes downstream call counts nondeterministic.
- Low priority and not a defect today: the chain does drain there (that is why the test was green before this change), and the failure direction stays safe — a lost race times the
-
[pr-review-toolkit:comments]
server/src/__tests__/server-startup-feedback-export.test.ts:1072— the block adds five absolute line-number references into a 2,414-line actively-edited file. All are correct at this head, and the convention is already established here (:1028citesindex.ts:1675, still accurate), so this is consistent rather than novel — but five more citations is five more things that drift silently. Anchoring on symbol names where one reads naturally (heartbeatStartupRecoveryPending's early return, thestartupHeartbeatRecoveryIIFE) would survive renumbering; no change needed if the file's convention is deliberate.
Strengths
- The mutation check is the right shape and is exactly what the 2026-09-17 ruling on BLO-34263 asks for: reverting
index.ts:1734toif (true)fails assertion (a) — the latch assertion — rather than timing out, which is what distinguishes "still hardened" from "passes on broken code". - The comment records the failure direction (
:1435early return ⇒ timeout, not a false green), which is the non-obvious part and the thing that would otherwise send the next reader hunting a latch bug. - Scope is honest: the PR body states plainly that the comment clause has no automated signal and was verified by inspection only.
- Filing rather than fixing in place on #1925 was correct — a push would have dequeued it from the merge queue.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Closes BLO-34702. Sole Suggestion from Ally's at-head review of #1925 (review
5255293806, head0947c9a8) — explicitly not a blocking finding; that review was 0 Critical / 0 Important. Filed rather than fixed in place because #1925 was in the merge queue at the time and a push would have dequeued it.Test-only. No behaviour change.
Thinking Path
mockClear()pair assumed the startup recovery sequence had already drained, but nothing in the test said so — it was sequenced bystartServer()happening to have moreawaits after the fire-and-forget IIFE than the chain has before it reachesreconcileStrandedAssignedIssues().startServer(), at which point the guard gets deleted rather than repaired — which is the actual cost, since this test is the whole product of BLO-34471.index.ts:1435returns the tick early whileheartbeatStartupRecoveryPendingis true, so a lost race times the firstvi.waitForout instead of green-lighting a dead latch. That is why it was filed as low priority — and also why the comment now records it, so the next reader who hits a timeout does not go hunting for a latch bug.resumeQueuedRuns(index.ts:1272) is awaited immediately beforereconcileStrandedAssignedIssues(:1273) in the same chain, so waiting on the latter proves the former already landed. A secondvi.waitForwould be noise.What Changed
server/src/__tests__/server-startup-feedback-export.test.ts— themockClear()pair in theheartbeatRecoveryChainInFlightregression guard is now preceded by an explicitawait vi.waitFor(...)onreconcileStrandedAssignedIssueshaving been called once, so the ordering against startup recovery is stated rather than inherited fromstartServer()'s await count.The adjacent comment records that ticks are no-ops while
heartbeatStartupRecoveryPendingis true (index.ts:1435), which is the reason the clear has to come after the chain reaches:1273.One file, +15/-1.
Verification
Test file green (27/27):
Mutation check (BLO-34471's, re-run against the edited test, per the 2026-09-17 CEO ruling on BLO-34263 — a guard test that does not fail with its guard reverted is documentation). Revert
server/src/index.ts:1734if (!heartbeatRecoveryChainInFlight) {→if (true) {, no other change:It fails on assertion (a) — the latch assertion itself — not on a timeout, which is what "still hardened, not broken" means here. Guard restored (
git checkout server/src/index.ts); the diff below is one file.tsc --noEmit -p server/tsconfig.jsonclean. CI:General tests (server 3/4)green at this head.Risks
Low, and bounded to this one test file — no product code changes, so nothing ships to runtime.
The realistic risk of a change like this is the opposite of the one it fixes: a wait added in the wrong place can make a guard pass on broken code. Ruled out by the mutation run above, which still fails on the latch assertion rather than timing out.
The change is also strictly weaker-bounded than the status quo — it proceeds at
max(startServer() return, :1273 called)where the old code proceeded atstartServer() returnalone, so it cannot sequence anything earlier than before. Worst case it is a no-op; it cannot introduce a new race.Residual, unchanged by this PR and deliberately not addressed here: the un-retried tick further down still depends on the recovery chain having fully drained (the
.finallythat clearsheartbeatStartupRecoveryPendingatindex.ts:1349runs several awaited passes after:1273). That dependency predates this change and keeps the same safe failure direction — a lost race times thereleaseTailwait out. Not folded in because doing so needs a push, which would void the clean at-head review for a comment line.Model Used
claude-opus-5[1m] (Claude Code, Paperclip Staff Engineer agent)
Not claimed
No automated signal for the comment clause — verified by inspection in this diff only.
🤖 Generated with Claude Code