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 <noreply@paperclip.ing>
This commit is contained in:
parent
fdb9a4880d
commit
bc9f70f54c
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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<void>(() => {}));
|
||||
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" });
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>): 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<string, unknown>): 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<T>(
|
||||
operation: string,
|
||||
timeoutMs: number,
|
||||
run: () => Promise<T>,
|
||||
): Promise<T> {
|
||||
if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) {
|
||||
return run();
|
||||
}
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
const deadline = new Promise<never>((_, 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<void> {
|
||||
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<string> {
|
||||
|
|
@ -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.");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Reference in New Issue