From 0f2c01b63ac8ee132661bcabfafcc23e4372cfcf Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 5 Nov 2026 15:39:18 -0800 Subject: [PATCH] fix(server): retry one Cloud portfolio connection reset (#19033) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - On Cloud instances, `GET /api/cloud/stacks` gets the user's stack portfolio from the Cloud control plane. > - The route sends one `fetch` to `/v1/tenant/portfolio`. It has a 10 s deadline. > - The Cloud edge can reset a pooled keep-alive connection when the request is written. The request then fails in a few milliseconds and the user gets a 502. > - The diagnostics from #15340 now show this failure mode on staging. > - This pull request retries the idempotent GET one time for a pre-response connection reset, inside the same deadline. > - The benefit is that a transient connection reset no longer shows an error to the user. Real Cloud failures and deadlines are still reported. ## Linked Issues or Issue Description Refs #15340 (bounded portfolio diagnostics). Refs #10850 (portfolio proxy). I searched the open and closed PRs for "portfolio". No other PR fixes this failure. **What happened?** On the hosted staging pool stack, one `GET /api/cloud/stacks` returned 502. The diagnostics were `phase: fetch`, `networkCode: ECONNRESET`, `upstreamStatus: null`, `elapsedMs: 19`. The Core process had started about 7 minutes before. CPU and memory were normal. The Railway HTTP logs for the Cloud harness show no failed or missing portfolio request at that time. All portfolio requests in a 12-minute window returned 200 in 15–91 ms. The next Core portfolio fetch, 0.4 s later, reached Cloud and returned 200. Thus the failed request did not get to the Cloud application. The connection was reset between Core and the Cloud edge. This failure is different from the older stalls on the same stack. Those failures had `DEADLINE_EXCEEDED` after more than 60 s, during whole-process freezes. This change does not retry those failures. **Steps to reproduce** 1. Start an HTTP server that resets (or closes) the socket for the first request and returns JSON for the second request. 2. Set `PAPERCLIP_CLOUD_API_ORIGIN` to that server and call `GET /api/cloud/stacks` with the native `fetch`. 3. Before this change, the route returns 502 with `networkCode: ECONNRESET` (or `UND_ERR_SOCKET`). Native `fetch` does not retry. **Expected behavior** The route retries the idempotent GET one time when the connection is reset before a response. The retry uses the same 10 s signal. Other failures stay unchanged. ## What Changed - `server/src/routes/cloud.ts`: If the fetch phase fails with `ECONNRESET` or `UND_ERR_SOCKET`, the route sends the same GET one more time. The retry uses the same `AbortSignal.timeout(10_000)`. The total time stays at or below 10 s. - The route classifies the first error with the existing bounded `CloudPortfolioError` diagnostics. Thus a reset after the deadline stays `DEADLINE_EXCEEDED` and gets no retry. - The first reset gets a warning log with the same safe fields (`Paperclip Cloud portfolio connection reset; retrying once`). It does not send a Sentry event. Thus the logs still show how frequently this occurs. - A second reset, a deadline, HTTP error statuses, body failures, `ECONNREFUSED`, `ENOTFOUND`, connect timeouts and unknown errors behave as before: one report and no retry. Failures are still not cached. - `server/src/__tests__/cloud-routes.test.ts`: The old test "classifies a fetch reset without retrying" now expects one retry. New tests cover a second reset, the codes that get no retry, a reset after the deadline, and two real-socket cases. `doc/observability.md` describes the diagnostic context. Its fields and semantics do not change. Per `CONTRIBUTING.md`, the change adds no code comments; the retry rationale is in this description. ## Verification - Red before the fix: with the old route and the new tests, `pnpm exec vitest run server/src/__tests__/cloud-routes.test.ts` gave 5 failed, 19 passed. The failures were the two mocked retry cases, the second-reset case and the two real-socket cases. Thus native `fetch` on Node 25.9 does not retry these resets itself. - Green after the fix: `pnpm exec vitest run server/src/__tests__/cloud-routes.test.ts server/src/__tests__/cloud-portfolio-error.test.ts` gave 31 passed. - Real dependency: the real-socket tests use `node:http` and the native global `fetch`. `socket.resetAndDestroy()` gives `ECONNRESET`. `socket.destroy()` gives `UND_ERR_SOCKET`. Each test makes sure that the second request reaches the server with the same authorization, the route returns 200, and no Sentry event occurs. - The second commit only removes the four-line comment that Greptile flagged. It has no runtime effect; a syntax check passed locally and CI reruns the suite. - Not run locally: `server/src/__tests__/sentry.test.ts` fails to load in my sandbox on the unchanged base and with this change. The error is the same: `Failed to resolve entry for package "@paperclipai/plugin-sdk"`. The workspace packages were not built. The full `tsc -p server` was stopped by the 8 GB sandbox memory limit. A typecheck of only the changed files gave the same output on the base and with this change, apart from a 5-line offset. CI runs the full typecheck and test suite. - Remaining limit: the staging evidence shows that the request did not reach the Cloud application. It does not show if the reset socket was a reused keep-alive connection or a new connection. The retry is correct in both cases. ## Risks - Low risk. The retry applies only to an idempotent GET that has no side effects on Cloud. It occurs only for a fetch-phase reset before a response. - The worst case is one more portfolio request to Cloud for each reset. The deadline is the same as before. - If the Cloud edge resets all connections, the user sees the same 502 as before, after two fast attempts. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), through Claude Code, with tool use (shell, code execution, Sentry, Railway and GitHub connectors). ## 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 - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- server/src/__tests__/cloud-routes.test.ts | 103 ++++++++++++++++++++-- server/src/routes/cloud.ts | 34 ++++--- 2 files changed, 122 insertions(+), 15 deletions(-) diff --git a/server/src/__tests__/cloud-routes.test.ts b/server/src/__tests__/cloud-routes.test.ts index a85c4c4087b..9453453153e 100644 --- a/server/src/__tests__/cloud-routes.test.ts +++ b/server/src/__tests__/cloud-routes.test.ts @@ -1,3 +1,5 @@ +import { createServer } from "node:http"; +import type { AddressInfo, Socket } from "node:net"; import express from "express"; import request from "supertest"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -48,6 +50,10 @@ function createApp(options: { return app; } +function fetchReset(code: string) { + return new TypeError("private fetch message", { cause: Object.assign(new Error("private cause"), { code }) }); +} + function jsonResponse(payload: unknown, status = 200) { return new Response(JSON.stringify(payload), { status, @@ -78,9 +84,32 @@ describe("GET /api/cloud/stacks", () => { expect(JSON.stringify(warn.mock.calls)).not.toContain("private"); }); - it("classifies a fetch reset without retrying or caching the failure", async () => { + it.each(["ECONNRESET", "UND_ERR_SOCKET"])("retries one pre-response %s within the same deadline", async (code) => { const fetchImpl = vi.fn() - .mockRejectedValueOnce(new TypeError("private fetch message", { cause: Object.assign(new Error("private cause"), { code: "ECONNRESET" }) })) + .mockRejectedValueOnce(fetchReset(code)) + .mockResolvedValueOnce(jsonResponse({ stacks: [] })); + const capture = vi.spyOn(sentry, "captureException").mockImplementation(() => {}); + const warn = vi.spyOn(logger, "warn").mockImplementation(() => {}); + const timeout = vi.spyOn(AbortSignal, "timeout"); + const response = await request(createApp({ fetchImpl })).get("/api/cloud/stacks"); + expect(response.status).toBe(200); + expect(response.body).toEqual({ stacks: [] }); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(timeout).toHaveBeenCalledTimes(1); + expect(fetchImpl.mock.calls[1]![1]?.signal).toBe(fetchImpl.mock.calls[0]![1]?.signal); + expect(fetchImpl.mock.calls[1]![1]?.headers).toEqual(fetchImpl.mock.calls[0]![1]?.headers); + expect(capture).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + { cloudPortfolio: expect.objectContaining({ phase: "fetch", upstreamStatus: null, networkCode: code }) }, + "Paperclip Cloud portfolio connection reset; retrying once", + ); + expect(JSON.stringify(warn.mock.calls)).not.toContain("private"); + }); + + it("reports a second consecutive fetch reset without caching the failure", async () => { + const fetchImpl = vi.fn() + .mockRejectedValueOnce(fetchReset("ECONNRESET")) + .mockRejectedValueOnce(fetchReset("ECONNRESET")) .mockResolvedValueOnce(jsonResponse({ stacks: [] })); const capture = vi.spyOn(sentry, "captureException").mockImplementation(() => {}); const warn = vi.spyOn(logger, "warn").mockImplementation(() => {}); @@ -88,13 +117,77 @@ describe("GET /api/cloud/stacks", () => { const failure = await request(app).get("/api/cloud/stacks"); expect(failure.status).toBe(502); expect(failure.body).toEqual({ error: "Paperclip Cloud portfolio request failed", code: "cloud_portfolio_upstream_error", details: { code: "cloud_portfolio_upstream_error" } }); - expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(capture).toHaveBeenCalledTimes(1); const error = capture.mock.calls[0]![0] as CloudPortfolioError; expect(error.diagnostics).toMatchObject({ phase: "fetch", upstreamStatus: null, networkCode: "ECONNRESET" }); + expect(error.stack).toContain("routes/cloud.ts:"); + expect(warn).toHaveBeenLastCalledWith({ cloudPortfolio: error.diagnostics }, "Paperclip Cloud portfolio request failed"); expect(JSON.stringify(warn.mock.calls)).not.toContain("private"); expect((await request(app).get("/api/cloud/stacks")).body).toEqual({ stacks: [] }); - expect((await request(app).get("/api/cloud/stacks")).body).toEqual({ stacks: [] }); - expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(fetchImpl).toHaveBeenCalledTimes(3); + }); + + it.each(["ECONNREFUSED", "ENOTFOUND", "UND_ERR_CONNECT_TIMEOUT", "unclassified"])("does not retry a %s fetch failure", async (code) => { + const fetchImpl = vi.fn().mockRejectedValue(fetchReset(code)); + const capture = vi.spyOn(sentry, "captureException").mockImplementation(() => {}); + vi.spyOn(logger, "warn").mockImplementation(() => {}); + const response = await request(createApp({ fetchImpl })).get("/api/cloud/stacks"); + expect(response.status).toBe(502); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect((capture.mock.calls[0]![0] as CloudPortfolioError).diagnostics).toMatchObject({ + phase: "fetch", networkCode: code === "unclassified" ? "unknown" : code, + }); + }); + + it("does not retry a reset that arrives after the deadline", async () => { + const signal = AbortSignal.abort(new DOMException("private timeout", "TimeoutError")); + vi.spyOn(AbortSignal, "timeout").mockReturnValue(signal); + const fetchImpl = vi.fn().mockRejectedValue(fetchReset("ECONNRESET")); + const capture = vi.spyOn(sentry, "captureException").mockImplementation(() => {}); + vi.spyOn(logger, "warn").mockImplementation(() => {}); + const response = await request(createApp({ fetchImpl })).get("/api/cloud/stacks"); + expect(response.status).toBe(502); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect((capture.mock.calls[0]![0] as CloudPortfolioError).diagnostics).toMatchObject({ phase: "fetch", networkCode: "DEADLINE_EXCEEDED" }); + }); + + it.each([ + ["ECONNRESET", (socket: Socket) => socket.resetAndDestroy()], + ["UND_ERR_SOCKET", (socket: Socket) => socket.destroy()], + ] as const)("recovers from a real %s on the native fetch path", async (code, closeSocket) => { + let requests = 0; + const server = createServer((req, res) => { + requests++; + if (requests === 1) { + closeSocket(req.socket); + return; + } + res.setHeader("content-type", "application/json"); + res.end(JSON.stringify({ stacks: [{ slug: "current" }], authorization: req.headers.authorization })); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + try { + const { port } = server.address() as AddressInfo; + const capture = vi.spyOn(sentry, "captureException").mockImplementation(() => {}); + const warn = vi.spyOn(logger, "warn").mockImplementation(() => {}); + const app = createApp({ + fetchImpl: fetch, + runtimeEnv: { ...cloudEnv, PAPERCLIP_CLOUD_API_ORIGIN: `http://127.0.0.1:${port}` }, + }); + const response = await request(app).get("/api/cloud/stacks"); + expect(response.status).toBe(200); + expect(response.body).toEqual({ stacks: [{ slug: "current" }], authorization: "Bearer tenant-secret" }); + expect(requests).toBe(2); + expect(capture).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + { cloudPortfolio: expect.objectContaining({ phase: "fetch", networkCode: code }) }, + "Paperclip Cloud portfolio connection reset; retrying once", + ); + } finally { + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + } }); it.each(["invalid_json", "body_reset", "deadline"] as const)("keeps body-phase %s failures separate from fetch failures", async (kind) => { diff --git a/server/src/routes/cloud.ts b/server/src/routes/cloud.ts index e8456d7e40f..9e5b3246720 100644 --- a/server/src/routes/cloud.ts +++ b/server/src/routes/cloud.ts @@ -9,6 +9,7 @@ import { const DEFAULT_PORTFOLIO_CACHE_TTL_MS = 30_000; const CLOUD_PORTFOLIO_PATH = "/v1/tenant/portfolio"; +const RETRYABLE_FETCH_RESET_CODES = new Set(["ECONNRESET", "UND_ERR_SOCKET"]); type PortfolioCacheEntry = { expiresAt: number; @@ -86,16 +87,29 @@ export function cloudRoutes(opts: { try { upstreamSignal = AbortSignal.timeout(10_000); - const upstream = await fetchImpl(portfolioUrl, { - method: "GET", - headers: { - accept: "application/json", - authorization: `Bearer ${tenantToken}`, - "x-paperclip-cloud-user-id": actorUserId, - "x-paperclip-cloud-stack-id": context.stackId, - }, - signal: upstreamSignal, - }); + let upstream: Response; + for (let attempt = 1; ; attempt++) { + try { + upstream = await fetchImpl(portfolioUrl, { + method: "GET", + headers: { + accept: "application/json", + authorization: `Bearer ${tenantToken}`, + "x-paperclip-cloud-user-id": actorUserId, + "x-paperclip-cloud-stack-id": context.stackId, + }, + signal: upstreamSignal, + }); + break; + } catch (error) { + const { diagnostics } = portfolioError("upstream", error); + if (attempt > 1 || !RETRYABLE_FETCH_RESET_CODES.has(String(diagnostics.networkCode))) throw error; + logger.warn( + { cloudPortfolio: diagnostics }, + "Paperclip Cloud portfolio connection reset; retrying once", + ); + } + } phase = "http_response"; upstreamStatus = upstream.status; if (!upstream.ok) {