diff --git a/doc/sandbox-work-folders.md b/doc/sandbox-work-folders.md index f3118eb7f2..a06ca20c56 100644 --- a/doc/sandbox-work-folders.md +++ b/doc/sandbox-work-folders.md @@ -94,8 +94,13 @@ A follow-up task run waits up to 30 seconds for a terminal predecessor to releas its reusable sandbox. While that handoff is pending, startup cannot allocate a second workspace or resume the sandbox concurrently. An active predecessor or an incomplete release reports a retryable resume error and preserves the lease. +The next run claims the reusable lease in Postgres before resuming the provider; +competing server processes cannot both resume the same released sandbox. This applies to both runner generations and leaves distinct task/user bindings isolated. +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. Acceptance must resume representative pre-upgrade legacy and native tasks with committed, staged, unstaged, and untracked work, verify their original paths and diff --git a/packages/paperclip-runner/src/drivers/codex/codex-security-config.test.ts b/packages/paperclip-runner/src/drivers/codex/codex-security-config.test.ts index 9d3162fb0b..24300d4a9e 100644 --- a/packages/paperclip-runner/src/drivers/codex/codex-security-config.test.ts +++ b/packages/paperclip-runner/src/drivers/codex/codex-security-config.test.ts @@ -172,6 +172,9 @@ describe("Codex security configuration", () => { it("uses the outer sandbox for default-mode commands only when the controller authorizes it", () => { const source = { PAPERCLIP_RUNNER_EXTERNAL_SANDBOX: "1" }; const externalArgs = createIsolatedCodexAppServerArgs(source); + // Adding this protected flag to pre-work-folder sessions breaks run.attach + // even though their provider session and retained workspace are intact. + expect(externalArgs).not.toContain("allow_login_shell=false"); const serializedExternalArgs = externalArgs.join("\n"); expect(externalArgs).toContain( "--dangerously-bypass-approvals-and-sandbox", diff --git a/packages/paperclip-runner/src/drivers/codex/codex-security-config.ts b/packages/paperclip-runner/src/drivers/codex/codex-security-config.ts index 7ddedbc3f6..888c00e330 100644 --- a/packages/paperclip-runner/src/drivers/codex/codex-security-config.ts +++ b/packages/paperclip-runner/src/drivers/codex/codex-security-config.ts @@ -136,8 +136,11 @@ export function createIsolatedCodexAppServerArgs( `permissions.${CODEX_SKILLLESS_PERMISSION_PROFILE}.network.enabled=${hasGitHubCredential}`, ...(externalRunnerSandbox ? [ - "-c", - "allow_login_shell=false", + // Existing unscoped sessions keep their protected launch profile. + // Only the new scoped layout requires HOME/PATH to survive shell startup. + ...(externalWorkFolderEnvironment(source).HOME + ? ["-c", "allow_login_shell=false"] + : []), "-c", `permissions.${CODEX_EXTERNAL_SANDBOX_PERMISSION_PROFILE}.filesystem={":root"="write"}`, "-c", diff --git a/server/src/__tests__/environment-runtime.test.ts b/server/src/__tests__/environment-runtime.test.ts index 21473df84b..c4b587e2e6 100644 --- a/server/src/__tests__/environment-runtime.test.ts +++ b/server/src/__tests__/environment-runtime.test.ts @@ -5855,6 +5855,43 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { expect(result.value.lease.metadata?.sandboxLeaseAcquisition).toEqual({ outcome: "resumed" }); expect(workerManager.call).toHaveBeenCalledOnce(); expect((await environmentService(db).getLeaseById(first.lease.id))?.status).toBe("expired"); + + // A second server may arrive while the provider resume RPC is still in + // flight. It must see the new database owner before it can touch the disk. + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, nextRunId)); + await environmentService(db).releaseLease(result.value.lease.id, "released"); + const raceIds = [randomUUID(), randomUUID()]; + await db.insert(heartbeatRuns).values(raceIds.map((id) => ({ + id, companyId: seeded.companyId, agentId: seeded.agentId, status: "running", + }))); + let finishResume!: () => void; + let startedResume!: () => void; + const resumeStarted = new Promise((resolve) => { startedResume = resolve; }); + const resumeGate = new Promise((resolve) => { finishResume = resolve; }); + vi.mocked(workerManager.call).mockClear(); + vi.mocked(workerManager.call).mockImplementation(async (_pluginId, method) => { + if (method !== "environmentResumeLease") throw new Error(`Unexpected replacement: ${method}`); + startedResume(); + await resumeGate; + return { providerLeaseId: first.lease.providerLeaseId, metadata: { + provider: "fake-plugin", image: "fake:test", timeoutMs: 1234, reuseLease: true, + } }; + }); + const winner = acquire(raceIds[0]!, taskId); + // If acquisition regresses before the RPC, surface that rejection too. + await Promise.race([resumeStarted, winner.then(() => undefined)]); + try { + const otherServer = environmentRuntimeService(db, { pluginWorkerManager: workerManager }); + await expect(otherServer.acquireRunLease({ + companyId: seeded.companyId, environment: seeded.environment, issueId: taskId, + agentId: seeded.agentId, heartbeatRunId: raceIds[1]!, adapterType, + persistedExecutionWorkspace: { id: seeded.executionWorkspaceId, mode: "shared_workspace" }, + })).rejects.toThrow("the lease was preserved and no replacement was created"); + expect(workerManager.call).toHaveBeenCalledOnce(); + } finally { + finishResume(); + await winner; + } }); it.each([false, true])("preserves active reusable sandbox leases held by another running run (same task: %s)", async (sameTask) => { diff --git a/server/src/services/environment-runtime.ts b/server/src/services/environment-runtime.ts index 448aca6b03..9bc92af396 100644 --- a/server/src/services/environment-runtime.ts +++ b/server/src/services/environment-runtime.ts @@ -1295,6 +1295,31 @@ function createSandboxEnvironmentDriver( ); const environmentsSvc = environmentService(db); + async function claimReusableLeaseBeforeResume( + lease: EnvironmentLease, + input: Parameters[0], + ): Promise { + if (!input.heartbeatRunId || lease.heartbeatRunId === input.heartbeatRunId) return lease; + // Transfer ownership atomically before touching the provider. Two server + // processes can observe the same released lease; only the winner of this + // conditional update/insert may resume its sandbox. A failed resume leaves + // the claimed row with this run for normal retention/recovery cleanup. + return environmentsSvc.acquireLease({ + companyId: input.companyId, + environmentId: input.environment.id, + executionWorkspaceId: input.executionWorkspaceId, + issueId: input.issueId, + heartbeatRunId: input.heartbeatRunId, + assertCompanyBinding: input.assertCompanyBinding, + leasePolicy: lease.leasePolicy, + provider: lease.provider, + providerLeaseId: lease.providerLeaseId, + expiresAt: lease.expiresAt ? new Date(lease.expiresAt) : null, + metadata: lease.metadata, + replacesReusableLeaseId: lease.id, + }); + } + // A live sandbox whose teardown failed needs a durable `pending_cleanup` row, // so a sweep can find and release it. When every synchronous write attempt // fails, the database is down but the process still runs. So the driver keeps @@ -1941,7 +1966,7 @@ function createSandboxEnvironmentDriver( ? (reusableExistingLeases.find((lease) => lease.metadata?.workFolderRecoveryRequired === true)?.providerLeaseId ?? findReusableSandboxLeaseId({ config: storedConfig, leases: reusableExistingLeases })) : null; - const reusableLease = reusableProviderLeaseId + let reusableLease = reusableProviderLeaseId ? reusableExistingLeases.find((lease) => lease.providerLeaseId === reusableProviderLeaseId) : null; @@ -1971,6 +1996,8 @@ function createSandboxEnvironmentDriver( providerLeaseId: reusableLease.providerLeaseId, }); } + reusableLease = await claimReusableLeaseBeforeResume(reusableLease, input); + if (!reusableLease.providerLeaseId) throw new Error("Reusable sandbox claim lost its provider identity"); try { const resumeDeadline = Date.now() + 60_000; const configuredResumeTimeoutMs = @@ -2308,10 +2335,11 @@ function createSandboxEnvironmentDriver( ? (reusableExistingLeases.find((lease) => lease.metadata?.workFolderRecoveryRequired === true)?.providerLeaseId ?? findReusableSandboxLeaseId({ config: parsed.config, leases: reusableExistingLeases })) : null; - const reusableLease = reusableProviderLeaseId + let reusableLease = reusableProviderLeaseId ? reusableExistingLeases.find((lease) => lease.providerLeaseId === reusableProviderLeaseId) : null; + if (reusableLease) reusableLease = await claimReusableLeaseBeforeResume(reusableLease, input); let providerLease; try { if (reusableLease && (reusableLease.metadata?.workFolderRecoveryRequired === true || hasLegacySandboxWorkspace(reusableLease))) {