Skip to content

fix(napi): finalize Worker resources with Node string lifetimes - #152

Merged
steipete merged 4 commits into
mainfrom
claude/napi-worker-finalizers
Oct 9, 2026
Merged

steipete merged 4 commits into
mainfrom
claude/napi-worker-finalizers

Conversation

@steipete

@steipete steipete commented Oct 9, 2026 •

Copy link
Copy Markdown

Workers could finish terminate() or process.exit() with external Latin-1 strings, external UTF-16 strings and napi_create_external values 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_finalizer forms 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_finalizer forms. 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-thread process.exit() remains 0/8, matching Node.

Node references: node_napi_env__::DeleteMe, TrackedStringResource, Finalizer::CallFinalizer, and the documented nullable env. Natural main exit returns through NodeMainInstance::Run and FreeEnvironment, 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):

  • Full test/napi/napi.test.ts and test/napi/napi-finalizer-delete-ref.test.ts runs 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.
  • Exact-head native Linux/macOS CI, Windows x64/ARM64 builds, smoke and compatibility, and Rust checks are green. Publishing is disabled. The dispatched native/Windows workflows use fixed compatibility selections; the separate full N-API runs above establish N-API coverage.
  • P2 review of the rebased branch is scoped-clean. Node 24 and Node 26 each pass all 36 strengthened retained/transport cases from the prior qualification, which exercises the identical task source.
  • The earlier PR-selected Linux run exposed an existing duplicate-hook deliberate-crash timeout. Its helper now drains both pipes, disposes children, and uses the existing POSIX no-core wrapper. Both full N-API suites pass at this head with the original crash assertions and timeout.

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.

@steipete steipete closed this Oct 9, 2026
@steipete steipete reopened this Oct 9, 2026
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
steipete force-pushed the claude/napi-worker-finalizers branch from 226849b to 60bb7a8 Compare October 9, 2026 18:32
@autofix-troubleshooter

Copy link
Copy Markdown

Hi! I'm the autofix logoautofix.ci troubleshooter bot.

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
steipete marked this pull request as ready for review October 9, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants