From bce92f91344161cdd449e058def065c9b83b6ff3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 10:47:38 +0000 Subject: [PATCH] fix(postgres,mariadb): attach pool 'error' listeners so a dropped idle 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 Claude-Session: https://claude.ai/code/session_01KU7ZccZ8RgLY6rqnnoQbgv --- .../__tests__/connect-failure-cleanup.test.ts | 2 + .../__tests__/pool-error-listener.test.ts | 118 ++++++++++++++++++ .../readonly-transaction-strategy.test.ts | 2 + src/connectors/mariadb/index.ts | 14 +++ src/connectors/postgres/index.ts | 12 ++ 5 files changed, 148 insertions(+) create mode 100644 src/connectors/__tests__/pool-error-listener.test.ts diff --git a/src/connectors/__tests__/connect-failure-cleanup.test.ts b/src/connectors/__tests__/connect-failure-cleanup.test.ts index bbd9080a..a6a5df3f 100644 --- a/src/connectors/__tests__/connect-failure-cleanup.test.ts +++ b/src/connectors/__tests__/connect-failure-cleanup.test.ts @@ -84,6 +84,7 @@ describe("connect() failure cleanup", () => { mariadbCreatePool.mockReturnValue({ query: vi.fn().mockRejectedValue(PROBE_FAILURE), end, + on: vi.fn(), }); const connector = new MariaDBConnector(); @@ -98,6 +99,7 @@ describe("connect() failure cleanup", () => { pgPoolCtor.mockReturnValue({ connect: vi.fn().mockRejectedValue(PROBE_FAILURE), end, + on: vi.fn(), }); const connector = new PostgresConnector(); diff --git a/src/connectors/__tests__/pool-error-listener.test.ts b/src/connectors/__tests__/pool-error-listener.test.ts new file mode 100644 index 00000000..13f6ee21 --- /dev/null +++ b/src/connectors/__tests__/pool-error-listener.test.ts @@ -0,0 +1,118 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { EventEmitter } from "events"; + +/** + * Pools must carry an 'error' listener (bytebase/dbhub#422). + * + * pg-pool re-emits an idle client's error (server restart, failover, + * idle-timeout close) as an 'error' event on the pool, and the mariadb pool + * emits 'error' when a background reconnect attempt fails. An EventEmitter + * with no 'error' listener throws on emit, which Node reports as an + * unhandled 'error' event and exits the process. Over stdio, MCP clients do + * not restart the server, so the crash takes the database tools away until + * the user reconnects manually. + * + * The fake pools below are real EventEmitters, so `emit("error")` throws + * exactly like the drivers' pools would if the connector forgot the listener. + */ + +class FakePgPool extends EventEmitter { + connect = vi.fn().mockResolvedValue({ release: vi.fn() }); + end = vi.fn().mockResolvedValue(undefined); +} + +class FakeMariadbPool extends EventEmitter { + query = vi.fn().mockResolvedValue([{ version: "11.4.0-MariaDB" }]); + end = vi.fn().mockResolvedValue(undefined); +} + +let pgPool: FakePgPool; +let mariadbPool: FakeMariadbPool; + +vi.mock("pg", () => ({ + default: { + Pool: function (this: any) { + return pgPool; + }, + }, +})); + +vi.mock("mariadb", () => ({ + createPool: () => mariadbPool, +})); + +const { PostgresConnector } = await import("../postgres/index.js"); +const { MariaDBConnector } = await import("../mariadb/index.js"); + +const IDLE_DROP = new Error("terminating connection due to administrator command"); + +const spyConsoleError = () => vi.spyOn(console, "error").mockImplementation(() => {}); + +describe("pool 'error' listener", () => { + let consoleError: ReturnType; + + beforeEach(() => { + pgPool = new FakePgPool(); + mariadbPool = new FakeMariadbPool(); + consoleError = spyConsoleError(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("an unlistened pool would crash the process (sanity check of the fake)", () => { + expect(() => pgPool.emit("error", IDLE_DROP)).toThrow(IDLE_DROP); + }); + + it("PostgreSQL survives an idle connection being dropped", async () => { + const connector = new PostgresConnector(); + await connector.connect("postgres://u:p@localhost:5432/db"); + + expect(pgPool.listenerCount("error")).toBe(1); + // Mirrors pg-pool's idleListener: the client is already purged, then the + // pool re-emits. With no listener this emit would throw. + expect(() => pgPool.emit("error", IDLE_DROP, {})).not.toThrow(); + + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError.mock.calls[0].join(" ")).toContain("PostgreSQL pool"); + expect(consoleError.mock.calls[0].join(" ")).toContain(IDLE_DROP.message); + + await connector.disconnect(); + }); + + it("PostgreSQL log line names the source", async () => { + const connector = new PostgresConnector(); + (connector as any).sourceId = "prod_pg"; + await connector.connect("postgres://u:p@localhost:5432/db"); + + pgPool.emit("error", IDLE_DROP, {}); + expect(consoleError.mock.calls[0][0]).toContain('source "prod_pg"'); + }); + + it("MariaDB survives a background reconnect failure", async () => { + const connector = new MariaDBConnector(); + await connector.connect("mariadb://u:p@localhost:3306/db"); + + expect(mariadbPool.listenerCount("error")).toBe(1); + const reconnectFailure = new Error("Pool fails to create connection: ECONNREFUSED"); + expect(() => mariadbPool.emit("error", reconnectFailure)).not.toThrow(); + + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError.mock.calls[0].join(" ")).toContain("MariaDB pool"); + expect(consoleError.mock.calls[0].join(" ")).toContain(reconnectFailure.message); + + await connector.disconnect(); + }); + + it("does not stack listeners across reconnects of the same connector", async () => { + const connector = new PostgresConnector(); + await connector.connect("postgres://u:p@localhost:5432/db"); + await connector.disconnect(); + + // A fresh pool per connect(): the listener is attached to the new pool. + pgPool = new FakePgPool(); + await connector.connect("postgres://u:p@localhost:5432/db"); + expect(pgPool.listenerCount("error")).toBe(1); + }); +}); diff --git a/src/connectors/__tests__/readonly-transaction-strategy.test.ts b/src/connectors/__tests__/readonly-transaction-strategy.test.ts index 93d92fc5..acbe0a0f 100644 --- a/src/connectors/__tests__/readonly-transaction-strategy.test.ts +++ b/src/connectors/__tests__/readonly-transaction-strategy.test.ts @@ -53,6 +53,8 @@ function makeFakePool(version: string, wrapResults: (rows: any[]) => any) { query: vi.fn(async () => wrapResults([{ version }])), getConnection: vi.fn(async () => conn), end: vi.fn(), + // Connectors attach a pool 'error' listener at connect time. + on: vi.fn(), }; return { pool, conn, statements }; } diff --git a/src/connectors/mariadb/index.ts b/src/connectors/mariadb/index.ts index 72fc19a3..c7e4e6f7 100644 --- a/src/connectors/mariadb/index.ts +++ b/src/connectors/mariadb/index.ts @@ -162,6 +162,20 @@ export class MariaDBConnector implements Connector { this.pool = mariadb.createPool(connectionConfig); + // The mariadb pool keeps `minimumIdle` connections open in the background + // (defaults to connectionLimit) and emits an 'error' event on the pool when + // one of those background reconnect attempts fails, e.g. while the server + // is restarting. Without a listener Node treats it as unhandled and exits + // the whole process. The pool retries with backoff on its own, so logging + // is all that is needed here. The typings omit this event, but the runtime + // Pool is an EventEmitter. + (this.pool as unknown as NodeJS.EventEmitter).on("error", (err: Error) => { + console.error( + `MariaDB pool (source "${this.sourceId}"): background connection error, pool will retry:`, + err.message + ); + }); + // Test the connection and detect the server flavor in the same round trip. const rows = await this.pool.query("SELECT VERSION() AS version"); this.supportsReadOnlyTransaction = !isTiDBVersion(rows?.[0]?.version); diff --git a/src/connectors/postgres/index.ts b/src/connectors/postgres/index.ts index 13d11f22..4f366b3e 100644 --- a/src/connectors/postgres/index.ts +++ b/src/connectors/postgres/index.ts @@ -206,6 +206,18 @@ export class PostgresConnector implements Connector { this.pool = new Pool(poolConfig); + // pg-pool re-emits an idle client's error (server restart, failover, + // idle-timeout close) as an 'error' event on the pool. Without a listener + // Node treats it as unhandled and exits the whole process. The client has + // already been purged from the pool by the time this fires, so logging is + // all that is needed; the next query checks out a fresh connection. + this.pool.on("error", (err: Error) => { + console.error( + `PostgreSQL pool (source "${this.sourceId}"): idle connection dropped, will reconnect on next query:`, + err.message + ); + }); + // Test the connection const client = await this.pool.connect(); client.release();