s3: post the streaming download's final task through a copy of the handle

[robobun]

Problem

• No crash is known. On the final chunk of every streaming S3 download (s3file.stream(), or reading an S3File as a ReadableStream) the HTTP thread posts its hand-back task by calling post_task on the loop_handle field stored inside the request's own allocation.
• The JS thread frees that allocation as soon as it sees the final chunk, which can be while post_task is still running on the HTTP thread with &self pointing into it. Freeing memory behind a live reference argument is undefined behaviour whether or not the callee reads through it again.
• A standalone reduction of this shape fails under Miri with Tree Borrows (deallocation through <tag> ... is forbidden, protected tags must never be Disabled) and with Stacked Borrows; the same reduction posting through a copy of the handle passes.
• This site copied the VM pointer out before Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 moved it to LoopHandle. The other three hand-offs in the directory already post through a clone.

Fix

• Clone the handle out before processing the callback, then post the task and hand the request back through the clone. The separate clone taken only on the final callback goes away, since one copy serves both calls.
• Correct because the clone lives on the HTTP thread's stack, so nothing borrows the request allocation at either point where the JS thread may free it. The same task reaches the same queue at the same point; the cost is one Arc increment and decrement per non-final callback.
• Adds a source lint over src/runtime/webcore/s3/ that bans posting or handing back through a stored loop_handle. On main it reports exactly the one line this PR changes.
• Verification: the Miri failure is on the reduction, not on bun. On bun itself an ASAN streaming stress script and the S3 test files pass with the change; there was no failing repro to flip.

Background

• LoopHandle: a cloneable, Arc backed handle to a JS VM's event loop. post_task queues a task for the JS thread and wakes it; embedded_work_finished tells the VM one piece of off-thread work is done, which the VM waits for before closing.
• S3 requests carry their own task node: the request struct embeds a concurrent_task, so posting it hands the JS thread the very allocation the handle field lives in, and the JS side (on_response) frees that allocation on the final delivery.
• Protected references: under Rust's aliasing models (Stacked Borrows, Tree Borrows) a &self argument must stay valid for the whole call, and codegen marks it dereferenceable. Freeing the memory it points into mid-call is UB even from another thread and even if the callee never touches it again.
• Miri (bun run rust:miri) is the interpreter that checks those models; it reports this class of bug where ASAN and normal tests see nothing.
• test/internal/source-lints/ holds regex tests over the source tree that stop a fixed pattern from coming back. This one is scoped to the s3 directory because the same spelling is sound in places where something else keeps the object alive across the post.

Original description

Problem

S3HttpDownloadStreamingTask::http_callback (src/runtime/webcore/s3/download_stream.rs) is how the HTTP thread delivers each chunk of a streaming S3 download (s3file.stream(), anything else that reads an S3File as a ReadableStream). It posted the task with

let bun_jsc::vm_handle::Posted::Queued = (*this).loop_handle.post_task(task) else { ... };

The task it posts is the request's own allocation (the inline concurrent_task field), and on the final callback the JS-thread consumer, on_response, frees that allocation (drop(heap::take(this_ptr))) as soon as it observes has_more == false, which it can do the moment VmHandle::post has pushed the task and woken the loop. post_task(&self) and the VmHandle::post(&self, ..) it calls are still running on the HTTP thread at that point, and their &self is &(*this).loop_handle: a reference into the allocation being freed. A reference argument is protected for the duration of its call, and freeing memory a protected reference points into is UB under both aliasing models whether or not the callee reads through it again (codegen relies on the same contract: the argument is annotated dereferenceable for the whole call). Nothing does read through it after the push today, so no crash is known; the contract is what is wrong, and it is wrong on the final callback of every streaming download.

The function already clones the handle for the embedded_work_finished() that follows the post, for exactly this reason ("after this may have been freed"), and the directory's other hand-offs (release_at_shutdown in the same file, and both S3HttpSimpleTask::http_callback and its release_at_shutdown in simple_request.rs) already post through the clone. Before #37075 this site copied the VM pointer out before enqueueing; the move to LoopHandle turned that into a call through the field.

A reduction of this exact shape (the consumer on a second thread frees the allocation while the poster is still inside post_task(&self), and post_task reads nothing from the allocation after the push) fails under Miri with Tree Borrows, the model bun run rust:miri uses:

error: Undefined Behavior: deallocation through <9276> at alloc1564[0x0] is forbidden
  = help: the accessed tag <9276> is foreign to the protected tag <6549> (i.e., it is not a child)
  = help: this deallocation (acting as a foreign write access) would cause the protected tag <6549> (currently Frozen) to become Disabled
  = help: protected tags must never be Disabled
help: the protected tag <6549> was created here, in the initial state Frozen
   |     fn post_task(&self, task: SendPtr) {
   |                  ^^^^^

and under Stacked Borrows with not granting access to tag <2994> because that would remove [SharedReadOnly for <6802>] which is strongly protected, where <6802> is the same &self. The same reduction posting through a copy of the handle passes under both.

Reduction run under Miri

use std::sync::{mpsc, Arc, Mutex};
use std::thread;

struct SendPtr(*mut Task);
unsafe impl Send for SendPtr {}

// `VmHandle`: an Arc to shared state the VM keeps alive; `LoopHandle` clones it.
struct Shared {
    queue: Mutex<mpsc::Sender<SendPtr>>,
    freed: Mutex<mpsc::Receiver<()>>,
}
#[derive(Clone)]
struct LoopHandle(Arc<Shared>);

impl LoopHandle {
    // `LoopHandle::post_task(&self, task)`: push, wake the loop, return. The JS
    // thread may run the task (and free it) at any point after the push; the
    // `recv` pins that interleaving, standing in for the work `VmHandle::post`
    // still does after the push (wakeup, dropping its access guard).
    fn post_task(&self, task: SendPtr) {
        // As `VmHandle::post`: everything it needs from the handle is read
        // before the push (its `Access` guard holds a `&Shared` into the Arc
        // payload, which the VM keeps alive); after the push it touches only
        // that. Nothing reads the handle field itself after the push.
        let shared: &Shared = &self.0;
        shared.queue.lock().unwrap().send(task).unwrap();
        shared.freed.lock().unwrap().recv().unwrap();
    }
}

// `S3HttpDownloadStreamingTask`: the handle lives inside the allocation the
// task hands over.
struct Task {
    loop_handle: LoopHandle,
    state: u64,
}

// main
fn http_callback_before(this: *mut Task) {
    unsafe { (*this).state = 0 };
    unsafe { (*this).loop_handle.post_task(SendPtr(this)) };
}

// this PR
fn http_callback_after(this: *mut Task) {
    let handle = unsafe { (*this).loop_handle.clone() };
    unsafe { (*this).state = 0 };
    handle.post_task(SendPtr(this));
}

fn main() {
    let (queue, tasks) = mpsc::channel::<SendPtr>();
    let (freed_tx, freed) = mpsc::channel::<()>();
    let vm = LoopHandle(Arc::new(Shared {
        queue: Mutex::new(queue),
        freed: Mutex::new(freed),
    }));
    // The JS thread: `on_response` sees the final state and frees the task.
    let js_thread = thread::spawn(move || {
        let task = tasks.recv().unwrap();
        drop(unsafe { Box::from_raw(task.0) });
        freed_tx.send(()).unwrap();
    });
    let task = Box::into_raw(Box::new(Task {
        loop_handle: vm.clone(),
        state: 1,
    }));
    if std::env::args().nth(1).as_deref() == Some("after") {
        http_callback_after(task);
    } else {
        http_callback_before(task);
    }
    js_thread.join().unwrap();
}

MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- before and the default (Stacked Borrows) run both exit 1 with the errors quoted above; -- after exits 0 under both.

Fix

http_callback clones the handle out unconditionally before process_http_callback, posts through the clone, and calls embedded_work_finished() on it when is_done, the shape of the three sibling hand-offs. The conditional done_handle is gone since the one copy now serves both calls; on non-final callbacks this adds one Arc increment and decrement per callback, next to the mutex that callback already takes. Behaviour is unchanged: the same task goes to the same queue at the same point, and the request is handed back at the same point.

The same spelling exists outside this directory and is not part of this change. Sites where it is the same bug each have their own fix: the two Windows mkdirp completions in blob/copy_file.rs and blob/write_file.rs (#37705), napi_async_work::post_to_js_thread (#37750), FetchTasklet::deref_from_thread (posts through a &self helper; reported separately) and StatWatcher::post_to_js_thread (reported separately; #37591 reshapes it). The others post while something else keeps the object alive (the bundler's plugin-dispatch thunk in js_bundle_completion_task.rs during the build, the stat watcher scheduler, the threadsafe function under its lock, AsyncModule's leaked WakeContext, the FetchTasklet helper's other two callers, which hold a ref), which is why the spelling cannot simply be banned tree-wide; the FSWatcher::post helper in node_fs_watcher.rs was not examined here beyond noting that its batch holds an activity ref across the post.

Tests

test/internal/source-lints/s3-post-through-handle-field.test.ts bans post_task and embedded_work_finished through a stored loop_handle (method-call and path-call spellings, including rustfmt-wrapped chains) in src/runtime/webcore/s3/, where every object storing a handle is freed by the JS thread on delivery, so the rule holds unconditionally and the allowlist is empty. embedded_work_scheduled() through the field, .clone() and posting through a local are allowed, and the file checks its pattern against both lists. It also checks that the scanned files still declare the field and still post, so a rename cannot blind it silently. Against main it reports exactly

src/runtime/webcore/s3/download_stream.rs:295: .loop_handle.post_task(

and passes with this branch.

Verification

On the debug (ASAN) build: a script streaming, 150 rounds of 6 concurrent downloads each, a 512 KiB body, an empty body, a 64-chunk trickled body, the same two with the consumer deliberately late (so that the final callback can arrive with a task already queued, the path where the earlier task is the one that frees), and a 404 whose XML error is reported on the final callback, with GC between rounds, completes without a sanitizer report. test/js/bun/s3/{s3-stream-cancel-leak,s3-stream-error-gc,s3-connection-close,s3-insecure,s3-requester-pays,s3-storage-class,s3-list-objects,s3-list-checksum-algorithm,s3-fd-validation,s3-argument-validation}.test.ts pass (the connection-close and list-objects files only with --timeout raised: their concurrent debug-build subprocesses take 7 to 15 s each in this container, with or without this change; the credentialed s3.test.ts streaming coverage runs in CI). bun test test/internal/source-lints/ (19 files) passes; rustfmt is clean on the changed file.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 21 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: 9ab8975b-e6a3-4e5c-b394-cf9b856646e2

📥 Commits

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

📒 Files selected for processing (2)

• src/runtime/webcore/s3/download_stream.rs
• test/internal/source-lints/s3-post-through-handle-field.test.ts

---

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

[robobun]

Status: fix pushed, self-review in progress.

Reproduced how: the new lint reports src/runtime/webcore/s3/download_stream.rs:295: .loop_handle.post_task( against main and passes with this branch (checked both ways with the debug build); the Miri reduction in the description fails on the old shape under Tree Borrows and Stacked Borrows and passes on the new one. Behaviour is unchanged; the streaming S3 download paths were exercised 900 times under the ASAN build without a report (details in the description).

[claude]

LGTM — brings http_callback into the same clone-then-post shape as the three sibling hand-offs in this directory.

What was reviewed:

• Confirmed simple_request.rs and release_at_shutdown already post through a cloned handle; this change makes http_callback match them exactly.
• The clone is taken before process_http_callback (whose unlock is the other point at which the JS thread can free *this), so both post_task and embedded_work_finished are covered.
• Verified no .loop_handle.post_task/.loop_handle.embedded_work_finished remain in src/runtime/webcore/s3/; the new lint's non-vacuity checks (field declaration + post site present) hold.
• The lint follows the existing test/internal/source-lints/ conventions (globAllSources, git ls-tree guard, self-tested regex).

Extended reasoning...

Overview

Two files changed. In src/runtime/webcore/s3/download_stream.rs, S3HttpDownloadStreamingTask::http_callback is refactored so the LoopHandle is cloned out of *this unconditionally before process_http_callback, and both post_task and embedded_work_finished go through that local copy instead of (*this).loop_handle. The conditional done_handle Option is removed since one clone now serves both calls. The task construction and control flow are otherwise byte-for-byte the same. A new source-lint test, test/internal/source-lints/s3-post-through-handle-field.test.ts, bans the .loop_handle.post_task( / .loop_handle.embedded_work_finished( spellings (method-call and path-call, rustfmt-wrapped) within src/runtime/webcore/s3/, self-tests its regex against banned/allowed sample lists, and asserts the scanned files still declare a loop_handle: LoopHandle field and still call .post_task( so a rename can't blind it silently.

Security risks

None. This is an internal cross-thread hand-off; no user-facing surface, parsing, or trust boundary is touched.

Level of scrutiny

Memory-safety code in unsafe Rust normally warrants high scrutiny, but the change here is mechanical: it converts one call site to the exact shape already used by release_at_shutdown in the same file and both hand-offs in simple_request.rs (verified by grep). The old code already cloned the handle for embedded_work_finished; the diff extends that same clone to cover post_task. The only behavioral delta is one extra Arc inc/dec on non-final callbacks, which the PR description acknowledges and which is negligible next to the mutex the callback already takes. The clone is placed before process_http_callback, which matters: on the final callback with a task already queued, the JS thread can free *this as soon as process_http_callback unlocks, so cloning after would be too late — the placement is correct.

Other factors

The source-lint test follows the established directory conventions closely: it uses globAllSources, the same git ls-tree tracked-file guard as dead-code-escapes.test.ts, strips full-line comments before matching (with the correct [ \t]* to preserve line numbers), self-tests the regex against explicit banned and allowed lists, and guards against vacuous passes by counting field declarations and post sites. The lint is scoped to src/runtime/webcore/s3/ where the rule holds unconditionally, and the PR description enumerates the out-of-scope sites and why they are handled separately. The PR description includes a Miri reduction demonstrating the aliasing violation under both Tree Borrows and Stacked Borrows, and reports the existing S3 test suite and all 19 source-lint files pass on the debug/ASAN build.

[robobun]

^{Updated 2:48 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 5b7b731 has 4 failures in Build #93374 (All Failures):

• test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts - unchecked exception at JSC__JSModuleLoader__loadAndEvaluateModule @ src/jsc/bindings/bindings.cpp:3788 (thrown from vendor/WebKit/Source/JavaScriptCore/runtime/VMTraps.cpp:443) on 🐧 13 x64-asan
• test/js/bun/util/inspect-error-leak.test.js - code 1 on 🐧 13 x64-asan
• test/js/web/timers/setInterval.test.js - code 1 on 🐧 13 x64-asan
• scripts/build/ci.ts - build failed on 🐧 aarch64-android - build-bun

---

🧪   To try this PR locally:

bunx bun-pr 37880

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

bun-37880 --bun