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

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

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. 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.
-                // 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,
@@ -1254,6 +1252,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

@@ -339,22 +339,15 @@ 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. 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.
+                // 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"),
                 );
-                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,173 @@
+// 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 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";
+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;
+
+/**
+ * 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<string, number>();
+  let connections = 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);
+          socket.write(onPrepare(text, n));
+        } 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));
+  });
+  return {
+    port,
+    preparesByText,
+    connections: () => connections,
+    async stop() {
+      for (const s of sockets) s.destroy();
+      await new Promise<void>(resolve => server.close(() => resolve()));
+    },
+  };
+}
+
+const tableMissing = () => mysqlErrorPacket(1, 1146, "42S02", "Table 'db.t' doesn't exist");
+
+const settled = (q: Promise<any>) =>
+  q.then(
+    value => ({ status: "fulfilled", value }),
+    reason => ({ status: "rejected", reason }),
+  );
+
+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;
+  const mock = await mockMySQLServer((_text, nth) =>
+    nth === 1 ? tableMissing() : mysqlStmtPrepareOk(1, ++stmtId, 0, 0),
+  );
+
+  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`);
+    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: mock.connections(),
+      prepares: mock.preparesByText.get("SELECT * FROM t"),
+      second: second.status,
+      third: third.status,
+    }).toEqual({
+      connections: 1,
+      prepares: 2,
+      second: "fulfilled",
+      third: "fulfilled",
+    });
+  } finally {
+    await mock.stop();
+  }
+});
+
+// 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());
+
+  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();
+  }
+});