From cb381c6a5fdfd54b19af7eea0146ba477004034a Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 9 Sep 2026 04:44:59 -0500 Subject: [PATCH] Fix sandbox command-session and lazy ACP model recovery --- doc/sandbox-work-folders.md | 9 +++ .../acpx/codex-runtime-adapter.test.ts | 64 +++++++++++++++++ .../src/drivers/acpx/codex-runtime-adapter.ts | 19 +++-- .../daytona/src/plugin.test.ts | 71 +++++++++++++++++++ .../sandbox-providers/daytona/src/plugin.ts | 43 +++++++++-- tests/runner-e2e/deployed-stack.test.ts | 30 +++++++- tests/runner-e2e/deployed-stack.ts | 25 +++++++ .../runner-e2e/deployed-work-folders.spec.ts | 13 ++-- 8 files changed, 255 insertions(+), 19 deletions(-) diff --git a/doc/sandbox-work-folders.md b/doc/sandbox-work-folders.md index 728f9d078d..5a420268fa 100644 --- a/doc/sandbox-work-folders.md +++ b/doc/sandbox-work-folders.md @@ -104,6 +104,15 @@ whose cached handles still say running. Failure to stop a reusable sandbox is reported and retained for retry; it never falls back to deletion or orphan cleanup. This applies to both runner generations and leaves distinct task/user bindings isolated. +If Daytona rejects a command because its cached shell session no longer exists, +the provider creates one replacement session and retries within the original +command deadline. This applies only to a confirmed rejection before dispatch; +errors while polling or reading output never replay a potentially executed +command. Concurrent callers share the replacement session. +Native ACPX recovery also admits a provider started lazily by model selection. +Selection waits for verified process ownership before accepting the configured +model; cleanup and out-of-band process launches remain fenced. This matters +when reopening a persisted Codex session after stopping its sandbox. The new scoped-shell startup setting is not added to an existing unscoped Codex session's protected launch arguments. Its durable provider profile remains unchanged during attachment. diff --git a/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.test.ts b/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.test.ts index 633209f371..4600763e86 100644 --- a/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.test.ts @@ -1273,6 +1273,70 @@ describe("Codex ACPX runtime adapter", () => { } }); + it("verifies a provider lazily started by recovered model selection before admitting it", async () => { + const runtime = fakeRuntime(); + const command = fakeCommand(); + vi.mocked(command.spawn).mockReturnValue(fakeChild()); + let runtimeOptions: AcpRuntimeOptions | undefined; + let confirmOwnership!: () => void; + const ownership = new Promise((resolve) => { confirmOwnership = resolve; }); + const observedOwnership = vi.fn(() => ownership); + vi.mocked(runtime.setConfigOption!).mockImplementation(async () => { + await Promise.resolve(); + runtimeOptions!.spawnAgent!({ command: "ignored", args: ["--stdio"], options: {} }); + }); + const port = await openCodexAcpxRuntime(openOptions(command), { + createRegistry: () => registry(), createStore: () => store(), + awaitProviderOwnership: observedOwnership, + awaitProviderExit: providerOwnershipEstablished, + createRuntime: (options) => { runtimeOptions = options; return runtime; }, + }); + let admitted = false; + const selection = port.setModel!("gpt-5.6-sol").then(() => { admitted = true; }); + await vi.waitFor(() => expect(observedOwnership).toHaveBeenCalledOnce()); + expect(admitted).toBe(false); + confirmOwnership(); + await selection; + expect(admitted).toBe(true); + expect(command.spawn).toHaveBeenCalledOnce(); + expect(() => runtimeOptions!.spawnAgent!({ command: "ignored", args: [], options: {} })) + .toThrow("provider spawned after ownership admission was sealed"); + await port.close({ reason: "model selection verified" }); + }); + + it("reseals model admission after a rejected config operation", async () => { + const runtime = fakeRuntime(); + const failure = new Error("model selection rejected"); + vi.mocked(runtime.setConfigOption!).mockRejectedValueOnce(failure).mockResolvedValueOnce(undefined as never); + const port = await openCodexAcpxRuntime(openOptions(fakeCommand()), { + createRegistry: () => registry(), createStore: () => store(), createRuntime: () => runtime, + }); + await expect(port.setModel!("gpt-5.6-sol")).rejects.toBe(failure); + await expect(port.setModel!("gpt-5.6-sol")).resolves.toBeUndefined(); + await port.close({ reason: "selection error verified" }); + await expect(port.setModel!("gpt-5.6-sol")).rejects.toThrow("ownership admission is closed"); + expect(runtime.setConfigOption).toHaveBeenCalledTimes(2); + }); + + it("rejects model admission when the lazily started provider cannot prove ownership", async () => { + const runtime = fakeRuntime(); + const command = fakeCommand(); + vi.mocked(command.spawn).mockReturnValue(fakeChild()); + let runtimeOptions: AcpRuntimeOptions | undefined; + const failure = new Error("provider ownership unavailable"); + vi.mocked(runtime.setConfigOption!).mockImplementation(async () => { + runtimeOptions!.spawnAgent!({ command: "ignored", args: [], options: {} }); + }); + const port = await openCodexAcpxRuntime(openOptions(command), { + createRegistry: () => registry(), createStore: () => store(), + awaitProviderOwnership: async () => { throw failure; }, + awaitProviderExit: providerOwnershipEstablished, + createRuntime: (options) => { runtimeOptions = options; return runtime; }, + }); + await expect(port.setModel!("gpt-5.6-sol")).rejects.toBe(failure); + await port.close({ reason: "unverified model process rejected" }); + }); + it("maps prompt turns to the admitted ACPX handle", async () => { const runtime = fakeRuntime(); const turn = { diff --git a/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.ts b/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.ts index ba78a6a746..e704d64e94 100644 --- a/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.ts +++ b/packages/paperclip-runner/src/drivers/acpx/codex-runtime-adapter.ts @@ -1037,11 +1037,20 @@ function runtimePort( ...(runtime.setConfigOption ? { async setModel(model: string) { - await runtime.setConfigOption?.({ - handle, - key: "model", - value: model, - }); + // Loading a persisted session can leave its provider unstarted. + // ACP config selection may start it before the first prompt, so + // admit and verify that process just as we do for a resumed turn. + const finishOwnershipAdmission = + children.beginLifetimeOwnershipAdmission(); + try { + await runtime.setConfigOption?.({ + handle, + key: "model", + value: model, + }); + } finally { + await finishOwnershipAdmission(); + } }, } : {}), diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts index a89d833f94..8e05fad34a 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts @@ -1499,6 +1499,77 @@ describe("Daytona sandbox provider plugin", () => { expect(sessionId).toMatch(/^paperclip-/); }); + const missingSession = () => Object.assign(new MockDaytonaNotFoundError("session not found"), { + code: "PROCESS_NOT_FOUND", statusCode: 404, + }); + + it("replaces a disappeared session only when dispatch was rejected", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + mockGet.mockResolvedValue(sandbox); + await plugin.definition.onEnvironmentExecute?.(sessionExecParams()); + sandbox.process.executeSessionCommand.mockRejectedValueOnce(missingSession()); + await plugin.definition.onEnvironmentExecute?.(sessionExecParams({ stdin: "input" })); + expect(sandbox.process.createSession).toHaveBeenCalledTimes(2); + const ids = sandbox.process.executeSessionCommand.mock.calls.map(([id]) => id); + expect(ids[0]).toBe(ids[1]); + expect(ids[2]).not.toBe(ids[1]); + expect(sandbox.fs.uploadFile).toHaveBeenCalledTimes(2); + expect(sandbox.fs.deleteFile).toHaveBeenCalledTimes(2); + }); + + it("shares one replacement when concurrent commands find the same missing session", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + mockGet.mockResolvedValue(sandbox); + await plugin.definition.onEnvironmentExecute?.(sessionExecParams()); + const staleId = sandbox.process.createSession.mock.calls[0]![0]; + sandbox.process.executeSessionCommand.mockImplementation(async (id: string) => { + if (id === staleId) throw missingSession(); + return { cmdId: "accepted" }; + }); + await Promise.all([ + plugin.definition.onEnvironmentExecute?.(sessionExecParams()), + plugin.definition.onEnvironmentExecute?.(sessionExecParams()), + ]); + expect(sandbox.process.createSession).toHaveBeenCalledTimes(2); + const replacementId = sandbox.process.createSession.mock.calls[1]![0]; + expect(sandbox.process.executeSessionCommand.mock.calls.filter(([id]) => id === replacementId)).toHaveLength(2); + }); + + it("does not replay an accepted command when its session disappears while reading its status", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + mockGet.mockResolvedValue(sandbox); + const error = missingSession(); + sandbox.process.getSessionCommand.mockRejectedValue(error); + await expect(plugin.definition.onEnvironmentExecute?.(sessionExecParams())).rejects.toBe(error); + expect(sandbox.process.executeSessionCommand).toHaveBeenCalledTimes(1); + expect(sandbox.process.createSession).toHaveBeenCalledTimes(1); + }); + + it.each([ + new Error("session not found"), + new MockDaytonaNotFoundError("sandbox not found"), + ])("does not retry an unclassified dispatch failure: %s", async (error) => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + mockGet.mockResolvedValue(sandbox); + sandbox.process.executeSessionCommand.mockRejectedValueOnce(error); + await expect(plugin.definition.onEnvironmentExecute?.(sessionExecParams())).rejects.toBe(error); + expect(sandbox.process.executeSessionCommand).toHaveBeenCalledTimes(1); + }); + + it("bounds missing-session recovery to one replacement", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + mockGet.mockResolvedValue(sandbox); + sandbox.process.executeSessionCommand.mockRejectedValue(missingSession()); + await expect(plugin.definition.onEnvironmentExecute?.(sessionExecParams())).rejects.toThrow("before command dispatch"); + expect(sandbox.process.executeSessionCommand).toHaveBeenCalledTimes(2); + expect(sandbox.process.createSession).toHaveBeenCalledTimes(2); + }); + it("opens one session when two first commands overlap", async () => { process.env.DAYTONA_API_KEY = "host-key"; const sandbox = createMockSandbox(); diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.ts index 1167d244ce..39771fb590 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.ts @@ -1413,8 +1413,11 @@ const sandboxHandleSessionStore = (() => { idByKey.set(sandboxHandleCacheKey(scope), sessionId); } - function clear(scope: SandboxScope): void { - idByKey.delete(sandboxHandleCacheKey(scope)); + function clear(scope: SandboxScope, expectedSessionId?: string): void { + const key = sandboxHandleCacheKey(scope); + if (expectedSessionId === undefined || idByKey.get(key) === expectedSessionId) { + idByKey.delete(key); + } } // Single-flight guard for the first-command session create. Two overlapping @@ -1794,6 +1797,12 @@ async function readSessionExitCode( return null; } +class MissingSessionBeforeDispatchError extends Error { + constructor(cause: Error) { + super("Daytona session was unavailable before command dispatch", { cause }); + } +} + // Dispatch one user command into the persistent session and return its true // stdout and stderr. // @@ -1850,7 +1859,17 @@ async function executeInSession( sessionId, { command, runAsync: true }, timeoutSeconds, - ); + ).catch((error: unknown) => { + // The daemon rejected the session lookup before accepting a command. + // Never classify errors from polling/logs this way: that command may + // already have executed and must not be replayed. + if (error instanceof DaytonaNotFoundError + && error.code === "PROCESS_NOT_FOUND" + && error.message.trim().toLowerCase() === "session not found") { + throw new MissingSessionBeforeDispatchError(error); + } + throw error; + }); const commandId = dispatched.cmdId; // Log-stream path. A session command always tries the stream first: it @@ -2706,7 +2725,23 @@ const plugin = definePlugin({ let result: PluginEnvironmentExecuteResult; if (!params.bypassSession) { const sessionId = await getOrCreateSession(sandbox, scope); - result = await executeInSession(sandbox, sessionId, params, config); + const configuredTimeout = resolveTimeoutMs(params.timeoutMs, config); + const dispatchDeadline = Date.now() + (isGitNetworkCommand(params.command, params.args ?? []) + ? Math.min(configuredTimeout, GIT_NETWORK_TIMEOUT_MS) : configuredTimeout); + try { + result = await executeInSession(sandbox, sessionId, params, config); + } catch (error) { + if (!(error instanceof MissingSessionBeforeDispatchError)) throw error; + // A stopped/restarted provider can lose its shell independently of + // this worker's cache. Replace only the rejected id; overlapping + // callers must share, rather than invalidate, its replacement. + sandboxHandleSessionStore.clear(scope, sessionId); + if (Date.now() >= dispatchDeadline) throw error; + const replacementId = await getOrCreateSession(sandbox, scope); + const remainingMs = dispatchDeadline - Date.now(); + if (remainingMs <= 0) throw error; + result = await executeInSession(sandbox, replacementId, { ...params, timeoutMs: remainingMs }, config); + } } else { result = await executeOneShot(sandbox, params, config); } diff --git a/tests/runner-e2e/deployed-stack.test.ts b/tests/runner-e2e/deployed-stack.test.ts index 5b7cd447e4..e0f2fa6336 100644 --- a/tests/runner-e2e/deployed-stack.test.ts +++ b/tests/runner-e2e/deployed-stack.test.ts @@ -1,5 +1,31 @@ -import { describe, expect, it } from "vitest"; -import { isStagingOrigin, assertDeployedAdapterExclusions, deployedAgentEngine } from "./deployed-stack.js"; +import { describe, expect, it, vi } from "vitest"; +import { isStagingOrigin, assertDeployedAdapterExclusions, deployedAgentEngine, findDeployedWorkFile } from "./deployed-stack.js"; + +describe("deployed work-file pagination", () => { + it.each([false, true])("finds a later-page entry while preserving trash=%s", async (trash) => { + const file = { path: "nested/empty.sh", byteSize: 0, executable: true }; + const json = vi.fn().mockResolvedValueOnce({ files: [{ path: "unrelated" }], nextCursor: "next-page" }) + .mockResolvedValueOnce({ files: [file], nextCursor: null }); + expect(await findDeployedWorkFile({ json }, "/api/folder", file.path, trash)).toEqual(file); + expect(json.mock.calls.map(([url]) => url)).toEqual([ + `/api/folder?limit=200&trash=${trash}`, + `/api/folder?limit=200&trash=${trash}&cursor=next-page`, + ]); + }); + + it("returns missing only after the final page", async () => { + const json = vi.fn().mockResolvedValueOnce({ files: [], nextCursor: "next-page" }) + .mockResolvedValueOnce({ files: [], nextCursor: null }); + expect(await findDeployedWorkFile({ json }, "/api/folder", "missing")).toBeUndefined(); + expect(json).toHaveBeenCalledTimes(2); + }); + + it("rejects a repeated cursor instead of looping", async () => { + const json = vi.fn().mockResolvedValue({ files: [], nextCursor: "same-page" }); + await expect(findDeployedWorkFile({ json }, "/api/folder", "missing")).rejects.toThrow("repeated its cursor"); + expect(json).toHaveBeenCalledTimes(2); + }); +}); describe("deployed stack target", () => { it("requires an explicit HTTPS staging tenant and rejects credential-bearing URLs", () => { diff --git a/tests/runner-e2e/deployed-stack.ts b/tests/runner-e2e/deployed-stack.ts index 5a8d9b3d9e..db96b6bcf5 100644 --- a/tests/runner-e2e/deployed-stack.ts +++ b/tests/runner-e2e/deployed-stack.ts @@ -1,5 +1,30 @@ import fs from "node:fs"; import assert from "node:assert/strict"; +import type { WorkFolderListing } from "../../packages/shared/src/work-folders.js"; + +/** Find one entry without assuming a shared folder fits in the first page. */ +export async function findDeployedWorkFile( + api: Pick, + folderPath: string, + filePath: string, + trash = false, +) { + const cursors = new Set(); + let cursor: string | null = null; + do { + const query = new URLSearchParams({ limit: "200", trash: String(trash) }); + if (cursor) query.set("cursor", cursor); + const listing: WorkFolderListing = await api.json(`${folderPath}?${query}`); + const file = listing.files.find((entry) => entry.path === filePath); + if (file) return file; + cursor = listing.nextCursor; + if (cursor) { + assert(!cursors.has(cursor), "Work-folder listing repeated its cursor"); + cursors.add(cursor); + } + } while (cursor); + return undefined; +} // A separate target contract deliberately has no local-server fallback. export function isStagingOrigin(value: string) { diff --git a/tests/runner-e2e/deployed-work-folders.spec.ts b/tests/runner-e2e/deployed-work-folders.spec.ts index 432a412d65..476eb9da10 100644 --- a/tests/runner-e2e/deployed-work-folders.spec.ts +++ b/tests/runner-e2e/deployed-work-folders.spec.ts @@ -1,10 +1,10 @@ import { randomUUID } from "node:crypto"; import { test, expect } from "@playwright/test"; import type { EnvironmentCapabilities } from "../../packages/shared/src/environment-support.js"; -import type { SandboxWorkFolderManifest, WorkFolderListing, WorkFolderSyncStatus } from "../../packages/shared/src/work-folders.js"; +import type { SandboxWorkFolderManifest, WorkFolderSyncStatus } from "../../packages/shared/src/work-folders.js"; import { QUALIFIED_ACPX_RUNNER_MODELS } from "../../server/src/services/native-runtime/provider-profile.js"; import { pollUntil } from "./api.js"; -import { DeployedStackApi, deployedAgentEngine, loadDeployedStack } from "./deployed-stack.js"; +import { DeployedStackApi, deployedAgentEngine, findDeployedWorkFile, loadDeployedStack } from "./deployed-stack.js"; import { repoAcceptancePrompt } from "./work-folder-acceptance-prompts.js"; @@ -60,15 +60,13 @@ for (const [scope, owner] of [["task", stack.taskId], ["agent", stack.agentId], }); expect((await write()).ok).toBe(true); expect((await write()).ok).toBe(true); - const listing = await api.json(base); - const file = listing.files.find((entry) => entry.path === filename)!; + const file = (await findDeployedWorkFile(api, base, filename))!; expect(file).toMatchObject({ byteSize: 0, executable: true, deletedAt: null }); const download = await api.request(`${base}/content?path=${encodeURIComponent(filename)}`); expect(download.status).toBe(200); expect((await download.arrayBuffer()).byteLength).toBe(0); await api.json(`${base}/operations`, "POST", { action: "delete", path: filename }); expect((await api.request(`${base}/content?path=${encodeURIComponent(filename)}`)).status).toBe(404); - const trash = await api.json(`${base}?trash=true`); - expect(trash.files.some((entry) => entry.id === file.id)).toBe(true); + expect((await findDeployedWorkFile(api, base, filename, true))?.id).toBe(file.id); await api.json(`${base}/operations`, "POST", { action: "restore", fileId: file.id }); expect((await api.request(`${base}/content?path=${encodeURIComponent(filename)}`)).status).toBe(200); }); @@ -109,8 +107,7 @@ for (const profile of stack.profiles) { const read = await api.request(`${scoped}/content?path=${encodeURIComponent(`roundtrip-${nonce}/message.txt`)}`); expect(read.status, `${profile.id} ${scope} durable bytes`).toBe(200); expect(await read.text()).toBe(nonce); - const listing = await api.json(scoped); - expect(listing.files.find(file => file.path === `roundtrip-${nonce}/empty.sh`)).toMatchObject({byteSize:0, executable:true}); + expect(await findDeployedWorkFile(api, scoped, `roundtrip-${nonce}/empty.sh`)).toMatchObject({byteSize:0, executable:true}); } await api.json(`/api/issues/${issue.id}`, "PATCH", { status: "todo", description: repoAcceptancePrompt(nonce, true) });