s3: reach the streaming download task through its raw pointer on both threads

[robobun]

Stacked on #38330: this PR needs its Mutex::unlock_raw, so its base is that PR's branch and the diff here is only this PR's own change (3 files). It should land after #38330 (or after #38345, which carries the same Mutex.rs / Futex.rs hunks); once one of them is in I will rebase this onto main.

Problem

• S3HttpDownloadStreamingTask (src/runtime/webcore/s3/download_stream.rs) is one heap object used by two threads at once for its whole life: the HTTP thread records each chunk into it under the task's mutex (http_callback -> process_http_callback -> update_state), and the JS thread reports the chunks under the same mutex (on_response -> report_progress) and frees the task once it has read has_more == false.
• Three of those functions were &mut self methods: process_http_callback and update_state (HTTP thread) and report_progress (JS thread). A &mut self argument is a protected exclusive borrow of the whole task, the mutex word and the atomics included, for the duration of the call (Background). The other thread writes those words whenever it tries to take the lock, which is routine: on_response locks while the HTTP thread is inside process_http_callback, and the HTTP thread locks while report_progress is running the chunk callback. Under Tree Borrows the first such access during the call is undefined behaviour whether or not anything is freed; a reduction of the shape is rejected by Miri at the other thread's lock() (read access through <tag> ... is forbidden ... foreign to the protected tag created at the &mut self; full diagnostics below).
• process_http_callback had a second problem on the final chunk. When a task is already queued (has_schedule_callback set), the final result posts nothing; the JS thread may be blocked in on_response's lock(), and the moment the HTTP thread releases the lock it reads has_more == false and frees the task. At that point the HTTP thread was still inside process_http_callback(&mut self) and inside MutexGuard::drop -> Mutex::unlock(&self), i.e. the freed task was still covered by protected references (&mut self, and &Mutex into it). The comment on the guard claimed it "drops strictly before the task can be freed by the main thread", which is the ordering that does not hold. The guard alone is enough for Miri to reject the free (deallocation through <tag> ... is forbidden, protected tag created at Mutex::unlock(&self)), same class as threading: finish a WaitGroup without holding a reference into it past the release #38330 and http: publish send_sync's result without holding a reference into the channel past the release #38345. release_at_shutdown used the same guard shape.
• Natively benign today: after the releasing store the HTTP thread only returns through those frames (plus an address-keyed futex wake), and the concurrent atomic traffic is real atomics, so there is no crash to reproduce; the contract is what is wrong. Found by analysis while working on threading: finish a WaitGroup without holding a reference into it past the release #38330, and confirmed on the reduction below.

Fix

• process_http_callback, update_state and report_progress become unsafe fn f(this: *mut Self, ..) and reach the task through (*this).field, so every reference they form covers one field for one call (&mut (*this).reported_response_buffer.list for body_into, the &AtomicBool for the exchange, and so on); none of those fields is touched by the other thread while we hold the lock, and the words the other thread does touch (the mutex, signal_store) are never inside a reference we hold. The &self helpers get_state / set_state stay: a shared reference leaves the interior-mutable words to the other thread, and the remaining fields are only written by whoever holds the lock.
• Both HTTP-thread critical sections (process_http_callback, release_at_shutdown) are now (*this).mutex.lock(); ...; Mutex::unlock_raw(&raw const (*this).mutex);, the WaitGroup::finish_raw shape from threading: finish a WaitGroup without holding a reference into it past the release #38330, so the releasing store is the HTTP thread's last access to the task. release_at_shutdown cannot race a free today (by the time the HTTP thread runs it, the task's JS thread is parked in shutdown_for_exit or has been joined); it is converted so that the HTTP-thread side of this mutex has one shape that does not depend on that teardown ordering, and so the lint below holds without exceptions.
• Mutex::unlock_raw becomes pub (it is pub(crate) in threading: finish a WaitGroup without holding a reference into it past the release #38330; the workspace denies unreachable_pub, so pub is exactly "has a user in another crate"), with a doc note on what the caller's own frames have to look like.
• release_portable (only called from Drop) is folded into Drop, so the inherent impl has no &mut self methods left; Drop itself runs after the JS thread has read the final state, i.e. after the HTTP thread's last access, so its &mut self is fine and is documented as such.
• Behaviour is unchanged: same lock, same state transitions, same body_into, same exchange and posts. The early-return ladder at the end of process_http_callback is now one && chain with the same short-circuiting (the exchange still only runs when the old code ran it; setting the flag without posting would stall the stream, which the comment there now says). The Err(prev) arm of the old code was unconditional in practice (a failed exchange from false always reports true), so .is_ok() is the same decision, and the same spelling release_at_shutdown already used.
• Why this is the right shape: what changes is only which references are live while the other thread accesses the task or frees it, which is exactly what the aliasing models and rustc's dereferenceable on reference arguments reason about. It is the tree's existing convention for objects another thread (or a re-entrant callback) can touch during the call: this: *mut Self receivers (src/CLAUDE.md, "Pointer provenance at FFI boundaries", borrow = ptr; self-receiver-reclaim.test.ts), and finish_raw / unlock_raw in threading: finish a WaitGroup without holding a reference into it past the release #38330. The type docs on the struct now state the rule.
• Test: test/internal/source-lints/s3-download-task-raw-access.test.ts bans &mut self receivers inside impl S3HttpDownloadStreamingTask and the two non-raw spellings of the mutex (.mutex.lock_guard() in any spelling, and self.mutex.lock()/unlock()/try_lock()) in src/runtime/webcore/s3/, checks its own patterns against banned and allowed snippets, and checks the scanned file still has the impl block, the mutex: Mutex field and a .mutex.lock( so it cannot pass vacuously. Against the unfixed source it reports exactly report_progress, update_state, process_http_callback, release_portable and the two lock_guard() sections; with this change it passes. Runs in the source-lints workflow on every PR touching src/**/*.rs; the whole test/internal/source-lints/ directory still passes (103 tests).
• Also verified:
  • Miri, on the reduction below pasted into bun_threading's tests (Tree Borrows, 400 iterations each): &mut self + guard is rejected with and without the free; raw pointer + guard is rejected when the other thread frees; raw pointer + unlock_raw (this PR's shape) is clean, with the other thread freeing the box right after the release in 336 of the 400 iterations. The reduction is not committed: it would test a copy of the protocol, not download_stream.rs, which the lint covers; threading: finish a WaitGroup without holding a reference into it past the release #38330's crate test already covers unlock_raw itself.
  • ASAN debug build: a local stress (8 concurrent S3Client.file(..).stream() downloads per round, 200 rounds, over a 512 KiB body, an empty body, a 64-chunk trickled body, a 404 error document and a trickled 500, half of them with a deliberately busy consumer) completes clean; a temporary counter confirmed 274 of 800 downloads in a 100-round run took the "final result with a task already queued" path, the one where the unlock is what frees the task.
  • bun bd test on the local-server S3 files (s3-stream-cancel-leak, s3-stream-error-gc, s3-insecure, s3-requester-pays, s3-storage-class, s3-argument-validation, s3-fd-validation, s3-list-checksum-algorithm, s3-connection-close): pass. s3-list-objects' "fall back to NoSuchKey" case exceeds the 5 s local timeout when the whole file runs on the debug build here, identically on the base build without this change (it passes alone); it uses the non-streaming request type. The credentialed s3.test.ts / s3.leak.test.ts streaming coverage runs in CI. (Locally the S3 tests need HTTP_PROXY unset in this container; S3 requests do not honour NO_PROXY yet, Make S3 requests honor NO_PROXY and HTTPS_PROXY like fetch #32046.)
  • cargo check and cargo clippy --no-deps on bun_threading and bun_runtime, cargo fmt --check; test/internal/threading-miri.test.ts from threading: finish a WaitGroup without holding a reference into it past the release #38330 still passes with the visibility change.
• Not in this PR: the final callback's (*this).loop_handle.post_task(..) in http_callback, which holds a &LoopHandle into the task while the JS thread may free it once the task is queued, is s3: post the streaming download's final task through a copy of the handle #37880 (the two compose; this PR leaves those lines alone apart from the comments next to them). The sibling S3HttpSimpleTask::stage_http_result(&mut self), which only races the teardown-time stop_for_vm_teardown store, and both types' stop_for_vm_teardown reading http while the HTTP thread overwrites it, are reported separately; they do not fit this type's unconditional rule.

Background

• Protected references: under Rust's aliasing models (Stacked Borrows, and Tree Borrows, which bun run rust:miri uses) a reference passed as a function argument is protected until that function returns: the callee may assume nothing else accesses the memory it covers (for &mut, no other access at all; for &, no writes to the non-interior-mutable bytes) and that it is not freed, even if the callee never touches it again. rustc relies on the same thing to mark reference arguments dereferenceable for LLVM. A raw pointer argument asserts nothing, and a reference to a single field covers only that field, which is why the fixed code takes *mut Self and forms field references per access.
• Interior mutability and & vs &mut: atomics and the mutex word are UnsafeCell-based, so a shared reference to the task tolerates the other thread's writes to them; an exclusive reference does not, because &mut claims every byte regardless of interior mutability. That is the difference between the &self helpers that stay and the &mut self methods that go.
• Mutex::unlock_raw (threading: finish a WaitGroup without holding a reference into it past the release #38330): releases through a raw pointer so that the store which releases the lock is the last access to the mutex's memory, and the futex wake that may follow goes by address only. unlock(&self) and MutexGuard::drop both hold a &Mutex until they return, so they cannot end a critical section whose release lets another thread free the owner.
• The task's handoff protocol: the HTTP thread publishes each result under the mutex and posts the task's inline concurrent_task to the JS thread only if no post is outstanding (has_schedule_callback exchange). on_response on the JS thread locks, reads the state, reports, unlocks, and frees the task if the state was final. So a final result arriving while a post is outstanding is delivered purely by the unlock: the JS thread is, or will be, waiting on the mutex and frees the task as soon as it gets it.

Miri reduction (temporarily appended to src/threading/Mutex.rs on top of #38330) and its results

Three writer shapes over a boxed Task { mutex: Mutex, has_more: AtomicBool, _payload: [u8; 64] }; the reader thread (standing in for on_response) is told to go once the writer thread starts, locks, reads has_more, unlocks, and either frees the box when it saw the final state or leaves it to the main thread:

impl Task {
    // main's process_http_callback(&mut self): receiver + guard
    fn receiver_guard(&mut self) {
        let _guard = self.mutex.lock_guard();
        self.has_more.store(false, Ordering::Relaxed);
    }
    // main's release_at_shutdown section: raw pointer, but a guard
    unsafe fn raw_guard(this: *mut Self) {
        let _guard = (*this).mutex.lock_guard();
        (*this).has_more.store(false, Ordering::Relaxed);
    }
    // this PR
    unsafe fn raw_unlock_raw(this: *mut Self) {
        (*this).mutex.lock();
        (*this).has_more.store(false, Ordering::Relaxed);
        Mutex::unlock_raw(&raw const (*this).mutex);
    }
}

bun run rust:miri -p bun_threading -- probe_ (Tree Borrows, 400 iterations per case):

writer	reader frees	result
&mut self + guard	yes	rejected: read access through <tag> at alloc[0x40] is forbidden at Mutex.rs DebugImpl::lock (the reader's lock()), the accessed tag is foreign to the protected tag created at fn receiver_guard(&mut self)
&mut self + guard	no	rejected, same diagnostic (the lock attempt alone is enough)
raw pointer + guard	yes	rejected: deallocation through <tag> at alloc[0x4c] is forbidden, protected tag created at pub fn unlock(&self) (the guard's drop)
raw pointer + guard	no	clean (the guard is only a problem at the free)
raw pointer + unlock_raw	yes	clean; the reader saw the final state and freed right after the release in 336 of 400 iterations

An earlier run of the first case with the reader's lock attempt landing as a write was reported as write access through <tag> at alloc[0x48] is forbidden ... this foreign write access would cause the protected tag (currently Reserved (conflicted)) to become Disabled; protected tags must never be Disabled, at FutexImpl::try_lock's fetch_or. Natively (cargo test) all five cases pass, which is the "benign today" part of the Problem section.

[robobun]

Status: analysis-based fix (no native repro exists; the shape is rejected by Miri on a reduction, see the description). Fail-before / pass-after is test/internal/source-lints/s3-download-task-raw-access.test.ts: against the unfixed source it reports report_progress, update_state, process_http_callback, release_portable and the two lock_guard() sections in download_stream.rs; with this branch it passes. Stacked on #38330 (needs its Mutex::unlock_raw); will rebase onto main once that or #38345 lands. PR: #38351. Waiting on CI.

[claude]

Beyond the inline nit, I traced the new should_enqueue && chain in process_http_callback against the old early-return ladder: the three conditions are equivalent (is_done || !buffer.is_empty() is the De Morgan of the old empty-chunk guard, and compare_exchange(false, true, ..) can only fail with Err(true), so .is_ok() matches the old Err-then-check-flag path exactly), and the exchange still short-circuits away under the same conditions as before, so has_schedule_callback is never set without a post. body_into still runs before the emptiness check.

Extended reasoning...

The one reported finding is a doc-comment filename typo. The substantive risk in this diff was the control-flow reshaping at the end of process_http_callback; I verified it is behavior-preserving (same short-circuit order, same side-effect gating on the atomic exchange). The rest of the change is a mechanical &mut self → this: *mut Self conversion plus lock_guard() → lock()/unlock_raw(), whose soundness argument is well-documented in the PR and the type docs. Given this is unsafe cross-thread lifetime code reasoning about Tree Borrows protected tags, and is stacked on an unlanded PR, a human look is still appropriate — not approving.

[robobun]

^{Updated 2:05 AM PT - Aug 14th, 2026}

❌ @robobun, your commit 59bec16 has some failures in Build #95421 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38351

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

bun-38351 --bun

[robobun]

Thanks. The stale lint filename in the type docs is fixed in 59bec16 (it now names s3-download-task-raw-access.test.ts); that is the only change in the push, the code is unchanged. Agreed on the should_enqueue chain: body_into runs unconditionally first, and the exchange is only reached under the same two conditions that guarded it before, so the flag is still never set without a post.

[claude]

I reviewed this PR (including the 59bec16 doc-comment fix) and didn't find any bugs. Because it's a soundness change to cross-thread raw-pointer access and mutex-release ordering — and is stacked on #38330 which hasn't landed yet — a human look is still worthwhile.

What was reviewed:

• The process_http_callback &&-chain is behaviour-equivalent to the old early-return ladder: (is_done || !is_empty) is the negation of the old guard, and compare_exchange(false, ..).is_ok() is equivalent because the Err arm always carries true for an AtomicBool; short-circuiting still gates the exchange.
• The remaining &self calls (get_state, set_state, (*this).mutex.lock()) all return before unlock_raw, so no protected reference into the task outlives the release on the HTTP thread.
• release_portable was only called from Drop; folding it in removes the last &mut self in the inherent impl without changing drop order.
• The lint's regexes were checked against the allowed/banned snippet lists and the actual patterns in the fixed file.

Extended reasoning...

Overview

This PR converts three &mut self methods on S3HttpDownloadStreamingTask (report_progress, update_state, process_http_callback) to unsafe fn(this: *mut Self, ..) and replaces two lock_guard() critical sections with explicit lock() / Mutex::unlock_raw(&raw const ..) pairs. The rationale is that a &mut self receiver is a protected exclusive borrow of the whole task under Tree Borrows, and the other thread routinely writes the mutex word (every lock() attempt) and, on the final chunk, may free the task while the HTTP thread is still inside MutexGuard::drop -> Mutex::unlock(&self). It also widens Mutex::unlock_raw from pub(crate) to pub (the workspace denies unreachable_pub, so pub here just means "used from another crate"), folds release_portable into Drop, and adds a source-lint test that bans &mut self receivers and non-raw mutex spellings for this type.

Security risks

None. This is an internal aliasing-model soundness refactor with no user-facing surface, no parsing of untrusted input, and no auth/crypto path. Native behaviour is claimed unchanged ("natively benign today"); the diff bears that out — same lock, same state transitions, same body_into, same compare_exchange and posts.

Level of scrutiny

High. This is exactly the category the repo review guide flags hardest: cross-thread lifetime, raw pointers, and "the release is what lets the other thread free the owner" ordering. The reasoning is very thorough and I did not find a flaw, but the correctness argument depends on subtle Tree Borrows semantics (protected-tag lifetimes, interior-mutability carve-outs for & vs &mut, futex-wake-by-address) that a maintainer who owns bun_threading / the S3 client should sign off on. It is also stacked on #38330, so the merge order matters.

Other factors

• I traced the process_http_callback refactor line by line: the old code returned true iff (!wait_until_done || is_done) AND NOT (is_empty && !is_done) AND the exchange did not report Err(true). Since a failed compare_exchange(false, ..) on an AtomicBool can only report Err(true), .is_ok() is exactly the same decision, and the new && chain preserves the short-circuit that keeps the exchange from running (and setting the flag) when we would not post.
• The remaining reference-forming calls under the lock ((*this).get_state(), (*this).set_state(..), the autoref &Mutex in (*this).mutex.lock()) all complete and drop their protected references before unlock_raw, so nothing on the HTTP thread holds a reference into the task across the release. On the JS side, report_progress no longer holds a whole-task &mut while the HTTP thread hammers the mutex word or while the callback re-enters via on_stream_cancelled.
• The prior review's only finding (stale lint filename in the type doc) is fixed in 59bec16 and confirmed present in the current diff.
• The source-lint test guards against regression and self-checks its patterns against banned/allowed snippets plus non-vacuity checks (impl block present, mutex: Mutex field present, at least one .mutex.lock( present), which matches the repo's "prove the test can fail" convention.

Given the subject matter and the stacked dependency, I'm deferring rather than approving.

[robobun]

Noted, nothing further to change from this round. On the merge order: this PR needs Mutex::unlock_raw from #38330 (or the identical hunks in #38345), which is why it is based on that branch; once either of those is on main I will rebase this onto main so it can be reviewed and merged as a plain 3-file change. The maintainer-facing argument is in the description (Problem / Fix / Background plus the Miri table); the bun_threading part of this PR is only the visibility change on unlock_raw.