Repository navigation
fix(napi): finalize Worker resources with Node string lifetimes - #152
Merged
Merged
Conversation
Register external values and added finalizers for environment cleanup. Keep external strings storage-owned with nullable environments after cleanup, and copy only addon-owned external strings before cross-thread sharing. Adapts the environment-registration approach from oven-sh#32912 by @robobun. Covers both Worker exit routes, thread affinity, fd ownership, escaped strings, main exit boundaries, and copy cost. Natural main-exit strings remain tied to the retained main heap.
Use callbacks that return normally so a successful napi_call_function increments the reentry counter. A throwing callback could disguise reentry as napi_pending_exception.
Drain piped output concurrently with process exit, dispose the child, and use the existing POSIX no-core wrapper. Preserve Windows direct execution, exit/signal assertions, and the test timeout.
steipete
force-pushed
the
claude/napi-worker-finalizers
branch
from
October 9, 2026 18:32
226849b to
60bb7a8
Compare
|
Hi! I'm the It looks like you correctly set up a CI job that uses the autofix.ci GitHub Action, but the autofix.ci GitHub App has not been installed for this repository. This means that autofix.ci unfortunately does not have the permissions to fix this pull request. If you are the repository owner, please install the app and then restart the CI workflow! 😃 |
steipete
marked this pull request as ready for review
October 9, 2026 19:09
This was referenced Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workers could finish
terminate()orprocess.exit()with external Latin-1 strings, external UTF-16 strings andnapi_create_externalvalues still unfinalized. A retained-resource addon observed 5/8 callbacks and three open fds. Long external strings sent to another Worker also shared the addon's backing storage and could finalize later on the receiving thread.This binds external values and both
napi_add_finalizerforms to environment cleanup. External strings remain storage-owned: cleanup clears their env, and their storage destructor invokes the callback with a null env if cleanup already ran. Only N-API-created external storage is copied by the common cross-thread string helper. Structured-clone fast paths, MessagePort, BroadcastChannel and object-URL filenames use that helper; existing isolation for Worker options remains intact. Ordinary strings and unrelated external storage retain their existing sharing behavior.The eight-resource probe now observes 8/8 callbacks, the creating thread, rejected JS reentry and zero owned fds on both Worker exit routes. The expanded fixture also covers both
napi_add_finalizerforms. The two escaped strings copy 4,096 bytes each; received copies and unrelated long strings add zero copied bytes. Main natural exit now runs the env-tracked finalizers (6/8 original resources); its two retained strings remain owned by the main heap, whose destruction is deliberately unchanged. Explicit main-threadprocess.exit()remains 0/8, matching Node.Node references:
node_napi_env__::DeleteMe,TrackedStringResource,Finalizer::CallFinalizer, and the documented nullable env. Natural main exit returns throughNodeMainInstance::RunandFreeEnvironment, which disables JS entry and runs cleanup.The environment-registration approach adapts oven-sh/bun#32912 by @robobun, closed as stale; no complete upstream external-string ownership fix was found.
Validation at
60bb7a8b2116a41af6410b15a034798468f079a5(rebased onto main after #151):test/napi/napi.test.tsandtest/napi/napi-finalizer-delete-ref.test.tsruns pass 262/262 tests, 1,120 assertions each on Linux and macOS. These include both Worker exit routes, escaped-string ownership/copy checks, and main natural/explicit exit.LSan delta versus the no-addon control = 0. In the calibrated Linux run, the 100-Worker retained-addon arm and the no-addon 100-Worker control each report 38,928 bytes / 1,118 allocations. A verification-only EventNames-cache cleanup diagnostic moves both arms identically to 12,028 bytes / 418 allocations. This is a pre-existing Worker cache leak tracked separately; the diagnostic is not in this PR. The leak gate for this change is no introduced leak, and that delta criterion is satisfied.
The verification-only relink exposes the sanitizer ABI and allocator interception to the instrumented addon; production linker files are unchanged. Deliberate native leak and use-after-free controls are detected. All 39 stress cases pass their functional assertions across 534 resource-owning Workers, with no ASAN memory-access report. LSan's exit 23 is retained in the evidence and is not described as a clean absolute leak report. Only the existing console-initialization suppression was reused; no N-API suppression was used or added. The rebased implementation and tests are byte-identical to the previously qualified head; only main's adjacent changelog entry and VM test change were incorporated.
Main natural-exit external strings retain the documented remaining divergence described above. This PR is ready for review and remains unmerged.