Skip to content

Cooperative termination mode: drain teardown instead of aborting - #182

Draft
clholgat wants to merge 1 commit into
mainfrom
cholgate/teardown-race-rootcause
Draft

Cooperative termination mode: drain teardown instead of aborting#182
clholgat wants to merge 1 commit into
mainfrom
cholgate/teardown-race-rootcause

Conversation

@clholgat

@clholgat clholgat commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Draft for discussion, not for merge. Updated to the cooperative-mode approach (replaces the earlier dispatch-guard version on this branch).

Problem

Aggressive teardown flips _isDisposed from another thread and force-terminates JS execution, aborting in-flight work and racing the JS thread. That's a large blast radius of teardown crashes / UB, and most of it is on host/session-scoped runtimes torn down on logout / scope-exit (not just workers).

Approach — a configurable mode, default cooperative

One flag on the runtime; cooperative mode drains instead of aborting:

  • Host: makeJsThreadDispatchFunction no longer skips work just because _isDisposed is set. In-flight and already-queued JS-thread work runs (the context is still alive, since teardown is serialized after it on the serial queue), then teardown destroys the context and later calls are refused by the liveness check. This effectively restores the pre-aggressive ordering.
  • Worker: terminate()/dtor skip requestExecutionTermination() and drain like self.close().

Set the flag false for aggressive termination (stops even frozen JS, at the cost of the teardown races), so the aggressive path stays available and can be rolled out again safely once the root cause is fully addressed.

Open question

This mitigates by reverting to cooperative behavior. The known trade is the original one: cooperative can't force-stop a genuinely frozen JS worker (it drains, so a busy loop won't tear down). A bounded wait-then-escalate would preserve the frozen-JS guarantee while keeping well-behaved teardown safe. Feedback welcome on whether to build that here or in the deeper fix.

Tests

bzl test //valdi:test_integration: new cooperativeTeardownDrainsInFlightWorkAggressiveSkipsIt (all 4 JS engines) asserts cooperative drains in-flight work and aggressive skips it; existing teardown and worker-termination tests stay green.

@github-actions github-actions Bot added area/runtime Valdi runtime (C++/native) size/M labels Aug 28, 2026
@github-actions

Copy link
Copy Markdown

📊 PR Size: size/M

Total changes: 141 lines (6 files)

Top files changed:

  • valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp: +36 -15
  • valdi_core/test/cpp/DispatchQueue_tests.cpp: +38 -0
  • valdi/test/integration/Runtime_tests.cpp: +26 -0
  • valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.hpp: +9 -1
  • valdi_core/src/valdi_core/cpp/Threading/ThreadedDispatchQueue.cpp: +8 -2
  • valdi_core/src/valdi_core/cpp/Threading/GCDDispatchQueue.cpp: +5 -1

Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+)

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

⚠️ Bazel & CI Test Results

Test Suite Result
macOS: C++ & Platform Tests ✅ success
Linux: Build & Export ✅ success
API Surface Check ✅ success
Snapshot Tests ✅ success
Valdi Smoke Tests ✅ success
valdi_web Integration Test ✅ success
Linux: C++ Tests ❌ failure
Linux: Module Tests ✅ success
Test Coverage Delta ✅ success
Linux: Build Compiler ✅ success
Linux: Registry Validation ✅ success
Linux: Hotreload Smoke ✅ success

Some tests failed. Please check the workflow logs for details.

🚀 Bazel remote cache is now enabled - future builds will be faster!

Workflow: Valdi CI

@clholgat clholgat closed this Aug 28, 2026
@clholgat clholgat reopened this Aug 28, 2026
@clholgat
clholgat force-pushed the cholgate/teardown-race-rootcause branch from 19d02b6 to e47b9c2 Compare August 28, 2026 23:01
@github-actions

Copy link
Copy Markdown

Sensitive Files Detected

⚠️ Core runtime — App-wide blast radius — touches the rendering core. Must be gated.

This is an automated notice. A maintainer will review after import.

@clholgat clholgat changed the title Fix runtime teardown race: run in-flight sync bridge calls, skip only async Cooperative termination mode: drain teardown instead of aborting Aug 28, 2026
Aggressive teardown flips _isDisposed from another thread and force-terminates JS
execution, aborting in-flight work and racing the JS thread. Add a mode flag (default
cooperative) that drains instead:

- Host: makeJsThreadDispatchFunction no longer skips work just because _isDisposed is
  set; in-flight/queued JS-thread work runs (context still alive, teardown serialized
  after it), then teardown destroys the context and later calls hit the liveness check.
- Worker: terminate()/dtor skip requestExecutionTermination and drain like self.close().

The mode is held on JavaScriptRuntime and pushed from the runtime tweak; worker runtimes
pull it in postInit (they inherit the host's listener, not the pushed tweak), so a host
override to aggressive reaches them too. Set false for aggressive termination (stops even
frozen JS, at the cost of the teardown races).

Draft for discussion, not for merge.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clholgat
clholgat force-pushed the cholgate/teardown-race-rootcause branch from e47b9c2 to 85c4703 Compare August 28, 2026 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/runtime Valdi runtime (C++/native) size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant