mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 14:16:39 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Sandbox runs prepare credentials and files before an agent starts. > - Stop must work during that preparation. > - ACPX registered cancellation, but did not handle it while a setup command was waiting. > - Daytona cleanup waited for that same command before stopping the sandbox. > - This change stops the run's sandbox first and requires proof before abandoning setup. ## Linked Issues or Issue Description **What happened?** Stop left a sandbox run active when remote credential setup stalled. The run kept its connection lease until the sandbox was stopped separately. **Expected behavior** Stop terminates the selected run's sandbox, prevents later setup from launching the agent, and lets run cleanup finish. It must not report success without proof from the provider. **Steps to reproduce** 1. Start an ACPX agent in Daytona. 2. Hold a command during remote credential or file setup. 3. Select Stop before the agent starts. 4. Before this fix, cleanup waits for the held command and never reaches sandbox stop. Related: #13351 exposed this during connection acceptance testing. #12150 addresses scheduler load and session initialization limits, a separate startup problem. ## What Changed - Handle cancellation during ACPX sandbox preparation with a host-owned stop callback. - Pass an explicit active-work cancellation flag through environment cleanup. - Stop Daytona before draining setup commands. Keep normal graceful cleanup. - Require an exact run and lease termination receipt. Keep ownership of outstanding requests when stop cannot be verified. - Reject late setup work and defer sandbox resume until old requests settle. - Add regression tests and document the cancellation boundary. ## Verification - Red: both the stalled ACPX setup test and the Daytona cancellation test failed before the fix because Stop never reached the provider. - Green: adapter and Daytona suites passed, along with cancellation boundary and database-backed receipt/isolation tests. - Two real Daytona probes ran a five-minute setup command. Cancellation returned matching stopped receipts in 6.01 and 6.533 seconds. No later setup command ran. Both test sandboxes were deleted. - Repository typecheck and build passed locally. The full required CI suite passed, including all server/workspace tests, browser shards, Paperclip Runner verification, and canary packaging. The duplicate local full-suite run was interrupted after CI passed; it is not claimed as a completed local pass. - No UI changes. The live probe uses the actual adapter cancellation boundary and Daytona plugin; it is not a browser acceptance test. ## Risks - Provider stop failures remain unacknowledged. The adapter keeps ownership while its original requests remain active. - A retry can receive a settling-work error until old provider requests finish. - Cancelled sandboxes are stopped and retained under their existing provider expiry policy. - Local execution and cancellation after the agent turn starts keep their current behavior. - Other providers must return an exact termination receipt to permit early setup cancellation. No database migration. ## Model Used OpenAI GPT-6 through Codex, with code execution and tool use. The exact deployment identifier and context window 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 (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 - [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>
125 lines
6.0 KiB
TypeScript
125 lines
6.0 KiB
TypeScript
import { describe, expect, it, vi } from "vitest";
|
|
import type { AdapterExecutionContext } from "../types.js";
|
|
import type { CommandManagedDuplexChannel, CommandManagedRuntimeRunner } from "../command-managed-runtime.js";
|
|
import { cancellableSandboxStartup } from "./startup-cancellation.js";
|
|
|
|
function deferred<T>() {
|
|
let resolve!: (value: T) => void;
|
|
let reject!: (error: unknown) => void;
|
|
const promise = new Promise<T>((yes, no) => { resolve = yes; reject = no; });
|
|
return { promise, resolve, reject };
|
|
}
|
|
const result = { exitCode: 0, signal: null, timedOut: false, stdout: "", stderr: "" };
|
|
function fixture(stop = vi.fn(async () => {}), extra: Partial<CommandManagedRuntimeRunner> = {}) {
|
|
const controller = new AbortController();
|
|
const execute = vi.fn(async () => result);
|
|
const ctx = {
|
|
signal: controller.signal, stopRemoteStartup: stop,
|
|
executionTarget: { kind: "remote", transport: "sandbox", providerKey: "daytona", runner: { execute, ...extra } },
|
|
} as unknown as AdapterExecutionContext;
|
|
const startup = cancellableSandboxStartup(ctx);
|
|
const runner = (startup.context.executionTarget as { runner: CommandManagedRuntimeRunner }).runner;
|
|
return { controller, execute, stop, startup, runner };
|
|
}
|
|
|
|
describe("sandbox startup cancellation boundary", () => {
|
|
it("keeps ownership until termination is confirmed, including an early command error", async () => {
|
|
const receipt = deferred<void>();
|
|
const command = deferred<typeof result>();
|
|
const f = fixture(vi.fn(() => receipt.promise));
|
|
f.execute.mockReturnValue(command.promise);
|
|
let settled = false;
|
|
const outcome = f.runner.execute({ command: "setup" }).catch(error => error).finally(() => { settled = true; });
|
|
await vi.waitFor(() => expect(f.execute).toHaveBeenCalledOnce());
|
|
f.controller.abort(new Error("Stopped"));
|
|
command.reject(new Error("socket closed"));
|
|
await vi.waitFor(() => expect(f.stop).toHaveBeenCalledOnce());
|
|
expect(settled).toBe(false);
|
|
receipt.resolve();
|
|
expect(await outcome).toEqual(new Error("Stopped"));
|
|
await expect(f.startup.finish()).rejects.toThrow("Stopped");
|
|
await expect(f.runner.execute({ command: "late setup" })).rejects.toThrow("Stopped");
|
|
expect(f.execute).toHaveBeenCalledOnce();
|
|
});
|
|
|
|
it("does not abandon a hung command when the provider cannot confirm stop", async () => {
|
|
const command = deferred<typeof result>();
|
|
const f = fixture(vi.fn(async () => { throw new Error("stop unverified"); }));
|
|
f.execute.mockReturnValue(command.promise);
|
|
let settled = false;
|
|
const outcome = f.runner.execute({ command: "setup" }).catch(error => error).finally(() => { settled = true; });
|
|
await vi.waitFor(() => expect(f.execute).toHaveBeenCalledOnce());
|
|
f.controller.abort();
|
|
await vi.waitFor(() => expect(f.stop).toHaveBeenCalledOnce());
|
|
expect(settled).toBe(false);
|
|
command.resolve(result);
|
|
expect(await outcome).toEqual(new Error("stop unverified"));
|
|
await expect(f.startup.finish()).rejects.toThrow("stop unverified");
|
|
});
|
|
|
|
it("waits for all parallel setup requests after an unverified stop", async () => {
|
|
const first = deferred<typeof result>();
|
|
const second = deferred<typeof result>();
|
|
const f = fixture(vi.fn(async () => { throw new Error("stop unverified"); }));
|
|
f.execute.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise);
|
|
let settled = false;
|
|
const a = f.runner.execute({ command: "first" }).catch(error => error).finally(() => { settled = true; });
|
|
const b = f.runner.execute({ command: "second" }).catch(error => error);
|
|
await vi.waitFor(() => expect(f.execute).toHaveBeenCalledTimes(2));
|
|
f.controller.abort();
|
|
first.reject(new Error("first failed"));
|
|
await vi.waitFor(() => expect(f.stop).toHaveBeenCalledOnce());
|
|
expect(settled).toBe(false);
|
|
second.resolve(result);
|
|
expect(await a).toEqual(new Error("stop unverified"));
|
|
expect(await b).toEqual(new Error("stop unverified"));
|
|
await expect(f.startup.finish()).rejects.toThrow("stop unverified");
|
|
});
|
|
|
|
it("disarms after successful startup and leaves ordinary turn cancellation alone", async () => {
|
|
const f = fixture();
|
|
await f.runner.execute({ command: "setup" });
|
|
await f.startup.finish();
|
|
f.controller.abort();
|
|
await expect(f.runner.execute({ command: "normal turn cleanup" })).resolves.toEqual(result);
|
|
expect(f.stop).not.toHaveBeenCalled();
|
|
expect(f.execute).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it("never starts a command when cancellation arrives before setup", async () => {
|
|
const f = fixture();
|
|
f.controller.abort(new Error("Stopped"));
|
|
await expect(f.runner.execute({ command: "setup" })).rejects.toThrow("Stopped");
|
|
await expect(f.startup.finish()).rejects.toThrow("Stopped");
|
|
expect(f.execute).not.toHaveBeenCalled();
|
|
expect(f.stop).toHaveBeenCalledOnce();
|
|
});
|
|
|
|
it("closes a duplex route returned after startup was cancelled", async () => {
|
|
const opening = deferred<CommandManagedDuplexChannel>();
|
|
const channel = {
|
|
write: vi.fn(), onData: vi.fn(), onExit: vi.fn(), stop: vi.fn(),
|
|
close: vi.fn(async () => {}),
|
|
};
|
|
const openDuplexChannel = vi.fn(() => opening.promise);
|
|
const f = fixture(vi.fn(async () => {}), { openDuplexChannel });
|
|
const outcome = f.runner.openDuplexChannel!({ command: ["agent"] }).catch(error => error);
|
|
await vi.waitFor(() => expect(openDuplexChannel).toHaveBeenCalledOnce());
|
|
f.controller.abort(new Error("Stopped"));
|
|
expect(await outcome).toEqual(new Error("Stopped"));
|
|
opening.resolve(channel);
|
|
await vi.waitFor(() => expect(channel.close).toHaveBeenCalledOnce());
|
|
await expect(f.startup.finish()).rejects.toThrow("Stopped");
|
|
});
|
|
|
|
it("does not stop another run's runner", async () => {
|
|
const a = fixture();
|
|
const b = fixture();
|
|
a.controller.abort(new Error("Stopped"));
|
|
await expect(a.startup.finish()).rejects.toThrow("Stopped");
|
|
await expect(b.runner.execute({ command: "setup" })).resolves.toEqual(result);
|
|
await b.startup.finish();
|
|
expect(b.stop).not.toHaveBeenCalled();
|
|
});
|
|
});
|