impl_field_parent!: stop reaching parents through readonly &self

[dylan-conway]

What does this PR do?

impl_field_parent! recovers a struct's parent from one of its embedded fields (Zig's @fieldParentPtr). Its &self -> &Parent form is unsound when the child type is Freeze (no by-value Cell/UnsafeCell): rustc passes that &self as noalias readonly, so LLVM may delete stores made through the recovered parent — refcount bumps, flag sets — depending purely on inlining. That is how SubscriptionCtx (three bools) over-released JSValkeyClient on release builds: subscribe() from a close/rejection continuation freed the client under its live wrapper.

• valkey: the subscription bookkeeping that needs the client now lives on &JSValkeyClient; SubscriptionCtx's accessor is gone.
• The macro's arms are now &mut self -> &Parent / -> *mut Parent (the parent pointer stays based on the argument, so LLVM sees the aliasing; defined under Tree Borrows), plus shared/raw &self arms that fail to compile for a Freeze child. assert_not_freeze! is exported for hand-rolled sites (TimerObjectInternals).
• ValkeyClient, MySQLConnection, CapturedWriter, ByteBlobLoader, Assets, Execution, DirectoryWatchStore use the &mut self arms; FileReader/ByteStream keep &self access through the checked arms; raw *mut Context sites use NewSource::from_context_ptr.
• container_of docs state what each derivation permits; bun_ptr gains Miri tests for every arm.

Audit of every user (Freeze? / writes through the accessor? / verdict) is in the review thread.

How did you verify your code works?

MIRIFLAGS=-Zmiri-tree-borrows cargo miri test -p bun_ptr container_of_tests (4 pass; a shared arm on a Freeze child is a compile error with the intended message). Debug build: valkey-gc.test.ts, fetch.stream.test.ts, spawn-stdout and shell-pipeline paths driven by hand. Full valkey/mysql suites need their containers — left to CI.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f2a69c91-91df-4c7e-ab40-080cbd7dab33

📥 Commits

Reviewing files that changed from the base of the PR and between e7460e3 and f6575ec.

📒 Files selected for processing (16)

• src/bun_core/lib.rs
• src/ptr/lib.rs
• src/runtime/bake/dev_server/assets.rs
• src/runtime/bake/dev_server/mod.rs
• src/runtime/shell/subproc.rs
• src/runtime/test_runner/Execution.rs
• src/runtime/timer/timer_object_internals.rs
• src/runtime/valkey_jsc/js_valkey.rs
• src/runtime/valkey_jsc/js_valkey_functions.rs
• src/runtime/valkey_jsc/valkey.rs
• src/runtime/webcore/ByteBlobLoader.rs
• src/runtime/webcore/ByteStream.rs
• src/runtime/webcore/FileReader.rs
• src/runtime/webcore/ReadableStream.rs
• src/runtime/webcore/fetch/FetchTasklet.rs
• src/sql_jsc/mysql/MySQLConnection.rs

---

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

[dylan-conway]

Audit

What LLVM/Tree Borrows actually permit, checked with Miri (bun_ptr tests + scratch cases):

how the parent pointer is obtained, from a child method	LLVM	Tree Borrows
container_of(from_mut(self)), &mut self receiver	✅ based on the arg	✅ out-of-range perms initialise lazily
container_of(from_ref(self)), &self, child !Freeze	✅ no noalias/readonly emitted	✅
container_of(from_ref(self)), &self, child Freeze	❌ readonly ⇒ stores deleted	❌ writes UB (reads fine)
stored back-pointer read in a &mut self method, parent then touches the child	❌ noalias arg vs. non-based pointer	❌ protector
stored back-pointer in a &self method of a Freeze child, parent writes the child	❌	❌
methods on &Parent, no child reference at all	✅	✅

So the fix direction depends on the receiver: for &mut self methods container_of is the safer mechanism (a stored BackRef would hide the aliasing from LLVM); for &self methods it is only OK while the child is !Freeze, which is now a compile-time check rather than luck.

child ⇒ parent.field	old form	child Freeze?	writes through it?	status on main	change
SubscriptionCtx ⇒ JSValkeyClient._subscription_ctx	&self→&P	yes	ref_scope, update_poll_ref	miscompiled (release: client freed under wrapper)	methods moved onto &JSValkeyClient, accessor deleted
ValkeyClient ⇒ JSValkeyClient.client	&self→&P, only called from &mut self fns	no — by exactly one Cell (auto_flusher.registered)	ref_/deref, on_valkey_*	latent	&mut self arms
MySQLConnection ⇒ JSMySQLConnection.connection	&self→&P + &mut→*mut, all callers &mut self	no (queue cells)	on_error_packet, on_query_result, ref guard	latent	&mut self arms
CapturedWriter ⇒ PipeReader.captured_writer	&self→&P + &mut→*mut	yes	&self path reads only; &mut path signals cmd	one store away	&mut self arms; is_done → PipeReader::captured_writer_done
ByteBlobLoader ⇒ Source.context	&self→&P (from a &mut self fn)	yes	is_closed.set	OK only because the enclosing frame is &mut	&mut self→&Source
ByteStream ⇒ Source.context	&self→&P + &mut→*mut	no	producer/is_closed cells	OK while !Freeze	shared arm (checked); raw sites → NewSource::from_context_ptr
FileReader ⇒ Source.context	&self→*mut + &self→&P	no	increment/decrement_count, global_this =	OK while !Freeze	raw + shared arms (checked)
Assets ⇒ DevServer.assets	&self→&P + &mut→*mut	yes in release (ArrayHashMap's atomic is debug-only)	&self: reads (magic, server); &mut: client_graph.insert_empty	OK (reads)	&mut self arms; two debug-only magic reads from &self getters dropped
SourceMapStore ⇒ DevServer.source_maps	&mut→*mut	—	—	OK	unchanged
Execution ⇒ BunTest.execution	&mut→NonNull	—	—	OK	mut arm
DirectoryWatchStore ⇒ DevServer.directory_watchers (hand-rolled)	&mut→*mut	—	—	OK	now the macro
IncrementalGraph<SIDE> ⇒ DevServer.{client,server}_graph (hand-rolled)	&mut→*mut	—	—	OK	unchanged (field depends on SIDE)
TimerObjectInternals ⇒ {Timeout,Immediate}Object.internals (hand-rolled)	&self→*mut	no	timer heap, RefCount, state =	OK while !Freeze	assert_not_freeze! added
HTTPClient ⇒ AsyncHTTP.client, LinkerContext ⇒ BundleV2.linker, DeferredBatchTask ⇒ BundleV2, bun_io::Request/Poll ⇒ ReadFile/WriteFile	&mut-derived	—	yes	OK for this class	unchanged
Graph ⇒ BundleV2.graph (HTMLImportManifest::write), EventLoopTimer::js_timer_epoch	&-derived, Freeze	yes	reads only	OK	unchanged — would need to change before anything writes through them

Not this bug class but adjacent, left alone: DeferredRequest::__free(&mut self) returns its own slot to the pool (poison/dealloc under a live &mut self), and AsyncHTTP::as_erased_ptr(&self) is later written through from the HTTP-thread callback.

Overlaps: supersedes the macro/valkey/ByteBlobLoader parts of #35787 (its RefCount::ref_ volatile tripwire and valkey-gc test are not included here).

[claude]

I reviewed this PR and the bug-hunting pass found no issues. Because it redesigns a foundational unsafe abstraction (impl_field_parent! / container_of) and migrates ~15 memory-safety-sensitive call sites across valkey, mysql, streams, shell, and the dev server based on Tree Borrows / noalias readonly semantics, a human look is still warranted.

What was reviewed:

• Macro arm redesign and the __IsFreeze specialization trick — inherent-const-shadows-trait-const detection looks correct; Miri tests cover each arm.
• Every removed/renamed accessor (SubscriptionCtx::parent, CapturedWriter::is_done, ByteStream::parent, ByteBlobLoader::parent_const, Assets::owner visibility) — grepped for orphaned callers, none found.
• MySQLConnection::can_* rewrites — the js_connection.connection.get().queue round-trip is the same self.queue, just reached without holding &self across the now-&mut self js_connection_ref().
• The two dropped debug_assert!(owner().magic == Valid) in Assets::get_hash/get are read-only and only removed because owner() now takes &mut self.

Extended reasoning...

Overview

This PR fixes a real release-build miscompilation: impl_field_parent!'s &self -> &Parent arm was unsound for Freeze child types because rustc emits noalias readonly on such &self, letting LLVM delete stores made through the recovered parent pointer. SubscriptionCtx (three bools, so Freeze) hit this and over-released JSValkeyClient. The fix redesigns the macro into four arms (&mut self -> &P, &mut self -> *mut P, and shared/raw &self arms gated by a compile-time !Freeze check via assert_not_freeze!), moves the affected SubscriptionCtx methods onto JSValkeyClient directly, and migrates every other user of the macro across 16 files.

Security risks

This is memory-safety code. The bug being fixed is a use-after-free in release builds. The change itself introduces no new attack surface, but any mistake in the migration (wrong receiver, borrow held across re-entry) would be a UAF or aliasing UB. The audit table posted by dylan-conway is thorough; the Miri tests under Tree Borrows validate each arm shape.

Level of scrutiny

High. This is a foundational unsafe primitive whose correctness depends on Tree Borrows semantics and LLVM's noalias/readonly attribute emission — subtle enough that the PR description and audit comment together run several pages. It touches ref-counting paths in valkey/mysql, stream sources, shell pipe readers, the dev server, timers, and the test runner. Per the review guidelines, complex changes to memory-safety-critical native code should get human review even when automated review is clean.

Other factors

• No bugs were found by the multi-agent bug hunter.
• I verified there are no orphaned callers of the removed/renamed accessors (SubscriptionCtx::close → close_subscription_ctx has no callers either before or after — dead but carried forward).
• The nonnull macro arm was dropped and its one caller (Execution::bun_test) migrated to the mut arm with (*ptr) deref — behavior-preserving.
• s3/client.rs still calls ByteStream::parent_const(), which the shared arm continues to provide.
• #[feature(freeze)] is added to bun_core — a nightly feature, consistent with the crate's other feature flags.
• CI (valkey/mysql container suites) is explicitly deferred to in the PR description; those need to be green before merge.

[claude]

I reviewed this PR and didn't find any bugs. Because it redesigns a foundational unsafe primitive (impl_field_parent!) with reasoning grounded in Tree Borrows / Freeze / LLVM noalias readonly semantics, and reshapes call sites across 16 files (valkey, mysql, shell, dev_server, streams, timers), a human look is still warranted.

What was reviewed:

• Macro rewrite: verified the plain arm's receiver flip (&self→&mut self) is compatible with every remaining caller; checked the __IsFreeze inherent-const-shadowing trick and its Miri test.
• SubscriptionCtx methods moved to &JSValkeyClient: confirmed all call sites in js_valkey_functions.rs / valkey.rs were updated and _subscription_ctx field access remains coherent.
• CapturedWriter::on_iowriter_chunk control-flow refactor: checked the all_written computation preserves the original done/err/suspend semantics.
• Assets::owner receiver flip: confirmed the two &self callers that lost access only dropped debug-only reads.

Extended reasoning...

Overview

This PR fixes a release-only miscompilation in impl_field_parent!: when a child type is Freeze, rustc marks &self as noalias readonly, so LLVM may delete stores made through a parent pointer derived from it. The concrete symptom was SubscriptionCtx (three bools) over-releasing JSValkeyClient in release builds. The fix redesigns the macro into four arms with explicit receiver contracts (&mut self→&Parent/*mut Parent; &self→&Parent/*mut Parent gated on a compile-time !Freeze check via assert_not_freeze!), documents the provenance rules on container_of, adds Miri tests under Tree Borrows, and migrates every user across 16 files.

Security risks

None in the traditional sense — no auth, network parsing, or user-input handling. The risk here is memory safety: an incorrect migration of any one of the ~15 call sites could introduce a new UAF or aliasing UB. The SubscriptionCtx→JSValkeyClient method move is the largest refactor and touches refcount / poll-ref bookkeeping.

Level of scrutiny

High. This is a foundational unsafe abstraction whose correctness depends on Tree Borrows semantics and LLVM's readonly/noalias attribute emission — reasoning that very few people can verify from first principles. The macro removed one arm (nonnull) and flipped the default arm's receiver from &self to &mut self, so every existing user's borrow structure needed re-audit. Several call sites also had non-mechanical control-flow changes (CapturedWriter::on_iowriter_chunk merged its two try_signal_done_to_cmd paths; MySQLConnection::can_* re-derives self.queue through the parent).

Other factors

• The PR includes Miri tests for each macro arm and a freeze_detection test for the specialization trick — good coverage of the primitive itself, but the per-call-site migrations are only covered by existing runtime tests (valkey/mysql containers left to CI).
• dylan-conway posted a detailed per-user audit table; that table is what a human reviewer should validate against the diff.
• The PR notes it supersedes parts of #35787 but omits that PR's regression test — a maintainer should decide whether that test belongs here.

Given the depth of the aliasing-model reasoning and the breadth of the cross-cutting migration, this should not be auto-approved.

[robobun]

^{Updated 6:05 PM PT - Aug 14th, 2026}

@dylan-conway, your commit f6575ec is building: #96799

[claude]

I reviewed this PR and didn't find any bugs. Given the scope — a redesign of impl_field_parent!'s soundness contract with per-call-site migrations across 16 files, plus control-flow reshuffles in the shell/valkey/mysql paths to satisfy the new &mut self receivers — a human look is still worthwhile.

What was reviewed:

• Macro arms and the __IsFreeze/__NotFreeze specialization trick — inherent const shadows the trait const exactly when T: Freeze; Miri tests cover all four arms.
• CapturedWriter::on_iowriter_chunk restructuring — all_written is computed from self.written + amount before the assignment, and both the err and all-written paths still reach try_signal_done_to_cmd, matching the old behavior.
• MySQLConnection::can_* now round-trips through js_connection.connection.get().queue instead of self.queue — same field via the JsCell, needed because js_connection_ref() now takes &mut self.
• Assets &self getters lost only debug-only magic asserts; replace_path (&mut self) keeps its assert.

Extended reasoning...

Overview

This PR fixes a soundness hole in impl_field_parent! (the @fieldParentPtr port): the old &self -> &Parent arm is UB when the child is Freeze, because rustc emits noalias readonly on &self and LLVM may delete stores through anything derived from it. The concrete miscompilation was SubscriptionCtx (three bools → Freeze) over-releasing JSValkeyClient in release builds. The fix redesigns the macro into four arms (&mut self -> &P, &mut self -> *mut P, and two &self arms gated by a compile-time !Freeze check via a specialization-style const trick), migrates every call site, and adds Miri tests under Tree Borrows.

Security risks

None in the traditional sense. This is a memory-safety hardening: it turns a class of latent UB (stores through readonly-derived pointers) into either a compile error or a sound derivation. The valkey change fixes a real UAF path.

Level of scrutiny

High. This is foundational unsafe Rust touching pointer-provenance reasoning across the shell, valkey, mysql, dev-server, streams, timer, and test-runner subsystems. The soundness argument relies on Tree Borrows semantics and LLVM's noalias/readonly attribute emission — subtle enough that the PR includes a per-site audit table. Several call sites (on_iowriter_chunk, can_pipeline/can_prepare_query/can_execute_query, do_write) had their control flow reshuffled to satisfy the new &mut self borrow discipline, and each reshuffling needs to be checked for behavioral equivalence.

Other factors

The PR is exceptionally well-documented (per-site audit, Miri tests, doc comments explaining what each derivation permits), and the bug-hunting pass found nothing. I traced the CapturedWriter::on_iowriter_chunk and MySQLConnection::can_* restructurings and they preserve behavior. The removed nonnull arm's only user (Execution::bun_test) now uses the mut arm and derefs with (*buntest) instead of (*buntest.as_ptr()), which is equivalent. But given the breadth (16 files, ~10 distinct intrusive-parent patterns) and the subtlety of the underlying model, this is the kind of change a maintainer should sign off on rather than an automated approval.

[claude]

I reviewed this PR and the automated bug-hunting pass found no issues. Given the scope — a redesign of the impl_field_parent! unsafe primitive plus refactoring of every call site across refcount- and GC-adjacent code in valkey, MySQL, shell subprocess, streams, and the dev server — a human review is still warranted.

Checked: the on_iowriter_chunk control-flow rewrite in subproc.rs preserves the err/all-written/suspended branching; the Assets::owner &self→&mut self migration only drops debug-only magic reads from &self getters; MySQLConnection::can_* re-derives queue through the parent to avoid the borrow overlap without changing behavior; every removed ByteStream::parent() raw-mut caller (ReadableStream::cancel, FetchTasklet) now goes through NewSource::from_context_ptr.

Extended reasoning...

Overview

This PR redesigns impl_field_parent! in bun_core to eliminate a class of UB where a Freeze child's &self (passed noalias readonly by rustc) is used to derive a parent pointer that is then written through — LLVM may delete those stores. The macro is rewritten from five fixed forms into four composable arms: &mut self → &Parent (default), &mut self → *mut Parent (mut), and two &self arms (shared/raw) gated by a compile-time assert_not_freeze! check using the nightly freeze feature and an inherent-const-shadows-trait-const specialisation trick. All ~13 users are migrated: SubscriptionCtx (the actively miscompiling case) has its methods moved onto &JSValkeyClient; ValkeyClient/MySQLConnection/CapturedWriter/ByteBlobLoader/Assets/Execution/DirectoryWatchStore move to &mut self arms; FileReader/ByteStream keep &self under the checked arms; TimerObjectInternals gets a bare assert_not_freeze!. Miri tests under Tree Borrows exercise every arm.

Security risks

None in the injection/auth sense. The risk profile here is memory safety: an incorrect migration could regress refcount balancing (valkey ref_scope/deref, ScopedRef), leave a stale &Parent alive across a &mut self reborrow, or change control flow in the subproc CapturedWriter::on_iowriter_chunk rewrite. I traced each of these and found them behavior-preserving, but the reasoning depends on Tree Borrows semantics and LLVM's noalias/readonly treatment of &T vs &mut T — subtle enough that a second pair of eyes on the macro contract and the shared/raw arm's !Freeze sufficiency argument is worth having.

Level of scrutiny

High. This is a foundational unsafe primitive used across 16 files in refcount- and GC-adjacent code. The PR fixes a real release-build miscompile (over-release of JSValkeyClient), so the change is load-bearing, not defensive. The author's audit table and Miri coverage are thorough, and the __IsFreeze specialisation-trick and macro tt-muncher recursion look correct. But the interaction between &mut self → &Parent (which mutably borrows self for the &Parent lifetime) and subsequent self.field = … writes required non-trivial restructuring in subproc.rs and MySQLConnection.rs — each of those rewrites is a place where a behavior change could hide.

Other factors

The PR adds a nightly feature flag (#[feature(freeze)]) to bun_core. The nonnull macro arm is removed and its one caller (Execution) migrated to the mut arm with .as_ptr() dropped — the doc comment on Execution::bun_test was updated to match. The ByteStream::finalize doc comment still references parent().deinit() which no longer exists, but that comment was already stale before this PR. CI is still building; the PR description notes valkey/mysql suites need containers and are left to CI. No prior human reviews on the thread.