Skip to content

fix: handle Postgres pool errors instead of crashing - #19

Merged
jerelvelarde merged 1 commit into
CopilotKit:mainfrom
ZJKung:codebase-bug-scan
Sep 23, 2026
Merged

jerelvelarde merged 1 commit into
CopilotKit:mainfrom
ZJKung:codebase-bug-scan

Conversation

@ZJKung

@ZJKung ZJKung commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

What changed

pg.Pool had no error listener. When the database restarts or fails over, an idle client disconnects and the pool emits error. With no listener, Node throws and the API and task worker processes crash.

createStore now builds its pool with a new createPool(), which registers a listener that logs the failure through backgroundFailure. Only the error's name is logged; connection details are never logged. The pool discards the broken client and opens a fresh connection on the next query.

Verification

  • A new test in tests/persistence.test.ts emits a pool error without connecting to a database. It checks that nothing throws and that the error is logged once. The test failed before the fix and passes after it.
  • pnpm typecheck passes.
  • pnpm test gives 157 passes and 1 failure. The failure is computer-runner.test.ts, which is unrelated to this change: its fake docker script depends on a shebang line, and Windows ignores that line, so the script cannot run there.
  • biome check is clean on the changed files.

An idle client disconnected by a database restart or failover makes
pg.Pool emit `error`; with no listener, Node throws and the API and
worker processes exit. Register a listener that logs via
backgroundFailure (error name only, no connection details).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 22, 2026 17:09
@ZJKung ZJKung changed the title Handle Postgres pool errors instead of crashing fix: handle Postgres pool errors instead of crashing Sep 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified, and regression coverage is included.

Review effort: Lite
Findings: None

What changed in this PR

Prevents PostgreSQL pool errors from crashing API and worker processes by logging them safely.

Changes:

  • Adds centralized PostgreSQL pool error handling.
  • Routes store creation through the new pool factory.
  • Adds regression coverage for emitted pool errors.
File Summary
tests/​persistence.test.ts Verifies pool errors are logged without throwing.
apps/​server/​src/​db.ts Adds safe pool creation and error handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@NathanTarbert

Copy link
Copy Markdown
Collaborator

Hey @ZJKung,

Thanks for this, good catch. The fix looks right, and the test does its job: if you remove the listener, it fails.

One idea, not a blocker. Postgres server errors from pg come through with name set to "error", so a restart shows up in the log as error: "error" under postgres pool. Adding the SQLSTATE code in backgroundFailure would make those lines more useful when debugging a failover. That's a log.ts change, so it's fine as a follow-up if you'd rather keep this PR focused.

I'll get this in front of the team to merge. Thanks again!

@jerelvelarde

Copy link
Copy Markdown
Collaborator

Security review — no issues found (reviewed head 9355b03)

  • Fixes a crash-on-failover DoS. Without an error listener, an idle-client disconnect from a Postgres restart or failover crashes the API and worker processes. The listener closes that.
  • No sensitive data in logs. backgroundFailure logs only error.name and the phase. The connection string, credentials and error message never reach the log.
  • The test is contained. It points at 127.0.0.1:1 and only calls pool.emit, so no connection is opened and nothing leaves the machine.
  • Scope is limited. It touches only apps/server/src/db.ts and tests/persistence.test.ts: no dependencies, lockfile, CI or config changes.

CI hasn't run on this PR yet; merging once it's green.

@jerelvelarde
jerelvelarde merged commit cb82379 into CopilotKit:main Sep 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants