mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-24 18:19:05 +02:00
fix(adapters): restore workspaces when instruction collection fails
Keep bridge and workspace cleanup in finally after the verified-stop collector. ACP records a bounded collection failure without exposing raw filesystem paths or preventing its normal teardown. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
14a003238f
commit
2c3c1f5d37
11 files changed
+160
-79
No files matched your search
@@ -6417,6 +6417,36 @@ describe("ACPX engine run lifecycle corrections (F3: one teardown error policy)"
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("restores managed home and releases the lease when instruction collection rejects", async () => {
|
||||
const { stateDir, localCwd, executionTarget } = await setupRemoteSandbox();
|
||||
const { paperclipStops, processStops, anyStopped } = stubBridges();
|
||||
const stagingLocks = new Map<string, Promise<unknown>>();
|
||||
const order: string[] = [];
|
||||
const logs: string[] = [];
|
||||
const execute = createAcpxEngineExecutor({
|
||||
stagingLocks, warmHandles: new Map(), stagedRuntimes: new Map(),
|
||||
prepareRemoteManagedHome: async (input) => ({
|
||||
stagedRuntime: await input.stage([]),
|
||||
teardown: async () => { order.push("restore"); return { ok: true }; },
|
||||
}),
|
||||
createRuntime: () => ({ ensureSession: async () => okHandle, startTurn: () => throwingTurn(),
|
||||
close: async () => { order.push("close"); } }) as never,
|
||||
});
|
||||
const result = await execute({ runId: "instruction-collection-reject",
|
||||
...remoteArgs(stateDir, localCwd, executionTarget, {
|
||||
onProviderStopped: async () => { order.push("collect"); throw new Error("collector failed reading /private/test-instructions/AGENTS.md"); },
|
||||
onLog: async (_stream: string, text: string) => { logs.push(text); },
|
||||
}),
|
||||
} as never);
|
||||
expect(result.exitCode).toBe(1);
|
||||
expect(order).toEqual(["close", "collect", "restore"]);
|
||||
expect(anyStopped(paperclipStops)).toBe(true);
|
||||
expect(anyStopped(processStops)).toBe(true);
|
||||
expect(stagingLocks.size).toBe(0);
|
||||
expect(logs.some(text => text.includes('teardown step "instruction-collection" failed: Instruction collection failed'))).toBe(true);
|
||||
expect(logs.some(text => text.includes("/private/test-instructions"))).toBe(false);
|
||||
});
|
||||
|
||||
it("test_staging_lease_releases_in_finally_on_every_exit_path", async () => {
|
||||
const scenarios: Array<{
|
||||
name: string;
|
||||
|
||||
@@ -5339,13 +5339,20 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) {
|
||||
// A transport error or terminal narrative cannot authorize collecting
|
||||
// mutable provider files. Require the owned close or local OS handle.
|
||||
const stopped = runtimeStopConfirmed || (!prepared.processSessionBridge && capturedProcessExited(processIdentitySink?.localProcess));
|
||||
if (stopped) await ctx.onProviderStopped?.();
|
||||
await runRuntimeSpan("sandbox.syncBack", async () => {
|
||||
const restoreOutcome = await syncBackManagedHome(prepared);
|
||||
if (!restoreOutcome.ok) {
|
||||
workspaceRestoreFailureField = { workspaceRestoreFailure: restoreOutcome.code };
|
||||
}
|
||||
});
|
||||
try {
|
||||
if (stopped) await ctx.onProviderStopped?.();
|
||||
} catch {
|
||||
// Match ACP's fail-soft teardown policy without describing this as
|
||||
// a workspace restore failure or exposing paths from a raw error.
|
||||
await recordTeardownError("instruction-collection", new Error("Instruction collection failed after provider stop. No instruction save is claimed."));
|
||||
} finally {
|
||||
await runRuntimeSpan("sandbox.syncBack", async () => {
|
||||
const restoreOutcome = await syncBackManagedHome(prepared);
|
||||
if (!restoreOutcome.ok) {
|
||||
workspaceRestoreFailureField = { workspaceRestoreFailure: restoreOutcome.code };
|
||||
}
|
||||
});
|
||||
}
|
||||
}),
|
||||
// The staging lease releases as the run's final act, AFTER the coordinator
|
||||
// reproduces the result, in the run root `finally` below. This step stays a
|
||||
|
||||
@@ -1358,16 +1358,19 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
|
||||
return toAdapterResult(initial, { fallbackSessionId: runtimeSessionId || runtime.sessionId });
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -200,6 +200,23 @@ describe("codex execute — outbound auth copy-back restore contribution", () =>
|
||||
expect(collected).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("stops the bridge and restores the workspace when instruction collection rejects", async () => {
|
||||
const order: string[] = [];
|
||||
startAdapterExecutionTargetPaperclipBridge.mockResolvedValueOnce({
|
||||
env: {}, stop: async () => { order.push("bridge-stop"); },
|
||||
} as never);
|
||||
prepareAdapterExecutionTargetRuntime.mockImplementationOnce(async () => ({
|
||||
target: { kind: "remote", transport: "ssh" }, workspaceRemoteDir: "/remote/workspace",
|
||||
runtimeRootDir: REMOTE_RUNTIME_ROOT, assetDirs: { home: `${REMOTE_RUNTIME_ROOT}/home` },
|
||||
restoreWorkspace: async () => { order.push("restore"); },
|
||||
}));
|
||||
await expect(runTeardown({ sandboxAuth: "{}", hostAuth: "{}", onProviderStopped: async () => {
|
||||
order.push("collect");
|
||||
throw new Error("instruction collection failed");
|
||||
} })).rejects.toThrow("instruction collection failed");
|
||||
expect(order).toEqual(["collect", "bridge-stop", "restore"]);
|
||||
});
|
||||
|
||||
it("declares a Codex `home` asset carrying both inbound provision and outbound restore contributions", async () => {
|
||||
await runTeardown({
|
||||
sandboxAuth: subscriptionAuth({ accountId: "acct", lastRefresh: "2026-07-09T01:00:00Z", marker: "s" }),
|
||||
|
||||
@@ -1583,30 +1583,33 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
executionError = error;
|
||||
throw error;
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
try {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
} catch (error) {
|
||||
await Promise.resolve(
|
||||
onLog(
|
||||
"stderr",
|
||||
`[paperclip] Failed to restore workspace changes from ${describeAdapterExecutionTarget(
|
||||
executionTarget,
|
||||
)}: ${error instanceof Error ? error.message : String(error)}\n`,
|
||||
),
|
||||
).catch(() => undefined);
|
||||
// A provider failure remains the primary outcome. When provider work
|
||||
// succeeded, however, silently accepting a failed copy-back can lose
|
||||
// the only workspace edits before a replacement sandbox starts.
|
||||
if (executionError === null) throw error;
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
try {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
} catch (error) {
|
||||
await Promise.resolve(
|
||||
onLog(
|
||||
"stderr",
|
||||
`[paperclip] Failed to restore workspace changes from ${describeAdapterExecutionTarget(
|
||||
executionTarget,
|
||||
)}: ${error instanceof Error ? error.message : String(error)}\n`,
|
||||
),
|
||||
).catch(() => undefined);
|
||||
// A provider failure remains the primary outcome. When provider work
|
||||
// succeeded, however, silently accepting a failed copy-back can lose
|
||||
// the only workspace edits before a replacement sandbox starts.
|
||||
if (executionError === null) throw error;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -778,19 +778,22 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
}
|
||||
return toResult(initial);
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
}
|
||||
if (localSkillsDir) {
|
||||
await fs.rm(localSkillsDir, { recursive: true, force: true }).catch(() => undefined);
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
if (paperclipBridge) {
|
||||
await paperclipBridge.stop();
|
||||
}
|
||||
if (restoreRemoteWorkspace) {
|
||||
await onLog(
|
||||
"stdout",
|
||||
`[paperclip] Restoring workspace changes from ${describeAdapterExecutionTarget(executionTarget)}.\n`,
|
||||
);
|
||||
await restoreRemoteWorkspace();
|
||||
}
|
||||
if (localSkillsDir) {
|
||||
await fs.rm(localSkillsDir, { recursive: true, force: true }).catch(() => undefined);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -787,11 +787,14 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
|
||||
return toResult(initial);
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -728,7 +728,13 @@ async function executeTurn(ctx: AdapterExecutionContext): Promise<AdapterExecuti
|
||||
};
|
||||
|
||||
try {
|
||||
return await withWorkspaceRestore(executeTurn, async () => { await providerStop.collectBeforeRestore(); await restoreRemoteWorkspace?.(); });
|
||||
return await withWorkspaceRestore(executeTurn, async () => {
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
await restoreRemoteWorkspace?.();
|
||||
}
|
||||
});
|
||||
} finally {
|
||||
// Cleanup runs after settlement on both success and failure.
|
||||
if (stagedGrokHomeDir) {
|
||||
|
||||
@@ -749,11 +749,14 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
|
||||
return toResult(initial);
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -758,12 +758,15 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
|
||||
return toResult(initial);
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
await preparedRuntimeConfig.cleanup();
|
||||
|
||||
@@ -870,12 +870,15 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
||||
|
||||
return toResult(initial);
|
||||
} finally {
|
||||
await providerStop.collectBeforeRestore();
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
try {
|
||||
await providerStop.collectBeforeRestore();
|
||||
} finally {
|
||||
await Promise.all([
|
||||
paperclipBridge?.stop(),
|
||||
restoreRemoteWorkspace?.(),
|
||||
localSkillsDir ? fs.rm(path.dirname(localSkillsDir), { recursive: true, force: true }).catch(() => undefined) : Promise.resolve(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
await preparedRuntimeConfig.cleanup();
|
||||
|
||||
Reference in new issue
Block a user