mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-15 20:53:23 +02:00
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - The server reports crashes to Sentry so operators can find real
faults
> - Three expected conditions report as crashes: a client that closes
the connection mid-request, one stale pooled database socket after a
pooled endpoint recycles, and the short boot window where a supervised
cloud stack runs a new app image before its migration runner has caught
up
> - These events arrive in the hundreds and bury real errors
> - This pull request classifies each condition as expected and stops
the Sentry capture for exactly that condition, with behavior unchanged
everywhere else
> - The benefit is a Sentry feed where each event is a real fault
## Linked Issues or Issue Description
**What happened?**
Three noise classes fill the backend Sentry project on managed cloud
fleets:
1. `Error: aborted` (ECONNRESET) reports as a 500 crash when a client
closes the tab or loses its network mid-request. Observed 18 times in
one week from routine client disconnects.
2. `Error: write CONNECTION_CLOSED ...` reports from many query paths
after a pooled Postgres endpoint suspends. The existing single retry in
cloud actor resolution still fails, because a suspended endpoint kills
every pooled socket at once and the one replay draws another dead
socket.
3. `Error: PostgreSQL has pending migrations (...). Refusing to start`
reports from every supervised stack during a fleet upgrade. The
supervisor delivers the new app image before it runs the migration
runner, so each stack crash-loops briefly by design. One fleet roll
produced 329 events (11 per container).
**Expected behavior**
A client disconnect ends the request quietly. A transient dead socket is
replayed until a live socket answers. A supervised mid-upgrade boot
refusal logs and exits nonzero without a Sentry capture, while the same
refusal on a self-hosted deployment keeps reporting.
**Steps to reproduce**
1. Abort an HTTP request mid-flight: the error handler reports a crash
to Sentry.
2. Suspend a pooled Postgres endpoint under an idle server, then issue
two quick requests: the first replay can draw a second dead socket and
surface `CONNECTION_CLOSED`.
3. On a deployment with `PAPERCLIP_CLOUD_API_ORIGIN` set, add a
migration file without running the migration runner and boot: the
refusal reports to Sentry.
**Paperclip version or commit**
master (0e14c61da)
## What Changed
- `server/src/middleware/error-handler.ts`: a request abort (`Error:
aborted` with `ECONNRESET`) ends the response with status 499 and skips
crash reporting and telemetry.
- `server/src/middleware/auth.ts`: `retryOnTransientDbConnectionError`
replays up to twice with a short pause, so a pool-wide recycle does not
defeat the retry.
- `server/src/startup-refusals.ts`: pending migrations on a database
with applied history classify as a supervised-transient refusal
(`schema-migration-pending`). The capture skip applies only when
`PAPERCLIP_CLOUD_API_ORIGIN` is set. A wiped journal beside real tables
keeps reporting. Self-hosted behavior is unchanged.
- Tests updated and added for all three behaviors.
## Verification
- `pnpm vitest run src/__tests__/startup-refusals.test.ts
src/__tests__/cloud-tenant-transient-db-retry.test.ts
src/__tests__/error-handler.test.ts` in `server/` — 25 tests, all pass.
- The abort test asserts no `captureException` and no telemetry crash
track.
- The refusal tests pin all three classifications: never-migrated,
pending-with-history, wiped journal.
## Risks
- Low risk. The abort path only triggers on the exact `aborted` +
`ECONNRESET` pair; every other error keeps reporting.
- The refusal reclassification suppresses a capture only under a cloud
supervisor. If an operator breaks a migration runner, the supervisor's
own deploy gates surface it; self-hosted deployments still report.
- The retry widening adds at most ~150 ms before a genuine connection
fault surfaces.
## Model Used
Claude (Anthropic) — claude-fable-5 (Claude Fable 5), Claude Code
harness, extended thinking with tool use.
## 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
296 lines
9.4 KiB
TypeScript
296 lines
9.4 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";
|
|
|
|
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();
|
|
});
|
|
|
|
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("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",
|
|
},
|
|
);
|
|
});
|
|
});
|