Repository navigation
test(worker-terminate-lifetime): exit the fetch-teardown fixture on its condition, not on a clock - #40781
test(worker-terminate-lifetime): exit the fetch-teardown fixture on its condition, not on a clock#40781robobun wants to merge 2 commits into
Conversation
…ts condition, not on a clock The worker in "worker exit with streaming-request-body fetches whose response.body was touched" exited 150 to 210 ms after it started and reported exit code 3 when no fetch had answered by then. A burst of 48 loopback connections takes 100 ms or more to establish on the macOS x64 CI hosts, so the deadline passed before the state the test tears down existed, and the test went red there in every parallel batch. The worker now exits once all eight responses have been touched. The per-worker offset is kept, anchored on that point. A fetch rejection and a generous bound (half the test timeout) still exit 3, with the reason on stderr, so a real failure is reported instead of a hang.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Status: reproduced on a macOS x64 CI host with the CI binary (old fixture fails 10 of 10, new one passes 10 of 10, both before and after a host reboot). Self-review done: the per-worker exit offset was dropped as a no-op. CI: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean move from a clock-driven exit to awaiting the actual condition the test tears down.
What was reviewed:
- Template-literal nesting:
\${J},\${i},\${(i * 13) % 60}resolve in the-escript scope; the unescaped${timeout / 2}correctly resolves at test-file scope wheretimeoutis 20_000/60_000. - Confirmed this is a wait-for-condition fix, not a timing shuffle to dodge a UAF — the awaited state (all J responses touched while request bodies still stream) is exactly the teardown state the test exists to exercise, and the PR verifies the unfixed
ProducerHoldstill crashes the new fixture under ASAN. - fetch rejections now fail loud (exit 3) instead of being swallowed; the
timeout / 2fallback is a diagnostic hang-guard, not a condition wait.
Extended reasoning...
Overview
This PR touches a single test file, test/js/web/workers/worker-terminate-lifetime.test.ts, reworking one fixture ("worker exit with streaming-request-body fetches whose response.body was touched"). The old fixture had each worker start 8 duplex fetches and then process.exit after a fixed ~150ms + jitter, exiting 3 if no response had been touched. On slow macOS x64 CI hosts the 48 loopback connections take longer than 150ms to establish, so the worker exited before the state under test existed. The new fixture hoists concurrency into J = 8, exits once all J responses are touched (keeping the per-worker jitter so exits still land at varied points in the 5ms enqueue cadence), fails loudly on fetch rejection, and adds a timeout / 2 fallback that prints which worker stalled before exiting 3.
Security risks
None. Test-only change to a spawned fixture string. No new network endpoints, no external hosts, port: 0 preserved, no credentials or filesystem writes.
Level of scrutiny
Low-to-moderate. It's a test-only de-flake with no production code touched. The two things worth checking were (1) whether this is the "changing a test so GC runs at a different time" anti-pattern from CLAUDE.md rule 15, and (2) whether the nested template-literal interpolation is correct. On (1): the change awaits the exact condition the test is designed to tear down (touched response body + still-streaming request body), and the PR description documents that removing the ProducerHold ref still crashes the new fixture 3/3 under debug ASAN — so coverage is preserved and this is not papering over a real failure. On (2): the escaping is correct — \${J}/\${i}/\${server.url} evaluate in the -e script's scope where those bindings exist, and the one unescaped ${timeout / 2} evaluates in the test file where timeout (line 12, slow ? 60_000 : 20_000) is defined.
Other factors
The change directly implements the repo's own test guidance ("do not use setTimeout to wait for a condition; await the event itself"). The old .catch(() => {}) swallow is replaced with a logged exit(3), which aligns with REVIEW.md's "wire every failure event to fail the test." The fallback setTimeout at timeout / 2 is a diagnostic hang-guard (fires well before the test-runner timeout), not a condition wait. No CODEOWNERS entry covers this path, and there are no outstanding reviewer objections in the timeline.
The teardown state is complete at the eighth touched response. The offset could not move a stream across any state boundary, and the exit already lands at a random point in the 5 ms enqueue cadence.
This branch has not been deployed
1 participant
Problem
test/js/web/workers/worker-terminate-lifetime.test.ts"worker exit with streaming-request-body fetches whose response.body was touched" is red on the macOS x64 lane in every parallel batch since build 107620, and failed alone in 107646:expect(stderr).toBe("")received"worker exited 3\n".setTimeout(() => process.exit(touched > 0 ? 0 : 3), 150 + (i * 13) % 60)(line 996 on main). On a macOS x64 CI host with hours of uptime, a burst of 48 loopback connections takes 130 to 800 ms to establish, so no fetch had answered when the deadline fired. The fixture came from Worker teardown: more fixes from fuzzing terminate()/process.exit() lifetimes; WebKit bump for Atomics.wait #38457.Fix
ProducerHoldremoved locally, both fixtures crash 3 of 3 under the debug ASAN build, so the new fixture covers the same teardown path. Whole file passes on the debug ASAN build.Background
ProducerHold(src/runtime/webcore/ByteStream.rs) is the fetch tasklet's counted ref on the response stream's native source. It lets the tasklet unhook itself at worker exit without reading a JS cell that the VM's last sweep may already have destroyed.netstat -sp tcpshowed about 137 retransmit timeouts per 100 loopback connects, and a node client against a node server saw the same delay. After the host rebooted, the same 48 connects took 10 ms. So the delay is host state, not bun.Notes
fetch()calls from the main thread to a localBun.servebefore the reboot: linux 18 ms, macOS x64 host 84 to 788 ms. Node client to Node server on the same host: 464 to 569 ms. Single connections were 1 to 17 ms there. After the reboot: 10 ms, and the old fixture passed 5 of 5. The new fixture passed 10 of 10 in both host states.dns.lookup("localhost")on the host takes 3 ms, so the DNS path (DNSServiceGetAddrInfoEx) is not involved.(i * 13) % 60offset, anchored on the eighth response. The self-review showed it changes nothing: all eight producer holds exist before the timer is armed, the only later stream transition (park at 256 KB buffered) is about 2.5 s away at 512 B per 5 ms, and the exit already lands at a random point in the 5 ms cadence. The second commit drops it. The unfix check was repeated with offset 0: 3 of 3 crash.ProducerHold::holdwithoutincrement_count, and a balancingincrement_countintake. Old and new fixture both hitASSERTION FAILED: decontaminate()inStructureID::decode()3 of 3 underbun bd.node_fs_binding::Bindingbox, not touched by this change (node:fs: mark the per-VM Binding box as LSan-ignored (fixes worker-terminate-lifetime.test.ts on main) #35159, node:fs: free the per-VM NodeFS when a worker's VM is torn down #39684 cover it).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts