mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(native): preserve parked Runners across controller restarts (#20586)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native execution parks a released sandbox Runner for a later turn. > - The warm idle timer detaches Core and leaves Runner retention to the host. > - Controller shutdown instead stops recently parked Runners through checkpoint cleanup. > - This makes reuse depend on whether the idle timer fired before shutdown. > - This pull request uses the same detach operation for idle expiry and shutdown. > - The benefit is consistent Runner retention without a new retention or recovery mechanism. ## Linked Issues or Issue Description Fixes: #20012. Related: #20005 unified cached and adopted process reuse. Closed #19975 included this shutdown change within a broader retention proposal. ## What Changed - Detach confirmed released entries during controller shutdown. Keep busy, unreleased, and close-marked entries on the existing cleanup path. - Share the detach operation with idle expiry and cached-owner handoff. - Record that a local handle has started detaching. Later cleanup cannot fall back to stopping its Runner after a partial detach failure. - Preserve explicit environment deletion, release proof, admission, and process-reuse checks. - Add Codex and Claude restart, detach-failure, and environment-deletion regressions. Add real Runner adoption cases after the restart sweep, including managed GitHub rebinding. - Document the lifetime rule and make the shutdown warning cover both detach and checkpoint failures. ## Verification - Final source revision: `e4bb6ecbd7`. Local environment: macOS arm64, Node 26.12.0. - New controller-restart and detach-failure tests fail against unchanged master: 4 expected failures. Old shutdown calls close or reports success instead of reporting detach failure. - Focused executor tests: 22 passed. - Full native session executor suite: 1,328 passed. - `cd server && node scripts/typecheck-layers.mjs`: passed all source and test layers. - `node scripts/check-module-boundaries.mjs` and `git diff --check`: passed. - Manual diff review and added-line credential/PII scan: passed. - `cd server && node ../node_modules/vitest/vitest.mjs run src/services/native-runtime/plugin-runner-release-resume.integration.test.ts --bail=1 --reporter=verbose`: 14 passed on this revision. This includes new Codex and Claude restart cases and managed GitHub rebinding. - Failure history: an earlier concurrent run had 4 passes and 10 failures. Its first failure was the existing Claude cached case exceeding 60 seconds. Later cases encountered a leftover prototype spy, recursive calls, and startup timeouts. No source changed before the passing full rerun. A clean worktree of unchanged master (`e01fe7f84a`) passed all 11 original cases. The first timeout's exact cause remains unknown; the later pass does not establish that cause. - Standard Greptile review: 5/5 on `e4bb6ecbd7bd965b5f56507d2c42c580e8a1d547`, with no actionable findings. - `node scripts/local-verify.mjs --explain` returned exit 2 because the execution-semantics document requires broad CI. Required CI will supply broad typecheck, test, and build coverage. - A hosted deployment and an actual controller process replacement have not been run. The added integration cases exercise the shutdown sweep and a new adopting transport against a real Runner. ## Risks - SIGINT and SIGTERM preserve confirmed parked Runners, including ordinary service stops. The host still owns eviction and retention. - Detach failure retains a fenced local owner and reports failure. Core does not infer permission to stop the Runner from that failure. - Reuse remains conditional on current admission, process configuration, and host availability. - No persisted schema, checkpoint format, launch identity, or migration changes. State from the previous release uses the existing adoption or verified restore paths. ## Model Used - OpenAI Codex, GPT-6, with reasoning, repository inspection, tool use, and local test execution. The exact deployment identifier and context-window size are not exposed in this session. ## 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 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
765ccd8691
commit
2e3cbe57a4
5 files changed
+65
-20
No files matched your search
@@ -4473,6 +4473,13 @@ admitted lifecycle, including when recovery acquires a lease after the agent or
|
||||
environment lifecycle setting changes. Recovery uses the persisted execution
|
||||
input for both lease acquisition and lifecycle validation.
|
||||
|
||||
Controller shutdown and warm idle expiry detach confirmed released plugin
|
||||
Runners without stopping them. The host owns parked Runner retention. The next
|
||||
turn uses the existing adoption and process-configuration checks to decide
|
||||
reuse. A failed detach keeps the local owner fenced and reports failure; it
|
||||
does not authorize a stop fallback. Unreleased sessions still checkpoint on
|
||||
shutdown, and explicit environment deletion still retires idle Runners.
|
||||
|
||||
A warm sandbox's shared workspace binding persists independently of the
|
||||
experimental isolated-workspaces UI. Ordinary workspace updates remain gated;
|
||||
the runtime can bind only a validated shared workspace in the issue's company
|
||||
|
||||
@@ -478,7 +478,7 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
enterShutdown();
|
||||
const idleSessions = await closeIdleWarmNativeSessionsForRestart();
|
||||
if (idleSessions.failed > 0) {
|
||||
logger.warn({ idleSessions }, "idle native sessions could not checkpoint before controller shutdown");
|
||||
logger.warn({ idleSessions }, "idle native session cleanup failed before controller shutdown");
|
||||
}
|
||||
let intent: Awaited<ReturnType<typeof readHotRestartIntent>>;
|
||||
try {
|
||||
|
||||
@@ -16910,14 +16910,19 @@ describe("runnerd provider runtime wiring", () => {
|
||||
} finally {owner.mockRestore();await rm(home,{recursive:true,force:true});}
|
||||
});
|
||||
|
||||
it.each((["codex", "claude"] as const).flatMap(provider => [[false, false, false, false], [true, false, false, false], [false, true, false, false], [false, false, true, false], ...(provider === "codex" ? [[false, false, false, true]] : [])].map(([unknown, shutdown, journalFailure, deadline]) => [provider, unknown, shutdown, journalFailure, deadline] as const)))(
|
||||
"releases settled %s only under its current owner and detaches its cache locally (unknown=%s, shutdown=%s, journalFailure=%s, deadline=%s)", async (provider, unknown, shutdown, journalFailure, deadline) => {
|
||||
it.each((["codex", "claude"] as const).flatMap(provider => [
|
||||
...[[false, false, false, false], [true, false, false, false], [false, true, false, false], [false, false, true, false], ...(provider === "codex" ? [[false, false, false, true]] : [])]
|
||||
.map(([unknown, shutdown, journalFailure, deadline]) => [provider, unknown, shutdown, journalFailure, deadline, "idle timeout"] as const),
|
||||
...(["controller restart", "detach failure", "environment deletion"] as const)
|
||||
.map(retirement => [provider, false, false, false, false, retirement] as const),
|
||||
]))(
|
||||
"releases settled %s only under its current owner and retires its cache (unknown=%s, shutdown=%s, journalFailure=%s, deadline=%s, retirement=%s)", async (provider, unknown, shutdown, journalFailure, deadline, retirement) => {
|
||||
await closeIdleWarmNativeSessionsForRestart();
|
||||
expect(readNativeSessionIdleWork()).toBe("none");
|
||||
const previousHome = process.env.PAPERCLIP_HOME;
|
||||
process.env.PAPERCLIP_HOME = isolatedStateDirectory;
|
||||
const current = scopedNativeExecution(`released-${provider}-${unknown}`);
|
||||
current.session.lifecyclePolicy = { mode: "warm", idleTimeoutMs: unknown ? 750 : 250 };
|
||||
current.session.lifecyclePolicy = { mode: "warm", idleTimeoutMs: retirement === "idle timeout" ? unknown ? 750 : 250 : 60_000 };
|
||||
if (provider === "claude") current.provider = { kind: "acpx", agent: "claude", model: "fixture", permissionMode: "deny-all" } as never;
|
||||
const source = { runnerInstanceId: "released-runner", environmentLeaseId: "released-lease", runId: current.binding.runId,
|
||||
normalizedSessionId: current.session.normalizedSessionId!, turnId: "settled-turn", itemId: "settled-item" };
|
||||
@@ -17055,7 +17060,28 @@ describe("runnerd provider runtime wiring", () => {
|
||||
expect(state.execute).toHaveBeenCalledTimes(1);
|
||||
expect(JSON.parse(await readFile(path, "utf8")).releasedRunner).toEqual(envelope.releasedRunner);
|
||||
}
|
||||
await vi.waitFor(() => expect(detachControllerForRestart).toHaveBeenCalledOnce());
|
||||
if (retirement === "environment deletion") {
|
||||
expect(await closeWarmNativeSessionsForEnvironment({ environmentId: target.environmentId, reason: "environment deleted" }))
|
||||
.toEqual({ closed: 1, busy: 0, failed: 0 });
|
||||
expect(close).toHaveBeenCalledExactlyOnceWith({ reason: "environment deleted" });
|
||||
expect(detachControllerForRestart).not.toHaveBeenCalled();
|
||||
return;
|
||||
}
|
||||
if (retirement === "detach failure") {
|
||||
detachControllerForRestart.mockRejectedValue(new Error("fixture journal drain failed"));
|
||||
for (let attempt = 0; attempt < 2; attempt++) {
|
||||
expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 0, busy: 0, failed: 1 });
|
||||
expect(close).not.toHaveBeenCalled();
|
||||
expect(readNativeSessionIdleWork()).toBe("present");
|
||||
expect(JSON.parse(await readFile(path, "utf8")).releasedRunner).toEqual(envelope.releasedRunner);
|
||||
}
|
||||
detachControllerForRestart.mockResolvedValue(undefined);
|
||||
}
|
||||
if (retirement !== "idle timeout") {
|
||||
expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 1, busy: 0, failed: 0 });
|
||||
expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 0, busy: 0, failed: 0 });
|
||||
} else await vi.waitFor(() => expect(detachControllerForRestart).toHaveBeenCalledOnce());
|
||||
expect(readNativeSessionIdleWork()).toBe("none");
|
||||
expect(close).not.toHaveBeenCalled();
|
||||
expect(task.mock.calls.map(([operation]) => (operation as { kind: string }).kind)).toEqual(["describe"]);
|
||||
expect(JSON.parse(await readFile(path, "utf8")).releasedRunner).toEqual(envelope.releasedRunner);
|
||||
@@ -17088,6 +17114,7 @@ describe("runnerd provider runtime wiring", () => {
|
||||
await expect(owners.mock.calls.at(-1)![0].onResumed!(source)).rejects.toThrow("released_source_mismatch");
|
||||
expect(JSON.parse(await readFile(path, "utf8")).releasedRunner).toEqual(replacement.releasedRunner);
|
||||
} finally {
|
||||
detachControllerForRestart.mockResolvedValue(undefined);
|
||||
allowRelease(); allowPhysicalStop(); await execute.catch(() => undefined); await closeIdleWarmNativeSessionsForRestart(); owners.mockRestore();
|
||||
if (previousHome === undefined) delete process.env.PAPERCLIP_HOME; else process.env.PAPERCLIP_HOME = previousHome;
|
||||
}
|
||||
|
||||
@@ -693,6 +693,7 @@ type WarmNativeSession = {
|
||||
environmentId: string | null;
|
||||
busy: boolean;
|
||||
released?: boolean;
|
||||
controllerDetachedForRestart?: boolean;
|
||||
finishRelease?: () => Promise<void>;
|
||||
closeOnReleaseReason?: string;
|
||||
idleTimer: ReturnType<typeof setTimeout> | null;
|
||||
@@ -703,6 +704,7 @@ async function closeWarmNativeSession(entry: WarmNativeSession, reason: string,
|
||||
preserveInstructionsForRunId?: string;
|
||||
discardUnsettledSession?: boolean;
|
||||
}) {
|
||||
if (entry.controllerDetachedForRestart) return detachReleasedWarmNativeSession(entry);
|
||||
// Revoke before awaiting process retirement/checkpoint IO.
|
||||
const stopping = entry.githubAccess?.stop();
|
||||
try {
|
||||
@@ -728,6 +730,13 @@ async function closeWarmNativeSession(entry: WarmNativeSession, reason: string,
|
||||
finally { await stopping; }
|
||||
}
|
||||
|
||||
async function detachReleasedWarmNativeSession(entry: WarmNativeSession) {
|
||||
entry.controllerDetachedForRestart = true;
|
||||
if (!entry.session.detachControllerForRestart) throw new Error("native_session_released_detach_unavailable");
|
||||
await entry.session.detachControllerForRestart();
|
||||
await entry.githubAccess?.stop();
|
||||
}
|
||||
|
||||
const warmNativeSessions = new Map<string, WarmNativeSession>();
|
||||
function forgetWarmNativeSession(scope: string, entry: WarmNativeSession) {
|
||||
if (warmNativeSessions.get(scope) !== entry) return;
|
||||
@@ -895,7 +904,7 @@ export async function closeWarmNativeSessionsForEnvironment(input: {
|
||||
return closeIdleWarmNativeSessions(input);
|
||||
}
|
||||
|
||||
/** Suspend idle owners and persist their remote backup before a controller
|
||||
/** Detach released owners and checkpoint unreleased owners before a controller
|
||||
* exits. Active turns keep their separate authenticated restart handoff. */
|
||||
export async function closeIdleWarmNativeSessionsForRestart(): Promise<{
|
||||
closed: number; busy: number; failed: number;
|
||||
@@ -903,6 +912,7 @@ export async function closeIdleWarmNativeSessionsForRestart(): Promise<{
|
||||
return closeIdleWarmNativeSessions({
|
||||
reason: "controller restart",
|
||||
closeBusyOnRelease: true,
|
||||
detachReleased: true,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -916,6 +926,7 @@ async function closeIdleWarmNativeSessions(input: {
|
||||
environmentId?: string;
|
||||
reason: string;
|
||||
closeBusyOnRelease?: boolean;
|
||||
detachReleased?: boolean;
|
||||
canRetire?: () => boolean;
|
||||
}): Promise<{ closed: number; busy: number; failed: number }> {
|
||||
let closed = 0;
|
||||
@@ -937,13 +948,14 @@ async function closeIdleWarmNativeSessions(input: {
|
||||
}
|
||||
// The host owns retention after release; a quiet local handle needs no retirement for sleep.
|
||||
if (input.canRetire && entry.released && !entry.closeOnReleaseReason) continue;
|
||||
const detach = input.detachReleased === true && entry.released === true && !entry.closeOnReleaseReason;
|
||||
if (entry.idleTimer !== null) clearTimeout(entry.idleTimer);
|
||||
// Keep a fenced owner until both retirement and stopped collection succeed.
|
||||
// Failure must not make a live directory eligible for a replacement owner.
|
||||
entry.busy = true;
|
||||
entry.closeOnReleaseReason = input.reason;
|
||||
if (!detach) entry.closeOnReleaseReason = input.reason;
|
||||
const closing = Promise.resolve().then(() =>
|
||||
closeWarmNativeSession(entry, input.reason),
|
||||
detach ? detachReleasedWarmNativeSession(entry) : closeWarmNativeSession(entry, input.reason),
|
||||
);
|
||||
closingWarmNativeSessions.set(sessionId, closing);
|
||||
try {
|
||||
@@ -951,6 +963,7 @@ async function closeIdleWarmNativeSessions(input: {
|
||||
forgetWarmNativeSession(sessionId, entry);
|
||||
closed += 1;
|
||||
} catch {
|
||||
if (detach) entry.closeOnReleaseReason = "released Runner local detach is unconfirmed";
|
||||
failed += 1;
|
||||
} finally {
|
||||
entry.busy = false;
|
||||
@@ -8020,11 +8033,7 @@ function armWarmNativeSessionIdleTimer(
|
||||
current.busy = true;
|
||||
const released = current.released === true;
|
||||
if (!released) current.closeOnReleaseReason = "warm native session idle timeout";
|
||||
const closing = (released ? (async () => {
|
||||
if (!current.session.detachControllerForRestart) throw new Error("native_session_released_detach_unavailable");
|
||||
await current.session.detachControllerForRestart();
|
||||
await current.githubAccess?.stop();
|
||||
})() : closeWarmNativeSession(current, current.closeOnReleaseReason!))
|
||||
const closing = (released ? detachReleasedWarmNativeSession(current) : closeWarmNativeSession(current, current.closeOnReleaseReason!))
|
||||
.then(() => { forgetWarmNativeSession(sessionId, current); })
|
||||
.catch(() => {
|
||||
if (released) current.closeOnReleaseReason = "released Runner local detach is unconfirmed";
|
||||
@@ -9898,10 +9907,8 @@ async function executePaperclipNativeSessionAttempt(
|
||||
const resumeFrom = await resolvePluginTaskResume(input.db, { companyId: input.execution.binding.companyId,
|
||||
leaseId: target.leaseId, priorIdentity: proof.source, inspectCheckpoint: true });
|
||||
if (resumeFrom !== proof.taskId) throw new Error("native_session_released_owner_scope_mismatch");
|
||||
if (!cachedReleasedOwner.session.detachControllerForRestart) throw new Error("native_session_released_detach_unavailable");
|
||||
try {
|
||||
await cachedReleasedOwner.session.detachControllerForRestart();
|
||||
await cachedReleasedOwner.githubAccess?.stop();
|
||||
await detachReleasedWarmNativeSession(cachedReleasedOwner);
|
||||
} catch (error) {
|
||||
cachedReleasedOwner.closeOnReleaseReason = "released Runner local detach is unconfirmed";
|
||||
throw error;
|
||||
|
||||
@@ -43,11 +43,12 @@ const parkId = (source: DurableRecoveryIdentity) => `park_${sha(JSON.stringify(O
|
||||
describeEmbeddedPostgres("plugin Runner release/resume through the Core execution entry", () => {
|
||||
const environment = nativeRestartFixture();
|
||||
const nativeIt = process.platform === "win32" ? it.skip : realProcessIt;
|
||||
nativeIt.for([...(["codex", "claude"] as const).flatMap(harness => (["cached", "adopted"] as const).map(mode => [harness, mode] as const)), ["codex", "adopted cancellation"] as const, ["codex", "adopted legacy cancellation"] as const,
|
||||
nativeIt.for([...(["codex", "claude"] as const).flatMap(harness => (["cached", "adopted", "adopted restart"] as const).map(mode => [harness, mode] as const)), ["codex", "adopted cancellation"] as const, ["codex", "adopted legacy cancellation"] as const,
|
||||
["codex", "cached cancellation"] as const, ["codex", "cached held claim"] as const, ["claude", "cached held claim"] as const,
|
||||
["codex", "cached managed GitHub"] as const, ["codex", "adopted managed GitHub"] as const])("completes %s runs through a retained %s process", { timeout: 60_000 }, async ([harness, mode], context) => {
|
||||
["codex", "cached managed GitHub"] as const, ["codex", "adopted managed GitHub"] as const, ["codex", "adopted restart managed GitHub"] as const])("completes %s runs through a retained %s process", { timeout: 60_000 }, async ([harness, mode], context) => {
|
||||
if (harness === "claude" && !existsSync(fakeAcpxSidecar)) context.skip();
|
||||
const adopted = mode.startsWith("adopted"), cancellation = mode.includes("cancellation"), heldClaimMode = mode === "cached held claim";
|
||||
const restart = mode.includes("restart");
|
||||
const qualifiedCancellation = mode === "adopted cancellation" || mode === "cached cancellation";
|
||||
const managedGitHub = mode.endsWith("managed GitHub"), brokerUrl = "https://core.example.test/api", brokerToken = (runId: string) => `fixture-broker-${runId}`;
|
||||
const preparedBrokers: Array<{ runId: string; token?: string }> = [];
|
||||
@@ -76,7 +77,7 @@ describeEmbeddedPostgres("plugin Runner release/resume through the Core executio
|
||||
task: { identifier: "RELEASE-ENTRY", title: issue!.title, description: null, prompt: "Complete the controlled task.", workMode: "standard" },
|
||||
workspace: { cwd: workspace, repoUrl: null, repoRef: null, branchName: null },
|
||||
session: { normalizedSessionId: native.normalizedSessionId, driverKind: harness === "codex" ? "codex_app_server" : "acpx_runtime", protocolVersion: 1,
|
||||
lifecyclePolicy: { mode: "warm", idleTimeoutMs: adopted ? 5_000 : 60_000 } },
|
||||
lifecyclePolicy: { mode: "warm", idleTimeoutMs: adopted && !restart ? 5_000 : 60_000 } },
|
||||
completionContract: { id: contract!.id, sha256: contract!.canonicalSha256, schemaVersion: "paperclip.completion-contract.v1",
|
||||
contract: buildNativeCompletionContract(issue!, { revision: contract!.revision }) }, interactionResponses: [], credentialBindings: [] });
|
||||
const resultFile = join(root, "result.json"), calls = join(root, "provider-calls");
|
||||
@@ -256,7 +257,10 @@ describeEmbeddedPostgres("plugin Runner release/resume through the Core executio
|
||||
.toEqual({ closed: 0, busy: 0, failed: 0 });
|
||||
expect(stops).toBe(0); expect(handle!.isExitConfirmed?.()).toBe(false);
|
||||
if (adopted) {
|
||||
await new Promise(done => setTimeout(done, 5_200));
|
||||
if (restart) {
|
||||
expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 1, busy: 0, failed: 0 });
|
||||
expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 0, busy: 0, failed: 0 });
|
||||
} else await new Promise(done => setTimeout(done, 5_200));
|
||||
expect(stops).toBe(0); expect(handle!.isExitConfirmed?.()).toBe(false);
|
||||
expect(await proof()).toMatchObject({ source: releases[0], taskId });
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user