2 Commits

Include renames

Author	SHA1	Message	Date
3 people	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
Devin FoleyandPaperclip	4a999089ef	Retry transient failures in dashboard reads (#14873) ... ## Thinking Path > - Paperclip shows company activity in the dashboard. > - The dashboard reads company, task, approval, and cost data. > - A pooled database connection can close during one of these reads. > - The driver must reject ambiguous statements because writes may have committed. > - These four dashboard queries are known to be read-only. > - This change retries only the failed read and preserves completed work. ## Linked Issues or Issue Description Refs #14773, which correctly removed automatic replay of ambiguous database statements. Searched existing database and dashboard PRs. Related #8780 changes pool recycling and logging; #13925 adds dashboard consistency coverage. Neither provides these per-query retries. **What happened?** A dashboard request returns a server error when its company lookup, task count, approval count, or monthly spend query loses its database connection. Drizzle wraps the driver's connection error in `cause`. **Expected behavior** A transient connection failure gets a bounded retry of the specific read. Completed reads and budget processing are not replayed. Persistent outages and non-connection errors still fail the request. **Steps to reproduce** Inject a typed `CONNECTION_CLOSED` error into one of these reads. Then allow the next query to succeed. Before this change, the dashboard request fails immediately. ## What Changed - Apply independent retries to the company lookup, task counts, pending approval count, and monthly spend query. Each callback rebuilds its own query with the same company scope. - Extract the existing authentication retry helper as `retryIdempotentDatabaseOperation`. Preserve authentication behavior and its existing exports. - Retain the existing limit of three total attempts with 50 ms and 100 ms pauses. Match typed connection codes through the error cause chain. - Test later-read failures, unchanged query parameters, retry exhaustion, missing companies, and errors that must not retry. Document the boundary. ## Verification - Final focused dashboard, authentication, and real database wire suites: 38 tests passed. Six initial recovery regressions failed before the implementation. - `pnpm -r typecheck`: passed on the final source. - Independent review: no actionable findings. The reviewer separately passed all 38 focused tests and checked the code allowlist, attempt bounds, pauses, and final error identity. - `pnpm test:run`: the general-server group completed with 14,738 tests passed, 13 failed, and 87 skipped. All 13 failures match the previously reproduced clean-base macOS skill-cache failures. The two test files and their implementations are unchanged from that baseline. The runner exited after this group, so the remaining local workspace and serialized groups did not run. All corresponding Linux CI groups passed on this commit. - `pnpm build`: passed on the final source. - Full CI: 53 successful checks and 2 skips on `6a113529c0`. Greptile: 5/5 on that commit, with no review threads or remaining findings. - Merge compatibility with master `f2e0f19630`, including #14866: no conflicts. The five reviewed files are unchanged in the resulting merge tree. ## Risks A persistent outage adds at most two retries per covered query. Each connection attempt retains the configured driver timeout. The change does not repair the underlying network or database failure. Agent counts, run-activity queries, and the budget workflow stay outside these retry boundaries. General database statements and disconnected transactions are not replayed. There is no schema or authorization change. ## Model Used OpenAI GPT-6 through Codex, with reasoning, repository inspection, code editing, and test execution. The runtime does not expose a more specific serving model identifier or 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 and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass (38 focused tests; the full local run has the baseline limitation documented above) - [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: Paperclip <noreply@paperclip.ing>	2026-10-01 19:23:47 -07:00