From 2c7ee842211650ad1c2933cafe1be5e354fe39b3 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 6 Nov 2026 00:39:27 -0800 Subject: [PATCH] fix(server): stop reporting work cancelled by a disconnected client (#19037) A route that aborts its storage read when the response closes rejects with AbortError after the client has gone. The error handler reported that cancellation as a server crash. Skip the error sinks for an AbortError when the response was closed before it finished; report it otherwise. Fixes PAPERCLIP-BACKEND-5A Co-Authored-By: Claude Opus 5.5 --- .../error-handler-client-abort.test.ts | 100 ++++++++++++++++++ server/src/middleware/error-handler.ts | 10 ++ 2 files changed, 110 insertions(+) create mode 100644 server/src/__tests__/error-handler-client-abort.test.ts diff --git a/server/src/__tests__/error-handler-client-abort.test.ts b/server/src/__tests__/error-handler-client-abort.test.ts new file mode 100644 index 00000000000..075d732bd39 --- /dev/null +++ b/server/src/__tests__/error-handler-client-abort.test.ts @@ -0,0 +1,100 @@ +import { createServer } from "node:http"; +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/error-handler.js"; + +const captureExceptionMock = vi.hoisted(() => vi.fn()); + +vi.mock("../sentry.js", () => ({ captureException: captureExceptionMock })); +vi.mock("../telemetry.js", () => ({ getTelemetryClient: () => null })); + +// The S3 client rejects an aborted GetObject with this error. +function storageAbortError() { + return Object.assign(new Error("Request aborted"), { name: "AbortError" }); +} + +/** Routes that cancel their storage read when the response closes. */ +function createApp(onReadStarted: () => void, onErrorHandled: (err: unknown) => void = () => {}) { + const app = express(); + const cancellableRead = (abortError: (signal: AbortSignal) => unknown) => + async (_req: express.Request, res: express.Response) => { + const controller = new AbortController(); + const cancelRead = () => controller.abort(); + res.once("close", cancelRead); + try { + await new Promise((_resolve, reject) => { + controller.signal.addEventListener("abort", () => reject(abortError(controller.signal))); + onReadStarted(); + }); + } finally { + res.off("close", cancelRead); + } + }; + app.get("/log", cancellableRead(() => storageAbortError())); + // A reader that rejects with the signal's own reason (a DOMException). + app.get("/log-reason", cancellableRead((signal) => signal.reason)); + app.get("/failure", async () => { + throw storageAbortError(); + }); + app.use((err: unknown, req: express.Request, res: express.Response, next: express.NextFunction) => { + try { + errorHandler(err, req, res, next); + } finally { + onErrorHandled(err); + } + }); + return app; +} + +async function withServer(app: express.Express, run: (baseUrl: string) => Promise) { + const server = createServer(app); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + try { + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Expected a TCP port"); + await run(`http://127.0.0.1:${address.port}`); + } finally { + server.closeAllConnections(); + await new Promise((resolve) => server.close(() => resolve())); + } +} + +describe("errorHandler client aborts", () => { + beforeEach(() => { + captureExceptionMock.mockReset(); + }); + + it.each([ + { path: "/log", message: "Request aborted" }, + { path: "/log-reason", message: "This operation was aborted" }, + ])("does not report work cancelled because the client disconnected ($path)", async ({ path, message }) => { + let readStarted!: () => void; + const started = new Promise((resolve) => { readStarted = resolve; }); + let errorHandled!: (err: unknown) => void; + const handled = new Promise((resolve) => { errorHandled = resolve; }); + await withServer(createApp(() => readStarted(), (err) => errorHandled(err)), async (baseUrl) => { + const clientRequest = request(baseUrl).get(path); + const settled = clientRequest.then(() => undefined, () => undefined); + await started; + clientRequest.abort(); + await settled; + // Wait until the route's rejection has passed through the error handler. + await expect(handled).resolves.toMatchObject({ name: "AbortError", message }); + }); + + expect(captureExceptionMock).not.toHaveBeenCalled(); + }); + + it("still reports an AbortError while the client is connected", async () => { + await withServer(createApp(() => {}), async (baseUrl) => { + const response = await request(baseUrl).get("/failure"); + expect(response.status).toBe(500); + }); + + expect(captureExceptionMock).toHaveBeenCalledWith(expect.objectContaining({ + name: "AbortError", + message: "Request aborted", + })); + }); +}); diff --git a/server/src/middleware/error-handler.ts b/server/src/middleware/error-handler.ts index 3463d122a2f..7907f60c733 100644 --- a/server/src/middleware/error-handler.ts +++ b/server/src/middleware/error-handler.ts @@ -298,6 +298,16 @@ export function errorHandler( return; } + // A route that cancels its work when the response closes (for example a + // storage read) rejects with AbortError after the client has gone. That + // rejection is the cancellation, not a server fault. An AbortError while + // the client is still connected is still reported. + if (rootError.name === "AbortError" && res.destroyed && !res.writableFinished) { + if (!res.headersSent) res.status(499); + res.end(); + return; + } + // Shutdown drains the HTTP listener before it ends the pool, but a handler // whose client already disconnected (or whose socket the drain timeout // closed) keeps running. Its next query then fails with CONNECTION_ENDED