fix(recovery): preserve lock handover evidence (BLO-19160)

server/src/__tests__/heartbeat-process-recovery.test.ts

@@ -6317,27 +6317,29 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
     // No reverse `blocks` relation is seeded, so the recovery reachability check
     // (findCycleFormingBlockerIssueIds) will not flag openChildId as cycle-forming - it
     // only sees a cycle at write time, simulating a relation update that lands between
-    // the check and the write. The real write-time check (assertNoBlockingCycles) runs
-    // inside issuesSvc.update's db.transaction(), so the fault is injected on the first
-    // transaction the reconcile pass opens rather than on db.update directly (that
-    // transaction's tx.update/tx.select calls are a separate client, invisible to a
-    // db.update spy).
-    const transactionSpy = vi.spyOn(db, "transaction");
+    // the check and the write. Inject the error at the review-wait helper's actual
+    // blocker-update boundary; mocking `db.transaction` would intercept the outer
+    // recovery transaction introduced for ownership serialization instead of the
+    // relation write that the helper catches.
     let cycleErrorThrown = false;
-    transactionSpy.mockImplementationOnce(async () => {
+    const beforeContinuationReviewBlockerUpdateForTest = vi.fn(async () => {
       cycleErrorThrown = true;
       throw new Error("Blocking relations cannot contain cycles");
     });
 
-    heartbeat = createHeartbeat({ penstockAvailabilityGate: allowPenstockGate });
+    heartbeat = createHeartbeat({
+      penstockAvailabilityGate: allowPenstockGate,
+      skipQueuedRunDispatch: true,
+      beforeContinuationReviewBlockerUpdateForTest,
+    });
     let result: Awaited<ReturnType<typeof heartbeat.reconcileStrandedAssignedIssues>>;
-    try {
-      result = await heartbeat.reconcileStrandedAssignedIssues();
-    } finally {
-      transactionSpy.mockRestore();
-    }
+    result = await heartbeat.reconcileStrandedAssignedIssues();
 
     expect(cycleErrorThrown).toBe(true);
+    expect(beforeContinuationReviewBlockerUpdateForTest).toHaveBeenCalledWith({
+      issueId,
+      blockedByIssueIds: [openChildId],
+    });
     expect(result.waitingOnReviewResolved).toBe(0);
     expect(result.reviewWaitingParked).toBe(1);
     expect(result.escalated).toBe(0);

server/src/__tests__/heartbeat-wake-dispatch-retry.test.ts

@@ -188,6 +188,64 @@ describeEmbeddedPostgres("heartbeat wake dispatch retry (BLO-14395)", () => {
     expect(dispatchFailedRows).toHaveLength(0);
   });
 
+  it("durably skips a recovery wake when issue ownership changes before enqueueWakeup locks it", async () => {
+    const { companyId, agentId } = await seedCompanyAndAgent();
+    const issueId = randomUUID();
+    const expectedLockOwnerState = {
+      executionRunId: null,
+      checkoutRunId: null,
+      assigneeAgentId: agentId,
+    };
+
+    await db.insert(issues).values({
+      id: issueId,
+      companyId,
+      title: "Ownership changes before recovery wake",
+      status: "todo",
+      priority: "high",
+      assigneeAgentId: agentId,
+      checkoutRunId: null,
+      executionRunId: null,
+    });
+
+    const racingHeartbeat = heartbeatService(db, {
+      skipQueuedRunDispatch: true,
+      beforeIssueWakeLockForTest: async ({ issueId: lockingIssueId }) => {
+        expect(lockingIssueId).toBe(issueId);
+        await db
+          .update(issues)
+          .set({ assigneeAgentId: null, updatedAt: new Date() })
+          .where(eq(issues.id, issueId));
+      },
+    });
+
+    const run = await racingHeartbeat.wakeup(agentId, {
+      source: "automation",
+      triggerDetail: "system",
+      reason: "issue_assignment_recovery",
+      payload: { issueId },
+      contextSnapshot: { issueId, taskId: issueId, wakeReason: "issue_assignment_recovery" },
+      expectedLockOwnerState,
+    });
+
+    expect(run).toBeNull();
+    const skippedRows = await db
+      .select({ reason: agentWakeupRequests.reason })
+      .from(agentWakeupRequests)
+      .where(and(
+        eq(agentWakeupRequests.companyId, companyId),
+        eq(agentWakeupRequests.agentId, agentId),
+        eq(agentWakeupRequests.status, "skipped"),
+      ));
+    expect(skippedRows).toEqual([{ reason: "issue_execution_ownership_changed" }]);
+
+    const issueRuns = await db
+      .select({ id: heartbeatRuns.id })
+      .from(heartbeatRuns)
+      .where(and(eq(heartbeatRuns.companyId, companyId), eq(heartbeatRuns.agentId, agentId)));
+    expect(issueRuns).toHaveLength(0);
+  });
+
   it("passes a business-rule HttpError straight through with no retry delay and no durable dispatch_failed record", async () => {
     const { agentId } = await seedCompanyAndAgent({ agentStatus: "paused" });
 

server/src/__tests__/issue-checkout-routine-lock-conflict.test.ts

@@ -251,13 +251,12 @@ describeEmbeddedPostgres("checkout adoption vs open routine-execution lock (PEN-
     expect(victim?.executionRunId).toBe(actorRunId);
   });
 
-  it("records what the inline-refresh path actually leaves behind when it loses the key", async () => {
-    // The other three cases all conflict inside a single transaction. This one
-    // does not, and that is the point: `clearStaleExecutionLock` COMMITS the
-    // reap of the victim's own dead lock, and only then does the separate
-    // refresh write raise 23505. So the victim's row does change even though the
-    // call fails — "not half-applied" is true of the unowned-adoption path and
-    // false here.
+  it("rolls back stale cleanup when the atomic adoption loses the key", async () => {
+    // Stale cleanup and adoption share one ownership transaction now. The
+    // trigger below attempts to let the sibling take the unique key at the
+    // exact point where the victim's dead lock would be cleared. The unique
+    // violation must abort that whole transaction: neither the cleanup nor the
+    // sibling's competing lock may survive the failed adoption.
     //
     // Both rows cannot hold the key at seed time (the partial index forbids it),
     // so the sibling's acquisition is injected with a trigger that fires exactly
@@ -268,11 +267,11 @@ describeEmbeddedPostgres("checkout adoption vs open routine-execution lock (PEN-
     const routineId = randomUUID();
     const fingerprint = "shared-dispatch-fingerprint";
 
-    const deadRunId = await seedRun(companyId, agentId, "failed");
-    await db
-      .update(heartbeatRuns)
-      .set({ finishedAt: new Date() })
-      .where(eq(heartbeatRuns.id, deadRunId));
+    // `queued` is reapable by stale-lock adoption but is not terminal, so the
+    // initial terminal-cleanup prepass leaves it in place. That puts the
+    // trigger inside the atomic stale-cleanup-plus-adoption transaction rather
+    // than in the earlier standalone cleanup transaction.
+    const deadRunId = await seedRun(companyId, agentId, "queued");
 
     // The victim holds the key via its OWN dead run, so it is the row the
     // inline-refresh path reaps.
@@ -349,18 +348,17 @@ describeEmbeddedPostgres("checkout adoption vs open routine-execution lock (PEN-
           details?: { ownerIssueId?: string | null };
         });
 
-      // Still a 409, not the 500 this PR removes — and specifically the
-      // routine-lock 409, so the assertions below cannot be satisfied by some
-      // other conflict path that never reached the inline refresh.
+      // Still a 409, not the 500 this PR removes. The sibling write happened
+      // inside the same transaction and therefore rolled back too, so there is
+      // no committed owner to name.
       expect(error?.status).toBe(409);
-      expect(error?.message).toBe("Routine execution already locked by another open issue");
-      expect(error?.details?.ownerIssueId).toBe(ownerIssueId);
+      expect(error?.message).toBe("Routine execution dispatch lock is contended; retry the request");
+      expect(error?.details?.ownerIssueId).toBeNull();
 
       const victim = await readIssueLockState(victimIssueId);
-      // The committed reap survives the failed refresh. This is the residual
-      // state, asserted rather than assumed: the dead run's lock is gone and the
-      // victim now holds nothing.
-      expect(victim?.executionRunId, "the reap already committed").toBeNull();
+      // The stale lock is still present because its cleanup rolled back with the
+      // failed adoption. A later ordinary cleanup can safely reap it.
+      expect(victim?.executionRunId, "stale cleanup must roll back").toBe(deadRunId);
       expect(victim?.checkoutRunId).toBeNull();
       expect(victim?.status).toBe("in_progress");
 
@@ -372,10 +370,10 @@ describeEmbeddedPostgres("checkout adoption vs open routine-execution lock (PEN-
         .from(heartbeatRuns)
         .where(eq(heartbeatRuns.id, deadRunId))
         .then((rows) => rows[0] ?? null);
-      expect(deadRun?.status).toBe("failed");
+      expect(deadRun?.status).toBe("queued");
 
       const owner = await readIssueLockState(ownerIssueId);
-      expect(owner?.executionRunId, "the sibling holds the key it took").toBe(ownerRunId);
+      expect(owner?.executionRunId, "the competing sibling write must roll back").toBeNull();
     } finally {
       await db.execute(sql`drop trigger if exists pen2395_take_lock_on_release on issues`);
       await db.execute(sql`drop function if exists pen2395_take_lock_on_release()`);