Implement hermes_napi_host for async work and thread-safe functions

[kraenhansen]

Provides a complete hermes_napi_host implementation to enable async work (napi_queue_async_work, napi_cancel_async_work) and thread-safe functions (napi_create_threadsafe_function and related APIs) in Hermes Node-API environments.

Key Changes

• New HermesNapiHost implementation (packages/host/cpp/HermesNapiHost.{hpp,cpp}):

  • Implements the hermes_napi_host struct required by Hermes' Node-API integration
  • Provides post_work callback for queuing async work on a shared worker thread pool
  • Provides cancel_work callback for cancelling queued work items
  • Provides post_task callback for dispatching thread-safe function calls to the JS thread
  • Includes a process-global WorkerPool with 4 worker threads (matching libuv's default)
  • Handles runtime teardown gracefully by tracking contexts with weak pointers

• Removed legacy async implementation (RuntimeNodeApiAsync.{hpp,cpp}):

  • Replaced with the new host-based approach that integrates directly with Hermes
  • Eliminates CallInvoker dependency for async work

• Comprehensive test coverage:

  • Added test_hermes_napi_host.cpp with Catch2 tests exercising work posting, cancellation, and task dispatch
  • Added threadsafe-function addon tests (packages/node-addon-examples/tests/threadsafe-function/) ported from Node.js test suite
  • Extended async work tests to verify execute runs off the JS thread and complete runs on it

• Integration updates:

  • Updated CxxNodeApiHostModule to instantiate and pass HermesNapiHost to Hermes
  • Updated Android CMakeLists.txt to build the new implementation
  • Added C++ test build configuration and CI workflow

Implementation Details

The worker pool uses a shared queue protected by a mutex and condition variable. Work items track their originating context via weak pointer to handle runtime reloads. Cancellation is atomic: an item is either removed from the queue (cancelled) or already executing (cannot be cancelled), with exactly one completion callback guaranteed in either case. Task dispatch for thread-safe functions posts to the JS dispatcher without blocking, enabling non-blocking queue modes.

https://claude.ai/code/session_01Y6xHHeMFF853z5R6th5CkF

[kraenhansen]

Read this against the Hermes sources at the pinned commit (facebook/hermes@5a795c9, API/napi/hermes_napi.h, hermes_napi_async_work.cpp, hermes_napi_tsfn.cpp, hermes_napi.cpp) and against the RuntimeNodeApiAsync.cpp it replaces. The direction is right and the changeset's claim holds up — the old implementation really did run execute on the JS thread via invokeAsync, so this is a genuine fix, not a refactor.

What I verified upstream (so it doesn't have to be re-derived):

• The mirrored hermes_napi_host struct matches API/napi/hermes_napi.h at the pin exactly — member order, signatures, and the uv_loop_s forward declaration. The re-diff instruction added to hermes.ts is the right guard for it.
• napi_queue_async_work / napi_cancel_async_work null-check only env->host_, never the individual function pointers — so the comment on host_'s initialiser is correct that post_work and cancel_work can't be left null. (napi_create_threadsafe_function does additionally check post_task.)
• napi_env__::releaseTsfnLoopRef() dereferences host_ unconditionally to read unref_loop, and shutdown() calls hermes_napi_cleanup_tsfns after the cleanup hooks — so the "no cleanup hook can tell us when the last env is done with the struct" reasoning behind retainForProcessLifetime is sound.
• tsfnDispatch's dispatch_pending protocol does require that a post_task is never dropped while the env lives; postTask's comment is right to call that out.
• kThreadCount = 4 is genuinely load-bearing for tests/async's TestCancel, which saturates with MAX_CANCEL_THREADS - 1 == 5 blocking jobs. The "keep this below 5" comment is accurate.

The main thing to fix. retainForProcessLifetime holds a strong shared_ptr in a process-lifetime static, so the std::weak_ptr<HostContext> in WorkItem can never expire. Every teardown path keyed off it is dead code, and in cancelWork that turns into observable behaviour: napi_cancel_async_work returns napi_ok while the completion is dropped inside the dispatcher a moment later, so the addon is told the cancel succeeded and never receives complete(work, napi_cancelled). Details inline, along with why the test that covers this can't catch it.

Two others worth acting on before merge: the double-queue warning path in WorkerPool::enqueue currently enqueues the duplicate anyway (which breaks the exactly-one-completion invariant and lands on a use-after-free), and the weak_ptr<CallInvoker>-as-runtime-liveness assumption in CxxNodeApiHostModule deserves an explicit justification since no test can reach it.

The rest — fatal_exception aborting, the blanket test timeout, deleting HostContext's copy/move — are smaller and can go either way.

Test coverage is otherwise good: porting the upstream tsfn suite with an honest note about what didn't port, and adding the off-JS-thread and cancel-while-running assertions to the async suite, are exactly the right supplements.

---

Generated by Claude Code

[kraenhansen]

This weak_ptr can never expire in production, so both of the drop paths it guards are dead code.

CxxNodeApiHostModule's constructor calls HostContext::retainForProcessLifetime(hostContext_), which pushes a strong shared_ptr into a process-lifetime static that nothing ever erases. So item.context.lock() succeeds for every context ever created, forever:

• workerMain()'s else branch ("dropping an async work completion posted after runtime teardown") is unreachable.
• cancelWork()'s "the runtime is being torn down" fallback is unreachable — and that one has a user-visible consequence, see the separate comment there.
• The only test covering these paths (test_hermes_napi_host.cpp, "work completing after its context died") gets there by dropping the last strong ref, which production never does.

The object whose liveness actually matters is the CallInvoker, and that check lives one level down, inside the JsDispatcher lambda — where the pool can't see its result. Either drop the weak indirection entirely (hold the context strongly; the dispatcher already handles teardown) or give HostContext a predicate/return value the pool can consult before it commits to an outcome.

---

Generated by Claude Code

[kraenhansen]

Following on from the WorkItem::context comment: this is where the unreachable branch bites. Because the context is retained for the process lifetime, the teardown sequence is:

1. tryRemove succeeds — the item is off the queue.
2. item.context.lock() succeeds (strong ref held forever).
3. dispatchToJs → the CxxNodeApiHostModule lambda's weakInvoker.lock() fails → log_warning, function dropped.
4. cancelWork has already returned true, so Hermes' napi_cancel_async_work returns napi_ok (hermes_napi_async_work.cpp:121-125).

The addon is told the cancellation succeeded and then never gets complete(work, napi_cancelled) — so its napi_delete_async_work never runs and whatever the work was backing never settles. That's precisely the outcome the comment on line 214-216 says it wants to avoid; the check is just on the wrong object.

Making HostContext::dispatchToJs return bool (i.e. "the dispatcher accepted this") and returning that from cancelWork would make the documented behaviour real, and would also let workerMain log the drop accurately.

---

Generated by Claude Code

[kraenhansen]

Detecting the double-queue and then enqueueing it anyway is the worst of the three options. With two entries for the same (loopData, workData):

• tryRemove erases the first match and posts complete(work, napi_cancelled), while the second copy still runs execute and posts complete(work, napi_ok). That's two completions for one napi_async_work — directly contradicting the "never both" invariant asserted in workerMain (lines 92-94) and the "exactly one completion callback guaranteed" claim in the PR description.
• Node addons overwhelmingly call napi_delete_async_work from the completion callback, so the second completion is a use-after-free on a napi_async_work__ Hermes has already deleted.

libuv aborts on this; warning and continuing converts an assert into a UAF. continue/return before the push_back — log it and drop the duplicate — preserves the one-completion invariant and is strictly safer than either extreme.

Separately, and independent of the above: this is a linear scan of the entire process-global queue, under the process-global mutex, on every napi_queue_async_work in the app. If the warning stays, consider guarding the scan with #ifndef NDEBUG so release builds don't pay for a diagnostic.

---

Generated by Claude Code

[kraenhansen]

This is the load-bearing assumption of the whole change, and it's the one thing the C++ tests can't reach (they use FakeJsQueue), so it's worth writing down why it holds.

The direction this guards is the safe one: if the shared_ptr<CallInvoker> is already gone, dropping is fine — the env died with the runtime that owned it. The risky direction is the opposite, a CallInvoker that is still alive while the jsi::Runtime is not, or a RuntimeScheduler that accepts invokeAsync during shutdown and then drops the task:

• For async work, complete → asyncWorkCompleteTrampoline → napi_open_handle_scope(env, …) against a napi_env that Hermes destroyed with its Runtime.
• For thread-safe functions, a silently dropped dispatch is unrecoverable: napi_threadsafe_function__::dispatch_pending stays true, so no producer ever posts again (hermes_napi_tsfn.cpp:451-455), the tsfn is never finalized and finalize_cb never runs. postTask's own comment in HermesNapiHost.cpp names this failure mode but the guarantee it relies on is implemented here.

Is CallInvoker release ordered before ~Runtime in bridgeless RN? If yes, a one-line note pointing at where that ordering comes from would settle it. If it isn't guaranteed, the drop probably needs to hang off something with the runtime's lifetime rather than the invoker's.

---

Generated by Claude Code

[kraenhansen]

This is the suite's only teardown test, and it exercises a state production can't reach. retainForProcessLifetime means the last strong ref to a HostContext is never released, so context.reset() here has no production analogue and item.context.lock() never fails in a real app.

The comment is candid about the modelling, but the two paths aren't equivalent in the way that matters: in production the pool does call dispatchToJs, and the drop happens inside the CxxNodeApiHostModule lambda — after cancelWork has already returned true. So the assertions here (js.size() == 0, no completion) pass for a reason that doesn't apply to shipped code, and the real behaviour (told-cancelled-but-never-completed) goes untested.

A dispatcher that models an expired CallInvoker — accepts the call and drops the function on the floor — would exercise the actual path, and would fail today on the cancel_work return value. Worth adding alongside this one.

---

Generated by Claude Code

[kraenhansen]

host_.data is initialised to this, so a copy or move of a HostContext yields a struct whose data points at the other object — and since postWork/postTask recover self by casting loop_data, every callback would then run against the wrong instance. create() is the only intended construction path (the constructor is private), but copy and move assignment/construction are still implicitly available on the resulting object.

= delete-ing them makes the "data always points at me" invariant compiler-enforced rather than convention.

---

Generated by Claude Code

[kraenhansen]

The comment is accurate — I checked, and Hermes' documented null-host default is "prints the error and calls abort() via hermes_fatal()". So this is behaviour-preserving relative to not providing the callback at all.

Worth considering whether that's the behaviour we want now that we're providing one, though. In Node this routes to process.emit('uncaughtException'), which is observable and handleable; node-addon-api reaches for napi_fatal_exception whenever an exception escapes a thread-safe-function callback. So under this implementation a single throwing JS tsfn callback hard-kills the app with no LogBox, no RN error handler, and no JS stack in the crash report beyond what describeError managed to stringify.

Routing through RN's error handling (with abort() as the fallback when that isn't available) would be closer to Node. Fine to defer, but if so a // TODO: here would be better than leaving it looking settled.

---

Generated by Claude Code

[kraenhansen]

Small accuracy point: the loopData half of the match doesn't disambiguate envs. CxxNodeApiHostModule creates exactly one HostContext and passes the same hostContext_->host() to hermes_napi_create_env for every addon in the runtime, so all envs in a runtime share one loopData. The pairing only separates contexts across runtimes/reloads.

That's still worth having (it's the reload case that motivates it), the comment just claims a bit more than the code delivers.

---

Generated by Claude Code

[kraenhansen]

This raises the timeout for every example in every suite, not just the one that needs it. The cost lands later: a suite that genuinely deadlocks — and this PR adds two new ways to deadlock, a saturated 4-thread pool and a wedged tsfn — now takes 30s to report instead of the default, on every CI device lane.

Both suiteName and exampleName are in scope here, so gating the bump (or letting a suite declare its own timeout in node-addon-examples/src/index.ts) keeps the rest tight while still giving the tsfn suite the room it needs.

---

Generated by Claude Code

[kraenhansen]

Addressed in 1b432c8 — all findings confirmed against the code, and the two substantive ones are fixed rather than papered over:

• Never-expiring weak_ptr / lying cancel_work: JsDispatcher now returns whether the function was accepted, WorkItem holds its HostContext strongly (the weak indirection was indeed dead), and cancelWork returns the dispatcher's verdict — so napi_cancel_async_work only reports napi_ok while the complete(napi_cancelled) can actually be delivered. workerMain/postTask now log drops where they really happen. The Catch2 teardown test was reworked to model an expired CallInvoker (dispatcher stops accepting) instead of dropping the last context ref; the new cancel_work reports failure… section fails against the previous code.
• Double-queue: WorkerPool::enqueue now drops the duplicate (log + return) instead of enqueueing it, preserving the one-completion invariant; covered by a new test, and the saturation helper uses distinct jobs per worker so it doesn't trip the detection. I kept the scan unconditional since it now prevents a UAF rather than just diagnosing one — the queue only ever holds a handful of items, so the linear scan is cheap.
• On the CallInvoker-vs-~Runtime ordering question: the property the host needs is narrower than release ordering — accepted work must either run on the JS thread while the runtime is alive or be dropped. Bridgeless RN's RuntimeSchedulerCallInvoker holds a std::weak_ptr<RuntimeScheduler>, and the ReactInstance owns scheduler and runtime together, so invokeAsync no-ops once the scheduler is gone rather than executing against a destroyed runtime; the weak capture in the module covers the window where the module's own reference is released during teardown. This is now written down at the dispatcher site, along with why a drop implies no live observer (every env this host serves dies with that same runtime) and why the one observer that remains — napi_cancel_async_work — now gets the truthful verdict.
• Also done: HostContext copy/move deleted, the 30s timeout scoped to the threadsafe-function suite only, the (loopData, workData) comment corrected to claim runtime/reload disambiguation rather than per-env, and a TODO on fatal_exception about routing through RN error handling instead of aborting.

Linux Catch2 lane is green locally (6/6); the labeled CI lanes will re-run on the push.

---

Generated by Claude Code