From 86afc2048bf60033f10000634579471e2b89e9e4 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 4 Nov 2026 16:29:25 -0600 Subject: [PATCH] fix(native): admit follow-ups after an owner finished in this process (#19071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native Runner per-turn sessions in a remote sandbox keep one conversation across task turns. > - Before a follow-up starts a new Runner, it must prove that the previous owner's controller stopped. #18846 added a retirement path for this proof. > - The native controller runs inside the webserver process. Thus the owner's recorded controller PID is the PID of the webserver. > - When the same server process serves the follow-up, that PID is alive and has the same start time. The proof reads this as "controller still alive" and refuses. The follow-up then fails with `runner_state_identity_mismatch: prior_owner_terminal_state_indeterminate`. This continues until a deploy replaces the process. > - This pull request uses the controller's per-process boot identity to recognize that this process recorded the owner. The process then uses its own session registry to prove that the owner's controller stopped. > - The benefit is that follow-ups on a long-lived server no longer fail before provider work. All other ownership fences stay as they are. ## Linked Issues or Issue Description Refs #18846 (retirement of recovered per-turn owners). Refs #19061 (names the refused retirement proof). The two changes are compatible. Whichever merges second needs a small rebase in `retireFinishedPerTurnRunnerdOwner`. **What happened?** On a test deployment, 17 runs over 14 tasks failed before provider work with `runner_state_identity_mismatch: prior_owner_terminal_state_indeterminate`. Each run had a per-turn lifecycle in a Daytona sandbox and got a new sandbox. In each inspected case, the previous run on the task succeeded and committed its result. Its run profile had no `nativeControllerRecovery` pointer. Without that pointer, `evaluateNativeControllerTakeover` is not lease-fenced, so it uses process liveness. The recorded `controllerPid` is `process.pid` of the webserver. When the follow-up is served in the same process, `process.kill(pid, 0)` succeeds and the start time matches. The result is `controller_still_alive`, and retirement refuses. **Expected behavior** A committed owner that this server process finished, released, and no longer runs is proven stopped. The follow-up then starts a new Runner, as #18846 intended. **Steps to reproduce** 1. Seed a per-turn sandbox owner that committed its result and released its lease. Record this process's controller identity (`currentNativeControllerIdentity()`) as the owner. 2. Keep the owner's file-backed journal in the local session root, and remove the database recovery pointer. 3. Admit a follow-up in a new sandbox. 4. Before this change, admission fails with `prior_owner_terminal_state_indeterminate`. **Paperclip version or commit** Observed on `e107a75755`. The same code is on `master`. ## What Changed - `native-restart-recovery.ts`: add `isCurrentNativeControllerProcess(owner)`. It is true only when the owner's `controllerBootId` is this process's per-start boot UUID and `controllerPid === process.pid`. A recycled PID or another container never matches. - `retireFinishedPerTurnRunnerdOwner`: when the owner was finished in this process and no `activeNativeSessions` or `nativeSessionStartups` entry exists for its run, treat its controller as stopped. Otherwise, use `evaluateNativeControllerTakeover` as before. The same in-process check runs again just before the archive. - All other retirement proofs are unchanged: committed result, released lease, no recovery state, settled journal, answered tool inputs, quarantine copies, and new or replacement sandbox. - `native-recovered-owner-followup.test.ts`: the seed helper accepts the owner's controller identity. New cases cover same-process admission (with and without a recovery pointer), an exited legacy controller, another live controller, and the same PID with a different boot identity. - `doc/execution-semantics.md`: describe the same-process stopped-controller proof. ## Verification - **Failure before the change:** with only the new tests applied, `cd server && npx vitest run src/services/native-runtime/native-recovered-owner-followup.test.ts` fails 1 case: "admits a follow-up in the process that finished the owner: local journal only". It expected admission and received `runner_state_identity_mismatch: prior_owner_terminal_state_indeterminate`, the exact error from the deployment. The safety cases already passed on the old code. - **After the change:** the same file passes 27 of 27. - Adjacent suites: `native-recovered-owner-followup`, `remote-warm-session-recovery`, `native-runner-restart-recovery.integration`, `native-restart-recovery`, `native-session-resume`: 210 passed, 20 skipped. - `native-session-executor.test.ts`: 1128 passed. - `tsc --noEmit -p server/tsconfig.json`: no errors. - **Not verified live:** this change has not run on a deployment. Run data came from read-only API calls only. No recovery was done on the deployment. ## Risks - **Second executor.** The new proof applies only when this exact process recorded the owner, and this process has no session or startup for that run. The database proofs still require a committed result and a released lease. Another live controller, or the same PID with a different boot identity, still holds the owner. Tests cover both cases. - **Scope.** The change applies only to the per-turn retirement path. The warm remote recovery path (`inspectRemoteWarmSessionRecovery`) is not changed. Warm handoff in the same process uses the in-memory idle owner instead. - **Hypothesis limit.** The deployment API does not expose finalization rows. Thus I inferred that the same process served each follow-up from the evidence: no recovery pointer, a local journal present, and no other failing proof. The local reproduction gives the exact error. #19061 can confirm the refused fence on the deployment. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic. Agentic coding in Claude Code with tool use (shell, browser for read-only staging API reads, code execution). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- doc/execution-semantics.md | 5 +- .../native-recovered-owner-followup.test.ts | 65 +++++++++++++++++-- .../native-runtime/native-restart-recovery.ts | 7 ++ .../native-runtime/native-session-executor.ts | 11 +++- 4 files changed, 80 insertions(+), 8 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index c69c928eb65..fccd60629d4 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1863,7 +1863,10 @@ All of these must be true: - The prior run is terminal and its coordinator committed a result. - The coordinator has no lease owner and no recovery state. -- The prior controller is proven stopped. +- The prior controller is proven stopped. The controller runs inside the + webserver, so when this same server process finished the owner, its live + PID proves nothing. The owner's exact controller boot identity, with no + in-process session or startup left for that run, proves it stopped instead. - The journal has a delivered terminal, only completed commands, and an answer for each tool input. - No local Runner lifecycle evidence exists for the old owner. diff --git a/server/src/services/native-runtime/native-recovered-owner-followup.test.ts b/server/src/services/native-runtime/native-recovered-owner-followup.test.ts index 071fdf49e8d..0e5ff1b62e9 100644 --- a/server/src/services/native-runtime/native-recovered-owner-followup.test.ts +++ b/server/src/services/native-runtime/native-recovered-owner-followup.test.ts @@ -1,3 +1,4 @@ +import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; import { existsSync } from "node:fs"; import { cp, mkdir, mkdtemp, readFile, readdir, rename, rm, writeFile } from "node:fs/promises"; @@ -16,6 +17,8 @@ import { prepareNativeHeartbeatRun } from "./prepare-native-run.js"; import { buildNativeExecutionInput } from "./native-execution-input.js"; import { nativeRunnerCheckpointWriter } from "./native-runner-checkpoint.js"; import { createNativeControllerRecovery } from "./native-controller-recovery.js"; +import { currentNativeControllerIdentity } from "./native-restart-recovery.js"; +import { readProcessStartedAt } from "../hot-restart.js"; import { buildNativeExecutionWithCheckpoint, findNativeSessionResumeRun, nativeToolContractFingerprintForTarget, } from "./native-session-resume.js"; @@ -83,7 +86,8 @@ describe.skipIf(!recoveryPostgresSupport.supported)("follow-up after a recovered * later committed its result and released the controller lease, but no * suspended Runner receipt or failover backup ever reached the controller. * Only the database controller checkpoint and recovery pointer survive. */ - async function seedRecoveredPriorRun(control: "settled" | "pending-command" | "unanswered-tool" = "settled") { + async function seedRecoveredPriorRun(control: "settled" | "pending-command" | "unanswered-tool" = "settled", + options: { controller?: { bootId: string; pid: number; processStartedAt: Date } } = {}) { const issueId = randomUUID(), workspaceId = randomUUID(), runId = randomUUID(); await db.insert(issues).values({ id: issueId, companyId, title: "Continue after recovery", assigneeAgentId: agentId, status: "in_progress" }); const [row] = await db.insert(heartbeatRuns).values({ id: runId, companyId, agentId, status: "running", runtimeMode: "native", @@ -104,10 +108,12 @@ describe.skipIf(!recoveryPostgresSupport.supported)("follow-up after a recovered const execution = executionFor(runId); const identity = { runId, normalizedSessionId: native.normalizedSessionId, runnerInstanceId: native.runnerInstanceId, environmentLeaseId: native.environmentLeaseId, turnId: native.turnId, itemId: native.itemId }; - const owner = { companyId, issueId, runId, authorityRunId: runId, bootId: randomUUID(), generation: 1, leaseOwner: randomUUID() }; + const priorController = options.controller ?? { bootId: randomUUID(), pid: 2_147_483_000, + processStartedAt: new Date(Date.now() - 60_000) }; + const owner = { companyId, issueId, runId, authorityRunId: runId, bootId: priorController.bootId, generation: 1, leaseOwner: randomUUID() }; await db.insert(nativeRunFinalizations).values({ companyId, issueId, runId, phase: "observed", - controllerBootId: owner.bootId, controllerGeneration: 1, controllerPid: 2_147_483_000, - controllerProcessStartedAt: new Date(Date.now() - 60_000), leaseOwner: owner.leaseOwner, + controllerBootId: owner.bootId, controllerGeneration: 1, controllerPid: priorController.pid, + controllerProcessStartedAt: priorController.processStartedAt, leaseOwner: owner.leaseOwner, leaseExpiresAt: new Date(Date.now() + 60_000) }); const scratch = await mkdtemp(join(tmpdir(), "native-recovered-owner-controller-")); const controller = new DurablePrpControlPlane({ stateDirectory: scratch, identity, @@ -214,6 +220,57 @@ describe.skipIf(!recoveryPostgresSupport.supported)("follow-up after a recovered expect(followUp.logs.join("")).toContain(`Run ${prior.runId} finished without a saved runner checkpoint`); }, 60_000); + async function keepLocalRootWithoutPointer(prior: Awaited>, + followUp: Awaited>) { + await db.update(nativeRunFinalizations).set({ phase: "observed" }).where(eq(nativeRunFinalizations.runId, prior.runId)); + expect(await followUp.start()).toBe(STRANDED); + const [copy] = await quarantined(); + await rename(join(stateDirectory, "quarantine", copy!), join(stateDirectory, copy!.split(".")[0]!)); + await rm(join(stateDirectory, "quarantine"), { recursive: true, force: true }); + const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, prior.runId)); + const { nativeControllerRecovery: _pointer, ...profile } = run.runnerProfileJson!; + await db.update(heartbeatRuns).set({ runnerProfileJson: profile }).where(eq(heartbeatRuns.id, prior.runId)); + await db.update(nativeRunFinalizations).set({ phase: "committed" }).where(eq(nativeRunFinalizations.runId, prior.runId)); + } + + it.each(["recovery pointer", "local journal only"] as const)( + "admits a follow-up in the process that finished the owner: %s", async evidence => { + const prior = await seedRecoveredPriorRun("settled", { controller: await currentNativeControllerIdentity() }); + const followUp = await admitFollowUp(prior); + if (evidence === "local journal only") await keepLocalRootWithoutPointer(prior, followUp); + expect(await followUp.start()).toBe(ADMITTED); + expect(await quarantined()).toEqual([]); + expect(await archivedOwners(await sessionRoot())).toEqual([prior.runId]); + }, 60_000); + + it("admits a local journal whose controller process has exited", async () => { + const prior = await seedRecoveredPriorRun(); + const followUp = await admitFollowUp(prior); + await keepLocalRootWithoutPointer(prior, followUp); + expect(await followUp.start()).toBe(ADMITTED); + }, 60_000); + + it.each(["another live controller", "an earlier controller in this process"] as const)( + "keeps a local journal held while %s may still own it", async scenario => { + const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 60_000)"], { stdio: "ignore" }); + try { + await new Promise(resolve => child.once("spawn", resolve)); + const self = await currentNativeControllerIdentity(); + const startedAt = await readProcessStartedAt(child.pid!); + expect(startedAt).toBeTruthy(); + const controller = scenario === "another live controller" + ? { bootId: randomUUID(), pid: child.pid!, processStartedAt: new Date(startedAt!) } + : { ...self, bootId: randomUUID() }; + const prior = await seedRecoveredPriorRun("settled", { controller }); + const followUp = await admitFollowUp(prior); + await keepLocalRootWithoutPointer(prior, followUp); + expect(await followUp.start()).toBe(STRANDED); + expect(followUp.logs.join("")).not.toContain("finished without a saved runner checkpoint"); + } finally { + child.kill("SIGKILL"); + } + }, 60_000); + /** The state left by a follow-up admitted before retirement existed. Its * admission quarantined the rebuilt projection and failed before provider * work; it keeps only the provider checkpoint inherited from the owner. */ diff --git a/server/src/services/native-runtime/native-restart-recovery.ts b/server/src/services/native-runtime/native-restart-recovery.ts index d8359ffd31f..79ca54fc5a8 100644 --- a/server/src/services/native-runtime/native-restart-recovery.ts +++ b/server/src/services/native-runtime/native-restart-recovery.ts @@ -179,6 +179,13 @@ export async function currentNativeControllerIdentity(): Promise isCurrentNativeControllerProcess(prior!.owner) && + !activeNativeSessions.has(identity.runId) && !nativeSessionStartups.has(identity.runId); const profile = record(prior.run.runnerProfileJson); - const stopped = await evaluateNativeControllerTakeover({ owner: prior.owner, now: new Date(), - requireStoppedController: true, + const stopped = finishedHere() ? { allowed: true } : await evaluateNativeControllerTakeover({ owner: prior.owner, + now: new Date(), requireStoppedController: true, remoteLeaseFenced: readNativeControllerRecovery(profile.nativeControllerRecovery, profile.sessionCheckpoint) !== null }); if (!stopped.allowed) return false; + const provenFinishedHere = finishedHere(); // Earlier rejected admissions can quarantine the same projection. Each copy // must pass the same proof: a rebuilt root never carries the local Runner // evidence that held an earlier attempt, so that evidence stays decisive. @@ -5266,7 +5270,8 @@ async function retireFinishedPerTurnRunnerdOwner(input: { } } // Database reads can await. Retire only the exact evidence proven above. - if (readSettledRunnerdProjection(input.root, identity) !== controlBytes) return false; + if (readSettledRunnerdProjection(input.root, identity) !== controlBytes || + (provenFinishedHere && !finishedHere())) return false; const archivedAt = Date.now(); const archiveRoot = resolve(input.root, "continuity-breaks", `${archivedAt}-${randomUUID()}`); mkdirSync(archiveRoot, { recursive: true, mode: 0o700 });