s3: stage simple-request results through the task pointer instead of a &mut self receiver - #38361
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it rewrites the aliasing contract on a cross-thread unsafe path (&mut self → *mut Self with per-field places) and is one of three coordinated sibling PRs (#38351, #38353) with a stated comment conflict, a maintainer sign-off on the reasoning and landing order would still be worthwhile.
What was reviewed:
- Verified the new body is statement-for-statement identical to the old one —
(*this).result = ...still runs drop glue on the old value likeself.result = ...did, and the metadata carry-over /ptr::writeoverwrite are unchanged. - Confirmed
stage_http_resulthas no other callers; each(*this).fieldborrow covers only that field, sosignal_storeis never inside a live reference. - Checked the source-lint regex against the actual declarations (doc comments and
S3HttpSimpleTask::release_at_shutdownat the callback registration site don't false-match; each of the four names appears exactly once).
Extended reasoning...
Overview
This PR converts S3HttpSimpleTask::stage_http_result from a &mut self method to an unsafe fn(this: *mut Self, ..) associated function, and updates its sole caller http_callback plus the SAFETY comments on both. It also adds a new source-lint test (test/internal/source-lints/s3-simple-task-raw-access.test.ts) that pins the four in-flight-task functions to raw-pointer receivers and self-tests its own regex against banned/allowed spellings.
Security risks
None user-facing. This is an aliasing-model contract fix in native code: the JS thread's stop_for_vm_teardown can atomically store into signal_store.aborted while the HTTP thread is inside stage_http_result, and a protected &mut self argument covering that byte is UB under Stacked/Tree Borrows and is passed to LLVM as noalias. The fix narrows every reference to a single non-signal_store field per statement. No new attack surface.
Level of scrutiny
High. This is exactly the review guide's most-blocked category: unsafe Rust, cross-thread lifetime, rewritten SAFETY comments. The change is mechanically small and I believe correct — (*this).result = X drops the old value just as self.result = X did; .take(), body_into(&mut ...), and as_mut_ptr() each form a reference to one field only; the ptr::write/ptr::read overwrite is byte-identical. But the correctness argument rests on Tree/Stacked Borrows protector semantics for interior-mutable bytes under a protected &mut, which a maintainer should confirm matches the tree's stated model (src/CLAUDE.md "Pointer provenance at FFI boundaries").
Other factors
- The PR is one of three coordinated siblings (#38351 for the streaming task, #38353 for
stop_for_vm_teardownreadinghttp), and the description flags a small comment merge conflict with #38353 on the lines above theptr::write. Landing order is a human call. - The new lint test references two sibling lints (
s3-download-task-raw-access.test.ts,s3-post-through-handle-field.test.ts) that don't exist on this branch — they belong to the sibling PRs. That's a comment-only forward reference, not a load-bearing dependency. - The lint's "every in-flight function is still declared" check uses
toEqualon sorted arrays, so it also catches a second declaration of the same name (would produce a length mismatch). The full-line-comment strip regex correctly handles///doc comments and preserves line numbers via[ \t]*(not\s*). - No runtime repro exists (the PR says so); the test is a source-shape lint following the existing
test/internal/source-lints/pattern (e.g.self-receiver-reclaim.test.ts), which is the tree's convention for this class of finding.
|
Two follow-up pushes since the review round, no change to the fix itself:
The CodeRabbit comments so far are rate-limit notices, not reviews. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches unsafe cross-thread code in the S3 request path and is one of three coordinated PRs (#38351, #38353) with an acknowledged merge conflict between them, a human look would still be worthwhile.
What was reviewed:
stage_http_resultbody is statement-for-statement identical to main; only the receiver shape changed (&mut self→*mut Selfwith(*this).fieldplaces), so the change strictly removes an aliasing claim without altering behaviour.http_callback's call site no longer forms&mut *this; theis_donehandoff block is unchanged.- Traced the new lint's regex against all four in-flight declarations in the file and against its own self-check fixtures — the parser reads each first param correctly, and the referenced sibling lint
self-receiver-reclaim.test.tsexists on this branch.
Extended reasoning...
Overview
The PR converts S3HttpSimpleTask::stage_http_result from a &mut self method to unsafe fn(this: *mut Self, ..) so the HTTP-thread callback never holds a protected exclusive borrow of the whole task while the JS thread's stop_for_vm_teardown may concurrently store into the task's signal_store.aborted atomic. The function body is otherwise identical (metadata carry-over, body_into, detach_lifetime, ptr::write — same statements, same order); http_callback's call site changes from (*this).stage_http_result(..) (which implicitly formed &mut *this) to Self::stage_http_result(this, ..). SAFETY comments on both functions are reworded to stop claiming exclusive ownership. A new source-lint test (test/internal/source-lints/s3-simple-task-raw-access.test.ts) pins the four in-flight functions to raw-pointer receivers, guards against renames, and self-checks its regex.
Security risks
None. This is an internal receiver-shape change on the S3 client's HTTP-thread callback; no user-facing surface, no input validation, no auth/crypto path is touched.
Level of scrutiny
High — this is unsafe Rust in a cross-thread path, which REVIEW.md flags as the most-blocked category. That said, the change is strictly a narrowing of what memory a live reference covers: the body semantics are byte-for-byte preserved, and the new shape matches the existing release_at_shutdown / stop_for_vm_teardown in the same file. It can only remove UB, not introduce it. I verified each (*this).field access maps 1:1 to the old self.field access, that the assignment to (*this).result still runs the old value's drop (as self.result = .. did), and that the ptr::write/ptr::read pair is unchanged.
Other factors
- The comment-cop bot flagged comment length on an earlier revision; the author shortened the contracts in a49340c and all six threads are resolved. What remains is the required
# Safetytext plus the pre-existingAsyncHTTPhand-back comment moved verbatim from main. - The lint test's self-check assertion order matches the
IN_FLIGHT_FNSiteration order overmatchAllsource order; I traced it against the fixture and it holds. The(?!\w)lookahead correctly excludesstage_http_result_for_tests, and full-line//stripping preserves line numbers via[ \t]*(not\s*). - This is one of three sibling PRs on the same hazard class; #38353 has a two-line comment conflict with this function. A maintainer coordinating the merge order is the main reason I'm deferring rather than approving.
|
Nothing further to change from the second review pass. State of a49340c: no open review threads; the GitHub Actions jobs (Format, cargo clippy, Source lints, which runs the new lint, Lint JavaScript, comment-cop) are green; Buildkite build 95453 is still running. |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit cc3aea6 has some failures in 🧪 To try this PR locally: bunx bun-pr 38361That installs a local version of the PR into your bun-38361 --bun |
|
380c8e3 is an empty retrigger. Build 95453 ran 177 jobs green; its only non-passing jobs were the two |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites concurrent unsafe access on the HTTP-thread side of S3HttpSimpleTask and is one of three coordinated aliasing-model fixes (#38351, #38353), a human look would still be worthwhile.
What was reviewed:
stage_http_resultbody is statement-for-statement identical to main (metadata carry-over,body_into,detach_lifetime,ptr::write) — only the receiver shape changed.- Per-field places touched (
result,response_buffer.list,http) are disjoint fromsignal_store, so the concurrent atomic store fromstop_for_vm_teardownno longer overlaps a protected borrow. - The source-lint's regex correctly reads all four in-flight declarations (including
release_at_shutdown(this: *mut ())) and its rename guard uses exact-array equality.
Extended reasoning...
Overview
The PR converts S3HttpSimpleTask::stage_http_result from a &mut self method to unsafe fn stage_http_result(this: *mut Self, ..), updates the one call site in http_callback to pass the raw pointer, and rewords the # Safety contracts on both functions to stop claiming exclusive ownership during the call. It adds a source-lint (test/internal/source-lints/s3-simple-task-raw-access.test.ts) that pins the four functions running on an in-flight task to raw-pointer receivers.
Security risks
None. No user-facing surface, parsing, or trust boundary is touched; the change is a receiver-shape adjustment on an internal HTTP-thread callback.
Level of scrutiny
High. This is native concurrent unsafe Rust in a hand-off path between the HTTP thread and the JS thread, squarely in REVIEW.md's most-blocked category (memory safety, thread affinity). The transformation itself is mechanical and the body is provably the same statements in the same order, but the justification — that a protected &mut self argument covers interior-mutable bytes under Tree Borrows and therefore races with stop_for_vm_teardown's atomic store — is a subtle aliasing-model claim. It is well-argued (Miri reduction, both models) and the fixed shape matches the file's existing release_at_shutdown/stop_for_vm_teardown pattern, but this is the kind of reasoning a maintainer who knows the S3 task lifecycle should confirm rather than take on automated say-so.
Other factors
- The PR is one of three coordinated siblings (#38351 for the streaming task, #38353 for the
httpread instop_for_vm_teardown). A human reviewing any one of them will likely want to see the set together, and #38353 has a known comment conflict with this change. - All comment-cop threads on the safety-doc length are resolved; the remaining comments are the required
# Safetysections plus the pre-existingAsyncHTTPhand-back block moved inside the newunsafe {}. - The lint's self-test covers both the before/after spellings and negative cases (name-prefix, comments), and the rename guard (
toEqualon sorted names) prevents a silent drop-out. I checked the[^,()]*(?:\(\))?capture againstrelease_at_shutdown(this: *mut ())— it readsthis: *mut ()correctly. - No observable runtime behaviour changes, so the risk of regression is low; the value of a human look is confirming the concurrency invariant (only
stop_for_vm_teardowntouches the task concurrently, and onlysignal_store) is complete.
|
On the point the review leaves for a human (is |
S3HttpSimpleTask::stage_http_result ran on the HTTP thread as a &mut self method while the JS thread's stop_for_vm_teardown could store into the same task's signal_store. Take the task as *mut Self and borrow one field per statement; the s3 task lint now also checks that the functions which run on an in-flight simple task take it as a raw pointer. signals::Store::to and to_with_backpressure only take the flags' addresses, so they take &self: the pointers the HTTP thread keeps must not descend from an exclusive borrow of a store the owner goes on storing into.
380c8e3 to
cc3aea6
Compare
|
Restructured (head cc3aea6), following a review of the standalone shape:
The description describes the current shape; the earlier one is summarised in a details block at the end. The comment-length threads that just appeared were raised against a diff that includes #38353's commit; the ones on |
|
Housekeeping: after the base change GitHub kept serving this PR's diff against main for about twenty minutes (six files, including #38353's), which is what the last review round and the comment-length bot were looking at. Re-setting the base fixed it; the PR now shows its own three files ( |
Stacked on #38353: the base of this PR is that PR's branch, so the diff here is only this PR's own change (3 files). It lands after #38353, at which point it is rebased onto main.
Problem
S3HttpSimpleTask::stage_http_result(src/runtime/webcore/s3/simple_request.rs) is a&mut selfmethod that the HTTP thread runs fromhttp_callbackfor every result callback of a request that is still out.S3HttpSimpleTask::stop_for_vm_teardownon the same task (dispatched fromstop_active_handles, src/runtime/jsc_hooks.rs:1819, for every request still registered: at workerterminate(), atprocess.exit(), and at the sweepbun testruns between files). With s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353 underneath, that function stores intosignal_store.abortedand readsasync_http_id, both fields of the task allocation.&mut selfargument is a protected exclusive borrow of the whole task for the duration of the call, interior-mutable bytes included (Background), so that store during the call is undefined behaviour under both of Rust's aliasing models, and rustc passes the same claim to LLVM asnoaliason the argument. Miri rejects a reduction of the shape at the teardown store: Tree Borrows,this foreign write access would cause the protected tag ... to become Disabled; protected tags must never be Disabled, pointing at&mut self; Stacked Borrows,would remove [Unique ...] which is strongly protected(output below).signal_storehas the same problem one level down:signals::Store::toandto_with_backpressure(src/http/Signals.rs) took&mut selfto hand out the flags' addresses, so every pointer the HTTP client reads the abort flag through (Signals) descended from an exclusive borrow of a store the owner goes on storing into. Under Stacked Borrows the owner's later store invalidates those pointers; under Tree Borrows an unprotected&mutover interior-mutable bytes happens to tolerate it. This applies to all three owners of aStore(both S3 tasks andFetchTasklet).stage_http_resultnever re-reads the flag and theSignalspointers are only used for atomic loads, so nothing observable goes wrong. There is no runtime repro; these are contract fixes found by analysis. The sibling findings on these task types are s3: reach the streaming download task through its raw pointer on both threads #38351 (the streaming task's&mut selfmethods) and s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353 (stop_for_vm_teardownreadinghttp); this PR is the remaining piece, on top of s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353.Fix
stage_http_resultbecomesunsafe fn stage_http_result(this: *mut Self, ..)and reaches the task through(*this).field, so each reference it forms covers one ofresult,response_buffer,httpfor one statement and the two fields teardown touches are never inside one.http_callbackcallsSelf::stage_http_result(this, ..). The body is otherwise the same statements in the same order ((*this).result = ..still drops the old value asself.result = ..did). This is the shape the file's other in-flight functions already have (release_at_shutdown,stop_for_vm_teardown, andschedulefrom s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353), and the tree's convention for objects another thread can touch during the call (src/CLAUDE.md, "Pointer provenance at FFI boundaries").# Safetytext of both functions is written once, against the post-s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353 state: the concurrent toucher isstop_for_vm_teardown, and what it touches issignal_storeandasync_http_id. That set is checkable by listing who holds a pointer to a live task: the HTTP callback context (HTTP thread), the active-handle registry (read only bystop_active_handles), and the task queue, which receives the task only from the final callback, after the HTTP thread's last access (on_responseruns from there, src/runtime/dispatch.rs:365, or its teardown release at :1235);grep -rn S3HttpSimpleTask srcshows no other holder.Store::toandto_with_backpressuretake&self. They only take the fields' addresses, so nothing needed the exclusive borrow; the four callers compile unchanged. This is the same class as the receiver (a reference oversignal_storethat the owner's later stores conflict with), so it rides in this PR rather than a fourth one; it has no test of its own because it changes no behaviour, and the lint below is what this PR's gate rests on.Storeits own allocation so that a&mutover a task never covers the flags. It would not make an in-flight&mut selfmethod sound: teardown would still have to read the store's pointer out of the task, and a foreign read of bytes under a protected&mut, followed by the method's own writes, is undefined behaviour under the same rules as the store into them is.http_callbackwould also still have to be pointer-shaped, since its post lets the JS thread free the task before it returns. So the receiver rule is needed either way, and the extra allocation per request would buy nothing.test/internal/source-lints/s3-task-http-field.test.ts, which already scans the two task files and attributes code to functions) gains the second rule of the same window: the functions that run on an in-flightS3HttpSimpleTask(http_callback,stage_http_result,release_at_shutdown,stop_for_vm_teardown) must take the task as a raw pointer. It checks that the listed functions are still declared (a rename cannot drop one out silently) and its parsing against banned and allowed spellings. The simple task's post-hand-back helpers (error_with_body,fail_if_contains_error,release_portable,Drop) keep their receivers, which is why this is a list; the streaming task has no post-hand-back phase, so its rule is every method, which is s3: reach the streaming download task through its raw pointer on both threads #38351's lint. Against the base (s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353's source) the new rule reports exactlysimple_request.rs:416: fn stage_http_result(&mut self, ..)and s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353's own checks still pass; with this change everything passes (verified both ways withgit stash push -- src/).test/internal/source-lints/directory (105 tests); the credential-free S3 files (s3-connection-close, s3-storage-class, s3-requester-pays, s3-insecure, s3-list-checksum-algorithm, s3-list-encode-overflow, s3-stream-cancel-leak, s3-stream-error-gc, s3-write-to-file-sync-close, s3-argument-validation: 56 tests); the five "VM teardown ordering" tests in worker_threads.test.ts, including s3: abort in-flight tasks at VM teardown by request id instead of reading task.http #38353's two, which terminate a worker with requests in flight; fetch's abort tests (fetch-abort-queued, fetch-abort-socket-close-race, fetch-abort-stream-body, abort-signal-leak, the last needing a longer timeout than 5 s on debug as on the base), since fetch is aStore::to_with_backpressurecaller;cargo clippy --no-depsonbun_httpandbun_runtimeand rustfmt clean. The Miri reduction below passes in the fixed shape under both models; it is not committed because it would exercise a copy of the shape, which the lint pins in the real file.FetchTasklet's HTTP-thread callback holds a function-long&mutover the tasklet (from_raw_mut, FetchTasklet.rs:2417) whileabort_taskruns on the JS thread; that type's cross-thread ownership is being reworked in fetch: encode FetchTasklet's cross-thread ownership in the type system #31745, with fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703 converting its release path, so it is left to those.Background
bun run rust:miriuses), a reference passed as a function argument is protected until the function returns. For&mut Tthat means no other access at all to the bytes it covers; Tree Borrows gives a protected&mutthat strength even over interior-mutable bytes such as atomics (only an unprotected one relaxes them), and only a shared&Tleaves interior-mutable bytes open to other threads. A raw pointer asserts nothing, and a reference to one field covers only that field, which is why the fixed function takes*mut Selfand borrows per field.&mut Storeis, to the aliasing models, a child of that exclusive borrow, and an access through the original owner that conflicts with the&mutcan invalidate the children with it (Stacked Borrows does; Tree Borrows lets an unprotected&mutover interior-mutable bytes survive it). Taking the addresses through&Storeremoves the exclusive link.S3HttpSimpleTask: one heap object per non-streaming S3 operation (stat, get, put, delete, list, multipart commit/part). The JS thread builds it andschedules it; the HTTP thread stages each result into it and, on the final one, posts the task itself back to the JS thread, which runson_response, delivers the result and frees it. While it is out it is registered as an active handle so the VM's stop phase can abort it.signal_store/Signals: the task-owned block of atomic flags (abortedamong them) and the struct of pointers to them that the HTTP client carries; settingabortedis how the owner asks for the request to fail. The stop phase sets it on the JS thread and asks the HTTP thread to close the socket; the task comes back through the normal final callback.Miri reduction of the receiver shape (struct with an atomic field; one thread inside the method, the other storing into the atomic during the call)
The reduction's method does the same three kinds of access as the real one (a
Vecappend into one field, a plain field assignment, aptr::writeinto aMaybeUninitfield) and parks inside the call on a static flag, kept outside the struct, until the other thread has stored into the atomic.Earlier shape of this PR
This PR first went up against main as the receiver change plus its own lint file (
s3-simple-task-raw-access.test.ts), without theSignals.rschange. Review of that shape found that it conflicted with #38353 inside this very function (both rewrite its comments), that its# Safetytext could only be complete once #38353'shttpread was gone, that a per-type lint file duplicated the scanning #38353's lint already does, and that theStore::to(&mut self)chain left the HTTP thread's hold onsignal_storein the same state the PR was fixing one level up. The current shape is the result: stacked on #38353, lint folded into its file,Signals.rsincluded.