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();