Files
PaperClipAI/server/src/__tests__/error-handler.test.ts
T
9cc979116e fix(server): stop reporting queries that outlive the database shutdown (#16243)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - The server reports unexpected route errors to Sentry through the
shared error handler.
> - An orderly shutdown drains the HTTP listener, and then it ends the
database pool.
> - `server.close()` completes when the sockets close. A route handler
whose client disconnected, or whose socket the drain timeout closed, can
still run.
> - The next query of that handler fails with postgres.js
`CONNECTION_ENDED`, because this process ended the pool. The error
handler reports this as a 500 crash.
> - This pull request marks the pool as closing just before it ends. The
error handler then skips the error sinks for that one error code during
shutdown.
> - Public MCP calls run in their own internal Express app, so the flag
is also shared with that app.
> - The benefit is that a normal restart does not create false Sentry
crash reports. Real errors are still reported.

## Linked Issues or Issue Description

No public GitHub issue exists. This is the bug description.

**What happened?**

A staging server restarted. During the restart, `GET
/api/companies/:companyId/heartbeat-runs` failed in the per-row run
privacy check (`canActorReadHeartbeatRun`). The error was `Failed query:
select "id" from "issues" ...`. Its cause was `write CONNECTION_ENDED
<host>:5432`. postgres.js raises this code only for a query that starts
after the process called `sql.end()`. The error handler sent the error
to Sentry as a crash.

**Expected behavior**

A query that runs after the shutdown ends the pool is an expected part
of the shutdown. The server must not report it as a crash. If the
response is still open, the server answers `503`. The server must
continue to report all other errors.

**Steps to reproduce**

1. Start the server, and send a slow request (for example, a large
heartbeat run list).
2. Close the client connection, or let the 5 second drain timeout close
it, while the handler still runs.
3. Send SIGTERM. The shutdown drains HTTP and ends the pool.
4. The handler's next query fails with `CONNECTION_ENDED`. Before this
change, the error goes to Sentry.

## What Changed

- `server/src/database-retry.ts`: Add `isEndedDbPoolError`. It finds the
postgres.js `CONNECTION_ENDED` code anywhere in the error `cause` chain.
`isTransientDbConnectionError` now uses the same chain walk. Its
behavior does not change.
- `server/src/index.ts`: The orderly shutdown sets
`app.locals.paperclipDatabaseClosing = true` immediately before it ends
the database pools.
- `server/src/middleware/error-handler.ts`: When the flag is set and the
error is `CONNECTION_ENDED`, the handler answers `503 { error: "Server
is shutting down" }`, or it ends the response if the headers are already
sent. It does not call Sentry or crash telemetry. This follows the
existing client-abort (`ECONNRESET`) case.
- `server/src/services/public-mcp/capabilities.ts` and
`server/src/app.ts`: `createMcpApiDispatch` takes the main app's locals.
Each per-call dispatch app reads `paperclipDatabaseClosing` through a
live getter, so a public MCP call that outlives the pool is handled the
same way (Greptile review finding).
- `server/src/__tests__/error-handler.test.ts`: Add tests for the 503
path, the headers-sent path, `CONNECTION_ENDED` outside shutdown (still
reported), and `CONNECTION_CLOSED` during shutdown (still reported).
- `server/src/__tests__/public-mcp-dispatch-shutdown.test.ts`: Send an
ended-pool error through the real MCP dispatch, both during shutdown
(503, not reported) and outside shutdown (reported).

## Verification

- `cd server && vitest run
src/__tests__/public-mcp-dispatch-shutdown.test.ts
src/__tests__/error-handler.test.ts
src/__tests__/cloud-tenant-transient-db-retry.test.ts`: 25 tests pass.
- With only the error-handler change reverted, the two new error-handler
shutdown tests fail. With only the dispatch change reverted, the MCP
shutdown test fails. The negative-control tests pass in both cases.
- I could not run the full `pnpm -r typecheck`, `pnpm test:run`, and
`pnpm build` locally, or `public-mcp.test.ts`, which needs built
workspace packages. The sandbox has a 4 GB memory limit. CI runs the
full gates.

## Risks

- Low risk. The skip applies only to `CONNECTION_ENDED`, and only after
the shutdown starts to end the pools. postgres.js raises that code only
for a pool that this process ended. Network and database failures
(`CONNECTION_CLOSED`, `CONNECTION_DESTROYED`, and others) are still
reported.
- `CONNECTION_ENDED` outside shutdown (for example, a dedicated client
used after it ended) is still reported as a crash.
- The MCP dispatch app gets only the shutdown flag from the main app's
locals. It does not get any other locals.
- No schema, API contract, or UI change.

## Model Used

Anthropic Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool
use (repository inspection, shell, test execution) and the Sentry 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
- [x] 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: devinfoley <139239+devinfoley@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-13 18:26:03 -07:00

387 lines
13 KiB
TypeScript

import type { NextFunction, Request, Response } from "express";
import { beforeEach, describe, expect, it, vi } from "vitest";
import { HttpError } from "../errors.js";
import { errorHandler } from "../middleware/error-handler.js";
import { mcpDiscoveryHttpFailure, retainMcpConnectionFailure } from "../services/mcp-connection-failure.js";
const recordResponsibleUserDenialOnActiveRunMock = vi.hoisted(() => vi.fn());
const captureExceptionMock = vi.hoisted(() => vi.fn());
const telemetryMocks = vi.hoisted(() => ({
client: {},
trackErrorHandlerCrash: vi.fn(),
}));
vi.mock("../services/responsible-user-denial-run-outcomes.js", () => ({
recordResponsibleUserDenialOnActiveRun:
recordResponsibleUserDenialOnActiveRunMock,
}));
vi.mock("../sentry.js", () => ({ captureException: captureExceptionMock }));
vi.mock("../telemetry.js", () => ({
getTelemetryClient: () => telemetryMocks.client,
}));
vi.mock("@paperclipai/shared/telemetry", () => ({
trackErrorHandlerCrash: telemetryMocks.trackErrorHandlerCrash,
}));
function makeReq(): Request {
return {
method: "GET",
originalUrl: "/api/test",
body: { a: 1 },
params: { id: "123" },
query: { q: "x" },
} as unknown as Request;
}
function makeRes(): Response {
const res = {
status: vi.fn(),
json: vi.fn(),
} as unknown as Response;
(res.status as unknown as ReturnType<typeof vi.fn>).mockReturnValue(res);
return res;
}
describe("errorHandler", () => {
beforeEach(() => {
recordResponsibleUserDenialOnActiveRunMock.mockReset();
recordResponsibleUserDenialOnActiveRunMock.mockResolvedValue(null);
captureExceptionMock.mockReset();
telemetryMocks.trackErrorHandlerCrash.mockReset();
});
it("attaches the original Error to res.err for 500s", () => {
const req = makeReq();
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new Error("boom");
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith({ error: "Internal server error" });
expect(res.err).toBe(err);
expect(res.__errorContext?.error?.message).toBe("boom");
});
it("ends aborted client requests without reporting a crash", () => {
// A closed tab or dropped network surfaces as `Error: aborted` with
// ECONNRESET; there is no server fault and nobody left to answer.
const req = makeReq();
const res = { ...makeRes(), end: vi.fn(), headersSent: false } as any;
(res.status as ReturnType<typeof vi.fn>).mockReturnValue(res);
const next = vi.fn() as unknown as NextFunction;
const err = Object.assign(new Error("aborted"), { code: "ECONNRESET" });
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(499);
expect(res.end).toHaveBeenCalled();
expect(res.json).not.toHaveBeenCalled();
expect(captureExceptionMock).not.toHaveBeenCalled();
expect(telemetryMocks.trackErrorHandlerCrash).not.toHaveBeenCalled();
});
function endedPoolQueryError() {
// Drizzle wraps the driver error; postgres.js uses CONNECTION_ENDED for a
// query issued after this process called `sql.end()`.
const driverError = Object.assign(new Error("write CONNECTION_ENDED db.example:5432"), {
code: "CONNECTION_ENDED",
});
return new Error('Failed query: select "id" from "issues"', { cause: driverError });
}
it("answers 503 without reporting a query that outlived the database shutdown", () => {
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
errorHandler(endedPoolQueryError(), req, res, next);
expect(res.status).toHaveBeenCalledWith(503);
expect(res.json).toHaveBeenCalledWith({ error: "Server is shutting down" });
expect(res.err).toBeUndefined();
expect(captureExceptionMock).not.toHaveBeenCalled();
expect(telemetryMocks.trackErrorHandlerCrash).not.toHaveBeenCalled();
});
it("only ends the response when headers were already sent during database shutdown", () => {
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
const res = { ...makeRes(), end: vi.fn(), headersSent: true } as any;
const next = vi.fn() as unknown as NextFunction;
errorHandler(endedPoolQueryError(), req, res, next);
expect(res.status).not.toHaveBeenCalled();
expect(res.end).toHaveBeenCalled();
expect(captureExceptionMock).not.toHaveBeenCalled();
});
it("still reports an ended-pool query error outside database shutdown", () => {
const req = { ...makeReq(), app: { locals: {} } } as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = endedPoolQueryError();
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(500);
expect(captureExceptionMock).toHaveBeenCalledWith(err);
});
it("still reports other connection failures during database shutdown", () => {
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new Error("Failed query", {
cause: Object.assign(new Error("write CONNECTION_CLOSED"), { code: "CONNECTION_CLOSED" }),
});
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(500);
expect(captureExceptionMock).toHaveBeenCalledWith(err);
});
it("exposes raw 500 messages for trusted Cloud tenant imports", () => {
const req = {
...makeReq(),
method: "POST",
originalUrl: "/api/companies/import",
actor: {
type: "board",
userId: "cloud-user",
source: "cloud_tenant",
},
} as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new Error("portable file references missing upload id");
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith({
error: "Internal server error",
message: "portable file references missing upload id",
});
expect(res.err).toBe(err);
});
it("attaches HttpError instances for 500 responses", () => {
const req = makeReq();
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new HttpError(500, "db exploded");
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith({ error: "db exploded" });
expect(res.err).toBe(err);
expect(res.__errorContext?.error?.message).toBe("db exploded");
});
it("keeps HTTP failures and existing telemetry for proven external MCP outages while skipping Sentry", () => {
const req = makeReq();
const res = makeRes() as any;
const original = mcpDiscoveryHttpFailure(new Response(null, { status: 503 }), "Remote app returned HTTP 503");
const err = retainMcpConnectionFailure(original, new HttpError(502, original.message));
errorHandler(err, req, res, vi.fn());
expect(res.status).toHaveBeenCalledWith(502);
expect(res.json).toHaveBeenCalledWith({ error: original.message });
expect(res.err).toBe(err);
expect(res.__errorContext.error.message).toBe(original.message);
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledOnce();
expect(captureExceptionMock).not.toHaveBeenCalled();
});
it("reports copied connection markers and unrelated database failures", () => {
for (const err of [
Object.assign(new HttpError(502, "Remote app returned HTTP 503", { status: 503 }), {
connectionFailure: { schemaVersion: 1, provider: "mcp_http", operation: "discover_tools", reason: "remote_unavailable" },
}),
new HttpError(500, "database unavailable"),
]) {
errorHandler(err, makeReq(), makeRes(), vi.fn());
}
expect(captureExceptionMock).toHaveBeenCalledTimes(2);
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledTimes(2);
});
it("sanitizes chat setup errors before logs and crash reporting", () => {
const req = {
...makeReq(),
method: "POST",
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
body: { credentials: { botToken: "setup-error-token-canary" } },
} as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new Error("provider echoed setup-error-token-canary");
err.name = "SecretName-setup-error-token-canary";
errorHandler(err, req, res, next);
expect(res.err).not.toBe(err);
expect(res.err).toMatchObject({
name: "Error",
message: "Secret-sensitive request failed",
});
expect(res.__errorContext.error).toEqual({
name: "Error",
message: "Secret-sensitive request failed",
});
expect(captureExceptionMock).toHaveBeenCalledWith(res.err);
expect(JSON.stringify(captureExceptionMock.mock.calls)).not.toContain(
"setup-error-token-canary",
);
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledWith(
telemetryMocks.client,
{ errorCode: "Error" },
);
expect(
JSON.stringify(telemetryMocks.trackErrorHandlerCrash.mock.calls),
).not.toContain("setup-error-token-canary");
});
it("keeps actionable setup validation details while removing submitted credentials", () => {
const req = {
...makeReq(),
method: "POST",
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
body: { credentials: { botToken: "invalid-token-canary" } },
} as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const err = new HttpError(
422,
"Missing required Slack scopes for invalid-token-canary",
{
code: "chat_provider_permissions_missing",
credentials: { botToken: "invalid-token-canary" },
explanation: "Provider rejected invalid-token-canary",
requiredScopes: ["chat:write", "reactions:write"],
},
);
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(422);
expect(res.json).toHaveBeenCalledWith({
error: "Missing required Slack scopes for [REDACTED]",
code: "chat_provider_permissions_missing",
details: {
code: "chat_provider_permissions_missing",
credentials: "[REDACTED]",
explanation: "Provider rejected [REDACTED]",
requiredScopes: ["chat:write", "reactions:write"],
},
});
});
it("returns 400 for Zod validation errors from another module instance", () => {
const req = makeReq();
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const issue = {
code: "invalid_type",
expected: "string",
received: "undefined",
path: ["provider"],
message: "Required",
};
const err = Object.assign(new Error("Validation failed"), {
name: "ZodError",
issues: [issue],
errors: [issue],
});
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(400);
expect(res.json).toHaveBeenCalledWith({
error: "Validation error",
details: [issue],
});
expect(res.err).toBeUndefined();
expect(res.__errorContext).toBeUndefined();
});
it("removes submitted credentials from setup Zod issue prose", () => {
const req = {
...makeReq(),
method: "POST",
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
body: { credentials: { botToken: "zod-token-canary" } },
} as unknown as Request;
const res = makeRes() as any;
const next = vi.fn() as unknown as NextFunction;
const issue = {
code: "custom",
path: ["credentials", "botToken"],
message: "Rejected zod-token-canary",
};
const err = Object.assign(new Error("Validation failed"), {
name: "ZodError",
issues: [issue],
errors: [issue],
});
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(400);
expect(res.json).toHaveBeenCalledWith({
error: "Validation error",
details: [
{
code: "custom",
path: ["credentials", "botToken"],
message: "Rejected [REDACTED]",
},
],
});
});
it("records responsible-user denial codes on the active agent run", () => {
const db = { marker: "db" };
const req = {
...makeReq(),
app: { locals: { paperclipDb: db } },
actor: {
type: "agent",
agentId: "agent-1",
companyId: "company-1",
runId: "run-1",
source: "agent_jwt",
},
} as unknown as Request;
const res = makeRes();
const next = vi.fn() as unknown as NextFunction;
const err = new HttpError(403, "Responsible user is not authorized", {
code: "RESPONSIBLE_USER_UNAUTHORIZED",
});
errorHandler(err, req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: "Responsible user is not authorized",
code: "RESPONSIBLE_USER_UNAUTHORIZED",
details: { code: "RESPONSIBLE_USER_UNAUTHORIZED" },
});
expect(recordResponsibleUserDenialOnActiveRunMock).toHaveBeenCalledWith(
db,
{
runId: "run-1",
agentId: "agent-1",
companyId: "company-1",
code: "RESPONSIBLE_USER_UNAUTHORIZED",
},
);
});
});