sql(mysql): retry a failed prepare instead of caching the error forever - #33189
sql(mysql): retry a failed prepare instead of caching the error forever#33189robobun wants to merge 5 commits into
Conversation
…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.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughMySQL prepared-statement failure handling now evicts failed cache entries, query execution defers terminal failure handling to the shared status path, and the regression tests were updated to verify re-prepare behavior with both a mock server and a live MySQL connection. ChangesMySQL prepared statement eviction and retry
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — the eviction mirrors the Postgres handler at PostgresSQLConnection.rs:2945 and the refcount reasoning checks out — but this adds an unsafe intrusive-deref on a hot driver path and rewrites sql-mysql-cached-error.test.ts to assert the opposite invariant, so it's worth a human look.
Extended reasoning...
Overview
The PR touches the MySQL driver's prepared-statement cache: MySQLConnection::handle_prepared_statement now evicts a statement from self.statements and derefs the map's intrusive ref when the server answers COM_STMT_PREPARE with an ERR packet, and MySQLQuery::run_prepared_query drops the now-unreachable found_existing && Failed fast-throw. Test coverage is thorough: a new wire-frame mock test asserts prepare counts and reuse, the container test in sql-mysql-cached-error.test.ts is rewritten to assert re-prepare behavior against a real server, and wire-frames.ts gains mysqlErrorPacket.
Security risks
None identified. No new user-controlled input reaches native code; the change only affects when an already-parsed server ERR packet causes cache eviction. No auth, TLS, or path handling is touched.
Level of scrutiny
This deserves careful human review despite being small. It sits squarely in the category CLAUDE.md flags as most-blocked: an unsafe { MySQLStatement::deref(...) } on a live &mut borrow inside a driver hot path, with correctness depending on the invariant that the request always holds an independent ref (which I verified — init_exact_refs(2) on insert, ref_() on found_existing). The pattern is a near-verbatim copy of PostgresSQLConnection.rs:2937-2951, which is reassuring, but the MySQL side lacks the status == Parsing guard the Postgres side has — here the ERROR arm is only reached via the Parsing match in handle_command plus statement_id == 0, so it's equivalent, but a reviewer should confirm.
Other factors
The bigger judgment call is that sql-mysql-cached-error.test.ts is rewritten to assert the opposite of what it previously pinned (that Com_stmt_prepare now does increment on retry). The PR description justifies this — the old test existed to catch a dangling-slice read on a code path this PR deletes, and the dangling-slice concern is still covered by the (now comment-only-updated) test in sql-mysql.test.ts — but per the repo's landing guidance, inverting an existing test's invariant is exactly the kind of change a maintainer should sign off on rather than a bot.
|
Good flags, both worth pre-answering for whoever picks this up. The missing The Postgres handler checks the pre-transition status and wraps both the if stmt.status == StatementStatus::Parsing {
stmt.status = StatementStatus::Failed;
// ...
if self.statements.with_mut(|m| m.remove(..)).is_some() { ... }
}On the MySQL side that precondition is established one frame up instead of at the eviction site, so the guard would be redundant here. Concretely:
A literal copy of the Postgres check would also read the wrong thing at that point: by the time the eviction block runs, One more invariant worth stating explicitly: the entry removed by Inverting the test invariant The The property that test file was originally added for in #29847 (the stored |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sql_jsc/mysql/MySQLConnection.rs`:
- Around line 1242-1244: The comment in MySQLConnection should be shortened to
fit the 3-line maximum. Update the explanatory text around the statement error
response handling so it stays concise while still covering the owned copy
requirement; keep the change localized near the stmt.error_response logic and
preserve the meaning in fewer words.
In `@src/sql_jsc/mysql/MySQLQuery.rs`:
- Around line 345-349: Condense the explanatory comment in
MySQLQuery::handle_prepared_statement to fit within 3 lines while preserving the
key points about `ref_()`, `ParentRef` deref behavior, and the cached entry
never being `Failed`; remove redundant wording and keep only the essential
rationale near the existing `match`/prepare-error handling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb01614b-589a-438b-bc2e-ca3e87e88d02
📒 Files selected for processing (6)
src/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rstest/js/sql/sql-mysql-cached-error.test.tstest/js/sql/sql-mysql-failed-prepare-retry.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/wire-frames.ts
|
CI status: the diff is green. The final matrix for the final sha is build 67601 (267 passed, 19 failed). I pulled the log of every one of the 19 failed test jobs: not one
A few further single-shard one-offs ( I used my one retrigger earlier, so I am not pushing more. Ready for review. |
… 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/sql/sql-mysql-failed-prepare-retry.test.ts (1)
100-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider running these as
test.concurrent.Each test spins up its own independent mock server on a random port with no shared state between them; they're good candidates for concurrent execution.
-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 () => {-test("MySQL: a concurrent query sharing a failed prepare is rejected, not left pending", async () => { +test.concurrent("MySQL: a concurrent query sharing a failed prepare is rejected, not left pending", async () => {As per coding guidelines: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with
test.concurrentordescribe.concurrentunless it's very difficult to make them concurrent."Also applies to: 147-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/sql/sql-mysql-failed-prepare-retry.test.ts` around lines 100 - 139, The MySQL prepare-retry test is independent and can run in parallel with the other cases in this file, so update the `test("MySQL: a failed prepare is evicted from the statement cache and retried", ...)` declaration to use concurrent execution. Apply the same change to the related tests in this block so they all use `test.concurrent` (or an equivalent `describe.concurrent` if grouped), since each one creates its own `mockMySQLServer` and `SQL` instance with no shared state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/sql/sql-mysql-failed-prepare-retry.test.ts`:
- Around line 141-146: Trim the extra narrative comment in this test so it only
states the durable invariant and does not repeat internal failure mechanics
already covered by the file’s opening banner. Update the comment near the shared
prepare retry scenario to remove references to advance(), JSMySQLQuery::run, and
reject_with_js_value, and keep only the high-level behavior this test asserts.
Use the surrounding test name and the top banner as the guide for what should
remain.
---
Outside diff comments:
In `@test/js/sql/sql-mysql-failed-prepare-retry.test.ts`:
- Around line 100-139: The MySQL prepare-retry test is independent and can run
in parallel with the other cases in this file, so update the `test("MySQL: a
failed prepare is evicted from the statement cache and retried", ...)`
declaration to use concurrent execution. Apply the same change to the related
tests in this block so they all use `test.concurrent` (or an equivalent
`describe.concurrent` if grouped), since each one creates its own
`mockMySQLServer` and `SQL` instance with no shared state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08a1d189-de37-445a-8c49-6ac194b29d3b
📒 Files selected for processing (4)
src/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rstest/js/sql/sql-mysql-failed-prepare-retry.test.ts
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.
There was a problem hiding this comment.
No further issues from my side — the concurrent-sibling hang I flagged earlier is fixed and covered. Deferring to a human for the unsafe MySQLStatement::deref on the eviction path and the errguard/mark_as_failed reshuffle, plus the inverted assertions in sql-mysql-cached-error.test.ts.
Extended reasoning...
Overview
This PR fixes two related bugs in the MySQL driver's prepared-statement handling: (1) a failed COM_STMT_PREPARE was cached forever with Status::Failed, so a transient error poisoned the connection for its lifetime; (2) a second identical query that attached to a shared in-flight prepare was left with an unsettled promise when that prepare failed. The fix evicts failed prepares from the per-connection statement map (mirroring the Postgres driver) and moves the fail() transition out of JSMySQLQuery::run's errguard so reject_with_js_value's settle-once guard can fire. Three Rust files are touched (MySQLConnection.rs, MySQLQuery.rs, JSMySQLQuery.rs), one test file is rewritten to assert the new behaviour, one new wire-level test file is added, wire-frames.ts gains mysqlErrorPacket, and one comment block in sql-mysql.test.ts is updated.
Security risks
None identified. The change is confined to client-side caching and error-path settling; no auth, TLS, or input-parsing surface is touched. The new mysqlErrorPacket builder is test-only.
Level of scrutiny
Medium-high. The eviction path adds an unsafe { MySQLStatement::deref(...) } on a &mut borrow of the statement while the request still holds its own ref — the SAFETY comment argues the count cannot reach zero, and I agree, but intrusive-refcount changes in a driver merit a maintainer's eyes. The errguard change (removing q.fail() from run()'s scopeguard and moving it to do_run) alters which caller owns the terminal-status transition on every error path reachable from run(); the PR description traces both callers (do_run and advance()) and I verified the trace, but this is exactly the class of change where a missed caller silently leaves a promise pending.
Other factors
My earlier inline comment (the concurrent-sibling hang) was confirmed, fixed in b74b3a8, and now has a dedicated test that would time out on the unfixed build. All CodeRabbit nits are resolved. sql-mysql-cached-error.test.ts previously asserted the opposite invariant (Com_stmt_prepare must not increment) to pin an unrelated dangling-slice read; the rewrite is justified in the PR body but a human should confirm the old coverage is genuinely subsumed. Test coverage is strong (wire-level mock + real-container), and the fix is symmetric with the Postgres driver's existing behaviour. No CODEOWNERS entry covers these paths.
|
Thanks for the final pass. For whoever picks this up, all three of the deferred points already have written answers:
CI is green for everything this PR touches; the red lanes are a repo-wide |
1 participant
Problem
In
Bun.SQL(MySQL), a prepared statement whoseCOM_STMT_PREPAREthe server answers with an ERR packet is stored in the per-connection statement cache withStatus::Failed. Every later execution of the same query text on that connection finds the cached entry and replays its storedErrorPacketwithout ever re-preparing, so a transient prepare-time error (a table created by a concurrent migration, a deadlock,ER_TOO_MANY_CONCURRENT_STMTS) poisons the connection for the process lifetime. With a pool this looks like "the same query works on some requests and permanently fails on others".mysql2and other clients re-prepare on the next use.Against a scripted server whose first
COM_STMT_PREPAREfor a text answersERR 1146and every later one answers OK:A second, related problem surfaced during review (thanks, claude-bot): if two identical queries start in the same synchronous turn, the second attaches to the first's in-flight statement instead of issuing its own prepare. When that shared prepare fails, the second query's promise is never settled:
Cause
handle_prepared_statement'sPacketType::ERRORarm setsstatement.status = Failedand stores the error packet but never removes the statement fromself.statements, sorun_prepared_queryserves the entry on every later lookup of the same signature hash. The Postgres driver already does this correctly:PostgresSQLConnection'sErrorResponsearm removes the failed statement from the map and drops the map's ref.advance()re-runs the attached second query, which hits theFailedstatus arm and returns an error.JSMySQLQuery::run's errguard set the query's status toFailon unwind, andreject_with_js_value's settle-once guard (if !q.fail() { return }) then returned before ever invoking the JS reject callback.PostgresSQLQuery::run's equivalent guard already only rolls back thethis_valueupgrade.Fix
MySQLConnection::handle_prepared_statement(ERRORarm): remove the failed statement fromself.statementsand deref the map's ref, mirroring the Postgres handler. 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.MySQLQuery::run_prepared_query: delete thefound_existing && Failedguard. The only place that setsStatus::Failednow also removes the entry, so a cached entry is neverFailed; the statusmatcha few lines below already rejects onFailedfor statements a query attached to before the failure.JSMySQLQuery::run: the errguard no longer callsquery.fail(); it only rolls back thethis_valueupgrade, matching Postgres. Theadvance()-driven caller settles throughreject_with_js_value, which owns thefail()transition; thedo_runcaller propagates the exception into JS (internal/sql/query.tsrejects the promise) and marks the native query terminal itself. This also makes that rejection a realMySQLError(viaonRejectMySQLQuery'swrapError) instead of the raw error-options object.Only
Failedentries are affected by the eviction. A successfully prepared statement is still cached and reused without re-preparing.Tests
test/js/sql/sql-mysql-failed-prepare-retry.test.ts(new, scripted wire server):COM_STMT_PREPAREfor a text answersERR 1146, every later one OK. Asserts the second identical query fulfills, the server saw exactly 2 prepares on 1 connection, and a third identical query reuses the now-prepared statement (still 2 prepares). Fails on the unfixed build withprepares: 1, second: rejected, third: rejected.Promise.all, every prepare answersERR 1146. Asserts both reject with a realErrorcarryingerrno: 1146and that exactly one prepare reached the wire. On the unfixed build the test times out because the second promise never settles.test/js/sql/sql-mysql-cached-error.test.ts(rewritten,describeWithContainer): queries a missing table twice on one connection (both prepares fail,Com_stmt_prepareincrements both times), creates the table, and the same text succeeds. This file previously asserted the opposite (Com_stmt_preparemust not increment across an identical re-run) to pin a dangling-slice read on the cache-hit path added in #29847; that path no longer exists.test/js/sql/sql-mysql.test.ts: the "Cached failed prepared statement returns stable error message" case still passes (a fresh prepare of the same text returns the identical error); its comments are updated since the cache-hit path is gone.test/js/sql/wire-frames.ts: adds themysqlErrorPacket(ERR_Packet) builder.Both tests were verified to fail on a build from
main'ssrc/and pass with the fix, and the rest of thetest/js/sqlwire-frame suites still pass.