mysql: hold the request queue refs as RefPtr<JSMySQLQuery>

[robobun]

Problem

• The MySQL request queue (src/sql_jsc/mysql/MySQLRequestQueue.rs) holds one intrusive ref per queued JSMySQLQuery, but stored raw pointers: add() called ref_(), and each of the five removal sites paired it with an unsafe { JSMySQLQuery::deref(request) }.
• The invariant lived in comments. The compiler could not catch a missing or a double release.

Fix

• The element type is now the ref: type Queue = VecDeque<RefPtr<JSMySQLQuery>>. do_run passes this.ref_guard() into enqueue_request, the one place a ref is taken. Each removal is a pop_front() whose result drops, which is the release.
• Correct because a RefPtr owns one count and releases it on Drop, so the queue holds as many refs as it has elements. Re-entrancy handling is unchanged: no borrow of the deque spans a call that can re-enter the queue.
• LinearFifo never runs item destructors and requires T: Copy since valkey: use VecDeque for the command queues; LinearFifo rejects droppable items #39570, so the container becomes a VecDeque.
• Verified: a new test in test/js/sql/sql-mysql-clean-reentry.test.ts drives the queue release paths (five rounds of 10 queued requests rejected by clean(), then GC finalizes the connection). Also bun bd test test/js/sql/sql-mysql.test.ts (99 pass, 7 MariaDB-specific failures identical on unmodified main), 17 other sql-mysql* files, and a smoke script under ASAN (Notes). The change preserves behavior, so the new test passes on main too. It pins the release-exactly-once behavior on both sides.

Background

• JSMySQLQuery is the Rust side of a MySQLQuery JS object, intrusively refcounted: the JS wrapper holds one ref, the queue one per element.
• RefPtr<T> (src/ptr/ref_count.rs) is the owning handle for such a count: Drop releases, Clone takes another ref. ref_guard() returns a new one taken on &self.
• ThisPtr<T> is a non-owning pointer to a live pointee. current_ref() returns one, so callers hold no borrow into the deque.

Notes

On tests: this is the ownership rework requested in review, not a bug fix. The manual ref/deref pairs on main are balanced, so no test can fail on main and pass here. The new test in sql-mysql-clean-reentry.test.ts pins the release-exactly-once behavior on both sides instead: it drives clean() and the GC finalizer five rounds under ASAN, where a double release crashes and a missed release hangs the round.

This PR was stacked on #37665 (OwnedRef). #40478 made RefPtr the RAII owning handle, so the PR is rebased onto main and uses RefPtr in that role. The earlier version kept LinearFifo<OwnedRef<..>> and added LinearFifo::peek_item_ref. Since #39570, LinearFifo rejects droppable element types (T: Copy on every impl), so the queue is a VecDeque now, the same move that PR made for the Valkey queues.

The file had 6 unsafe blocks. It has 0 now. cargo check, cargo clippy and rustfmt are clean for bun_sql_jsc, and cargo check --target x86_64-pc-windows-msvc -p bun_sql_jsc passes.

Smoke script (not committed), run on the debug ASAN build against the container's MariaDB 11.8, output identical to the release build of main:

1. 600 pipelined inserts, selects and simple queries plus a transaction on one connection (add, advance releases completed heads).
2. A SLEEP(0.2) holds the connection, then 120 queued requests of which 60 have an out-of-range BigInt bind. Their run() fails inside advance() and the run-failure path releases them (60 ERR_OUT_OF_RANGE, the rest succeed or reject at bind time).
3. close({ timeout: 0 }) with 50 queued requests: clean() rejects all 50 with ERR_MYSQL_CONNECTION_CLOSED.
4. A reject handler that re-enters close() while clean() drains.
5. Ten rounds of close-with-queued-work followed by Bun.gc(true), so connections are finalized with work still queued (Drop).

The 7 failures in sql-mysql.test.ts are MariaDB vs MySQL differences (CAST(.. AS JSON), error wording, FROM_UNIXTIME(0) before the epoch, repeat() returning a blob) and two debug-build timeouts (10,000 inserts in a 10 s budget). They fail the same way on unmodified main in this container. sql-mysql.transactions.test.ts has 2 error-wording failures of the same kind.

Unrelated observation, same on the release build of main: pipelining SELEC nope (a syntax error) behind a SLEEP(0.2) on one connection hangs the queued requests. Not touched here.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 10b14bb1-4e12-4cab-baf5-35df1d30e7b3

📥 Commits

Reviewing files that changed from the base of the PR and between 474f68b and 0fd5bba.

📒 Files selected for processing (1)

• test/js/sql/sql-mysql-clean-reentry.test.ts

Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review.

---

Walkthrough

The MySQL request path now passes RefPtr<JSMySQLQuery> values. MySQLRequestQueue stores owned references in VecDeque and releases them during completion, failure cleanup, queue cleaning, and destruction. A regression test covers repeated connection cleanup with queued queries.

Changes

MySQL query ownership

Layer / File(s)	Summary
Reference-counted enqueueing src/sql_jsc/mysql/JSMySQLQuery.rs, src/sql_jsc/mysql/JSMySQLConnection.rs, src/sql_jsc/mysql/MySQLConnection.rs, src/sql_jsc/mysql/MySQLRequestQueue.rs	Query enqueueing now passes RefPtr<JSMySQLQuery> through the connection layers. The queue stores these references in VecDeque.
Queue processing, cleanup, and regression validation src/sql_jsc/mysql/MySQLConnection.rs, src/sql_jsc/mysql/MySQLRequestQueue.rs, test/js/sql/sql-mysql-clean-reentry.test.ts	Queue advancement uses ParentRef and NonNull identities. Completion, failure, cleaning, and destruction release owned references through queue removal and container destruction. The regression test repeats cleanup rounds and checks connection-closed errors and clean subprocess exits.

Suggested reviewers: alii, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: storing MySQL request queue references as RefPtr.
Description check	✅ Passed	The description explains the problem, implementation, ownership model, re-entrancy behavior, and verification results. It provides the information required by the repository template, although it uses…

Full details: Description check

Explanation

The description explains the problem, implementation, ownership model, re-entrancy behavior, and verification results. It provides the information required by the repository template, although it uses Problem and Fix headings instead of the exact template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — mechanical *mut → NonNull tightening with no behavioral change.

What was reviewed:

• deref_nn is exactly unsafe { deref(p.as_ptr()) } (ref_count.rs:637), so all 5 release sites are byte-identical to before.
• impl_field_parent! split: the nonnull arm returns NonNull from the same from_mut(self) provenance the old get_js_connection used; both call sites (advance, upgrade_to_tls) updated, no others exist.
• NonNull::from(this) in do_run carries the same shared provenance as the old as_ctx_ptr(); the queue only forms ParentRef (&T) and deref_nn from it.
• peek_item(0) == request still compares by address (NonNull PartialEq).

Extended reasoning...

Overview

Changes the MySQLRequestQueue element type from *mut JSMySQLQuery to NonNull<JSMySQLQuery> and threads that through add/current/advance and the two enqueue_request wrappers. The 5 release sites switch from unsafe { JSMySQLQuery::deref(p) } to the safe deref_nn(p) trait method, and the 6 NonNull::new(p).expect(...) re-checks disappear. get_js_connection is regenerated via the nonnull arm of impl_field_parent! so advance and upgrade_to_tls receive a NonNull<JSMySQLConnection> directly.

Security risks

None. No I/O, protocol, or auth logic touched — pure pointer-type plumbing inside the request queue.

Level of scrutiny

Low-to-medium. This is native code with intrusive refcounting, so I verified each substitution is semantics-preserving: deref_nn at ref_count.rs:637 is literally unsafe { Self::deref(this.as_ptr()) }; the nonnull macro arm at bun_core/lib.rs:783 wraps the same from_field_ptr!(.., from_mut(self)) the previous two-arm form used, so provenance is unchanged; NonNull::from(&Self) in do_run yields the same shared-provenance address as the prior as_ctx_ptr() (from_ref(self).cast_mut()); NonNull<T> has identical size/repr to *mut T so the LinearFifo layout is unchanged; and NonNull's PartialEq compares addresses, so the peek_item(0) == request head-check in advance behaves identically.

Other factors

Grepped for other get_js_connection callers — only the two updated in this diff exist. The pattern (two impl_field_parent! invocations for ref + nonnull forms) already appears elsewhere (FileReader, Execution). Net -5 unsafe blocks, -6 expect, +0 unsafe. Existing MySQL tests pass per the PR description; this change has no observable behavior to add a new test for.

[Jarred-Sumner]

Do we need to call deref_nn? Why isn't this an RAII type?

[robobun]

Agreed. The reason it is not one today is that bun_ptr has no owning RAII handle for intrusive counts: RefPtr/IntrusiveRc deliberately has no Drop, and ScopedRef is a scope guard without Deref. I will add one (an owned ref that holds exactly one count, derefs to &T, clones by taking another count, releases on Drop, with from_raw/into_raw for the FFI hand-offs) in a small base PR, and then make this queue a LinearFifo of those so popping or clearing releases the query without any deref_nn. Will update this PR on top of that.

[robobun]

^{Updated 1:10 AM PT - Aug 26th, 2026}

✅ @robobun, your commit 0fd5bba2555c83194ef750a0a49f9bab711a94e5 passed in Build #106157! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37618

That installs a local version of the PR into your bun-37618 executable, so you can run:

bun-37618 --bun

[robobun]

Reworked as promised, now that the owning handle exists (#37665, which this PR is stacked on): the fifo holds OwnedRef<JSMySQLQuery> values, do_run passes this.owned_ref() in, and every removal is a read_item() whose result is dropped, so the five deref_nn calls are gone along with the unsafe in current_ref. Description rewritten to match; the queue paths (run failure, clean() on close with queued work, Drop) were exercised against a local MariaDB on the debug build in addition to the existing test files.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks intrusive-refcount ownership in native code (the queue now relies on LinearFifo's bag-of-bits move semantics interacting correctly with OwnedRef::Drop on every removal path), a human look would still be worthwhile.

What was reviewed

• Refcount balance: every removed deref maps to exactly one read_item() + drop; add() no longer double-refs; OOM in write_item drops the moved-in OwnedRef so no leak.
• LinearFifo never drops elements — verified no remaining discard() calls on the OwnedRef fifo; realign/ensure_total_capacity bit-move + MaybeUninit drop of the old buffer don't double-free; clean() and Drop still drain fully.
• Re-entrancy in advance(): the run-failure identity check now uses current() == Some(request) (address compare, no deref of a possibly-freed pointer); req is not touched after on_error returns.
• peek_item_ref is the previous peek_item body minus the copy; peek_item delegates to it, so existing callers are unchanged.

Extended reasoning...

Overview

This PR converts MySQLRequestQueue's element type from raw *mut JSMySQLQuery to OwnedRef<JSMySQLQuery>, so the queue's intrusive ref on each request is released by RAII rather than by five hand-paired deref() calls. It adds LinearFifo::peek_item_ref (a &T-returning peek for move-only element types), a one-line JSMySQLQuery::owned_ref() helper, and threads NonNull<JSMySQLConnection> through advance()/get_js_connection() in place of *mut. The enqueue_request signature on both the JS wrapper and the protocol struct changes to take the owned ref by value.

Security risks

None identified. This is a type-level refactor of an existing ownership pattern; no new attack surface, no protocol/parsing changes, no user-controlled input handling touched.

Level of scrutiny

High. Per REVIEW.md, native memory safety is the most-blocked category, and this PR sits squarely in it: it changes who releases each intrusive ref and when, and it stores a Drop type inside LinearFifo, which explicitly does not run destructors on its contents. The correctness argument depends on the invariant that every element leaves the fifo via read_item() (never discard(), never a silent buffer drop). I checked that this holds on all paths in MySQLRequestQueue.rs, but the invariant is documented only in a comment on the type Queue alias — a future discard() or ordered_remove_item() call would silently leak.

Other factors

• No new automated test ships in this PR. It's a behavior-preserving refactor covered by the existing sql-mysql*.test.ts suite, and the description reports ASAN-clean smoke runs exercising each removal path — but a maintainer should confirm that's acceptable for a refcount-balance change.
• The comment-cop github-actions bot has left eight inline flags on multi-line comments in the diff. Most look like doc/SAFETY comments the bot fires on indiscriminately, but a maintainer may want to triage them.
• The current() return type changed from Option<*mut> to Option<NonNull<>>; I checked the two external callers (is_idle, can_flush) and they only use .is_none() / current_ref(), so no behavior change.
• Drop ordering in advance() is preserved: with_mut(|q| q.read_item()) returns the OwnedRef after the fifo borrow ends, then the explicit drop() releases the ref (which may run the query's destructor and re-enter). This matches the old discard(1)-then-deref sequence.

[claude]

I reviewed this and didn't find any bugs. Since it reworks intrusive-refcount ownership on a re-entrant JS-facing path, a human pass on the drop ordering would still be worthwhile.

What was reviewed:

• Refcount balance: owned_ref() takes the same increment add() used to; every removal is now read_item() whose returned OwnedRef drops after the fifo borrow ends — matches the old discard(1) + deref order at all five sites.
• Run-failure path in advance(): current() == Some(request) subsumes the old length + peek check; request is a copied NonNull used only for identity comparison after on_error, so re-entrant clean() can't cause a UAF or double-release there.
• LinearFifo doesn't drop elements — confirmed MySQLRequestQueue::Drop and clean() still drain via read_item(), and write_item's OOM early-return drops the moved-in OwnedRef so no leak.
• peek_item_ref is the old peek_item body minus the copy; get_js_connection() → NonNull and current() → Option<NonNull> have no other callers.

Extended reasoning...

Overview

Converts MySQLRequestQueue's element type from raw *mut JSMySQLQuery to OwnedRef<JSMySQLQuery> (the RAII intrusive-ref handle added in #37665), so removing an element from the fifo and dropping it is what releases the ref. Removes five hand-paired unsafe deref calls and the unsafe ThisPtr::new in current_ref. Adds LinearFifo::peek_item_ref (same body as peek_item, returns &T), tightens MySQLRequestQueue::advance's param from *mut to NonNull, and splits impl_field_parent! so get_js_connection() returns NonNull. Signature-only changes to enqueue_request in JSMySQLConnection / MySQLConnection.

Security risks

None. No untrusted-input parsing, auth, or crypto is touched; this is an internal ownership refactor.

Level of scrutiny

High. This is native intrusive-refcount management on a JS-cell payload with re-entrant callbacks (on_error, reject) that can synchronously mutate or drain the same queue — the exact class REVIEW.md calls out as most-blocked. I traced the drop order at each removal site (with_mut(|q| q.read_item()) returns the OwnedRef after the fifo borrow ends, so the destructor can't re-enter under a live borrow), the run-failure re-entry case (only pointer identity is compared after on_error; no deref of a possibly-freed request), and the OOM path in write_item (the moved OwnedRef is dropped by the early return, so no leak). The LinearFifo no-drop caveat is handled: Drop and clean() drain with read_item, and no discard() calls remain on this element type. peek_item_ref only accesses slots at offset < count, so the pre-existing assume_init_slice UB note in linear_fifo.rs is not made worse (OwnedRef is NonNull-bearing, but so were several existing element types the file already documents).

Other factors

The base PR (#37665, OwnedRef) is merged. Verification was ASAN + local MariaDB smoke over the four queue paths plus the existing mysql test files, but no new automated test in this PR (it's a behavior-preserving type change). The comment-cop bot flags are all resolved; the added comments are short doc/ordering notes, not workaround justifications. I'm deferring rather than approving because refcount lifetime changes across re-entrant JS boundaries are the category where a second pair of eyes on drop ordering has the highest payoff, even when the mechanical trace checks out.

[robobun]

Retargeted onto main. The base PR #37665 (OwnedRef) is superseded by #40478, which made RefPtr the owning handle that releases on Drop, so this PR now uses RefPtr<JSMySQLQuery> in that role.

One more change since the last revision: LinearFifo requires T: Copy since #39570, because it never runs item destructors. The queue is therefore a VecDeque<RefPtr<JSMySQLQuery>> now, and the LinearFifo::peek_item_ref addition is gone. The description is rewritten for the current diff.

[robobun]

Addressed the comment-cop findings in 474f68b. MySQLRequestQueue::advance now takes ParentRef<MySQLConnection> instead of a raw pointer, so the null check and its justification are gone. The caller builds the ParentRef from the field-parent accessor. The remaining comments are cut to the re-entrancy and ownership facts. cargo check -p bun_sql_jsc is clean and the sql-mysql tests pass unchanged against the local MariaDB.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

Closing as superseded by #40516.

That PR (opened today, base main) makes the same change to these four files: type Queue = VecDeque<RefPtr<JSMySQLQuery>>, enqueue_request takes a RefPtr<JSMySQLQuery>, do_run passes this.ref_guard(), and every removal is a pop_front() whose drop is the release. It also converts the Postgres request queue and the prepared statement holders in both drivers, which this PR did not touch. Merging this PR first would only hand #40516 a conflict in the same hunks.

For the record, this branch was rebased onto main and tested before the overlap was found. The queue tests (sql-mysql* files and a local smoke script under ASAN) pass with the VecDeque<RefPtr<..>> shape, which is the shape #40516 carries.

[claude]

Code review found no issues

No high-confidence issues detected in this change.