fix(heartbeat): correct the overstated latch-scope comment and cover heartbeatRecoveryChainInFlight (BLO-34471)

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<typeof setInterval>;
+      }) 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",

server/src/index.ts

@@ -1674,9 +1674,31 @@ export async function startServer(): Promise<StartedServer> {
           //
           // 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

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<Parameters<Db["transaction"]>[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)

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<typeof readInstanceSettingsOn>[0]) =>
+    readInstanceSettingsOn(dbOrTx);
   const treeControlSvc = issueTreeControlService(db);
 
   async function lockIssueBlockerRelations(