feat: per-runtime EventLoop - v8 platform tasks + two-lane scheduler (Java MessageQueue / ALooper fd)

[edusperoni]

Problem

Nothing pumps the V8 platform's foreground task queues. v8::platform::PumpMessageLoop only ran inside the WASM-scoped MessageLoopTimer (an ALooper fd fed by a detached thread polling every 100ms, wrapped around WebAssembly.compile/instantiate via JS proxies) and the inspector pause loops. Everything else V8 posts to its foreground runner just sat there:

• Atomics.waitAsync promises never resolved (their wakeup is a foreground task).
• GC finalization / heap tasks never ran.
• Async WASM compilation resolved with up to 100ms latency, and only while the proxy's start/stop window was open.

Separately, the runtime had grown four bespoke implementations of "get work onto a runtime thread": TimerHandler tokens, LooperTasks' eventfd, the worker inbound eventfd queue, and (in this PR's first cut) another token Handler for platform tasks.

Change: a per-runtime EventLoop with two lanes

Each Runtime now owns an EventLoop (the Android analogue of the iOS runtime's ExecuteOnRunLoop seam), bound to its thread in PrepareV8Runtime. Work is routed by ordering contract:

Ordered lane — work whose ordering is observable against app-level Java messages. Rides the Java MessageQueue via a dedicated com.tns.EventLoopHandler using anonymous "task due" tokens (the Timers scheme from bfd7650), so it is strictly FIFO with Handler.post runnables and JS timers. First producer: __ns__queueMacrotask(cb), the seam future spec'd macrotasks (e.g. performance-observer callbacks) will use.

Internal lane — work in its own ordering domain: v8 platform foreground tasks (WASM finalization, Atomics.waitAsync wakeups, GC tasks), worker→parent messages, unhandled-rejection drains. Rides an EFD_SEMAPHORE eventfd plus one timerfd (armed to the earliest delayed due time) on the thread's ALooper — Chrome's MessagePumpAndroid shape. No JNI on the post path, so V8's non-JVM worker threads post without attaching to the JVM. One eventfd unit = one unit of work per looper callback, so bursts interleave fairly with Java messages instead of draining in one go.

NativeScriptPlatform wraps the default platform (workers/jobs/time/tracing delegate to libplatform) and serves GetForegroundTaskRunner(isolate) from the isolate's EventLoop. The loop starts unbound and buffers (v8 requests the runner during Isolate::New); binding flushes. Each executed entry ends with a microtask checkpoint — work like the waitAsync wakeup resolves promises without entering JS, which kAuto's depth-0 drain never sees. Shutdown drops queued work and late posts, mirroring the old LooperTasks "message to a terminated runtime" semantics; leftover wakeups no-op like cleared-timer tokens.

Inspector pause loops (where the looper isn't polling) drain only nestable v8 tasks, bounded to the entries present at call time; non-nestable tasks and plain posts run from their own wakeups after the pause unwinds — matching the old PumpMessageLoop + LooperTasks behavior split.

Removed

• MessageLoopTimer (polling thread, pipe, WebAssembly proxies in message-loop-timer.js) — async WASM promises now resolve promptly with no start/stop windows.
• LooperTasks — consolidated into the internal lane; call sites (worker messaging, exception drains) ported 1:1 including the weak_ptr child semantics and drop-after-shutdown behavior.

Timers deliberately stays separate: it is the ordered lane specialized with sub-millisecond ordering machinery, and it is battle-tested.

Not in this PR

Microtask policy is untouched (kAuto). The cross-thread microtask-drain design (continuations always landing on the runtime thread under multithreaded JS) builds on this seam later.

Tests

• testEventLoop.js: Atomics.waitAsync notify/timeout/mismatch + promise-chain ordering (the async cases hang without this change), plus ordered-lane specs: __ns__queueMacrotask async delivery, runs-after-microtasks, FIFO interleaving with setTimeout(0), TypeError on non-function.
• Existing async-WASM, worker messaging, and error-event suites exercise the internal lane's ported paths.

Post-review hardening

An independent deep review of the scheduler surfaced two defects, both fixed here:

• Internal-lane unit starvation: an eventfd unit written for an immediate entry could be spent on a due-but-unsignaled delayed entry (its timerfd unit not yet issued); the later timerfd fire then found nothing due and issued nothing, leaving the lane permanently off-by-one — the most recently posted entry always waited for a future post. The unit-consuming path now skips unsignaled delayed entries; nested (unit-free) drains and the ordered lane are unaffected. Regression test: worker reply racing an overdue Atomics.waitAsync timeout.
• Stale loop registry across isolate-pointer reuse: the registry entry was erased in ~Runtime, which runs several JNI calls after Isolate::Dispose frees the address — a concurrently created worker isolate could reuse the pointer and inherit the dead runtime's stopped loop (dropping all its work), and the late destructor could then evict the new tenant's entry. Fixes: matched erase (only removes the entry while it still maps to the disposing runtime's loop) immediately after Dispose, PrepareV8Runtime refreshes a stopped loop found under its key, and the v8 task runner resolves the loop through the registry on every post so a refresh redirects already-handed-out runners.

Also from review: the inspector-pause drain guards against C++ exceptions unwinding through v8 inspector frames, and fd callbacks ignore spurious wakeups (read failure) instead of consuming an entry.

Semantic deltas vs the old LooperTasks (deliberate): worker→parent messages now run one-per-looper-poll instead of batch-per-wakeup (Java messages interleave between them; relative order preserved), and each entry is followed by a Locker + microtask checkpoint. Open question flagged by review: microtask checkpoints currently run during debugger pauses (Blink parity) — see PR discussion.

Review sequencing, applied on this PR

Per the design review's sequencing (everything except the kExplicit microtask work, which remains a follow-up):

Timers merged into the ordered lane (with tombstones). TimerHandler is deleted; timers post anonymous tokens through the EventLoop, and the token drain runs the earliest due item across timers and ordered macrotasks — one due-ordered domain, still strictly FIFO with Handler.post. clearTimeout now leaves a tombstone whose own token consumes it as a no-op, so no token gains surplus capacity to run a later-scheduled item ahead of foreign Java messages between the two token positions — this also fixes the pre-existing congestion deviation in shipped timers. FireTimer's internals (sub-ms sorted list, interval catch-up, nesting clamp) are untouched; the check-and-run is a single RunIfEarliest call under one Locker acquisition, since background threads mutate timer bookkeeping via setTimeout under multithreaded JS.

__runOnMainThread promoted into the internal lane. The 2MB main-looper pipe is gone; closures ride bare internal-lane entries that skip the loop's Locker/checkpoint — the closure locks the caller's isolate, and taking the main isolate's Locker first would nest Lockers across isolates (deadlock-capable against worker→main entry paths, per review). Delivery stays one-per-poll like the old fd callback. Incidental fixes: the callback cache is now mutex-guarded (it was written from arbitrary threads whose different-isolate Lockers provided no mutual exclusion), and uncaught callback exceptions surface as pending Java exceptions instead of unwinding C++ through the ALooper frame.

Not applied: kExplicit microtask policy (excluded by request) and the internal-lane budgeted batch drain (the review gates it on profiling evidence; the old pipe was also one-per-poll, so there is no parity argument for it).

Cancellable timer tokens (wakeup hygiene for debounce workloads)

Tombstones fix ordering but leave a cleared timer's wakeup in the queue — a no-op that still wakes the looper at due time and, worse, acquires the isolate Locker (a stale token could park the main thread behind a long background JS turn under multithreaded JS). Cancelled timers now neutralize their token, in two tiers by remaining delay:

• < 32ms — native claim cells. The token carries a slot from a fixed per-loop atomic table (indexed by timer id, id embedded in the cell word so cancellation can never hit a recycled cell). clearTimeout is a single native CAS — zero JNI: winning proves the token dead (sorted entry erased outright); losing leaves a tombstone for the in-flight token. EventLoopHandler claims cells via a @CriticalNative CAS (public API in current SDKs; degrades to plain JNI with identical semantics where unapplied) before entering the runtime — a cancelled token dies in Java in nanoseconds, never touching the Locker. Cells see exactly one gate pass by construction (only the gate retires; cell tokens are never removed), and a busy slot just downgrades to plain+tombstone.
• ≥ 32ms — identified tokens. The token carries a GC-owned AtomicBoolean peer claimed in handleMessage; clearing CASes it and, on winning, removeMessages()es the queued token — a cleared debounce timer produces no wakeup at all. The CAS makes the removal-vs-in-flight race harmless: a lost race costs one no-op wakeup, never an ordering violation. Below the cutoff a stale wakeup lands within two frames of the interaction that scheduled it (app provably awake), so the zero-allocation path applies.

The cutoff is a fixed constant (32ms): timer delays cluster bimodally (0–16ms scheduling/animation vs ≥100ms debounce/timeouts), and the identified clear is a net lifetime JNI reduction (one clear-time crossing replaces a deferred full dispatch). Only the newest token of an interval is cancellable; older re-arm-orphaned tokens keep functioning anonymously, preserving token/slot parity under anonymous dispatch.

Verified on device: the orphan-token ordering probes pass 100% across every scenario (timer-FIFO ties, clear-vs-Handler.post in both orders, orphan-across-gap, triple-clear, clearInterval from its own callback, starvation after heavy clearing), and the full suite is green (78 suites / 668 specs) including new specs for identified clears, a background-thread clear racing dispatch (multithreaded JS), and interval stop.

Summary by CodeRabbit

• New Features

  • Added a unified event-loop system for foreground tasks, timers, workers, and callbacks.
  • Added ordered macrotask scheduling and support for queuing macrotasks.
  • Improved coordination of asynchronous waits, timer cancellation, worker messages, and runtime lifecycle events.

• Bug Fixes

  • Improved task ordering, timer consistency, thread affinity, cancellation races, and cleanup during shutdown.

• Tests

  • Added comprehensive event-loop coverage for asynchronous waits, timers, validation, worker communication, cancellation, and repeated worker lifecycles.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: edea7f5d-517a-49af-8d16-0d70b9f9e798

📥 Commits

Reviewing files that changed from the base of the PR and between c2be640 and d89d08e.

📒 Files selected for processing (5)

• test-app/app/src/main/assets/app/tests/testEventLoop.js
• test-app/runtime/src/main/cpp/CallbackHandlers.cpp
• test-app/runtime/src/main/cpp/EventLoop.cpp
• test-app/runtime/src/main/cpp/EventLoop.h
• test-app/runtime/src/main/cpp/Timers.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• test-app/app/src/main/assets/app/tests/testEventLoop.js
• test-app/runtime/src/main/cpp/CallbackHandlers.cpp

---

📝 Walkthrough

Walkthrough

The runtime replaces legacy looper and timer infrastructure with per-isolate EventLoop scheduling. NativeScriptPlatform adapts V8 tasks. Timers, callbacks, promises, workers, inspector pauses, and tests now use the new event-loop paths.

Changes

Unified event-loop refactor

Layer / File(s)	Summary
EventLoop scheduler and Android bridge test-app/runtime/src/main/cpp/EventLoop.*, test-app/runtime/src/main/java/com/tns/EventLoopHandler.java	Adds internal and ordered task lanes with Android Looper tokens, native descriptors, cancellation, V8 task execution, microtask checkpoints, lifecycle handling, and JNI dispatch.
Platform and runtime lifecycle test-app/runtime/src/main/cpp/NativeScriptPlatform.*, test-app/runtime/src/main/cpp/Runtime.*, test-app/runtime/src/main/cpp/*InspectorClient.cpp, test-app/runtime/CMakeLists.txt	Adds per-isolate platform runners, binds and shuts down runtime event loops, updates inspector pause handling, and removes legacy loop sources.
Timers, callbacks, and worker integration test-app/runtime/src/main/cpp/Timers.*, test-app/runtime/src/main/cpp/CallbackHandlers.*, test-app/runtime/src/main/cpp/WorkerWrapper.*, test-app/runtime/src/main/cpp/NativeScriptException.*	Routes timers, macrotasks, callbacks, promise-rejection drains, and worker delivery through EventLoop. Timer cancellation preserves ordered tombstone slots.
Event-loop test coverage test-app/app/src/main/assets/app/mainpage.js, test-app/app/src/main/assets/app/tests/*	Adds tests for Atomics.waitAsync, macrotask ordering, argument validation, timer cancellation, worker wakeup races, and worker churn.

Estimated code review effort: 4 (Complex) | ~60 minutes

Mergeability Score: ⚪ Minimal · up to d89d0

The PR centralizes runtime task scheduling and preserves exception handling for repeat timers; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant JavaScript
  participant CallbackHandlers
  participant NativeScriptPlatform
  participant EventLoop
  participant EventLoopHandler
  participant Timers
  JavaScript->>CallbackHandlers: Queue macrotask
  CallbackHandlers->>NativeScriptPlatform: Resolve isolate runner
  NativeScriptPlatform->>EventLoop: Post ordered task
  EventLoop->>EventLoopHandler: Post task token
  EventLoopHandler->>EventLoop: Invoke nativeRunTask
  EventLoop->>Timers: Arbitrate earliest timer
  EventLoop->>JavaScript: Execute task and microtasks

Loading

Possibly related PRs

• NativeScript/android#1967: Modifies Timers and Android message-loop scheduling.
• NativeScript/android#1973: Updates inspector pause-loop task pumping and worker task dispatch.

Suggested reviewers: nathanwalker

Poem

A rabbit queues tasks in a steady line,
Timers and workers now share one design.
Tombstones preserve each ordered place,
Promises and callbacks complete their race.
V8 and Android safely run—
Event-loop tests hop in the sun.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%.	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 specifically summarizes the main change: a per-runtime two-lane EventLoop integrated with V8 platform tasks.
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.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

---

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

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

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

test-app/runtime/src/main/cpp/NativeScriptPlatform.h (1)

120-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider guarding the static JNI cache with std::once_flag.

Each ForegroundTaskRunner owns its own mutex_, so the lock in BindToCurrentThread does not serialize writes to these process-wide statics across runners. The current safety argument depends on the main runtime always binding before any worker. std::call_once would remove that dependency and would survive a future change to worker startup order.

♻️ Suggested guard

     // process-wide JNI cache, written once under the first bind's lock
+    static std::once_flag EVENT_LOOP_HANDLER_INIT;
     static jclass EVENT_LOOP_HANDLER_CLASS;

Then wrap the lookup block in BindToCurrentThread with std::call_once(EVENT_LOOP_HANDLER_INIT, ...).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.h` around lines 120 - 124,
Guard initialization of the process-wide JNI cache used by
ForegroundTaskRunner::BindToCurrentThread with a shared std::once_flag, such as
EVENT_LOOP_HANDLER_INIT. Move the class and method lookups into std::call_once
so initialization is serialized across all runners, while preserving the
existing cached symbols and subsequent use.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test-app/app/src/main/assets/app/tests/testEventLoop.js`:
- Around line 52-59: Add a rejection handler to the Atomics.waitAsync promise
chain so rejected promises and assertion errors call done.fail with the captured
error, matching the handling in the other asynchronous tests and preventing
unhandled rejections.

In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp`:
- Around line 82-99: Synchronize handler lifetime with posting: in
test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp lines 82-99, take mutex_
in ForegroundTaskRunner::~ForegroundTaskRunner before accessing handler_, call
DeleteGlobalRef while holding it, and clear handler_ under the same lock; in
lines 106-139, keep mutex_ held through PostToken in both PostImmediate and
PostDelayed so posting cannot overlap destruction.
- Around line 215-233: Update ForegroundTaskRunner::RunNestableTasks to capture
the entry-time due-task boundary before draining, then process only tasks that
were due at that point. Ensure tasks reposted during task->Run() are not
consumed in the same invocation, allowing the inspector pause loop to return and
read the next CDP message.

In `@test-app/runtime/src/main/cpp/Runtime.cpp`:
- Around line 299-301: Guard the NativeScriptPlatform::Instance() and m_isolate
values before calling IsolateDisposed in ~Runtime, so destruction before
PrepareV8Runtime initialization is safe. Inspect DestroyRuntime and
tns::disposeIsolate to verify disposal completes synchronously and that ~Runtime
runs only afterward; if disposal is deferred, adjust the ordering so
IsolateDisposed executes after v8::Isolate::Dispose completes and before the
isolate mapping is forgotten.

In `@test-app/runtime/src/main/java/com/tns/EventLoopHandler.java`:
- Around line 26-29: Update EventLoopHandler’s constructor to validate
Looper.myLooper() before passing it to Handler, and fail with a clear diagnostic
when no Looper is prepared. In ForegroundTaskRunner::BindToCurrentThread, check
env.ExceptionCheck() immediately after env.NewObject and stop the binding flow
before creating a global reference or storing handler_ when construction fails.

---

Nitpick comments:
In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.h`:
- Around line 120-124: Guard initialization of the process-wide JNI cache used
by ForegroundTaskRunner::BindToCurrentThread with a shared std::once_flag, such
as EVENT_LOOP_HANDLER_INIT. Move the class and method lookups into
std::call_once so initialization is serialized across all runners, while
preserving the existing cached symbols and subsequent use.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5b8fba76-b1fd-4594-9322-720eefa43cf1

📥 Commits

Reviewing files that changed from the base of the PR and between f284059 and cb526bc.

📒 Files selected for processing (13)

• test-app/app/src/main/assets/app/mainpage.js
• test-app/app/src/main/assets/app/tests/testEventLoop.js
• test-app/runtime/CMakeLists.txt
• test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp
• test-app/runtime/src/main/cpp/MessageLoopTimer.cpp
• test-app/runtime/src/main/cpp/MessageLoopTimer.h
• test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp
• test-app/runtime/src/main/cpp/NativeScriptPlatform.h
• test-app/runtime/src/main/cpp/Runtime.cpp
• test-app/runtime/src/main/cpp/Runtime.h
• test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp
• test-app/runtime/src/main/cpp/js/message-loop-timer.js
• test-app/runtime/src/main/java/com/tns/EventLoopHandler.java

💤 Files with no reviewable changes (4)

• test-app/runtime/src/main/cpp/MessageLoopTimer.h
• test-app/runtime/src/main/cpp/js/message-loop-timer.js
• test-app/runtime/src/main/cpp/Runtime.h
• test-app/runtime/src/main/cpp/MessageLoopTimer.cpp

[edusperoni]

Addressed the CodeRabbit findings; note the runner has since been restructured into a two-lane EventLoop (see updated PR description), so some fixes landed in EventLoop.cpp rather than the file the comment anchored to:

• testEventLoop.js promise chain without rejection handler — fixed, .catch(done.fail) added.
• handler_ global ref unsynchronized between post path and destructor — fixed: ordered-lane token posts now happen while holding mutex_ (Handler.sendMessageAtTime only enqueues, so this is cheap), and the destructor takes mutex_ before DeleteGlobalRef. The internal lane no longer posts through JNI at all (eventfd write under the same lock).
• Unbounded RunNestableTasks drain could wedge the inspector pause loop — fixed: RunNestableV8Tasks is bounded to the entry count snapshotted at call time, so a self-reposting task can't starve the CDP read.
• Instance()/m_isolate unguarded in ~Runtime — fixed with null guards. On the ordering question: tns::disposeIsolate is the runtime's own synchronous per-isolate cleanup (not v8::Isolate::Dispose); the actual Dispose() happens in WorkerWrapper between DestroyRuntime() and delete runtime_, so ~Runtime (which drops the registry entry) always runs after disposal completes, on the same thread.
• Missing-Looper NPE from Handler(Looper) — fixed: requireLooper() throws a descriptive IllegalStateException; the JNI side also asserts the constructed handler is non-null (JEnv converts pending Java exceptions into native exceptions at the call site).

[edusperoni]

Scheduler design review (independent deep review) — outcome

Current implementation: two must-fix defects found and fixed in the latest push:

1. Internal-lane unit starvation — an eventfd unit could be spent on a due-but-unsignaled delayed entry, permanently stranding the entry the unit was written for (TakeDueLocked now skips unsignaled delayed entries on the unit-consuming path only). Reachable with one Atomics.waitAsync(…, timeout) racing one worker message.
2. Stale loop registry across isolate-pointer reuse — the registry erase ran in ~Runtime, several JNI calls after Isolate::Dispose freed the address; worker churn could hand a new isolate the dead runtime's stopped loop (silently dropping all its work). Now: matched erase immediately after Dispose, stale-loop refresh at bind, and a registry-resolving v8 task runner so refreshes redirect runners v8 already holds.

Multithreaded JS: no new exposure. No v8::Unlocker exists anywhere, so the home-thread checkpoint can only observe completed background turns; kAuto already allowed any-thread drains. Home-thread Locker stalls behind long background JS turns are pre-existing (Timers has the identical shape) and granularity improved (one entry per poll vs captive batches). __ns__queueMacrotask from background-thread JS is safe and semantically sane.

Merge proposal (Timers + ordered lane into one token stream): endorsed, with one addition. Adversarial analysis confirmed a leftover clearTimeout token could run a later-posted item ahead of foreign Java messages — and found shipped Timers already exhibits exactly this deviation under congestion. Adopting tombstones on clear (cancelled entries no-op in their slot instead of being deleted) keeps tokens and entries 1:1 in due order, making the merged scheme strictly tighter than either predecessor. Migration guidance: merge only the outer token stream + due-selection; keep FireTimer's internals (interval catch-up, nesting clamp, TryCatch discipline) untouched.

Agreed sequencing: (a) this PR with the fixes above → (b) Timers/ordered-lane merge with tombstones → (c) __runOnMainThread promotion into the internal lane with an own-isolate entry flavor (routing it through the ordered lane would nest main-isolate and worker-isolate Lockers and can deadlock against multithreaded-JS entry paths) → (d) kExplicit microtask work (by then the kAuto-reliant sites are down to the JNI trampolines + ModuleInternal) → (e) budgeted internal-lane batch drain, profiling-gated.

Open questions for maintainers:

1. Should the ordered lane's FIFO contract hold under congestion/backlog (tombstones needed in today's Timers too), or only quiescently?
2. Should microtask checkpoints run during debugger pauses (current behavior, Blink parity) or be suppressed until resume (Node parity)?
3. Is per-entry Locker + checkpoint acceptable for high-rate worker messaging, or should the batch drain land together with the merge?
4. Under future kExplicit: do main-thread JNI-entry turns get inline checkpoints, or a posted drain token (continuations delayed by one looper trip)?

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

test-app/runtime/src/main/cpp/CallbackHandlers.cpp (1)

809-819: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Catch std::exception by const reference.

Line 811 catches by value. The copy slices any derived exception, so e.what() at Line 813 reports the base std::exception text instead of the real message. Catch by const std::exception&.

♻️ Proposed refactor

-    } catch (std::exception e) {
+    } catch (const std::exception& e) {
         stringstream ss;
         ss << "Error: c++ exception: " << e.what() << endl;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp` around lines 809 - 819,
Update the std::exception handler in CallbackHandlers.cpp to catch the exception
as const std::exception& instead of by value, while preserving the existing
e.what() logging and rethrow flow.

test-app/runtime/src/main/cpp/Timers.cpp (1)

244-248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use the now parameter instead of recomputing the time.

RunIfEarliest receives now, then compares against a fresh now_ms() at Line 245. The otherDue value was computed by the event loop against the passed now. Two different time bases make the "earliest across both domains" decision inconsistent: a timer whose dueTime falls between now and now_ms() becomes eligible while the loop treated no ordered entry as due. Use the parameter for one consistent basis.

♻️ Proposed refactor

     auto ref = sortedTimers_.front();
-    if (ref.dueTime > now_ms() || (otherDue >= 0 && ref.dueTime > otherDue)) {
+    if (ref.dueTime > now || (otherDue >= 0 && ref.dueTime > otherDue)) {
         // not due, or the loop's own entry is earlier - not this source's slot
         return false;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/Timers.cpp` around lines 244 - 248, Update
RunIfEarliest to compare ref.dueTime against its now parameter instead of
calling now_ms(), while preserving the existing otherDue comparison and return
behavior.

test-app/runtime/src/main/cpp/Timers.h (1)

139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the thread-safety comment with the actual locking model.

This comment states that sortedTimers_ is "Only ever touched on the isolate's home thread, no mutex". Timers::RunIfEarliest in Timers.cpp states the opposite: sortedTimers_ is mutated through setTimeout from background threads under multithreaded JS, and the isolate Locker is the guard. OrderedTaskSource in EventLoop.h documents the same Locker-based contract. Update this comment so future readers do not remove the Locker acquisition.

📝 Proposed comment fix

         // scheduled timers (and tombstones) sorted by exact (sub-millisecond)
-        // dueTime, stable for equal dueTimes. Only ever touched on the
-        // isolate's home thread, no mutex. The Java message queue is
-        // millisecond-quantized, so this preserves the relative order of JS
-        // timers; each anonymous EventLoop token consumes the front slot.
+        // dueTime, stable for equal dueTimes. Guarded by the isolate Locker,
+        // not a mutex: background threads mutate it through setTimeout under
+        // multithreaded JS. The Java message queue is millisecond-quantized,
+        // so this preserves the relative order of JS timers; each anonymous
+        // EventLoop token consumes the front slot.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/Timers.h` around lines 139 - 144, Update the
comment above sortedTimers_ to document that access is synchronized by the
isolate Locker, including mutations from background threads via setTimeout;
remove the inaccurate home-thread-only and no-mutex claims, consistent with
Timers::RunIfEarliest and OrderedTaskSource.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test-app/app/src/main/assets/app/tests/testEventLoop.js`:
- Around line 59-61: Replace the unsupported done.fail call in the promise
rejection handler of testEventLoop with Jasmine 2.0.1’s explicit failure
assertion, then call done() afterward so the handler always completes and
reports the original error.

In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 687-701: Resolve and validate Runtime::GetMainEventLoop() before
the cache insertion block in the callback registration flow. Update the
surrounding logic so a null mainLoop returns without calling cache_.try_emplace,
while valid loops retain the existing insertion, duplicate assertion, and
PostInternalBare behavior.
- Around line 704-717: Update CallbackHandlers::RunMainThreadEntry so the cached
isolate remains alive from cache lookup through v8::Locker acquisition, rather
than copying an unprotected raw pointer after releasing cacheMutex_. Use the
existing ownership or liveness mechanism for the cache entry, and ensure
teardown cannot dispose the isolate until the lock-acquisition phase completes.

---

Nitpick comments:
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 809-819: Update the std::exception handler in CallbackHandlers.cpp
to catch the exception as const std::exception& instead of by value, while
preserving the existing e.what() logging and rethrow flow.

In `@test-app/runtime/src/main/cpp/Timers.cpp`:
- Around line 244-248: Update RunIfEarliest to compare ref.dueTime against its
now parameter instead of calling now_ms(), while preserving the existing
otherDue comparison and return behavior.

In `@test-app/runtime/src/main/cpp/Timers.h`:
- Around line 139-144: Update the comment above sortedTimers_ to document that
access is synchronized by the isolate Locker, including mutations from
background threads via setTimeout; remove the inaccurate home-thread-only and
no-mutex claims, consistent with Timers::RunIfEarliest and OrderedTaskSource.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 176a9121-a7f6-46ca-bc75-1b507efbd79e

📥 Commits

Reviewing files that changed from the base of the PR and between cb526bc and 33d7547.

📒 Files selected for processing (23)

• test-app/app/src/main/assets/app/tests/eventLoopEchoWorker.js
• test-app/app/src/main/assets/app/tests/testEventLoop.js
• test-app/runtime/CMakeLists.txt
• test-app/runtime/src/main/cpp/CallbackHandlers.cpp
• test-app/runtime/src/main/cpp/CallbackHandlers.h
• test-app/runtime/src/main/cpp/EventLoop.cpp
• test-app/runtime/src/main/cpp/EventLoop.h
• test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp
• test-app/runtime/src/main/cpp/LooperTasks.cpp
• test-app/runtime/src/main/cpp/LooperTasks.h
• test-app/runtime/src/main/cpp/NativeScriptException.cpp
• test-app/runtime/src/main/cpp/NativeScriptException.h
• test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp
• test-app/runtime/src/main/cpp/NativeScriptPlatform.h
• test-app/runtime/src/main/cpp/Runtime.cpp
• test-app/runtime/src/main/cpp/Runtime.h
• test-app/runtime/src/main/cpp/Timers.cpp
• test-app/runtime/src/main/cpp/Timers.h
• test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp
• test-app/runtime/src/main/cpp/WorkerWrapper.cpp
• test-app/runtime/src/main/cpp/WorkerWrapper.h
• test-app/runtime/src/main/java/com/tns/EventLoopHandler.java
• test-app/runtime/src/main/java/com/tns/TimerHandler.java

💤 Files with no reviewable changes (3)

• test-app/runtime/src/main/cpp/LooperTasks.h
• test-app/runtime/src/main/java/com/tns/TimerHandler.java
• test-app/runtime/src/main/cpp/LooperTasks.cpp

🚧 Files skipped from review as they are similar to previous changes (4)

• test-app/runtime/CMakeLists.txt
• test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp
• test-app/runtime/src/main/java/com/tns/EventLoopHandler.java
• test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test-app/app/src/main/assets/app/tests/testEventLoop.js`:
- Around line 177-196: Update the background-thread clear flow in the test
iteration to use an AtomicBoolean or equivalent completion signal set after
__ns__clearTimeout(t1) runs. Before completing the iteration with done(), assert
that the signal confirms the clear operation executed, while preserving the
existing order assertions and retry behavior.

In `@test-app/runtime/src/main/cpp/EventLoop.cpp`:
- Around line 284-296: Update the token-posting flow around claimCells_ and
EVENT_LOOP_HANDLER_POST_TOKEN so a failed env.CallVoidMethod releases the
claimed cell back to 0 when no token reaches the queue. Preserve the existing
dispatch-gate cleanup for successfully queued active tokens, and ensure the
failure path is triggered only when the JNI post throws or otherwise does not
complete.
- Around line 102-116: Update the EventLoop native binding around
EventLoop::ClaimTokenCritical to track whether critical registration succeeds
and whether the runtime supports the critical JNI ABI (API 26+); handle
RegisterNatives failure without leaving a pending exception. Gate claim-token
behavior on that capability, keep nativeClaimToken provided only through
RegisterNatives, and make PostTimerToken emit plain tokens whenever the critical
binding is unavailable so EventLoopHandler.handleMessage uses the compatible
legacy path.

In `@test-app/runtime/src/main/cpp/Timers.cpp`:
- Around line 263-270: Ensure failed token posts roll back committed state: in
test-app/runtime/src/main/cpp/Timers.cpp lines 263-270, update addTask around
postTimer so exceptions remove the timerMap_ entry and sortedTimers_ slot before
propagating to the existing catch; in
test-app/runtime/src/main/cpp/EventLoop.cpp lines 284-296, update the
env.CallVoidMethod exception path to store 0 in the claim cell before
propagating the exception.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6de1b1b0-c38a-4bbb-9d5f-fff507bf1c32

📥 Commits

Reviewing files that changed from the base of the PR and between 33d7547 and 375eecd.

📒 Files selected for processing (6)

• test-app/app/src/main/assets/app/tests/testEventLoop.js
• test-app/runtime/src/main/cpp/EventLoop.cpp
• test-app/runtime/src/main/cpp/EventLoop.h
• test-app/runtime/src/main/cpp/Timers.cpp
• test-app/runtime/src/main/cpp/Timers.h
• test-app/runtime/src/main/java/com/tns/EventLoopHandler.java

🚧 Files skipped from review as they are similar to previous changes (1)

• test-app/runtime/src/main/cpp/Timers.h

[edusperoni]

Second CodeRabbit batch triaged; all verified against the code and addressed in the latest push except one, dispositioned below:

• @CriticalNative ABI below API 26 (EventLoop.cpp) — confirmed against minSdk 21 and fixed: registration now binds a standard-ABI twin (ClaimTokenLegacy(JNIEnv*, jclass, jlong, jlong)) when android_get_device_api_level() < 26, where ART ignores the annotation and calls through the normal convention — same behavior on every API level, no plain-token downgrade needed. RegisterNatives failure no longer asserts: it clears the pending exception and flips a gate flag, and PostTimerToken emits only plain tokens while the gate is unregistered, so nativeClaimToken can never be reached unbound.
• Failed token post leaks the claim cell (EventLoop.cpp) — fixed: the JNI post is wrapped; on throw the cell is stored back to 0 before propagating, since no dispatch gate will ever retire it.
• Throwing post leaves partially committed timer state (Timers.cpp) — fixed: addTask now rolls back on a postTimer throw by erasing the just-inserted sorted slot and map entry outright (erasing, not tombstoning — a tokenless live slot would steal another token, and a tokenless tombstone would starve the item behind it) before rethrowing to the existing SetTimer catch.
• cache_ insert before the main-loop null check (CallbackHandlers.cpp) — fixed by reordering; a callback can no longer be pinned in the cache with no post to consume it.
• done.fail doesn't exist in Jasmine 2.0.1 (testEventLoop.js) — fixed at all three sites with the record-then-done() pattern (expect("resolved").toBe("rejected: " + e); done();), so a rejection reports the real error instead of a silent timeout.
• Race spec can pass without racing (testEventLoop.js) — fixed: each iteration carries an AtomicBoolean set after the background clear, and only iterations whose clear provably ran count toward the 30-run quota (bounded at 300 attempts with a diagnostic failure).
• RunMainThreadEntry isolate liveness across Locker acquisition — acknowledged, deliberately not fixed here. The window (cached isolate pointer read under cacheMutex_, v8::Locker acquired after release, worker Isolate::Dispose in between) is byte-for-byte the behavior of the removed pipe implementation — RunOnMainThreadFdCallback had the identical unprotected read-then-Lock sequence — so this PR neither introduces nor widens it. Closing it properly needs a liveness mechanism that spans lock acquisition (holding cacheMutex_ through the Locker inverts the caller-side lock order and can deadlock), which belongs to the teardown-coordination work already queued with the kExplicit follow-up.

Full device suite re-run green after these changes.