fix: keep the Worker object rooted until its thread ends, and report the end as nsworkerended

[edusperoni]

Stacked on #2043 (feat/worker-threads) — merge that first.

What this fixes

terminate() reset the Worker object's Persistent and dropped the registry entry synchronously, while the worker thread was still winding down. From that moment the wrapper was unreachable from native, the end of the worker was unobservable from JS, and any message the worker had already queued on the parent's loop was discarded when it arrived.

Android vs iOS lifetime — what was verified before porting

The iOS change is largely about replacing finalizer resurrection with reachability. Android never had that problem, so most of it does not apply:

• The Worker object is already a strong root, from construction. WorkerWrapper::poWorker_ is new Persistent<Object>(parentIsolate, workerObject) (WorkerWrapper.cpp) and is never made weak. There is no RootWorkerObject/UnrootWorkerObject to add.
• The Worker object is not an ObjectManager object. CallbackHandlers::NewThreadCallback takes args.This() of the plain FunctionTemplate installed in Runtime::PrepareV8Runtime (Runtime.cpp, the Worker constructor block) and hands it to the wrapper; nothing registers it with ObjectManager or marks it for GC. So none of iOS's DataWrapper/ObjectManager refuse-and-re-weaken work has an Android counterpart.
• The wrapper itself is shared_ptr-owned by the registry and by the detached thread (Start() captures shared_from_this() and BackgroundLooper holds it for the thread's whole life), so this cannot die under the thread. iOS's "read everything before publishing isDisposed_" hardening exists because a tearing-down iOS parent can delete the wrapper concurrently; that cannot happen here, and it is left alone.
• Worker-thread posts already reach the parent only through its event loop. parentTasks_ is a std::weak_ptr<EventLoop> captured on the parent's thread at construction, and PostMessageToParent, PassUncaughtExceptionFromWorkerToParent and the thread-exit post all go through it. The only reads of the parent isolate's runtime slot (Runtime::GetRuntime(parentIsolate)) happen inside lambdas that run on the parent's thread. Nothing was wrong; nothing changed.

The change

• CallbackHandlers::WorkerObjectTerminateCallback no longer calls WorkerWrapper::ClearWorkerOnParent(id). terminate() only starts the wind-down.
• WorkerWrapper::BackgroundLooper's final post to the parent's loop now runs a new WorkerWrapper::NotifyThreadEndedOnParent(workerId): on the parent's thread, with the parent isolate locked and entered, it resolves the wrapper by id, takes the Worker object out of poWorker_, calls WorkerEvents::EmitEnded under a TryCatch (a throwing listener is reported exactly as FireErrorOnParentWorkerObject/FireMessageOnParentWorkerObject report one — ContainUncaughtCallbackException then ReportFromEventLoopEntry), and only then calls ClearWorkerOnParent.
• WorkerEvents::EmitEnded mirrors EmitError's shape: a third emitEnded callout cached in WorkerEventsState at WorkerEvents::Init.
• js/worker-events.js gains emitEnded(), which dispatches a plain Event("nsworkerended") on the Worker object.
• js/node-worker-threads.js: the shim's Worker listens for nsworkerended and reports the exit from there. exit (code 0) is emitted exactly once, for a self-close() as much as for terminate(), and terminate() now resolves from the same notification instead of off a microtask, so nothing the worker sent can arrive after exit.
• A parent that is itself tearing down (TerminateChildren → ClearWorkerOnParent on the parent's thread) or whose loop is gone (expired parentTasks_) never delivers the notification. That is deliberate, matches iOS, and is documented as best effort.

Consumer audit for the removed clear. Nothing relied on the persistent being empty after terminate() for correctness: PostMessageToParent already returns early on isTerminating_; every error source is gated at the point of forwarding (CallWorkerScopeOnErrorHandle returns early on IsTerminating(), the unhandled-rejection path in NativeScriptException.cpp checks IsTerminating()/IsDisposed(), and BackgroundLooper's catch checks !isTerminating_), so no error from a terminated worker reaches the parent; the isTerminated private only guards a double terminate(); FireMessageOnParentWorkerObject/FireErrorOnParentWorkerObject keep their empty-persistent guards for the teardown paths that still clear early. The one observable change is the intended one: a message the worker queued before terminate() is now delivered instead of being dropped on arrival, and it is delivered before nsworkerended.

Tests

New Android-only specs in test-app/app/src/main/assets/app/tests/testWorkerLifetime.js (wired in mainpage.js), with workerLifetimeCloseWorker.js and messaging/deadlockChild.js / messaging/deadlockParent.js:

• a live Worker survives GC as a WeakMap key (the ephemeron repro, kept as a regression guard; collections are driven from a task via __collect({ execution: "async" }) so conservative stack scanning does not keep the workers alive)
• an unreferenced live Worker still answers messages
• a terminated Worker becomes collectable — observed after nsworkerended, since the root outlives terminate() by design
• a Worker that closed itself becomes collectable
• node:worker_threads: exit exactly once on self-close(); exit exactly once on terminate(), after the thread ended, with terminate() resolving after it and a second terminate() resolving immediately
• a terminated worker whose dropped message sentinels a port it owns still ends

Full device suite on a Pixel_3a_API_36 arm64 emulator: 1398 specs, 0 failures, 4 skipped (baseline on feat/worker-threads was 1391/0/4; the 7 new specs all ran and passed). npm run lint clean.

Deviations from iOS (ab72efc2)

1. Not ported: DataWrapper.h, ObjectManager.mm, RootWorkerObject/UnrootWorkerObject, EndWrapperLifetime, selfRef_. Android's persistent is strong from construction and the Worker object is not an ObjectManager object, so there is no weak handle to clear and no resurrection branch to take it off. The wrapper is kept alive by shared_ptrs, so iOS's atomic liveness token is unnecessary — the notification resolves the wrapper through the id-keyed registry instead.
2. Not ported: PostToRuntimeLoop → PostToLoop / mainLoop_. Android already captures parentTasks_ as a weak_ptr<EventLoop> on the parent's thread; the isolate-slot read iOS was removing does not exist here.
3. Not ported: the isDisposed_ publication reordering. The thread owns a shared_ptr to the wrapper, so nothing can delete it mid-teardown.
4. EmitEnded lives in WorkerEvents, not Worker. Android splits the worker-events callouts into WorkerEvents.{h,cpp}; that is where EmitMessage/EmitError already are.
5. docs/knowledge/v8-resurrecting-finalizers.md has no Android counterpart (docs/knowledge/ holds only v8-14-migration.md), so that hunk is skipped. The js/README.md listener-bag rule is reworded to the same effect without referring to a patch document this repo does not carry.
6. Docs say "from the moment it is constructed" where iOS says "from the moment its thread starts", because Android's persistent is strong from construction rather than from Start().
7. Tests are in Android style (var/function, jasmine done) and bump jasmine.DEFAULT_TIMEOUT_INTERVAL to 30s the way the other worker suites do; the fixed waits in the node:worker_threads specs are 2000 ms rather than iOS's 800 ms for emulator slowness. The two collectability specs additionally gate on nsworkerended before observing the WeakRef, which iOS approximates with a fixed delay.

Mirrors NativeScript/ios#456.

Summary by CodeRabbit

• Bug Fixes
  • Workers now remain available while termination is in progress and become eligible for garbage collection after their thread ends.
  • The exit event is reported once after the worker finishes, with exit code 0. Calls to terminate() now resolve when shutdown completes rather than immediately.
• Documentation
  • Clarified when worker exit notifications occur, how termination results are reported, and how worker lifetime relates to garbage collection.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 39253bab-22f6-4744-8ac3-3ed9bf4ce90a

📥 Commits

Reviewing files that changed from the base of the PR and between 42a8bcf and 59edaa4.

📒 Files selected for processing (15)

• docs/worker-threads.md
• test-app/app/src/main/assets/app/mainpage.js
• test-app/app/src/main/assets/app/tests/messaging/deadlockChild.js
• test-app/app/src/main/assets/app/tests/messaging/deadlockParent.js
• test-app/app/src/main/assets/app/tests/testWorkerLifetime.js
• test-app/app/src/main/assets/app/tests/workerLifetimeCloseWorker.js
• test-app/runtime/src/main/cpp/CallbackHandlers.cpp
• test-app/runtime/src/main/cpp/WorkerEvents.cpp
• test-app/runtime/src/main/cpp/WorkerEvents.h
• test-app/runtime/src/main/cpp/WorkerWrapper.cpp
• test-app/runtime/src/main/cpp/WorkerWrapper.h
• test-app/runtime/src/main/cpp/js/README.md
• test-app/runtime/src/main/cpp/js/events.js
• test-app/runtime/src/main/cpp/js/node-worker-threads.js
• test-app/runtime/src/main/cpp/js/worker-events.js

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

---

📝 Walkthrough

Walkthrough

The runtime now retains worker objects until their threads finish and reports completion through an internal event. The Worker API uses that event to emit one zero-code exit event and resolve pending terminate() promises. New tests and documentation cover worker lifetime and exit behavior.

Changes

Worker lifecycle

Layer / File(s)	Summary
Native thread-end notification test-app/runtime/src/main/cpp/CallbackHandlers.cpp, test-app/runtime/src/main/cpp/WorkerEvents.*, test-app/runtime/src/main/cpp/WorkerWrapper.*, test-app/runtime/src/main/cpp/js/worker-events.js	Termination no longer immediately clears the worker root and registry entry. When the thread ends, the runtime dispatches nsworkerended on the parent-side Worker, then clears those entries.
Worker exit and termination behavior test-app/runtime/src/main/cpp/js/node-worker-threads.js, docs/worker-threads.md	The Worker API reports one exit event with code 0 when the worker ends and resolves pending termination promises at that point. The documentation describes exit conditions, worker reachability, and the internal completion event.
Worker lifetime tests and supporting notes test-app/app/src/main/assets/app/mainpage.js, test-app/app/src/main/assets/app/tests/*, test-app/runtime/src/main/cpp/js/README.md, test-app/runtime/src/main/cpp/js/events.js	The test runner includes worker lifetime tests for reachability, collection, exit events, termination promises, and termination with a transferred port in flight. The notes describe listener storage and worker lifetime behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant WorkerThread
  participant WorkerWrapper
  participant WorkerEvents
  participant Worker
  participant NodeWorkerThreads
  WorkerThread->>WorkerWrapper: Post NotifyThreadEndedOnParent
  WorkerWrapper->>WorkerEvents: EmitEnded(worker)
  WorkerEvents->>Worker: Dispatch nsworkerended
  Worker->>NodeWorkerThreads: Handle nsworkerended
  NodeWorkerThreads->>NodeWorkerThreads: Emit exit(0) and resolve waiters
  WorkerWrapper->>WorkerWrapper: Clear worker registry entry and persistent handle

Loading

Merge Risk: ⚪ Minimal · up to 59eda

The change keeps Worker objects alive until their threads end and reports exit and terminate() results at that point. No merge-blocking issue was identified from the supplied review material.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 59eda

The lifecycle change improves Worker retention and completion ordering without an identified expansion of worker authority. One failure-containment issue remains: a throwing exit listener can leave earlier termination promises permanently pending, even though native cleanup proceeds.

Retained concerns

• Medium · reliability · observed: The new completion transition depends on application exit callbacks returning normally. #reportExit marks the Worker exited and synchronously emits exit before clearing or resolving pending terminate waiters. If a listener throws, those existing promises remain pending permanently; subsequent terminate calls take the already-exited fast path without repairing them. Native exception reporting and Worker cleanup still proceed, so the defect can strand shutdown or recovery workflows awaiting termination in an otherwise live parent. Previously, the terminate promise rejected on the same callback exception.

Security review details

Security Blast Radius

• inferred — The demonstrated failure requires an exit callback running with parent application authority and affects pending termination waits for that Worker. Native cleanup continues; the inspected path does not establish cross-worker, cross-tenant, credential, or service-level compromise.

Trust Boundaries and Controls

• observed — Native completion uses the registry-bound worker ID and its persistent parent-side receiver. The compatibility module separates this control path from worker message delivery. These are concrete counterevidence against ordinary worker messages impersonating completion for another Worker.

Resilience and Maintainability Implications

• observed — The native completion boundary catches and reports listener exceptions before clearing Worker ownership. This protects native cleanup but cannot restore termination resolvers skipped by the interrupted JavaScript completion method, leaving logical recovery state inconsistent with completed native shutdown.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 34.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 13 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: keep the Worker rooted until its thread ends, then report the end with nsworkerended.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 34.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 13 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit watched the worker run,
Then waited till its thread was done.
“Exit zero!” rang through the air,
The waiting promises settled there.
The rabbit hopped through fields of green,
While workers rested, safely seen.}

_{Comment @coderabbitai help to get the list of available commands.}