napi: run napi_add_finalizer and napi_create_external finalizers at env teardown

[robobun]

Problem

Node-API guarantees that every registered finalizer runs at environment teardown, even for objects that are still strongly reachable at exit (the RefTracker::FinalizeAll pass in napi_env__::DeleteMe()). Addons rely on this to flush and close native resources held by externals: DB handles, mmap'd files, thread pools.

Bun only honored that contract for napi_wrap. Its finalizer is registered on the NapiEnv teardown list (m_finalizers) that NapiEnv::cleanup() drains. napi_add_finalizer and napi_create_external registered their finalizers only as GC callbacks (vm.heap.addFinalizer and the NapiExternal destructor respectively), so for anything still alive at exit they never ran.

At process exit that means buffers are never flushed. In a long-lived process that creates and destroys envs (workers), it is an unbounded native leak.

Reproduction

An addon that registers all three kinds of finalizer (each prints finalize: <name> to stderr), with every JS object kept strongly reachable until exit so env teardown is their only chance to run:

napi_wrap(env, o1, NULL, fin, "wrap", NULL);
napi_add_finalizer(env, o2, NULL, fin, "add_finalizer", NULL);
napi_add_finalizer(env, o3, NULL, fin, "add_finalizer_ref", &ref);
napi_create_external(env, NULL, fin, "external", &ext);

Node v26.3.0 prints all four. Bun prints only:

finalize: wrap

Fix

• napi_add_finalizer now always creates a NapiRef and registers the same wrap_cleanup teardown entry napi_wrap uses, for both the napi_ref-returning and result == NULL variants. The GC-time deferral semantics are unchanged: NapiRef::callFinalizer applies the same mustDeferFinalizers() && inGC() rule the old doFinalizer lambda did.
• napi_create_external registers an external_cleanup entry on the same list. NapiExternal gets an m_boundCleanup field; its destructor deactivates the entry when GC collects the external first, and external_cleanup clears m_finalizer before calling it, so a finalizer never runs twice on either ordering.

Existing behavior that protected against double-invocation is preserved: callFinalizer() copies and clears the finalizer before calling it, and BoundFinalizer::deactivate removes the entry (or marks it inactive if teardown is mid-iteration).

Review also found a second, narrower instance of the same class in the drain loop itself. A dying owner's entry is only tombstoned during the drain (erasing the current node would invalidate the iterator), and NapiRef is TZone allocated, so a teardown finalizer that deletes a ref and registers a new one can get the freed address back; the new entry's (callback, hint, data) key then aliases the tombstone, ListHashSet::add deduplicates to the dead node, and clear() frees it while the new ref still points at it. addFinalizer now reactivates the entry it returns, which is a no-op for genuinely new nodes and is the only way a key can alias a dead one (two live owners can never collide because the key includes the owner pointer).

Intentionally excluded sibling

napi_create_external_arraybuffer / napi_create_external_buffer have the same gap (Node runs their finalizers at teardown, Bun does not). Their finalizer is the ArrayBuffer::Contents deleter, and invoking it at env teardown while the JSArrayBuffer is still a reachable cell would free memory that later env cleanup hooks can still read through the buffer. Doing it safely requires detaching the buffer first, which needs per-env tracking of live external buffers. That is a larger, separate change.

Verification

test/napi/napi-teardown-finalizers.test.ts (new addon test_teardown_finalizers.c):

1. All four registration kinds fire at teardown when every object is rooted until exit.
2. A finalizer already run by GC does not run again at teardown, and teardown-time ones still do: each name appears exactly once.

Both tests fail on the unfixed build (only wrap fires) and pass with the fix. With the fix, all 7 test_wrap_lifetime_* / test_ref_deleted_* sub-tests produce output identical to node v26.3.0, and the napi_create_external_buffer / napi_create_external_arraybuffer suites still pass.

Rebase conflicts

Rebased onto main after it landed the #30286 fix, which added clearExceptionsBetweenFinalizers() after each finalizer in the same cleanup() loop this PR rewrites into a drain-until-idle. Resolved by calling clearExceptionsBetweenFinalizers() after each callback inside the new drain loop, preserving both the #30286 semantics (each finalizer starts from a clean exception state) and this PR's (entries appended mid-drain are reached before clear() frees them). Both of main's #30286 tests pass with the merged loop, as do this PR's four tests and the existing LIFO-order test.

The other conflict was binding.gyp: both sides appended a target at the end of the list; kept both.

[robobun]

^{Updated 6:49 AM PT - Jul 8th, 2026}

❌ @robobun, your commit 6ed3422 has 2 failures in Build #70437 (All Failures):

• test/integration/next-pages/test/dev-server.test.ts - code 1 on 🍎 14 aarch64
• test/js/bun/s3/s3.test.ts - code 1 on 🍎 14 x64

---

🧪   To try this PR locally:

bunx bun-pr 32912

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

bun-32912 --bun

[github-actions]

Found 3 issues this PR may fix:

1. napi: NapiFinalizerTask boxes leak at exit when the final GC sweeps deferred finalizers (LSAN failure in 30205 isolate test on asan CI) #32144 - NapiFinalizerTask boxes leak at exit because deferred finalizers are never drained during teardown; this PR ensures finalizers run at env teardown rather than relying solely on GC
2. NAPI FATAL Error::New napi_create_error during Worker exitAndDeinit (NapiFinalizer cleanup) — deterministic at 15 jobs via gitnexus + tree-sitter #30286 - NAPI fatal error during Worker exitAndDeinit in the finalizer cleanup path; this PR properly manages finalizer invocation at env teardown and prevents double-invocation
3. bun test 1.3.13 crashes on shutdown (macOS, 'A C++ exception occurred') and during execution (Linux x86-64, 'Segmentation fault at address 0x1A' + SIGILL) in onnxruntime-node test suite; 1.2.23 is fine #30431 - Native NAPI addon (onnxruntime-node) crashes on shutdown during cleanup; this PR's fix for proper finalizer teardown and double-invocation prevention could address the shutdown crash

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #32144
Fixes #30286
Fixes #30431

🤖 Generated with Claude Code

[robobun]

I looked at each of these against their stack traces before adding any Fixes lines, and none of the three is resolved by this PR, so I am not adding them. Leaving the reasoning here so nobody pastes the block in later.

#32144 is not about which finalizers are registered for teardown. Its leaked objects are NapiFinalizerTask boxes created from NapiRefSelfDeletingWeakHandleOwner::finalize during the final collectNow() in Zig__GlobalObject__destructOnExit, after cleanup hooks have already been walked. Those come from napi_wrap refs, which this PR does not touch. The leak is that NapiFinalizerTask::schedule() parks the task on a queue that never drains again at that point in teardown. #32146 is the change that targets that drain path.

#30286 fatals inside a finalizer that is already running during NapiEnv::cleanup() at web_worker.exitAndDeinit. "NAPI FATAL ERROR: Error::New napi_create_error" is node-addon-api's NAPI_FATAL_IF_FAILED firing because napi_create_error itself returned a failing status during worker teardown, most likely a termination request or pending exception on the VM making every subsequent napi call fail and cascade. This PR changes which finalizers are on the cleanup list, not what happens when a napi call fails while one is executing. For transparency: since tree-sitter uses Napi::External heavily, this PR will cause more of its finalizers to actually run at worker teardown (as they do on Node), so if that underlying bug exists it may become easier to reproduce, but it is a separate bug either way.

#30431 reports two distinct crashes, and the Linux one (Segmentation fault at address 0x1A + SIGILL) happens mid-execution, not at shutdown, so it cannot be a teardown-finalizer issue. The macOS A C++ exception occurred at shutdown is too unspecific to attribute without a stack; I have no evidence this PR changes it.

[coderabbitai]

Walkthrough

napi_add_finalizer now always registers teardown cleanup, napi_create_external adds a teardown cleanup helper, NapiExternal stores the bound cleanup handle, and NapiEnv::cleanup() repeats passes for newly added finalizers. A native addon and Bun test suite cover teardown, GC, nesting, and ref recycling cases.

Changes

NAPI teardown finalizer correctness

Layer / File(s)	Summary
NapiExternal m_boundCleanup field and destructor deactivation src/jsc/bindings/napi_external.h, src/jsc/bindings/napi_external.cpp	Adds m_boundCleanup initialized to nullptr; the destructor deactivates and clears it before the existing finalizer call flow.
napi_add_finalizer and napi_create_external teardown registration src/jsc/bindings/napi.cpp	napi_add_finalizer always creates a NapiRef and registers wrap_cleanup through env->addFinalizer. napi_create_external adds external_cleanup, registers it with env->addFinalizer, stores the returned bound cleanup on the external, and returns the JS value directly from the Bun::NapiExternal*.
NapiEnv cleanup pass behavior src/jsc/bindings/napi.h	NapiEnv::cleanup() now repeats reverse passes until no new active finalizers run, and addFinalizer marks returned entries active while BoundFinalizer::call is removed.
Native addon for teardown finalization test/napi/napi-app/binding.gyp, test/napi/napi-app/test_teardown_finalizers.c	Adds the test_teardown_finalizers build target and a C addon exporting wrap, addFinalizer, createExternal, nesting, and recycling helpers for teardown finalizer scenarios.
Bun teardown finalizer test suite test/napi/napi-teardown-finalizers.test.ts	Adds build setup, fixture execution, and four skipped tests covering teardown finalizers, GC separation, nesting during teardown, and ref recycling behavior.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: teardown finalizers for napi_add_finalizer and napi_create_external.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description explains the change and includes verification details, even though it uses custom headings instead of the template.

---

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

[coderabbitai]

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-teardown-finalizers.test.ts`:
- Around line 16-18: The rebuild guard in the native fixture setup is too weak
because the `existsSync(addonPath)` check in the teardown finalizers test only
skips when the `.node` file exists. Update the fixture build logic in
`napi-teardown-finalizers.test.ts` to compare the addon binary against its
source inputs, such as `test_teardown_finalizers.c` and `binding.gyp`, or
otherwise force a rebuild in this hook so local reruns don’t reuse stale native
code. Keep the fix centered around the fixture setup that resolves `addonPath`
and triggers `bun install`, and ensure the gating reflects source changes rather
than file existence alone.
- Around line 81-95: The current test in runFixture only verifies the final set
of finalized names, so it does not prove that gc_add_finalizer and gc_external
actually ran during Bun.gc(true) rather than at process exit. Add a clear
barrier in the fixture after the forced garbage collections and assert the gc_*
finalizer output appears before that barrier, while keeping the existing
exit-time checks for exit_add_finalizer and exit_external. Use the existing
runFixture, Bun.gc(true), and finalized assertions to ensure the test validates
GC-time finalization specifically, not just that finalizers were invoked once.

🪄 Autofix (Beta)

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: 826b55e0-0538-44f4-bd3b-ad6316b7f9ee

📥 Commits

Reviewing files that changed from the base of the PR and between 8706328 and 9366d31.

📒 Files selected for processing (6)

• src/jsc/bindings/napi.cpp
• src/jsc/bindings/napi_external.cpp
• src/jsc/bindings/napi_external.h
• test/napi/napi-app/binding.gyp
• test/napi/napi-app/test_teardown_finalizers.c
• test/napi/napi-teardown-finalizers.test.ts

[claude]

The drain-until-quiescent loop in cd76b3d (plus the wrapNesting test) addresses the UAF I flagged — every appended entry now runs its *_cleanup and nulls its owner's back-pointer before m_finalizers.clear(). Still deferring to a human given this reworks NAPI finalizer lifetime across GC and env teardown.

Extended reasoning...

Overview

This PR makes napi_add_finalizer and napi_create_external register their finalizers on the NapiEnv::m_finalizers teardown list (previously only napi_wrap did), so finalizers for objects still alive at exit run during env teardown as Node-API guarantees. It touches src/jsc/bindings/napi.{cpp,h} and napi_external.{cpp,h}, adds a C test addon, and a three-case test suite.

Since my previous review, commit cd76b3d changed NapiEnv::cleanup() from a single reverse pass + clear() to a do { … } while (ranAny) loop that re-scans until no active entries remain, and added a wrapNesting fixture/test that registers new finalizers from inside a teardown finalizer. I traced the new loop: each entry is marked active = false before its callback runs, the callback (wrap_cleanup / external_cleanup) nulls the owning NapiRef::boundCleanup / NapiExternal::m_boundCleanup, and newly appended entries are picked up on the next pass — so by the time m_finalizers.clear() frees the nodes, no live cell holds a pointer into them. That resolves the dangling-boundCleanup UAF I raised.

Security risks

No auth/crypto/input-parsing surface. The risk class here is memory safety (UAF / double-free) in native finalizer lifetime management, reachable from any addon. The specific hazard I identified is now mitigated; the new test exercises it and the ASAN CI failure from the first commit should be re-checked on this revision.

Level of scrutiny

High. This is C++ GC-heap / destructor-ordering code on a hot Node-API path used by every native addon, with subtle interactions between ListHashSet iterator stability, DeferGCForAWhile, weak-handle owners, and worker teardown. Mistakes here manifest as rare crashes in third-party addons. A maintainer familiar with Bun's NAPI ref/finalizer model should sign off.

Other factors

• napi_add_finalizer(result == NULL) now allocates a NapiRef per call instead of the cheaper vm.heap.addFinalizer path; that's a deliberate cost-for-correctness trade the author should confirm is acceptable.
• The two CodeRabbit test-quality nits and my prior inline comment are all addressed in cd76b3d.
• CI on the first commit showed the new test failing on Windows (CRLF, fixed) and on x64-asan; the asan result on the new commit is the thing I'd want a human to confirm is green before merge.

[robobun]

Thanks for the careful trace of the drain loop, and especially for calling out that the x64-asan result needed to be confirmed rather than assumed. You were right: the x64-asan failure on the first commit was not the Windows CRLF bug, and it still reproduced on cd76b3d. I have fixed it in b2ebce5.

What it was. On the linux x64-asan lane, all three tests received a completely empty array, in about 120ms, meaning the spawned child produced no finalizer output at all. Two in-tree facts pin the cause. On that same lane, napi.test.ts's "napi_wrap finalizers run in LIFO order during env teardown" test passes, and it spawns an identical bunExe() -e teardown-finalizer child while asserting expect(stderr).toBe("") and reading its finalizer output from stdout. Since my tests failed rather than skipped, canBuildNodeAddons() was true there, so that test really ran. Together that proves that on x64-asan a spawned child's stderr arrives empty while stdout works. My addon printed every finalize: line to stderr, so the test saw nothing. The fix is to print to stdout, which is what the existing test_wrap_cleanup_order fixture already does, presumably for the same reason.

The failing assertions were also hiding this: they compared only the filtered name list, so CI reported an unreadable + []. Each test now asserts a single combined object carrying the exit code plus the child's raw stdout and stderr whenever no finalizer fired, so a lane-specific failure names its own cause instead of needing this kind of archaeology.

On the napi_add_finalizer(result == NULL) cost. That is a fair observation. The old vm.heap.addFinalizer path was cheaper precisely because it skipped the per-env bookkeeping, and that bookkeeping is exactly what the teardown contract requires: the env has to know the finalizer exists in order to run it at cleanup(). So the extra small allocation per registration is inherent to the correctness, not incidental. napi_add_finalizer is a per-object registration, not a hot per-call path, and it now shares the identical, better-tested code shape as napi_wrap.

[claude]

The drain-until-idle rewrite in cd76b3d looks like it correctly closes the UAF I flagged, and I didn't find anything new in b2ebce5 — but this is subtle NAPI finalizer/GC lifecycle code with a rewritten teardown loop, so it's worth a human pass before merging.

Extended reasoning...

Overview

This PR registers napi_add_finalizer and napi_create_external finalizers on the env's teardown list (previously only napi_wrap was), so they run at env teardown for objects still alive at exit. It touches src/jsc/bindings/napi.cpp (rewrites napi_add_finalizer to always allocate a NapiRef with a weak handle owner and adds an external_cleanup registration to napi_create_external), napi.h (rewrites the NapiEnv::cleanup() drain loop to repeat until a pass runs nothing new), napi_external.{h,cpp} (adds m_boundCleanup and deactivates it in the destructor), plus a new C addon and test suite.

Security risks

No injection/auth/data-exposure surface. The risk profile here is memory safety: finalizer double-invocation, use-after-free on BoundFinalizer nodes, and GC/teardown ordering. My earlier review found a real UAF (finalizers registered during teardown leaving dangling boundCleanup pointers after m_finalizers.clear()); the author confirmed it and fixed it in cd76b3d by looping the reverse pass until idle and pre-deactivating each entry. That fix looks correct to me, and external_cleanup now mirrors wrap_cleanup's deactivate-then-call shape.

Level of scrutiny

High. This is production-critical native lifecycle code where the failure modes are heap corruption and crashes at process/worker shutdown, the interactions (GC sweep vs. teardown drain vs. DeferGCForAWhile vs. weak-handle-owner self-delete) are subtle, and the drain loop in NapiEnv::cleanup() was rewritten mid-review. The napi_add_finalizer path also switched from vm.heap.addFinalizer to NapiRef + NapiRefSelfDeletingWeakHandleOwner, which changes the ownership/deletion model for the result == NULL case.

Other factors

The author addressed all three prior review threads (my UAF, the two CodeRabbit test-shape comments) with code changes and detailed reasoning, added a regression test for the nested-registration case, and reports ASAN clean. CI on the first commit had real failures that were diagnosed and fixed in the two follow-ups. Given that a memory-safety bug was already found and fixed during review and the teardown loop is newly rewritten, a human reviewer familiar with Bun's NAPI ref/weak-owner machinery should sign off.

[claude]

Thanks for the thorough fix to the drain loop and the regression test — the UAF concern looks addressed. I'd still like a human to sign off here since this reworks finalizer lifetime across GC and env teardown with new raw-pointer bookkeeping on a GC cell.

Extended reasoning...

Overview

This PR makes napi_add_finalizer and napi_create_external register their finalizers on the per-env m_finalizers teardown list (previously only napi_wrap did), so they run at env teardown for objects still reachable at exit. It also reworks NapiEnv::cleanup() to loop the reverse drain until quiescent so finalizers registered mid-teardown are reached, adds a m_boundCleanup raw pointer to NapiExternal that the destructor deactivates, and adds a new native test addon plus three Bun tests.

Security risks

No injection/auth/data-exposure surface. The risk class here is memory safety: a new raw const BoundFinalizer* is stored on a GC-managed cell and must stay valid across the GC-sweep ↔ env-teardown ordering. My earlier review flagged a UAF on that pointer when a finalizer registered during teardown was freed by m_finalizers.clear(); the author fixed it by draining until a pass runs nothing new, deactivating each entry before calling it, and added an ASAN-verified regression test. I don't see a remaining concrete hole, but the invariants (deactivate-before-call, m_isFinishingFinalizers routing removals to active=false, clear() only after a fully-inactive pass) are subtle.

Level of scrutiny

High. This is production NAPI lifecycle code in C++ that interacts with JSC's GC, weak handle owners, and destructible-object sweep ordering. napi_add_finalizer also changed shape (now always allocates a NapiRef and uses NapiRefSelfDeletingWeakHandleOwner for the no-ref variant instead of vm.heap.addFinalizer), which is a behavior-equivalent-but-different code path. None of this is mechanical.

Other factors

• My prior inline UAF comment was addressed in cd76b3d with a clear explanation, a regression test, and ASAN confirmation; CodeRabbit's two test-quality nits were also addressed.
• The current bug-hunting pass found nothing.
• Test coverage is good (teardown-only, GC-then-teardown with a barrier, and nested-registration-during-teardown), and existing test_wrap_lifetime_* / external-buffer suites are reported unchanged.
• Still, the combination of a new raw pointer on a GC cell, a modified drain loop, and a swapped weak-handle-owner path is exactly the kind of change that benefits from a maintainer familiar with Bun's NAPI/GC interaction taking a look.

[robobun]

Closing the loop on the x64-asan question, since it was the one open item. The lane is now explained, and it was not a teardown or memory-safety problem.

What the lane showed. After I made the assertions self-diagnosing in b2ebce5, the x64-asan failure finally carried its own cause: every spawned child exited 134 with

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: NAPICallFrame @ src/jsc/bindings/napi.h
    But the exception was unchecked as of this scope: napi_get_cb_info @ src/jsc/bindings/napi.cpp
ASSERTION FAILED: exception check validation failed

before a single finalizer ran. That is JSC's BUN_JSC_validateExceptionChecks validator. One debian x64-asan agent has it set in its environment, and bunEnv spreads process.env, so my subprocesses inherited it. napi.test.ts stayed green on that lane only because its shard landed on a clean agent.

It is not caused by this PR. A bun-debug built from main (df92f8fd6), with none of these changes, aborts identically on the simplest possible addon call (napi_get_cb_info then napi_wrap).

It also cannot be fixed one site at a time, which I verified rather than assumed. I added the exception check the first message asks for (discharging the NAPICallFrame constructor's scope in the dispatcher). The abort then moved to the next pair: napi_get_cb_info blaming napi_get_value_string_utf8. The underlying reason is structural: every napi_* function opens a ThrowScope, and ThrowScope::~ThrowScope unconditionally simulates a throw when it returns into a non-LLInt/JIT frame, which an addon's C frame always is. Node-API reports errors as napi_status, so there is no place for a JSC exception check between two napi calls made by addon C code. Any addon callback that makes two napi calls trips the validator by construction. I dropped that dispatcher change from the PR since it is moot and nothing can test it.

The fix (0da31b6) is to strip BUN_JSC_validateExceptionChecks from the child env, exactly the treatment harness.ts already gives JSC_useJIT for "ad-hoc JSC debug options that may be set on CI agents." I verified the suite passes with that option exported in the test runner's environment, which reproduces the failing lane precisely.

Two things a maintainer may want to act on separately: that agent should probably have the option removed from its environment, and bunEnv in harness.ts may want to strip BUN_JSC_validateExceptionChecks globally the way it strips JSC_useJIT, since any napi subprocess test that lands on that agent will abort. I kept this PR's change scoped to its own fixture rather than widening bunEnv for every test in the repo.

[coderabbitai]

Actionable comments posted: 4

🤖 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 `@src/jsc/bindings/napi.h`:
- Around line 210-215: Shorten the long inline comments in the affected Napi
finalizer logic so each comment stays within the 3-line limit while preserving
the key invariant. Update the comments near the boundCleanup/finalizer handling
in napi.h to be more concise, and trim the other matching comment block
referenced in the review so the explanation stays brief with extra detail moved
out of the source.

In `@test/napi/napi-app/test_teardown_finalizers.c`:
- Around line 2-8: Trim the long teardown comment block in
test_teardown_finalizers.c to the repo’s 3-line limit by keeping only the
durable invariant about finalizers running once, teardown draining newly
registered finalizers, and stdout being used; move CI/debug history and extra
failure mechanics out of the comment. Apply the same reduction to the related
comment block noted in the review, preserving the intent around
teardown_finalizers and any nearby fixture names if needed for context.

In `@test/napi/napi-teardown-finalizers.test.ts`:
- Around line 11-14: Shorten the newly added explanatory comments in the napi
teardown finalizers test to satisfy the repo’s 3-line comment limit. Keep only
the essential invariant next to the relevant assertions in the test file (around
the gc_* finalizers/barrier logic in the test blocks), and move the extra
CI/bug-history context out of the code comments. Apply the same cleanup to the
other highlighted comment blocks so each comment stays brief and directly tied
to the local code.
- Line 94: Remove the explicit per-test timeout arguments from the affected
bun:test cases in napi-teardown-finalizers.test.ts and let bun:test use its
default timeout instead. Update the test declarations around the finalizer and
recycling cases so they no longer pass 30_000 (and any matching timeout in the
other referenced tests), keeping the existing test names and bodies unchanged.

🪄 Autofix (Beta)

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: 1fe0080d-2e52-40d1-a8f1-9e6b2ffd15d2

📥 Commits

Reviewing files that changed from the base of the PR and between 9366d31 and 5dc3935.

📒 Files selected for processing (4)

• src/jsc/bindings/napi.cpp
• src/jsc/bindings/napi.h
• test/napi/napi-app/test_teardown_finalizers.c
• test/napi/napi-teardown-finalizers.test.ts

[claude]

All my earlier findings are addressed and this round found nothing new, but the change reworks finalizer lifecycle and the cleanup() drain in memory-safety-critical NAPI bindings, so it's worth a maintainer's pass before merge.

Extended reasoning...

Overview

The PR makes napi_add_finalizer and napi_create_external register their finalizers on the per-env teardown list so they run at env teardown (Node parity). Concretely: napi_add_finalizer now always allocates a NapiRef and registers wrap_cleanup (both the ref-returning and result == NULL variants, the latter via NapiRefSelfDeletingWeakHandleOwner); napi_create_external registers a new external_cleanup and stores the bound entry on a new NapiExternal::m_boundCleanup field whose destructor deactivates it. NapiEnv::cleanup() is rewritten from a single reverse pass into a do-while drain that repeats until no active entries remain, and addFinalizer() reactivates the entry it returns to handle tombstone aliasing under TZone address reuse. BoundFinalizer::call() is removed as dead. A new C addon and four-test suite cover teardown, GC-vs-teardown exclusivity, nested registration during teardown, and the delete-then-reregister recycling path.

Security risks

No injection, auth, or data-exposure surface. The risk class is memory safety: dangling BoundFinalizer* back-pointers across m_finalizers.clear(), double-invocation of user finalizers, and iterator invalidation under reentrant registration. Two real UAFs in this class were found in earlier review rounds and fixed (the single-pass-then-clear UAF in cd76b3d, and the ListHashSet dedup-to-tombstone aliasing in bc35f2c). The current revision looks correct on those axes and the bug hunter found nothing further.

Level of scrutiny

High. This is core NAPI bindings C++ that every native addon goes through, with bidirectional raw pointers between GC-managed cells (NapiExternal), TZone-allocated objects (NapiRef), and ListHashSet node storage, across both GC-time and env-teardown orderings. The napi_add_finalizer(result == NULL) path also changes ownership model (heap finalizer → self-deleting weak ref). These are exactly the interactions a maintainer familiar with Bun's NAPI/JSC lifetime model should sign off on.

Other factors

All prior inline comments (mine and CodeRabbit's) are resolved; the most recent commit only trimmed comments and dropped per-test timeouts. Test coverage is good and the PR description carefully scopes out napi_create_external_arraybuffer/_buffer as a separate change. CI shows unrelated build-rust failures on several lanes. Given the complexity, the two UAFs already found and fixed during review, and the runtime-wide blast radius, deferring to a human is the right call rather than auto-approving.

[robobun]

CI status on the final head (6896241, build 66156): the diff is green; every red lane is unrelated to this PR, and large parts of the build never ran because Buildkite did not assign them agents.

Of 53 non-passing jobs, only 3 actually executed and failed, and none involves this PR's test or code:

• test/js/bun/util/v8-heap-snapshot.test.ts, SIGKILL ("main process killed by SIGKILL but no core file found") on one Ubuntu 25.04 x64 runner. That is the OOM killer taking out an inherently memory-heavy heap-snapshot test on a shared runner. The file is untouched here, loads no addon, and nothing in this PR changes its memory profile.
• test/js/node/test/parallel/test-net-write-slow.js, ERR_STREAM_PUSH_AFTER_EOF inside node:net readable internals on the Darwin aarch64 shards. node:net backpressure has no relationship to NAPI finalizer teardown.
• The other 50 are 8 jobs that expired (Buildkite never assigned them an agent) plus the 42 downstream jobs those starved into waiting_failed. The same agent starvation killed build 65875 outright earlier, which is where this branch's one ci: retrigger went.

The signal that matters: test/napi/napi-teardown-finalizers.test.ts appears in no failure annotation on any lane, and the debian x64-asan lane, which was the hard one for this PR, ran 17 of its 20 shards and is green for this suite.

I am deliberately not pushing further empty retrigger commits. If a maintainer re-runs the expired jobs or re-triggers the build, I would expect it to come back with only the usual unrelated flakes. Local verification is in the description: all 4 tests fail on an unfixed build and pass under the debug ASAN build.

[claude]

All of my earlier findings look addressed and I didn't find anything new on this revision, but this rewrites finalizer lifecycle in GC-adjacent native NAPI code and went through several UAF iterations, so it should get a human sign-off.

Extended reasoning...

Overview

This PR changes napi_add_finalizer and napi_create_external to register their finalizers on the per-env teardown list (NapiEnv::m_finalizers) so they run at env teardown for objects still alive at exit, matching Node's RefTracker::FinalizeAll contract. To make that safe it also rewrites NapiEnv::cleanup() from a single reverse pass into a repeat-until-idle drain, adds an m_boundCleanup back-pointer to NapiExternal (deactivated in its destructor), makes addFinalizer() reactivate any tombstoned entry it hands back, and removes the now-dead BoundFinalizer::call(). A new C addon and four subprocess tests cover teardown, GC-then-teardown de-duplication, mid-teardown nesting, and the delete-then-reregister recycling case.

Security risks

No injection/auth/data-exposure surface. The risk here is memory safety: the change threads raw pointers into ListHashSet nodes (m_boundCleanup / boundCleanup) across GC and env-teardown ordering, and the drain loop's correctness depends on ListHashSet iterator stability under append plus the tombstone/reactivate invariant. Two UAF-class bugs were found and fixed during review (mid-drain append never reached before clear(); TZone address reuse aliasing a tombstoned entry). I did not find a residual instance on the current head, and the bug-hunting pass came back clean.

Level of scrutiny

High. This is production C++ in src/jsc/bindings/ that changes GC/finalizer interaction and object-lifetime ownership across the JS/native boundary — exactly the category the repo's own review guidance calls the most-blocked. It also changes user-visible behavior for every native addon that uses napi_add_finalizer or napi_create_external (more finalizers now run at exit/worker teardown), which the PR description itself notes may make latent addon bugs easier to hit.

Other factors

Every inline comment I raised earlier (mid-drain UAF, tombstone-aliasing UAF, dead call(), stale comment) is resolved in the current diff, and the CodeRabbit nits (comment length, per-test timeouts, rebuild gating, GC barrier) are also addressed. Test coverage is good and self-diagnosing. That said, this is not a mechanical or obvious change — it took multiple rounds to converge on a memory-safe drain design, and the addFinalizer reactivation fix rests on an argument about TZone address reuse that a maintainer who owns this code should validate. Deferring rather than approving.

[robobun]

CI on the rebased head (6ed3422, build 70437) has finished: 284 jobs passed, and the two failures are both unrelated macOS integration/infra issues.

• test/js/bun/s3/s3.test.ts on Darwin 14 x64: Cannot connect to the Docker daemon at unix:///var/run/docker.sock (MinIO container failed to start; Docker not running on that runner).
• test/integration/next-pages/test/dev-server.test.ts on Darwin 14 aarch64: the Next.js dev-server + puppeteer integration test, which downloads Chrome and drives a browser. Unrelated to napi.

test/napi/napi-teardown-finalizers.test.ts appears in no failure annotation. Every debian x64-asan test shard passed. Binary size is +0.0 KB on every platform. I am deliberately not pushing another empty retrigger commit; this branch already carries one. Ready for maintainer review.