test(worker): make the message flood test deterministic instead of racing worker boot by robobun · Pull Request #37355 · oven-sh/bun

robobun · 2026-08-11T00:24:35Z

Test-only change to test/js/web/workers/worker.test.ts.

Problem

"a message flood from a worker does not starve the parent's event loop" (added in #37075) starts three 10ms timer turns at new Worker(...) and then asserts received > 0. That makes it an assertion that the worker booted and posted its first message within roughly 30ms of construction, which has nothing to do with the property the test is for. On a debug+ASAN build the first message lands about 150ms after construction (release: 2-3ms), so the test fails every time:

$ bun bd test test/js/web/workers/worker.test.ts -t "message flood"
419 |       expect(received).toBeGreaterThan(0);
                             ^
error: expect(received).toBeGreaterThan(expected)

Expected: > 0
Received: 0
(fail) web worker > terminate() races and lifecycle edges > a message flood from a worker does not starve the parent's event loop [65.13ms]

4/4 runs fail on a debug build of main (23d233b), both in isolation and as part of the whole file. CI is green because its ASAN lanes are release builds, which boot a worker well inside the window; any slower machine or build turns it red. On the failing path the test also never reaches terminate(), so the flooding worker keeps running under the rest of the file.

The test also only exercised the thing it guards when the worker happened to outrun the parent. WorkerMessagingProxy::drainMessagesToWorkerObject delivers at most drainBatchLimit (1024) messages per task and posts the rest to the next loop iteration; a drain that does not yield only shows up if the inbox never runs dry. Measured with the test's flood: on a release build the worker outruns the parent, on a debug build a worker posts about 3k messages/s and the parent keeps up, and on a release+ASAN build it is somewhere in between (review measurement: bounded batches in about 5 of 13 runs). So on debug builds a drain with no bound passed the test, and where the test did detect one, the failure was the whole file hanging, since the runner's per-test timeout lives on the loop being pinned.

Fix

The backlog is now built deterministically instead of by racing the worker. The parent creates a shared flag, posts it to the worker, and blocks in Atomics.wait while the worker posts 2 * 1024 + 1 messages and then sets the flag (the flag itself is asserted afterwards: the wait returns "not-equal" rather than "ok" if the worker was already done, and a worker that never gets there is an assertion failure instead of a hang). When the parent resumes, everything is already in its inbox, so the first drain task starts with more than its budget on every build, however fast or slow either thread is, and the continuation it posts still has more than its budget too, so it has to yield and re-post as well.

The test then resumes inside the first drain task (await once(w, "message")) and re-arms an immediate until a tick delivers nothing new, recording the count after each one. An immediate armed during a task runs at the start of the next tick, before the continuation that task posted is promoted, and microtasks drain right after the immediate callback, so each immediate sees exactly one more task's worth of messages. The recorded counts are [1024, 2048, 2049, 2049] on every build (95/95 iterations across release and debug+ASAN, idle and with the machine oversubscribed by 32 busy loops). The test asserts that sequence ([budget, 2 * budget, total, total]; the total is derived from the budget anyway, so pinning the batch size adds no coupling) and that every message arrived, in order. The flood is finite, so every failure mode is an ordinary assertion failure. Checked with three local mutations of WorkerMessagingProxy.cpp on the debug build, each of which fails at the sequence assertion:

first drain task made unbounded (DrainBudget::UntilEmpty in postMessageToWorkerObject): [2049, 2049]
continuation made unbounded (UntilEmpty in the re-posted task): [1024, 2049, 2049] (the 1500-message revision of this test passed this mutation)
re-post of the remaining messages removed: [1024, 1024] (previously a hang)

The per-message microtask checkpoint on the same path is already covered by "round-trip burst delivers in order with microtasks between each" in message-port-pipe.test.ts, so this test does not repeat it.

Verification

Debug+ASAN build (where the old test fails 4/4): passes in isolation (~500-900ms each on this loaded machine) and within the whole file. Release build: passes, ~8ms per run. The three mutations above each fail as listed on the debug build. The 1500-message revision of this test was green on all 190 CI jobs in build #92008; this revision uses the same mechanism with a longer backlog.

#37374 (debug-build timing of the other tests in this describe block) carries an earlier revision of this test; whichever lands first, the other rebases onto it.

…mer turns The "message flood does not starve the parent's event loop" test asserted received > 0 after three 10ms timer turns that started at Worker construction, so it also asserted that the worker had booted and posted within ~30ms. On a debug+ASAN build the first message lands ~150ms after construction and the test fails deterministically with received === 0, which says nothing about starvation. Wait for the first message before taking the timer turns, and after each turn wait for the next message, so the turns are taken while the flood is known to be running and the test no longer depends on boot latency. Under an unbounded drain the timers still never fire, so the test still covers the property it was added for.

coderabbitai · 2026-08-11T00:24:43Z

Walkthrough

The worker flood test now sends 2049 messages, synchronizes worker startup, checks delivery across event-loop turns, and verifies ordered receipt of all messages.

Changes

Worker test validation

Layer / File(s)	Summary
Message timing assertions `test/js/web/workers/worker.test.ts`	The flood test uses a fixed message count, waits for the first message, checks bounded intermediate delivery, and verifies ordered receipt across multiple drain tasks.

Suggested reviewers: dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely describes the deterministic worker message flood test change.
Description check	✅ Passed	The description explains the problem, implementation, and verification results in sufficient detail, although it uses different headings than the template.

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

robobun · 2026-08-11T00:25:41Z

Status: reproduced and fixed (test-only change), ready for review.

Reproduced how: on a debug+ASAN build of main (23d233b), bun bd test test/js/web/workers/worker.test.ts -t "message flood" fails 4/4 with Expected: > 0 / Received: 0 (the first worker message lands ~150ms after construction on that build; the test gave it ~30ms). With this branch (8946b9b) the test passes on that build, in isolation and within the whole file, and on a release build; each of three local mutations of the drain (first task unbounded, continuation unbounded, re-post removed) fails at the sequence assertion on the debug build, where the unmodified test passed the first one and could only hang on the others.

CI: the 1500-message revision (221aa81) was green on all 190 jobs in build #92008; 8946b9b (same mechanism, longer backlog, exact sequence) is running now. #37374 carries an earlier revision of this test; whichever lands first, the other rebases.

claude

LGTM — test-only de-flake that removes the boot-latency dependency; the inline nit about the vacuous > before assertion is worth a look but doesn't affect the test's ability to catch the regression (that's enforced by the timer await not hanging).

Extended reasoning...

Overview

This PR touches a single test in test/js/web/workers/worker.test.ts — the message-flood starvation test added in #37075. The old version raced three 10ms timers against worker boot, which on debug+ASAN takes ~150ms, so it failed 4/4 on debug builds and leaked a flooding worker into the rest of the file. The new version awaits the first message before starting the timer turns and awaits a message after each turn, so nothing depends on boot or delivery speed.

Security risks

None. Test-only change with no production code, no new inputs, no external network.

Level of scrutiny

Low. This is a targeted de-flake of one test with a clear mechanism (wait for the observable condition instead of a fixed window), thorough verification in the PR description (18/18 debug+ASAN, 30/30 release, oversubscribed machine), and the regression it guards is still caught — with an unbounded drain the setTimeout never fires and the test times out rather than passes.

Other factors

One nit was flagged: expect(received).toBeGreaterThan(before) is structurally guaranteed once await once(w, "message") resolves, since w.onmessage (registered earlier) fires in the same synchronous dispatch. That's a valid observation per REVIEW.md's "every assertion must be able to fail", but it doesn't change what the test enforces — the property is held by the awaits not hanging, which the PR description explicitly states. Strengthening to before + 1 or dropping the expect would be a one-line polish, not a blocker. The change is a strict improvement over main, which currently fails every debug run and leaks a worker on the failure path.

After `await once(w, "message")` the count has necessarily grown, so the assertion could not fail. The turns coming back and a message following each of them is the whole check; the message events are dispatched (and deserialized) by the drain whether or not a handler is installed.

robobun · 2026-08-11T00:52:55Z

^{Updated 3:57 AM PT - Aug 11th, 2026}

✅ @robobun, your commit 8946b9b7a6367bb41549996db1c7243256a32e4d passed in Build #92168! 🎉

🧪 To try this PR locally:

bunx bun-pr 37355

That installs a local version of the PR into your bun-37355 executable, so you can run:

bun-37355 --bun

robobun · 2026-08-11T00:54:40Z

Addressed the review nit in 82687bc: the received counter and its assertion are gone, since after await once(w, "message") the count had necessarily grown. The test is now just the waits: first message, then three times (10ms timer turn, next message). PR description updated with what the test does and does not detect on release vs debug builds.

claude

LGTM — the follow-up commit (82687bc) addressed the vacuous-assertion note by dropping received/onmessage/expect entirely, which was one of the two suggested fixes. The property is now enforced purely by the awaits not hanging.

Checked that the rewritten test still fails for the right reason: with an unbounded drain the setTimeout(10) after the first once(w, "message") never fires, so the test times out rather than passing. Also confirmed once from node:events works on a Web Worker (EventTarget path) and that the worker's setImmediate(burst) loop guarantees a next message is always coming, so once(w, "message") after each timer turn cannot spuriously hang on a correct build.

Extended reasoning...

Overview

Test-only de-flake of one test in test/js/web/workers/worker.test.ts. The original test asserted received > 0 after three 10ms timer turns started at new Worker(...), which conflated worker boot latency (~150ms on debug+ASAN) with the actual property (parent timers keep firing during a message flood). The fix waits for the first message before starting timer turns, and after each turn awaits another message to prove the flood was still running. A follow-up commit dropped the received counter and expect after my earlier note that the assertion was structurally guaranteed by the preceding await once(w, "message").

Security risks

None. Test-only; no src/ changes, no new external inputs, no network.

Level of scrutiny

Low. This is a targeted de-flake of a recently-added test (#37075) with a clear mechanical explanation (boot latency vs. 30ms window), reproduced 4/4 on debug and verified 18/18 + 30/30 after the fix per the PR body. The author took the "drop it" option from my prior review, which is the cleaner of the two — the comment on the loop already explains what the once() after each turn establishes, so there is no lost signal.

Other factors

The test now has no expect() in the loop body, but that's correct: it's a hang-guard test, and REVIEW.md's "await the actual observable condition" is exactly what it does. A regression (unbounded drain) makes the setTimeout await hang → test timeout.
The worker source uses setImmediate(burst) to keep posting indefinitely, so there is always a next message for once(w, "message") to observe after each timer turn — no race where the flood ends between the timer and the once.
terminate() + await once(w, "close") at the end still cleans up on the success path; on the (real-regression) hang path the worker leaks, same as before, but that path now only triggers on an actual bug rather than on boot latency — a strict improvement.

claude

LGTM — thanks for dropping the vacuous counter/assertion in 82687bc; the test is now purely the awaits, which is the right shape for a hang-guard.

What was reviewed:

Confirmed the property is still enforced: an unbounded drain starves the setTimeout await → test-runner timeout → fail; bounded drain lets all four once(w, "message") + three timer awaits return.
Checked once() from node:events on a Web Worker EventTarget wires error to reject, so a worker startup failure surfaces rather than hanging.
Considered the failure-path leak (worker not terminated on timeout): in the regression scenario the parent loop is starved, so the test-runner's own timeout timer is equally starved and the whole file hangs — no silent leak into later tests.

Extended reasoning...

Overview

Test-only de-flake of one test in test/js/web/workers/worker.test.ts: "a message flood from a worker does not starve the parent's event loop". The old version raced three 10ms timers against worker boot and asserted received > 0, which on debug+ASAN (~150ms boot) failed 4/4 for a reason unrelated to the property under test. The rewrite awaits the first message before starting, then per iteration awaits a 10ms timer followed by the next message. No src/ changes.

Prior review

I flagged an intermediate revision's expect(received).toBeGreaterThan(before) as vacuous (structurally guaranteed by the preceding await once(w, "message")). The author addressed this in 82687bc by removing the counter and assertion entirely, which was one of the two suggested fixes. That thread is resolved and the current diff reflects it.

Security risks

None. Test-only change to synchronization ordering; no new inputs, no runtime code touched.

Level of scrutiny

Low. This is a targeted flake fix in a single test, with a clear mechanical explanation (boot latency vs. fixed timer window), reproduced 4/4 fail → 28/28 pass on debug and 50/50 on release including under 32× CPU oversubscription. The regression the test guards (drainBatchLimit in WorkerMessagingProxy::drainMessagesToWorkerObject) is still detected: an unbounded drain would starve the setTimeout and the test would hang until the runner timeout.

Other factors

REVIEW.md's "await conditions, not time" and "every assertion must be able to fail" are both now satisfied — the test awaits observable events and contains no vacuous expect.
The one candidate concern raised this run (worker leaks past the test on the failure path it detects) was examined and ruled out: in the actual regression scenario the parent event loop is starved, so the test-runner's own per-test timeout timer is equally starved — the file hangs as a unit rather than the flooding worker silently poisoning later tests. On any other failure (worker error), once() on an EventTarget rejects on error and the test fails before terminate(), but that path is not the property under test and matches the pre-PR behavior.
once is already imported at the top of the file and used identically by neighboring tests in the same describe block.

…ng the worker Waiting for the first message removed the boot-latency dependence, but the test still only reached a bounded drain when the worker happened to outrun the parent: never on a debug build (a debug worker posts ~3k messages/s and the parent keeps up), and only some of the time on a release+ASAN build. On those builds a drain with no bound passed it, and where it did detect one the failure was the whole file hanging, since the test runner's timeout lives on the loop being pinned. Have the worker post more messages than one drain task may deliver while the parent is blocked in Atomics.wait, so the first drain task starts with more than its budget on every build. Resuming inside that task and arming an immediate observes exactly what it delivered before yielding: the immediate runs when the task ends, before the continuation the task posted is promoted, and microtasks drain right after it. The flood is finite, so a drain that does not yield now fails as an ordinary assertion (received equals the total) on a debug build too, verified by making the drain unbounded locally, and the rest of the messages arriving proves the yielded continuation still delivers them.

robobun · 2026-08-11T06:12:07Z

Restructured the test in d2a836e after a closer look at what the first rewrite actually exercised. Waiting for the first message fixed the boot-latency dependence, but the test still only reached a bounded drain when the worker happened to outrun the parent: never on a debug build (a debug worker posts ~3k messages/s and the parent keeps up), only some of the time on a release+ASAN build, so a drain with no bound passed it there, and where it did fail, it failed by hanging the file.

The backlog is now built up front: the parent posts a shared flag to the worker and blocks in Atomics.wait while the worker posts 1500 messages (more than one drain task's budget), so the first drain task starts with more than its budget on every build. Resuming inside that task and arming an immediate observes what it delivered before yielding (1024 in 30/30 release and 10/10 debug runs); the assertion is that it was some but not all of them. With the drain made unbounded locally, the debug build now fails at that assertion (Expected: < 1500, Received: 1500) instead of passing. PR description updated accordingly.

Assert the flag itself after Atomics.wait: the wait returns "not-equal" rather than "ok" if the worker finished before the parent got there, and that state satisfies the precondition just the same. Drop the > 0 check, which cannot fail after awaiting a message; received < total is the check.

claude

LGTM — 221aa81 addresses both remaining notes: the Atomics.wait result is no longer asserted directly (Atomics.load(flag, 0) === 1 accepts both "ok" and "not-equal"), and the vacuous toBeGreaterThan(0) is gone.

Checked that total = 1500 exceeds drainBatchLimit = 1024 in WorkerMessagingProxy.cpp:278, so the first drain task is guaranteed to hit its budget and yield; toBeLessThan(total) is the load-bearing assertion and await delivered.promise covers the continuation. The finite backlog means an unbounded drain now fails as an assertion rather than a hang.

Extended reasoning...

Overview

Test-only rewrite of a single test case in test/js/web/workers/worker.test.ts — "a message flood from a worker does not starve the parent's event loop". The original test raced worker boot latency against three 10ms timers and failed 4/4 on debug+ASAN builds; where it did exercise the bounded drain, an unbounded drain manifested as a file-level hang rather than an assertion failure. The rewrite builds a 1500-message backlog deterministically via Atomics.wait on a shared flag, then asserts that the first drain task delivered fewer than total before a setImmediate fires, and that the continuation delivers the rest.

Prior review threads

I left two inline notes on d2a836e: (1) .toBe("ok") on the Atomics.wait result would spuriously fail on "not-equal" if the parent were preempted long enough for the worker to finish before entry; (2) toBeGreaterThan(0) after await once(w, "message") was structurally vacuous. Commit 221aa81 addresses both: the wait result is now discarded and Atomics.load(flag, 0) is asserted to be 1 (which holds for both "ok" and "not-equal" and fails only on timeout), and the > 0 assertion is removed. The PR description was also updated to match.

Security risks

None. Test-only change with no production code touched.

Level of scrutiny

Low-to-medium. The change is confined to one test case, but the reasoning about event-loop task ordering (drain task → microtasks → immediate → drain continuation) is subtle. I verified against src/jsc/bindings/webcore/WorkerMessagingProxy.cpp: drainBatchLimit is 1024 and drainInbox with DrainBudget::Bounded caps per-task delivery there, so 1500 queued messages guarantees a yield. The PR description reports 1024 delivered at the immediate in 40/40 runs across release and debug, and a clean toBeLessThan failure when the drain is made unbounded locally — the test now fails for the right reason.

Other factors

The author has been responsive across three review rounds and each concern was addressed with a targeted commit. The test avoids sleeps-as-synchronization (the Atomics.wait timeout is a ceiling, not a delay), the flood is finite so cleanup is reached on every path, and the source comment names the constant it depends on. No outstanding reviewer concerns remain.

…ng worker boot Same change as #37355, carried here so that the whole file passes on a debug build; it drops out on rebase once that lands.

robobun · 2026-08-11T09:09:11Z

Heads-up: #37374 (debug-build timing of the other tests in this describe block) carries this PR's diff unchanged as its last commit, because without it the file cannot pass as a whole on a debug build. Whichever lands first, the other rebases cleanly: if this one lands first, the carried commit drops out over there; if #37374 lands first, this becomes a no-op.

…he flood test With 1500 messages only the first drain task hit its budget: the one continuation found 476 left and delivered them in one go, so a continuation with no budget (or one that never re-posts the rest) passed the test on every build. Post two budgets plus one, and record the count after each drain task by re-arming an immediate until a tick delivers nothing, which gives [1024, 2048, 2049, 2049] on every build. The first task and its continuation must each stop short of the total, and the messages must all arrive in order; making either drain unbounded, or dropping the re-post, now fails one of those assertions on a debug build instead of passing or hanging.

robobun · 2026-08-11T10:30:03Z

One more round in 8b04a99, from a closer look at coverage: with 1500 messages only the first drain task hit its budget, and the single continuation delivered the remaining 476 in one go, so a continuation with no budget, or one that never re-posts the rest, passed the test on every build (and the old infinite flood had at least caught the unbounded case on release, as a hang). The worker now posts 2 * 1024 + 1 messages, and the test records the count after each drain task by re-arming an immediate until a tick delivers nothing: [1024, 2048, 2049, 2049] on every build, 95/95 iterations including under load. It asserts that neither the first task nor its continuation delivered everything and that all messages arrived in order. Each of the three corresponding local mutations of WorkerMessagingProxy.cpp (either drain unbounded, re-post removed) now fails at a named assertion on the debug build; details in the description.

coderabbitai

Actionable comments posted: 1

🤖 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/js/web/workers/worker.test.ts`:
- Around line 441-442: Strengthen the assertions in the worker delivery test
around afterEachTask so they verify the documented drainBatchLimit batch size,
not merely that both values are below total. Assert the expected sequence of
delivered counts, preserving the existing checks for continuation yielding and
ordered delivery.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a2944662-4be1-40de-b66b-422cbdec981e

📥 Commits

Reviewing files that changed from the base of the PR and between 82687bc and 8b04a99.

📒 Files selected for processing (1)

test/js/web/workers/worker.test.ts

The test already derives its total from the budget, so comparing the whole sequence against [budget, 2 * budget, total, total] costs no extra coupling and also pins the batch size; the failure output then shows which task misbehaved.

claude

LGTM — the earlier review rounds have all been addressed and the final revision is a well-reasoned, mutation-tested de-flake.

What was reviewed:

Confirmed drainBatchLimit = 1024 in WorkerMessagingProxy.cpp:278 matches the test's budget constant, and that both the initial drain and its continuation use DrainBudget::Bounded.
Checked the do/while loop is bounded: the flood is finite (2049 messages, all in the inbox before the first drain), got.length is monotone, and .at(-2) is undefined on the first iteration so it always takes at least two.
Checked the worker does not exit naturally (its onmessage keeps it alive), so the UntilEmpty exit-path drain at WorkerMessagingProxy.cpp:532 cannot interfere with the per-task counts.
The prior Atomics.wait "not-equal" concern and the vacuous > 0 assertion are both resolved in the current diff.

Extended reasoning...

Overview

Test-only rewrite of one test in test/js/web/workers/worker.test.ts. The old test raced worker boot against three 10ms timers and failed 4/4 on debug+ASAN. The new test builds a deterministic 2049-message backlog by blocking the parent in Atomics.wait while the worker posts, then observes per-drain-task delivery counts via re-armed setImmediates and asserts the exact sequence [1024, 2048, 2049, 2049] plus in-order delivery of every message.

Security risks

None. Test-only change; no runtime code touched.

Level of scrutiny

Medium — it's test-only, but the test relies on two implementation details (the drainBatchLimit constant and setImmediate running before postTaskAfterYield continuations). I verified the constant against source (WorkerMessagingProxy.cpp:278), confirmed the coalescing via drainScheduled means exactly one initial drain task is posted regardless of how many messages arrive while the parent is blocked, and confirmed the worker stays alive (its onmessage listener holds the loop open) so the exit-path UntilEmpty drain cannot fire mid-test. The scheduling assumption is documented in the test comment and the author validated it empirically across 95 iterations on release and debug+ASAN, idle and under 32× oversubscription.

Other factors

This PR has been through four review rounds. My two prior findings (vacuous > 0 assertion, Atomics.wait returning "not-equal" treated as failure) were both addressed — the current diff asserts Atomics.load(flag, 0) === 1 instead of the wait's return value, and there is no lower-bound assertion after once(). CodeRabbit's suggestion to assert the exact per-task sequence was taken in 8946b9b, and the author re-ran the three drain mutations to show each fails at that assertion with a distinct diff. All review threads are resolved. The bug-hunting system found nothing on the current head.

The one thing I'd note is that w.terminate() runs after the assertions rather than in a try/finally, so a failing assertion leaks an idle worker — but this matches every other test in the same describe block and the worker is no longer flooding (unlike the old test), so it's a strict improvement and not worth blocking on.

robobun assigned dylan-conway Aug 11, 2026

github-actions Bot added the claude label Aug 11, 2026