sql(mysql): retry a failed prepare instead of caching the error forever

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

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

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(() => {});
+      }
     });
   });
 }

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<string, number>();
+  let connections = 0;
+  let stmtId = 0;
+  const sockets = new Set<Socket>();
+  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<any>) =>
+      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<void>(resolve => server.close(() => resolve()));
+  }
+});

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,

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<EOF>(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 {