Conversation
…ansform output, readdir and mkdir results WriteFile carried the Box holding Bun.write's promise as a raw ctx pointer in its off-thread half, freed only by the completion. A job released instead of completed (the VM tore down first, or the pool's post was refused) leaked the box and kept the promise rooted. The promise is now the job's Js half (WriteFilePromise), released by the VM with its other live jobs; on Windows WriteFileWindows holds it as a field until it settles it. TransformTask.output_code, StringOrUndefined (ret::Mkdir) and ret::Readdir held bare bun_core::Strings / Dirents that only the to_js path released, so the same release paths leaked them too. output_code and the mkdir path are OwnedStrings now and ret::Readdir releases whatever to_js did not hand over in Drop, which also covers a conversion that fails part-way. The FromAny impl for Box<[bun_core::String]> had no other user and is gone. worker-refused-completion.test.ts gets rows for the four producers, starts each row's work from a microtask so leaksan.supp's module-evaluation entry does not hide what it allocates, and turns LSan on itself on ASAN builds.
|
Warning Review limit reached
Next review available in: 37 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 (6)
Comment |
|
Status: ready for review (head ac8b966). Reproduced on the unfixed build with The readdir / mkdir / transform rows exercise the new release paths under ASAN but cannot show their leaks by themselves (WTF string memory is not tracked by LSan); the Since the first push: the blob read / write rows are skipped on Windows, where those operations are libuv requests rather than pool jobs (15271bf); the flagged doc comments are trimmed (27a670a); the one comment still naming |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks ownership across several GC/job-lifecycle boundaries (WriteFile's Js half, a new Drop for ret::Readdir, OwnedString in TransformTask/StringOrUndefined) and includes a #[cfg(windows)] path that only Windows CI exercises, a human look is still worthwhile.
Checked: Bun__Dirent__toJS transfers via transferToWTFString() so the extra Dirent::deref in the new Drop is a no-op on already-delivered entries; BunString__createArray refs (does not consume), matching the new Files arm; reject_with_async_stack preserves the async-stack behavior the old to_error_instance_with_async_stack provided; the sync readdirSync path (fs_to_js → &mut self to_js) still balances via the new Drop; the removed FromAny for Box<[bun_core::String]> had no other callers.
Extended reasoning...
Overview
Six files: write_file.rs moves WriteFilePromise from an erased *mut c_void box in the off-thread half to a JsAffine Js half of the WriteFile job (and a plain field on WriteFileWindows), removing create_with_ctx/the callback pointer/the heap box; Blob.rs updates the two call sites; JSTranspiler.rs swaps output_code to OwnedString; node_fs.rs swaps StringOrUndefined::String to OwnedString, changes ret::Readdir::to_js to &mut self, and adds impl Drop for Readdir; JSValue.rs deletes the now-unused FromAny for Box<[bun_core::String]>; the test file adds five rows and turns on LSan for the ASAN lane locally.
Security risks
None. Pure resource-lifetime work on internal job/result types; no parsing of untrusted input, no auth/crypto, no user-facing API surface change.
Level of scrutiny
High — this is native memory-safety code (the most-blocked category per REVIEW.md): GC roots (JSPromiseStrong), WTF refcounts released off the JS thread, a new Drop impl on a return type that flows through both sync and async fs paths, and a cfg-gated Windows rewrite of run_from_js_thread/create. I traced the double-release hazards (Dirent::to_js_newly_created already derefs after transferToWTFString() leaves the strings Dead, so the added Drop deref is a no-op; to_js_array → BunString__createArray → Bun::toJS refs rather than transfers, so Drop releasing the caller's refs is the same balance as the removed FromAny loop) and the async-stack behavior (JSPromiseStrong::reject_with_async_stack replaces to_error_instance_with_async_stack + reject). All looked correct.
Other factors
The PR description is thorough and lists cross-platform cargo check and the relevant test suites. Verifier agents already ruled out the Windows-Bun.write()-row concern (WriteFileWindows is not a pool job, so no refusal path — but that means the row is POSIX-only fail-before proof, which the description acknowledges). Still, the Windows run_from_js_thread rewrite (dereferencing (*(*this).event_loop).global_ref() before deinit) and the new Drop for Readdir interacting with ResultListEntryValue::deinit on the recursive-readdir error path are the kind of ownership reshaping a maintainer should sign off on.
…buv requests rather than pool jobs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
On the points in the review above:
27a670a trims the doc comments the comment lint flagged to one line each (or drops them where the |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/blob/write_file.rs:616-618— The SAFETY comment inWriteFileWindows::on_open'sdebug_assert(line 819) still says "data was set tothisin create_with_ctx/open", but this PR renamedcreate_with_ctx→createand deleted the wrapper. One-word fix:create_with_ctx→create(the analogous comment indeinitwas already updated).Extended reasoning...
What the bug is
This PR renames
WriteFileWindows::create_with_ctxtocreateand deletes the old thincreatewrapper that used to call it. It updated one comment that referenced the old name —deinitnow says "paired with the RAII note increate" — but missed a second one. The SAFETY comment insideon_open'sdebug_assertat write_file.rs:819 still reads:debug_assert!(core::ptr::eq( this, // SAFETY: req == &(*this).io_request; data was set to `this` in create_with_ctx/open. unsafe { (*req).data }.cast::<WriteFileWindows>() ));
create_with_ctxno longer exists onWriteFileWindowsafter this PR.The specific code path
io_request.datais now assigned in the renamedcreate((*write_file).io_request.data = write_file.cast::<c_void>();) and again inopen. So the substance of the SAFETY justification is still correct — only the function name it points at is stale.Why existing code doesn't prevent it
This is a comment inside
#[cfg(windows)]-gated code. Renames don't touch comments, andcargo checkon non-Windows targets never even parses this module's body semantically, so nothing mechanical catches it. REVIEW.md calls this out explicitly: "Signature changes and renames → grep the whole repo including cfg-gated code and generated-binding inputs; stale call sites compile fine", and "SAFETY comments are required above use ofunsafeand must be accurate."Impact
None at runtime — it's a debug-only assertion whose SAFETY comment names a function that was renamed in the same PR. The next reader grepping for
create_with_ctxto understand wheredatais set will find nothing.Step-by-step proof
- Before this PR,
WriteFileWindowshad two constructors:create_with_ctx(did the work, setio_request.data = write_file) andcreate(a.cast()shim over it). - This PR deletes the shim and renames
create_with_ctx→create; the diff shows thedeinitcomment updated from "paired with the RAII note increate_with_ctx" to "paired with the RAII note increate". grep create_with_ctx src/runtime/webcore/blob/write_file.rson the PR head yields exactly one hit — line 819 inon_open— with no corresponding function definition anywhere in the file.- The assignment the comment justifies now lives in
create(andopen), so the correct text is "data was set tothisin create/open".
How to fix
- // SAFETY: req == &(*this).io_request; data was set to `this` in create_with_ctx/open. + // SAFETY: req == &(*this).io_request; data was set to `this` in create/open.
- Before this PR,
…ts io_request.data
|
Good catch on the |
|
The line comment above is the same |
|
I had the Differences, all cosmetic on the fix itself: it makes Two pieces of evidence from that branch that this PR does not have yet, if you want them:
|
There was a problem hiding this comment.
I reviewed this PR again at ac8b966 and didn't find any bugs; the earlier SAFETY-comment nit is fixed. Because it reworks cross-thread ownership of the WriteFile promise, adds a Drop to ret::Readdir, and touches the Windows libuv path, a human look is still worthwhile.
What was reviewed:
- Refcount balance of the new
Drop for ret::Readdir:BunString__createArrayrefs each element (soFilesderef-on-drop is the caller's +1), andBun__Dirent__toJStransferToWTFStrings name/path, soWithFileTypesderef-on-drop is a no-op on transferred entries and releases the rest on a mid-loop failure. - Sync readdir path (
run_sync→fs_to_js(&mut self)) still dropsresultafter conversion, so no double-free and no leak with the new signature. reject_with_async_stackonJSPromiseStrongattaches the same async frames the oldto_error_instance_with_async_stack+rejectpair did.- The removed
FromAny for Box<[bun_core::String]>has no remaining callers.
Extended reasoning...
Overview
The PR fixes leaks on the refused/unrun-job release path for four producers: Bun.write()'s promise box, TransformTask.output_code, StringOrUndefined (mkdir), and ret::Readdir entries. It restructures WriteFile's completion from an erased fn(*mut c_void, ...) callback + heap-boxed WriteFilePromise (freed only in then) into a typed Js = WriteFilePromise half that the job machinery releases at VM teardown. WriteFileWindows holds the promise as an Option field. The three string-producing results move from BunString (Copy, no Drop) to OwnedString, and ret::Readdir gains a Drop that derefs whatever to_js didn't hand off. The FromAny for Box<[bun_core::String]> impl is removed as dead. Tests add rows to worker-refused-completion.test.ts and turn on LSan for the ASAN build.
Security risks
None identified. This is memory-lifecycle / leak-fix work; no untrusted-input parsing, auth, crypto, or permissions logic is touched.
Level of scrutiny
High. Per the repo's review guidance, "Native code: memory safety" is the most-blocked category, and this PR sits squarely in it: cross-thread ownership transfer, JSC GC rooting (JSPromiseStrong inside a JsAffine-derived struct), a new Drop on a type whose contents may already have been transferred to C++, and a Windows-only libuv path that reads global_ref() before freeing the request box. The correctness depends on FFI-side semantics (Bun::toJS refs vs. transferToWTFString transfers; String::deref() being a no-op on non-WTF tags) that aren't visible in the diff alone.
Other factors
I traced the specific ref-count questions and found them correct:
BunString__createArraycallsBun::toJSper element, which forWTFStringImplconstructsWTF::String(impl)→ +1 ref, soDrop's per-elementderefonFilesreleases exactly the caller's ref.Bun__Dirent__toJStransferToWTFStrings bothnameandpathunconditionally, so afterto_js_newly_createdeach Dirent's fields are Dead and both the existingself.deref()there and the newDropderef are no-ops on delivered entries. On a mid-loopput_indexfailure, undelivered Dirents keep their refs and are released byDrop— the exact partial-failure leak the PR describes.run_syncbinds the result aslet mut resultand callsfs_to_js(&mut self), then drops it — matching the asyncFsReturnpath — so theto_js(self) → to_js(&mut self)signature change plusDropdoesn't double-free the sync path.- The
WriteFilePromise::settleerror path usesto_error_instance+JSPromiseStrong::reject_with_async_stack; the latter (inJSPromise.rs) attaches async frames from the promise before rejecting, equivalent to the oldto_error_instance_with_async_stack+rejectsequence, and the PR verified stack traces match. transfer_to_json theOwnedString's inner leaves it Dead, so the subsequentOwnedString::dropderef is a no-op.
The comment-cop lint findings and my earlier stale-comment nit are all resolved. The change is well-tested (fail-before demonstrated for the LSan-visible Bun.write leak; ASAN exercises the string paths) and cross-compiled for Windows/macOS. Still, this is exactly the class of change where a maintainer's eye on the ownership model — particularly the WriteFileWindows field promise + deinit ordering and the recursive-readdir result_list vs ret::Readdir single-owner argument — is worth having before merge.
|
Thanks for the second pass. Nothing further to change from it; all review threads are resolved and the branch is at ac8b966, waiting on CI. |
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
Understood. For anyone picking this up later: the branch stays at ac8b966; the LSan fail-before output for the |
3 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.