mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix(native): admit follow-ups after an owner finished in this process (#19071)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
c19c4348c5
commit
86afc2048b
4 files changed
+80
-8
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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<ReturnType<typeof seedRecoveredPriorRun>>,
|
||||
followUp: Awaited<ReturnType<typeof admitFollowUp>>) {
|
||||
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. */
|
||||
|
||||
@@ -179,6 +179,13 @@ export async function currentNativeControllerIdentity(): Promise<NativeControlle
|
||||
};
|
||||
}
|
||||
|
||||
export function isCurrentNativeControllerProcess(owner: {
|
||||
controllerBootId: string | null;
|
||||
controllerPid: number | null;
|
||||
}): boolean {
|
||||
return owner.controllerBootId === controllerBootId && owner.controllerPid === process.pid;
|
||||
}
|
||||
|
||||
function sameProcessStart(left: Date | null, right: Date | null): boolean {
|
||||
return left !== null && right !== null && left.getTime() === right.getTime();
|
||||
}
|
||||
|
||||
@@ -280,6 +280,7 @@ import {
|
||||
import {
|
||||
currentNativeControllerIdentity,
|
||||
evaluateNativeControllerTakeover,
|
||||
isCurrentNativeControllerProcess,
|
||||
nextNativeProviderAttempt,
|
||||
type NativeControllerIdentity,
|
||||
type NativeRestartRecoveryClaim,
|
||||
@@ -5249,11 +5250,14 @@ async function retireFinishedPerTurnRunnerdOwner(input: {
|
||||
}
|
||||
// A durable controller is fenced by its released database lease; a legacy
|
||||
// controller still needs process evidence, exactly as for warm recovery.
|
||||
const finishedHere = () => 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 });
|
||||
|
||||
Reference in new issue
Block a user