fix(postgres,mariadb): attach pool 'error' listeners so a dropped idle connection no longer crashes the process - #423
Merged
Conversation
…e connection no longer crashes the process
pg-pool re-emits an idle client's error (server restart, failover,
idle-timeout close) as an 'error' event on the pool. The PostgreSQL
connector never attached a listener, so Node reported an unhandled
'error' event and exited the whole DBHub process. Over stdio, MCP
clients do not restart the server, so the database tools were gone
until the user reconnected manually.
The mariadb pool has the same shape: it keeps `minimumIdle`
connections (default: connectionLimit) open in the background and
emits 'error' on the pool when a background reconnect attempt fails,
e.g. while the server is restarting.
Both pools have already discarded or will retry the affected
connection by the time the event fires, so the listener only logs the
error (with the source id) and lets the next query pick up a fresh
connection.
Tests: a new unit test drives each connector with an EventEmitter-based
fake pool, where emit('error') throws exactly as it would without a
listener, and asserts the connector survives and logs. Existing fake
pools gain an `on` stub.
Fixes #422
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KU7ZccZ8RgLY6rqnnoQbgv
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
Prevents PostgreSQL and MariaDB pool errors from crashing the process by adding error listeners and regression tests.
Changes:
- Adds pool error logging for PostgreSQL and MariaDB.
- Adds listener behavior tests.
- Updates affected fake pool mocks.
File summaries
| File | Summary |
|---|---|
src/connectors/postgres/index.ts |
Handles PostgreSQL pool errors. |
src/connectors/mariadb/index.ts |
Handles MariaDB pool errors. |
src/connectors/__tests__/readonly-transaction-strategy.test.ts |
Updates fake pool support. |
src/connectors/__tests__/pool-error-listener.test.ts |
Tests listener behavior and logging. |
src/connectors/__tests__/connect-failure-cleanup.test.ts |
Updates pool mocks. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #422
Problem
The PostgreSQL connector creates its pool with
new Pool(poolConfig)but never attaches apool.on("error", ...)listener. pg-pool re-emits an idle client's error (server restart, failover, idle-timeout close) as an'error'event on the pool. With no listener, Node treats it as an unhandled'error'event and the whole DBHub process exits:Over stdio, MCP clients (Claude Code, Codex) do not restart the server, so the database tools are gone until the user reconnects manually.
I reproduced this at the driver level: pg-pool 3.10.0 driven with a fake client, released into the idle set, then given an error. Without a listener the process exits with code 1 and the stack in the issue. With a listener the client is purged, the idle count drops to zero, and the next query reconnects normally.
The mariadb driver has the same shape. Its pool keeps
minimumIdleconnections open in the background (default:connectionLimit) and emits'error'on the pool from a plain callback when a background reconnect attempt fails, e.g. while the server is restarting. mysql2 handles idle errors internally without emitting on the pool, and mssql emits inside a promise chain (a rejection, not a crash), so those connectors are not changed.Fix
src/connectors/postgres/index.ts: attach an'error'listener right afternew Pool(...). The client has already been purged by the time the event fires, so the listener only logs the error with the source id.src/connectors/mariadb/index.ts: same, right aftercreatePool(...). The pool retries with backoff on its own. The mariadb typings omit the'error'overload onPool, so it is cast toNodeJS.EventEmitter(the runtime class extends it).Tests
src/connectors/__tests__/pool-error-listener.test.ts: drives each connector with an EventEmitter-based fake pool, whereemit("error")throws exactly as it would without a listener (the file includes a sanity check for that). Asserts that afterconnect()the pool has one listener, emitting no longer throws, the log line names the pool and source, and listeners do not stack across reconnects.connect-failure-cleanup.test.tsandreadonly-transaction-strategy.test.tsgain anonstub so their connect paths keep working.Full unit project: 38 files, 1002 tests passing. Integration suites need Docker, which was not available in this session.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KU7ZccZ8RgLY6rqnnoQbgv
Generated by Claude Code