From 728f68d14828f39b3c021b9bac4f31b8839d8174 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sat, 19 Sep 2026 00:24:35 +0000 Subject: [PATCH 1/2] fix(heartbeat): correct the overstated latch-scope comment and cover 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 --- .../server-startup-feedback-export.test.ts | 94 +++++++++++++++++++ server/src/index.ts | 28 +++++- server/src/services/instance-settings.ts | 6 +- server/src/services/issues.ts | 2 +- 4 files changed, 125 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index af6665165ad3..53bb3b75d530 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -1015,6 +1015,100 @@ describe("startServer feedback export wiring", () => { } }); + // BLO-34207 / BLO-34471: `heartbeatRecoveryChainInFlight` single-flights the + // lock-taking recovery tail across ticks. That latch is the whole mechanism + // bounding the convoy — a tail pass walks 147 stranded candidates + // sequentially, each taking the company-wide issue-graph advisory lock, + // against a 30 s tick, so two overlapping passes contend with EACH OTHER and + // starve `POSTGRES_POOL_MAX=10`. It had no regression guard, which is the + // "test passes while missing the real failure mode" shape: every other test + // in this file is satisfied by an unlatched implementation. + // + // Also pins the deliberate SPLIT. The dispatch chain above the tail is + // unlatched on purpose (see the rationale at index.ts:1675), so a guard that + // only proved "the tail is single-flighted" would be equally satisfied by + // latching the whole tick — which reproduces the symptom the latch is + // deployed against. + it("single-flights the recovery tail across ticks while leaving dispatch unlatched", async () => { + loadConfigMock.mockReturnValue(buildTestConfig({ + heartbeatSchedulerEnabled: true, + heartbeatSchedulerIntervalMs: 30000, + })); + let intervalCallback: (() => void) | null = null; + const setIntervalSpy = vi + .spyOn(globalThis, "setInterval") + .mockImplementation(((callback: () => void) => { + intervalCallback = callback; + return 1 as unknown as ReturnType; + }) as typeof setInterval); + + const idleReconcile = { + assignmentDispatched: 0, + dispatchRequeued: 0, + continuationRequeued: 0, + successfulRunHandoffEscalated: 0, + escalated: 0, + skipped: 0, + issueIds: [], + }; + // The tail's release is observable only by a LATER tick running it again, + // and the chain's trailing pass is a real (unmocked) import, so there is no + // mock to await for "the `.finally` has run". Tick until it does; a latch + // that never releases simply never satisfies this and times out. + const tickUntilTailRuns = async (times: number) => + vi.waitFor(() => { + intervalCallback?.(); + expect(heartbeatServiceMock.reconcileStrandedAssignedIssues).toHaveBeenCalledTimes(times); + }); + + let releaseTail: (() => void) | null = null; + try { + await startServer(); + // Both also run once from the startup recovery sequence. + heartbeatServiceMock.reconcileStrandedAssignedIssues.mockClear(); + heartbeatServiceMock.resumeQueuedRuns.mockClear(); + + // Park the tail so it is still in flight when the next tick fires. + heartbeatServiceMock.reconcileStrandedAssignedIssues.mockImplementationOnce( + () => new Promise((resolve) => { + releaseTail = () => resolve(idleReconcile); + }), + ); + + intervalCallback?.(); + await vi.waitFor(() => expect(releaseTail).not.toBeNull()); + await vi.waitFor(() => expect(heartbeatServiceMock.resumeQueuedRuns).toHaveBeenCalledTimes(1)); + + // (a) Second tick with the tail still parked must NOT start a second + // pass. This is the assertion the latch exists to make true, and the one + // that fails when the `if (!heartbeatRecoveryChainInFlight)` guard is + // reverted. + intervalCallback?.(); + // (c) ...while the unlatched dispatch chain DOES run again. Awaiting it + // also gives a would-be second tail pass more than enough turns to start, + // so (a) below is not just observing an unresolved microtask. + await vi.waitFor(() => expect(heartbeatServiceMock.resumeQueuedRuns).toHaveBeenCalledTimes(2)); + expect(heartbeatServiceMock.reconcileStrandedAssignedIssues).toHaveBeenCalledTimes(1); + + // (b1) `.finally` releases the latch when the tail RESOLVES. + releaseTail?.(); + releaseTail = null; + await tickUntilTailRuns(2); + + // (b2) ...and when it REJECTS. The chain's terminal `.catch` swallows the + // error, so a latch released on the happy path only would leave recovery + // wedged estate-wide until restart, with no output at all. + heartbeatServiceMock.reconcileStrandedAssignedIssues.mockRejectedValueOnce( + new Error("recovery pass blew up"), + ); + await tickUntilTailRuns(3); + await tickUntilTailRuns(4); + } finally { + releaseTail?.(); + setIntervalSpy.mockRestore(); + } + }); + it("refuses authenticated public startup without an external database URL", async () => { loadConfigMock.mockReturnValue(buildTestConfig({ deploymentExposure: "public", diff --git a/server/src/index.ts b/server/src/index.ts index a90ae9898933..775bf5515d29 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1674,9 +1674,31 @@ export async function startServer(): Promise { // // Deliberately NOT under `heartbeatRecoveryChainInFlight`. These four // passes are the dispatch path — `resumeQueuedRuns` is what actually - // starts a queued run — and they do not take - // `lockIssueParentMutationCompany`, which is the contention the latch - // exists to remove. Gating them on the latched tail would couple + // starts a queued run — and they do not iterate the stranded + // candidate set under `lockIssueParentMutationCompany`, which is the + // contention the latch exists to remove. + // + // They are NOT lock-free, and an earlier wording of this comment + // claimed they were (BLO-34471). `reapOrphanedRuns` -> + // `releaseIssueExecutionAndPromote` takes that lock whenever a + // promotion comes back `blocked`, via + // `recovery.escalateStrandedAssignedIssue` / + // `escalateStrandedRecoveryIssueInPlace`; `startNextQueuedRunForAgent` + // reaches the same helper on the cancel paths. What makes them safe + // to leave unlatched is the BOUND, not the absence: that exposure is + // one escalation per reaped or cancelled run, against the 147 + // candidates a single tail pass walks sequentially (measured + // 2026-09-16). It cannot build the convoy the latch removes. + // + // The split does admit one race: `reapOrphanedRuns` can finalize a + // run after an in-flight tail pass has already sampled its candidate + // set, so a newly-eligible issue waits for the next tick. That is + // bounded latency, not a correctness defect — every pass in the tail + // is idempotent and repeating — and the pre-latch code already let + // tick N's tail overlap tick N+1's dispatch, so it is not a new + // concurrency class. + // + // Gating them on the latched tail would couple // dispatch to that tail's slowest pass: it ends in // `reconcileContendedPrReviewerWakes`, which crosses the // plugin-worker RPC bridge that logged ~976 x 30 s timeouts in the diff --git a/server/src/services/instance-settings.ts b/server/src/services/instance-settings.ts index 7d4b4028ba88..b07134e68140 100644 --- a/server/src/services/instance-settings.ts +++ b/server/src/services/instance-settings.ts @@ -1,5 +1,9 @@ import type { Db } from "@paperclipai/db"; import { companies, instanceSettings } from "@paperclipai/db"; + +// Same shape as `agent-invokability.ts` / `recovery/service.ts`: a caller's +// open transaction handle, which the db package does not export as a name. +type DbTransaction = Parameters[0]>[0]; import { DEFAULT_FEEDBACK_DATA_SHARING_PREFERENCE, DEFAULT_BACKUP_RETENTION, @@ -308,7 +312,7 @@ function toInstanceSettings(row: typeof instanceSettings.$inferSelect): Instance * equivalent to bootstrapping — minus the write. Writers still go through * `instanceSettingsService`, which keeps creating the row. */ -export async function readInstanceSettingsOn(dbOrTx: Db) { +export async function readInstanceSettingsOn(dbOrTx: Db | DbTransaction) { const row = await dbOrTx .select() .from(instanceSettings) diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 8eacc9b19017..1fbcf34da45e 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5685,7 +5685,7 @@ export function issueService(db: Db) { // bootstraps the singleton row, which on a caller's tx is a row lock taken // BEFORE the company graph lock and held to commit — a deadlock edge. These // are pure reads, so they take the read-only view on either handle. - const instanceSettingsOn = (dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db); + const instanceSettingsOn = readInstanceSettingsOn; const treeControlSvc = issueTreeControlService(db); async function lockIssueBlockerRelations( From 0947c9a8524377d67f45545c714080463644a9b8 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sat, 19 Sep 2026 03:36:50 +0000 Subject: [PATCH 2/2] fix(issues): restore the lazy instanceSettingsOn dereference (BLO-34471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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[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 --- .../src/__tests__/server-startup-feedback-export.test.ts | 5 +++++ server/src/services/instance-settings.ts | 8 ++++---- server/src/services/issues.ts | 7 ++++++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index 53bb3b75d530..b233ca7a370b 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -1055,6 +1055,11 @@ describe("startServer feedback export wiring", () => { // and the chain's trailing pass is a real (unmocked) import, so there is no // mock to await for "the `.finally` has run". Tick until it does; a latch // that never releases simply never satisfies this and times out. + // Ticking inside the predicate means the tick count is however many times + // `waitFor` happened to retry, so every call count downstream of the first + // `tickUntilTailRuns` is nondeterministic: assert on the tail's own count + // (which this waits for) and never add a `toHaveBeenCalledTimes` on the + // unlatched passes after it. const tickUntilTailRuns = async (times: number) => vi.waitFor(() => { intervalCallback?.(); diff --git a/server/src/services/instance-settings.ts b/server/src/services/instance-settings.ts index b07134e68140..d390e2fc1670 100644 --- a/server/src/services/instance-settings.ts +++ b/server/src/services/instance-settings.ts @@ -1,9 +1,5 @@ import type { Db } from "@paperclipai/db"; import { companies, instanceSettings } from "@paperclipai/db"; - -// Same shape as `agent-invokability.ts` / `recovery/service.ts`: a caller's -// open transaction handle, which the db package does not export as a name. -type DbTransaction = Parameters[0]>[0]; import { DEFAULT_FEEDBACK_DATA_SHARING_PREFERENCE, DEFAULT_BACKUP_RETENTION, @@ -19,6 +15,10 @@ import { } from "@paperclipai/shared"; import { eq } from "drizzle-orm"; +// Same shape as `agent-invokability.ts` / `recovery/service.ts`: a caller's +// open transaction handle, which the db package does not export as a name. +type DbTransaction = Parameters[0]>[0]; + const DEFAULT_SINGLETON_KEY = "default"; const instanceGeneralSettingsStorageSchema = instanceGeneralSettingsSchema.strip(); const instanceExperimentalSettingsStorageSchema = instanceExperimentalSettingsSchema.strip(); diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 1fbcf34da45e..a35bb976a90a 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5685,7 +5685,12 @@ export function issueService(db: Db) { // bootstraps the singleton row, which on a caller's tx is a row lock taken // BEFORE the company graph lock and held to commit — a deadlock edge. These // are pure reads, so they take the read-only view on either handle. - const instanceSettingsOn = readInstanceSettingsOn; + // Wrapper, not a bare alias: an alias dereferences the module binding when + // `issueService(db)` is constructed, so any test that partially mocks + // `instance-settings.js` fails just by building the service. Reading it + // inside the arrow keeps the dereference at call time. + const instanceSettingsOn = (dbOrTx: Parameters[0]) => + readInstanceSettingsOn(dbOrTx); const treeControlSvc = issueTreeControlService(db); async function lockIssueBlockerRelations(