Repository navigation
Conversation
|
Updated 9:18 AM PT - Oct 15th, 2026
✅ @robobun, your commit f5e9fba0e31eb857f1fe52cda1c94831265f9c34 passed in 🧪 To try this PR locally: bunx bun-pr 44927That installs a local version of the PR into your bun-44927 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe fixture measures idle CPU across up to three consecutive windows. Busy windows trigger another measurement until the limit. It records an idle verdict after a low-CPU window, or records CPU time and the number of windows when measurement ends. ChangesIdle CPU measurement
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 identifies the test fixture and the change to remeasure the EPOLLHUP idle window after teardown. |
| Description check | |
The description explains the problem, fix, background, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections. |
- 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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… fills it The fixture of "AF_UNIX CONNECT sockets whose peer closes first do not spin the loop on EPOLLHUP" reads process.cpuUsage() over one window of 1000 ms and reports a spin at 500 ms or more. The hangup closes the 8 server sockets inside that window. A debug build spends 370 to 550 ms of CPU on that teardown, so the fixture reported a spin with an idle loop in 7 of 32 runs. The fixture now measures a busy window again, for at most 3 windows. The first idle window gives "idle". A spin burns the whole of every window, so it still gives "spun". A first window that is idle gives the same result in the same time as before.
The first window keeps its limit of half a window. A window that the fixture measures after a busy one has no teardown left in it, so its limit is now a quarter. With half in each window, a spin on a loaded machine passed in a later window. On a debug build with the EPOLLHUP handling removed, with 18 or 24 busy loops on 12 cores, that rule reported a spin in 4 of 40 runs. One window of the fixture on main reported it in 28. With a quarter in the later windows, each of the 40 runs gives the result of the one window.
b013b3b to
f5e9fba
Compare
There was a problem hiding this comment.
LGTM — the second commit addresses the loaded-machine false-pass I raised on the first version: later windows now hold to WINDOW_MS / 4, so a spin throttled to half a core no longer lands on the idle branch.
What was reviewed:
- Traced measure(1..3) in test/js/node/http/node-http-connect-unix-hangup-fixture.js: the first window keeps the 500 ms limit, windows 2-3 use 250 ms, the verdict is set exactly once, and report() still waits for both the verdict and N closes before printing.
- Checked the test in test/js/node/http/node-http-connect.test.ts (lines 1234-1253) still matches: stdout ends in "idle" on pass, and a spin yields "spun ... 3 windows in a row" with a different stdout, so the assertion still fails on the unfixed loop.
- maybeMeasure() is still entered once (both counters reach N at a single event and no later client/connect event fires), so measurement chains cannot overlap.
- Cost: an idle first window takes the same 1 s as before; only a busy first window extends the fixture to 3 s.
Extended reasoning...
The PR changes only one spawned test fixture (12 insertions, 2 deletions): it re-measures a busy CPU window up to 3 times, keeping the 500 ms limit for the first window and tightening later windows to 250 ms. It touches no runtime code and no security-sensitive surface. The second commit resolved the only concern from the prior review (later windows could pass a throttled spin), the file is not covered by CODEOWNERS, and the hunt ran dry with no findings. The change is small, self-contained, and its fail-on-bug behavior traces cleanly through the fixed constants.
Problem
spun 524ms cpu in 1000ms.test/js/node/http/node-http-connect-unix-hangup-fixture.js:31). The hangup closes the 8 server sockets inside it. A debug build spends 370 to 550 ms at 71d0d43, 193 to 228 ms at 5f5799e.Fix
EPOLLHUPhandling removed. 3 of 3 runs fail. Under load, 40 of 40 runs give the verdict of the one window on main (18 or 24 busy loops, 12 cores).node-http-connect.test.ts -t "AF_UNIX CONNECT", 0 of 20 debug runs fail. An idle first window gives the same result in the same time. Alsonode-net-allowHalfOpen.test.js.Background
EPOLLHUPfor a socket with both directions down, on eachepoll_waitwhile the fd is registered. The loop never sleeps. usockets: close half-open sockets on EPOLLHUP instead of spinning on it #37920 closes such a socket.Notes
All runs: Linux x64, 12 cores. Only the fixture changes. No file under
src/orpackages/changes.The false result (debug build of main at 71d0d43)
bun testthe test failed in 2 of 30 runs (spun 524ms cpu in 1000ms).300 96 9 1 1 1 25 28 3 2 ..., 561 ms in total. The 8 server sockets close 104 to 205 ms after the window starts. The CPU time is a fixed cost at the start, not a spin.Results at 5f5799e
spunin 6 of 6 (715 to 784 ms). This fixture:idlein 6 of 6, and in 6 of 6 with 18 busy loopsEPOLLHUPhandling removedspun 970ms cpu in 1000ms, 3 windows in a rowidleEPOLLHUPhandling removed" is one line inpackages/bun-usockets/src/eventing/epoll_kqueue.c:EPOLLHUPmaps toLIBUS_POLL_EOFand not toLIBUS_POLL_HANGUP. That turns off each hangup branch from usockets: close half-open sockets on EPOLLHUP instead of spinning on it #37920. No server socket closes then, and the loop uses one CPU.A spin on a loaded machine (debug build,
EPOLLHUPhandling removed)Each run records 3 windows. Each rule reads the same run. The cells count the runs that report the spin.
What the limit of 250 ms costs
Other notes
test/js/node/net/node-net-paused-unix-hangup-fixture.jsuses the same check and is not changed. Its sockets stay open during the window, so there is no teardown in it: 39 to 61 ms on the debug build of 71d0d43.EPOLLHUPhandling removed. At 71d0d43 each of its windows had 910 to 1000 ms, and 8 of 8 runs failed in 3.1 s. Not run at all: therelease-asanprofile.no test proof · iteration 4 · no src or test change; test-proof not applicable