From 46d89ed8f784450949500dbf3b02cd0bb01a8e22 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:13:17 +0000 Subject: [PATCH 1/5] sql(mysql): evict a failed prepare from the statement cache so it is retried A COM_STMT_PREPARE that the server answers with an ERR packet left the statement in the per-connection statement cache with status Failed. Every later execution of the same query text on that connection found the cached entry and replayed its stored ErrorPacket without ever re-preparing, so a transient prepare-time error (a table created by a concurrent migration, a deadlock, ER_TOO_MANY_CONCURRENT_STMTS) poisoned the connection for the process lifetime. With connection pooling that surfaces as the same query permanently failing on an arbitrary subset of the pool. handle_prepared_statement's ERROR arm now removes the failed statement from the map and drops the map's ref on it, exactly as PostgresSQLConnection's ErrorResponse handler already did. Queries that attached to the statement before the failure still hold their own refs and still observe the error; the next query with the same text gets a fresh prepare. The found_existing + Failed guard in run_prepared_query is deleted: the only place that sets Status::Failed now also removes the entry, so a cached entry is never Failed, and the status match below it already rejects on Failed. test/js/sql/sql-mysql-cached-error.test.ts and the 'Cached failed prepared statement' case in sql-mysql.test.ts asserted the old no-re-prepare behavior (Com_stmt_prepare must not increment); both are updated to the new contract. --- src/sql_jsc/mysql/MySQLConnection.rs | 18 ++- src/sql_jsc/mysql/MySQLQuery.rs | 15 +-- test/js/sql/sql-mysql-cached-error.test.ts | 114 +++++++++-------- .../sql-mysql-failed-prepare-retry.test.ts | 117 ++++++++++++++++++ test/js/sql/sql-mysql.test.ts | 16 +-- test/js/sql/wire-frames.ts | 12 ++ 6 files changed, 217 insertions(+), 75 deletions(-) create mode 100644 test/js/sql/sql-mysql-failed-prepare-retry.test.ts diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index 8bba3068c9fd..a6bbfaca603f 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -1239,9 +1239,9 @@ impl MySQLConnection { self.flags.insert(ConnectionFlags::IS_READY_FOR_QUERY); statement.status = mysql_statement::Status::Failed; // err.error_message is a Data{ .temporary = ... } slice into the socket read - // buffer which will be overwritten by the next packet. The statement is cached - // in this.statements and its error_response may be read later via - // stmt.error_response.toJS(), so we must own a copy of the message bytes. + // buffer which will be overwritten by the next packet. Queries that attached + // to this statement before the failure read stmt.error_response later, so we + // must own a copy of the message bytes. // ErrorPacket lacks Clone in bun_sql (Data is not Clone), so // reconstruct field-by-field with an owned dupe of the message // — the scalar fields (header / error_code / sql_state) are @@ -1254,6 +1254,18 @@ impl MySQLConnection { error_message: Data::create(err.error_message.slice()) .map_err(|_| AnyMySQLError::OutOfMemory)?, }; + // Evict the failed prepare from the statement cache so the next query with + // this text re-prepares instead of rethrowing the stale server error forever + // (mirrors PostgresSQLConnection's ErrorResponse handler). + if self + .statements + .remove(&bun_wyhash::hash(&statement.signature.name)) + .is_some() + { + // SAFETY: the map held one intrusive ref on the statement; the request + // still holds its own ref, so this cannot drop the count to zero. + unsafe { MySQLStatement::deref(core::ptr::from_mut(statement)) }; + } self.queue.mark_as_ready_for_query(); self.queue.mark_current_request_as_finished(request); diff --git a/src/sql_jsc/mysql/MySQLQuery.rs b/src/sql_jsc/mysql/MySQLQuery.rs index 8432a8a249c7..21c87d18f845 100644 --- a/src/sql_jsc/mysql/MySQLQuery.rs +++ b/src/sql_jsc/mysql/MySQLQuery.rs @@ -342,19 +342,14 @@ impl MySQLQuery { let stmt: *mut MySQLStatement = *entry.value_ptr; // `found_existing` ⇒ the map already holds a live, ref-counted // `*mut MySQLStatement` (separate heap allocation, never aliases - // `*self`); this thread is the only mutator. Every access in this - // branch is a shared read (`status`, `error_response.to_js`, - // `ref_()` are `&self`), so a single `ParentRef` deref covers all - // three former per-site raw `(*stmt).…` derefs. + // `*self`); this thread is the only mutator. `ref_()` is `&self`, + // so a `ParentRef` deref covers the former raw `(*stmt).…` deref. + // A cached entry is never `Failed`: handle_prepared_statement + // evicts the statement from the map when its prepare errors, and + // the `match` below rejects on any `Failed` status regardless. let stmt_ref = bun_ptr::ParentRef::from( core::ptr::NonNull::new(stmt).expect("found_existing ⇒ non-null map entry"), ); - if stmt_ref.status == my_sql_statement::Status::Failed { - let error_response = stmt_ref.error_response.to_js(global_object); - // If the statement failed, we need to throw the error - let _ = global_object.throw_value(error_response); - return Err(bun_core::err!("JSError")); - } self.statement = stmt; stmt_ref.ref_(); drop(signature); diff --git a/test/js/sql/sql-mysql-cached-error.test.ts b/test/js/sql/sql-mysql-cached-error.test.ts index 8377a889e13a..1e5e31a72df7 100644 --- a/test/js/sql/sql-mysql-cached-error.test.ts +++ b/test/js/sql/sql-mysql-cached-error.test.ts @@ -1,73 +1,77 @@ -// Regression test: MySQLConnection.handlePreparedStatement stored an ErrorPacket whose -// error_message was a Data{ .temporary = ... } slice pointing into the socket read buffer. -// The statement is cached in the connection's statements map with status = .failed, so -// re-running the same failing query would read the stale slice after subsequent packets -// overwrote the buffer. +// Regression test: MySQLConnection cached a prepared statement whose +// COM_STMT_PREPARE failed (status = .failed) in the per-connection statement map +// and never evicted it, so every later execution of the same query text on that +// connection rethrew the stale ErrorPacket without ever re-preparing. A +// transient prepare-time error (a table created by a concurrent migration, a +// deadlock, ER_TOO_MANY_CONCURRENT_STMTS) therefore poisoned the connection for +// the process lifetime. handlePreparedStatement now evicts the failed statement +// from the map (as the Postgres driver already did) so the prepare is retried on +// the next use of that text. +// +// This file previously asserted the opposite (Com_stmt_prepare must NOT +// increment across an identical re-run) to pin a dangling-slice read in the +// cached ErrorPacket's error_message; that cache-hit path no longer exists. +// test/js/sql/sql-mysql-failed-prepare-retry.test.ts is the wire-level +// counterpart that runs without a container. -import { SQL } from "bun"; +import { SQL, randomUUIDv7 } from "bun"; import { expect, test } from "bun:test"; import { describeWithContainer, isDockerEnabled } from "harness"; if (isDockerEnabled()) { describeWithContainer("mysql", { image: "mysql_plain" }, container => { - test("MySQL: cached failed prepared statement error_message is not a dangling slice", async () => { + test("MySQL: a failed prepare is re-prepared instead of served from the statement cache", async () => { await container.ready; + // max: 1 so every query runs on the same connection / same statement map, + // and Com_stmt_prepare (a SESSION counter) observes exactly that session. await using sql = new SQL({ url: `mysql://root@${container.host}:${container.port}/bun_sql_test`, max: 1, }); - // Long bogus identifiers so the server's echoed error_message exceeds the 15-byte - // inline-string threshold and is heap-backed, and so the two messages differ at - // bytes the second packet would overwrite in the read buffer. MySQL truncates the - // "near '...'" clause to ~80 chars, so keep these short enough to appear in full. - const longA = Buffer.alloc(50, "A").toString(); - const longZ = Buffer.alloc(50, "Z").toString(); + const table = "t_retry_" + randomUUIDv7("hex").replaceAll("-", ""); + // .simple() = COM_QUERY (text protocol), so the counter read itself never + // sends a COM_STMT_PREPARE. + const prepares = async () => + Number((await sql.unsafe("SHOW SESSION STATUS LIKE 'Com_stmt_prepare'").simple())[0].Value); - // First failing query → statement cached as .failed with error_message. - const err1 = await sql`wat ${1} ${sql.unsafe(longA)}`.catch((x: any) => x); - expect(err1).toBeInstanceOf(Error); - expect(err1.code).toBe("ERR_MYSQL_SYNTAX_ERROR"); - expect(err1.errno).toBe(1064); - expect(err1.message).toContain(longA); + try { + // 1. The table does not exist yet: the prepare fails (ER_NO_SUCH_TABLE). + const err1 = await sql`SELECT n FROM ${sql(table)}`.catch((x: any) => x); + expect(err1).toBeInstanceOf(Error); + expect(err1.errno).toBe(1146); + const afterFirst = await prepares(); + expect(afterFirst).toBeGreaterThan(0); - // Different failing query → server sends a different ERROR packet that overwrites - // the connection read buffer where err1's message slice used to point. - const errOverwrite = await sql`other ${1} ${sql.unsafe(longZ)}`.catch((x: any) => x); - expect(errOverwrite).toBeInstanceOf(Error); - expect(errOverwrite.message).toContain(longZ); - expect(errOverwrite.message).not.toBe(err1.message); + // 2. Same text, table still missing. Before the fix the stale cached + // ErrorPacket was replayed and the server never saw a second + // COM_STMT_PREPARE; now Bun re-prepares and the server answers the + // same (still true) error. + const err2 = await sql`SELECT n FROM ${sql(table)}`.catch((x: any) => x); + expect({ errno: err2.errno, message: err2.message, prepares: await prepares() }).toEqual({ + errno: 1146, + message: err1.message, + prepares: afterFirst + 1, + }); - // Same as the first failing query → hits the cached .failed statement and calls - // stmt.error_response.toJS(). Before the fix this read the overwritten buffer and - // returned bytes from errOverwrite's packet; after the fix it returns the original. - // Com_stmt_prepare (read via .simple() so the status query itself does not prepare) - // must not increment across this call — proving the third query was served from - // Bun's failed-statement cache, not re-prepared on the server. A fresh prepare - // would return an identical error for identical SQL and silently satisfy every - // assertion below without exercising the cached-slice path. - const [{ Value: preparesBefore }] = await sql.unsafe("SHOW SESSION STATUS LIKE 'Com_stmt_prepare'").simple(); - // err1 and errOverwrite each reached COM_STMT_PREPARE, so the counter is - // already non-zero here; if it were 0 the "no increment" check below would - // be vacuous because the prepared path was never taken. - expect(Number(preparesBefore)).toBeGreaterThan(0); - const err2 = await sql`wat ${1} ${sql.unsafe(longA)}`.catch((x: any) => x); - const [{ Value: preparesAfter }] = await sql.unsafe("SHOW SESSION STATUS LIKE 'Com_stmt_prepare'").simple(); - expect({ - code: err2.code, - errno: err2.errno, - sqlState: err2.sqlState, - message: err2.message, - preparesAfter: Number(preparesAfter), - }).toEqual({ - code: err1.code, - errno: err1.errno, - sqlState: err1.sqlState, - message: err1.message, - preparesAfter: Number(preparesBefore), - }); - expect(err2.message).toContain(longA); - expect(err2.message).not.toContain(longZ); + // 3. The migration lands. The same text on the same connection must now + // prepare successfully and return rows. + await sql.unsafe(`CREATE TABLE \`${table}\` (n INT)`).simple(); + await sql.unsafe(`INSERT INTO \`${table}\` VALUES (42)`).simple(); + const beforeThird = await prepares(); + expect(await sql`SELECT n FROM ${sql(table)}`).toEqual([{ n: 42 }]); + expect(await prepares()).toBe(beforeThird + 1); + + // 4. Only Failed entries are evicted: the now-Prepared statement is + // served from the cache, so the counter does not move. + expect(await sql`SELECT n FROM ${sql(table)}`).toEqual([{ n: 42 }]); + expect(await prepares()).toBe(beforeThird + 1); + } finally { + await sql + .unsafe(`DROP TABLE IF EXISTS \`${table}\``) + .simple() + .catch(() => {}); + } }); }); } diff --git a/test/js/sql/sql-mysql-failed-prepare-retry.test.ts b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts new file mode 100644 index 000000000000..ca0818beee05 --- /dev/null +++ b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts @@ -0,0 +1,117 @@ +// Regression test: MySQLConnection cached a prepared statement whose +// COM_STMT_PREPARE failed (status = .failed) in the per-connection statement +// map and never evicted it, so every later execution of the same query text on +// that connection rethrew the stale ErrorPacket without ever re-preparing. +// Transient prepare failures are normal (a table that appears after a +// migration, deadlocks, ER_TOO_MANY_CONCURRENT_STMTS); with pooling this +// poisons a connection for the process lifetime. +// +// The oracle is the number of COM_STMT_PREPARE frames the client emits for one +// query text, so the server here is a scripted mock that observes the client's +// outbound frames directly: the first prepare of the text fails, every later +// prepare of it succeeds. A real container cannot make the same prepare fail +// once and then succeed without an out-of-band DDL racing the client. +// All wire-protocol bytes come from test/js/sql/wire-frames.ts; do not inline +// Buffer.alloc frame construction here. + +import { SQL } from "bun"; +import { expect, test } from "bun:test"; +import type { Socket } from "node:net"; +import { + listeningServer, + mysqlErrorPacket, + mysqlHandshakeV10, + mysqlOkPacket, + mysqlReadPackets, + mysqlStmtPrepareOk, +} from "./wire-frames"; + +const COM_QUIT = 0x01; +const COM_STMT_PREPARE = 0x16; +const COM_STMT_EXECUTE = 0x17; +const COM_STMT_CLOSE = 0x19; + +test("MySQL: a failed prepare is evicted from the statement cache and retried", async () => { + // First COM_STMT_PREPARE for a given text answers ERR 1146 (table missing), + // every later one answers OK. COM_STMT_EXECUTE always answers OK. + const preparesByText = new Map(); + let connections = 0; + let stmtId = 0; + const sockets = new Set(); + const { server, port } = await listeningServer(socket => { + connections++; + sockets.add(socket); + let buffered = Buffer.alloc(0); + let authed = false; + socket.write(mysqlHandshakeV10()); + socket.on("data", chunk => { + buffered = mysqlReadPackets(Buffer.concat([buffered, chunk]), (seq, payload) => { + if (!authed) { + authed = true; + socket.write(mysqlOkPacket(seq + 1)); + return; + } + const cmd = payload[0]; + if (cmd === COM_STMT_PREPARE) { + const text = payload.subarray(1).toString("utf-8"); + const n = (preparesByText.get(text) ?? 0) + 1; + preparesByText.set(text, n); + if (n === 1) { + socket.write(mysqlErrorPacket(1, 1146, "42S02", "Table 'db.t' doesn't exist")); + } else { + socket.write(mysqlStmtPrepareOk(1, ++stmtId, 0, 0)); + } + } else if (cmd === COM_STMT_EXECUTE) { + socket.write(mysqlOkPacket(1)); + } else if (cmd === COM_STMT_CLOSE) { + // COM_STMT_CLOSE expects no response. + } else if (cmd === COM_QUIT) { + socket.end(); + } else { + socket.end(); + } + }); + }); + socket.on("error", () => {}); + socket.on("close", () => sockets.delete(socket)); + }); + + try { + await using sql = new SQL({ url: `mysql://root@127.0.0.1:${port}/db`, max: 1 }); + + const settled = (q: Promise) => + q.then( + value => ({ status: "fulfilled", value }) as const, + reason => ({ status: "rejected", reason }) as const, + ); + + // 1. The prepare fails with a transient server error. + const first = await settled(sql`SELECT * FROM t`); + expect(first.status).toBe("rejected"); + expect((first as any).reason).toMatchObject({ errno: 1146, code: "ERR_MYSQL_SERVER_ERROR" }); + + // 2. The same text again on the same connection. Before the fix the stale + // ErrorPacket was replayed from the statement cache and the server never + // saw a second COM_STMT_PREPARE; after the fix it re-prepares and runs. + const second = await settled(sql`SELECT * FROM t`); + + // 3. Same text a third time: the now-Prepared statement IS served from the + // cache, proving only Failed entries are evicted, not the cache itself. + const third = await settled(sql`SELECT * FROM t`); + + expect({ + connections, + prepares: preparesByText.get("SELECT * FROM t"), + second: second.status, + third: third.status, + }).toEqual({ + connections: 1, + prepares: 2, + second: "fulfilled", + third: "fulfilled", + }); + } finally { + for (const s of sockets) s.destroy(); + await new Promise(resolve => server.close(() => resolve())); + } +}); diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 4a3e160cb19b..931818168afa 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -763,13 +763,15 @@ if (isDockerEnabled()) { expect(err.code).toBe("ERR_MYSQL_SYNTAX_ERROR"); }); - // Regression: the error_message stored on a cached failed prepared statement - // was a .temporary slice into the socket read buffer. Re-running the same - // failing query after other queries overwrote the buffer would read garbage - // (or crash under ASAN) when constructing the error from the cached statement. - test("Cached failed prepared statement returns stable error message", async () => { + // Regression: the error_message held on a failed prepared statement was a + // .temporary slice into the socket read buffer, so re-running the same + // failing query after other traffic overwrote the buffer returned garbage + // (or crashed under ASAN). A failed prepare is now also evicted from the + // statement cache, so the second attempt re-prepares; the server must + // answer with the same error either way. + test("Re-running a failing prepared statement returns a stable error message", async () => { await using sql = new SQL({ ...getOptions(), max: 1 }); - // Need a parameter so it goes through the prepared-statement cache path. + // Need a parameter so it goes through the prepared-statement path. const err1 = await sql`wat ${1}`.catch(x => x); expect(err1.code).toBe("ERR_MYSQL_SYNTAX_ERROR"); expect(typeof err1.message).toBe("string"); @@ -783,7 +785,7 @@ if (isDockerEnabled()) { expect(rows[0].x).toBe(filler); } - // Hitting the cached .failed statement must reproduce the same error. + // The re-prepare of the same text must reproduce the same error. const err2 = await sql`wat ${1}`.catch(x => x); expect({ code: err2.code, diff --git a/test/js/sql/wire-frames.ts b/test/js/sql/wire-frames.ts index 221241102030..4323be7e6c89 100644 --- a/test/js/sql/wire-frames.ts +++ b/test/js/sql/wire-frames.ts @@ -256,6 +256,18 @@ export function mysqlAuthSwitchRequest(seq: number, pluginName: string, pluginDa return mysqlRawPacket(seq, Buffer.concat([Buffer.from([0xfe]), Buffer.from(pluginName + "\0"), pluginData])); } +// MySQL ERR_Packet (CLIENT_PROTOCOL_41) — page_protocol_basic_err_packet.html: +// Int<1>(0xff) Int<2>(error_code) Byte1('#') String<5>(sql_state) String(error_message) +export function mysqlErrorPacket(seq: number, errorCode: number, sqlState: string, message: string): Buffer { + const fixed = Buffer.alloc(3); + fixed[0] = 0xff; + fixed.writeUInt16LE(errorCode, 1); + return mysqlRawPacket( + seq, + Buffer.concat([fixed, Buffer.from("#"), Buffer.from(sqlState, "latin1"), Buffer.from(message, "utf-8")]), + ); +} + // MySQL length-encoded integer — page_protocol_basic_dt_integers.html#sect_protocol_basic_dt_int_le: // <0xfb 1B; 0xfc + Int<2>; 0xfd + Int<3>; 0xfe + Int<8>. export function mysqlLenencInt(n: number | bigint): Buffer { From d6654c246315b7e1c4aa968f31b7d68b380b4d63 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:57:10 +0000 Subject: [PATCH 2/5] ci: retrigger From 1c206caf22debad1590fe286d5b572f0c0f68dd1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:04:49 +0000 Subject: [PATCH 3/5] sql(mysql): keep the failed-prepare comments within the 3-line limit --- src/sql_jsc/mysql/MySQLConnection.rs | 14 ++++++-------- src/sql_jsc/mysql/MySQLQuery.rs | 12 +++++------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index a6bbfaca603f..df1be72cb4f6 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -1238,14 +1238,12 @@ impl MySQLConnection { // `on_error_packet` below. self.flags.insert(ConnectionFlags::IS_READY_FOR_QUERY); statement.status = mysql_statement::Status::Failed; - // err.error_message is a Data{ .temporary = ... } slice into the socket read - // buffer which will be overwritten by the next packet. Queries that attached - // to this statement before the failure read stmt.error_response later, so we - // must own a copy of the message bytes. - // ErrorPacket lacks Clone in bun_sql (Data is not Clone), so - // reconstruct field-by-field with an owned dupe of the message - // — the scalar fields (header / error_code / sql_state) are - // all Copy. + // err.error_message is a temporary slice into the socket read buffer that + // the next packet overwrites, and queries that attached to this statement + // before the failure read stmt.error_response later, so own a copy of it. + // ErrorPacket lacks Clone in bun_sql (Data is not Clone), so rebuild it + // field-by-field with an owned dupe of the message; the scalar fields + // (header / error_code / sql_state) are all Copy. statement.error_response = ErrorPacket { header: err.header, error_code: err.error_code, diff --git a/src/sql_jsc/mysql/MySQLQuery.rs b/src/sql_jsc/mysql/MySQLQuery.rs index 21c87d18f845..e610ccc9893d 100644 --- a/src/sql_jsc/mysql/MySQLQuery.rs +++ b/src/sql_jsc/mysql/MySQLQuery.rs @@ -339,14 +339,12 @@ impl MySQLQuery { }; if entry.found_existing { + // A cached entry is never `Failed`: handle_prepared_statement evicts a + // failed prepare from the map; the `match` below rejects on `Failed`. let stmt: *mut MySQLStatement = *entry.value_ptr; - // `found_existing` ⇒ the map already holds a live, ref-counted - // `*mut MySQLStatement` (separate heap allocation, never aliases - // `*self`); this thread is the only mutator. `ref_()` is `&self`, - // so a `ParentRef` deref covers the former raw `(*stmt).…` deref. - // A cached entry is never `Failed`: handle_prepared_statement - // evicts the statement from the map when its prepare errors, and - // the `match` below rejects on any `Failed` status regardless. + // The map holds a live, ref-counted `*mut MySQLStatement` (separate heap + // allocation, never aliases `*self`; this thread is the only mutator), + // so a `ParentRef` covers the former raw `(*stmt).…` deref in `ref_()`. let stmt_ref = bun_ptr::ParentRef::from( core::ptr::NonNull::new(stmt).expect("found_existing ⇒ non-null map entry"), ); From b74b3a8d16f13af827fe1293f1ba0cdbb36d411e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:14:46 +0000 Subject: [PATCH 4/5] sql(mysql): settle a query rejected from advance() instead of leaving it pending When two identical queries start in the same synchronous turn, the second attaches to the first's in-flight statement. If that shared prepare fails, advance() re-runs the second query, which hits the Failed status arm and returns an error. JSMySQLQuery::run's errguard then set the query's status to Fail on unwind, and reject_with_js_value's settle-once guard saw fail() return false and returned before ever invoking the JS reject callback, so the second query's promise was never settled. Move the fail() out of run()'s errguard: the guard now only rolls back the this_value upgrade, matching PostgresSQLQuery::run. The advance()-driven caller settles through reject_with_js_value, which owns the fail() transition; the do_run caller propagates the exception into JS (query.ts rejects the promise there) and marks the native query terminal itself. This also fixes the rejection value on that path: it now goes through onRejectMySQLQuery's wrapError and is a MySQLError rather than the raw error-options object thrown by run_prepared_query. The new concurrent test in sql-mysql-failed-prepare-retry.test.ts times out on the unfixed build (the second promise never settles) and passes with the fix. --- src/sql_jsc/mysql/JSMySQLQuery.rs | 8 +- .../sql-mysql-failed-prepare-retry.test.ts | 125 +++++++++++++----- 2 files changed, 99 insertions(+), 34 deletions(-) diff --git a/src/sql_jsc/mysql/JSMySQLQuery.rs b/src/sql_jsc/mysql/JSMySQLQuery.rs index bfdb3025ea48..2e656ac9ab59 100644 --- a/src/sql_jsc/mysql/JSMySQLQuery.rs +++ b/src/sql_jsc/mysql/JSMySQLQuery.rs @@ -165,6 +165,10 @@ impl JSMySQLQuery { } this.set_target(target); if let Err(err) = this.run(connection) { + // The thrown exception propagates out of this host function and + // internal/sql/query.ts rejects the promise, so the native query is + // marked terminal here rather than in run()'s errguard (see there). + this.mark_as_failed(); if !global_object.has_exception() { return Err(global_object.throw_value(mysql_error_to_js( global_object, @@ -411,8 +415,10 @@ impl JSMySQLQuery { // value, mutation is `JsCell`-backed, and `into_inner` disarms on the // success path below. let errguard = scopeguard::guard(self, |s| { + // `query.fail()` is deliberately not here (PostgresSQLQuery::run + // matches): the advance()-driven caller settles the promise via + // `reject_with_js_value`, whose once-guard no-ops on a failed query. s.this_value.with_mut(|v| v.downgrade()); - let _ = s.query.with_mut(|q| q.fail()); }); let columns_value = self.get_columns().unwrap_or(JSValue::UNDEFINED); diff --git a/test/js/sql/sql-mysql-failed-prepare-retry.test.ts b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts index ca0818beee05..c6f96b637e9d 100644 --- a/test/js/sql/sql-mysql-failed-prepare-retry.test.ts +++ b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts @@ -1,18 +1,22 @@ -// Regression test: MySQLConnection cached a prepared statement whose -// COM_STMT_PREPARE failed (status = .failed) in the per-connection statement -// map and never evicted it, so every later execution of the same query text on -// that connection rethrew the stale ErrorPacket without ever re-preparing. -// Transient prepare failures are normal (a table that appears after a -// migration, deadlocks, ER_TOO_MANY_CONCURRENT_STMTS); with pooling this -// poisons a connection for the process lifetime. +// Regression tests for how a failed COM_STMT_PREPARE is handled, against a +// scripted MySQL server. All wire-protocol bytes come from +// test/js/sql/wire-frames.ts; do not inline Buffer.alloc frame construction. +// +// 1. MySQLConnection cached a prepared statement whose prepare failed +// (status = .failed) in the per-connection statement map and never evicted +// it, so every later execution of the same query text on that connection +// rethrew the stale ErrorPacket without ever re-preparing. Transient +// prepare failures are normal (a table that appears after a migration, +// deadlocks, ER_TOO_MANY_CONCURRENT_STMTS); with pooling this poisons a +// connection for the process lifetime. +// 2. A second identical query started in the same synchronous turn attaches to +// the first's in-flight statement. When the shared prepare failed, the +// second query's promise was never settled (see the second test). // // The oracle is the number of COM_STMT_PREPARE frames the client emits for one -// query text, so the server here is a scripted mock that observes the client's -// outbound frames directly: the first prepare of the text fails, every later -// prepare of it succeeds. A real container cannot make the same prepare fail -// once and then succeed without an out-of-band DDL racing the client. -// All wire-protocol bytes come from test/js/sql/wire-frames.ts; do not inline -// Buffer.alloc frame construction here. +// query text, so the server is a mock that observes the client's outbound +// frames directly: a real container cannot make the same prepare fail once and +// then succeed without an out-of-band DDL racing the client. import { SQL } from "bun"; import { expect, test } from "bun:test"; @@ -31,12 +35,15 @@ const COM_STMT_PREPARE = 0x16; const COM_STMT_EXECUTE = 0x17; const COM_STMT_CLOSE = 0x19; -test("MySQL: a failed prepare is evicted from the statement cache and retried", async () => { - // First COM_STMT_PREPARE for a given text answers ERR 1146 (table missing), - // every later one answers OK. COM_STMT_EXECUTE always answers OK. +/** + * A scripted MySQL server: handshake, OK for the auth response, then routes + * each COM_STMT_PREPARE through `onPrepare(text, nth)` (nth is 1-based per + * distinct query text) and answers every COM_STMT_EXECUTE with an OK packet. + * Call `stop()` in a `finally`. + */ +async function mockMySQLServer(onPrepare: (text: string, nth: number) => Buffer) { const preparesByText = new Map(); let connections = 0; - let stmtId = 0; const sockets = new Set(); const { server, port } = await listeningServer(socket => { connections++; @@ -56,11 +63,7 @@ test("MySQL: a failed prepare is evicted from the statement cache and retried", const text = payload.subarray(1).toString("utf-8"); const n = (preparesByText.get(text) ?? 0) + 1; preparesByText.set(text, n); - if (n === 1) { - socket.write(mysqlErrorPacket(1, 1146, "42S02", "Table 'db.t' doesn't exist")); - } else { - socket.write(mysqlStmtPrepareOk(1, ++stmtId, 0, 0)); - } + socket.write(onPrepare(text, n)); } else if (cmd === COM_STMT_EXECUTE) { socket.write(mysqlOkPacket(1)); } else if (cmd === COM_STMT_CLOSE) { @@ -75,15 +78,35 @@ test("MySQL: a failed prepare is evicted from the statement cache and retried", socket.on("error", () => {}); socket.on("close", () => sockets.delete(socket)); }); + return { + port, + preparesByText, + connections: () => connections, + async stop() { + for (const s of sockets) s.destroy(); + await new Promise(resolve => server.close(() => resolve())); + }, + }; +} - try { - await using sql = new SQL({ url: `mysql://root@127.0.0.1:${port}/db`, max: 1 }); +const tableMissing = () => mysqlErrorPacket(1, 1146, "42S02", "Table 'db.t' doesn't exist"); + +const settled = (q: Promise) => + q.then( + value => ({ status: "fulfilled", value }), + reason => ({ status: "rejected", reason }), + ); + +test("MySQL: a failed prepare is evicted from the statement cache and retried", async () => { + // First COM_STMT_PREPARE for a given text answers ERR 1146 (table missing), + // every later one answers OK. + let stmtId = 0; + const mock = await mockMySQLServer((_text, nth) => + nth === 1 ? tableMissing() : mysqlStmtPrepareOk(1, ++stmtId, 0, 0), + ); - const settled = (q: Promise) => - q.then( - value => ({ status: "fulfilled", value }) as const, - reason => ({ status: "rejected", reason }) as const, - ); + try { + await using sql = new SQL({ url: `mysql://root@127.0.0.1:${mock.port}/db`, max: 1 }); // 1. The prepare fails with a transient server error. const first = await settled(sql`SELECT * FROM t`); @@ -100,8 +123,8 @@ test("MySQL: a failed prepare is evicted from the statement cache and retried", const third = await settled(sql`SELECT * FROM t`); expect({ - connections, - prepares: preparesByText.get("SELECT * FROM t"), + connections: mock.connections(), + prepares: mock.preparesByText.get("SELECT * FROM t"), second: second.status, third: third.status, }).toEqual({ @@ -111,7 +134,43 @@ test("MySQL: a failed prepare is evicted from the statement cache and retried", third: "fulfilled", }); } finally { - for (const s of sockets) s.destroy(); - await new Promise(resolve => server.close(() => resolve())); + await mock.stop(); + } +}); + +// Two identical queries started in the same synchronous turn share one prepare: +// the second attaches to the first's in-flight (Parsing) statement before the +// server answers. When that shared prepare failed, advance() re-ran the second +// query, JSMySQLQuery::run's errguard marked it failed on unwind, and +// reject_with_js_value's settle-once guard then returned without ever invoking +// the reject callback, leaving the second query's promise pending forever. +test("MySQL: a concurrent query sharing a failed prepare is rejected, not left pending", async () => { + // Every COM_STMT_PREPARE for the text answers ERR 1146, so the only correct + // outcome for BOTH queries is a rejection carrying that error. + const mock = await mockMySQLServer(() => tableMissing()); + + try { + await using sql = new SQL({ url: `mysql://root@127.0.0.1:${mock.port}/db`, max: 1 }); + + const results = await Promise.all( + [sql`SELECT * FROM t`, sql`SELECT * FROM t`].map(q => + q.then( + () => ({ status: "fulfilled" }), + (e: any) => ({ status: "rejected", isError: e instanceof Error, errno: e?.errno }), + ), + ), + ); + + // `prepares: 1` proves the second query shared the first's prepare attempt + // instead of issuing its own COM_STMT_PREPARE. + expect({ results, prepares: mock.preparesByText.get("SELECT * FROM t") }).toEqual({ + results: [ + { status: "rejected", isError: true, errno: 1146 }, + { status: "rejected", isError: true, errno: 1146 }, + ], + prepares: 1, + }); + } finally { + await mock.stop(); } }); From 2e3dc4dd49bd80abd9e67287bb580682cae118a2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:23:03 +0000 Subject: [PATCH 5/5] test(sql): run the failed-prepare tests concurrently and trim a comment The two tests each start their own mock server on an ephemeral port with no shared state, so they can run as test.concurrent. The comment above the second test narrated the bug's mechanism, which belongs in the PR description; it now states only the invariant the test asserts. --- test/js/sql/sql-mysql-failed-prepare-retry.test.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/js/sql/sql-mysql-failed-prepare-retry.test.ts b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts index c6f96b637e9d..939e5b07e881 100644 --- a/test/js/sql/sql-mysql-failed-prepare-retry.test.ts +++ b/test/js/sql/sql-mysql-failed-prepare-retry.test.ts @@ -97,7 +97,7 @@ const settled = (q: Promise) => reason => ({ status: "rejected", reason }), ); -test("MySQL: a failed prepare is evicted from the statement cache and retried", async () => { +test.concurrent("MySQL: a failed prepare is evicted from the statement cache and retried", async () => { // First COM_STMT_PREPARE for a given text answers ERR 1146 (table missing), // every later one answers OK. let stmtId = 0; @@ -138,13 +138,10 @@ test("MySQL: a failed prepare is evicted from the statement cache and retried", } }); -// Two identical queries started in the same synchronous turn share one prepare: -// the second attaches to the first's in-flight (Parsing) statement before the -// server answers. When that shared prepare failed, advance() re-ran the second -// query, JSMySQLQuery::run's errguard marked it failed on unwind, and -// reject_with_js_value's settle-once guard then returned without ever invoking -// the reject callback, leaving the second query's promise pending forever. -test("MySQL: a concurrent query sharing a failed prepare is rejected, not left pending", async () => { +// Two identical queries started in the same synchronous turn share one prepare +// (the second attaches to the first's in-flight statement); a shared failure +// must reject both, not leave one pending forever. +test.concurrent("MySQL: a concurrent query sharing a failed prepare is rejected, not left pending", async () => { // Every COM_STMT_PREPARE for the text answers ERR 1146, so the only correct // outcome for BOTH queries is a rejection carrying that error. const mock = await mockMySQLServer(() => tableMissing());