Repository navigation
test: start the exit timer in worker-terminate-lifetime.test.ts once the fetches have responded - #45600
Conversation
…hes have their responses, not a fixed time after it started
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — test-only de-flake that moves the worker's exit timer behind the condition the test is actually about (all 8 fetches responded and had response.body touched), and surfaces fetch rejections instead of swallowing them.
Extended reasoning...
The change touches only test/js/web/workers/worker-terminate-lifetime.test.ts (one eval'd worker body, lines 1027-1038) and the comment on this file's entry in test/flaky-tests.txt; no runtime or security-sensitive surface is involved. The remaining 150-210 ms setTimeout is a deliberate dwell with the streams alive before exit rather than a wait for a condition, so the inline note is a nit. I checked the hang concern: a fetch that never settles now keeps the worker alive instead of exiting 3, but the per-test timeout (20s, 60s on slow lanes) still fails the test, so it degrades to a slower failure, not a false pass. The new flaky-tests.txt reason names a real sibling test in the same file ("a worker ends while a stream is still being piped into a FileSink", line 1218), so the file staying listed is consistent. Neither changed file is covered by .github/CODEOWNERS, and the PR reports CPU-quota runs where main failed and this branch passed.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/js/web/workers/worker-terminate-lifetime.test.ts— nit: the worker still exits on a 150-210 ms timer, so maintainers keep a time-based wait the PR title says it removes. At test/js/web/workers/worker-terminate-lifetime.test.ts:1036 the exit is scheduled with setTimeout(..., 150 + ...) after the 8th fetch responds, and the comment at line 1035 does not say why no observable signal exists. Fix: either await an observable condition before exiting (for example the first rd.read() of each fetch resolving, which shows the response stream is flowing) or keep the timer with a one-line comment naming why no signal exists for "the streams have run for a while". The repo review rules ask for that comment on any literal setTimeout of 50ms or more outside a poll loop. [also at: test/js/web/workers/worker-terminate-lifetime.test.ts:1036 - pre-existing nit: maintainers still get a bare 150-210 ms timer before the worker exits, with no comment saying why no observable signal exists.]Why this was flagged
Each eval'd worker in the test at test/js/web/workers/worker-terminate-lifetime.test.ts:1012 schedules its exit at line 1036 with setTimeout(() => process.exit(0), 150 + (i*13)%60) once all 8 fetches have resolved. That literal delay is 150 to 209 ms and sits outside any bounded poll loop. The only comment, line 1035, reads "Exit with that state alive in every fetch." and does not name why no observable signal (such as the rd.read() promise from line 1034 resolving) can replace the timer. The base branch had the same timer at the old line 1037, so the delay itself is pre-existing; the diff rewrites the line and its comment, and the PR title says the test now waits for the fetches rather than 150 ms, which is only half true. The repository review rules state that a literal sleep/setTimeout of 50ms or more outside a bounded poll loop needs a comment naming why no observable signal exists.
Verification: nit — triggers on every run of the test, since the exit is still gated on a wall-clock timer. At test/js/web/workers/worker-terminate-lifetime.test.ts:1036 the diff keeps a literal 150–202 ms delay outside any bounded poll loop. The only comment, line 1035, says what the exit does, not why no observable signal exists. The base had the same timer, so this is partly pre-existing.
dylan-conway
changed the title
There was a problem hiding this comment.
LGTM — small test-only de-flake that keeps the property the test protects (worker exit with streaming request bodies and touched response.body) and makes the state stronger (all 8 fetches, not at least one).
What was reviewed:
- Exit ordering in the worker:
process.exit(0)is only scheduled after the 8th.then, and a rejection can only fire before that (each promise is settled once), so exit(3) and exit(0) cannot race. - A fetch that never settles now surfaces as the file's 20s/60s test timeout rather than a quick exit 3 — bounded, and the old 150ms cutoff is what caused the spurious failures.
- The retained 150–210ms timer is commented with why no signal exists (exits are meant to land mid-transfer); the
test/flaky-tests.txtentry still lists the file, now citing the FileSink test that exists at line 1219 of the same file.
Extended reasoning...
The diff touches two test files only: it moves the worker's exit timer in one test of test/js/web/workers/worker-terminate-lifetime.test.ts from worker start to after all 8 fetches have resolved and touched response.body, turns a swallowed fetch rejection into a logged exit 3, and rewrites the reason text of the file's existing test/flaky-tests.txt entry. No production code, no security-sensitive surface, and no CODEOWNERS entry covers these paths. The logic is small enough to verify by reading: the exit(0) gate and the exit(3) catch are mutually exclusive, a hang is bounded by the test's existing timeout, and the state at exit is a superset of what the original test asserted. The second commit added the justifying comment after the earlier COMMENTED review, so the prior feedback appears addressed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe worker teardown test now waits for eight fetch responses to be touched and reports fetch failures. The flaky-test record now describes a worker ending during a stream-to- ChangesWorker fetch test
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main test change: delaying the worker exit timer until the fetches have responded. |
| Description check | |
The description includes both required sections. It explains the test change, retained flaky-test entry, verification results, and known verification limitation. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
|
Updated 5:05 PM PT - Oct 16th, 2026
@dylan-conway, your commit 2421a68 is building: |
|
Updated 6:05 PM PT - Oct 16th, 2026
❌ @dylan-conway, your commit 2421a68 has some failures in 🧪 To try this PR locally: bunx bun-pr 45600That installs a local version of the PR into your bun-45600 --bun |
What does this PR do?
Fixes the flaky "worker exit with streaming-request-body fetches whose response.body was touched" in
test/js/web/workers/worker-terminate-lifetime.test.ts. It fails withworker exited 3on the macOS x64 lane: in the 328 recent builds I read, it failed every retry in 64 and passed on a retry in 35 more.Each of the 6 workers starts 8 fetches against one server and exited on a timer, 150 to 202 ms after it started, with code 3 if no response had arrived by then. On a busy machine none has.
The timer now starts when all 8 fetches of the worker have their response and have touched
response.body, which is the state the test is about. Its length is unchanged, so at exit the streams have run at least as long as before, and all 8 fetches are in that state instead of at least one. A fetch that fails is now printed and fails the test; it was swallowed before.test/flaky-tests.txtkeeps the file, with the reason that is left: "a worker ends while a stream is still being piped into a FileSink" crashed with a segmentation fault on a libuv-worker thread in 4 builds on Windows. That is a bug in Bun, not in the test, and this PR does not touch it.How did you verify your code works?
With the Linux x64 binary that CI built for main (f8bc171), under
systemd-run --user --scope -p CPUQuota=N, 10 runs each:worker exited 3worker exited 3The whole file passes without a quota. Not checked: that the test still fails on a runtime without the fix it was written for (#38457), which needs an ASAN build of that commit.