fix(mysql): kill server-side query on timeout instead of leaking it into the pool - #386
Conversation
…nto the pool mysql2's `timeout` option is client-side only: it rejects the query's promise but never tells the connection's command queue the statement is done, and never notifies the server. The abandoned statement keeps running server-side, and anything sent afterward on that same connection (including our own best-effort ROLLBACK) silently queues behind it and blocks until the runaway query eventually finishes. On a PROTOCOL_SEQUENCE_TIMEOUT: skip the doomed ROLLBACK, issue KILL QUERY over a separate connection, and destroy (never release) the poisoned connection so it can't be handed to an unrelated caller in a stuck state. Fixes #384 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes MySQL query timeouts so they don’t leave server-side statements running and don’t return a poisoned pooled connection that can hang subsequent callers. It adds explicit handling for mysql2’s client-side-only timeout behavior by skipping rollback (which would otherwise block) and attempting to terminate the server-side query, then discarding the bad connection.
Changes:
- Add
isClientSideTimeout()helper and skipROLLBACKfor mysql2 timeout errors in the read-only transaction backstop. - In the MySQL connector, detect
PROTOCOL_SEQUENCE_TIMEOUT, attemptKILL QUERY <threadId>via a separate pooled connection, anddestroy()the timed-out connection instead of releasing it. - Add a unit test covering the timeout path: rollback skipped, kill issued on a fresh connection, and the original connection destroyed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utils/readonly-transaction.ts | Skips rollback for mysql2 client-side timeouts to avoid post-timeout hangs on the same connection. |
| src/connectors/mysql/index.ts | Attempts server-side cancellation and destroys poisoned connections on mysql2 timeouts. |
| src/connectors/tests/readonly-transaction-strategy.test.ts | Adds unit coverage for the timeout cleanup behavior (skip rollback, kill query, destroy connection). |
Suppressed comments (1)
src/connectors/mysql/index.ts:751
- KILL QUERY is part of the timeout cleanup path; it should be time-bounded as well so it can’t stall indefinitely if the server is unhealthy. Consider using mysql2 QueryOptions with a small timeout (independent of the user query timeout).
killer = await this.pool.getConnection();
await killer.query(`KILL QUERY ${threadId}`);
} catch {
Per Copilot review on #386: the cleanup KILL QUERY sent after a client-side timeout wasn't itself time-bounded, so it could stall indefinitely against an unhealthy server. Give it a short, fixed timeout independent of the user's query_timeout, and if that kill itself times out client-side, destroy the killer connection too instead of releasing it — it's subject to the same stuck-queue hazard as the original connection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed: the KILL QUERY cleanup call now carries its own short, fixed timeout (independent of the user's |
killerPoisoned ? killer.destroy() : killer.release() used a ternary purely for its side effects rather than to select a value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/connectors/mysql/index.ts:760
- In strict TS mode,
let killer;is an implicit-anycompile error. Also,killQueryis described as "bounded" by a short timeout, but only theKILL QUERYcall is timed out —pool.getConnection()can still wait indefinitely under pool exhaustion, causing the timeout path to hang again. Consider bounding the connection acquisition (and ensuring any late-acquired connection is immediately released) before issuingKILL QUERY.
if (!this.pool) return;
let killer;
let killerPoisoned = false;
try {
killer = await this.pool.getConnection();
await killer.query({ sql: `KILL QUERY ${threadId}`, timeout: MySQLConnector.KILL_QUERY_TIMEOUT_MS });
src/connectors/mysql/index.ts:727
- This adds a mocked unit test for the timeout path, but there’s still no integration-level regression test proving the end-to-end behavior against a real MySQL server (e.g.,
query_timeouttriggers promptly and a subsequent unrelated query is not blocked, or the timed-out statement disappears from PROCESSLIST). Given this fixes a production incident (#384), adding an integration test would better protect against regressions in mysql2/pool behavior.
} catch (error) {
if (isClientSideTimeout(error)) {
// mysql2's `timeout` option only aborts client-side: the statement
// keeps running on the server and this connection's command queue
// still thinks that statement is in flight (see isClientSideTimeout).
// Best-effort kill the server-side statement over a fresh connection
// so the timeout actually frees whatever the query was holding.
isConnectionPoisoned = true;
await this.killQuery(threadId);
}
The MCP request's AbortSignal reached `execute_sql` (as `extra`) but was only ever read for telemetry. Nothing in `src/` referenced an AbortSignal, and `ExecuteOptions` had no field to carry one, so cancelling a tool call merely stopped us waiting for the answer: the backend went on producing a result set nobody would ever read. That is the same class of bug as bytebase#384 on MySQL — a query outliving the client that asked for it — and this mirrors the fix accepted for it in bytebase#386. MySQL's trigger is its own client-side timeout; here the trigger is the client cancelling. Both end the same way: kill the statement server-side over a second connection, and don't hand the connection back to the pool in a state the next borrower would inherit. It matters most exactly where it hurts most: on read replicas serving agents, where an abandoned analytical query keeps consuming IO for its full runtime and drives replication lag long after the agent has gone. Reproduction (integration test `Query cancellation (options.signal)`): start `SELECT pg_sleep(30)`, abort the signal, then watch `pg_stat_activity`. Before this change the test fails after 30,042ms with the query having *resolved* — the abort changed nothing and the backend slept the full 30s. After it, the statement rejects with SQLSTATE 57014 (query_canceled) within milliseconds and the backend is gone from `pg_stat_activity`. - `ExecuteOptions` gains an optional `signal`; `execute_sql` passes the request's own. Connectors that ignore it are unaffected. - PostgreSQL cancellation is out-of-band: the request must arrive on a different connection, since the one running the query is blocked waiting for it. `cancelBackend` opens a dedicated short-lived connection rather than borrowing from the pool — a pool saturated with the very queries being cancelled has nothing left to lend. `pg`'s own `Client.cancel` is not reusable here: it needs the internal Query object, which the promise-based query API never exposes. - The connection is destroyed instead of released after a cancellation. A CancelRequest races the statement it targets, so it can instead land on the ROLLBACK issued during cleanup and leave the session in an aborted transaction. Mirrors MySQL's `isConnectionPoisoned`. - The abort listener is removed in `finally`, so listeners don't accumulate across the statements of one request. - An already-aborted signal is refused before a connection is taken, and re-checked once one is held: an AbortSignal dispatches "abort" exactly once, so a signal that fired while we waited for the pool would never reach a listener registered afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Fixes #384.
query_timeouton the MySQL connector currently only aborts the client-side wait — the statement keeps running on the MySQL server, and worse, the pooled connection is silently handed back in a state that can hang the next unrelated caller.Root cause:
mysql2'stimeoutoption is documented as client-side only (confirmed by the maintainer in sidorares/node-mysql2#185 and #789) — on timeout it rejects the query's own promise, but never tells the connection's internal command queue that the statement is done, and never notifies the server. Any command sent afterward on that same connection — including our own best-effortROLLBACKin the read-only transaction backstop — silently queues behind the abandoned statement and only runs once the real (still in-flight) server response for it eventually arrives. This is also why the issue's repro saw DBHub return at ~8s for a 3squery_timeout: it was blocked inside thatROLLBACK, waiting outSLEEP(8).MariaDB is not affected the same way:
src/connectors/mariadb/index.tsmapsqueryTimeoutto the server session variablemax_statement_time, which is real server-side enforcement.Changes
src/utils/readonly-transaction.ts: addedisClientSideTimeout()and skip the doomedROLLBACKfor that error class instead of hanging behind it.src/connectors/mysql/index.ts: onPROTOCOL_SEQUENCE_TIMEOUT, best-effort issueKILL QUERY <threadId>over a separate pooled connection (the original connection's queue can't be used for anything else), thenconn.destroy()the poisoned connection instead ofconn.release()-ing it back to the pool, permysql2's own documented contract for timed-out connections.Test plan
KILL QUERYsent on a fresh connection, original connection destroyed (not released).pnpm test, includes MySQL/MariaDB Testcontainers): 48 files / 1252 tests.tsc --noEmitshows no new errors.🤖 Generated with Claude Code