fix(worker): keep the worker isolate alive while terminate() uses it, and let it stop a closing worker

[adrian-niculescu]

Calling terminate() on a Worker that is ending on its own at the same moment, for example through its own close(), can make the parent thread interrupt the worker's isolate after the worker thread has already disposed it. A parent that terminates its child workers during its own shutdown races the same way. Separately, terminate() cannot stop a worker that called close() and kept running, so close(); while (true) {} spins forever.

WorkerWrapper::Terminate reads workerIsolate_ and then calls TerminateExecution() and the event-loop lookup on it with no lock, while BackgroundLooper clears the pointer and disposes the isolate during teardown. Nothing makes the worker thread wait for a Terminate that has already read the pointer. Terminate now holds a mutex across the read and the use, and the worker thread takes the same mutex when it clears the pointer, before disposing the isolate: a terminate that already read it finishes with it first, and a later one finds null. This is the same fix as NativeScript/ios#478 on iOS.

close() only ends the worker once its running callback returns, and Terminate returned early for a closing worker. It no longer returns early for a closing worker, which matches the iOS runtime, so it interrupts one that is still running.

The isolate race window is a few instructions wide, so it has no spec. With a sleep injected between the read and the use, a worker that closes itself while terminate() runs is already disposed when the parent touches its isolate on main, and stays alive until the parent is done with this change. The new spec for a worker spinning after close() fails on main and passes here, and the full device suite passes.

Summary by CodeRabbit

• Bug Fixes
  • Workers that have called close() but continue running can now be terminated reliably. This prevents them from continuing background activity after termination is requested.
• Tests
  • Added coverage to verify that termination stops a closing worker’s ongoing activity.

[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: e04a8df7-9ff1-4aab-9060-bbe32ef28009

📥 Commits

Reviewing files that changed from the base of the PR and between 9b12329 and 44f3c8c.

📒 Files selected for processing (5)

• test-app/app/src/main/assets/app/mainpage.js
• test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js
• test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js
• test-app/runtime/src/main/cpp/WorkerWrapper.cpp
• test-app/runtime/src/main/cpp/WorkerWrapper.h

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

Worker termination can now proceed after a worker calls close(), with isolate access synchronized against shutdown. The app test suite adds a worker test that checks whether termination stops shared-counter increments.

Changes

Worker termination after close

Layer / File(s)	Summary
Synchronize termination and isolate shutdown test-app/runtime/src/main/cpp/WorkerWrapper.h, test-app/runtime/src/main/cpp/WorkerWrapper.cpp	Terminate proceeds unless the worker is disposed and holds workerIsolateMutex_ while using the isolate. Shutdown uses the same mutex when clearing the isolate pointer.
Test termination after close test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js, test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js, test-app/app/src/main/assets/app/mainpage.js	The worker calls close() and increments a shared counter until signaled to stop. The test terminates the worker after observing increments and checks that the counter remains unchanged. The app loads the test module.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to 44f3c

The change lets terminate() stop a worker that has called close(), and it keeps the isolate alive while terminate() uses it. No concrete merge-blocking risk was found, and a new test covers the close-then-spin case.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files.	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 both worker termination fixes: protecting the isolate while terminate() uses it and allowing terminate() to stop a closing worker.
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.

• Fix all pre-merge checks with AI

• 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 spin,
Then saw the counter hold its grin.
“Close,” said the worker, “I must go!”
Termination stopped its steady flow.
The rabbit hopped and thumped the ground,
While quiet counters made no sound.}

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