ReviewOS

home-lang/home

fix(runtime): complete napi_wrap reference deletion at finalization

#573
Closed chris opened this 9 days ago ·
9 days ago

Follow-up to #459 and #66 from the pinned Bun Node-API corpus at 4982b91e3702094330f3be3883354c52b8c01323.

Exact failing test:

  • napi > napi_wrap > has the right lifetime

The native fixture prints finalize_for_create_wrap, data = 42, hint = 123, but Home omits the subsequent delete_the_ref observed under Node. Other napi_wrap coverage, including removal, accepted values, proxy/prototype handling, shared addons, and LIFO teardown, is green.

Acceptance:

  • Preserve the wrapped reference until its documented finalization point.
  • Run the finalizer and reference deletion exactly once in Node-compatible order.
  • Add a focused lifecycle regression covering GC and environment teardown.
  • Re-run the pinned napi/napi.test.ts corpus file.
9 days ago

Resolved by 436b7b574439dbc19c4d6a7d7d8f59facf7522dc (fix(runtime): complete Node-API finalizer lifecycle).

node_api_post_finalizer was incorrectly using the common Node-API preamble, which rejects calls once the environment can no longer call JavaScript. That prevented a wrap finalizer from posting the basic-environment callback that deletes its reference during environment teardown. The implementation now follows Node's node_api_basic_env semantics: validate the environment and callback without the GC, pending-exception, or can-call-JS checks, enqueue the finalizer, and clear the last error.

The runtime regression now covers compiled and dynamically loaded Node-API modules on the main thread and workers. It verifies WRAP_FINALIZED and POSTED_FINALIZER, including collection through zero-argument global.gc() before execution continues.

Verification:

  • Exact Bun wrap cleanup fixture prints both finalize_for_create_wrap and delete_the_ref.
  • Full pinned Bun N-API file: 99 passed, 0 failed, 0 unsupported, 0 todo.
  • Runtime aggregate: 57 passed, 0 failed.
  • Native runtime units: 1,816 passed, 19 skipped, 0 failed.

Sign in to comment on this issue.