From 75666224d43aea75a59e8895a3b904d9402be0ce Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:50:49 -0500 Subject: [PATCH] fix(native): use the deadline for remote health retries (#18324) ## Thinking Path > - Paperclip manages agent work and its execution state. > - Remote Runner processes use health reads to check their saved process identity. > - A failed health request cannot prove that the process stopped. > - Three fast gateway errors can exhaust the old retry limit in half a second. > - The caller already gives these reads a shared 30-second recovery budget. > - This pull request uses that budget with backoff for read-only failures. > - A short gateway outage can then clear without stopping the existing turn. ## Linked Issues or Issue Description **What happened?** A transient HTTP 502 during sandbox process supervision caused an active native Codex turn to fail. The probe stopped after three attempts, even when most of its 30-second budget remained. The run reported `notification_transport_failed` and required workspace recovery. **Expected behavior** Retry read-only transport failures within the existing deadline. Keep process identity conflicts authoritative. Do not repeat a signal after a lost acknowledgement. **Steps to reproduce** 1. Start a read-only process verification with a 30-second deadline. 2. Return HTTP 502 from its first three transport reads, then allow the next read to succeed. 3. The old helper rejects after three reads. The fixed helper returns the successful observation. Related repairs: #16451 introduced bounded remote probes. #16835 and #17799 require proof of process exit before replacement. This change retains those guards. ## What Changed - Use the existing absolute deadline instead of a three-attempt limit for read-only probe failures. - Back off from 250 milliseconds to two seconds. Clip each request to ten seconds or the remaining budget. - Preserve the original error and diagnostic when the deadline is exhausted. Reject non-finite deadlines before an RPC. - Add a red/green regression and a repeated-gateway-failure case around a real listening process. - Update execution semantics and both recovery maps. ## Verification - Before the fix: the new probe suite failed two behavioral assertions. The gateway case returned its HTTP 502 error instead of the later successful observation. - After the fix: 75 focused probe, diagnostic, process-supervision and adoption tests pass. The diagnostic failure tests use a controlled clock for their full 30-second budget. Their error and redaction assertions stay unchanged. - Map alignment and module-boundary checks pass. - Full workspace typecheck and build pass at final commit `ddd42f66378ebc36ec1d7e0ca1f45943473561b3`. - The broader affected suite against disposable PostgreSQL passed 994 of 997 tests; its three startup/database timeouts passed unchanged in isolation. The initial full default local run encountered embedded PostgreSQL startup failures and did not complete. The four diagnostic tests that assumed the old short retry timing now use a controlled clock without weakening their assertions. - Incident evidence came from Linux AMD64, Node 24.22.2, Core commit `0c70cedce9a803bdc201324528abb35e423e7cd0`, native Codex `gpt-6.1-sol`, and a shared ChatGPT subscription. Two separate runs recorded the same supervision HTTP 502 within one second. - A disposable live Daytona sandbox check passed on Linux x86_64 and Node 24.22.1. Three injected gateway failures recovered in 3.31 seconds on production source from `e54a355fa179022caaa6854302ff56bd85de0229` (unchanged by the final test-only commit). The real Node listening process had one launch, retained its exact identity and saved file across a second read, and produced a confirmed Stop receipt. Sandbox deletion succeeded. This is process-supervision integration evidence, not a Codex task or model execution. - Fixed-revision hosted acceptance and recovery of the original conversation require rollout. This PR does not claim either. The duration of the original provider outage remains unknown. - Final commit `ddd42f66378ebc36ec1d7e0ca1f45943473561b3`: required `ci / verify` and `ci / e2e` checks pass; Greptile is 5/5 with no inline findings. The separate Paperclip Review service has not completed an assessment and remains queued. It is not a required branch check. ## Risks - Persistent fast transport failures cause more read-only requests within the same deadline. Capped backoff limits their rate. - Missing or changed identity still returns immediately. Failed health reads still cannot authorize replacement, process signals, or provider replay. - Signal requests still have one attempt. The timeout budget and execution permissions do not change. - No schema or package changes. ## Model Used - OpenAI GPT-6, through Codex, with reasoning, repository inspection, and shell tool use. The exact deployment model ID and context-window size are not exposed by this session. ## 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 ++- maps/agent-access-map/recovery.md | 3 +- maps/feature-map/recovery.md | 6 ++- .../native-session-executor.test.ts | 24 +++++++--- .../remote-runner-recovery-probe.test.ts | 46 +++++++++++++++++++ .../remote-runner-recovery-probe.ts | 12 +++-- 6 files changed, 83 insertions(+), 15 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 1df952eea59..c59bab4b3f6 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1532,8 +1532,11 @@ Every continuation carries the triggering request, ordered user direction, inter ### Remote runner process evidence A failed remote health request does not prove that runnerd stopped. Read-only -process checks retry transient transport failures up to three times within one -30-second budget. Missing or changed identity, an unknown process generation, +process checks retry transient transport failures within one 30-second budget, +with backoff from 250 milliseconds to two seconds between failed reads. Fast +failures do not exhaust that budget after three attempts. Each read uses at +most ten seconds of the remaining budget. Signals still have one attempt when +their acknowledgement is lost. Missing or changed identity, an unknown process generation, and exhausted health reads retain uncertainty. They do not authorize a signal, replacement launch, or provider replay. Confirmed predecessor exit permits the transport to attempt recovery within its existing reconnect deadline. Repeated diff --git a/maps/agent-access-map/recovery.md b/maps/agent-access-map/recovery.md index e18533217d4..0624f02976e 100644 --- a/maps/agent-access-map/recovery.md +++ b/maps/agent-access-map/recovery.md @@ -17,7 +17,8 @@ point, and still requires the caller's exact stopped-source and workspace fences Provider transfer tests do not establish task admission or agent authority. Remote process probes remain server-owned. Confirmed exit requires `ESRCH` or a -zombie with the saved generation. Unknown adopted-process ownership rejects the +zombie with the saved generation. Read-only transport retries use the existing +shared deadline with backoff; signal requests remain single-attempt. Unknown adopted-process ownership rejects the probe and grants no permission to replace a runner, signal a different process, or replay work. Launch requires a working Node probe before detachment. diff --git a/maps/feature-map/recovery.md b/maps/feature-map/recovery.md index 00983d8aeb8..071404c8b83 100644 --- a/maps/feature-map/recovery.md +++ b/maps/feature-map/recovery.md @@ -287,8 +287,10 @@ historical provider outage or successful hosted task recovery. Active remote process supervision distinguishes a failed health read from a confirmed exit. The [executor tests](../../server/src/services/native-runtime/native-session-executor.test.ts) -exercise timeout recovery with a real listening process, unchanged ownership, -and rejected identity evidence. The Runner restart tests require positive exit +exercise timeout and repeated gateway-failure recovery with a real listening +process, unchanged ownership, and rejected identity evidence. Read-only probes +use their shared deadline with bounded backoff; fast failures do not exhaust a +three-attempt limit. Signal requests remain single-attempt. The Runner restart tests require positive exit evidence before replacement. The executor regressions also retire a real child during its metadata read, keep unknown adopted-process ownership held, and reject launch before detachment when Node is unavailable. The diagnostic tests cover diff --git a/server/src/services/native-runtime/native-session-executor.test.ts b/server/src/services/native-runtime/native-session-executor.test.ts index a6f6ff86327..b7775c11fda 100644 --- a/server/src/services/native-runtime/native-session-executor.test.ts +++ b/server/src/services/native-runtime/native-session-executor.test.ts @@ -692,6 +692,8 @@ describe("remote controller restart adoption", () => { remoteRunnerRecovery: { stage: "identity_marker", reason } }); }); it.each(["timeout", "transport"] as const)("records a marker %s without retaining transport text", async failure => { + vi.useFakeTimers(); + onTestFinished(() => { vi.useRealTimers(); }); const { target, execute } = fixture(); const transportError = new Error("private bridge response"); const original = execute.getMockImplementation()!; @@ -700,7 +702,9 @@ describe("remote controller restart adoption", () => { if (failure === "transport") throw transportError; return { stdout: "", stderr: "private bridge response", exitCode: -1, timedOut: true }; }); - const error = await verifyRemoteRunnerReattachment({ claim, target: target as never, identity, runId: "run", normalizedSessionId: "session" }).catch(error => error); + const outcome = verifyRemoteRunnerReattachment({ claim, target: target as never, identity, runId: "run", normalizedSessionId: "session" }).catch(error => error); + await vi.advanceTimersByTimeAsync(30_000); + const error = await outcome; const diagnostic = readRemoteRunnerRecoveryError(error); expect(diagnostic).toEqual({ stage: "identity_marker", reason: failure === "timeout" ? "probe_timeout" : "probe_transport_failed" }); expect(JSON.stringify(diagnostic)).not.toContain("private"); @@ -752,6 +756,8 @@ describe("remote controller restart adoption", () => { } finally { await rm(root, { recursive: true, force: true }); } }); it.each(["liveness", "timeout", "transport"] as const)("retains the reason for a failed process %s probe", async failure => { + vi.useFakeTimers(); + onTestFinished(() => { vi.useRealTimers(); }); const { target, execute } = fixture(); const original = execute.getMockImplementation()!; const transportError = new Error("provider_transport_failed: private bridge response"); @@ -760,7 +766,9 @@ describe("remote controller restart adoption", () => { if (failure === "transport") throw transportError; return { stdout: "", stderr: "private probe output", exitCode: failure === "liveness" ? 45 : -1, timedOut: failure === "timeout" }; }); - const error = await verifyRemoteRunnerReattachment({ claim, target: target as never, identity, runId: "run", normalizedSessionId: "session" }).catch(error => error); + const outcome = verifyRemoteRunnerReattachment({ claim, target: target as never, identity, runId: "run", normalizedSessionId: "session" }).catch(error => error); + await vi.advanceTimersByTimeAsync(30_000); + const error = await outcome; const diagnostic = readRemoteRunnerRecoveryError(error); expect(diagnostic).toEqual({ stage: "process_verification", reason: failure === "liveness" ? "process_not_running" : failure === "timeout" ? "probe_timeout" : "probe_transport_failed" }); expect(JSON.stringify(diagnostic)).not.toContain("private"); @@ -1046,7 +1054,7 @@ fs.readFileSync = function(path, ...args) { } }, 15_000); - it("preserves a real listening process and its marker when a health RPC times out", async () => { + it.each(["timeout", "gateway"] as const)("preserves a real listening process and its marker through a %s health outage", async failure => { const root = await mkdtemp(join(tmpdir(), "runner-health-listener-")); const marker = join(root, "identity"); const fixture = join(root, "listener.cjs"); @@ -1054,6 +1062,7 @@ fs.readFileSync = function(path, ...args) { let child: ReturnType | undefined; let probes = 0; let healthyProbes = 0; + const failedProbes = failure === "gateway" ? 3 : 1; let releaseExitProbe!: () => void; const exitProbeReady = new Promise(resolve => { releaseExitProbe = resolve; }); let settled = false; @@ -1070,8 +1079,11 @@ fs.readFileSync = function(path, ...args) { await writeFile(marker, `${input.args![4]}\n${child.pid}\n2026-10-19T00:00:00.000Z\nrunner-health\n`); return ok; } - if (label === "paperclip-runner-monitor" && ++probes === 1) return { ...ok, exitCode: null, timedOut: true }; - if (label === "paperclip-runner-monitor" && probes > 2) await exitProbeReady; + if (label === "paperclip-runner-monitor" && ++probes <= failedProbes) { + if (failure === "gateway") throw new Error("Request failed with status code 502"); + return { ...ok, exitCode: null, timedOut: true }; + } + if (label === "paperclip-runner-monitor" && probes > failedProbes + 1) await exitProbeReady; const observed = await new Promise<{ exitCode: number | null; signal: null; timedOut: boolean; stdout: string; stderr: string }>(resolve => { execFile(input.command, input.args ?? [], { timeout: 2_000 }, (error, stdout, stderr) => resolve({ exitCode: error ? typeof error.code === "number" ? error.code : null : 0, @@ -1189,7 +1201,7 @@ fs.readFileSync = function(path, ...args) { expect(await failure).toEqual({ stage: "process_verification", reason: "probe_timeout" }); expect(handle.isExitConfirmed?.()).toBe(false); const probes = execute.mock.calls.filter(([x]) => x.args?.[2] === "paperclip-runner-monitor"); - expect(probes.map(([x]) => x.timeoutMs)).toEqual([10_000, 10_000, 9_500]); + expect(probes.map(([x]) => x.timeoutMs)).toEqual([10_000, 10_000, 9_250]); expect(execute.mock.calls.filter(([x]) => x.args?.[2] === "paperclip-runner-launch")).toHaveLength(1); } finally { vi.useRealTimers(); } }); diff --git a/server/src/services/native-runtime/remote-runner-recovery-probe.test.ts b/server/src/services/native-runtime/remote-runner-recovery-probe.test.ts index fe25cf3296d..d8c31383bc8 100644 --- a/server/src/services/native-runtime/remote-runner-recovery-probe.test.ts +++ b/server/src/services/native-runtime/remote-runner-recovery-probe.test.ts @@ -7,6 +7,52 @@ afterEach(() => vi.useRealTimers()); const timeout = { exitCode: null, signal: null, timedOut: true, stdout: "", stderr: "", pid: null, startedAt: "2026-10-15T00:00:00Z" }; +it.each([NaN, Infinity, -Infinity])("rejects a non-finite deadline (%s) before a probe", async deadline => { + const execute = vi.fn().mockResolvedValue({ ...timeout, timedOut: false, exitCode: 0 }); + const result = await readRemoteRunnerRecoveryProbe({ + runner: { execute }, request: { command: "sh" }, stage: "process_verification", + readOnly: true, deadline, + }).catch(readRemoteRunnerRecoveryError); + expect(result).toEqual({ stage: "process_verification", reason: "probe_timeout" }); + expect(execute).not.toHaveBeenCalled(); +}); + +it("uses the read deadline when a transport outage outlasts three fast probes", async () => { + vi.useFakeTimers(); + const unavailable = Object.assign(new Error("Request failed with status code 502"), { response: { status: 502 } }); + const healthy = { ...timeout, timedOut: false, exitCode: 0 }; + const execute = vi.fn() + .mockRejectedValueOnce(unavailable) + .mockRejectedValueOnce(unavailable) + .mockRejectedValueOnce(unavailable) + .mockResolvedValue(healthy); + const result = readRemoteRunnerRecoveryProbe({ + runner: { execute }, request: { command: "sh" }, stage: "process_verification", + readOnly: true, deadline: Date.now() + 30_000, + }).catch(error => error); + await vi.runAllTimersAsync(); + expect(await result).toBe(healthy); + expect(execute).toHaveBeenCalledTimes(4); +}); + +it("bounds fast transport failures by the existing deadline and retains the cause", async () => { + vi.useFakeTimers(); + const unavailable = new Error("sandbox unavailable"); + const execute = vi.fn().mockRejectedValue(unavailable); + const startedAt = Date.now(); + const result = readRemoteRunnerRecoveryProbe({ + runner: { execute }, request: { command: "sh" }, stage: "process_verification", + readOnly: true, deadline: startedAt + 2_000, + }).catch(error => error); + await vi.runAllTimersAsync(); + expect(await result).toBe(unavailable); + expect(readRemoteRunnerRecoveryError(unavailable)).toEqual({ stage: "process_verification", reason: "probe_transport_failed" }); + expect(execute.mock.calls.length).toBeGreaterThan(3); + expect(execute.mock.calls.length).toBeLessThan(9); + expect(Date.now() - startedAt).toBeLessThanOrEqual(2_000); + expect(execute.mock.calls.at(-1)?.[0].timeoutMs).toBeLessThanOrEqual(250); +}); + it("shares the remaining deadline across timed-out reads", async () => { vi.useFakeTimers(); const execute = vi.fn(async (request) => { diff --git a/server/src/services/native-runtime/remote-runner-recovery-probe.ts b/server/src/services/native-runtime/remote-runner-recovery-probe.ts index 7f48277f7e3..77486aa4496 100644 --- a/server/src/services/native-runtime/remote-runner-recovery-probe.ts +++ b/server/src/services/native-runtime/remote-runner-recovery-probe.ts @@ -17,9 +17,12 @@ export async function readRemoteRunnerRecoveryProbe(input: { let lastError: unknown = new RemoteRunnerRecoveryError({ stage: input.stage, reason: "probe_timeout", }); - for (let attempt = 0; attempt < (input.readOnly ? 3 : 1); attempt++) { + // A few fast gateway failures can exhaust an attempt count in under a + // second. Read-only observations share the caller's deadline instead; + // signals still have one attempt because their outcome may be unknown. + for (let attempt = 0; ; attempt++) { const remaining = input.deadline - Date.now(); - if (remaining <= 0) break; + if (!Number.isFinite(remaining) || remaining <= 0) break; try { const result = await input.runner.execute({ ...input.request, @@ -32,8 +35,9 @@ export async function readRemoteRunnerRecoveryProbe(input: { stage: input.stage, reason: "probe_transport_failed", }); } - if (!input.readOnly || attempt === 2 || input.deadline - Date.now() <= 250) break; - await new Promise(resolve => setTimeout(resolve, 250)); + const delayMs = Math.min(250 * 2 ** Math.min(attempt, 3), 2_000); + if (!input.readOnly || input.deadline - Date.now() <= delayMs) break; + await new Promise(resolve => setTimeout(resolve, delayMs)); } throw lastError; }