ReviewOS

home-lang/home

Bun parity: replace no-op Node-API environment bridges and verify native FFI

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

Follow-up to #66 section E. Reproduced exact native cc.test.ts: 7 pass, 10 explicit upstream skips, 1 failure. cc-fixture.js crashes at JSGlobalObject::vm when its compiled C napi_main calls napi_create_string_utf8. Root cause: native_skip_paths excludes napi.cpp while native/napi_weak_home_dups.cpp supplies NapiEnv__globalObject returning nullptr and no-op lifetime, error, cleanup, finalizer, and module-registration bridges. The Zig N-API layer calls those bridges. Work: compile Home-owned napi.cpp with ABI-matched headers, remove no-op bridges, verify complete C FFI fixture, N-API value/instance-data behavior and lifecycle regressions. Native compilation/callback/GC-liveness cases already pass and must remain green. Skips are not passing coverage. Parent #66.

9 days ago

Progress on #66: Home now compiles its own napi.cpp instead of no-op NapiEnv bridges. The exact cc.test.ts runs natively with 8 pass, 10 upstream skips, 0 fail; modern/legacy addon loading, per-library environments, error state, strings, GC, and concurrent embedded-header initialization regressions also pass. Review additionally reproduced a real use-after-unmap at VM exit after cc().close() with a registered C finalizer. Fixing executable-code lifetime and reduced-adapter ABI safety before committing; no full NAPI parity claim yet.

9 days ago

Committed locally as Chris, without coauthors: fc807a4 (refs #66 and #460). Home-owned napi.cpp replaces the weak environment stubs; cc ships atomic content-addressed Node-API headers. Unsafe reduced-context addon loading now reports unsupported before constructors or fabricated exports; NAPI/FFI test files route natively. Confirmed and fixed close-before-finalizer crashes by retaining executable code through environment and final heap cleanup, including main-thread termination signaling. All 12 guarded native regression files pass, including cc/dlopen main+worker finalizers and concurrent header initialization. Exact four-file FFI shard: 14 pass, 11 upstream/platform skips, 0 fail; cc.test.ts alone: 8 pass, 10 skips. Build and cached rebuild 11/11; generator/ABI 5/5; Zig binding helpers 3/3. Full NAPI coverage remains open; git push is blocked by github.com DNS resolution.

9 days ago

Post-#465/#468 audit found an additional concrete lifetime gap in the pinned napi_async_work implementation (Home currently matches Bun source here):

  • each work object retains raw JSGlobalObject, EventLoop, and cloned NapiEnv ownership;
  • schedule() posts directly to the shared work pool without per-VM admission;
  • the execute callback receives the environment on a worker thread, then publishes an owner-thread completion through the raw event loop;
  • explicit Worker teardown can therefore destroy the VM/environment without joining an executing or queued addon callback;
  • a queued completion cannot simply be discarded or the work blindly freed: the addon completion callback may call napi_delete_async_work and is the only owner-aware place that can release opaque addon data, but it may also call into JavaScript.

Node v24's official implementation confirms the ownership contract: Work::AfterThreadPoolWork() enters the module callback and warns that the complete callback likely deletes the work; napi_delete_async_work() directly deletes the Work. Home needs an equivalent shutdown-aware protocol rather than treating this as an ordinary runtime-owned AnyTask.

Required work for this issue now includes:

  • close per-VM async-work admission and join every admitted execute callback before N-API/JSC teardown;
  • define exact ownership for pending, started, completed, explicitly cancelled, deleted, and shutdown-cancelled work;
  • coordinate with environment cleanup hooks so opaque addon data and cloned NapiEnv refs are not leaked or double-freed;
  • prevent completion callbacks from entering stopped JavaScript while preserving normal and explicit napi_cancel_async_work behavior;
  • add a self-contained native-addon regression that saturates the work pool, terminates a Worker with queued and started async work, releases the blockers, and checks exact execute/complete/delete/environment-cleanup counts across repetitions;
  • compare observable completion/cancellation behavior with pinned Bun and current Node where their teardown models differ.

This is a source-audited gap, not a reproduced crash and not evidence that all N-API work is currently unsafe. The one-shot runtime-owned producer protocol landed for DNS, crypto, filesystem tasks, archives/transpilation, and native node:zlib; poll-parked Blob I/O is tracked separately in #569.

9 days ago

Pushed a7fe8d888443fca9f399a775871d4fc0a843c17a (fix(runtime): drain Node-API async work on shutdown) to main.

This closes the source-audited napi_async_work shutdown race:

  • Async work now participates in the per-VM native-work admission barrier.
  • Teardown closes admission and waits for every admitted execute callback to publish its owner-thread completion before JSC/event-loop destruction.
  • Queued Node-API completions have an explicit shutdown cancellation path and release their keepalive exactly once.
  • Native completion callbacks still run during shutdown, matching Node, while JavaScript-capable Node-API calls report the closing environment (napi_pending_exception before N-API 10, napi_cannot_run_js from N-API 10).
  • A self-contained native addon regression saturates every work-pool lane, queues an additional work item, terminates its Worker, and verifies exact execute/complete counts, no JavaScript completion, zero remaining native jobs, and exact shutdown-task ownership.

Direct Node and Home probes now agree: [executed=1, completed=1, completion_status=napi_ok, first_JS_capable_call=napi_pending_exception].

Verification:

  • zig build debug -j1
  • focused async-work shutdown regression
  • all four prior native Node-API regression programs
  • runtime aggregate: 57/57
  • home_rt: 1,816 passed, 19 skipped, 0 failed
  • Pickier on the new regression and git diff --check

The pinned Bun napi/napi.test.ts file now builds with C++20 and reaches 96/99 passing. This umbrella stays open for the two real remaining runtime gaps, split into #572 and #573. The third failure is a 5-second Node-target bundle timeout (the equivalent bundle assertions pass through the other invocation paths), which still needs an isolated timing/runner audit.

9 days ago

Resolved by the Node-API bridge/lifetime series through 436b7b574439dbc19c4d6a7d7d8f59facf7522dc.

The current exact, byte-identical js/bun/ffi/cc.test.ts result in Home is:

  • 8 passed
  • 0 failed
  • 10 upstream-declared skips

The exact pinned Bun 4982b91e3702094330f3be3883354c52b8c01323 control is 7 passed, 1 failed, 10 skipped. Its failure is the original cc-fixture.js case: napi_main is missing from cc-fixture.c. Home now passes that case and preserves the same ten upstream skip declarations.

The broader Node-API implementation is also verified by the pinned napi/napi.test.ts file at 99 passed, 0 failed, 0 unsupported, 0 todo, including async-work shutdown, experimental-finalizer safety, wrap reference deletion, cleanup hooks, FFI registration, main-thread and Worker teardown.

Current checkpoint gates:

  • runtime aggregate: 57 passed, 0 failed
  • native runtime units: 1,816 passed, 19 skipped, 0 failed

The ten cc.test.ts skips are present in pinned Bun itself and are not remaining Home-only failures in this issue's environment-bridge scope.

Sign in to comment on this issue.