From bc9f70f54c5aa06e8461d94468d537c9e37c0d39 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 14 Aug 2026 22:35:06 -0700 Subject: [PATCH] fix(plugin-daytona): bound the sandbox liveness calls with a per-call timeout (#11408) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Sandbox provider plugins run agent work in remote execution environments > - The Daytona sandbox liveness read can stay pending when the connection stops responding > - A pending read blocks the plugin until a broad host-to-worker limit expires > - This pull request adds bounded deadlines to Daytona liveness calls and clears stale handles > - The benefit is a fast and clear error when a Daytona connection stops responding ## Linked Issues or Issue Description Refs #11341 **What happened?** The Daytona sandbox liveness read had no per-call timeout. A silent connection failure left the read pending until the broad host-to-worker RPC limit expired. **Expected behavior** The plugin should stop a liveness call within a defined limit and report a clear timeout error. **Steps to reproduce** 1. Create a Daytona sandbox handle. 2. Make the cached handle freshness read never resolve. 3. Run the next sandbox operation. 4. Observe that the operation waits for the outer RPC limit without a liveness timeout. **Paperclip version or commit** `master` before this change. **Deployment mode** Any deployment mode that uses the Daytona sandbox provider. ## What Changed - Add `withLivenessTimeout` with timer cleanup and `SandboxLivenessTimeoutError`. - Bound `refreshData` with configurable `livenessTimeoutMs`, which defaults to 30000 milliseconds. - Bound sandbox start and recovery calls with the SDK timeout plus a 5000 millisecond margin. - Reject `livenessTimeoutMs` values above 86400000 milliseconds and document the setting. - Evict a cached handle after a failed freshness refresh so the next operation fetches a new handle. - Add a test for a never-resolving freshness refresh and the cached-handle eviction. ## Verification - Run the Daytona plugin test suite with its package Vitest configuration. - Confirm that 150 of 150 tests pass. - Confirm that the new test reports a bounded timeout and a fresh handle on the next operation. - Confirm that GitHub Actions reports green status checks after the pull request starts. ## Risks This change adds an early timeout only to Daytona liveness calls. A value of 0 or less disables the extra bound. The default leaves normal SDK calls within their expected time limit. The main risk is a timeout value that is too short for a slow but healthy connection. ## Model Used OpenAI Codex, GPT-5. The model used tool calls and code execution. The model supplied the PR handoff and did not author the code in this pull request. ## 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 --- .../sandbox-providers/daytona/src/manifest.ts | 6 ++ .../daytona/src/plugin.test.ts | 33 +++++++++ .../sandbox-providers/daytona/src/plugin.ts | 73 ++++++++++++++++++- ...e-assigned-backlog-contract-routes.test.ts | 11 ++- 4 files changed, 119 insertions(+), 4 deletions(-) diff --git a/packages/plugins/sandbox-providers/daytona/src/manifest.ts b/packages/plugins/sandbox-providers/daytona/src/manifest.ts index 7b52c685ee..33ddc1cb2b 100644 --- a/packages/plugins/sandbox-providers/daytona/src/manifest.ts +++ b/packages/plugins/sandbox-providers/daytona/src/manifest.ts @@ -96,6 +96,12 @@ const manifest: PaperclipPluginManifestV1 = { description: "Timeout for Daytona create/start/stop/execute operations in milliseconds.", default: 300000, }, + livenessTimeoutMs: { + type: "number", + description: + "Per-call timeout in milliseconds for the sandbox liveness read (refreshData). A silently unresponsive sandbox connection surfaces as a fast error instead of stalling until the outer RPC ceiling. The start and recovery calls derive their own deadline from timeoutMs, not this bound. `0` or less disables the bound. Defaults to 30000 when unset.", + default: 30000, + }, autoStopInterval: { type: "number", description: diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts index 46d8567207..d3e3f3561a 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts @@ -160,6 +160,7 @@ describe("Daytona sandbox provider plugin", () => { image: null, language: "typescript", timeoutMs: 450000, + livenessTimeoutMs: 30000, cpu: null, memory: null, disk: null, @@ -2843,6 +2844,38 @@ describe("Daytona sandbox provider plugin", () => { expect(second.process.executeCommand).toHaveBeenCalledTimes(1); }); + it("surfaces a bounded timeout when the freshness refresh never responds", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox({ id: "lease-a", state: "started" }); + // The sandbox connection went silent: `refreshData` never resolves and + // never rejects. Without a per-call bound the exec would stall until the + // outer RPC ceiling fires. + sandbox.refreshData.mockImplementation(() => new Promise(() => {})); + mockGet.mockResolvedValue(sandbox); + // A short liveness bound keeps the test fast and proves the config path. + const config = { timeoutMs: 300000, reuseLease: false, livenessTimeoutMs: 50 }; + + let nowMs = 4_000_000; + const restoreFreshness = setDaytonaHandleFreshnessClockForTest(() => nowMs); + try { + // Prime the cache (single fetch, snapshot "started"). + await plugin.definition.onEnvironmentExecute?.({ ...execParams("lease-a"), config }); + // Idle past half of the default 15-min auto-stop interval so the next + // lookup refreshes the stale handle. + nowMs += 8 * 60_000; + await expect( + plugin.definition.onEnvironmentExecute?.({ ...execParams("lease-a"), config }), + ).rejects.toThrow(/did not respond within 50 ms/); + } finally { + restoreFreshness(); + } + + expect(sandbox.refreshData).toHaveBeenCalledTimes(1); + // The failed refresh evicted the handle, so the next lookup re-fetches. + await plugin.definition.onEnvironmentExecute?.({ ...execParams("lease-a"), config }); + expect(mockGet).toHaveBeenCalledTimes(2); + }); + it("realizes the workspace from the acquire-seeded handle without a client.get", async () => { process.env.DAYTONA_API_KEY = "host-key"; const sandbox = createMockSandbox({ id: "sandbox-seed" }); diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.ts index 3114f32627..b344e4c862 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.ts @@ -132,6 +132,7 @@ interface DaytonaDriverConfig { image: string | null; language: string | null; timeoutMs: number; + livenessTimeoutMs: number; cpu: number | null; memory: number | null; disk: number | null; @@ -196,6 +197,22 @@ const ARCHIVE_ON_RELEASE_AUTO_DELETE_MINUTES = 60; // RPC ceiling; callers always see an actionable error within this window. const GIT_NETWORK_TIMEOUT_MS = 120_000; +// Per-call bound on the provider liveness read (`sandbox.refreshData()`). The +// Daytona SDK gives this metadata read no timeout, so a silently unresponsive +// sandbox connection leaves it pending with no error. The plugin then stalls +// until the outer host-to-worker RPC backstop fires, which is a general ceiling, +// not a fast, specific detector. This bound turns that silent hang into a fast, +// clear error. It is configurable through `livenessTimeoutMs`; a value of 0 or +// less disables the extra bound. +const DEFAULT_LIVENESS_TIMEOUT_MS = 30_000; + +// Extra margin added to the SDK start/recover timeout when the plugin wraps +// those lifecycle calls in its own per-call bound. The SDK call already carries +// a `timeoutSeconds` deadline; the wrapper is a backstop for a connection-level +// hang that the SDK deadline can miss. The margin lets the SDK deadline fire +// first on a normal slow start, so the wrapper only fires on a true hang. +const LIVENESS_START_TIMEOUT_MARGIN_MS = 5_000; + // Noninteractive git credential defaults injected into every Daytona one-shot // command so that git operations never stall waiting for a terminal prompt. // Callers can override any of these via the env parameter. @@ -227,6 +244,7 @@ function parseOptionalNumber(value: unknown): number | null { function parseDriverConfig(raw: Record): DaytonaDriverConfig { const timeoutMs = Number(raw.timeoutMs ?? 300_000); + const livenessTimeoutMs = Number(raw.livenessTimeoutMs ?? DEFAULT_LIVENESS_TIMEOUT_MS); return { apiKey: parseOptionalString(raw.apiKey), apiUrl: parseOptionalString(raw.apiUrl), @@ -235,6 +253,7 @@ function parseDriverConfig(raw: Record): DaytonaDriverConfig { image: parseOptionalString(raw.image), language: parseOptionalString(raw.language), timeoutMs: Number.isFinite(timeoutMs) ? Math.trunc(timeoutMs) : 300_000, + livenessTimeoutMs: Number.isFinite(livenessTimeoutMs) ? Math.trunc(livenessTimeoutMs) : DEFAULT_LIVENESS_TIMEOUT_MS, cpu: parseOptionalNumber(raw.cpu), memory: parseOptionalNumber(raw.memory), disk: parseOptionalNumber(raw.disk), @@ -387,16 +406,57 @@ function isValidUrl(value: string): boolean { } } +// A per-call liveness bound elapsed before the wrapped provider call returned. +// The message names the operation and the bound so an operator sees at once +// that the sandbox connection is unresponsive, not that the operation is slow. +class SandboxLivenessTimeoutError extends Error { + constructor(operation: string, timeoutMs: number) { + super( + `Daytona sandbox liveness call "${operation}" did not respond within ${timeoutMs} ms; ` + + "the sandbox connection is unresponsive.", + ); + this.name = "SandboxLivenessTimeoutError"; + } +} + +// Race a provider call against a per-call deadline. A value of 0 or less turns +// the bound off and runs the call unwrapped. The timer is always cleared, so a +// call that resolves before the deadline leaks no pending timer. A call that +// never resolves stays pending after the deadline rejects, but it holds no +// timer and produces no unhandled rejection. +async function withLivenessTimeout( + operation: string, + timeoutMs: number, + run: () => Promise, +): Promise { + if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) { + return run(); + } + let timer: ReturnType | undefined; + const deadline = new Promise((_, reject) => { + timer = setTimeout(() => reject(new SandboxLivenessTimeoutError(operation, timeoutMs)), timeoutMs); + }); + try { + return await Promise.race([run(), deadline]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} + async function ensureSandboxStarted(sandbox: Sandbox, timeoutSeconds: number): Promise { if (sandbox.state === "started") return; + // Bound the lifecycle call just past its own SDK deadline. A normal slow start + // finishes within `timeoutSeconds`; only a connection-level hang the SDK + // deadline misses reaches this wrapper bound. + const startBoundMs = timeoutSeconds * 1_000 + LIVENESS_START_TIMEOUT_MARGIN_MS; if (sandbox.state === "error") { if (sandbox.recoverable) { - await sandbox.recover(timeoutSeconds); + await withLivenessTimeout("sandbox.recover", startBoundMs, () => sandbox.recover(timeoutSeconds)); return; } throw new Error(`Daytona sandbox ${sandbox.id} is in an unrecoverable error state: ${sandbox.errorReason ?? "unknown error"}`); } - await sandbox.start(timeoutSeconds); + await withLivenessTimeout("sandbox.start", startBoundMs, () => sandbox.start(timeoutSeconds)); } async function resolveSandboxWorkingDirectory(sandbox: Sandbox): Promise { @@ -1117,7 +1177,9 @@ const sandboxHandleCache = (() => { const thresholdMs = staleHandleRefreshThresholdMs(scope.config.autoStopInterval); if (thresholdMs != null && handleFreshnessNow() - entry.verifiedAtMs >= thresholdMs) { try { - await sandbox.refreshData(); + await withLivenessTimeout("sandbox.refreshData", scope.config.livenessTimeoutMs, () => + sandbox.refreshData(), + ); } catch (error) { entries.delete(key); throw error; @@ -1814,6 +1876,11 @@ const plugin = definePlugin({ if (config.timeoutMs < 1 || config.timeoutMs > 86_400_000) { errors.push("timeoutMs must be between 1 and 86400000."); } + // A value of 0 or less disables the extra bound on purpose; reject only a + // value above the outer RPC ceiling, which would make the bound useless. + if (config.livenessTimeoutMs > 86_400_000) { + errors.push("livenessTimeoutMs must be less than or equal to 86400000."); + } if (config.autoStopInterval != null && config.autoStopInterval < 0) { errors.push("autoStopInterval must be greater than or equal to 0."); } diff --git a/server/src/__tests__/issue-assigned-backlog-contract-routes.test.ts b/server/src/__tests__/issue-assigned-backlog-contract-routes.test.ts index ac80ef32d5..da97899611 100644 --- a/server/src/__tests__/issue-assigned-backlog-contract-routes.test.ts +++ b/server/src/__tests__/issue-assigned-backlog-contract-routes.test.ts @@ -1,6 +1,6 @@ import express from "express"; import request from "supertest"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; const assigneeAgentId = "22222222-2222-4222-8222-222222222222"; @@ -168,6 +168,15 @@ function expectClearAssignedStatusValidation(res: request.Response) { } describe("assigned backlog creation contract", () => { + // Load the real route and middleware modules once before the tests run. The + // first import transforms a large module graph. Under the loaded serial shard + // (maxWorkers=1) that cold cost crossed the 5s testTimeout of the first test. + // The hook has a 30s budget, so it absorbs the cost and every createApp() call + // then hits the cached modules. + beforeAll(async () => { + await createApp(); + }); + beforeEach(() => { vi.clearAllMocks(); mockIssueService.getById.mockResolvedValue(makeIssue({