From 1a79c8bdc7df9711946bad74f8c01d4cacdeef83 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:04:23 -0500 Subject: [PATCH] fix(runtime): honor provider startup budgets on sandbox resume (#16802) ## Thinking Path > - Paperclip manages AI agents and their execution environments. > - Follow-up runs reuse saved sandbox computers to keep their workspace. > - Starting a stopped or archived computer can take more than one minute. > - Acquisition already uses the provider's startup budget, but reuse imposed a separate one-minute deadline. > - This pull request gives resume the configured or declared provider startup budget. > - The benefit is that a healthy slow start can continue on the same computer. ## Linked Issues or Issue Description **What happened?** A follow-up run failed during setup with `ReusableSandboxResumeError`. The saved computer became ready just after the host stopped waiting. The environment had no explicit timeout. Its provider declared a five-minute startup default, which the resume path ignored. **Expected behavior** Resume must honor the configured startup budget or the provider's declared default. It must retain the same lease. Failed or uncertain resumes must keep the saved computer. **Steps to reproduce** 1. Use a reusable sandbox provider with a startup budget longer than one minute. 2. Retain its lease and start a follow-up run. 3. Make provider resume finish after 61 seconds, within that budget. 4. Observe that the old host rejects at 60 seconds. **Paperclip version or commit** Reproduced on `aaeff418cf72341c674ff3393cd94f40dc4855f6`. **Deployment mode** Hosted sandbox execution. The regression uses a local isolated PostgreSQL database and controlled worker RPC timers. Searched open and closed PRs and open issues for sandbox resume and timeout fixes. No duplicate fix was found. Related acquisition timeout handling already exists on the base branch. ## What Changed - Use the provider's declared acquisition timeout when no explicit startup timeout exists. - Apply the resolved RPC budget to the shared resume/retry deadline instead of 60 seconds. - Add two deterministic 61-second resume regressions for declared and explicit startup budgets. Assert exact RPC budgets, including the 30-second margin, and check that the same lease remains active without replacement. - Document the timeout behavior, provider-author contract, and verification recipe. ## Verification - Red: both new cases failed before the production change with `ReusableSandboxResumeError`, caused by `Plugin RPC timed out`. - Green: `pnpm exec vitest run --fsModuleCache server/src/__tests__/environment-runtime.test.ts` passed all 163 cases. The final review follow-up also passed both slow-resume cases with exact RPC-budget assertions (330 seconds by declaration; 150 seconds by explicit config). - An earlier full run passed the 161 existing cases but timed out in the two new tests because their fake clock also captured database scheduling. The corrected test restores real timers before lease persistence. The complete rerun then passed. - `node scripts/check-maps.mjs` passed. - `pnpm -r typecheck` and `pnpm build` passed locally. - Started `pnpm test:run`, then stopped the duplicate serial run in favor of complete parallel PR CI. The repository timing data totals about 126 minutes for server suites alone. Full parallel CI passed on the final head. - CI attempt 1 had one unrelated failure: `FileViewerSheet.copy.test.tsx` expected the asynchronous `Copied link` confirmation before React rendered it. That test and component are unchanged from the base branch. All six cases passed unchanged locally. A single rerun of the failed shard passed; attempt 2 of the full workflow succeeded. The timing-race diagnosis is an inference from the test helper, not a claim that this PR repairs that UI test. - Greptile scored the final head 5/5. Both review findings are fixed and both threads are resolved. - The consequential assumption is that provider startup may exceed one minute. The test preserves the RPC timeout race with controlled timers and uses real lease persistence. Read-only provider evidence also showed the retained computer ready just after the failed run ended. The patch has not been deployed to that instance. ## Risks - A slow or unavailable provider may hold setup for its configured startup budget instead of one minute. Retries still share one finite deadline. Providers with no explicit or declared timeout keep the one-minute default. - Lease identity, company scope, backup validation, and replacement decisions use the existing paths. No schema or dependency changes are required. ## Model Used - OpenAI Codex, GPT-6. This session supports reasoning, tool use, code editing, and test execution. The exact runtime variant and context-window size are not exposed. ## 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 --- doc/execution-semantics.md | 7 +++ doc/plugins/SANDBOX_PROVIDER_CAPABILITIES.md | 14 +++-- maps/feature-map/execution-environments.md | 6 ++ .../src/__tests__/environment-runtime.test.ts | 61 +++++++++++++++++-- server/src/services/environment-runtime.ts | 7 ++- 5 files changed, 83 insertions(+), 12 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 46492a0df5f..5593fd17f38 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1563,6 +1563,13 @@ configuration, and controller-owned credential or instruction resources use verified suspension and saved-history resume. This recovery can restart a provider; it does not suspend every healthy warm turn. +Plugin sandbox reuse during acquisition uses the configured startup timeout or +the provider's declared acquisition default, with the host RPC margin. Resume +attempts and transient retries share that deadline. Providers without either +timeout retain the one-minute host default. A resume failure still preserves +the recorded sandbox unless the provider proves it lost and replacement passes +the existing backup checks. + For the native runner, warm mode requests a reusable sandbox lease **before** lease acquisition. The environment's explicit runner lifecycle overrides the agent default; an inherited lifecycle uses the current agent setting on each diff --git a/doc/plugins/SANDBOX_PROVIDER_CAPABILITIES.md b/doc/plugins/SANDBOX_PROVIDER_CAPABILITIES.md index c3988f98f11..5c8eb927d2a 100644 --- a/doc/plugins/SANDBOX_PROVIDER_CAPABILITIES.md +++ b/doc/plugins/SANDBOX_PROVIDER_CAPABILITIES.md @@ -28,10 +28,16 @@ Drivers that omit the declaration keep the existing fallback. The host does not infer this budget from config-schema defaults: a field named `timeoutMs` can describe sandbox lifetime instead of the time needed to acquire it. -This declaration does not change provider config, lease expiry, the resume -deadline, or other lifecycle calls. Providers must still enforce their operation -timeouts and report uncertain allocations for cleanup. A bundled plugin must bump -its manifest version when adding the field so existing installations receive it. +For reusable sandbox acquisition, the host also uses this budget for +`environmentResumeLease`, with the same explicit-config precedence, bridge +extension, and RPC margin. Resume attempts and transient retries share one +deadline. Without a configured or declared budget, resume keeps its one-minute +host default. Generic plugin-driver resume keeps its existing behavior. + +This declaration does not change provider config, lease expiry, or other +lifecycle calls. Providers must still enforce their operation timeouts and +report uncertain allocations for cleanup. A bundled plugin must bump its +manifest version when adding the field so existing installations receive it. ## How the host resolves an effective capability diff --git a/maps/feature-map/execution-environments.md b/maps/feature-map/execution-environments.md index 00d59b7dcb8..6beb90d4a26 100644 --- a/maps/feature-map/execution-environments.md +++ b/maps/feature-map/execution-environments.md @@ -77,6 +77,12 @@ execution. Remote-driver probes and a real bounded run need their own evidence. Automated: [execution target](../../server/src/__tests__/environment-execution-target.test.ts) and [custom images](../../server/src/__tests__/environment-custom-images-service.test.ts) cover contracts; [SSH live test](../../server/src/__tests__/environment-live-ssh.test.ts) requires its own external setup. +The [environment runtime regression](../../server/src/__tests__/environment-runtime.test.ts) +also covers sandbox reuse after a start longer than one minute. Acquisition +uses the configured or provider-declared startup budget for resume and shares +one deadline across retries. Failure preserves the recorded sandbox. This +controlled timing check does not establish live provider acceptance. + Manual: Run a bounded command in the intended target, inspect its output and workspace, and verify execution did not silently fall back to the server host. For custom-image changes, build/probe the disposable image before assigning it. Inspect cleanup and shutdown after the run. ## Gotchas diff --git a/server/src/__tests__/environment-runtime.test.ts b/server/src/__tests__/environment-runtime.test.ts index 1385e9167e6..d85b6d8a19e 100644 --- a/server/src/__tests__/environment-runtime.test.ts +++ b/server/src/__tests__/environment-runtime.test.ts @@ -567,13 +567,16 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { }; } - async function seedReusablePluginSandboxLease(adapterType: string | null = null) { + async function seedReusablePluginSandboxLease( + adapterType: string | null = null, + options: { timeoutMs?: number | null; defaultAcquireTimeoutMs?: number } = {}, + ) { const pluginId = randomUUID(); const { companyId, agentId, environment: baseEnvironment, runId } = await seedEnvironment(); const providerConfig = { provider: "fake-plugin", image: "fake:test", - timeoutMs: 1234, + ...(options.timeoutMs === null ? {} : { timeoutMs: options.timeoutMs ?? 1234 }), reuseLease: true, }; const environment = { @@ -598,6 +601,7 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { kind: "sandbox_provider", displayName: "Fake Plugin", supportsReusableLeases: true, + defaultAcquireTimeoutMs: options.defaultAcquireTimeoutMs, configSchema: { type: "object", properties: { @@ -645,10 +649,7 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { pluginId, pluginKey: "acme.reusable-sandbox-provider", sandboxProviderPlugin: true, - provider: "fake-plugin", - image: "fake:test", - timeoutMs: 1234, - reuseLease: true, + ...providerConfig, reusableSandboxLease: { version: 1, companyId, @@ -4618,6 +4619,54 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { ); }); + it.each([ + { label: "declared provider startup budget", timeoutMs: null, defaultAcquireTimeoutMs: 300_000, expectedRpcTimeoutMs: 330_000 }, + { label: "explicit startup budget", timeoutMs: 120_000, defaultAcquireTimeoutMs: 300_000, expectedRpcTimeoutMs: 150_000 }, + ])("resumes the same sandbox after a slow start within its $label", async (options) => { + const seeded = await seedReusablePluginSandboxLease("paperclip_runner", options); + let signalResumeStarted!: () => void; + const resumeStarted = new Promise(resolve => { signalResumeStarted = resolve; }); + const call = vi.fn(async (_pluginId: string, method: string, _params: unknown, rpcTimeoutMs: number) => { + if (method !== "environmentResumeLease") throw new Error(`Unexpected plugin method: ${method}`); + vi.useFakeTimers({ toFake: ["Date", "setTimeout", "clearTimeout"] }); + const response = new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("Plugin RPC timed out")); + }, rpcTimeoutMs); + setTimeout(() => { + clearTimeout(timeout); + vi.useRealTimers(); + resolve({ providerLeaseId: seeded.reusableLease.providerLeaseId, metadata: { remoteCwd: "/workspace" } }); + }, 61_000); + }); + signalResumeStarted(); + return await response; + }); + const runtime = environmentRuntimeService(db, { pluginWorkerManager: { + isRunning: () => true, call, + getWorker: () => ({ supportedMethods: ["environmentResumeLease", "environmentReleaseLease", "environmentDestroyLease"] }), + } as unknown as PluginWorkerManager }); + const now = vi.spyOn(Date, "now").mockReturnValue(Date.now()); + const acquisition = runtime.acquireRunLease({ + companyId: seeded.companyId, environment: seeded.environment, + issueId: null, agentId: seeded.agentId, adapterType: "paperclip_runner", heartbeatRunId: seeded.runId, + persistedExecutionWorkspace: { id: seeded.executionWorkspaceId, mode: "shared_workspace" }, + }).then(value => ({ value, error: null }), error => ({ value: null, error })); + try { + await resumeStarted; + await vi.advanceTimersByTimeAsync(61_000); + } finally { + vi.useRealTimers(); + now.mockRestore(); + } + const result = await acquisition; + expect(result.error).toBeNull(); + expect(result.value?.lease).toMatchObject({ id: seeded.reusableLease.id, providerLeaseId: seeded.reusableLease.providerLeaseId }); + expect(call).toHaveBeenCalledOnce(); + expect(call.mock.calls[0]?.[3]).toBe(options.expectedRpcTimeoutMs); + expect(await environmentService(db).getLeaseById(seeded.reusableLease.id)).toMatchObject({ status: "active", heartbeatRunId: seeded.runId }); + }); + it("preserves the exact lease when plugin-backed sandbox resume throws", async () => { const pluginId = randomUUID(); const { companyId, agentId, environment: baseEnvironment, runId } = await seedEnvironment(); diff --git a/server/src/services/environment-runtime.ts b/server/src/services/environment-runtime.ts index a7c2f0098af..e737779a283 100644 --- a/server/src/services/environment-runtime.ts +++ b/server/src/services/environment-runtime.ts @@ -2018,9 +2018,12 @@ function createSandboxEnvironmentDriver( }); } try { - const resumeDeadline = Date.now() + 60_000; const configuredResumeTimeoutMs = - resolvePluginSandboxRpcTimeoutMs(workerConfig) ?? 60_000; + resolvePluginSandboxRpcTimeoutMs( + workerConfig, + pluginProvider.resolved.driver.defaultAcquireTimeoutMs, + ) ?? 60_000; + const resumeDeadline = Date.now() + configuredResumeTimeoutMs; let retryDelayMs = 250; let resumed: PluginEnvironmentLease; while (true) {