From be3e614d61bda152812cb51d7b0c173569d3113d Mon Sep 17 00:00:00 2001 From: Harjoth Khara Date: Tue, 13 Oct 2026 14:25:09 -0700 Subject: [PATCH] fix(workspace-runtime): keep the real readiness failure when the deadline-clamped probe aborts (#11575) ## Thinking Path > - Paperclip manages AI agents and the services they use for work. > - An agent can wait for a workspace service to become ready. > - Each readiness probe has a time limit. > - A final probe can expire and hide an earlier HTTP failure. > - This pull request keeps that observed failure in the reported error. > - Operators can then see why the service was not ready. ## Linked Issues or Issue Description Refs #11525. Related: #11494. **What happened?** A deadline-limited final readiness probe replaced an observed HTTP 503 with an abort message. **Expected behavior** Keep an earlier service failure when only the readiness deadline aborts the last probe. Report normal connection failures and probe timeouts as before. **Steps to reproduce** Return HTTP 503 from the first probe. Leave one millisecond in the readiness budget. Abort the last probe. The reported error must still contain HTTP 503. ## What Changed - Track whether a readiness probe observed a service failure. - Preserve that observation when the final probe expires at the readiness deadline. - Add a deterministic regression with an injected clock. - Keep probe scheduling, request count and timeout limits unchanged. ## Verification - Author verification: the readiness suite passed all nine tests; server typecheck passed. The regression failed before the fix and passed after it. - The later task review recorded 172 passing workspace tests and seven additional readiness checks. Historical author baseline failures are not claimed as passing local runs. - Latest head `936a374a08d0e74d87d588ba034f674388e8d508`: CI passed with 54 successful checks and two intentional skips. Fresh Apex review is 5/5 with no unresolved findings or recommendations. - A test merge with current master is clean. ## Risks - Error text changes when a service first fails and then hangs. The earlier observed failure is retained. - A hang without a prior service failure still reports the abort. - No data model, API or permission change. ## Model Used Anthropic Claude Opus 5 (`claude-opus-5`) authored the fix through Claude Code with reasoning, file editing and test tools. OpenAI GPT-5.6 assisted the original diagnosis. OpenAI GPT-6 (`gpt-6`) through Codex assisted this final review and PR preparation with repository and terminal tools. This session does not expose an exact context-window size. ## 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: Claude Opus 5 --- ...orkspace-runtime-start-terminality.test.ts | 38 +++++++++++++++++++ server/src/services/workspace-runtime.ts | 28 ++++++++++---- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/server/src/__tests__/workspace-runtime-start-terminality.test.ts b/server/src/__tests__/workspace-runtime-start-terminality.test.ts index 35e96a6db85..e1a0b7865d1 100644 --- a/server/src/__tests__/workspace-runtime-start-terminality.test.ts +++ b/server/src/__tests__/workspace-runtime-start-terminality.test.ts @@ -120,6 +120,44 @@ describe("managed runtime start terminality", () => { expect(probes).toBeLessThan(12); }); + it("preserves the last HTTP failure when the deadline-clamped probe aborts", async () => { + // The readiness budget (1s) is smaller than the per-probe timeout, so every probe is bounded + // by the deadline. The first probe observes the real failure (503); the second is admitted + // with 1ms left and aborts immediately. That abort says the budget ran out, not that the + // service misbehaved, so it must not overwrite the 503 in the thrown message. + let nowMs = 0; + let probes = 0; + const fetchImpl = (async (_url: string | URL | Request, init?: RequestInit) => { + probes += 1; + if (probes === 1) { + nowMs = 999; + return new Response(null, { status: 503 }); + } + return await new Promise((_resolve, reject) => { + const rejectOnAbort = () => { + nowMs = 1_000; + reject(new Error("The operation was aborted")); + }; + if (init?.signal?.aborted) { + rejectOnAbort(); + return; + } + init?.signal?.addEventListener("abort", rejectOnAbort, { once: true }); + }); + }) as unknown as typeof fetch; + + await expect( + waitForRuntimeServiceReadiness({ + service: { readiness: { type: "http", timeoutSec: 1, intervalMs: 100 } }, + url: "http://127.0.0.1:45439/", + readinessUrl: null, + fetchImpl, + now: () => nowMs, + }), + ).rejects.toThrow("Readiness check failed for http://127.0.0.1:45439/: received HTTP 503"); + expect(probes).toBe(2); + }); + it("hands two concurrent starts distinct ports even when the kernel repeats a candidate", async () => { // The kernel really does hand the same ephemeral port to two `listen(0)` probes once the // first probe socket has closed, which is exactly the reallocation collision in the report. diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index 2a553671640..d497fd1feeb 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -5276,21 +5276,33 @@ export async function waitForRuntimeServiceReadiness(input: { let lastError = "service did not become ready"; let lastCause: unknown; let probes = 0; + let sawServiceFailure = false; while (now() < deadline) { - const probeBudgetMs = Math.max(1, Math.min(RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS, deadline - now())); + const remainingMs = deadline - now(); + // When the readiness budget is what bounds this probe, aborting it only means we ran out of + // time — it is not an observation about the service, so it must not replace one we already have. + const boundedByDeadline = remainingMs <= RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS; + const probeBudgetMs = Math.max(1, Math.min(RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS, remainingMs)); + let probeSignal: AbortSignal | undefined; probes += 1; try { - const response = await fetchImpl(readinessUrl, { signal: AbortSignal.timeout(probeBudgetMs) }); + probeSignal = AbortSignal.timeout(probeBudgetMs); + const response = await fetchImpl(readinessUrl, { signal: probeSignal }); if (response.ok) return; lastError = `received HTTP ${response.status}`; lastCause = undefined; + sawServiceFailure = true; } catch (err) { - lastCause = err; - lastError = err instanceof Error ? err.message : String(err); - // Node fetch hides connection errors behind "fetch failed". Retain the - // transport cause so a refused port is distinguishable from a timeout. - if (err instanceof Error && err.cause instanceof Error) { - lastError += `: ${err.cause.message}`; + const abortedByDeadline = Boolean(probeSignal?.aborted) && boundedByDeadline; + if (!abortedByDeadline || !sawServiceFailure) { + lastCause = err; + lastError = err instanceof Error ? err.message : String(err); + // Node fetch hides connection errors behind "fetch failed". Retain the + // transport cause so a refused port is distinguishable from a timeout. + if (err instanceof Error && err.cause instanceof Error) { + lastError += `: ${err.cause.message}`; + } + sawServiceFailure = true; } } if (now() >= deadline) break;