Pass the job to JobContext::run and FileOpener::get_fd by pointer, not &mut by robobun · Pull Request #37820 · oven-sh/bun

robobun · 2026-08-12T12:20:28Z

Problem

No crash is known. The pool jobs behind Bun.file reads and writes hand the job to another thread while the pool thread's frames still hold &mut references to it, and that thread finishes and frees the job through its own pointer. On Windows, ReadFileUV frees itself from inside a continuation that received it as &mut.
A &mut argument is protected until the call returns, so another thread touching or freeing that memory before then is undefined behaviour under Stacked Borrows and Tree Borrows (what bun run rust:miri checks).
A read that fails at open, or of an empty file, finishes on the JS thread while those frames are still unwinding as a matter of course; the polled endings race with the io thread.
This is the contract shape blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 / blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 / Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768 / io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 convert elsewhere, and the one chain io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 leaves as is.

Fix

JobContext::run and FileOpener::get_fd / get_fd_by_opening receive the job as a raw pointer, and the open continuation type becomes unsafe fn(*mut T, Fd). The 16 synchronous jobs reborrow for their work and return the token as before; the longer bodies move unchanged into a &mut self method.
The read and write continuations make their decision under a reborrow that has ended before the step that hands the job on runs, so at hand-over no reference to the job is live on the pool thread. A raw pointer makes no claim, so the same sequence is accepted; the miri reduction of the four shapes is in the original description below.
Out of scope: the recursive readdir scan only has its outer frame converted (its hand-over happens inside &mut self methods; filed separately), the steps under the continuations are io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787's, and the Windows libuv open branch gets the minimum conversion until blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 removes it.
Verification: behaviour is meant to be unchanged. A new source lint fails on the base commit (19 run implementations, 11 continuation spellings) and passes here; the existing tests for each ending (ENOENT, empty file, pipe, fd, copy) and for the other jobs pass on an ASAN debug build; clippy and a Windows and darwin cargo check pass.

Background

A job is a unit of work sent to the thread pool, with an off-thread part the pool thread may touch and a JS-side part. run either returns the completion token to finish now, or keeps it so another thread can finish later; finishing makes the JS thread complete and free the whole job.
FileOpener is the trait shared by the Blob read, write and copy state machines. get_fd finds the fd (already open, given as an fd, or by opening the path, retrying after mkdir) and then calls a continuation that takes the task over.
Protectors: in Rust's aliasing models (Stacked Borrows and Tree Borrows, which miri enforces) a reference passed as an argument is protected for the whole call. Any access to that memory not derived from the reference, and any deallocation of it, is UB even if the reference is never used again. Raw pointers make no such claim.
ReadFileUV is the Windows read path. libuv callbacks drive it instead of the pool, and it frees itself when it finishes.

Original description

What

JobContext::run (src/jsc/job.rs) received the job's off-thread part as &mut, from Job::run_on_pool's C::run(&mut (*this).off, ..). FileOpener::get_fd / get_fd_by_opening (src/runtime/webcore/Blob.rs) took &mut self and a continuation typed fn(&mut Self, Fd), invoked as callback(self, fd). For a ReadFile or WriteFile that is the whole way into the object:

run_on_pool: C::run(&mut (*job).off, ..)
  -> ReadFile::run(&mut self) -> run_async(&mut self)
    -> get_fd(&mut self, ..) -> get_fd_by_opening(&mut self, ..) -> callback(self, fd)
      -> run_async_with_fd(&mut self)        // ends in on_finish / wait_for_readable / do_read_loop

and the last step hands the object on: to the io thread, or, by finishing the Completion, to the JS thread, whose Job::complete reads and frees the whole job through the job's own pointer. ReadFileUV (Windows) reaches its continuation the same way and can free the task from it. No crash is known; this is the contract shape that #37681 / #37705 / #37768 / #37787 are converting elsewhere, and the one chain #37787 deliberately leaves as is (its run_async_with_fd / run_with_fd comments name these two traits as the remaining piece).

Why the reference shape is wrong

A reference passed as an argument is protected until the call returns, in Stacked Borrows and in Tree Borrows (what bun run rust:miri uses). Once the job has been handed on, the thread that finishes it accesses and deallocates memory those references cover, through a pointer that is not derived from them, and the pool thread has not necessarily returned through the six frames above yet. For a read that finishes in its first step (an open error, an empty file: prepare_read returns Finish and on_finish posts the token at once) the JS thread is idle and gets there while those frames are still unwinding as a matter of course; for the polled endings it is a race with the io thread and a second pool thread. Both models reject a foreign access to memory a protected reference covers, and any deallocation of such memory, whether or not the reference is used again; ReadFileUV freeing itself from under the continuation's own &mut is rejected for the same reason (the self-receiver-reclaim lint's case). A raw pointer makes no claim, so the same sequence is fine once the frames carry one. A standalone reduction of the four shapes under miri is below; the two reference shapes are rejected and the two pointer shapes accepted, and the one thing that is not rejected (another thread writing through a pointer derived from the reference, joined before the return) is also the one thing these frames do not rely on.

Tree Borrows reduction (MIRIFLAGS=-Zmiri-tree-borrows cargo miri run)

struct Job { off: Off }
struct Off { state: u32 }
#[derive(Clone, Copy)] struct Token(*mut Job);   // what Completion holds: the job's own pointer
unsafe impl Send for Token {}

// JobContext::run as it was: the body finishes the token, the JS thread completes the job.
fn run_by_ref(off: &mut Off, done: Token) {
    off.state = 1;
    std::thread::spawn(move || { let done = done; drop(unsafe { Box::from_raw(done.0) }) }).join().unwrap();
}
// JobContext::run as it is now.
unsafe fn run_by_ptr(off: *mut Off, done: Token) {
    unsafe { (*off).state = 1 };
    std::thread::spawn(move || { let done = done; drop(unsafe { Box::from_raw(done.0) }) }).join().unwrap();
}
// OpenCallback as it was / as it is for ReadFileUV, which frees the task itself.
fn continuation_by_ref(task: &mut Off) { task.state = 1; drop(unsafe { Box::from_raw(task as *mut Off) }); }
unsafe fn continuation_by_ptr(task: *mut Off) { unsafe { (*task).state = 1 }; drop(unsafe { Box::from_raw(task) }); }

fn main() {
    let job = Box::into_raw(Box::new(Job { off: Off { state: 0 } }));
    // run_by_ref(unsafe { &mut (*job).off }, Token(job));
    //   error: Undefined Behavior: reborrow through <1533> at alloc787[0x0] is forbidden
    //   help: the accessed tag <1533> is foreign to the protected tag <1634> (i.e., it is not a child)
    //   help: protected tags must never be Disabled        (<1533> = job, <1634> = off)
    unsafe { run_by_ptr(&raw mut (*job).off, Token(job)) };                       // ok
    let task = Box::into_raw(Box::new(Off { state: 0 }));
    // continuation_by_ref(unsafe { &mut *task });
    //   error: Undefined Behavior: deallocation through <1871> at alloc891[0x0] is forbidden
    //   help: the allocation of the accessed tag <1871> also contains the strongly protected tag <1862>
    //   help: the strongly protected tag <1862> disallows deallocations   (<1862> = task)
    unsafe { continuation_by_ptr(task) };                                           // ok
}

Fix

JobContext::run is unsafe fn run(off: *mut Self::OffThread, vm, done), with the contract on the trait; run_on_pool passes &raw mut (*this).off. The 16 synchronous implementations reborrow for their work and return done as before: the ones whose body was already a call do it through (*this).run() (or a field copy), and the longer inline bodies (ZstdJob, WalkTask, Pbkdf2Job, RandomFillJob) move unchanged into a &mut self method the shim calls. The recursive readdir scan, which keeps done and fans out, sets the token and calls perform_work through the pointer; that is as far as this PR takes it. perform_work, write_results and finish_concurrently still take &mut self, the hand-over (done.finish() by whichever thread decrements subtask_count to zero) happens inside them, and the subtasks form their own overlapping &mut to the shared task through ParentRef::assume_mut, so for this job only the carrier frame is converted here. The rest is a separate change of its own (the state the subtasks share wants to be reached through &self / the pointer, and finish_concurrently wants the ReadFile treatment); it has been filed as such and is not started by this PR.
FileOpener: pub type OpenCallback<T> = unsafe fn(*mut T, Fd); get_fd / get_fd_by_opening take this: *mut Self, make their accessor calls through call-scoped reborrows, and invoke the continuation last. The POSIX open loop is open_pathlike(&mut self) -> Fd, so get_fd_by_opening there is "open, then continue". The Windows branch (libuv open) is converted to the same shape because the callback type forces it: the completion thunk reads and cleans up the request before touching the task, and the stash hooks take an OpenCallback. blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 removes that branch and ReadFileUV's use of the trait altogether; the conversion here is the minimum that keeps it compiling until then.
ReadFile / WriteFile: run_async(this: *mut Self, task) replaces run + run_async; the continuations run_async_with_fd / run_with_fd take the pointer, get the decision from prepare_read / prepare_write (the old bodies, returning a Next instead of calling the step) under a reborrow that has ended by the time the step runs, and perform the step through the pointer. This split is the same one io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 makes (same enum, same function names, same edits to the bodies) so that the two merge cleanly; whichever lands second keeps one copy and routes the shim to proceed. The steps themselves (wait_for_*, on_finish, do_close, the read/write loops) still take &mut self here and are io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787's; the WorkPool hand-overs in these files are Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768's; neither is touched. ReadFileUV::on_file_open becomes the continuation on Windows (its on_finish -> finalize is blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705's).

Tests

test/internal/source-lints/self-receiver-job-start.test.ts makes four checks: the first parameter of fn run inside the JobContext declaration and every impl block must be a raw pointer; so must the first parameter of get_fd / get_fd_by_opening inside the FileOpener trait; the OpenCallback<T> definition must read unsafe fn(*mut T, Fd) (the three continuations are passed as fn items where the alias is expected, so pinning the alias pins their signatures through the type checker, and reverting the alias alone fails the lint); and, as a net under that, a continuation type spelled fn(&mut X, Fd) in any parameter syntax is banned anywhere. The anchored checks also record what they examined and assert they found the declaration in job.rs plus at least ten implementations, exactly the two FileOpener entry points and exactly one alias definition, so renaming or moving any of them fails the lint instead of emptying it; each regex has banned / allowed / not-examined fixtures. With src/ at the base commit it reports the trait declaration plus all 19 implementations, both FileOpener entry points and the 11 continuation spellings (Blob.rs, read_file.rs incl. ReadFileUV, write_file.rs), and the alias check reports it missing; with this branch it passes, as does the rest of test/internal/source-lints/.

Behaviour is meant to be unchanged. The converted frames are exercised by the existing coverage of each ending: test/js/bun/util/bun-file.test.ts and bun-write.test.js (ENOENT on read and write, i.e. open_pathlike failing and the job finishing in its first step; the createPath cases go through try_mkdirp), bun-file-fd-read.test.ts (the opened_fd branch of get_fd, empty file), test/regression/issue/07500 and bun-stdin-slice.test.ts (a pipe: prepare_read -> WaitForReadable), bun-write.test.js's pipe and fd cases (prepare_write -> wait / loop, CopyFile), plus the files covering the other implementations.

Verification

Debug (ASAN) build: the files above, bun-file-read, zstd, password, glob/scan, pbkdf2, scrypt, crypto-random, hkdf-callback-null, web/streams/compression, archive, node/fs/promises, fs.test.ts -t readdir (the recursive scan), bundler/transpiler/transpiler.test.js, and the test-crypto-{prime,hkdf,keygen-async-*,secret-keygen,dh-stateless,sign-verify,random,pbkdf2,scrypt} parallel tests all pass. Two things time out in this container independently of the change: bun-write.test.js's 256 MB copy-without-copy_file_range test (the test's own Bun.hash of 256 MB takes 1.7 s each way under ASAN; the same test is noted in #37787) and glob/scan.test.ts's six node_modules cases, where the fast-glob reference run the test awaits alongside takes 63 s under the debug build while Bun.Glob's scan job returns the same 17325 entries in 1.8 s. cargo clippy -p bun_jsc -p bun_runtime is clean; cargo check -p bun_runtime passes for x86_64-pc-windows-msvc (the libuv branch and ReadFileUV) and aarch64-apple-darwin; cargo fmt is clean.

JobContext::run took the job's off-thread part as &mut, and FileOpener::get_fd / get_fd_by_opening and their fn(&mut Self, Fd) continuation took the task as &mut as well. ReadFile and WriteFile hand the object on from inside that chain (to the io thread, or by finishing the completion token, after which the JS thread reads and frees the job through its own pointer), and ReadFileUV's continuation can free it, so every one of those reference arguments was still protected on the publishing thread's stack while that happened. Job::run_on_pool now passes &raw mut (*job).off to an unsafe fn run(off: *mut OffThread, ..); the synchronous implementations reborrow for their work (inline, or through a &mut self helper) and return the token as before. FileOpener::get_fd / get_fd_by_opening take this: *mut Self and an OpenCallback<Self> = unsafe fn(*mut Self, Fd), do their accessor calls through call-scoped reborrows and invoke the continuation last; the POSIX open loop moves into open_pathlike(&mut self). ReadFile::run_async / WriteFile::run_async take the pointer, and the continuations (run_async_with_fd / run_with_fd) decide the next step in prepare_read / prepare_write under a reborrow that ends before the step runs, then perform it through the pointer. ReadFileUV's on_file_open is the same continuation shape on Windows. test/internal/source-lints/self-receiver-job-start.test.ts bans the reference shapes: a JobContext run (declaration or impl) whose first parameter is not a pointer, FileOpener entry points taking self, and an open continuation spelled fn(&mut Self, Fd).

coderabbitai · 2026-08-12T12:20:38Z

Warning

Review limit reached

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

Next review available in: 3 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: 94c9d7b1-19b3-493d-b28d-5e3c1c3d629d

📥 Commits

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

📒 Files selected for processing (18)

src/jsc/JSSecrets.rs
src/jsc/job.rs
src/runtime/api/Archive.rs
src/runtime/api/BunObject.rs
src/runtime/api/JSTranspiler.rs
src/runtime/api/glob.rs
src/runtime/crypto/PBKDF2.rs
src/runtime/crypto/PasswordObject.rs
src/runtime/dns_jsc/dns.rs
src/runtime/image/Image.rs
src/runtime/node/node_crypto_binding.rs
src/runtime/node/node_fs.rs
src/runtime/webcore/Blob.rs
src/runtime/webcore/CompressionStreamCoder.rs
src/runtime/webcore/blob/copy_file.rs
src/runtime/webcore/blob/read_file.rs
src/runtime/webcore/blob/write_file.rs
test/internal/source-lints/self-receiver-job-start.test.ts

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

robobun · 2026-08-12T12:23:17Z

Status: fix is up in this PR.

Reproduced how: test/internal/source-lints/self-receiver-job-start.test.ts against src/ at the base commit reports the JobContext::run declaration and its 19 implementations, both FileOpener entry points and the 11 fn(&mut Self, Fd) spellings (and the missing OpenCallback alias), and passes on this branch; the aliasing argument itself is the miri reduction in the description (reference shapes rejected under Tree Borrows, pointer shapes accepted).

Verified: debug (ASAN) build on Linux with the tests listed in the description; native Windows debug build of 3ace166 (the libuv branch of get_fd_by_opening and ReadFileUV::on_file_open): bun-file, bun-file-fd-read, bun-file-read, bun-file-windows 14 pass / 3 skip / 0 fail, bun-write.test.js 41 pass / 7 skip / 0 fail, 07500 + bun-stdin-slice 1 pass / 2 skip / 0 fail; cargo check for x86_64-pc-windows-msvc and aarch64-apple-darwin; clippy and fmt clean.

claude

I reviewed this PR and the bug-hunting pass found no issues. Because it changes the JobContext::run trait signature and FileOpener::get_fd across 18 files — core pool-job and file-I/O infrastructure with extensive new unsafe blocks, a restructured Windows libuv open path, and merge-ordering ties to #37705/#37768/#37787 — a human look is still worthwhile.

What was reviewed:

All 19 JobContext::run conversions: synchronous impls reborrow-and-return done; the three that hand off (ReadFile, WriteFile, readdir-recursive) make the hand-over their last access.
FileOpener rewrite: POSIX open_pathlike extraction preserves the ENOENT/mkdirp retry loop; Windows wrapped_callback reads result and runs uv_fs_req_cleanup before touching the task, and req.data is set before uv_fs_open (sync-failure ordering preserved).
prepare_read/prepare_write → Next split: each early-return branch maps 1:1 to the step it previously called; the collapsed could_block && NotReady condition is equivalent.
The new source lint's itemBlock bounding and POINTER_PARAM regex against the fixture cases in the test.

Extended reasoning...

Overview

This PR converts two trait entry points from &mut receivers to raw-pointer parameters to fix a Stacked/Tree Borrows protector violation: JobContext::run (src/jsc/job.rs, the pool-thread body of every Job<C>) and FileOpener::get_fd/get_fd_by_opening plus its continuation type (src/runtime/webcore/Blob.rs). It updates all 19 JobContext implementations, restructures ReadFile/WriteFile's first pool step around a Next enum so the hand-over runs after the &mut reborrow ends, converts ReadFileUV::on_file_open (Windows) to the new OpenCallback shape, and adds a source-lint test enforcing the pointer signature.

Security risks

None identified. This is an aliasing-model soundness fix with no user-facing behavior change; no new inputs are parsed, no auth/crypto/permission logic is touched (the crypto-job files only get a mechanical signature update).

Level of scrutiny

High. src/jsc/job.rs is the carrier for every off-thread job in the runtime, and the FileOpener path is the entry to all Bun.file() reads/writes. The change adds ~30 new unsafe blocks with SAFETY comments whose correctness depends on reborrow-scoping arguments, and significantly restructures the Windows libuv get_fd_by_opening thunk (request cleanup ordering, req.data assignment before dispatch). The PR is one of a coordinated series (#37681/#37705/#37768/#37787) with explicit merge-ordering notes — a maintainer tracking that series should confirm the intended landing order.

Other factors

The PR description is unusually thorough (miri reduction, per-ending test coverage map, cross-target cargo check). The bug hunter found nothing; I spot-checked the Windows wrapped_callback ordering change (uv_fs_req_cleanup now runs before the task's error fields are set rather than after — harmless, since cleanup only frees libuv-internal request state and result is copied out first), the prepare_read/prepare_write branch-to-Next mapping, and the readdir-recursive perform_work call sequencing. All look behavior-preserving. Deferring on scope and infrastructure criticality, not on any specific concern.

github-actions · 2026-08-12T12:39:34Z

This PR may be a duplicate of:

io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 - Adds the same enum Next and identical prepare_read/prepare_write extraction out of run_async_with_fd/run_with_fd in blob/read_file.rs and blob/write_file.rs, for the same Tree Borrows motivation.
blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 - Rewrites the same FileOpener open-callback surface (set_open_callback/open_callback/get_fd_by_opening, ReadFileUV::on_file_open) to fix the same UB, with mutually exclusive edits to those functions.

🤖 Generated with Claude Code

robobun · 2026-08-12T12:41:23Z

Not duplicates, but both are deliberately adjacent:

io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 converts the steps below this chain (IoRequestLoop::schedule, wait_for_readable / wait_for_writable, on_finish, FileCloser::do_close, the read/write loops) and leaves JobContext::run and FileOpener::get_fd as they are; its comments on run_async_with_fd / run_with_fd name those two traits as the remaining piece, and this PR is that piece. The Next / prepare_read / prepare_write split appears in both on purpose: it is the same edit so that the two branches merge without a semantic conflict, and whichever lands second keeps a single copy (here the shim routes to its proceed). After both land, every frame from run_on_pool to the hand-over carries a pointer; each one alone converts only its own frames.
blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 frees CopyFileWindows and ReadFileUV through the task pointer and, as part of that, stops ReadFileUV using FileOpener at all (it deletes the Windows hooks and makes get_fd POSIX-only); it keeps get_fd(&mut self, callback: fn(&mut Self, Fd)). This PR changes that signature and the continuation type, and the POSIX ReadFile / WriteFile chain behind it, which blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 does not touch. The Windows edits here are only what the new callback type forces on the code that still exists on main; once blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 lands they go away in whichever order the two merge.

robobun · 2026-08-12T15:05:32Z

^{Updated 11:29 AM PT - Aug 12th, 2026}

❌ @robobun, your commit dd27fce has 2 failures in Build #93497 (All Failures):

test/js/node/test/parallel/test-cluster-shared-leak.js - timeout on 🪟 11 aarch64
scripts/build/ci.ts - build failed on 🍎 aarch64 - build-bun
scripts/build/ci.ts - build failed on 🐧 aarch64-musl - build-bun
scripts/build/ci.ts - build failed on 🐧 x64-android - build-bun

🧪 To try this PR locally:

bunx bun-pr 37820

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

bun-37820 --bun

…atch The continuation type now only exists as the OpenCallback<T> alias, so the lint checks the alias definition itself (unsafe fn over *mut) and widens the spelled-out net to any fn(&mut X, Fd). The anchored checks record what they examined and assert the JobContext declaration, the two FileOpener entry points and the one alias were found, so a rename fails the lint rather than emptying it. The readdir scan's run comment says that its hand-over still happens inside perform_work's &mut self.

github-actions · 2026-08-12T17:25:29Z

+    /// must touch only the off-thread part.
+    ///
+    /// `off` is a pointer rather than `&mut` because a body that keeps `done`
+    /// hands the job on before it returns (to an io loop, to sub-tasks, or, by
+    /// finishing `done`, to the JS thread), and whoever ends up finishing it
+    /// makes the JS thread read and free the allocation through the job's own
+    /// pointer (`Job::complete`), possibly before this thread has returned
+    /// from here. A reference argument is protected for the whole call, and an
+    /// access through another pointer to memory a protected reference covers
+    /// is UB under the aliasing model whether or not the reference is used
+    /// again; a raw pointer makes no such claim. A body that completes
+    /// synchronously just reborrows for the work and returns `done`; one that
+    /// hands the job on does its own work through reborrows that end before the
+    /// hand-over, and makes the hand-over its last access.
+    ///
+    /// # Safety
+    /// `off` is the live job's off-thread part, and nothing else touches it
+    /// until this returns `Some(done)` or the body hands the job on.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:30Z

+        // On `None` the body handed the job on (it may already be freed), and
+        // nothing below touches `*this`: releasing `vm` goes through our own
+        // `handle` clone, not the job's.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:31Z

+/// What [`FileOpener::get_fd`] continues with once the task's fd is known: the
+/// fd, or `Fd::INVALID` with `errno` / `system_error` set when the open failed.
+/// It takes the task over: the implementations end by handing it on
+/// (`ReadFile`, `WriteFile`; the JS thread then reads and frees the job through
+/// the job's own pointer) or by freeing it right there (`ReadFileUV`). Neither
+/// is allowed while a `&mut Self` argument is still protected, i.e. until the
+/// continuation has returned (see [`bun_jsc::JobContext::run`]); hence the
+/// pointer, here and in the `get_fd` frames that invoke it.
+///
+/// # Safety
+/// `this` is the live task `get_fd` was given, and the caller does not touch it
+/// afterwards.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:32Z

+    /// Opens the path in `pathlike()` and records the outcome: the fd in
+    /// `opened_fd`, or `Fd::INVALID` plus `errno` / `system_error`. Returns
+    /// what it recorded.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:33Z

+                                // `mkdir_if_not_exists` already populated
+                                // `errno`/`system_error` on the impl.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:34Z

+    /// [`get_fd`](Self::get_fd) for a task whose `pathlike()` is a path.
+    ///
+    /// # Safety
+    /// As [`get_fd`](Self::get_fd).


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:35Z

+    /// Finds the task's fd (`opened_fd` if it is already set, the descriptor
+    /// of an fd-backed `pathlike()`, or else by opening the path) and continues
+    /// with `callback`, which takes the task over.
+    ///
+    /// # Safety
+    /// `this` is the live task and nothing else is using it; the caller does
+    /// not touch it afterwards, since `callback` hands it on or frees it (see
+    /// [`OpenCallback`]).


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:35Z

+/// What [`ReadFile::prepare_read`] decided the read continues with; performed
+/// by `run_async_with_fd` once that `&mut self` stage has returned.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:36Z

+    /// The job's first pool step: keeps the token and starts the read. From
+    /// `get_fd` on, `*this` belongs to whichever thread the read continues on.
+    ///
+    /// # Safety
+    /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch
+    /// `*this` afterwards.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:37Z

+    /// The read's [`OpenCallback`](crate::webcore::blob::OpenCallback): decides
+    /// the next step under a reborrow that ends before the step runs, because
+    /// the step hands `*this` on (to the io thread, or to the JS thread, which
+    /// frees it).
+    ///
+    /// # Safety
+    /// `OpenCallback`'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:38Z

+    /// The read's [`OpenCallback`]: queues the fstat, or finishes (which frees
+    /// the task) if the open failed or the fstat cannot be queued.
+    ///
+    /// # Safety
+    /// `OpenCallback`'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:38Z

+/// What [`WriteFile::prepare_write`] decided the write continues with;
+/// performed by `run_with_fd` once that `&mut self` stage has returned.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:39Z

+    /// The job's first pool step: keeps the token and starts the write. From
+    /// `get_fd` on, `*this` belongs to whichever thread the write continues on.
+    ///
+    /// # Safety
+    /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch
+    /// `*this` afterwards.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T17:25:40Z

+    /// The write's [`OpenCallback`](crate::webcore::blob::OpenCallback):
+    /// decides the next step under a reborrow that ends before the step runs,
+    /// because the step hands `*this` on (to the io thread, or to the JS
+    /// thread, which frees it).
+    ///
+    /// # Safety
+    /// `OpenCallback`'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

claude

I reviewed this PR and the automated bug-hunting pass found no issues. Given the scope — a core trait signature change (JobContext::run and FileOpener::get_fd/OpenCallback) across ~19 implementations, all of it unsafe cross-thread pointer handling in the work-pool and blob I/O paths, plus deliberate overlap with #37787 and #37705 that needs merge-order coordination — a human review is still warranted.

What was checked:

Each converted JobContext::run impl either reborrows-and-returns done (synchronous jobs) or passes the pointer through without a protected reference on the hand-over path (ReadFile/WriteFile/readdir scan).
FileOpener::get_fd/get_fd_by_opening and the Windows wrapped_callback thunk: reborrows end before the continuation runs; req.data is set before uv_fs_open so a synchronous failure path is sound.
The prepare_read/prepare_write extraction preserves the original branch order and side effects; the Next dispatch matches the calls it replaced.
The new source-lint's anchor assertions guard against the checks going vacuously empty on rename.

Extended reasoning...

Overview

This PR converts two trait entry points — JobContext::run (src/jsc/job.rs) and FileOpener::get_fd/get_fd_by_opening (src/runtime/webcore/Blob.rs) — from taking &mut to taking *mut, and threads that change through every implementation (19 JobContext impls across 15 files) and the three continuations (ReadFile::run_async_with_fd, WriteFile::run_with_fd, ReadFileUV::on_file_open). The motivation is Stacked/Tree Borrows: a &mut argument is protected until the call returns, but bodies that keep the Completion hand the job to another thread (which may free it through the job's own pointer) before returning, which is a protector violation. It also adds a source-lint test enforcing the pointer shape.

Security risks

None identified. This is an aliasing-model soundness fix with no user-facing behavior change; no auth, crypto semantics, path validation, or untrusted-input handling is altered. The crypto/DNS/fs impls only had their run bodies moved into a &mut self helper called through (*this).run().

Level of scrutiny

High. This is unsafe Rust in the hottest cross-thread paths in the runtime (every pool job goes through JobContext::run; every Bun.file() read/write on POSIX goes through FileOpener::get_fd). The Windows libuv branch of get_fd_by_opening was substantially rewritten and per the author's own status comment has only been cargo checked for the Windows target — a native Windows run is noted as "in progress". The change also intentionally duplicates the Next/prepare_* split from #37787 and touches the same FileOpener surface as #37705, so a maintainer should decide landing order.

Other factors

CI build #93427 for the head commit is still running; results are not yet in.
The comment-cop bot has left 13 unaddressed inline comments flagging long doc comments. Several of these are on required # Safety sections for the newly-unsafe fn trait methods (which the repo's own review rules mandate), so they may be false positives — but the author should triage them.
The recursive readdir scan (AsyncReaddirRecursive in node_fs.rs) is explicitly only partially converted here (only the entry frame), with the rest deferred to a follow-up; the PR description acknowledges perform_work still forms overlapping &mut under it.
The bug-hunting system found no issues, and my own read of each converted impl confirms the reborrow-then-return / pointer-pass-through pattern is applied consistently. But the combination of scope, unsafe density, incomplete Windows runtime verification, and merge coordination with two adjacent PRs makes this unsuitable for auto-approval.

github-actions · 2026-08-12T18:12:13Z

+    /// must touch only the off-thread part.
+    ///
+    /// `off` is a pointer, not `&mut`: a body that keeps `done` hands the job
+    /// on before returning, and the JS thread then frees it through the job's
+    /// own pointer (`Job::complete`), which is UB while a reference argument
+    /// is still protected here. Such a body reborrows only for work that ends
+    /// before the hand-over, which is its last access.
+    ///
+    /// # Safety
+    /// `off` is the live job's off-thread part and nothing else touches it
+    /// until this returns `Some(done)` or the body hands the job on.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:14Z

+        // On `None` the job may already be freed; nothing below touches it
+        // (`vm` is released through our own `handle` clone).


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:15Z

+/// [`FileOpener::get_fd`]'s continuation: gets the fd, or `Fd::INVALID` with
+/// `errno` / `system_error` set, and takes the task over. It ends by handing
+/// the task on (`ReadFile`, `WriteFile`) or freeing it (`ReadFileUV`), hence a
+/// pointer rather than a `&mut` that would still be protected then, here and
+/// in the `get_fd` frames that invoke it (as for [`bun_jsc::JobContext::run`]).
+///
+/// # Safety
+/// `this` is the live task `get_fd` was given; the caller does not touch it
+/// afterwards.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:16Z

+    /// Opens `pathlike()`; returns the fd it recorded in `opened_fd`, or
+    /// `Fd::INVALID` after recording `errno` / `system_error`.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:17Z

+    /// Finds the task's fd (already opened, the store's own, or by opening the
+    /// path) and continues with `callback`, which takes the task over.
+    ///
+    /// # Safety
+    /// `this` is the live task and nothing else is using it; the caller does
+    /// not touch it afterwards ([`OpenCallback`] hands it on or frees it).


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:18Z

+    /// First pool step: keeps the token and starts the read; `get_fd` hands `*this` on.
+    ///
+    /// # Safety
+    /// [`bun_jsc::JobContext::run`]'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:19Z

+    /// The read's continuation: `prepare_read`'s reborrow has ended by the
+    /// time the step it chose hands `*this` on.
+    ///
+    /// # Safety
+    /// [`OpenCallback`](crate::webcore::blob::OpenCallback)'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:19Z

+    /// The read's continuation: queues the fstat, or finishes (freeing the task).
+    ///
+    /// # Safety
+    /// [`OpenCallback`]'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:20Z

+    /// First pool step: keeps the token and starts the write; `get_fd` hands `*this` on.
+    ///
+    /// # Safety
+    /// [`bun_jsc::JobContext::run`]'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-12T18:12:21Z

+    /// The write's continuation: `prepare_write`'s reborrow has ended by the
+    /// time the step it chose hands `*this` on.
+    ///
+    /// # Safety
+    /// [`OpenCallback`](crate::webcore::blob::OpenCallback)'s contract.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

claude

I reviewed this PR and didn't find any bugs. Given it changes the JobContext::run signature to unsafe fn(*mut ...) across all 19 implementations, restructures FileOpener's entry points and the ReadFile/WriteFile continuation chain, and is designed to land alongside #37787 / #37705 with intentionally overlapping edits, a human look is warranted.

What was reviewed

The 16 synchronous JobContext::run shims: each reborrow is call/statement-scoped and returns Some(done) — no hand-off, so (*this).run() is sound under the fn contract.
get_fd / get_fd_by_opening (POSIX + the Windows libuv branch): the continuation is the last access on every path; wrapped_callback reads result and cleans up req before touching the task, and req.data is set before uv_fs_open (sync-failure path preserved).
prepare_read / prepare_write split: control flow matches the old bodies branch-for-branch; the &mut self ends before the Next step hands *this on.
AsyncReaddirRecursiveTask: only the entry frame is converted (perform_work still &mut self), matching the description's stated scope.

Extended reasoning...

Overview

The PR converts two trait entry points that carry a job from the pool thread into code that hands the job on to another thread: JobContext::run (src/jsc/job.rs, the declaration and every impl block — 19 sites across 15 files) and FileOpener::get_fd / get_fd_by_opening plus the OpenCallback<T> continuation type (src/runtime/webcore/Blob.rs), from &mut to *mut. ReadFile::run_async_with_fd / WriteFile::run_with_fd are split into a prepare_*(&mut self) -> Next decision followed by a pointer-carried step, and ReadFileUV::on_file_open becomes the continuation on Windows. A new source-lint test pins the four shapes (trait first parameter, FileOpener entry points, the OpenCallback alias, and any fn(&mut X, Fd) spelling).

Security risks

None identified. This is an aliasing-model correctness change to internal unsafe Rust; no user-facing input handling, auth, crypto, or trust boundary is touched. The behavioural surface is meant to be unchanged and is exercised by existing coverage.

Level of scrutiny

High. This is memory-safety-critical unsafe Rust across the work-pool job carrier and the Blob file-open state machine, with hand-written SAFETY comments at every reborrow. The change is well-reasoned (the miri reduction in the description demonstrates the protector violation, and the pointer-carrying shape is the established fix from #37681/#37705/#37768/#37787), but the number of touched implementations, the deliberate overlap with two open PRs that must merge cleanly in either order, and the Windows libuv branch (which #37705 removes) make this a change a human should sign off on rather than an automated approval.

Other factors

The comment-cop bot fired again at 18:12 (after the "Shorten the new doc and SAFETY comments" commit) on ~10 locations in job.rs / Blob.rs / read_file.rs / write_file.rs. The flagged comments read as legitimate design docs (why *mut and not &mut) rather than workaround justifications, but whether they need further trimming is a style call for a human.
CI on 3ace166 shows failures (complex-workspace.test.ts, bake/deinitialization.test.ts segfault on Windows, freebsd aarch64 / linux aarch64-musl build failures) that look unrelated to this change but should be confirmed pre-existing before merge; no CI result is visible yet for dd27fce.
The PR intentionally leaves AsyncReaddirRecursiveTask::perform_work and the steps below run_async_with_fd / run_with_fd on &mut self for the sibling PRs; a reviewer should confirm the merge plan with #37787 / #37705 (which one lands first, and that the shared Next/prepare_* edits collapse cleanly).

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

claude Bot reviewed Aug 12, 2026

View reviewed changes

robobun mentioned this pull request Aug 12, 2026

node:fs: share the recursive readdir scan between pool threads by pointer, not &mut self #37861

Open

github-actions Bot reviewed Aug 12, 2026

View reviewed changes

claude Bot reviewed Aug 12, 2026

View reviewed changes

robobun mentioned this pull request Aug 12, 2026

shell rm: drop a DirTask's own count from a pointer-only frame #37907

Open

Shorten the new doc and SAFETY comments

dd27fce

github-actions Bot reviewed Aug 12, 2026

View reviewed changes

claude Bot reviewed Aug 12, 2026

View reviewed changes

		// `mkdir_if_not_exists` already populated
		// `errno`/`system_error` on the impl.

		/// What [`ReadFile::prepare_read`] decided the read continues with; performed
		/// by `run_async_with_fd` once that `&mut self` stage has returned.

		/// What [`WriteFile::prepare_write`] decided the write continues with;
		/// performed by `run_with_fd` once that `&mut self` stage has returned.

		// On `None` the job may already be freed; nothing below touches it
		// (`vm` is released through our own `handle` clone).

		/// Opens `pathlike()`; returns the fd it recorded in `opened_fd`, or
		/// `Fd::INVALID` after recording `errno` / `system_error`.

Uh oh!

Conversation

robobun commented Aug 12, 2026 • edited by alii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

What

Why the reference shape is wrong

Fix

Tests

Verification

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

robobun commented Aug 12, 2026

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

robobun commented Aug 12, 2026 •

edited by alii

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

robobun commented Aug 12, 2026 •

edited

Loading

robobun commented Aug 12, 2026 •

edited

Loading