jsc: move close_all_socket_groups to VirtualMachine; hot_reloader scopeguard via BackRef

[robobun]

Follow-up to #35373: finishes the rare_data/field-alias borrowck cleanup (cluster C4 in the audit on farm/2a32eccb/borrowck-audit) with the three src/jsc/ sites that did not depend on the RareData::*_group signature change.

What / Why

close_all_socket_groups

Was a &mut self method on RareData that didn't use self (let _ = self;): it walks the per-VM uSockets loop's linked group list, not RareData's embedded fields. Moved to VirtualMachine as a &self method. global_exit and WebWorker::shutdown now call it directly:

// before
let vm_ref = unsafe { &*core::ptr::from_ref(self) };
self.rare_data.as_deref_mut().unwrap().close_all_socket_groups(vm_ref);
// after
self.close_all_socket_groups();

hot_reloader::on_file_update

Held the Watcher as a *mut so self could be reborrowed in the loop body and the flush_evictions scopeguard didn't pin a &mut across it. The reloader already stores a BackRef<Ctx> (which is Copy); the guard now captures that and re-derives the Watcher on drop, and the two remove_at_index sites reborrow via self.get_context() inline. Same unsafe obligation (the existing BACKREF invariant), one audited site instead of three.

Verification

• bun run rust:check-all passes on all 10 targets.
• bun bd test test/cli/hot/hot.test.ts test/js/web/workers/worker.test.ts test/cli/watch/watch.test.ts all pass.
• clippy clean on bun_jsc.

 src/jsc/VirtualMachine.rs | 46 ++++++++++++++++++++++++++++++++++++----------
 src/jsc/hot_reloader.rs   | 26 +++++++++++++-------------
 src/jsc/rare_data.rs      | 37 -------------------------------------
 src/jsc/web_worker.rs     | 11 ++---------
 4 files changed, 51 insertions(+), 69 deletions(-)

This is a behavior-preserving refactor (both changes relocate code to where its invariant lives with byte-identical runtime behavior), so there is no fail-before test; the listed suites are the regression guard.

[coderabbitai]

Walkthrough

Changes

VM socket-group shutdown

Layer / File(s)	Summary
Centralized VM socket shutdown src/jsc/VirtualMachine.rs, src/jsc/rare_data.rs, src/jsc/web_worker.rs	Socket-group draining moves from RareData to VirtualMachine, which drains the full uSockets loop group list and is used by both shutdown paths.

Hot-reloader lifetime handling

Layer / File(s)	Summary
Hot-reloader context and eviction handling src/jsc/hot_reloader.rs	File removals use self.get_context(), and deferred eviction flushing accesses the stored context backreference through a scopeguard.

Possibly related PRs

• oven-sh/bun#34154: Modifies VM and worker shutdown ordering through shutdown gating.
• oven-sh/bun#34219: Adds another shutdown drain in VirtualMachine::global_exit.
• oven-sh/bun#34455: Adjusts socket closure ordering with DNS teardown.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately captures the two main refactors in the change set.
Description check	✅ Passed	The description includes both the change summary and verification details required by the template, even though the headings differ.
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.

---

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

[robobun]

^{Updated 8:03 AM PT - Jul 24th, 2026}

❌ @autofix-ci[bot], your commit 542a3c1 has 1 failures in Build #79582 (All Failures):

• step failed outside runner - exit 1 on 🍎 14 aarch64 - test-bun

---

🧪   To try this PR locally:

bunx bun-pr 35376

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

bun-35376 --bun

[github-actions]

Found 1 issue this PR may fix:

1. unsafe: raw-pointer borrowck workaround — should add compiler-checkable invariants #30767 - This PR removes the exact raw-pointer borrowck workaround pattern documented in the issue (e.g. ws_client_group and other RareData socket-group accessors), replacing the &VirtualMachine parameter with *mut uws::Loop to eliminate the split-borrow need entirely.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #30767

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. http: remove lifetime-launder workarounds in client body/buffer and RareData socket-group paths #35373 - Also refactors RareData socket-group accessors to eliminate unsafe split-borrow workarounds, touching the same files (rare_data.rs, VirtualMachine.rs, and all caller sites)

🤖 Generated with Claude Code

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/sql_jsc/jsc.rs:326-334 — The trait doc comment on VirtualMachineSqlExt::postgres_socket_group (jsc.rs:289-292) still says it "Encapsulates the rare_data(&mut self) / *_group(.., &VirtualMachine) borrowck conflict … so the four call sites need no per-site raw-pointer dance." After this PR the *_group accessors take *mut uws::Loop and there is no borrowck conflict left to encapsulate — the doc now describes a removed mechanism and should be updated (or dropped) alongside the signature change.

  Extended reasoning...

  What is stale

  The trait declaration at src/sql_jsc/jsc.rs:289-292 reads:

  /// Lazy-init `RareData`'s per-protocol uws [`bun_uws::SocketGroup`].
  /// Encapsulates the `rare_data(&mut self)` / `*_group(.., &VirtualMachine)`
  /// borrowck conflict (the two borrows touch field-disjoint state) so the
  /// four call sites need no per-site raw-pointer dance.
  fn postgres_socket_group<const SSL: bool>(&mut self) -> &mut bun_uws::SocketGroup;

  Three claims in this doc are now false after the PR:

  1. *_group(.., &VirtualMachine) — the signature is now *_group(.., *mut uws::Loop) (see rare_data.rs in this diff).
  2. "borrowck conflict (the two borrows touch field-disjoint state)" — there is no conflict anymore. uws_loop() returns a Copy raw pointer that the caller snapshots before calling rare_data(), so the two borrows never overlap.
  3. "raw-pointer dance" — this PR's whole point is to eliminate that dance; the impl body just below (lines 326-334) is now a trivial two-liner with no raw pointers at all.

  Why the PR should have updated it

  This PR systematically deleted or rewrote every other "reshaped for borrowck" / "raw-pointer split-borrow" comment at the call sites it touched — websocket_client.rs, WebSocketUpgradeClient.rs, VirtualMachine.rs, js_bun_spawn_bindings.rs, Channel.rs, socket_body.rs, js_valkey.rs, and the impl-body comments in this very file (the old "Route the read-only vm argument through the JS-thread singleton accessor…" comment at lines 327-332 was removed). The trait-level doc on the same method is the one instance that was missed.

  Per REVIEW.md, "Comments carry only durable non-obvious content" and "One source of truth; update every consumer atomically." A doc comment describing a mechanism the PR removes is exactly the kind of stale content that rule targets.

  Step-by-step proof

  1. Before this PR: RareData::postgres_group<const SSL>(&mut self, vm: &VirtualMachine) — calling vm.rare_data().postgres_group(vm) needed &mut vm and &vm simultaneously, hence the borrowck workaround the doc describes.
  2. After this PR (rare_data.rs hunk): RareData::postgres_group<const SSL>(&mut self, loop_: *mut uws::Loop).
  3. After this PR (jsc.rs:326-328 hunk): the impl is let loop_ = self.uws_loop(); self.rare_data().postgres_group::<SSL>(loop_) — loop_ is a Copy value taken while no borrow of self is outstanding, then rare_data() takes &mut self cleanly. No conflict, no raw pointer, nothing to "encapsulate."
  4. The trait doc at lines 289-292 was not touched by the diff and still names the &VirtualMachine signature and the raw-pointer dance.

  Impact

  Documentation-only. No runtime effect. A future reader following the doc to understand why this trait method exists will be misled into thinking there is still a split-borrow being hidden here.

  Fix

  Rewrite the doc to describe what the method actually does now, e.g.:

  /// Lazy-init `RareData`'s per-protocol uws [`bun_uws::SocketGroup`].
  /// Snapshots `self.uws_loop()` before the `rare_data()` `&mut self` borrow
  /// so callers get a single `&mut self` accessor.
  

  or simply drop the second sentence, since the new accessor block in rare_data.rs already documents the snapshot-loop pattern.

[robobun]

Rebased onto #35373 (which independently made the identical RareData::*_group signature change). This PR now carries only the three sites #35373 doesn't cover: the close_all_socket_groups move and the hot_reloader scopeguard.

The stale VirtualMachineSqlExt::postgres_socket_group doc comment the review bot flagged is in src/sql_jsc/jsc.rs, which is in #35373's diff (not touched here after the rebase); noted it there.

[robobun]

No test file accompanies this PR because it is a behavior-preserving refactor with no fail-before.

Both changes relocate code to where its invariant already lives, with byte-identical runtime behavior:

• close_all_socket_groups: moved from RareData (where it began with let _ = self;) to VirtualMachine. Same body, same two call sites, same if rare_data.is_some() guard. The removed unsafe { &*core::ptr::from_ref(self) } reborrow produced exactly the &VirtualMachine that &self now supplies directly.
• hot_reloader::on_file_update: the _flush scopeguard and the two remove_at_index calls reach the same Watcher through self.ctx (a BackRef) instead of a cached *mut Watcher. self.ctx and self.bun_watcher are both set once at init and never reassigned, so re-deriving at use time yields the same pointer as caching it at the top.

A test that exercises these paths (worker teardown with live sockets, --hot file delete/flush) passes identically before and after, so there is no test that fails without this diff and passes with it. The self-review probed for behavioral deltas along eight dimensions and found none; bun run rust:check-all and the hot-reload / worker / watch / IPC suites listed in the PR body are the regression guard.

(The src/http/lib.rs and src/runtime/api/bun/js_bun_spawn_bindings.rs hunks in the combined diff belong to the base branch, #35373.)

[claude]

No issues found — the refactor looks behavior-preserving. Deferring to a human because it touches unsafe VM-teardown ordering and watcher-thread BackRef derefs, and it's stacked on #35373 (merge order + the binary-size CI gate need a maintainer call).

What was reviewed:

• close_all_socket_groups: body moved verbatim from RareData (where self was unused) to VirtualMachine; both call sites keep the rare_data.is_some() guard, so no behavior change.
• hot_reloader::on_file_update: drop order (_flush → flush_evictions before current_task → enqueue) is preserved; the guard now reaches the same Watcher via BackRef<Ctx> → bun_watcher_mut() instead of a cached *mut Watcher.
• src/http/lib.rs and js_bun_spawn_bindings.rs hunks are pure autofix.ci rustfmt — no logic change.

Extended reasoning...

Overview

This PR relocates close_all_socket_groups from RareData (where it took &mut self but did let _ = self;) to VirtualMachine as a &self method, and rewrites the hot_reloader::on_file_update scopeguard to capture the reloader's BackRef<Ctx> (Copy) instead of a raw *mut Watcher. Two call sites (VirtualMachine::global_exit, WebWorker::shutdown) drop their unsafe { &*core::ptr::from_ref(self) } split-borrow workaround. The src/http/lib.rs and js_bun_spawn_bindings.rs hunks are autofix.ci rustfmt reflows with zero semantic change.

Security risks

None identified. No user-input handling, no auth/crypto, no new FFI surface. The change removes unsafe raw-pointer reborrows rather than adding them.

Level of scrutiny

High — this is unsafe Rust in two memory-safety-sensitive paths: (1) VM/worker teardown ordering (close_all_socket_groups runs on_close JS callbacks and must precede JSC teardown), and (2) the file-watcher thread's scopeguard drop order (which the existing comments document as load-bearing for an EBADF race). REVIEW.md flags native memory safety as the most-blocked category, and "before deleting odd-looking code, git-blame why it was written" applies directly to the removed raw-pointer laundering. I traced both changes and they are behavior-preserving: the moved method body is byte-for-byte equivalent (same 8-round loop, same close_all_groups/drain_closed_sockets sequence), and the hot_reloader guard reaches the identical Watcher instance via the already-audited bun_watcher_mut() path with the same LIFO drop order.

Other factors

The PR is stacked on #35373; the autofix.ci reformats and the failing binary-size check (+513KB linux) almost certainly belong to the base branch, not this ~20-line net change — but that's a merge-order/[skip size check] decision a maintainer should make. Tests (hot.test.ts, worker.test.ts, watch.test.ts) and rust:check-all on 10 targets are reported passing. Given the domain sensitivity and the stacked-PR logistics, I'm deferring rather than approving.

[robobun]

CI on build 79293: neither failure is from this diff.

binary-size (+513 KB linux): baseline drift. This build's bun-linux-x64 is 72.30 MB, byte-identical to #35373's build 79250 (also 72.30 MB, which passed the check against main #79216). Main shrank to 71.80 MB between #79216 and #79256 (the new comparison baseline); #35373 branched before that shrink and this PR is stacked on it. Resolves when the stack rebases onto current main. This PR's own 4-file, +51/-69 diff has no effect on binary size.

proxy-stress-errors.test.ts (debian 13 x64, 1 lane): the same run reported panic: Failed to start File Watcher: EAGAIN (inotify instance exhaustion on the CI box). The 407-vs-502 test passes locally under bun bd test with this branch and passed on #35373's build 79250 on the same lane. This PR does not touch src/http.

The diff is ready for review; no retrigger pushed since the binary-size check is deterministic and would fail again until the base branch rebases.

[Jarred-Sumner]

Fix conflicts

[robobun]

Rebased onto the current tip of #35373 (cbda8de431); conflicts resolved. Diff against the base branch is unchanged (4 files, +51/-69).

[claude]

No bugs found. The 4-file src/jsc/ change this PR describes (moving close_all_socket_groups to VirtualMachine, and the hot_reloader scopeguard capturing BackRef<Ctx> instead of a cached *mut Watcher) looks correct and behavior-preserving. Deferring because the diff against main also carries the stacked #35373 changes — notably the src/http/lib.rs restructuring of handle_on_data_headers / removal of pending_response / clone_metadata taking &Response — which is a substantial HTTP-client hot-path refactor that should get human eyes (ideally on #35373 itself).

What was reviewed:

• close_all_socket_groups: body is byte-identical to the removed RareData method; both call sites keep the rare_data.is_some() guard.
• hot_reloader::on_file_update: self.get_context() and the guard's ctx_ref.get_mut().bun_watcher_mut() reach the same Watcher as the old cached *mut; drop order (_flush before current_task) is preserved.
• Checked that self.get_context() reborrows don't conflict with the watchlist-derived slices (they're a parameter, not from self).

Extended reasoning...

Overview

This is a stacked PR on top of #35373. The PR's own contribution is 4 files in src/jsc/ (+51/-69): moving close_all_socket_groups from RareData (where it began with let _ = self;) to VirtualMachine as a &self method, and reworking the hot_reloader::on_file_update scopeguard to capture the BackRef<Ctx> (which is Copy) instead of a cached *mut Watcher.

However, because the PR base is main, the diff GitHub shows (and that the bug-hunting system reviewed) is 19 files including all of #35373's changes: the RareData::*_group signature retarget across ~10 call sites, plus a substantial src/http/lib.rs refactor that removes InternalState::pending_response, changes clone_metadata to take &picohttp::Response<'_> by reference, restructures handle_on_data_headers around a moved-out response_message_buffer local, changes to_result() to return HTTPClientResult<'static> with body: None, and simplifies the chunked-decode path to always copy into scratch. New tests in fetch-proxy-connect-tunnel-split-envelope.test.ts pin the split-read accumulation behavior.

Security risks

None identified. This is internal refactoring of borrow patterns and buffer ownership in the HTTP client and VM teardown paths; no user-facing input validation or auth surface changes.

Level of scrutiny

The 4-file src/jsc/ portion is low-risk: it relocates code with byte-identical bodies and swaps one raw-pointer pattern for another that resolves to the same target. I traced both changes and they preserve behavior (same Watcher reached, same drop ordering, same rare_data.is_some() guards).

The stacked src/http/ portion is higher-risk: it touches the fetch client's response-header parsing hot path, changes struct layout (InternalState loses a field), and restructures buffer ownership in handle_on_data_headers. The new tests cover the short-read / 1xx / chunked-in-same-read paths, which is good, but this is exactly the kind of change REVIEW.md flags for careful behavior-preservation audit ("Treat every refactor as guilty until proven behavior-preserving").

Other factors

• Jarred-Sumner is already engaged on the PR (requested conflict resolution).
• The author has documented extensively that the http/ hunks belong to #35373; approving here would merge both, so the http/ changes should be reviewed on #35373 (or here if #35373 merges first and this rebases to a 4-file diff).
• No bugs were found by the bug-hunting system across the full 19-file diff.
• CI status (build 79571) is still building at time of review.

[robobun]

#35373 merged; rebased this branch onto current main (028f7a3b54). The diff is now exactly the 4 src/jsc/ files (+51/-69) with no stacked content.

The build 79571 failures were all npm registry outage during the run (every failing test hung at "Resolving dependencies" or got 503/522 from registry.npmjs.org; the registry is responding 200 again now).

[coderabbitai]

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 `@src/jsc/VirtualMachine.rs`:
- Around line 984-989: Update the invariant comment near the loop’s linked-group
traversal to remove the transient issue number, build reference, allocation-size
detail, and incident-specific leak wording. Retain only the durable rationale:
Listener/uWS-App groups own separate SocketGroup instances, accepted sockets are
stored there rather than in RareData, and the linked-group list must be
traversed during shutdown.

🪄 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: e8d17c22-0a93-40d8-94f0-bea8a9f5da83

📥 Commits

Reviewing files that changed from the base of the PR and between 028f7a3 and 542a3c1.

📒 Files selected for processing (4)

• src/jsc/VirtualMachine.rs
• src/jsc/hot_reloader.rs
• src/jsc/rare_data.rs
• src/jsc/web_worker.rs

💤 Files with no reviewable changes (1)

• src/jsc/rare_data.rs

[claude]

LGTM — pure relocation refactor removing raw-pointer borrowck workarounds.

What was reviewed:

• close_all_socket_groups: body moved verbatim from RareData (where it began let _ = self;) to VirtualMachine; both call sites keep the rare_data.is_some() guard, and no other callers exist.
• hot_reloader::on_file_update: confirmed BackRef<Ctx> is Copy and get_context() reaches the same Watcher as the removed cached *mut; scopeguard drop order (_flush before current_task) is preserved.
• Checked uws_loop_mut() is &self so the new &self receiver works from both global_exit (&mut self) and WebWorker::shutdown.

Extended reasoning...

Overview

Two independent borrowck-cleanup refactors in src/jsc/, follow-up to #35373:

1. close_all_socket_groups moved from RareData (where it discarded self with let _ = self; and only touched vm.uws_loop_mut()) to VirtualMachine as a &self method. Both call sites — VirtualMachine::global_exit and WebWorker::shutdown — replace an unsafe { &*core::ptr::from_ref(self) } split-borrow with a direct self.close_all_socket_groups() call, keeping the if rare_data.is_some() guard.
2. hot_reloader::on_file_update replaces a cached *mut Watcher (held so self could be reborrowed in the loop and the _flush scopeguard didn't pin &mut) with the existing BackRef<Ctx> field. The guard now copies self.ctx (verified Copy at src/ptr/lib.rs:162) and re-derives the Watcher on drop; the two remove_at_index sites reborrow via self.get_context() inline.

Security risks

None. No user input, network, auth, or crypto surface touched. This is internal lifetime plumbing.

Level of scrutiny

Moderate — VM/worker shutdown and hot-reload eviction are correctness-sensitive paths, but the change is mechanical: the method body is a verbatim move (comments included, as noted in the resolved CodeRabbit thread), and the hot-reloader change substitutes one indirection (*mut Watcher cached at top) for an equivalent one (BackRef<Ctx> → bun_watcher_mut() re-derived per use). Both self.ctx and self.bun_watcher are set once at init and never reassigned, so re-deriving yields the same pointer as caching. The _flush-before-current_task drop order (which prevents the documented EBADF race) is preserved by declaration order.

Other factors

• Grep confirms the two call sites are the only callers of close_all_socket_groups; no stragglers.
• uws_loop_mut() takes &self (line 864), so the new &self receiver composes cleanly.
• The one CodeRabbit inline (comment-history metadata in the moved block) was withdrawn after the move-verbatim rationale was explained; the thread is resolved.
• Jarred's only request ("Fix conflicts") was addressed by rebase; no outstanding human review feedback.
• PR body reports rust:check-all on all 10 targets and the hot/watch/worker suites passing; the no-new-test rationale (no fail-before for a pure relocation) matches the REVIEW.md exception criteria.
• Net -18 lines, three unsafe raw-pointer reborrows eliminated.

[robobun]

Build 79582 (post-rebase onto main): all six test annotations are flaky (passed on retry), none in src/jsc/. The only red job is darwin-14-aarch64-test-bun with state "Expired" (agent didn't pick it up in time). Binary-size is back within threshold. Ready for review/merge; the expired lane can be retried from the Buildkite UI if needed.