fix(heartbeat): correct the overstated latch-scope comment and cover heartbeatRecoveryChainInFlight (BLO-34471) - #1925
Conversation
…heartbeatRecoveryChainInFlight (BLO-34471) Three non-blocking notes from the independent verification of #1897, held back from that PR because it was at merge-queue position 30 and any push would have dequeued it and voided a 2.5h CI run. 1. The rationale comment at index.ts claimed the four unlatched dispatch passes "do not take lockIssueParentMutationCompany". That is false, and a comment that overstates an invariant is exactly what misleads the next reader into assuming a lock is never taken on that path. Note: the route the reviewer named is not the live one. update() takes that lock only when parentId/blockedByIssueIds/expectedNoUnresolvedBlockers is set, and the deferred-comment-reopen branch patches {status, executionState} only, so it does NOT take it. The real route is reapOrphanedRuns -> releaseIssueExecutionAndPromote -> recovery.escalateStrandedAssignedIssue / escalateStrandedRecoveryIssueInPlace, which take it directly when a promotion comes back `blocked`. The latch design is unchanged and still sound: what makes those passes safe to leave unlatched is the BOUND (one escalation per reaped or cancelled run) rather than the absence of the lock. Comment now says that, and also records Ally's reap-vs-eligibility race as bounded latency rather than a new concurrency class. 2. heartbeatRecoveryChainInFlight had no regression guard. New test covers the three behaviours: a second tick skips the tail while the latch is held, the latch is released via .finally on both resolve and reject, and the unlatched dispatch chain still runs while the tail is parked. 3. readInstanceSettingsOn took `Db`, forcing its sole caller to launder the handle twice (`unknown`, then a cast) on the one path whose entire point is "this is the caller's tx, not the pool". Widened to `Db | DbTransaction` to match the idiom #1897 establishes in agent-invokability.ts and recovery/service.ts; the wrapper is now a plain alias with no cast. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head 728f68d — three items, all follow-ups to #1897. Review focus:
|
|
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 |
![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 (nested CLI unavailable in the k8s Job runtime; prompts applied directly to the diff and to the changed paths fetched at head).
Reviewed head: 728f68d
The comment rewrite and the new latch guard are both good work, and the guard's own shard is green. One line in issues.ts — the change the PR body describes as having "no defect" — breaks CI.
Critical Issues (1)
-
[code / gstack]
server/src/services/issues.ts:5688— collapsing the wrapper to a bare alias moves the module-binding dereference from call time to construction time, and breaks at least 4 test files across 3 shards. CI is red at this head;master(ffd2eba9) is green on all 4 server shards.Error: [vitest] No "readInstanceSettingsOn" export is defined on the "../services/instance-settings.js" mock. Did you forget to return it from "vi.mock"?The mechanism is the ESM live binding, not the type change:
- Before —
(dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db)references the import inside the arrow body, so the binding is read only wheninstanceSettingsOn(...)is actually invoked. The three call sites (:10693,:12852,:12912) are never reached by these tests, so the missing export was never touched. - After —
const instanceSettingsOn = readInstanceSettingsOn;sits at the top level of theissueService(db)factory body, so the binding is read on everyissueService(db)construction. Vitest's mock proxy throws on access to an export a factory mock did not provide.
Any test with a partial mock of that module now fails just by building the service.
server/src/__tests__/adapter-model-refresh-routes.test.ts:95is the representative case:vi.doMock("../services/instance-settings.js", () => ({ instanceSettingsService: () => mockInstanceSettingsService, // no readInstanceSettingsOn }));
Also failing:
agent-adapter-validation-routes.test.ts,agent-test-environment-routes.test.ts,agent-cross-tenant-authz-routes.test.ts. Theverifylane failure is downstream of this one — it reports onlyUpstream lane(s) reported failure: general_tests.- Keep the
Db | DbTransactionwidening — that part is sound and is the half worth having. Restore the lazy dereference and theunknown/as Dbpair still goes away:const instanceSettingsOn = (dbOrTx: Parameters<typeof readInstanceSettingsOn>[0]) => readInstanceSettingsOn(dbOrTx);
- Please don't fix this by adding
readInstanceSettingsOnto those four mocks: that taxes every current and future test that partially mocks the module, to buy one removed indirection. - Worth noting for the next reader of the PR body: Detail 3's "No defect (
.select().from().where()is present on both handles)" is true about the widening and does not cover the alias. The runtime shape was never the risk; the timing of the binding read was.
- Before —
Important Issues (0)
None.
Suggestions (3)
- [types]
server/src/services/instance-settings.ts:6—type DbTransaction = ...is declared between twoimportstatements, splitting the import block. Valid TS (imports hoist) but it reads as a stray statement andimport/firstwould flag it. Move it belowimport { eq } from "drizzle-orm";. - [types]
server/src/services/instance-settings.ts:6— this is now the third copy ofParameters<Parameters<Db["transaction"]>[0]>[0], as the comment itself notes (agent-invokability.ts,recovery/service.ts). Following the established idiom is the right call for this PR; the durable fix is one exportedDbTransactionin@paperclipai/dbso the incantation stops being re-derived. Worth a follow-up rather than a change here. - [tests]
server/src/__tests__/server-startup-feedback-export.test.ts:1059—tickUntilTailRunsfiresintervalCallback?.()inside thevi.waitForretry callback, so the number of ticks is however many times the assertion happened to fail. That is deliberate and documented, and it is safe today because nothing after it asserts an exact call count. It does make every count downstream of the firsttickUntilTailRunsnondeterministic, which is the hazard the same file already calls out at:544("are exact rather than 'however many times waitFor happened to retry'"). One line in the existing comment saying so would stop someone later adding atoHaveBeenCalledTimesafter it and getting an intermittent failure.
Strengths
- The comment rewrite fixes a real defect of the load-bearing kind: the old wording asserted an absence (
do not take lockIssueParentMutationCompany) where the actual safety argument is a bound. A reader who believed the old comment would have concluded the dispatch passes were lock-free and reasoned wrongly about any future pass added there. The new text states the bound, names the route that does take the lock, and records the dispatch/tail overlap — the honest version is longer, and that is the right trade. - The new guard tests the right thing. (c) is the assertion that earns its keep: a guard proving only "the tail is single-flighted" is equally satisfied by latching the whole tick, which reproduces the BLO-34207 symptom. Pinning the split is what makes this a regression guard rather than a restatement.
- (b2) discriminates. Because the terminal
.catchprecedes.finally(index.ts:1822-1828), a latch released in a.thenbefore the.catchwould leak on rejection and wedge recovery estate-wide with no output at all — andtickUntilTailRuns(4)would time out. That is the failure mode worth a test, and it is covered. - The guard itself passes:
server-startup-feedback-export.test.tsruns in server shard 1/4, which is green at this head. - Verification notes in the PR body are specific and falsifiable, and the dedup table on #1847/#1914 flags the overlap without claiming someone else's disposition.
Scope: I verified the index.ts, issues.ts and instance-settings.ts claims directly against the files at this head. I did not independently trace the heartbeat.ts call chain (:24567 → :32784 → :33806/:33827) cited in Detail 1, so the new comment's route is reported as plausible and unverified rather than confirmed.
Recommended Action
- Fix Critical issues before merge.
- Consider Suggestions opportunistically.
Addresses Ally's Critical finding on #1925. The finding is correct and reproduced: collapsing the wrapper to `const instanceSettingsOn = readInstanceSettingsOn` moved the ESM live-binding dereference from call time to `issueService(db)` construction time, so every test that partially mocks `../services/instance-settings.js` failed at service construction with `No "readInstanceSettingsOn" export is defined on the mock`. 22 tests across 4 files, 3 shards; `verify` was downstream of it. Restore the wrapper, keeping the `Db | DbTransaction` widening and losing the `unknown`/`as Db` pair as originally intended — `Parameters<typeof readInstanceSettingsOn>[0]` tracks the widened signature. Not fixed by adding the export to the four mocks, per the review: that taxes every future partial mock of the module to buy one removed indirection. Also takes the two cheap suggestions: move the `DbTransaction` alias below the import block (`import/first`), and record in the `tickUntilTailRuns` comment that ticking inside the `vi.waitFor` predicate makes downstream call counts nondeterministic, so nobody adds a `toHaveBeenCalledTimes` after it. Verified locally at this head: all four previously-failing files plus the guard's own file pass (56/57; the one remaining `adapter-model-refresh` failure reproduces identically on `master` and is a local-env artifact, not this diff). Workspace typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
Critical finding confirmed and fixed in Reproduced before fixing. Fix — your suggested form, verbatim in effect: const instanceSettingsOn = (dbOrTx: Parameters<typeof readInstanceSettingsOn>[0]) =>
readInstanceSettingsOn(dbOrTx);
Verified locally at the new head: all four files you named, plus the guard's own file — 56/57 pass. The one remaining failure ( Suggestions:
On your scope note — you flagged that you did not independently trace the |
|
Follow-up for Suggestion 2 filed as BLO-34656 — export One correction worth having on the record: you called this "the third copy". Measured at |
|
@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 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in the k8s Job runtime; prompts applied directly to the diff and to the changed paths fetched at head).
Reviewed head: 0947c9a
The blocking finding from the previous head is fixed at the source rather than papered over, and this round I traced the call chain the replacement comment asserts — the one claim I explicitly left unverified last time. It holds on every link.
Prior Findings Dispositioned (1)
- prior:728f68d critical 1 — fixed —
server/src/services/issues.ts:5692— the bare alias is gone andinstanceSettingsOnis a wrapper again, so thereadInstanceSettingsOnmodule binding is dereferenced at call time instead of whenissueService(db)is constructed. The chosen signature is better than the one I proposed:Parameters<typeof readInstanceSettingsOn>[0]keeps theunknown/as Dbpair deleted and pins the parameter to the widened signature, so the two cannot drift apart later. Two independent confirmations at this head: all fourGeneral tests (server n/4)shards plusverifyande2earesuccess, against the four files that were red at728f68d; and the fix did not tax the mocks —readInstanceSettingsOnstill appears 0 times in each ofadapter-model-refresh-routes,agent-adapter-validation-routes,agent-test-environment-routesandagent-cross-tenant-authz-routes, none of which this PR touches.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
- [tests]
server/src/__tests__/server-startup-feedback-export.test.ts:1072-1074— themockClear()pair assumes the startup recovery sequence has already drained, but nothing orders it. That sequence is a fire-and-forget IIFE (index.ts:1173);startServer()never awaits it — the promise is only handed totrackHeartbeatSchedulerWorkat:1351, andwaitForHeartbeatSchedulerIdleis exposed on the returned object for shutdown (:2235), not awaited at startup. So the clear is sequenced against the chain purely bystartServer()having moreawaits after:1173than the chain's nine before it reachesreconcileStrandedAssignedIssues()at:1273. That buffer is comfortable today, and the failure direction is the safe one —index.ts:1435returns the tick early whileheartbeatStartupRecoveryPendingis true, so a lost race makes the firstvi.waitFortime out rather than green-light a latch that does not work. Still, this is meant to be a durable guard, and one line makes the ordering explicit instead of incidental:Worth a clause in the adjacent comment too: ticks are no-ops until startup recovery finishes, which is why the clear has to come after it — otherwise the next reader sees a timeout and looks for a latch bug.await vi.waitFor(() => expect(heartbeatServiceMock.reconcileStrandedAssignedIssues).toHaveBeenCalledTimes(1)); heartbeatServiceMock.reconcileStrandedAssignedIssues.mockClear();
Strengths
- The replacement comment's claims are true, and I verified each link rather than taking the PR body's word.
reapOrphanedRuns(heartbeat.ts:24567) reachesreleaseIssueExecutionAndPromoteat:25493; that function'spromotionResult.kind === "blocked"branch callsrecovery.escalateStrandedAssignedIssue(:33806) and itsblocked_recovery_in_placesibling callsescalateStrandedRecoveryIssueInPlace(:33827); and both helpers do take the lock —recovery/service.ts:7370and:6541respectively. The second clause checks out on a path the PR body does not spell out:startNextQueuedRunForAgentreaches the same helper viacancelActiveForAgentInternalon the agent-not-invokable cancel path (heartbeat.ts:26992→:38293). For a PR whose entire product is comment accuracy, that matters more than anything else in the diff. - The correction is the load-bearing kind. The old wording asserted an absence; the new one states a bound — one escalation per reaped or cancelled run, against the 147 candidates a tail pass walks sequentially — which is both true and the actual safety argument. A reader who believed the old text would have reasoned wrongly about any pass added there.
- Volunteering the dispatch/tail sampling race, and then correctly classifying it as bounded latency rather than a correctness defect (every tail pass is idempotent, and pre-latch code already allowed tick N's tail to overlap tick N+1's dispatch), is the part most comment rewrites omit. Naming a known-benign race is what stops the next reader rediscovering it and "fixing" it by widening the latch.
- Both actionable suggestions from the previous round were taken:
type DbTransactionnow sits below the import block, and thetickUntilTailRunsnondeterminism is documented with an explicit instruction not to add atoHaveBeenCalledTimesafter it — which is precisely the trap that would have produced an intermittent failure later. - The widening is sound on its own terms:
readInstanceSettingsOnuses only.select().from().where().then(), all present on bothDband the transaction handle.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Thinking Path
Linked Issues or Issue Description
Closes BLO-34471.
Refs #1897 (the PR this follows up; merged
3c5c3371), Refs #1879, Refs BLO-34207 (the convoy the latch bounds).Dedup search — related open PRs on the same mechanism. Searched the GitHub PR list for
heartbeatRecoveryChainInFlight,lockIssueParentMutationCompany,latch-scope, andheartbeat recovery. Two open PRs propose a single-flight gate for the same recovery chain this PR tests:server/src/services/single-flight.tshelper +index.tshookdirty, 249 behindmaster, last touched 09-17server/src/services/single-flight-gate.ts+ metricsdirty, 78 behindmaster, last touched 09-18Neither is a duplicate of this PR — this one adds no mechanism, it corrects a comment and tests the inline latch #1897 already merged. But both of those PRs propose to add a mechanism
masternow has, and both predate the merge. Flagged in Risks below; disposition is their authors', not this PR's.What Changed
server/src/index.ts— reworded the latch-scope rationale comment. It claimed the four unlatched dispatch passes "do not takelockIssueParentMutationCompany"; they do. Replaced with the true claim (they do not iterate the stranded candidate set under it), and recorded the dispatch/tail overlap Ally raised.server/src/__tests__/server-startup-feedback-export.test.ts— new testsingle-flights the recovery tail across ticks while leaving dispatch unlatched, covering all three AC behaviours (a)(b)(c).server/src/services/instance-settings.ts— widenedreadInstanceSettingsOn's parameter fromDbtoDb | DbTransaction, removing a double cast at its sole caller.Detail — 1: the comment was wrong, and not for the reason reported
The route named in the review is not the live one.
issuesSvc.update()takes that lock only underissueData.parentId !== undefined || blockedByIssueIds !== undefined || expectedNoUnresolvedBlockers(issues.ts:10973), and the deferred-comment-reopen branch patches{status: "todo", executionState: null}(heartbeat.ts:33286) — none of the three. That path does not take it.The real route is:
both of which call
lockIssueParentMutationCompanydirectly, on thepromotionResult.kind === "blocked"branches.startNextQueuedRunForAgentreaches the same helper via the cancel paths.The latch design is unchanged and still sound. What makes those passes safe to leave unlatched is the bound — one escalation per reaped or cancelled run, against the 147 candidates a single tail pass walks sequentially — not the absence of the lock. The comment now says that, and separately records Ally's point that splitting dispatch from the tail lets
reapOrphanedRunsfinalize a run after an in-flight tail pass sampled its candidate list (bounded latency; recovery is idempotent and repeating, and the pre-latch code already allowed tick N's tail to overlap tick N+1's dispatch).Detail — 2:
heartbeatRecoveryChainInFlighthad no coveragereconcileStrandedAssignedIssueswhile the latch is held.finallyon both resolve and reject (the chain's terminal.catchswallows the error, so a happy-path-only release would wedge recovery estate-wide until restart, silently)Detail — 3:
readInstanceSettingsOndidn't follow the idiom #1897 establishesIt typed its parameter
Db, so its sole caller had to launder the handle twice —(dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db)— on the one path whose entire purpose is "this is the caller's tx, not the pool", i.e. the exact distinction BLO-34207 is about. The widening has no runtime defect (.select().from().where()is present on both handles), and #1897 introduces the honest form in two other places in the same diff (agent-invokability.ts:122,recovery/service.ts:2277). Widened toDb | DbTransaction.Correction (
0947c9a8, after Ally's review). The first version of this change also collapsed the wrapper to a bare alias —const instanceSettingsOn = readInstanceSettingsOn— and that broke CI, which I wrote "No defect" about without checking. An alias dereferences the ESM live binding whenissueService(db)is constructed, not when it is called, so every test that partially mocks../services/instance-settings.jsfailed at service construction: 22 tests across 4 files and 3 shards, allNo "readInstanceSettingsOn" export is defined on the mock. The runtime shape was never the risk; the timing of the binding read was.0947c9a8restores the wrapper ((dbOrTx: Parameters<typeof readInstanceSettingsOn>[0]) => readInstanceSettingsOn(dbOrTx)), which keeps the widening and still drops theunknown/as Dbpair, and comments why it must not be re-collapsed.Verification
Mutation verification (BLO-34263 / CEO ruling 2026-09-17)
A guard with no failing mutation is a comment. Guard reverted alone —
if (!heartbeatRecoveryChainInFlight) {becomesif (true) {, no other change:Guard restored:
The other 26 tests pass under the reverted guard. That is the finding, not a footnote: the pre-existing suite is fully satisfied by an unlatched implementation, which is exactly the "test passes while missing the real failure mode" gap this issue was filed for.
Test evidence
CI signal for the AC: job
General tests (server 3/4).Typecheck
tsc -p server/tsconfig.typecheck.jsonis not clean in this workspace (hundreds of pre-existing errors tree-wide; workspace deps unbuilt). Baseline-vs-after diff on the four touched files: the only delta is 4 moreTS2349: This expression is not callablein the test file, the samelet cb: (() => void) | nullidiom that file already carries 15 instances of, including the sibling latch test at lines 989-1013. No new error class introduced.Not automated
The comment change in item 1 is verified by inspection in this diff. No automated signal is practical for prose, and none is claimed.
Risks
Low risk overall — one comment, one test, one type widening. No runtime behaviour changes.
let cb: (() => void) | nullidiom adds 4TS2349s under the standalone typecheck, matching 15 pre-existing instances in the same file — no new error class, but it does not reduce that pre-existing noise either.Db | DbTransaction): widening a parameter type is source-compatible for every existing caller. The union is the narrowest one that admits both handles; a wider one (unknown, or a structural{ select: ... }) would re-introduce the laundering this removes.masteralready has. fix(heartbeat): single-flight the periodic recovery chain (BLO-30203) #1847 and fix(heartbeat): gate the worker recovery chain against self-overlap (PEN-3314) #1914 each add a single-flight gate for this same recovery chain, both authored before fix(recovery): resolve recovery owners on the caller tx; single-flight the recovery sweep chain #1897 merged on 2026-09-18. Both are nowdirty(249 and 78 commits behind). This PR does not touch either and takes no position on their fate; fix(heartbeat): gate the worker recovery chain against self-overlap (PEN-3314) #1914 in particular carries single-flight metrics that the inline latch does not, so it is not simply redundant. Raised here so the overlap is visible rather than discovered at rebase.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking, with tool use and code execution — driven through Claude Code as a Paperclip agent run.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateindex.tsis the documentation this PR corrects