mysql: route query writes through advance() so wire order matches queue order - #32008
mysql: route query writes through advance() so wire order matches queue order#32008robobun wants to merge 5 commits into
Conversation
…ue order JSMySQLQuery::do_run wrote a query's packets optimistically before enqueueing it, while responses are matched to requests in FIFO queue order. A write from the enqueue path could jump ahead of an earlier queued-but-unwritten request, making that request consume the new query's response packets and desyncing the connection. do_run now only enqueues; the queue's advance() walk (pumped by the auto-flusher and the response handlers) performs every write in queue order, same as the postgres fix in #32006. This also fixes a hang: when run() failed for an already-queued query (e.g. queued behind an in-flight COM_STMT_PREPARE of the same statement whose prepare then failed), run()'s error guard pre-marked the query as failed, so reject_with_js_value's settle-once gate dropped the rejection and the promise never settled. run() no longer marks the query failed; the reject path owns that transition. Fixes #32005
|
Updated 3:05 AM PT - Jun 10th, 2026
❌ @robobun, your commit 3d07e55 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32008That installs a local version of the PR into your bun-32008 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughEnqueue MySQL requests before executing them and delegate run() error settlement to the caller so all writes/response matching go through the FIFO advance path. Adds a mock MySQL server and three regression tests validating prepare-failure propagation and FIFO write/drain ordering. ChangesMySQL native request queue ordering
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-queue-write-order.test.ts`:
- Around line 167-183: Change the test to create a real queue barrier before
issuing the simple-protocol queries: start by issuing a prepared/parameterized
request that will be held in-flight by the mockServer (e.g. call
sql.prepare(...) or sql.query(...) with params and ensure the mockServer delays
responding to that prepare), then while that prepare is still in-flight create
the simple-protocol queries with sql.unsafe(...) and call (q as any).execute()
for each; this ensures one request is actually parked behind the in-flight
prepare and reproduces the queued-but-unwritten window so the subsequent
expect(wireLog) assertion will fail on the buggy path. Use the existing
mockServer, SQL constructor, sql.prepare/sql.query and sql.unsafe symbols to
locate and modify the test.
🪄 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: 16710c2f-d267-491b-861c-ff9ef788eddf
📒 Files selected for processing (2)
src/sql_jsc/mysql/JSMySQLQuery.rstest/js/sql/sql-mysql-queue-write-order.test.ts
The mock can now hold the first COM_STMT_PREPARE response until the test releases it, so queries issued during that window park in the native queue behind an in-flight request. The wire log must show them only after the release marker, in issuance order.
|
Checked the find-issues suggestions against local runs (MariaDB 11.8 on 127.0.0.1, pre-fix released builds vs this branch):
So no auto-close lines for these three. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-queue-write-order.test.ts`:
- Around line 89-90: The heldPrepare promise (created via let onPrepareHeld /
const heldPrepare = new Promise<() => void>(resolve => (onPrepareHeld =
resolve))) never rejects on socket/server failure when holdPrepare is enabled;
wire the mock socket/server error and close handlers to reject that promise so
tests fail fast. Concretely, change the heldPrepare construction to capture both
resolve and reject, then in the mock's 'error' and 'close' (and any abort/exit)
handlers call the captured reject with an explanatory Error when holdPrepare is
true; also ensure any existing onPrepareHeld resolve still works and that
handlers are removed after resolve/reject to avoid leaks. This targets the
heldPrepare/onPrepareHeld logic and the mock's error/close paths used when
holdPrepare is toggled.
🪄 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: 4ad3843a-aa9c-48bf-9497-dc65d8b4990f
📒 Files selected for processing (1)
test/js/sql/sql-mysql-queue-write-order.test.ts
If the handshake or the held COM_STMT_PREPARE never arrives, the awaiting test now fails immediately with a message instead of burning the per-test timeout.
|
CI status for build 61701 (final): 283 of 286 test/build jobs passed, including every lane that runs the sql tests. The three non-green jobs are all unrelated to this diff:
Earlier reds on this PR were the bunx @angular/cli@latest breakage (since skipped on main in #32042 and merged into this branch). The one retrigger for this branch is spent. The diff (src/sql_jsc/mysql + one sql test file) is unchanged since e14fba9 and ready for review. |
|
The window this PR closes is reachable through public APIs after all, and it produces wrong results rather than just a hang, so this is worth landing on its own merits.
const sql = new SQL({ url, max: 1 });
let nested, reads = 0;
const values = [0];
Object.defineProperty(values, "0", {
get() {
// 1st read: Signature.generate(); 2nd read: bind()
if (++reads === 2) (nested = sql.unsafe("select ? as y", ["nested"])).execute();
return 42;
},
});
await sql.unsafe("select ? as x", values); // resolves [] (consumed the nested COM_STMT_PREPARE reply)
await nested; // rejects ERR_MYSQL_UNEXPECTED_PACKET, connection closedObserved on main (b5afcac, debug build) and on the released canary, against MariaDB 11.8 (the trigger and the misattribution are client side; only the garbage values in row 2 would differ by server):
With this PR's diff merged onto current main every shape above gets its own result (7/7 pass; 5/7 fail on main alone), which follows from the fix: the nested Two things for whoever picks this up:
Not opening a separate PR for this; the fix is this one. |
1 participant
Fixes #32005. MySQL twin of the postgres native-queue fix in #32006 (no file overlap with that PR; the pool-level change there already covers the MySQL adapter).
Problem
The MySQL request queue matches server responses to requests strictly in FIFO queue order (
handle_commandreadsqueue.current()), so query packets must reach the wire in that same order.JSMySQLQuery::do_runbroke that invariant structurally: it calledthis.run(connection), which writes COM_QUERY / Bind+Execute / COM_STMT_PREPARE optimistically, beforeenqueue_request. A write from the enqueue path can jump ahead of an earlier queued-but-unwritten request, and the bypassed request then consumes the new query's response packets, permanently desyncing the connection. The gates (can_pipelineetc.) make that window hard to hit from public APIs today, but nothing enforces the ordering.The optimistic path also had a concrete user-visible bug, reproducible on main: when
run()failed for a query that was already queued, the promise never settled.run()'s error guard pre-marked the query as failed, soon_error -> reject_with_js_valuehit the settle-oncefail()gate, concluded the query was already settled, and silently dropped the rejection:Fix (
src/sql_jsc/mysql/JSMySQLQuery.rs)do_runno longer callsrun(); it upgrades the wrapper's ref (so the cached target/binding/columns survive GC while queued, asrun()used to do) and enqueues.enqueue_requestalready registers the auto-flusher, whosedrain_internal -> flush_queue -> advance()performs the write; on a busy connection the response handlers advance the queue as before.advance()walks the queue FIFO and already enforces every ordering barrier, so wire order now matches queue order by construction.run()no longer marks the query failed on error. Its only caller is nowadvance(), which routes every failure toon_error -> reject; the reject path owns the Fail transition and the ref downgrade, and its settle-once gate stays open so the rejection is actually delivered.Wire behavior is otherwise unchanged: the old do_run write only reached the socket at the auto-flusher anyway, and MySQL's gates never allowed writing behind an in-flight request (
can_pipelinerequiresis_ready_for_query), so routing throughadvance()removes no pipelining.Tests
test/js/sql/sql-mysql-queue-write-order.test.ts(mock MySQL server, no Docker):Other verification on the fixed debug (ASAN) build:
sql-mysql-cached-error,clean-reentry,raw-length-prefix,datetime-roundtrip,columns-realloc-oom,auth-short-nonce,tls-plaintext-injection)sql-mysql-bind-oob(its bind failure now fires on the advance() path and still rejects withERR_MYSQL_WRONG_NUMBER_OF_PARAMETERS_PROVIDED),sql-mysql-bind-blob-borrow, plus ordering/stress scripts: 1200 mixed ops (transactions + concurrent pooled simple/prepared queries, pool max 2) and a 3000-query continuation storm, 0 mismatches, andBUN_JSC_validateExceptionChecks=1clean on the new tests.