mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 14:19:00 +01:00
refactor(recovery): judge native owner processes with the shared prover (#19997)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Native runs record the server and adapter processes that own them, and recovery must prove an owner gone before it takes over or releases its work > - Phase 2 of the durable ownership plan gave the repository one prover, `proveOwnerGone`. The native runtime still had four private proofs of the same fact > - The private proofs disagreed with the shared rules in edge cases: some judged a PID from another PID space, and one accepted an inconsistent start time as proof of reuse > - This pull request moves those four proofs onto `proveOwnerGone` and deletes the private liveness code > - The benefit is one rule for "the owner is gone" across native recovery, with the stricter shared behaviour where they differed ## Linked Issues or Issue Description Refs #19933, #19950, #19953 (the shared ownership path and earlier Phase 2 kinds). **What is the problem?** Native recovery proved dead owners with four private checks instead of the shared prover. The checks used different rules for PID spaces, reused PIDs and unreadable start times. **What should happen?** Every native owner proof uses `proveOwnerGone`. Only a `gone` verdict releases or takes over. The remote fence and the one-release legacy fallback stay as they are. **Why does it matter?** Recovery decisions about the same process must agree. A dead PID in another PID space must never be used as proof. ## What Changed - `native-restart-recovery.ts` `evaluateNativeControllerTakeover`: the local-process branch now calls `proveOwnerGone`. The remote fenced branch (#19830), the `operator_attested` sentinel and the null-host fallback (`processSpaceUnrecorded` with `legacyHostFallback`, recorded by callers as `legacy_host_unknown_dead_pid_proof`) are kept. Reasons are unchanged, plus a new `controller_pid_start_inconsistent`. Owners built from receipts with no PID space are judged in this PID space: their callers check the recorded host first. - `native-failed-run-owner-release.ts`: `recordedProcessMayBeLive` is deleted. `runProcessMayBeLive` and `controllerMayBeLive` release only on a `gone` verdict. Each use of the legacy fallback is recorded as `legacy_host_unknown_dead_pid_proof` when the release commits. - `native-session-executor.ts`: `verifiedRecoveryProcessIsAlive` is replaced by `recoveryProcessMayBeAlive`, which is false only when the prover proves the runner gone. Signalling a recovered runner requires a proven same process. - `native-local-process-stop.ts` `recordedLocalExecutionStopped`: the bound identity (launch event, host, PID, group, start time) is judged by the prover. It uses `process_host_hash` when the row has one. - New `native-runtime/native-owner-stamps.ts`: controller and adapter-process stamp builders, shared with the ownership registry. Not changed: `proveLocalWorkspaceOwnerStopped` (workspaces), `evaluateNativeProviderProcesses`, the stop observation in `recordNativeLocalProcessStop`, and `runProcessMayBeLive` in `native-workspace-sync-retention.ts`. ## Verification - `cd server && npx vitest run src/services/native-runtime/native-restart-recovery.test.ts src/services/controller-process-identity.test.ts`: 51 passed. - `cd server && npx vitest run src/services/native-runtime/native-failed-run-owner-release.test.ts`: 14 passed. - `cd server && npx vitest run src/services/native-runtime/native-controller-host-recovery.test.ts`: 2 passed. - `cd server && npx vitest run src/services/quarantined-task-retry.test.ts -t "keeps quarantine"`: 18 passed. `explicit-native-continuation.test.ts`: 414 passed. `ownership-reconciliation-report.test.ts`: 23 passed. - Red with the master versions of the two modules: 3 new tests fail ("accepts a reused PID only when the live process started definitely later", "never releases on a dead PID recorded in another PID space", "keeps the one-release fallback for a run process saved before its PID space, and records it"). The tests were written after the code. - Two fixtures changed to match the shared rules: - `controller-process-identity.test.ts`: the "recycled" case recorded a start after the live process, which is an inconsistent stamp. It now records an earlier start (a real reuse), and the inconsistent case is asserted as alive. - `quarantined-task-retry.test.ts` `worker_alive`: the live worker now records this process's real start time. The old fixture was a reused PID, which the prover correctly proves gone. - `cd server && npx vitest run src/services/native-local-process-stop.test.ts`: 5 passed (review round 1). With the master module, 2 fail: the other-PID-space case and the reused-PID case. - Not run locally: typecheck (CI runs it) and the wide suites. ## Risks Behaviour changes, all from the shared rules: - A live PID observed to start before the recorded controller is no longer accepted as recycled (`controller_pid_start_inconsistent`; the run waits for evidence). - A failed-run owner whose recorded adapter process or controller is in another PID space, or is a remote sandbox PID, is no longer released by a local probe. It keeps the lease path. - An unreadable start time is no longer exit evidence for a recovered local runner. - A local execution whose PID was reused by a later process now counts as stopped. Before, any live PID blocked the proof. - A local execution whose row records another PID space is no longer counted as stopped by a dead local PID. - A PID space that cannot be read here makes a hostless receipt owner unprovable. No persisted state changes. Rows saved by the previous release keep the one-release null-host fallback, as before. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edits, local test runs). ## 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
8475826d2d
commit
a4dbde6181
11 files changed
+291
-112
No files matched your search
@@ -302,8 +302,8 @@ import { noteNativeRunnerEvidence } from "./native-runner-evidence-signal.js";
|
||||
import { removeNativeHarnessBackup } from "./native-harness-backup-cleanup.js";
|
||||
import { registerLiveRunnerGoalController } from "../runner-goal-control-broker.js";
|
||||
import { applyRunnerGoalPrpEvent } from "../runner-goals.js";
|
||||
import { readProcessStartedAt } from "../hot-restart.js";
|
||||
import { processExists, processStartsDefinitelyDiffer } from "../controller-process-identity.js";
|
||||
import { currentProcessSpaceHash } from "../controller-process-identity.js";
|
||||
import { proveOwnerGone } from "../owner-proof.js";
|
||||
import {
|
||||
NativeRunnerOwnershipUnverifiedError,
|
||||
isNativeRunnerOwnershipHeld,
|
||||
@@ -591,32 +591,33 @@ const nativeRuntimeRequestResolutions = new Map<
|
||||
NativeRuntimeRequestResolution
|
||||
>();
|
||||
|
||||
/** The shared prover's verdict on a recovered local runner, recorded on this
|
||||
* host by an earlier boot (no boot id, so this process is never assumed). */
|
||||
function proveRecoveryProcess(input: { pid: number; startedAt: string }) {
|
||||
return proveOwnerGone({ kind: "local", scope: "native_recovery_runner", generation: null, host: null,
|
||||
processSpace: currentProcessSpaceHash(), bootId: null, pid: input.pid, processStartedAt: input.startedAt,
|
||||
processGroupId: null });
|
||||
}
|
||||
|
||||
/**
|
||||
* True when the recorded runner still owns its PID. A start time within the
|
||||
* shared precision window is the same process: `ps -o lstart` on macOS has
|
||||
* whole-second precision, so an exact match would call a live runner gone.
|
||||
* False only when the shared prover proves the recorded runner gone (its PID
|
||||
* is dead or reused by a later process). The runner treats false as exit
|
||||
* evidence, so an unreadable or inconsistent start time stays alive.
|
||||
*/
|
||||
export async function verifiedRecoveryProcessIsAlive(input: {
|
||||
export async function recoveryProcessMayBeAlive(input: {
|
||||
pid: number;
|
||||
startedAt: string;
|
||||
}): Promise<boolean> {
|
||||
if (!processExists(input.pid)) return false;
|
||||
try {
|
||||
const observed = await readProcessStartedAt(input.pid);
|
||||
return (
|
||||
observed !== null &&
|
||||
!processStartsDefinitelyDiffer(input.startedAt, observed)
|
||||
);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
return (await proveRecoveryProcess(input)).verdict !== "gone";
|
||||
}
|
||||
|
||||
async function signalVerifiedRecoveryProcess(
|
||||
input: { pid: number; startedAt: string },
|
||||
signal: NodeJS.Signals,
|
||||
): Promise<boolean> {
|
||||
if (!(await verifiedRecoveryProcessIsAlive(input))) return false;
|
||||
// Signal only the proven same process (start time within the shared
|
||||
// precision window), never an ambiguous or reused PID.
|
||||
if ((await proveRecoveryProcess(input)).verdict !== "alive") return false;
|
||||
try {
|
||||
return process.kill(input.pid, signal);
|
||||
} catch {
|
||||
@@ -5467,7 +5468,7 @@ const RETIREMENT_REASON_NAMES = {
|
||||
recoveryState: new Set(["blocked", "resuming_session", "awaiting_evidence", "awaiting_runner_reattach"]),
|
||||
controllerReason: new Set(["controller_still_alive", "coordinated_controller_still_alive", "remote_controller_lease_live",
|
||||
"expired_lease_without_controller_identity", "live_legacy_lease_without_controller_identity",
|
||||
"controller_host_unprovable", "controller_host_unrecorded"]),
|
||||
"controller_host_unprovable", "controller_host_unrecorded", "controller_pid_start_inconsistent"]),
|
||||
sandboxOutcome: new Set(["created", "resumed", "replacement"]),
|
||||
} as const;
|
||||
|
||||
@@ -14664,7 +14665,7 @@ async function createRunnerdBackendWithinSessionClaim(
|
||||
target, binding: input.execution.binding, trace: input.trace, onLog: input.onLog,
|
||||
}).controlPlaneRegistration,
|
||||
adoptExistingRunner: remoteDot?.adoptExistingRunner ?? (recoveredProcess ? { ...recoveredProcess,
|
||||
isAlive: () => verifiedRecoveryProcessIsAlive(recoveredProcess),
|
||||
isAlive: () => recoveryProcessMayBeAlive(recoveredProcess),
|
||||
signal: (signal: NodeJS.Signals = "SIGTERM") => signalVerifiedRecoveryProcess(recoveredProcess, signal) } : undefined),
|
||||
};
|
||||
const backendOptions = {
|
||||
@@ -16012,7 +16013,7 @@ async function createRunnerdBackendWithinSessionClaim(
|
||||
: target.kind === "local" && localRecoveryProcess
|
||||
? {
|
||||
...localRecoveryProcess,
|
||||
isAlive: () => verifiedRecoveryProcessIsAlive(localRecoveryProcess),
|
||||
isAlive: () => recoveryProcessMayBeAlive(localRecoveryProcess),
|
||||
signal: (signal: NodeJS.Signals) =>
|
||||
signalVerifiedRecoveryProcess(localRecoveryProcess, signal),
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user