From 5c5366d0c181ec4a047be8c07c6b32d711571979 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 29 Jul 2026 14:27:23 -0700 Subject: [PATCH] fix(server): honor explicit plugin RPC timeouts (#10460) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Plugin workers connect Paperclip to external runtimes and sandbox providers. > - Some adapter heartbeats run a full sandbox session inside one `environmentExecute` RPC. > - The worker manager limited every RPC timeout to 15 minutes, even when the caller gave a longer timeout. > - This pull request keeps the normal default timeout behavior but honors explicit caller timeouts. > - The benefit is that long sandboxed agent sessions can continue past 15 minutes while other safety guards still bound hung work. ## Linked Issues or Issue Description No public GitHub issue exists for this bug. ### Bug Report Pre-submission checklist: - Searched existing open and closed issues and did not find a duplicate. - Confirmed the bug is reproducible on `master` from the current source tree. - Confirmed the error starts in Paperclip timeout handling, not in an adapter provider or local configuration. What happened? - A sandbox-backed adapter heartbeat can run a full agent session inside one `environmentExecute` plugin RPC. - The plugin worker manager capped every RPC timeout at 15 minutes. - The cap also applied when the caller passed a longer explicit timeout for an execute-style call. - A long sandbox command could fail before the adapter budget expired. Expected behavior: - Ordinary plugin RPC calls should keep the normal 30-second default timeout. - The default timeout path should still have a 15-minute maximum. - A caller-supplied positive finite timeout should be honored, including values above 15 minutes. Steps to reproduce: 1. Use a plugin environment driver that calls `environmentExecute` with an explicit timeout above 15 minutes. 2. Run a command that stays active longer than 15 minutes and remains inside the adapter budget. 3. Observe that the worker manager times out the RPC at 15 minutes before this fix. 4. Run the same path after this fix and observe that the explicit timeout is used. Paperclip version or commit: - Reproduced from the current `master` line before this change. Deployment mode: - Local dev or self-hosted server with sandbox-backed execution. Installation method: - Built from source. Agent adapter(s) involved: - Codex. - Custom or external plugin adapter. - Core plugin worker timeout handling. Database mode: - Not database-related. Access context: - Agent execution context. Node.js version: - Not version-specific. Operating system: - Not OS-specific. Relevant logs or output: ```shell RPC call "environmentExecute" timed out after 900000ms ``` Relevant config: - Not config-related. Additional context: - Execute-style sandbox calls already have adapter inactivity monitors, platform silent-run checks, and provider command timeouts. This PR removes the unintended worker-manager clamp only for explicit positive finite caller timeouts. Privacy checklist: - Reviewed all pasted output for PII, user paths, API keys, tokens, company names, and internal instance links. Duplicate search: - Searched open PRs and open issues in `paperclipai/paperclip` for `environmentExecute timeout`, `MAX_RPC_TIMEOUT_MS`, and `plugin-worker-manager timeout`. - Searched the same terms in `HenkDz/paperclip`. - Found no matching open PRs or issues. - Compared this patch-id against my open PRs in `paperclipai/paperclip`; no match was found. ## What Changed - Added `resolveRpcCallTimeoutMs()` to keep explicit positive finite timeouts intact. - Kept the 15-minute maximum only for the default timeout path. - Updated `callInternal()` to use the new resolver. - Added unit tests for explicit long timeouts, default timeout clamping, fractional values, and invalid explicit values. - Clarified why notification invocation scopes still use the 15-minute TTL. ## Verification - `corepack pnpm install --frozen-lockfile` - `corepack pnpm --filter @paperclipai/plugin-sdk ensure-build-deps` - `corepack pnpm --filter @paperclipai/server exec vitest run src/__tests__/plugin-worker-manager.test.ts` - `corepack pnpm --filter @paperclipai/server exec tsc --noEmit` - `git diff --check 9574cad3e85a641ef6314843676d8b8dd75848f9 HEAD` - GitHub PR checks on `bf7bfd0d`: all passing; Storybook visual regression skipped by workflow. - Greptile on `bf7bfd0d`: 5/5, no blocking failure remains; prior P2 thread resolved. ## Risks Low risk. The change affects RPC timeout resolution in the plugin worker manager. Ordinary plugin calls still use the 30-second default and the default path is still capped at 15 minutes. Callers that pass explicit long timeouts now own that budget. Adapter inactivity monitors and platform silent-run safety checks still bound hung runs. ## Model Used OpenAI Codex, GPT-5-based coding agent, with shell and GitHub CLI tool use. ## 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 --- .../__tests__/plugin-worker-manager.test.ts | 51 +++++++++++++++++++ server/src/services/plugin-worker-manager.ts | 45 +++++++++++++++- 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/plugin-worker-manager.test.ts b/server/src/__tests__/plugin-worker-manager.test.ts index 216313a7d0..451e7d1c60 100644 --- a/server/src/__tests__/plugin-worker-manager.test.ts +++ b/server/src/__tests__/plugin-worker-manager.test.ts @@ -13,6 +13,7 @@ import { appendStderrExcerpt, createPluginWorkerHandle, formatWorkerFailureMessage, + resolveRpcCallTimeoutMs, } from "../services/plugin-worker-manager.js"; const FIXTURES_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), "fixtures"); @@ -35,6 +36,56 @@ const TEST_MANIFEST: PaperclipPluginManifestV1 = { entrypoints: { worker: "dist/worker.js" }, }; +describe("resolveRpcCallTimeoutMs", () => { + const MAX_RPC_TIMEOUT_MS = 15 * 60 * 1_000; + const MAX_NODE_TIMER_TIMEOUT_MS = 2_147_483_647; + const DEFAULT_RPC_TIMEOUT_MS = 30_000; + + it("honors an explicit timeout above the 15-minute default ceiling", () => { + // The sandbox environment driver requests ~4h + 30s buffer for + // environmentExecute; this must not be clamped to 15 minutes. + const fourHoursPlusBuffer = 4 * 60 * 60 * 1_000 + 30_000; + expect(resolveRpcCallTimeoutMs(fourHoursPlusBuffer, DEFAULT_RPC_TIMEOUT_MS)).toBe( + fourHoursPlusBuffer, + ); + }); + + it("honors an explicit timeout below the ceiling", () => { + expect(resolveRpcCallTimeoutMs(100, DEFAULT_RPC_TIMEOUT_MS)).toBe(100); + expect(resolveRpcCallTimeoutMs(MAX_RPC_TIMEOUT_MS - 1, DEFAULT_RPC_TIMEOUT_MS)).toBe( + MAX_RPC_TIMEOUT_MS - 1, + ); + }); + + it("truncates fractional explicit timeouts", () => { + expect(resolveRpcCallTimeoutMs(1_000.9, DEFAULT_RPC_TIMEOUT_MS)).toBe(1_000); + }); + + it("normalizes explicit timeouts to Node's timer-safe range", () => { + expect(resolveRpcCallTimeoutMs(0.5, DEFAULT_RPC_TIMEOUT_MS)).toBe(1); + expect(resolveRpcCallTimeoutMs(MAX_NODE_TIMER_TIMEOUT_MS + 1, DEFAULT_RPC_TIMEOUT_MS)).toBe( + MAX_NODE_TIMER_TIMEOUT_MS, + ); + }); + + it("uses the default timeout when no explicit timeout is provided", () => { + expect(resolveRpcCallTimeoutMs(undefined, DEFAULT_RPC_TIMEOUT_MS)).toBe( + DEFAULT_RPC_TIMEOUT_MS, + ); + }); + + it("clamps only the default path to the 15-minute ceiling", () => { + expect(resolveRpcCallTimeoutMs(undefined, 24 * 60 * 60 * 1_000)).toBe(MAX_RPC_TIMEOUT_MS); + }); + + it("falls back to the clamped default for unusable explicit timeouts", () => { + for (const bad of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(resolveRpcCallTimeoutMs(bad, DEFAULT_RPC_TIMEOUT_MS)).toBe(DEFAULT_RPC_TIMEOUT_MS); + } + expect(resolveRpcCallTimeoutMs(Number.NaN, 24 * 60 * 60 * 1_000)).toBe(MAX_RPC_TIMEOUT_MS); + }); +}); + describe("plugin-worker-manager stderr failure context", () => { it("appends worker stderr context to failure messages", () => { expect( diff --git a/server/src/services/plugin-worker-manager.ts b/server/src/services/plugin-worker-manager.ts index 25e6882a67..af2c6b78ba 100644 --- a/server/src/services/plugin-worker-manager.ts +++ b/server/src/services/plugin-worker-manager.ts @@ -61,9 +61,23 @@ import { logger } from "../middleware/logger.js"; /** Default timeout for RPC calls in milliseconds. */ const DEFAULT_RPC_TIMEOUT_MS = 30_000; -/** Hard upper bound for any RPC timeout (15 minutes). Prevents unbounded waits. */ +/** + * Upper bound for the *default* RPC timeout path (15 minutes). Explicit + * caller-supplied timeouts are not subject to this cap: execute-class RPCs such + * as `environmentExecute` run entire sandboxed agent sessions in one call and + * their callers deliberately request multi-hour budgets (see + * `resolvePluginExecuteRpcTimeoutMs` in plugin-environment-driver.ts). + * Clamping those explicit budgets here killed long sandboxed runs mid-work. + */ const MAX_RPC_TIMEOUT_MS = 15 * 60 * 1_000; +/** + * Maximum delay accepted by Node timers before Node clamps the timeout to 1ms. + * Keep accepted explicit RPC budgets inside this range before calling + * setTimeout, otherwise a huge timeout can expire almost immediately. + */ +const MAX_NODE_TIMER_TIMEOUT_MS = 2_147_483_647; + /** Timeout for the initialize RPC call. */ const INITIALIZE_TIMEOUT_MS = 15_000; @@ -154,6 +168,30 @@ export function formatWorkerFailureMessage(message: string, stderrExcerpt: strin return `${message}\n\nWorker stderr:\n${excerpt}`; } +/** + * Resolve the effective timeout for an RPC call. + * + * An explicit, positive, finite caller-supplied timeout bypasses the 15-minute + * RPC cap after normalization to Node's timer-safe integer range. Callers that + * pass one (e.g. the environment driver for `environmentExecute`) own their + * budget, and independent inactivity/safety guards bound hung runs. Only the + * default path (no usable explicit timeout) is clamped to MAX_RPC_TIMEOUT_MS so + * ordinary plugin calls stay bounded. + */ +export function resolveRpcCallTimeoutMs( + explicitTimeoutMs: number | undefined, + defaultTimeoutMs: number, +): number { + if ( + explicitTimeoutMs !== undefined && + Number.isFinite(explicitTimeoutMs) && + explicitTimeoutMs > 0 + ) { + return Math.min(Math.max(Math.trunc(explicitTimeoutMs), 1), MAX_NODE_TIMER_TIMEOUT_MS); + } + return Math.min(defaultTimeoutMs, MAX_RPC_TIMEOUT_MS); +} + /** * Options for starting a worker process. */ @@ -1232,7 +1270,7 @@ export function createPluginWorkerHandle( } const id = nextRequestId++; - const timeout = Math.min(timeoutMs ?? rpcTimeoutMs, MAX_RPC_TIMEOUT_MS); + const timeout = resolveRpcCallTimeoutMs(timeoutMs, rpcTimeoutMs); const invocationScope = deriveInvocationScope(method, params); const invocation = invocationScope ? registerInvocation(invocationScope) : null; @@ -1357,6 +1395,9 @@ export function createPluginWorkerHandle( notify(method: string, params: unknown) { if (status !== "running") return; const invocationScope = deriveInvocationScope(method, params); + // Notifications have no response to settle on, so the invocation scope + // is GC'd by TTL. Call-path invocations are registered without a TTL and + // cleared on settlement, so they survive arbitrarily long call timeouts. const invocation = invocationScope ? registerInvocation(invocationScope, MAX_RPC_TIMEOUT_MS) : null; try { sendMessage({