ReviewOS

zig-utils/zig-js

Threads: synchronize capped admission with completion state

#895
Closed chris opened this 11 days ago ·
11 days ago

Parent: #482 Related: #887, #493

Problem

The optional max_js_threads admission check reads every ThreadRecord.done field while holding Gil.api_lock. Completion writes that field while holding the record's different join_mutex.

At a8f1b3c:

These locks do not establish a happens-before edge between a completing worker and a concurrently admitting caller under no-GIL execution. This is an engine-state synchronization defect. The cap is currently a TestingOptions knob; this finding does not claim a production resource-limit bypass or a measured TSan failure. It was identified by source review during #887 validation.

Required implementation

  • Read completion through the existing record synchronization, preserving the API-lock transaction for cap check, ID claim, and record insertion.
  • Verify the API-lock/record-lock order against GC tracing, join settlement, spawn failure, and teardown. Do not introduce a global execution lock or a suppression.
  • Establish the intended live boundary (done versus exited) from the existing contract before changing policy. Do not quietly change the cap's meaning while fixing the race.
  • Preserve ID-space checks and allocation/spawn failure rollback, including the startup/queue reservations introduced by #887 and #886.

Acceptance

  • A deterministic bounded witness exercises capped admission concurrently with completion and passes suppression-free TSan.
  • The chosen live-boundary semantics, successful admission/refusal, and unique IDs are checked exactly in GIL and no-GIL modes.
  • Allocation/spawn failure does not leave an admission or startup reservation behind.
  • Applicable unit, lifecycle, and GC checks pass; no new Thread limit, retry of JS callbacks, or race suppression is used.
11 days ago

The race is now reproduced under suppression-free TSan: capped admission reads done while publishThreadCompletion writes it under join_mutex. The baseline witness aborts with a data-race report.

The pending fix reads each completion field under that same mutex while retaining the existing API-lock transaction for cap checking, ID allocation, and registry insertion. It preserves done as the live boundary, including completed bodies whose native exit or join settlement is unfinished.

Three targeted regressions now pass under TSan: concurrent publication/counting, public admission/refusal with exact ID accounting in both GIL and no-GIL modes, and allocation failure while constructing the refusal error with startup and queue reservations unchanged. The public test holds a Lock to keep the first worker live. Typed-array Atomics.wait remains refused in shared-realm Threads as required by the existing contract.

These are focused results, not full-suite acceptance. Combined Debug/ReleaseSafe/TSan, lifecycle, GC, ABI, fuzzing, and exact Promise/async conformance validation is next, shared with #885.

11 days ago

Landed fix(threads): synchronize capped admission with completion (#895).

The previously reproduced TSan race is fixed by reading done under join_mutex while keeping the API-lock admission/ID transaction intact. The live boundary remains done. Regression tests cover concurrent publication/counting, exact admission/refusal and ID accounting in both execution modes, and refusal-error OOM with unchanged startup/queue reservations.

Combined validation with #885 passed 51 distinct focused tests per Debug/ReleaseSafe/TSan mode, the full Debug suite (2,348 passed, 1 skipped, no failures/leaks), 14 selected PR-249 runs, the initial default/verify fuzz profiles, and Debug ABI fixtures. The Promise-only versus combined Test262 comparison flips zero of the same 1,828 Promise/async cases. Amplified (30 programs) and broad (80 programs) fuzzing also passed.

The clean rebase preserved the patch exactly. Post-rebase checks and the extended/full TSan acceptance matrix remain pending, so this issue stays open.

11 days ago

Final integration verification for fix(threads): synchronize capped admission with completion (#895) is complete on the pushed engine source in 09279120. The deterministic publication/counting race, exact GIL/no-GIL admission and ID accounting, and refusal-OOM reservation rollback witnesses are now covered by the completed integration matrix.

  • Final local Debug suite: 2,380 passed, one Linux-only skip, zero failures/leaks.
  • Public C, Home/private ABI lifecycle/readable-stream, and Bun C API extension fixtures/audits pass in Debug, ReleaseSafe, and unsuppressed TSan.
  • Linux CI 34287554681 passes every full Debug/TSan unit shard, no-GIL Debug/ReleaseSafe/TSan corpus shard, ABI job, and normal/TSan fuzz profile. The sole red job was the stale #903 reference diagnostic, reconciled separately in 656f4d81; this is not a claim that that whole CI run was green.
  • Exact paired Test262 diagnostics remain identical across the ten measured Promise/Atomics/async/module/weak subtrees: 2,893 passed, zero failures, zero flips/regressions.
  • Linux mid-GC: 900 programs from seed 1, zero failures; suppression-free TSan mid-GC: 90 programs, zero failures.

Closing this transaction issue. The earlier local incomplete-sweep observation is preserved and remains under investigation in #907; #896's remaining private exception-box admission audit and #906's finalization cleanup ownership work remain separate open tasks. No timeout, workload, expected failure, or sanitizer suppression was relaxed to obtain this evidence.

Sign in to comment on this issue.