fix(worker): decide who deletes a WorkerWrapper with one atomic state

[adrian-niculescu]

A worker that ends while a child worker of its own is still running (terminated by its parent, or calling close()) reads freed memory on the child's thread. With AddressSanitizer on the TestRunner the new spec aborts on main:

__asan_report_load1
tns::WorkerWrapper::IsWeak()
tns::Runtime::~Runtime()

The same applies to an embedder that shuts the main runtime down while workers run. Without a sanitizer it is usually silent; when the freed byte happens to read non-zero it becomes a double free.

The parent's teardown terminates its children and, a few lines later, disposes their Worker objects, so both threads reach the end of a child's WorkerWrapper together. Who deletes it was decided by two flags checked in opposite orders: the worker published isDisposed_ and later read isWeak_, the parent read isDisposed_ and then either deleted or set isWeak_. That allows:

• parent deletes after isDisposed_, while the child's ~Runtime still reaches the wrapper through its Caches::Workers entry (any other runtime's ~Runtime walking the registry can hit the stale entry too);
• parent reads "not disposed", child publishes disposed and reads "not weak", parent sets weak: nobody deletes;
• a worker whose operation only starts after the parent is gone takes the no-runtime branch, which never deleted.

This replaces both flags with one atomic Holders { Parent, Both, WorkerThread }. It starts as Parent, and Start() makes it Both. The worker thread lets go as its last touch of the wrapper, after its registry entry is removed (both branches): CAS Both -> Parent, or delete if it finds WorkerThread. The parent's final disposal does CAS Both -> WorkerThread, or deletes if it finds Parent. The finalizer only deletes in Parent. Exactly one CAS out of Both succeeds, so exactly one side ends up last. Registry readers stay safe because ForEach holds the map mutex across its callback and the worker's Remove has to get that mutex before the wrapper can become deletable by anyone. A wrapper that never reached Start() stays Parent, so its finalizer can free it, which the old flags never allowed.

isDisposed_ remains only as the worker thread's own "teardown has begun" flag.

The new spec in WorkerLifetimeTests.js runs 24 parent/child rounds, alternating terminate() and close(). It needs ASan to fail on main; with the change the full suite is clean under ASan apart from the thread QoS specs, which read a different QoS class back when the sanitizer interposes thread creation.

Summary by CodeRabbit

• Bug Fixes

  • Improved worker cleanup and ownership handling during termination and runtime shutdown.
  • Prevented premature worker disposal when parent and worker execution are still active.
  • Ensured worker completion notifications are delivered reliably.

• Tests

  • Added stress coverage for repeatedly terminating nested workers.
  • Added coverage for worker startup, communication, and graceful closure.

[coderabbitai]

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: 0fa8fab8-bd33-4ba7-b425-2e40e0e3f1cb

📥 Commits

Reviewing files that changed from the base of the PR and between 6221ca8 and a2ed445.

📒 Files selected for processing (6)

• NativeScript/runtime/DataWrapper.h
• NativeScript/runtime/ObjectManager.mm
• NativeScript/runtime/Runtime.mm
• NativeScript/runtime/WorkerWrapper.mm
• TestRunner/app/tests/WorkerLifetimeTests.js
• TestRunner/app/tests/workerLifetimeNestedParent.js

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

WorkerWrapper now tracks parent and worker-thread ownership with an atomic holder state. Disposal and runtime teardown use explicit release operations. New stress tests cover repeated termination and self-close with a nested worker.

Changes

Worker lifetime ownership

Layer / File(s)	Summary
Holder state contract NativeScript/runtime/DataWrapper.h, NativeScript/runtime/WorkerWrapper.mm	WorkerWrapper replaces weak-state accessors with parent and worker-thread release methods. An atomic holder state coordinates wrapper deletion.
Parent and worker teardown NativeScript/runtime/ObjectManager.mm, NativeScript/runtime/WorkerWrapper.mm, NativeScript/runtime/Runtime.mm	Parent disposal and worker teardown release their respective holders. Runtime destruction removes the worker registry entry without deleting the wrapper directly.
Nested worker teardown validation TestRunner/app/tests/workerLifetimeNestedParent.js, TestRunner/app/tests/WorkerLifetimeTests.js	Tests exercise repeated parent termination and self-close while a child worker runs. Cleanup waits for worker-ended events and handles late callbacks.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Parent
  participant ObjectManager
  participant WorkerWrapper
  participant BackgroundLooper
  participant Runtime
  Parent->>ObjectManager: Dispose worker wrapper
  ObjectManager->>WorkerWrapper: ReleaseFromParent
  BackgroundLooper->>Runtime: Tear down worker runtime
  BackgroundLooper->>WorkerWrapper: ReleaseFromWorkerThread
  WorkerWrapper->>Parent: Post worker-ended notification

Loading

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to a2ed4

This change reworks how the native worker wrapper's memory is released between the parent thread and the worker's background thread, replacing a simpler weak-reference flag with a coordinated ownership handoff, and adds a stress test that repeatedly terminates and closes a worker with a running child. Verification of the core ownership handoff, the object finalizer re-registration, and the shared worker registry's thread-safety found no double-free, leak, or race introduced by this change, so it appears safe to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (3 skipped: 3 …	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 and concisely describes the main change: using one atomic state to determine which side deletes a WorkerWrapper. It matches the lifetime-management objectives and changed implementat…
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 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

---

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 guards the holder state,
While parent and worker share the gate.
Threads release, then safely part,
Nested workers test each start.
The wrapper rests when claims are through.}

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