From 97719a8ac54a6aceed2be0b2b7db7aa2d3381c74 Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 26 Oct 2026 03:34:02 -0500 Subject: [PATCH] fix(server): fence reassessment against changed execution authority Co-Authored-By: Paperclip --- ...2026-10-24-recurring-server-sleep-stack.md | 10 ++++ ...-native-status-reassessment-sleep-audit.md | 55 +++++++++++++++++++ .../native-status-arbiter-corpus.test.ts | 21 ++++++- .../native-status-reassessment-work.test.ts | 12 +++- .../native-status-reassessment.ts | 2 +- .../status-decision-committer.ts | 5 ++ 6 files changed, 101 insertions(+), 4 deletions(-) diff --git a/doc/plans/2026-10-24-recurring-server-sleep-stack.md b/doc/plans/2026-10-24-recurring-server-sleep-stack.md index b04b5ca664a..5bc775c6621 100644 --- a/doc/plans/2026-10-24-recurring-server-sleep-stack.md +++ b/doc/plans/2026-10-24-recurring-server-sleep-stack.md @@ -2490,3 +2490,13 @@ embedded-PostgreSQL failures and the external-database test repairs are preserve in the owner audit. Approval/interaction/run evidence and workspace producers, concurrent authority, backlog cost, startup wiring and periodic removal remain. Master remains 1fbbef45a8; older stack gates and the full original scope stay open. + +Slice 50 authority checkpoint: two real commit-boundary regressions reproduced +stale completion after the source run failed or another run took task execution. +The scheduled retired-review path now rechecks both facts under existing locks. +All 115 status-corpus cases pass. A retained-history startup assertion failed in +the combined run; after waiting for bounded startup batches while enforcing the +saved retry deadline, all 28 lifecycle cases and server types pass. The owner +audit preserves failures and handoff/contract producer findings. Remaining +business-fact races, producer coverage, cost, wiring, periodic removal and PR +acceptance are still open. Nothing merged; the full stack goal is incomplete. diff --git a/doc/plans/2026-10-26-native-status-reassessment-sleep-audit.md b/doc/plans/2026-10-26-native-status-reassessment-sleep-audit.md index 356ff0b6603..3cea63b4317 100644 --- a/doc/plans/2026-10-26-native-status-reassessment-sleep-audit.md +++ b/doc/plans/2026-10-26-native-status-reassessment-sleep-audit.md @@ -456,3 +456,58 @@ Evidence: `/tmp/sleep50-events-before.log` and `.status` (four failures), this checkpoint are terminal. Case-handoff creation/cancellation, remaining contract/ownership/authority producers and the full acceptance tasks listed above remain outstanding; the old periodic reassessment path is still enabled. + +## Scheduled retired-review authority checkpoint + +Two commit-boundary regressions reproduce unsafe completion: after reassessment +reads a succeeded run, a competing writer can change its outcome to failed or +bind the task to a different run without changing the task's status version. +Both previously committed a `done` decision. The scheduled source contract now +identifies retired automatic-review reassessment, and the existing status +committer rechecks run success and task execution ownership under its existing +coordinator/task/run locks. It adds no outer transaction, lock order, provider +call, or database query. Explicit legacy reconciliation remains unchanged. + +The two regressions assert that the competing state survives, no new decision +commits, and reassessment succeeds once the original authority is restored. +All 115 status-corpus cases pass. The combined corpus/lifecycle run initially +finished with 142 passes and one failed lifecycle assertion: retained test +history needs multiple startup batches, so the first registered wake can be the +next batch rather than the target's future retry. The retry test now waits for +startup batches to settle while asserting that the target stays in review, +then verifies the exact saved deadline before and after restart. All 28 lifecycle +cases pass against that same retained-history database; final server types pass. +This is 143 distinct passing cases across runs, not a green initial combined run. + +Evidence: `/tmp/sleep50-authority-before.log` and `.status` (two failures), +`/tmp/sleep50-authority-after.log` and `.status` (115 corpus passes and 27/28 +lifecycle passes), `/tmp/sleep50-authority-lifecycle.log` and `.status` (28 passes), +and `/tmp/sleep50-authority-final-types.log` and `.status` (pass). All launched +handles reached terminal status. No remote acceptance or host sleep is claimed. + +Producer audit notes for the next checkpoint: + +- Handoff evidence is accepted for every persisted non-cancelled state. Delivery, + completion, leases and retries do not change that classification. Agent-created + handoffs require the source run to be running and own the task; their generated + UUID is returned after creation. Board requests can use the current execution + binding. Do not add wake traffic to every handoff lease renewal. +- Task mutation cancellations are in the issue transaction, which also signals + reassessment. Direct handoff cancellation, delivery cancellation, and run-control + cancellation still require their own transaction/activation audit; no complete + handoff producer-coverage claim is made here. +- Superseded external-review withdrawal is called by the status committer inside + its decision transaction, which already signals native finalization and chat + milestones. OAuth authorization-start separately reopens/creates a pending + confirmation; its interaction writes remain to be audited. +- The evidence classifier checks target-agent existence, not invokable status. + Do not add status-change fanout based only on the name of that check. +- Completion-contract creation is serialized with the per-company/task advisory + lock in `ensureNativeCompletionContract`. Checking the latest revision before + the commit transaction is still insufficient. Audit its callers' lock order + before extending the scheduled commit guard. + +Latest-contract, workspace-barrier, governance, dependency and other business-fact +races remain open, along with remaining producers, query cost/backlog proof, +startup wiring, periodic removal, publication and PR gates. The full original +stack scope, older unresolved gates and master reconciliation remain outstanding. diff --git a/server/src/__tests__/native-status-arbiter-corpus.test.ts b/server/src/__tests__/native-status-arbiter-corpus.test.ts index 108c8964c1b..9de419aee3c 100644 --- a/server/src/__tests__/native-status-arbiter-corpus.test.ts +++ b/server/src/__tests__/native-status-arbiter-corpus.test.ts @@ -2453,7 +2453,7 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => { return seeded; } - it.each(["future", "claim", "unknown claim", "assessment", "result", "phase", "decision", "effects", "ownership"] as const)( + it.each(["future", "claim", "unknown claim", "assessment", "result", "phase", "decision", "effects", "ownership", "run outcome", "task execution"] as const)( "guards scheduled reassessment against a changed %s at the commit boundary", async changed => { const seeded = await seedAutomaticReview(); await dismissAutomaticCompletionReviews(db, seeded.issueId); @@ -2476,6 +2476,12 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => { : changed === "phase" ? { phase: "terminal_failure" } : changed === "decision" ? { decisionId: null } : null; const [originalRun] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, seeded.runId)); + const [originalIssue] = await db.select().from(issues).where(eq(issues.id, seeded.issueId)); + const replacementRunId = randomUUID(); + if (changed === "task execution") await db.insert(heartbeatRuns).values({ + id: replacementRunId, companyId, agentId, status: "running", runtimeMode: "native", + nativeIssueId: seeded.issueId, contextSnapshot: { issueId: seeded.issueId }, + }); const originalEffects = await db.select().from(statusDecisionEffects).where(eq(statusDecisionEffects.decisionId, selected.decisionId!)); expect(originalEffects.length).toBeGreaterThan(0); const transaction = db.transaction.bind(db); @@ -2486,6 +2492,10 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => { if (changed === "ownership") await db.update(heartbeatRuns).set({ status: "running", runtimeMode: "native", errorCode: "native_execution_ownership_unverified", nativePhase: "terminal_failure" }) .where(eq(heartbeatRuns.id, seeded.runId)); + if (changed === "run outcome") await db.update(heartbeatRuns).set({ status: "failed" }) + .where(eq(heartbeatRuns.id, seeded.runId)); + if (changed === "task execution") await db.update(issues).set({ executionRunId: replacementRunId }) + .where(eq(issues.id, seeded.issueId)); return transaction(callback, options); }) as typeof db.transaction); try { @@ -2506,6 +2516,15 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => { await db.update(heartbeatRuns).set({ status: originalRun.status, runtimeMode: originalRun.runtimeMode, errorCode: originalRun.errorCode, nativePhase: originalRun.nativePhase }).where(eq(heartbeatRuns.id, seeded.runId)); } + if (changed === "run outcome") { + expect((await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, seeded.runId)))[0].status).toBe("failed"); + await db.update(heartbeatRuns).set({ status: originalRun.status }).where(eq(heartbeatRuns.id, seeded.runId)); + } + if (changed === "task execution") { + expect((await issueService(db).getById(seeded.issueId))!.executionRunId).toBe(replacementRunId); + await db.update(issues).set({ executionRunId: originalIssue.executionRunId }).where(eq(issues.id, seeded.issueId)); + await db.delete(heartbeatRuns).where(eq(heartbeatRuns.id, replacementRunId)); + } await db.update(nativeRunFinalizations).set({ phase: selected.coordinatorPhase, resultId: seeded.resultId, assessmentId: selected.assessmentId, decisionId: selected.decisionId, nextAttemptAt: null, leaseOwner: null, leaseExpiresAt: null }).where(eq(nativeRunFinalizations.runId, seeded.runId)); diff --git a/server/src/__tests__/native-status-reassessment-work.test.ts b/server/src/__tests__/native-status-reassessment-work.test.ts index e869ab3efb8..fe3385a7eaf 100644 --- a/server/src/__tests__/native-status-reassessment-work.test.ts +++ b/server/src/__tests__/native-status-reassessment-work.test.ts @@ -416,13 +416,21 @@ describe("native reassessment worker lifecycle", () => { await dismissAutomaticCompletionReviews(db, seeded.issueId); const future = new Date(Date.now() + 60_000); await db.update(nativeRunFinalizations).set({ nextAttemptAt: future }).where(eq(nativeRunFinalizations.runId, seeded.runId)); + const waitForSavedDeadline = async () => { + await vi.waitFor(async () => { + await vi.advanceTimersByTimeAsync(1000); + expect(idleWorkSnapshot().active).toBe(0); + expect(coordinator!.nextWakeAt()).toBe(future.getTime()); + expect(await status(seeded.issueId)).toBe("in_review"); + }, { timeout: 15_000 }); + }; await start(); - expect(coordinator!.nextWakeAt()).toBe(future.getTime()); + await waitForSavedDeadline(); await vi.advanceTimersByTimeAsync(20_000); expect(await status(seeded.issueId)).toBe("in_review"); await coordinator!.stop(); coordinator = undefined; await start(); - expect(coordinator!.nextWakeAt()).toBe(future.getTime()); + await waitForSavedDeadline(); await vi.advanceTimersByTimeAsync(Math.max(0, future.getTime() - Date.now())); await flush(); expect(await status(seeded.issueId)).toBe("done"); expect(await db.select().from(statusDecisions).where(eq(statusDecisions.issueId, seeded.issueId))).toHaveLength(2); diff --git a/server/src/services/native-runtime/native-status-reassessment.ts b/server/src/services/native-runtime/native-status-reassessment.ts index eacb2da9588..c83d63de6ac 100644 --- a/server/src/services/native-runtime/native-status-reassessment.ts +++ b/server/src/services/native-runtime/native-status-reassessment.ts @@ -207,7 +207,7 @@ export async function reassessNativeStatusFromSource( decision, requireReassessmentSource: options.onlyWhenDue ? { phase: row.coordinatorPhase, resultId: resultRow.id, assessmentId: assessment.id, - decisionId: row.decisionId, agentId: row.agentId, + decisionId: row.decisionId, agentId: row.agentId, retiredAutomaticReview: !!retiredAutomaticReview, } : undefined, requireNoPendingChildCompletion: decision.statusAction === "done" ? childCompletionRecipient : undefined, supersedesCommittedDecisionId: row.coordinatorPhase === "committed" diff --git a/server/src/services/native-runtime/status-decision-committer.ts b/server/src/services/native-runtime/status-decision-committer.ts index c379e490059..72a2cce10f6 100644 --- a/server/src/services/native-runtime/status-decision-committer.ts +++ b/server/src/services/native-runtime/status-decision-committer.ts @@ -1602,6 +1602,7 @@ export async function commitNativeStatusDecision(input: { requireNoPendingChildCompletion?: NativeChildCompletionRecipient; requireReassessmentSource?: { phase: string; resultId: string; assessmentId: string; decisionId: string | null; agentId: string; + retiredAutomaticReview: boolean; }; requireModelRejectionOwner?: { agentId: string; reviewContext: NativeReviewAssignmentContext | null }; requireProviderFailureOwner?: { agentId: string; reviewContext: NativeReviewAssignmentContext | null }; @@ -1670,6 +1671,10 @@ export async function commitNativeStatusDecision(input: { eq(heartbeatRuns.nativeIssueId, input.issueId), eq(heartbeatRuns.agentId, expected.agentId), )).for("update").limit(1).then(rows => rows[0]); if (!run || run.runtimeMode !== "native" || isNativeRunnerOwnershipHeld(run)) throw new NativeStatusRaceError(); + if (expected.retiredAutomaticReview && (run.status !== "succeeded" || + (issue.executionRunId !== null && issue.executionRunId !== input.runId))) { + throw new NativeStatusRaceError(); + } const pendingEffect = expected.decisionId ? await tx.select({ id: statusDecisionEffects.id }) .from(statusDecisionEffects).where(and( eq(statusDecisionEffects.companyId, input.companyId), eq(statusDecisionEffects.issueId, input.issueId),