napi: drive a threadsafe function's JS-thread side off its pointer, not &mut self

[robobun]

Problem

• The JS-thread half of napi's ThreadSafeFunction (dispatching queued calls, ref/unref, env teardown) ran as &mut self methods on the shared object.
• A &mut self argument claims the whole object for the length of the call, but addon threads take its lock and bump its counters the whole time the user's callback runs, the callback can unref the same function re-entrantly (test_issue_11949), and after teardown's last phase an addon thread may free it while the method is still returning.
• The finalize task was posted with a pointer derived from that receiver; the caller's next use of its own pointer invalidates it before destroy frees through it.
• A reduction of these four shapes fails Miri under both Tree Borrows (protected tags must never be Disabled) and Stacked Borrows, and passes in this PR's shape. No crash is known and the compiler does not exploit it today. Same class as fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703, bundler: make Worker::deinit_soon take the worker pointer instead of &mut self #37685, node:fs: let the fs completions own their task box instead of freeing it through &mut self #37693, blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705, bundler: hand a finished Bun.build back through its pointer, not a &mut receiver #37723, bundler: allocate, link and post a plugin request through one pointer instead of a &mut receiver #37732; the mirror image of napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741, which converts the addon-thread half.

Fix

• The four JS-thread functions take this: *mut ThreadSafeFunction and borrow one field per statement, as push / release already do; the loop_mut, ref_ and unref wrappers go away and their callers touch poll_ref or a copied BackRef directly.
• Sound because no borrow of the object is live across the callback, a lock release, or a call into another method (call copies its inputs out first; the finalize task carries the pointer the object was dispatched with, the one destroy frees). The lock is taken and dropped at the same points as before and the finalize / free decisions are unchanged.
• The new lint ratchets the five methods napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741 converts and one reborrow in napi_acquire_threadsafe_function; whichever PR lands second drops the other's allowlist entry.
• Verification: the lint fails on main (listing the seven old receivers) and passes here, but it checks signatures only; there is no runtime repro since nothing crashes. The existing napi addon tests, including unref from inside the callback and worker teardown, pass on the ASAN build; clippy and fmt are clean.

Background

• A threadsafe function is the Node-API object addon threads use to queue calls (napi_call_threadsafe_function) that the JS thread later runs; addon threads acquire and release it, and the last release, the JS-thread finalizer, or env teardown frees it. One allocation is shared between the JS thread and any number of addon threads for its whole life.
• In Rust a &mut self (or &self) argument is a promise, passed to LLVM as noalias, that nothing else reads or writes the object while the call is on the stack; holding a lock around each field access does not change that. Going through (*this).field claims only that field for that statement.
• Miri is rustc's interpreter that checks those aliasing rules; Tree Borrows (what bun run rust:miri uses) and Stacked Borrows are its two models. A "protected tag" in its errors is the reference argument of a call that has not returned yet.
• poll_ref is the object's keep-the-event-loop-alive handle and is touched only by the JS thread (ref/unref are JS-thread-only in Node as well), so borrowing that one field is fine while addon threads are in the rest of the object.
• Tests under test/internal/source-lints/ read the source tree and assert a shape; a ratchet is an allowlist of remaining violations that may shrink but not grow.

---

[review] gate passed · iteration 0 · 2 files touched

fails on main (without fix)

ASAN without fix: 2 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/napi-tsfn-receivers.test.ts
bun test v1.4.0 (4ddfdae64)

test/internal/source-lints/napi-tsfn-receivers.test.ts:
(pass) the audit recognizes the shapes it claims to [41.00ms]
(pass) ThreadSafeFunction is still where this lint looks for it [3.53ms]
376 |   );
377 | });
378 | 
379 | test("the pinned functions take the object as `this: *mut`", () => {
380 |   const firstParams = Object.fromEntries(PINNED.map(name => [name, tree.methods[name]]));
381 |   expect(firstParams).toEqual(
                            ^
error: expect(received).toEqual(expected)

  {
-   "call": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "destroy": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "dispatch_one": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "env_teardown": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "free_orphaned": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "maybe_queue_finalize
... (truncated)

release without fix: 2 FAILED
bun test v1.4.0-canary.1 (da3851e57)

test/internal/source-lints/napi-tsfn-receivers.test.ts:
(pass) the audit recognizes the shapes it claims to [0.54ms]
(pass) ThreadSafeFunction is still where this lint looks for it [0.08ms]
376 |   );
377 | });
378 | 
379 | test("the pinned functions take the object as `this: *mut`", () => {
380 |   const firstParams = Object.fromEntries(PINNED.map(name => [name, tree.methods[name]]));
381 |   expect(firstParams).toEqual(
                            ^
error: expect(received).toEqual(expected)

  {
-   "call": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "destroy": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "dispatch_one": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "env_teardown": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "free_orphaned": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "maybe_queue_finalizer": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "on_dispatch": StringMatching /^this\s*:\s*\*\s*mut\s+(?:Self|ThreadSafeFunction)$/,
-   "push": Strin
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/napi-tsfn-receivers.test.ts
bun test v1.4.0 (4ddfdae64)

test/internal/source-lints/napi-tsfn-receivers.test.ts:
(pass) the audit recognizes the shapes it claims to [48.94ms]
(pass) ThreadSafeFunction is still where this lint looks for it [3.63ms]
(pass) the pinned functions take the object as `this: *mut` [12.22ms]
(pass) no other parameter takes the object by reference [1.37ms]
(pass) no ThreadSafeFunction method takes a reference receiver, beyond the ones still being converted [2.14ms]
(pass) neither the methods nor the entry points reborrow the whole object from its pointer [6.93ms]

 6 pass
 0 fail
 9 expect() calls
Ran 6 tests across 1 file. [21.22s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 816ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/144] gen NodeModuleModule.lut.h
Generating /workspace/bun/build/release/codegen/NodeModuleModule.lut.h from /workspace/bun/src/jsc/modules/NodeModuleModule.cpp
[2/144] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser 
... (truncated)

diff hotspot

src/runtime/napi/napi_body.rs                      | 315 ++++++++--------
 .../source-lints/napi-tsfn-receivers.test.ts       | 401 +++++++++++++++++++++
 2 files changed, 569 insertions(+), 147 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file

file                                                    reads  edits  tests
src/runtime/napi/napi_body.rs                              14     24      0
test/internal/source-lints/napi-tsfn-receivers.test.ts      2      7      0

Original description

Problem

The JS-thread side of ThreadSafeFunction (src/runtime/napi/napi_body.rs) still went through whole-object receivers: on_dispatch(this) called (*this).dispatch_one(&mut self), which called call(&mut self) and maybe_queue_finalizer(&mut self), and napi_internal_threadsafe_function_env_teardown called (*this).env_teardown(&mut self). napi_ref_threadsafe_function / napi_unref_threadsafe_function went through ref_(&mut self) / unref(&mut self).

A reference argument claims the object for the duration of the call (it is what rustc's noalias on the argument promises LLVM), and this object is shared for its whole life:

• dispatch_one takes lock and then runs the user's callback in call, for as long as that takes. Any napi_call_threadsafe_function / napi_acquire_threadsafe_function / napi_release_threadsafe_function arriving from an addon thread in that time does a read-modify-write on the lock word and then on thread_count / queue.count, i.e. writes inside the range the protected &mut self covers. This is the steady state of every threadsafe function with a producer thread, not a race window.
• The callback run by call(&mut self) can re-enter the object from the same thread: napi_unref_threadsafe_function on the TSFN being dispatched (what the test_issue_11949 addon in test/napi does) formed a second &mut Self under the protected one.
• maybe_queue_finalizer posted let self_ptr: *mut Self = self as the finalize task. on_dispatch goes on using the object through its own pointer after dispatch_one returns (the Running -> Idle CAS), which invalidates that receiver-derived pointer, and the next on_dispatch then frees through it in destroy.
• env_teardown coordinates with live addon threads through lock across its three phases, and its last phase publishes env_teardown_done: from the moment its guard drops, a thread dropping the last reference frees the allocation, while the &mut self frame is still returning.

A reduction of these four shapes (below) fails under both aliasing models and passes with the shape this PR uses. Tree Borrows, the model bun run rust:miri uses, reports the first, second and fourth as foreign read access would cause the protected tag <N> (currently Unique) to become Disabled; protected tags must never be Disabled, with <N> pointing at the &mut self of dispatch_one / env_teardown, and the third as the posted pointer having state Disabled which forbids this reborrow when it is freed; Stacked Borrows reports would remove [Unique for <N>] which is strongly protected and tag does not exist in the borrow stack. No crash is known from any of this; the compiler does not currently exploit it. It is the contract that is wrong, the same class as #37703, #37685, #37693, #37705, #37723 and #37732, and the mirror image of #37741, which converts the addon-thread side of this same object and lists this side as reported separately.

Reduction run under Miri

HANDLE is the addon's copy of the napi_threadsafe_function pointer; POSTED is what maybe_queue_finalizer put in the finalize task. <scenario>-before is the shape on main, <scenario>-after the shape in this PR.

use std::sync::atomic::{AtomicI64, AtomicPtr, AtomicU8, AtomicU32, Ordering::SeqCst};

struct Tsfn {
    lock: AtomicU32,
    thread_count: AtomicI64,
    dispatch_state: AtomicU8,
    // The JS thread's plain fields.
    has_queued_finalizer: bool,
    poll_ref: u32,
}

static HANDLE: AtomicPtr<Tsfn> = AtomicPtr::new(std::ptr::null_mut());
static POSTED: AtomicPtr<Tsfn> = AtomicPtr::new(std::ptr::null_mut());

/// An addon thread calling napi_call/acquire/release_threadsafe_function
/// while the JS thread is inside its method: takes the lock, touches a counter.
fn addon_thread_uses_it() {
    std::thread::scope(|s| {
        s.spawn(|| {
            let h = HANDLE.load(SeqCst);
            unsafe {
                (*h).lock.swap(1, SeqCst);
                (*h).thread_count.fetch_sub(1, SeqCst);
                (*h).lock.swap(0, SeqCst);
            }
        });
    });
}

/// The last release of an orphaned TSFN: frees it once it gets the lock.
fn addon_thread_frees_it() {
    std::thread::scope(|s| {
        s.spawn(|| {
            let h = HANDLE.load(SeqCst);
            unsafe {
                (*h).lock.swap(1, SeqCst);
                (*h).thread_count.fetch_sub(1, SeqCst);
                drop(Box::from_raw(h));
            }
        });
    });
}

// main
impl Tsfn {
    fn dispatch_one_before(&mut self, scenario: &str) {
        self.lock.swap(1, SeqCst); // dispatch_one's lock_guard
        let _ = self.thread_count.load(SeqCst);
        self.lock.swap(0, SeqCst);
        match scenario {
            // call(): the user's callback runs here, for as long as it takes.
            "js" => addon_thread_uses_it(),
            // ... and it may napi_unref_threadsafe_function this very TSFN.
            "reenter" => unsafe { (*HANDLE.load(SeqCst)).unref_before() },
            "finalize" => self.maybe_queue_finalizer_before(),
            _ => unreachable!(),
        }
    }
    fn unref_before(&mut self) {
        self.poll_ref -= 1;
    }
    fn maybe_queue_finalizer_before(&mut self) {
        self.has_queued_finalizer = true;
        let self_ptr: *mut Self = self;
        POSTED.store(self_ptr, SeqCst); // loop.enqueue_task(Task::init(self_ptr))
    }
    fn env_teardown_before(&mut self) -> bool {
        self.lock.swap(1, SeqCst); // phase 3
        self.poll_ref = 0;
        let caller_frees = self.thread_count.load(SeqCst) <= 0;
        self.lock.swap(0, SeqCst); // guard dropped: the addon thread may free us now
        if !caller_frees {
            addon_thread_frees_it();
        }
        caller_frees
    }
}

// this PR
impl Tsfn {
    unsafe fn dispatch_one_after(this: *mut Tsfn, scenario: &str) {
        unsafe {
            (*this).lock.swap(1, SeqCst);
            let _ = (*this).thread_count.load(SeqCst);
            (*this).lock.swap(0, SeqCst);
        }
        match scenario {
            "js" => addon_thread_uses_it(),
            "reenter" => unsafe { Tsfn::unref_after(HANDLE.load(SeqCst)) },
            "finalize" => unsafe { Tsfn::maybe_queue_finalizer_after(this) },
            _ => unreachable!(),
        }
    }
    unsafe fn unref_after(this: *mut Tsfn) {
        unsafe { (*this).poll_ref -= 1 };
    }
    unsafe fn maybe_queue_finalizer_after(this: *mut Tsfn) {
        unsafe { (*this).has_queued_finalizer = true };
        POSTED.store(this, SeqCst);
    }
    unsafe fn env_teardown_after(this: *mut Tsfn) -> bool {
        let caller_frees = unsafe {
            (*this).lock.swap(1, SeqCst);
            (*this).poll_ref = 0;
            let caller_frees = (*this).thread_count.load(SeqCst) <= 0;
            (*this).lock.swap(0, SeqCst);
            caller_frees
        };
        if !caller_frees {
            addon_thread_frees_it();
        }
        caller_frees
    }
}

fn main() {
    let arg = std::env::args().nth(1).unwrap_or_default();
    let (scenario, after) = match arg.rsplit_once('-') {
        Some((s, "before")) => (s.to_string(), false),
        Some((s, "after")) => (s.to_string(), true),
        _ => panic!("usage: {{js,reenter,finalize,teardown}}-{{before,after}}"),
    };
    let t = Box::into_raw(Box::new(Tsfn {
        lock: AtomicU32::new(0),
        thread_count: AtomicI64::new(1),
        dispatch_state: AtomicU8::new(0),
        has_queued_finalizer: false,
        poll_ref: 1,
    }));
    HANDLE.store(t, SeqCst);
    if scenario == "teardown" {
        // napi_internal_threadsafe_function_env_teardown
        let caller_frees = unsafe {
            if after { Tsfn::env_teardown_after(t) } else { (*t).env_teardown_before() }
        };
        if caller_frees {
            unsafe { drop(Box::from_raw(t)) };
        }
        return;
    }
    // on_dispatch(this): one dispatch_one, then the Running -> Idle CAS.
    unsafe {
        if after { Tsfn::dispatch_one_after(t, &scenario) } else { (*t).dispatch_one_before(&scenario) }
    }
    let _ = unsafe { (*t).dispatch_state.compare_exchange(0, 1, SeqCst, SeqCst) };
    // The next on_dispatch sees Closed and runs destroy() on the posted pointer.
    let posted = POSTED.load(SeqCst);
    unsafe { drop(Box::from_raw(if posted.is_null() { t } else { posted })) };
}

scenario	Tree Borrows (-Zmiri-tree-borrows)	Stacked Borrows (default)
js-before	UB: foreign read access would cause the protected tag ... to become Disabled, tag created at fn dispatch_one_before(&mut self, ..)	UB: would remove [Unique for <dispatch_one_before>] which is strongly protected
reenter-before	UB: same, at the reborrow for unref_before	UB: same, naming unref_before and dispatch_one_before
finalize-before	UB: the posted tag has state Disabled which forbids this reborrow, created at fn maybe_queue_finalizer_before(&mut self)	UB: trying to retag from <posted> for Unique permission ..., but that tag does not exist in the borrow stack
teardown-before	UB: foreign read access would cause the protected tag ... to become Disabled, tag created at fn env_teardown_before(&mut self)	UB: would remove [Unique for <env_teardown_before>] which is strongly protected
all four -after	pass	pass

Fix

dispatch_one, call, maybe_queue_finalizer and env_teardown take this: *mut ThreadSafeFunction and borrow one field per statement, the way push / release already do on the other side of the object: (*this).lock.lock_guard() (the guard holds the lock by pointer, so nothing points into the object across the callback except that), (*this).queue.data.read_item() under it, and so on. Concretely:

• on_dispatch passes its pointer down: Self::dispatch_one(this, is_first).
• call copies what the callback needs out of the object first (the env pointer, the Copy tracker, the JS value or call_js function pointer, ctx), so no borrow of the threadsafe function exists while the microtask drain or the callback runs. The JS and C paths do the same things in the same order as before; they are just no longer two arms of one match on a borrowed field.
• maybe_queue_finalizer posts Task::init(this), the pointer on_dispatch was dispatched with, which is the one destroy later frees.
• env_teardown keeps its three phases, with the phase 1 and phase 3 critical sections as single blocks; env_teardown_done is still published, and thread_count still read, under the lock, and nothing touches the object after the guard drops. Its caller already had the pointer.
• loop_mut, ref_ and unref are deleted: the two loop users copy the Copy BackRef out of the field and call get_mut() on the copy, and napi_ref_threadsafe_function, napi_unref_threadsafe_function, napi_create_threadsafe_function and destroy call poll_ref.ref_() / unref() on the field directly, which is all the wrappers did. poll_ref is the JS thread's (as napi_ref/unref_threadsafe_function are JS-thread-only in Node too), so a borrow of that one field is sound while addon threads are in the rest of the object.
• The rule itself is stated and enforced by the lint below rather than by a comment on the struct (an earlier revision had one; it said nothing the signatures and the lint do not).

Behaviour is unchanged: the lock is taken and dropped at the same points, the condvar signals, state transitions and the finalize / free decisions are the same, and call runs the same code in the same order.

Why raw pointers rather than &self plus interior mutability

The other way to make this object sound is to change its layout: put queue.data behind a Guarded, the JS thread's plain fields (poll_ref, callback, env, finalizer_fun, has_queued_finalizer) into JsCell, and then give everything that does not post or free the object a &self receiver, keeping *mut only for on_dispatch, the two posting functions and the freeing ones. That is the better end state and nothing here is in its way: every function this PR converts would keep working unchanged under that layout (a raw pointer is sound wherever a shared reference is), so it is a later simplification on top of this, not a reversal of it. It is not done here because it rewrites both halves of the object at once (the addon-thread half is #37741, in flight, and #36831 / #36801 are open against the same functions), while this PR is a behaviour-preserving change to one half. With the current layout, &self is not a sound receiver either (a shared reference covering queue.data and the plain fields is invalidated by the writes the other party makes to them), which is why the lint bans both kinds of reference for now; its header says that the &self half of the ban is what to lift when the layout changes.

Relationship to other open PRs

• napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741 converts the addon-thread methods of this object (enqueue -> push_locked, release_locked, schedule_dispatch, is_closing, acquire); this PR converts the JS-thread methods. The two are independent (each removes its own side's receivers); a three-way merge of the two branches' napi_body.rs against their common base applies cleanly, and the merged file passes the lint below once its two ratchets are emptied. The lint below ratchets napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741's five methods and the one &mut *func in napi_acquire_threadsafe_function by name, so when it lands its entries are deleted and both lists are empty; conversely napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741's self-receiver-publish lint allowlists maybe_queue_finalizer's post, which this PR removes, so whichever lands second drops that allowlist entry (this PR, if it is the second one).
• napi: keep dispatching threadsafe function calls when a callback blocks in a nested event loop #36831 and napi: align symbol/buffer/coercion/reference/tsfn semantics with Node.js #36801 rewrite parts of on_dispatch / dispatch_one / call for other reasons and will conflict with this; napi: keep dispatching threadsafe function calls when a callback blocks in a nested event loop #36831 already moves dispatch_one and call to *mut Self as part of its change, so the shapes are compatible.

Tests

test/internal/source-lints/napi-tsfn-receivers.test.ts parses the signature and body of every method of the inherent impl ThreadSafeFunction and of every *threadsafe_function* extern "C" entry point under src/runtime/napi/ (brace- and paren-matched, so rustfmt-wrapped parameter lists, generics, lifetimes and qualified pub(crate) unsafe extern "C" items are all seen) and asserts four things:

1. the converted functions (on_dispatch, dispatch_one, call, maybe_queue_finalizer, env_teardown, plus push, release, destroy, free_orphaned, which already had the shape) take the object as this: *mut ..;
2. no other parameter of any of these functions takes the object as anything but a raw pointer, a move or a Box (so renaming the receiver to a this: &mut ThreadSafeFunction parameter, or Option<&mut Self> / NonNull<Self>, fails; zero today, so there is no ratchet);
3. the set of methods with a reference receiver is exactly the ratcheted list of the five napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741 converts;
4. neither the methods nor the entry points reborrow the whole object from the handle (&mut *this, &*func, func.as_mut()), with napi_acquire_threadsafe_function's one site ratcheted.

It checks its own parser and patterns against an inline fixture (including the lines it reports), anchors itself on the pinned names and the seven entry points so it cannot pass vacuously if the code moves, and its header lists what it knowingly does not see (a reborrow through a local alias of the handle, and callers outside src/runtime/napi/ such as the dispatch.rs arm, which passes cast_ptr!). Against main's napi_body.rs assertions 1 and 3 fail, the latter listing

+   "call",
+   "dispatch_one",
+   "env_teardown",
+   "loop_mut",
+   "maybe_queue_finalizer",
+   "ref_",
+   "unref",

beyond the allowed list; with this branch all four pass. I also applied seven single-site regressions to this branch's napi_body.rs (dispatch_one(this: &mut ThreadSafeFunction) with the caller coercing, env_teardown(this: &mut Self) reached through a local alias, on_dispatch(this: &mut ThreadSafeFunction), an entry point with a &mut ThreadSafeFunction parameter, a new &self helper, a lifetime-annotated &'a self helper, and a qualified entry point reborrowing &mut *func); each one fails the lint.

The behaviour the converted functions implement is covered by the existing addon tests, all run on the debug (ASAN) build of this branch: test/napi/napi.test.ts (166 pass; the threadsafe-function ones exercise the empty-queue and after-call finalize paths, abort with and without queued items, blocked producers on a bounded queue, microtask draining between callbacks, napi_unref_threadsafe_function from inside the callback being dispatched (test_issue_11949), napi_ref/unref_threadsafe_function with a NULL env, and the worker-teardown paths through all three phases of env_teardown, including the ones where an addon thread's later call or release frees the orphan and where the caller frees it), and test/napi/node-napi-tests/test/node-api/{test_threadsafe_function,test_worker_terminate,test_worker_terminate_finalization,test_env_teardown_gc}/do.test.ts (10 pass; test_threadsafe_function/test.js is a pre-existing todo, it crashes in uv_thread_create). bun test test/internal/source-lints/ (19 files, 88 tests), cargo clippy -p bun_runtime --no-deps and cargo fmt --check are clean.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 24 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: f6e74460-66f7-4edd-8aa0-08cb67a4e20a

📥 Commits

Reviewing files that changed from the base of the PR and between 9518091 and 4ddfdae.

📒 Files selected for processing (2)

• src/runtime/napi/napi_body.rs
• test/internal/source-lints/napi-tsfn-receivers.test.ts

---

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

[robobun]

Status: ready for review, at 4ddfdae.

Reproduced as the Miri reduction in the PR body (all four -before shapes fail under Tree Borrows and Stacked Borrows, all four -after shapes pass); the source lint in the PR fails against main's napi_body.rs (the pinned functions still take &mut self, and it names the seven converted methods) and passes with this branch. The threadsafe-function addon tests listed under Tests were run on the debug (ASAN) build of this branch, and again after the follow-up commits.

Review so far: the automated review found no bugs and asked for human eyes on call and env_teardown phase 3; the comment-cop threads are addressed (the struct note is removed, the SAFETY lines are down to one or two lines) and resolved. For whoever picks this up, the two invariants to check are:

• call: everything the callback uses (env, tracker, the JS value or call_js pointer, ctx) is read out of the object before drain_microtasks runs or the callback is invoked, so nothing points into the threadsafe function while user code runs; the JS and C paths perform the same calls in the same order as the two former match arms.
• env_teardown phase 3: env_teardown_done is published and thread_count is read inside the same guard as before, the guard drops when the block ends, and after that only the caller's raw pointer exists (it frees if the result says so; otherwise the addon thread that drops the last reference does).

CI: two builds so far, 92891 on the first revision and 93276 on the current head (4ddfdae). Both are 192 of 194 jobs passed, every build lane, the asan lane and the source lints included; on both, the only non-passing jobs are the two darwin 26 aarch64 test-bun entries, which expired after three attempts without ever being picked up by an agent (the same lane has been ending waiting_failed on essentially every PR build today, so it is lane capacity, not this diff). The test-level failures the builds reported all passed on retry or alone and are unrelated (child_process, transpiler-stack-overflow and inspect-error-leak in parallel batches, a solc timeout on Windows aarch64, a localPort collision in node-net, and one run of napi_get_value_string_* > behaves like node on edge cases, a string test whose output comparison came out reordered and which passed on retry; the threadsafe-function tests passed on every lane that ran). Not retriggering into the darwin shortage; say the word if you want a re-run once that lane is healthy.

The addon-thread half of the same object is #37741; the two branches' napi_body.rs merge without conflicts, and whichever lands second empties the other's ratchet entries as described in the body.

[claude]

I reviewed this PR and didn't find any bugs. Because it restructures the JS-thread dispatch/teardown paths of ThreadSafeFunction — unsafe raw-pointer code with cross-thread access and lock/guard-drop ordering that determines who frees the allocation — a human look would still be worthwhile, particularly on the reshaped call() and env_teardown() phase 3.

What was reviewed:

• maybe_queue_finalizer: the prev == Closed early-return is equivalent to the old Closing || NotClosing match (ClosingState has exactly three variants); has_queued_finalizer ordering and the event_loop == None early return are preserved.
• call: traced both TsfnCallback::Js and ::C paths — same operations in the same order; _dispatch and _hs scope-guard lifetimes match the old arms; BackRef and AsyncTaskTracker are Copy so the field reads move nothing.
• dispatch_one / env_teardown: lock-guard scopes (labeled block → unsafe block) drop at the same points; thread_count is still read under the phase-3 lock and nothing touches *this after the guard drops.
• Confirmed ref_/unref/loop_mut inlining is behavior-identical at all four call sites.

Extended reasoning...

Overview

This PR converts the JS-thread-side methods of ThreadSafeFunction in src/runtime/napi/napi_body.rs from &mut self receivers to this: *mut Self associated functions: dispatch_one, call, maybe_queue_finalizer, env_teardown. It deletes the loop_mut/ref_/unref wrappers and inlines their single-field access at the four call sites (destroy, napi_create/ref/unref_threadsafe_function). It adds a source-lint test that ratchets the remaining &self receivers (the addon-thread half, converted separately in #37741) and forbids whole-object reborrows of the handle. ~200 lines of native diff plus a 268-line lint test.

Security risks

None user-facing. This is an aliasing-model soundness fix (Tree Borrows / Stacked Borrows protector violations, noalias on &mut self that addon threads write through concurrently). No new attack surface, no input parsing, no privilege changes. The risk is a subtle behavior regression in a memory-safety-critical path — wrong lock-guard scope, dropped scope guard, or a field read that used to happen under the lock now happening outside it — which would show up as UAF or hang in native addons.

Level of scrutiny

High. This is unsafe Rust in the NAPI threadsafe-function implementation — raw pointers, a mutex whose guard-drop point determines whether an addon thread may concurrently free the object, condvar signaling, and a user callback that can synchronously re-enter the object. The call() body was restructured from a two-arm match on a borrowed field into destructure + let Some(call_js) else, which is the largest shape change and the place a subtle ordering difference would hide. env_teardown phase 3 wraps the critical section in an unsafe {} block whose tail expression is the return value; the _g guard still drops after thread_count is read and before anything else touches *this, which is the invariant that lets an addon thread free the orphan.

Other factors

The change follows an established pattern in this codebase (six sibling PRs cited, plus #37741 for the other half of this same struct), and the PR description is unusually thorough with a Miri reduction. Behavior coverage relies on the existing test/napi/napi.test.ts and node-api addon suites, which the description reports passing on debug+ASAN. I traced each converted function against its predecessor and found the lock/condvar/state-transition/guard-drop ordering preserved; I verified ClosingState has exactly three variants so the inverted condition in maybe_queue_finalizer is equivalent, and that BackRef<_, Mut> and AsyncTaskTracker are Copy so the by-value field reads are sound. That said, the PR itself flags textual conflicts with three other open PRs (#37741, #36831, #36801) touching the same functions, and the correctness of concurrent raw-pointer code in a finalize/free path is exactly where a maintainer's second pair of eyes adds the most value. Deferring rather than auto-approving.

[github-actions]

This PR may be a duplicate of:

1. napi: keep dispatching threadsafe function calls when a callback blocks in a nested event loop #36831 - Also rewrites ThreadSafeFunction::dispatch_one and call in src/runtime/napi/napi_body.rs from &mut self to *mut Self receivers for the same re-entrancy/aliasing reason, so the two changes overlap function-for-function.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #36831, though the two overlap textually (the PR body already lists it under the conflicts). #36831 fixes a deadlock when a callback blocks in a nested event loop (#36828): it adds in-flight dispatch accounting and a backup dispatch, and changing the signatures of dispatch_one and call is incidental to that. As of its current diff it still keeps the shapes this PR is about: the locked dequeue runs as take_one_locked(&mut self) (the whole-object receiver held while addon threads contend for the lock), it still calls (*this).maybe_queue_finalizer() as a &mut self method that posts a receiver-derived pointer, and it does not touch env_teardown, ref_ / unref or loop_mut, nor add the lint. It has also been conflicting with main since the 4th. This PR changes no behaviour and only converts the receivers; if #36831 is revived, it rebases onto this cleanly in shape (its dispatch_one / call are already *mut Self), and the lint here will tell it to convert take_one_locked as well.

[robobun]

^{Updated 1:05 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 4ddfdae has some failures in Build #93276 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37762

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

bun-37762 --bun

[robobun]

Three follow-up commits since the first round of review:

• b2b7ffd: the lint now asserts the shape instead of only banning two spellings of the old one. It parses each function's parameter list and requires the converted functions to take this: *mut .., rejects any other parameter that takes the object by reference (this: &mut ThreadSafeFunction, Option<&mut Self>, ...), and sees qualified extern "C" items and lifetime-annotated receivers. Seven single-site regressions applied to this branch each fail it; details in the Tests section of the description.
• a790170 and 4ddfdae: comment trims. The struct-level note is gone (the lint and the signatures carry the rule), which also means a three-way merge of this branch and napi: drive a threadsafe function's addon-thread entry points off its pointer, not &mut self #37741's napi_body.rs now applies with no conflicts; the remaining added comments are the one- or two-line SAFETY lines the unsafe blocks require.

The description also gained a section on why this is raw pointers rather than &self plus Guarded / JsCell: that layout change is the better end state, nothing here is in its way, and it is not done in this PR because it would rewrite both halves of the object while #37741 is in flight.

[claude]

I reviewed this PR and didn't find any bugs. Because it restructures the concurrent JS-thread paths of ThreadSafeFunction (lock-guard scope in env_teardown phase 3, copy-out ordering in call before user code runs), a human look at those two invariants would still be worthwhile.

Checked: maybe_queue_finalizer's inverted branch is equivalent (ClosingState has exactly three variants); BackRef<EventLoop, Mut> is Copy, so (*this).event_loop reads a copy, not a borrow; env_teardown phase 3's guard now drops at the unsafe {} block end, which is still after the thread_count read and the last field write; the deleted ref_/unref/loop_mut wrappers have all four call sites updated in this diff.

Extended reasoning...

Overview

This PR converts the JS-thread-side methods of ThreadSafeFunction in src/runtime/napi/napi_body.rs from &mut self receivers to this: *mut ThreadSafeFunction associated functions: dispatch_one, call, maybe_queue_finalizer, env_teardown. It deletes the thin wrappers loop_mut, ref_, unref and inlines their bodies at the four call sites (destroy, napi_create_threadsafe_function, napi_ref/unref_threadsafe_function). It also adds a 400-line source-lint test that parses the impl block and *threadsafe_function* entry points to enforce the pointer-receiver shape and forbid whole-object reborrows, with a ratchet list for the addon-thread methods that #37741 converts.

Security risks

None. This is an internal refactor of receiver shapes to satisfy Rust's aliasing model (Stacked/Tree Borrows); no user-facing API, input validation, or trust boundary changes.

Level of scrutiny

High. This is the "Native code: memory safety" category from REVIEW.md — the most-blocked class. ThreadSafeFunction is reached concurrently from addon threads and re-entrantly from user callbacks; the refactor is not purely mechanical (call's two match arms became a destructure + early-return; env_teardown phase 3's lock guard moved from function-tail scope into a block; maybe_queue_finalizer's branch was inverted). Each of those preserves behavior on inspection, but the ordering invariants (what is read out of the object before user code can re-enter, and where the phase-3 guard drops relative to env_teardown_done being published) are exactly the kind of thing a human maintainer of this file should confirm.

Other factors

• The bug hunter found nothing; CI (192/194) and the ASAN napi suites passed per the author's status comment.
• I verified ClosingState has exactly {NotClosing, Closing, Closed}, so if prev == Closed { return } is equivalent to the old match prev { Closing | NotClosing => ..., _ => {} }.
• I verified BackRef is Copy (src/ptr/lib.rs:93-96), so the new unsafe { (*this).event_loop } copies the Option<BackRef<..>> out rather than borrowing into the object across drain_microtasks/enqueue_task.
• The comment-cop bot fired three times on paragraph-length comments; follow-up commits a790170 and 4ddfdae shortened/removed them.
• This PR overlaps textually with open PRs #37741 / #36831 / #36801 and encodes a merge-order-dependent ratchet in the new lint — a maintainer should be aware of the coordination when landing.

Given the concurrency-critical nature and the non-mechanical restructuring, deferring rather than auto-approving.