diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index af6665165ad3..b233ca7a370b 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -1015,6 +1015,105 @@ 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. + // 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?.(); + 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..d390e2fc1670 100644 --- a/server/src/services/instance-settings.ts +++ b/server/src/services/instance-settings.ts @@ -15,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(); @@ -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..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 = (dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db); + // 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(