napi: drive napi_async_work through the addon's pointer instead of &mut self receivers - #37750
napi: drive napi_async_work through the addon's pointer instead of &mut self receivers#37750robobun wants to merge 4 commits into
Conversation
…ut self receivers napi_async_work::run(&mut self) posted the work to the JS thread through post_to_js_thread(&mut self, self_ptr), and the JS thread runs the addon's complete callback (which normally calls napi_delete_async_work) as soon as the post lands, so the allocation could be freed while both &mut self receivers, and the &self of the loop_handle field the post went through, were still protected arguments. run_from_js(&mut self) had the same shape with complete itself doing the free, and schedule(&mut self) handed the pool a task pointer projected from the reference. schedule, run, cancel and run_from_js now take the raw work pointer, read the fields they need through statement-scoped accesses, post through a cloned LoopHandle, and touch nothing after the hand-over; the extern "C" entry points and the dispatch arm pass the pointer straight through. Adds a source lint for posting a receiver's address through an embedded task (.from(..) with self's address, a local bound to it, or a *mut Self parameter next to a reference receiver) and an addon test that cancels a work while its execute callback is running.
WalkthroughThe async work lifecycle now uses explicit unsafe raw-pointer functions. A repository lint detects intrusive mutable receiver posts. New N-API tests cover cancellation while ChangesN-API async work safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit c321da5 has some failures in 🧪 To try this PR locally: bunx bun-pr 37750That installs a local version of the PR into your bun-37750 --bun |
|
Status: ready for review (head c321da5). How the shape was confirmed: the new lint in Revisions since opening: the new addon test compares its output line by line (the first revision failed on the Windows lanes of build 92850 because the addon's Behavioural coverage run on the debug (ASAN) build: CI: build 93247 (head c321da5) and build 92883 (ad26827) both ended with every job that ran green (192 of 194, napi lanes included; one unrelated flaky test per build passed on retry). The two remaining jobs in each were the Related but separate: #37741 converts the threadsafe-function entry points in the same file; the two touch different functions. |
printf() through the Windows CRT writes \r\n, so the exact-string comparison failed on the Windows lanes; compare line by line like the file's other printf-backed assertions.
…m comments The shared-ownership rule lives on the struct doc once; the per-function docs keep only the thread and the one fact each caller needs.
|
Pushed a82cd9e (line-by-line comparison in the new addon test; build 92850 failed on the Windows lanes because the addon's printf emits CRLF there) and ad26827 (comment trim: the ownership rule is stated once on the struct, |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-app/async_tests.cpp`:
- Around line 371-376: Update the async entry point in async_tests.cpp to
validate that info[0] exists and is callable before allocating RunningCancelData
or queuing async work; on invalid input, propagate the established N-API error
instead of returning undefined or proceeding with side effects.
- Around line 374-383: Update the async setup flow surrounding NODE_API_CALL and
complete_for_running_cancel to release RunningCancelData, the callback
reference, and the async-work handle on every setup failure. Keep ownership with
the setup code until napi_queue_async_work succeeds, then transfer it to
complete_for_running_cancel so the completion path performs cleanup exactly
once.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 063547b0-d6f3-48bf-86f0-f102f60f7fec
📒 Files selected for processing (6)
src/runtime/dispatch.rssrc/runtime/napi/napi_body.rstest/internal/source-lints/self-receiver-intrusive-post.test.tstest/napi/napi-app/async_tests.cpptest/napi/napi-app/module.jstest/napi/napi.test.ts
|
Answered the two review threads on the new addon fixture (setup-failure cleanup and argument validation): both paths are unreachable from the fixture's only caller and the fixture follows the shape of |
There was a problem hiding this comment.
Thanks for the CRLF fix in a82cd9e — the split-on-either-ending assertion now matches the file's other printf-backed cases. This run found no further issues, but given it rewrites the receiver/lifetime contract for napi_async_work across the pool↔JS boundary in unsafe Rust, a human pass is still worthwhile.
What was reviewed:
run/run_from_js: confirmed no reference into*thissurvives past.from(this, ..)/complete; theNonNull::fromtemporary and the&mut concurrent_taskauto-ref both die beforehandle.post_task.env_refheld acrosscomplete: same shape the old_hsscope guard already relied on; env is kept alive byGlobalObject::m_napiEnvs, not the work's own ref.napi_cancel_async_workwhilerunis on the pool thread: the debug-assert reads only(*work_).global, whichrunnever writes;canceltouches only the atomic.- New
test_cancel_running_async_work: bounded busy-wait is in the spawned subprocess (not the test runner), pool thread is independent of the blocked JS thread, and the fallback branch fails loudly.
Extended reasoning...
Overview
Converts napi_async_work's entry points (schedule, run, cancel, run_from_js) from &mut self receivers to unsafe fn(this: *mut Self), and updates the three extern "C" napi functions plus the dispatch arm and Taskable::release_unrun to pass the addon's raw pointer through instead of materializing &mut. post_to_js_thread is folded into run. Adds a source lint (self-receiver-intrusive-post.test.ts) banning .from(self-as-ptr, ..) inside a reference-receiver method, and a napi test that cancels work while execute is running on the pool thread and deletes the work in complete.
Security risks
None. No user-facing input handling, no auth/crypto/permissions. The change tightens an aliasing contract; it does not add new capability.
Level of scrutiny
High. This is memory safety in native code — REVIEW.md's most-blocked category — with cross-thread lifetime (pool thread hands the work to the JS thread, whose complete callback frees it, potentially before the posting call returns). The transformation is mechanical and follows the established pattern from #37681/#37703/#37723/#37741 in the same series, and the PR includes a Miri reduction proving both the defect and the fix under Tree Borrows and Stacked Borrows. But the code path is exercised by every addon that uses napi_async_work (node-addon-api's AsyncWorker), so a maintainer should confirm the field-access ordering in run (read loop_handle/execute/env/data up front, CAS status, run execute, store status + fill task, post through the cloned handle) matches their model of the protocol.
Other factors
My prior finding (Windows CRLF in the new test's .toBe(...join("\n"))) was fixed in a82cd9e, and the comment-cop flags on the per-function doc comments were addressed in ad26827 by consolidating the ownership rule onto the struct doc. All inline threads on the PR are resolved. The source lint has positive/negative fixtures including the exact two-function shape from main and asserts scanned > 0 to guard against vacuous passes. The behavioral test runs under checkSameOutput (so Node parity is asserted) and its complete deletes the work under the ASAN build, covering the run_from_js receiver that the lint cannot express syntactically. No outstanding reviewer comments.
…ption The embedded-task .from() spelling is covered by the sibling lint in #37750, so this one goes back to the heap-task constructors only.
…per-thread fields The lint now recognises the receiver's address taken through a reborrow (from_mut(&mut *self), NonNull::from(&mut *self), a bare &mut *self) and a pointer parameter typed by the enclosing impl's name as well as Self, with self-test cases for each; the tree stays at zero hits and main's site is still the one report. napi_async_work's field docs now say which thread writes each field, the SAFETY comments in schedule and run refer to that instead of overstating what the JS thread touches, and the BackRef note left over from the LoopHandle conversion is gone.
|
Pushed c321da5 after another review pass over the diff: the lint now also catches the receiver's address taken through a reborrow ( |
There was a problem hiding this comment.
I re-reviewed after the CRLF fix and comment trim; no issues found this pass. Given the change rewrites cross-thread unsafe pointer handling for napi_async_work (aliasing-model correctness across pool/JS threads), a human look is still worthwhile.
What was reviewed:
run/run_from_js/schedule/cancelfield-access ordering: last touch of*thisprecedes the post;envliveness aftercompletefrees the work is viaGlobalObject::m_napiEnvs, not the work's own ref.- The new running-cancel fixture: bounded busy-wait on the JS thread while the pool starts
execute;release_executeis set on both branches so the pool thread cannot spin forever. - The source lint's
restOfFunctionscoping and impl-type resolution against its positive/negative examples.
Extended reasoning...
Overview
This PR converts napi_async_work's entry points (schedule, run, cancel, run_from_js) from &mut self methods to unsafe fn(this: *mut Self) associated functions, so no Rust reference to the whole work is live while another thread (or the addon's complete callback) may free or write to it. The three FFI entry points (napi_queue_async_work, napi_cancel_async_work, napi_delete_async_work) and the dispatch arm now pass the raw pointer through instead of .as_mut(). It adds a source lint (self-receiver-intrusive-post.test.ts) banning the .from(self-address, ..) shape, and a new addon test that cancels a work while execute is running on the pool thread.
Security risks
None. This is internal memory-model correctness; no user-facing API surface, parsing, or trust boundary changes.
Level of scrutiny
High. This is squarely in REVIEW.md's most-blocked category (native memory safety, cross-thread ownership, unsafe Rust). The change is well-motivated (Miri Tree Borrows / Stacked Borrows reproduction in the PR description) and follows the same shape as recent conversions (#37681, #37703, #37723, #37741), but the field-by-field access ordering in run — which fields are read before vs. after the post, and that the cloned LoopHandle (not (*this).loop_handle) is what post_task and embedded_work_finished go through — is exactly the kind of thing a human reviewer should sanity-check. The env pointer's liveness after complete frees the work relies on GlobalObject::m_napiEnvs holding an independent ref, which the code already relied on but is now stated in the comment.
Other factors
- My earlier finding (Windows CRLF in the new test assertion) was fixed in a82cd9e.
- The comment-cop bot has one unresolved ping on the struct doc at line 1763; the author already explained in resolved threads that this is the one place the shared-ownership protocol is documented (not a workaround justification), and the per-function comments were trimmed in ad26827 to rely on it. I don't consider it blocking.
- CI build 92883 was green on all lanes except darwin 26 aarch64, whose jobs never picked up an agent (infrastructure, not the diff).
- The new source lint has a self-test with positive/negative examples pinning each spelling; I checked that
restOfFunctioncorrectly bounds each search to the enclosing function so a binding in one method and a post of the same name in the next don't cross-match. - The C++ fixture's bounded busy-wait (10s) on the JS thread is safe: Bun's
WorkPooland Node's libuv threadpool dispatch independently of the JS event loop, andrelease_execute = trueis set on both the started and did-not-start branches so the pool thread cannot spin forever.
Problem
napi_async_workis freed by the addon, usually from inside its owncompletecallback. On every ordinary completion that free happened while a&mut selfto the work was still a live argument: the pool thread posted the work from inside two&mut selfframes, and the JS thread calledcompletefrom a&mut selfmethod on the work itself.Undefined Behavior: reborrow through <tag> ... is forbiddenon the pool side,deallocation through <tag> ... is forbiddenon the JS side) and passes in the fixed shape.bun_runtimeitself cannot run under Miri.schedulegave the pool a task pointer derived from the reference instead of from the whole allocation, andcancel(JS thread) andrun(pool thread) each claimed the whole struct exclusively while the other could legitimately be running.Fix
napi_*_async_workC functions, the dispatch arm, the teardown release path) pass the pointer through instead of turning it into&mut.completeis copied out before the call and the pointer is not touched after it.statusis written by both threads, which is what letscancelandrunoverlap and what the per-field accesses rest on. No behaviour change is intended: the same fields are read and written and the post goes to the same loop.executeis running and hascompletedelete it; the napi and node-api async suites were run on the ASAN build. The Miri result is from the reduction, not from Bun.run_from_jshas no lint and relies on the addon tests.Background
napi_async_workis the Node-API object an addon gets fromnapi_create_async_work. Bun heap-allocates it, but the addon owns it and frees it withnapi_delete_async_work, conventionally from insidecomplete(node-addon-api'sAsyncWorkerdoes this).napi_queue_async_workhands the work to Bun's work pool; a pool thread runs the addon'sexecute, then posts the work back to the JS thread, which runscomplete.napi_cancel_async_workmay be called from JS at any time and only succeeds ifexecutehas not started.concurrent_task), so posting it means filling in a field of the work and linking that field into the JS thread's queue. From the moment the post lands the JS thread owns the allocation, even though the posting function has not returned yet.&mut selfincluded) as valid and exclusive for the whole call (noaliasanddereferenceablein codegen), so another thread writing or freeing that memory during the call is UB even if the callee never touches the reference again. A raw pointer makes no such promise, and a(*this).fieldaccess only claims that field for that statement.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts
Original description
Problem
napi_async_work(src/runtime/napi/napi_body.rs) is an allocation the addon owns. Its pool-thread side posted it back to the JS thread like this:As soon as the post lands, the JS thread drains the embedded task and runs
run_from_js, whosecompletecallback belongs to the addon and normally callsnapi_delete_async_workon this very work (node-addon-api'sAsyncWorkerdoes; so do the fixtures in test/napi/napi-app/async_tests.cpp). That can happen beforepost_taskhas returned, i.e. while three references into the allocation are still live function arguments:run's&mut self,post_to_js_thread's&mut self(which has just writtenconcurrent_taskthrough itself), and the&self.loop_handlethe post went through. On the JS thread,run_from_js(&mut self)calledcompletedirectly, so on every ordinary completion the work was freed while that receiver was a live argument too. Its own comment said so ("the 'this' value here may already be freed by the user incomplete") and movedpoll_refout beforehand, which handles the later reads but not the receiver itself.A reference argument is protected for the whole call, and another party reading, writing or freeing memory it covers is UB under both aliasing models whether or not the function touches the reference again (rustc's codegen relies on the same thing: the argument is
noaliasanddereferenceablefor the whole call).bun_runtimecannot run under Miri, so the reduction below has exactly these shapes: the embedded task filled in and posted from two&mut selfframes, a JS thread that frees the work from the completion callback, and the two receivers the fix replaces. Under Tree Borrows (whatbun run rust:miriuses) the JS thread's first access to the work is rejected againstpost_to_js_thread's receiver, and the free is rejected againstrun_from_js's:Stacked Borrows reports the same two sites (
not granting access to tag ... because that would remove [Unique for <..>] which is strongly protected) and also passes the fixed shape. No crash is known from this; nothing in Bun reads through the references after the hand-over today. It is the contract that is wrong, the same class as #37681, #37703, #37723 and the threadsafe-function conversion in #37741 (a different set of functions in the same file; this PR does not touch them).Two smaller things of the same kind in the same functions:
schedule(&mut self)handed the pool&raw mut self.task, a pointer projected from the reference, whileIntrusiveWorkTask::from_task_ptrdocuments that the pointer it gets back must carry provenance for the whole allocation (WorkPool::schedule_ownedprojects from the raw pointer for exactly this reason); andcancel(&mut self)on the JS thread andrun(&mut self)on the pool thread each claimed the whole struct exclusively while the other was legitimately running (napi_cancel_async_workduringexecuteis an ordinary, documented call).Reduction run under Miri
MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- pooland-- jsfail with the errors quoted above;-- fixedprintsfixed: ok. The default Stacked Borrows run gives the same three results.Fix
The addon, the pool thread and the event-loop queue all hold the work as a raw pointer, so the entry points now keep it one.
schedule,run,cancelandrun_from_jstakethis: *mut napi_async_work; each reads or writes the individual fields it needs through statement-scoped(*this).fieldaccesses (the atomicstatusthrough its own&AtomicU32, which is what letscancelandrunoverlap), and nothing forms a reference to the whole work at any point:runcopies theLoopHandle(cloned),execute,envanddataout of the work first, runsexecute, then in one last access stores the final status (or cancels) and fills in the embedded task with(*this).concurrent_task.from(this, ..); the post goes through the cloned handle and only the clone is touched afterwards. This is the shapeS3HttpSimpleTask::http_callback(src/runtime/webcore/s3/simple_request.rs) already uses for its embedded task, and the clone-the-handle-first orderasync_job_runin node_zlib_binding.rs uses;post_to_js_threadis folded into it.run_from_jstakespoll_ref,complete,env,statusanddataout of the work before callingcompleteand does not usethisafterwards. The post-completeexception check goes through the env pointer, which the global keeps alive (GlobalObject::m_napiEnvs) independently of the work's own ref; that was already what the old code relied on, and the comment now says so. The struct's field docs record which thread writes each field (the JS thread still readsglobalandscheduled, and CASesstatus, while the pool has the work; the pool writesstatusandconcurrent_task; the rest is immutable afternew), which is the invariant the field-by-field accesses rest on.scheduleprojects the task pointer from the work pointer (&raw mut (*this).task), which is whatfrom_task_ptrasks for.napi_queue_async_work,napi_cancel_async_workandnapi_delete_async_worknull-check and pass the pointer through instead of going viaas_mut(); theNapiAsyncWorkdispatch arm (src/runtime/dispatch.rs) usescast_ptr!, andTaskable::release_unruncalls the associated function.No behaviour change: the same fields are read and written (
execute/env/dataare now copied out before the status CAS instead of after it, which is unobservable: nothing else writes them), the status store still happens before the VM borrow is released, the post still goes to the same loop (the clone refers to the same VM handle), refusal is still unreachable for the same reason (counted work), and the teardown path still runscompletefrom the queue release.Seen while reviewing, not changed here
Running an addon's
completefrom the queue release at worker teardown is pre-existing behaviour this PR keeps. If thatcompletequeues another work (node-addon-api's chaining pattern),napi_queue_async_workruns intoembedded_work_scheduled's closed-handle assertion on debug builds and, on release builds,run's refusalunreachable!on the pool thread. That reproduces identically on main and is being handled separately; it is mentioned becausescheduleandrunare in this diff.Tests
test/internal/source-lints/self-receiver-intrusive-post.test.ts(new) bans filling an embedded task with the receiver's address:.from(..)applied toselfspelled as a pointer (directly or through a reborrow:from_mut(&mut *self),NonNull::from(&mut *self), a bare&mut *self; the same spelling list self-receiver-reclaim.test.ts uses), to a local of the same function bound to one, or to a parameter of a method that also has a reference receiver and is typed as a raw pointer to the method's own type, writtenSelfor by the enclosing impl's name (thepost_to_js_thread(&mut self, self_ptr: *mut Self)shape, and the same thing spelled*mut napi_async_work). It checks its patterns against positive and negative examples, including the two-function shape as it was on main in both spellings. Against main it reports exactlysrc/runtime/napi/napi_body.rs:1871; with this change it reports nothing and the tree needs no allowlist. The heap-task constructors (Task::init/create_from/from_callback) are the population bundler: hand a finished Bun.build back through its pointer, not a &mut receiver #37723's lint covers, so the two do not overlap; each lint in this family currently carries its own copy of the spelling list and scan scaffold, and folding those into a shared helper is a separate cleanup once the in-flight siblings have landed.run_from_jshas no syntactic marker a lint can pin; it is covered by the addon tests below, which delete the work insidecompleteunder the ASAN build.test/napi/napi.test.ts: new casetest_napi_async_work_cancel_running(addon side in async_tests.cpp) queues a work, waits forexecuteto start on the pool thread, callsnapi_cancel_async_workfrom the JS thread while it is running, and checks throughcomplete(which then deletes the work) that the cancel reportednapi_generic_failureand the completionnapi_ok, comparing against Node's output (compared line by line, since the addon'sprintfgoes through the Windows CRT and emits\r\nthere, which the first revision of this test tripped over on the Windows lanes). This is thecancel/runoverlap the field-scoped accesses exist for; the existing cancel test only cancels work that has not started.On the debug (ASAN) build: test/napi/napi.test.ts (the
napi_async_work, handle-scope and async-complete exception cases, plus the whole file: 166 pass, the one failure being the unrelatedbigint conversioncase hitting the 5 s default timeout on this machine, which it also does without this change and passes with a longer timeout), the node-api suitestest_async(its 500-iterationtest-loop.jspasses;test.jsand friends are pre-existing todos in that suite),test_worker_terminate(passes), andtest_instance_data,test_uv_threadpool_size,test_async_cleanup_hook(build, with their pre-existing todo entries unchanged), andbun test test/internal/source-lints/(86 pass).cargo clippy -p bun_runtimeandrustfmt --checkare clean on the touched files.