io: return a FilePoll to its store through the owner's pointer, not a &mut receiver by robobun · Pull Request #37803 · oven-sh/bun

robobun · 2026-08-12T10:35:30Z

Problem

The three FilePoll deinit entry points (POSIX, and the Windows twin) are &mut self methods that end by handing their own receiver to Store::put.
For a poll that was never registered, put frees the slot on the spot, while the &mut self of the deinit frames still belongs to calls that have not returned. Both Miri models reject that (Tree Borrows: "the strongly protected tag disallows deallocations"; Stacked Borrows: "deallocating while item is strongly protected").
Casting self to a raw pointer before the put, as the old comments did, does not lift the protection; only returning does.
Nothing reaches that branch today: every POSIX owner registers right after init, registration flags the poll before checking the syscall result, and no Windows code creates a poll. The old code relied on that unstated invariant. No behaviour changes. Same shape as blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 and Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver #37778.

Fix

The deinit entry points take *mut FilePoll. Unregistering and field clearing move into a &mut self helper whose borrow ends at its statement, and the raw pointer is what goes to the store, so nothing references the slot when put may free it.
Every owner already holds the poll as a pointer, so callers pass that. The two event loop callbacks that deinit the poll they are handed take a pointer too; those polls were registered, so their put is deferred, and the &mut self further up the event loop stack is unchanged and tracked separately.
A new source lint bans handing a method's receiver to .push(..) / .put(..). On main it reports exactly the two sites above; here the tree is clean. Two bundler sites are allowlisted pending bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver #37732; the file is shared with Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver #37778, and whichever lands second deletes the leftover entries.
Verification: the lint fails on main and passes here. There is no behavioural fail-before test, since nothing reaches the freeing branch. cargo check for Linux, macOS and Windows targets, clippy, fmt, and the spawn, child_process, dns, memory-pressure and shell suites were run on a debug build; the three child_process failures also fail on released bun or are timeouts.

Background

A FilePoll is bun's registration of one fd with the platform event loop (epoll / kqueue). Pipes, child processes, the DNS resolver, dns_sd and the memory-pressure watcher each own one and hold it as a raw pointer.
Polls are not boxed. They live in a per-thread Store whose hive has 128 inline slots and spills the rest to the heap, and a poll is released with Store::put rather than Drop.
Store::put has two branches: a poll that was ever registered is queued and freed after the current event loop turn; one that never was is freed or recycled immediately.
In Rust a &mut T argument is protected until its call returns (rustc emits noalias dereferenceable), so freeing it inside the call is undefined behaviour even via a raw pointer. bun run rust:miri checks this with Tree Borrows.

[review] gate passed · iteration 1 · 9 files touched

fails on main (without fix)

ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/self-receiver-push-put.test.ts
bun test v1.4.0 (68ad6349a)

test/internal/source-lints/self-receiver-push-put.test.ts:
(pass) scans a non-empty set of tracked Rust sources [2.39ms]
(pass) the patterns match the banned spellings and nothing else [41.67ms]
318 |   expect(banned.filter(s => findHandOvers(s).length === 0)).toEqual([]);
319 |   expect(allowed.filter(s => findHandOvers(s).length !== 0)).toEqual([]);
320 | });
321 | 
322 | test("no method pushes or puts its own receiver", () => {
323 |   expect(offenders).toEqual([]);
                          ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/io/posix_event_loop.rs:419",
+   "src/io/windows_event_loop.rs:134",
+ ]

- Expected  - 1
+ Received  + 4

      at <anonymous> (/workspace/bun/test/internal/source-lints/self-receiver-push-put.test.ts:323:21)
(fail) no method pushes or puts its own receiver [5.33ms]
(pass) allowlisted files still carry exactly their documented count [6.86ms]

 3 pass
 1 fail
 5 expect() calls
Ran 4 tests across 1 
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (da3851e57)

test/internal/source-lints/self-receiver-push-put.test.ts:
(pass) scans a non-empty set of tracked Rust sources [0.12ms]
(pass) the patterns match the banned spellings and nothing else [0.62ms]
318 |   expect(banned.filter(s => findHandOvers(s).length === 0)).toEqual([]);
319 |   expect(allowed.filter(s => findHandOvers(s).length !== 0)).toEqual([]);
320 | });
321 | 
322 | test("no method pushes or puts its own receiver", () => {
323 |   expect(offenders).toEqual([]);
                          ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/io/posix_event_loop.rs:419",
+   "src/io/windows_event_loop.rs:134",
+ ]

- Expected  - 1
+ Received  + 4

      at <anonymous> (/workspace/bun/test/internal/source-lints/self-receiver-push-put.test.ts:323:21)
(fail) no method pushes or puts its own receiver [0.23ms]
(pass) allowlisted files still carry exactly their documented count [0.16ms]

 3 pass
 1 fail
 5 expect() calls
Ran 4 tests across 1 file. [828.00ms]
__F:1:S:0

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/self-receiver-push-put.test.ts
bun test v1.4.0 (68ad6349a)

test/internal/source-lints/self-receiver-push-put.test.ts:
(pass) scans a non-empty set of tracked Rust sources [2.54ms]
(pass) the patterns match the banned spellings and nothing else [43.42ms]
(pass) no method pushes or puts its own receiver [1.54ms]
(pass) allowlisted files still carry exactly their documented count [6.83ms]

 4 pass
 0 fail
 5 expect() calls
Ran 4 tests across 1 file. [24.62s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 981ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/5] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_io v0.0.0 (/workspace/bun/src/io)
�[1m�[92m   Compiling�[0m bun_zlib v0.0.0 (/workspace/bun/src/zlib)
�[1m�[92m   Compiling�[0m bun_event_loop v0.0.0 (/workspace/bun/src/event_loop)
�[1m�[92m   Compiling�[0m bun_sourcemap v0.0.0 (/workspace/bun/src/sourcemap)
�[1m�[92m   Compiling�[0m bun_css v0.0.0 (/workspace/bun/src/css)
�[1m�[92m   Compiling�[0m bun_options_types v0.0.0 (/workspace/bun/src/options_types)
�[1m�[92m   Compiling�[0m bun_http v0.0.0 (/workspace/bun/src/http)
�[1m�[92m   Compiling�[0m bun_crash_handler v0.0.0 (/workspace/bun/src/crash_handler)
�[1m�[92m   Compiling�[0m bun_resolve_builtins v0.0.0 (/workspace/bun/src/resolve_builtins)
�[1m�[92m   Co
... (truncated)

diff hotspot

src/io/lib.rs                                      |  14 +-
 src/io/posix_event_loop.rs                         |  62 ++--
 src/io/windows_event_loop.rs                       |  56 ++--
 src/runtime/dispatch.rs                            |  19 +-
 src/runtime/dns_jsc/dns.rs                         |  28 +-
 src/runtime/dns_jsc/dns_sd.rs                      |  26 +-
 src/runtime/node/memory_pressure.rs                |  41 ++-
 src/spawn/process.rs                               |  33 +-
 .../source-lints/self-receiver-push-put.test.ts    | 331 +++++++++++++++++++++
 9 files changed, 498 insertions(+), 112 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file

file                                                      reads  edits  tests
src/io/lib.rs                                                 5      7      0
src/io/posix_event_loop.rs                                    7     10      0
src/io/windows_event_loop.rs                                  4      8      0
src/runtime/dispatch.rs                                       2      4      0
src/runtime/dns_jsc/dns.rs                                    6      5      0
src/runtime/dns_jsc/dns_sd.rs                                 3      4      0
src/runtime/node/memory_pressure.rs                           5      9      0
src/spawn/process.rs                                          2      5      0
…st/internal/source-lints/self-receiver-push-put.test.ts      5     12      0

root cause · written by the author bot

FilePoll::deinit_possibly_defer (and its Windows twin) handed its own hive slot to Store::put while still executing under a &mut self receiver, so on the never-registered path the slot could be dropped and freed while that protected reference argument was still live, which is undefined behaviour under both Stacked Borrows and Tree Borrows even though no crash was known. The fix converts deinit, deinit_with_vm and deinit_force_unregister on POSIX and Windows into unsafe functions that take the slot pointer the owner already holds, perform the field work through a statement-scoped clear_for_p…

Original description

What

FilePoll::deinit, deinit_with_vm and deinit_force_unregister (src/io/posix_event_loop.rs, and the twin in src/io/windows_event_loop.rs) were &mut self methods that ended by putting their own receiver back into the event loop's Store:

fn deinit_possibly_defer(&mut self, vm: EventLoopCtx, force_unregister: bool) {
    ...
    let this = ptr::NonNull::from(self);
    vm.file_polls_mut().put(this, vm, was_ever_registered);   // posix_event_loop.rs:419
}

(windows_event_loop.rs:134 is the same line spelled NonNull::from(&mut *self).)

Same shape as #37681 (blob read handler) and #37778 (transpiler / patch jobs), in the FilePoll store.

Why

Store::put has two branches. For a poll that went through register* it queues the slot and frees it after the event loop turn. Otherwise it calls hive.put(slot) right there: drop_in_place + recycle for one of the 128 inline slots, heap::destroy (a Box free) for a slot that spilled to the heap because more than 128 polls were live. In that branch the slot is freed while the &mut self of deinit_possibly_defer, and of the entry point one frame up, are still arguments of calls that have not returned. A reference argument has to stay valid until its call returns: rustc marks it noalias dereferenceable for the whole call, and both aliasing models reject freeing it (Tree Borrows, which bun run rust:miri uses: "the strongly protected tag disallows deallocations"; Stacked Borrows: "deallocating while item is strongly protected"). Converting self to a raw pointer before the put, which is what the old comments did, does not end the protection; only returning does.

How reachable that branch is today, checked while writing this: not at all. register_with_fd_impl sets WasEverRegistered right after the syscall, before it looks at the result (epoll, macOS and FreeBSD kqueue alike), and every owner in the tree registers immediately after FilePoll::init (pipe reader and writers, Process::watch, the resolver's socket-state callback, dns_sd, the memory-pressure watcher), so every POSIX deinit, including the ones on registration-failure paths, takes the deferred branch. The Windows twin would take the immediate branch for every poll, but no Windows code creates one (the POSIX reader/writer types that do are compiled there and unused). So this is not a fix for a path something takes; it is the store's contract (put may free) being satisfiable by the API's shape instead of by an invariant that nothing states or checks (a new owner that deinits before registering, or a change to how a failed registration is flagged, would make the old &mut self put free its own receiver), and it is what lets the push/put lint below hold tree-wide with no standing exception for this store. No behaviour changes.

Fix

Every owner already holds the slot as a pointer (FilePollRef(NonNull), PollerPosix::Fd(NonNull), the resolver's PollsMap of *mut FilePoll, dns_sd's file_poll, the memory-pressure watcher's Option<NonNull>), so the entry points now take it:

pub unsafe fn deinit(this: *mut FilePoll)
pub unsafe fn deinit_with_vm(this: *mut FilePoll, vm: EventLoopCtx)
pub(crate) unsafe fn deinit_force_unregister(this: *mut FilePoll)

The unregister + field clearing moved into a &mut self helper (clear_for_put) invoked as a statement-scoped reborrow, and the pointer itself goes to the store. Callers pass the pointer they hold (FilePollRef::deinit_force_unregister, PollerPosix::deinit, which Process::close now also goes through, Resolver::on_dns_socket_state, SharedConnection::{init,destroy}, memory_pressure::{deinit_poll,uninstall,register_os_watch}).

The two dispatch targets that deinit the poll they are handed, Resolver::on_dns_poll and memory_pressure::on_poll, take *mut FilePoll as well, and __bun_run_file_poll reads owner/hup through scoped accesses. To be clear about what that is: those are the same shape as deinit, not a change in what the aliasing models see. A dispatched poll was registered, so its put is the deferred one, and the &mut self of on_kqueue_event / on_epoll_event / on_update in src/io/posix_event_loop.rs is still live up the stack while an owner deinits its poll from inside the callback, before and after this PR. That chain is reported separately and left alone here.

Test

test/internal/source-lints/self-receiver-push-put.test.ts bans handing the receiver to .push(..) / .put(..) inside a method. Against main it reports exactly the two sites above; with this change the tree is clean. Compared with the version #37778 adds, this one also catches the spellings that need no conversion function at all, list.push(self) / store.put(&mut *self) (the &mut Self to *mut Self coercion at the call, which clippy's ref_as_ptr / borrow_as_ptr do not see), NonNull::new_unchecked(self) and self.into(), so the ratchet cannot be satisfied by deleting a from_mut. The extra arm finds the two outstanding_*.push(self) sites in src/bundler/bundle_v2.rs, allowlisted with a pointer to #37732, which converts them.

The file is shared with #37778: that PR allowlists these two FilePoll sites, this one allowlists the three transpiler/patch sites it converts (plus the two bundler sites). Whichever lands second deletes the entries that remain; the ratchet test in the file fails until it does.

There is no behavioural fail-before test: as described above, nothing reaches the branch whose shape this fixes, and the old code never touched the slot after the put.

Verification

cargo check --workspace for x86_64-unknown-linux-gnu, aarch64-apple-darwin (dns_sd and the macOS memory-pressure arm) and x86_64-pc-windows-msvc (the Windows twin); cargo clippy on bun_io / bun_spawn / bun_runtime; cargo fmt --check; the full test/internal/source-lints/ directory.
Against the debug build: test/js/bun/spawn/spawn.test.ts (140 pass), spawn-many-teardown.test.ts (350 children, so the hive spills to the heap), spawn-pipe-stale-fd-unregister.test.ts, spawn-noread-leak.test.ts, spawn-stdin-destroy.test.ts, exit-code.test.ts, test/js/node/child_process/child_process.test.ts (62 pass; the 3 failures are the $SHELL-is-unset test, which fails the same way with the released bun, and two tests that hit their 5 s timeout on a loaded machine while spawning 20 debug children; the GC fixture from the latter prints OK when run directly), child-process-stdio.test.js, test/js/node/dns/dns-tcp-bidirectional-poll.test.ts and dns-resolver-concurrent-timeout.test.ts (the on_dns_poll / socket-state path), test/js/node/process/process-memory-pressure.test.ts, and the shell suites bunshell.test.ts (418 pass), pipeline_stack, epipe, shell-pipe-read-fault.

… &mut receiver FilePoll::deinit, deinit_with_vm and deinit_force_unregister were &mut self methods ending in Store::put on their own receiver. For a poll that was never registered, put() recycles the slot before returning (a Box free once the 128-slot hive has spilled to the heap), while the &mut self of deinit_possibly_defer and of the entry point above it are still protected arguments of calls that have not returned; decaying the reference to a raw pointer first does not end that. Make the three entry points take the slot pointer every owner already holds (FilePollRef, PollerPosix::Fd, the resolver's polls map, dns_sd's connection, the memory-pressure watcher), do the unregister/clear work through a statement-scoped reborrow (clear_for_put), and hand the pointer to the store. The Windows twin gets the same shape. The two dispatch targets that deinit the poll they are given (Resolver::on_dns_poll, memory_pressure::on_poll) take the pointer as well instead of a &mut parameter, and __bun_run_file_poll no longer keeps a reference into the slot across the dispatch. Adds the self-receiver push/put source lint (shared with #37778, which carries the same file with the FilePoll sites allowlisted; whichever lands second drops the remaining allowlist entries).

coderabbitai · 2026-08-12T10:35:39Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 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: 3b0a5060-1b22-4bb9-bf1c-657c15c72ff7

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 68ad634.

📒 Files selected for processing (9)

src/io/lib.rs
src/io/posix_event_loop.rs
src/io/windows_event_loop.rs
src/runtime/dispatch.rs
src/runtime/dns_jsc/dns.rs
src/runtime/dns_jsc/dns_sd.rs
src/runtime/node/memory_pressure.rs
src/spawn/process.rs
test/internal/source-lints/self-receiver-push-put.test.ts

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

robobun · 2026-08-12T10:36:35Z

Status: ready for a maintainer; the diff is green, CI needs a rebuild from the Buildkite side.

Reproduced how: test/internal/source-lints/self-receiver-push-put.test.ts against main reports src/io/posix_event_loop.rs:419 and src/io/windows_event_loop.rs:134 (the Store::put of the method's own receiver); clean with this branch.
Fix: the FilePoll::deinit* entry points (and the two dispatch targets that deinit their poll) take the slot pointer. The description has the reachability analysis: nothing in the tree reaches the immediate-free branch today, so this is the store's contract made structural plus the lint, not a crash fix.
Review follow-ups (fa6a7c4, 17eb5a2): the lint also catches the bare coercion spellings (push(self), NonNull::new_unchecked(self), self.into()), which adds a bundle_v2.rs allowlist entry tracked against bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver #37732; comments trimmed and corrected (a failed register* still marks the poll registered). All review threads are resolved.
CI, final: across builds 93075, 93289 and 93545 (identical tree) every lane that built and ran tests passed, including both darwin 14 and darwin 26 in 93545. What is red in 93545 is two build-bun lanes (linux x64, linux aarch64-musl) that failed to download the c-ares / mimalloc / WebKit tarballs from github.com during an outage that hit every build in the pipeline at the time, and the 62 jobs queued behind them; the remaining annotations are retried-green flakes in unrelated areas. A Buildkite rebuild should go green; I am not pushing further retriggers.
Shares the lint file with Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver #37778 (allowlist turned around); whichever lands second drops the remaining entries, same for the bundle_v2.rs entry and bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver #37732.

claude

I reviewed this PR and didn't find any bugs. Because it reworks unsafe pointer/receiver handling across the FilePoll store, dispatch, and several platform-gated callers, a human look would still be worthwhile.

What was reviewed:

Confirmed every FilePoll::deinit* caller in the tree is converted (grep shows no remaining &mut-receiver call sites; pipes.rs goes through the updated FilePollRef wrapper).
Checked Process::close: stranded_watch_ref is computed the same way and poller.deinit() covers both the Fd and WaiterThread arms the old if/else handled.
Checked on_dns_poll: fd/readable/writable are read before channel.process() (which reaches on_dns_socket_state → deinit_with_vm), so no reference into the slot outlives the reads.
The Windows twin's deinit_with_vm mirrors the POSIX split (clear_for_put reborrow ends before Store::put).

Extended reasoning...

Overview

Converts FilePoll::deinit, deinit_with_vm, and deinit_force_unregister from &mut self methods to unsafe fn(this: *mut FilePoll) associated functions, so Store::put (which may drop_in_place/Box-free the slot for a never-registered poll) never runs while a protected &mut self argument is still live in a caller frame. Field access is factored into a clear_for_put(&mut self) helper invoked as a statement-scoped (*this).clear_for_put(..) reborrow. Callers in FilePollRef, PollerPosix, Process::close, Resolver::{on_dns_poll,on_dns_socket_state}, SharedConnection, and memory_pressure now pass the raw slot pointer they already hold. __bun_run_file_poll no longer holds a &mut FilePoll across dispatch. A new source-lint test (self-receiver-push-put.test.ts) bans the pattern tree-wide with a ratcheted allowlist for the sites #37778 covers.

Security risks

None. This is a Rust aliasing-model correctness change with no user-facing surface, no input parsing, and no auth/crypto/permission code touched.

Level of scrutiny

High. The change is behaviourally intended to be a no-op, but it rewires unsafe pointer handling in the core event-loop store across POSIX, Windows, and macOS-specific (dns_sd, EVFILT_MEMORYSTATUS) paths, and restructures Process::close and on_dns_poll. The reasoning is subtle (Tree/Stacked Borrows protector semantics; statement-scoped reborrow lifetimes) and warrants a maintainer who knows this store's invariants confirming the shape.

Other factors

Follows the pattern of two related merged PRs (#37681, #37778); the source-lint is the same file as #37778 with the allowlist inverted, and the ratchet test forces whichever lands second to reconcile.
Verification listed in the description is thorough: cross-target cargo check for linux/darwin/windows, clippy, and the spawn/child_process/dns/memory-pressure/shell suites against a debug build.
I traced every remaining deinit/deinit_with_vm/deinit_force_unregister reference in src/ — all go through the new raw-pointer entry points (the one .deinit_force_unregister() in pipes.rs is on a FilePollRef, which now forwards the raw pointer).
The Process::close restructuring is not purely mechanical (a two-branch if/else became is_some_and + unconditional poller.deinit()), but PollerPosix::deinit's new match covers the same arms, and Detached is a no-op, so behaviour is preserved.
The 286-line source-lint regex machinery is non-trivial but has its own positive/negative fixture test embedded.

robobun · 2026-08-12T13:05:34Z

^{Updated 7:22 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 68ad634 has 1 failures in Build #93545 (All Failures):

scripts/build/ci.ts - build failed on 🐧 aarch64-musl - build-bun
scripts/build/ci.ts - build failed on 🐧 x64 - build-bun

🧪 To try this PR locally:

bunx bun-pr 37803

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

bun-37803 --bun

…oll branch precisely The lint missed `list.push(self)` / `store.put(&mut *self)` (the receiver coercing to `*mut Self` at the call), `NonNull::new_unchecked(self)` and `self.into()`, which are the shortest spellings of what it bans and the ones clippy's ref_as_ptr / borrow_as_ptr do not cover. Adding them surfaces the two `outstanding_*.push(self)` sites in bundle_v2.rs, allowlisted against the PR converting them. The comments now say what is actually the case: register* marks a poll WasEverRegistered even when the syscall fails and every in-tree owner registers right after init, so the immediate recycle in Store::put is the store's contract rather than a branch anything takes today, and the raw dispatch parameters are the same shape as FilePoll::deinit, not a change in what the aliasing models see (on_update's receiver is still live up the stack in both cases).

claude

I reviewed this PR and didn't find any bugs. Because it reworks the FilePoll deinit lifecycle across the event loop, spawn, DNS, and memory-pressure paths on three platforms — memory-safety-critical code — a human look is still warranted. The comment-cop flags on the new SAFETY/doc comments are also outstanding.

Checked: Process::close still computes stranded_watch_ref before poller.deinit() clears the flags; __bun_run_file_poll no longer holds a &mut FilePoll across arms that may return the slot; the Windows deinit_with_vm mirrors the POSIX split (clear_for_put then Store::put); on_dns_poll reads fd/is_readable/is_writable before process() can reach the slot via the socket-state callback.

Extended reasoning...

Overview

Converts FilePoll::deinit, deinit_with_vm, and deinit_force_unregister from &mut self methods to unsafe fn(this: *mut FilePoll) associated functions, so Store::put (which may free/recycle the slot for never-registered polls) is not called while a protected &mut self argument is still live on the stack. The unregister/field-clearing work is factored into a &mut self helper (clear_for_put) whose borrow ends before the store call. All callers are updated to pass the raw slot pointer they already hold: FilePollRef, PollerPosix::deinit (now also used by Process::close), Resolver::on_dns_poll/on_dns_socket_state, SharedConnection::{init,destroy}, and the memory-pressure watcher. Two dispatch targets (on_dns_poll, memory_pressure::on_poll) and __bun_run_file_poll itself also switch to raw-pointer parameters so no reference into the slot spans a call that may deinit it. A new source-lint test enforces the pattern tree-wide with a ratcheted allowlist for the sites #37778 handles.

Security risks

None identified. This is an aliasing-model / Stacked/Tree Borrows soundness refactor with no user-facing behavior change; no untrusted input handling, auth, or crypto is touched.

Level of scrutiny

High. This is core event-loop lifetime management for pool-allocated FilePoll slots, touching POSIX (kqueue/epoll), Windows, spawn, DNS (c-ares and dns_sd), and the memory-pressure watcher. A mistake here is a use-after-free or double-free on a rarely-taken error path (registration failure) that tests can't easily reach. The change is behaviorally inert by design, which is reassuring, but the receiver-shape change and the Process::close reordering deserve human eyes.

Other factors

Outstanding automated review comments: comment-cop flagged ~18 new comments as paragraph-long. Many are # Safety doc sections and SAFETY comments that the repo also requires above unsafe; the author should trim or dismiss these before merge.
Process::close refactor: now calls fd_poll_mut() (forming a &mut FilePoll) only to read is_registered(), then calls poller.deinit() which routes Fd through the raw-pointer FilePoll::deinit. The read happens before the deinit, so stranded_watch_ref is computed correctly; behavior matches the old inline path.
Cross-PR coordination: the lint file overlaps with #37778; the ratchet test will fail on whichever lands second until the allowlist is trimmed. This is stated in the description.
Verification listed is thorough (cargo check on all three targets, clippy, and the relevant runtime test suites), but CI is still building.

robobun · 2026-08-12T14:46:51Z

The comment flags are addressed: 17eb5a2 trims the added comments to the unsafe-fn contracts, the one-sentence rationale for the pointer shape, and corrections to two pre-existing docs; each remaining thread has a reply and is resolved. The description was rewritten with the reachability analysis (no in-tree path reaches the immediate-free branch; see Why) and the lint now covers the bare coercion spellings as well.

claude

I reviewed this PR and didn't find any bugs. Because it reshapes the FilePoll::deinit* API (&mut self → raw slot pointer) and rewires every owner across the event loop, DNS, spawn, and memory-pressure paths — core memory-safety code — a human look would still be worthwhile.

Checked: the new deinit_possibly_defer scopes its &mut reborrow to clear_for_put before Store::put; Process::close still computes stranded_watch_ref before poller.deinit(); on_dns_poll reads fd/readable/writable before process can reach the slot via on_dns_socket_state.
Noted: the lint's ALLOW ratchet interlocks with #37778/#37732 — whichever lands second must drop entries or the ratchet test fails.
Also: comment-cop is still flagging several comments on 17eb5a2 (lib.rs:376, posix_event_loop.rs:391/396, etc.) — worth confirming whether those are actionable or bot noise before merge.

Extended reasoning...

Overview

Converts FilePoll::deinit / deinit_with_vm / deinit_force_unregister (posix + windows) from &mut self methods to unsafe fn(this: *mut FilePoll) so that Store::put — which may free the slot in its never-registered branch — is not called with a protected &mut self still on the stack. The unregister/field-clear work is factored into a &mut self clear_for_put helper invoked as a statement-scoped reborrow. All owners (FilePollRef, PollerPosix, Resolver, dns_sd SharedConnection, memory_pressure) are updated to pass the raw slot pointer they already hold. __bun_run_file_poll and the two dispatch targets that may deinit their poll (Resolver::on_dns_poll, memory_pressure::on_poll) now take *mut FilePoll and read fields through scoped raw derefs. A new 331-line source lint bans .push(self) / .put(self) shapes tree-wide with a ratcheted allowlist covering #37778 and #37732.

Security risks

None. This is an internal memory-management/aliasing refactor with no user-facing surface, no parsing of external input, and no auth/crypto changes.

Level of scrutiny

High. This is exactly the "Native code: memory safety (the most-blocked category)" bucket in REVIEW.md — pointer lifetime and aliasing across the event loop's FilePoll store, touching spawn, DNS, dns_sd, and memory-pressure teardown paths on three platforms. The PR itself notes the freeing branch is unreachable today (every in-tree owner registers before deinit), so this is a shape fix rather than a live-bug fix, but it changes a pub API to unsafe and rewires eight call sites. A maintainer should confirm the aliasing story (particularly the acknowledged remaining &mut self of on_update up-stack during in-callback deinit, which the PR explicitly leaves for a separate change) and the interlock with the sibling PRs.

Other factors

The lint test's ALLOW table hard-codes counts for RuntimeTranspilerStore.rs (2), patch_install.rs (1), and bundle_v2.rs (2), tied to #37778/#37732. The ratchet test will fail if one of those lands first without this PR's allowlist being adjusted, and vice versa — merge ordering matters.
Process::close was refactored to route through PollerPosix::deinit; verified stranded_watch_ref is still read via fd_poll_mut().is_some_and(..) before the deinit, preserving the ref-leak guard.
comment-cop (github-actions) still has 7 open flags on the head commit after the "Trim the deinit comments" push. They target doc/safety comments rather than workaround justifications, so they may be noise, but they're unresolved bot feedback on the current head.
No behavioural test is possible (the branch is unreachable today per the PR); coverage is the source lint plus the listed spawn/dns/shell suites run against the debug build.

__bun_run_file_poll reads the slot through scoped accesses and passes the pointer to on_dns_poll, which takes it raw: c-ares re-registers or deinits that same poll from inside Channel::process. These two frames overlap with #37803; whichever lands second resolves the conflict by taking either side, both pass the lint, so the allowlist that pinned them is gone. The lint now also requires each frame to call the next one itself with its pointer as the first argument, so a helper interposed on any edge, or a &mut *this argument, is reported, not only on the dns edge. NonNull is no longer accepted as the pointer spelling, and an as_mut() binding counts as a held reborrow. The safety comments no longer claim the slot stays allocated across the owner call; the chain only needs it live on entry and never reads it after.

claude

I reviewed this PR and didn't find any bugs. Because it reshapes the FilePoll deinit API (&mut self → pub unsafe fn(*mut)) across the POSIX/Windows event loops and every caller, with aliasing-model reasoning that governs when Store::put may free a live receiver, a human look would still be worthwhile.

What was reviewed:

Process::close() refactor preserves stranded_watch_ref semantics — fd_poll_mut().is_some_and(|p| p.is_registered()) reads the flag before poller.deinit() reaches the slot, and the WaiterThread/Detached arms match the old else-branch.
on_dns_poll reads fd/is_readable/is_writable into locals before Channel::process, so no reference into the slot spans the re-entrant call that can deinit it via on_dns_socket_state.
The lint's FN_ITEM boundary and SELF_REF lookahead correctly exclude self.field / closure bodies; the allowlist ratchet interacts cleanly with #37778/#37732 (whichever lands second deletes stale entries, enforced by the actual === ALLOW assertion).

Extended reasoning...

Overview

Converts FilePoll::deinit, deinit_with_vm, and deinit_force_unregister from &mut self methods to unsafe fn(*mut FilePoll) associated functions, so the slot pointer can be handed to Store::put without a protected reference argument still being live when put frees it. Touches src/io/{lib,posix_event_loop,windows_event_loop}.rs (the API and clear_for_put split), every caller (spawn/process.rs, dns_jsc/{dns,dns_sd}.rs, node/memory_pressure.rs, runtime/dispatch.rs), and adds a 331-line source-lint test that bans the push(self)/put(self) shape tree-wide with an allowlist for sites being converted in #37778 and #37732.

Security risks

None. This is a Rust aliasing-model soundness change with no user-facing surface, no parsing of untrusted input, and no auth/crypto involvement.

Level of scrutiny

High. Per REVIEW.md, native memory safety is the most-blocked category. The change is in the core event-loop FilePoll lifecycle (kqueue/epoll/libuv), reshapes a public API from safe to pub unsafe, and threads raw pointers through six call sites across three platforms. The author's reachability analysis (nothing in-tree hits the immediate-free branch today) is careful, but the correctness of the new shape — that every statement-scoped (*this).clear_for_put(..) reborrow really ends before file_polls_mut() forms &mut Store, and that no caller holds a stale &mut FilePoll across the new unsafe boundary — is exactly the kind of invariant a maintainer familiar with the hive/store aliasing discipline should confirm.

Other factors

The comment-cop bot flagged over-long comments; the author trimmed them in 17eb5a2 and all threads are resolved. The lint test file is shared with #37778 (each allowlists the other's sites), so merge order matters and the ratchet test enforces cleanup. The PR description is unusually thorough (Tree/Stacked Borrows citations, per-platform verification, explicit statement that the on_update chain further up the stack is out of scope and reported separately). No behavioural test exists because, as documented, no path reaches the branch whose shape this fixes — the source-lint is the regression guard. The Process::close control-flow rewrite and the __bun_run_file_poll scoped-read change both check out as behaviour-preserving on inspection.

github-actions Bot added the claude label Aug 12, 2026

robobun mentioned this pull request Aug 12, 2026

Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver #37778

Open

claude Bot reviewed Aug 12, 2026

View reviewed changes

robobun mentioned this pull request Aug 12, 2026

io: dispatch a ready FilePoll through its pointer, not a &mut receiver #37840

Open