Skip to content

Commit 5c3e6e7

Browse files
tianzhouclaude
andauthored
fix(timeout): server-side query_timeout on MySQL and one client-side fallback for PostgreSQL, MySQL, MariaDB (#465)
* fix(timeout): enforce query_timeout on the MySQL server and share one client-side fallback query_timeout behaved differently per connector. PostgreSQL and MariaDB applied it on the server (statement_timeout, max_statement_time); MySQL only had mysql2's client-side timer plus KILL QUERY. The client-side fallback differed too: PostgreSQL's fired 5s after the limit but then queued a ROLLBACK behind the abandoned statement and returned the stuck client to the pool, and MariaDB had no fallback at all, so a statement ran unbounded once max_statement_time was cleared on the connection. MySQL: set max_execution_time on every new pool connection, so the server stops a read-only SELECT at the limit and the connection stays usable. Writes and DDL are not covered by that variable and stay bounded by the client-side fallback. Fallback, now the same on PostgreSQL, MySQL and MariaDB (src/utils/query-timeout.ts): it fires 5s after query_timeout, skips the ROLLBACK, discards the connection, and cancels the statement on the server over a separate connection (pg_cancel_backend, KILL QUERY, or the mariadb driver's own KILL on destroy). The connection is discarded before the cancel so a pool at its limit cannot wait on itself. Behaviour change: a MySQL statement the server does not stop (a write, DDL) now times out 5s after query_timeout instead of exactly at it. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CQAiMsFZyDVEdGv5DXztxK * fix(timeout): bound query_timeout to Node's timer range; document cancellation as best-effort Node's setTimeout clamps delays above 2^31-1 ms, NaN and Infinity to 1 ms, and TOML accepts `inf`, so a large or non-finite query_timeout turned the client-side fallback into an immediate timeout on every statement. The config loader now rejects such values (at most 2147478 seconds), and clientQueryTimeoutMs throws on the direct ConnectorConfig path instead of scheduling a deadline Node cannot represent. The docs said DBHub cancels the statement after the fallback fires; the connectors only attempt it, so say so. Co-Authored-By: Claude Fable 5.1 <[email protected]> --------- Co-authored-by: Claude <[email protected]>
1 parent b8d7571 commit 5c3e6e7

16 files changed

Lines changed: 941 additions & 102 deletions

‎docs/config/toml.mdx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,18 @@ Sources define database connections. Each source represents a database that DBHu
305305

306306
Supported databases: PostgreSQL, MySQL, MariaDB, SQL Server, Oracle (not applicable to SQLite).
307307

308+
On PostgreSQL, MySQL and MariaDB the limit is applied on the database server, so a timed-out statement stops running there and the connection stays usable:
309+
310+
| Database | Server-side setting | Covers |
311+
|----------|---------------------|--------|
312+
| PostgreSQL | `statement_timeout` | All statements |
313+
| MySQL | `max_execution_time` | Read-only `SELECT` statements |
314+
| MariaDB | `max_statement_time` | All statements |
315+
316+
A client-side fallback fires 5 seconds after `query_timeout` for statements the server did not stop, such as a MySQL write or a statement on a connection whose session setting was changed. DBHub then discards that connection and attempts to cancel the statement on the server over a separate connection. The cancellation is best-effort: if it fails or the server does not honor it, the statement may keep running until the server finishes it.
317+
318+
The largest accepted value is 2147478 seconds (about 24 days), the longest fallback deadline a Node.js timer can represent.
319+
308320
```toml
309321
[[sources]]
310322
id = "production"

‎src/config/__tests__/toml-loader.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
22
import { loadTomlConfig, buildDSNFromSource, interpolateEnvVars } from '../toml-loader.js';
33
import type { SourceConfig } from '../../types/config.js';
44
import { SQLiteConnector } from '../../connectors/sqlite/index.js';
5+
import { MAX_QUERY_TIMEOUT_SECONDS } from '../../utils/query-timeout.js';
56
import { SQLServerConnector } from '../../connectors/sqlserver/index.js';
67
import fs from 'fs';
78
import path from 'path';
@@ -376,6 +377,32 @@ query_timeout = ${value}
376377
`)).toThrow('invalid query_timeout');
377378
});
378379

380+
// Node's setTimeout clamps delays above 2^31-1 ms (and non-finite ones)
381+
// to 1ms, which would turn the client-side fallback into an immediate
382+
// timeout on every query.
383+
it.each([
384+
['infinite', 'inf'],
385+
['NaN', 'nan'],
386+
['beyond the timer range', String(MAX_QUERY_TIMEOUT_SECONDS + 1)],
387+
])('should reject a query_timeout that is %s', (_label, value) => {
388+
expect(() => loadToml(`
389+
[[sources]]
390+
id = "test_db"
391+
dsn = "postgres://user:pass@localhost:5432/testdb"
392+
query_timeout = ${value}
393+
`)).toThrow('invalid query_timeout');
394+
});
395+
396+
it('should accept a query_timeout at the timer-range limit', () => {
397+
const result = loadToml(`
398+
[[sources]]
399+
id = "test_db"
400+
dsn = "postgres://user:pass@localhost:5432/testdb"
401+
query_timeout = ${MAX_QUERY_TIMEOUT_SECONDS}
402+
`);
403+
expect(result?.sources[0].query_timeout).toBe(MAX_QUERY_TIMEOUT_SECONDS);
404+
});
405+
379406
it.each([
380407
['zero', '0'],
381408
['negative', '-1'],

‎src/config/toml-loader.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { SourceConfig, TomlConfig, ToolConfig } from "../types/config.js";
66
import { parseCommandLineArgs, requireFlagValue } from "./env.js";
77
import { parseConnectionInfoFromDSN, getDefaultPortForType } from "../utils/dsn-obfuscate.js";
88
import { SafeURL } from "../utils/safe-url.js";
9+
import { MAX_QUERY_TIMEOUT_SECONDS } from "../utils/query-timeout.js";
910
import { BUILTIN_TOOL_EXECUTE_SQL, BUILTIN_TOOL_SEARCH_OBJECTS, ALL_BUILTIN_TOOL_NAMES } from "../tools/builtin-tools.js";
1011

1112
/**
@@ -524,12 +525,19 @@ function validateSourceConfig(source: SourceConfig, configPath: string): void {
524525
}
525526
}
526527

527-
// Validate query_timeout if provided
528+
// Validate query_timeout if provided. TOML accepts `inf`, and Node timers
529+
// cannot represent delays above ~24.8 days (see utils/query-timeout.ts), so
530+
// the value is bounded here instead of silently clamping to an immediate
531+
// client-side timeout.
528532
if (source.query_timeout !== undefined) {
529-
if (typeof source.query_timeout !== "number" || source.query_timeout <= 0) {
533+
if (
534+
!Number.isFinite(source.query_timeout) ||
535+
source.query_timeout <= 0 ||
536+
source.query_timeout > MAX_QUERY_TIMEOUT_SECONDS
537+
) {
530538
throw new Error(
531539
`Configuration file ${configPath}: source '${source.id}' has invalid query_timeout. ` +
532-
`Must be a positive number (in seconds).`
540+
`Must be a positive number of seconds, at most ${MAX_QUERY_TIMEOUT_SECONDS}.`
533541
);
534542
}
535543
}

‎src/connectors/__tests__/mariadb.integration.test.ts‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest';
22
import { MariaDbContainer, StartedMariaDbContainer } from '@testcontainers/mariadb';
33
import { MariaDBConnector } from '../mariadb/index.js';
44
import { IntegrationTestBase, type TestContainer, type DatabaseTestConfig } from './shared/integration-test-base.js';
5+
import { settle, waitFor } from './shared/query-timeout-helpers.js';
6+
import { CLIENT_QUERY_TIMEOUT_GRACE_MS } from '../../utils/query-timeout.js';
57
import type { Connector } from '../interface.js';
68

79
class MariaDBTestContainer implements TestContainer {
@@ -559,4 +561,73 @@ describe('MariaDB Connector Integration Tests', () => {
559561
}
560562
});
561563
});
562-
});
564+
565+
describe('query_timeout', () => {
566+
it('should let the server stop a statement and keep the connection usable', async () => {
567+
const connector = new MariaDBConnector();
568+
try {
569+
await connector.connect(mariadbTest.connectionString, undefined, { queryTimeoutSeconds: 1 });
570+
571+
const result = await settle(
572+
connector.executeSQL('SELECT SLEEP(30) AS slept', { readonly: true })
573+
);
574+
575+
// The server ends the statement at max_statement_time (ER_STATEMENT_TIMEOUT,
576+
// or SLEEP() returning early), well before the client-side fallback.
577+
if (result.error) {
578+
expect(result.error.errno).toBe(1969);
579+
}
580+
expect(result.elapsedMs).toBeLessThan(CLIENT_QUERY_TIMEOUT_GRACE_MS);
581+
582+
const after = await connector.executeSQL('SELECT 1 AS ok', { readonly: true });
583+
expect(Number(after.resultSets[0].rows[0].ok)).toBe(1);
584+
} finally {
585+
await connector.disconnect();
586+
}
587+
}, 20_000);
588+
589+
it('should fall back to the client-side timeout and kill the statement when the server limit is cleared', async () => {
590+
const connector = new MariaDBConnector();
591+
const observer = new MariaDBConnector();
592+
const probe = 'dbhub_client_timeout_probe';
593+
594+
const runningProbeCount = async (): Promise<number> => {
595+
const result = await observer.executeSQL(
596+
`SELECT COUNT(*) AS count FROM information_schema.processlist
597+
WHERE info LIKE '%${probe}%' AND info NOT LIKE '%processlist%'`,
598+
{}
599+
);
600+
return Number(result.resultSets[0].rows[0].count);
601+
};
602+
603+
try {
604+
await connector.connect(mariadbTest.connectionString, undefined, { queryTimeoutSeconds: 1 });
605+
await observer.connect(mariadbTest.connectionString);
606+
607+
// The batch clears the session limit first, so only the client-side
608+
// fallback is left to bound the SELECT.
609+
const result = await settle(
610+
connector.executeSQL(
611+
`SET SESSION max_statement_time = 0; SELECT SLEEP(30), '${probe}'`,
612+
{}
613+
)
614+
);
615+
616+
expect(result.error?.code).toBe('DBHUB_CLIENT_QUERY_TIMEOUT');
617+
expect(result.elapsedMs).toBeGreaterThanOrEqual(1000 + CLIENT_QUERY_TIMEOUT_GRACE_MS - 100);
618+
expect(result.elapsedMs).toBeLessThan(10_000);
619+
expect(await waitFor(async () => (await runningProbeCount()) === 0, 2_000)).toBe(true);
620+
621+
// The abandoned connection was discarded; a fresh one carries the limit again.
622+
const after = await connector.executeSQL(
623+
'SELECT @@session.max_statement_time AS mst',
624+
{ readonly: true }
625+
);
626+
expect(Number(after.resultSets[0].rows[0].mst)).toBe(1);
627+
} finally {
628+
await connector.disconnect();
629+
await observer.disconnect();
630+
}
631+
}, 30_000);
632+
});
633+
});

‎src/connectors/__tests__/mysql.integration.test.ts‎

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest';
22
import { MySqlContainer, StartedMySqlContainer } from '@testcontainers/mysql';
33
import { MySQLConnector } from '../mysql/index.js';
44
import { IntegrationTestBase, type TestContainer, type DatabaseTestConfig } from './shared/integration-test-base.js';
5+
import { settle, waitFor } from './shared/query-timeout-helpers.js';
6+
import { CLIENT_QUERY_TIMEOUT_GRACE_MS } from '../../utils/query-timeout.js';
57
import type { Connector } from '../interface.js';
68

79
class MySQLTestContainer implements TestContainer {
@@ -550,4 +552,80 @@ describe('MySQL Connector Integration Tests', () => {
550552
}
551553
});
552554
});
553-
});
555+
556+
describe('query_timeout', () => {
557+
it('should let the server stop a read-only SELECT and keep the connection usable', async () => {
558+
const connector = new MySQLConnector();
559+
try {
560+
await connector.connect(mysqlTest.connectionString, undefined, { queryTimeoutSeconds: 1 });
561+
562+
const limit = await connector.executeSQL(
563+
'SELECT @@session.max_execution_time AS met',
564+
{ readonly: true }
565+
);
566+
expect(Number(limit.resultSets[0].rows[0].met)).toBe(1000);
567+
568+
const result = await settle(
569+
connector.executeSQL(
570+
`SELECT COUNT(*) FROM information_schema.columns a,
571+
information_schema.columns b, information_schema.columns c`,
572+
{ readonly: true }
573+
)
574+
);
575+
576+
// ER_QUERY_TIMEOUT from the server, well before the client-side fallback.
577+
expect(result.error?.errno).toBe(3024);
578+
expect(result.elapsedMs).toBeLessThan(CLIENT_QUERY_TIMEOUT_GRACE_MS);
579+
580+
const after = await connector.executeSQL('SELECT 1 AS ok', { readonly: true });
581+
expect(Number(after.resultSets[0].rows[0].ok)).toBe(1);
582+
} finally {
583+
await connector.disconnect();
584+
}
585+
}, 20_000);
586+
587+
it('should fall back to the client-side timeout and kill the statement when the server limit is cleared', async () => {
588+
const connector = new MySQLConnector();
589+
const observer = new MySQLConnector();
590+
const probe = 'dbhub_client_timeout_probe';
591+
592+
const runningProbeCount = async (): Promise<number> => {
593+
const result = await observer.executeSQL(
594+
`SELECT COUNT(*) AS count FROM information_schema.processlist
595+
WHERE info LIKE '%${probe}%' AND info NOT LIKE '%processlist%'`,
596+
{}
597+
);
598+
return Number(result.resultSets[0].rows[0].count);
599+
};
600+
601+
try {
602+
await connector.connect(mysqlTest.connectionString, undefined, { queryTimeoutSeconds: 1 });
603+
await observer.connect(mysqlTest.connectionString);
604+
605+
// The batch clears the session limit first, so only the client-side
606+
// fallback is left to bound the SELECT.
607+
const result = await settle(
608+
connector.executeSQL(
609+
`SET SESSION max_execution_time = 0; SELECT SLEEP(30), '${probe}'`,
610+
{}
611+
)
612+
);
613+
614+
expect(result.error?.code).toBe('PROTOCOL_SEQUENCE_TIMEOUT');
615+
expect(result.elapsedMs).toBeGreaterThanOrEqual(1000 + CLIENT_QUERY_TIMEOUT_GRACE_MS - 100);
616+
expect(result.elapsedMs).toBeLessThan(10_000);
617+
expect(await waitFor(async () => (await runningProbeCount()) === 0, 2_000)).toBe(true);
618+
619+
// The abandoned connection was discarded; a fresh one carries the limit again.
620+
const after = await connector.executeSQL(
621+
'SELECT @@session.max_execution_time AS met',
622+
{ readonly: true }
623+
);
624+
expect(Number(after.resultSets[0].rows[0].met)).toBe(1000);
625+
} finally {
626+
await connector.disconnect();
627+
await observer.disconnect();
628+
}
629+
}, 30_000);
630+
});
631+
});
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest";
2+
import { EventEmitter } from "events";
3+
import { CANCEL_QUERY_TIMEOUT_MS } from "../../utils/query-timeout.js";
4+
5+
/**
6+
* Unit coverage for the PostgreSQL client-side query timeout fallback.
7+
*
8+
* node-postgres's `query_timeout` only rejects the query's promise: the client
9+
* keeps waiting for the server's answer, so anything sent on it afterwards
10+
* queues behind the abandoned statement. The integration suite exercises the
11+
* fallback against a real server; these tests pin the order of operations.
12+
*/
13+
14+
const BACKEND_PID = 4242;
15+
16+
/** The error node-postgres raises for its client-side timeout: no `code`. */
17+
const clientTimeout = () => new Error("Query read timeout");
18+
19+
class FakePgPool extends EventEmitter {
20+
statements: string[] = [];
21+
failOn: (sql: string) => Error | undefined = () => undefined;
22+
client = {
23+
processID: BACKEND_PID,
24+
query: vi.fn(async (sql: string) => {
25+
this.statements.push(sql);
26+
const failure = this.failOn(sql);
27+
if (failure) throw failure;
28+
return { rows: [{ ok: 1 }], rowCount: 1 };
29+
}),
30+
release: vi.fn(),
31+
};
32+
connect = vi.fn(async () => this.client);
33+
query = vi.fn(async (_config: unknown) => ({ rows: [{ pg_cancel_backend: true }], rowCount: 1 }));
34+
end = vi.fn().mockResolvedValue(undefined);
35+
}
36+
37+
let pool: FakePgPool;
38+
39+
vi.mock("pg", () => ({
40+
default: {
41+
Pool: function () {
42+
return pool;
43+
},
44+
types: { builtins: {}, getTypeParser: () => (value: string) => value },
45+
},
46+
}));
47+
48+
const { PostgresConnector } = await import("../postgres/index.js");
49+
50+
async function connect() {
51+
const connector = new PostgresConnector();
52+
await connector.connect("postgres://u:p@localhost:5432/db", undefined, {
53+
queryTimeoutSeconds: 30,
54+
});
55+
// connect() checks out and releases a client to test the connection.
56+
pool.client.release.mockClear();
57+
pool.connect.mockClear();
58+
return connector;
59+
}
60+
61+
describe("PostgreSQL client-side query timeout", () => {
62+
beforeEach(() => {
63+
pool = new FakePgPool();
64+
});
65+
66+
it.each([
67+
["a read-only statement", "SELECT pg_sleep(600)", { readonly: true }],
68+
["a writable statement", "SELECT pg_sleep(600)", {}],
69+
["a multi-statement batch", "SET statement_timeout = 0; SELECT pg_sleep(600)", {}],
70+
])("discards the client and cancels the backend for %s, skipping rollback", async (_label, sql, options) => {
71+
pool.failOn = (statement) => (statement.includes("pg_sleep") ? clientTimeout() : undefined);
72+
const connector = await connect();
73+
74+
await expect(connector.executeSQL(sql, options)).rejects.toThrow("Query read timeout");
75+
76+
// The client is still waiting on the abandoned statement, so a ROLLBACK
77+
// would queue behind it.
78+
expect(pool.statements).not.toContain("ROLLBACK");
79+
// Released exactly once, with a truthy argument: pg-pool closes the socket
80+
// and drops the client instead of returning it to the pool.
81+
expect(pool.client.release).toHaveBeenCalledTimes(1);
82+
expect(pool.client.release).toHaveBeenCalledWith(true);
83+
// The statement is cancelled on the server over another pooled connection,
84+
// bounded by its own short timeout...
85+
expect(pool.query).toHaveBeenCalledWith({
86+
text: "SELECT pg_cancel_backend($1)",
87+
values: [BACKEND_PID],
88+
query_timeout: CANCEL_QUERY_TIMEOUT_MS,
89+
});
90+
// ...after the client was discarded, so the cancel can use its pool slot
91+
// (pool_max_connections = 1 would otherwise wait on itself).
92+
expect(pool.client.release.mock.invocationCallOrder[0]).toBeLessThan(
93+
pool.query.mock.invocationCallOrder[0]
94+
);
95+
});
96+
97+
it("surfaces the timeout even if the cancel fails", async () => {
98+
pool.failOn = (statement) => (statement.includes("pg_sleep") ? clientTimeout() : undefined);
99+
pool.query.mockRejectedValue(new Error("connection refused"));
100+
const connector = await connect();
101+
102+
await expect(connector.executeSQL("SELECT pg_sleep(600)", { readonly: true })).rejects.toThrow(
103+
"Query read timeout"
104+
);
105+
expect(pool.client.release).toHaveBeenCalledWith(true);
106+
});
107+
108+
it("keeps the client for a server-side statement_timeout cancellation", async () => {
109+
const cancelled = Object.assign(new Error("canceling statement due to statement timeout"), {
110+
code: "57014",
111+
});
112+
pool.failOn = (statement) => (statement.includes("pg_sleep") ? cancelled : undefined);
113+
const connector = await connect();
114+
115+
await expect(connector.executeSQL("SELECT pg_sleep(600)", { readonly: true })).rejects.toBe(
116+
cancelled
117+
);
118+
119+
// The server ended the statement itself: the client is clean, so it is
120+
// rolled back and returned to the pool as usual, and nothing is cancelled.
121+
expect(pool.statements[pool.statements.length - 1]).toBe("ROLLBACK");
122+
expect(pool.client.release).toHaveBeenCalledTimes(1);
123+
expect(pool.client.release).toHaveBeenCalledWith();
124+
expect(pool.query).not.toHaveBeenCalled();
125+
});
126+
});

0 commit comments

Comments
 (0)