Skip to content

test(worker_threads): assert stdout before exitCode in #29211 regress…

08de32c
Select commit
Loading
Failed to load commit list.
Open

Stop dispatching parent messages to self.onmessage in node:worker_threads workers #29215

test(worker_threads): assert stdout before exitCode in #29211 regress…
08de32c
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 12, 2026 in 34m 46s

Code review found 2 important issues

Found 3 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 2
🟡 Nit 1
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important src/js/node/worker_threads.ts:427-435 parentPort.removeListener cannot remove listeners added via parentPort.on
🔴 Important src/js/node/worker_threads.ts:174-197 messageerror-only listener installs message forwarder, leaking event loop and dropping messages
🟡 Nit test/regression/issue/29211.test.ts:30-36 cleanStderr filter too narrow causes false ASAN CI failures

Annotations

Check failure on line 435 in src/js/node/worker_threads.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

parentPort.removeListener cannot remove listeners added via parentPort.on

When `parentPort.on('message', fn)` is called, `injectFakeEmitter.on()` wraps `fn` into a `callback` (setting `fn[wrappedListener] = callback`) and registers `callback` with `parentPortAddEventListener` — so `trackedByListener` is keyed on `callback`, not `fn`. When `parentPort.removeListener('message', fn)` is called, it invokes `parentPortRemoveEventListener('message', fn)` directly; `trackedByListener.get(fn)` returns `undefined`, the call returns early, and the listener is never removed. Thi

Check failure on line 197 in src/js/node/worker_threads.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

messageerror-only listener installs message forwarder, leaking event loop and dropping messages

When a worker sets only `parentPort.onmessageerror = handler` (or calls `parentPort.on('messageerror', fn)` / `parentPort.addEventListener('messageerror', fn)`) without registering any `message` listener, `acquireListener()` fires and `installForwarders()` unconditionally installs BOTH the `messageForwarder` AND `messageErrorForwarder` on `self` with `capture: true`. Installing the `message` forwarder on `self` increments `m_messageEventCount` in `BunWorkerGlobalScope.cpp` and calls `refEventLoo

Check warning on line 36 in test/regression/issue/29211.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

cleanStderr filter too narrow causes false ASAN CI failures

The `cleanStderr` helper (lines 30–36) only strips lines containing `ASAN interferes with JSC signal handlers`, but ASAN builds can emit additional output — leak reports, AddressSanitizer summary headers, and stack traces — that slips through the filter. All 7 tests then assert `cleanStderr(stderr) === ""`, which will cause false CI failures on ASAN builds whenever any unfiltered diagnostic appears. The project-wide convention from PR #28694 is explicit: do not assert spawned subprocess stderr i