Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 4:07 PM PT - Jul 28th, 2026
❌ @robobun, your commit 2b62faa has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36207That installs a local version of the PR into your bun-36207 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate. #33704 touches many files and its only change to this one is the debug/ASAN threshold; this PR is scoped to |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup with verifiably correct simplifications and strictly stronger assertions.
What was reviewed:
- Confirmed "shutdown after timeout" and "gently close" had byte-identical bodies (
socket.end()/ no-op) — dropping one loses no coverage. - Confirmed the batch loop only ever indexes
objects[0..47], so shrinking allocation fromtotaltobatchis dead-code removal. - The debug/ASAN threshold branch follows the repo convention (branch per build type, release lanes keep the full 16k/8k threshold);
isASAN/isDebugboth exist in harness. - Checked that the new
serverReceived === issued/bodyMismatches === 0assertions don't add fragility beyond the pre-existing body-parse logic — they turn a silentconsole.loginto a real assertion.
Extended reasoning...
Overview
Single-file test change to test/js/web/fetch/fetch-tcp-stress.test.ts. Four modifications: (1) removes one of two byte-identical test variants and collapses the remaining three into a table, (2) shrinks the objects array from total (up to 32k) entries to batch (48) since only indices 0..47 are ever read, (3) branches PORT_EXHAUSTION_THRESHOLD on isASAN || isDebug to 2048 while keeping release at 16k/8k, (4) replaces a console.log("Data is missing!") with counted assertions on serverReceived/bodyMismatches/initialMaxFD.
Security risks
None. Test-only; no runtime code, no network egress beyond localhost, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. The mechanical changes (duplicate removal, array shrink, table refactor) are directly verifiable from the diff and I confirmed each. The threshold branch is a judgment call, but it follows REVIEW.md's explicit guidance ("branch per-platform rather than dropping precision", "thresholds branch on isASAN/isDebug") and the PR description gives a sound argument: at debug/ASAN speed the ephemeral-port window drains faster than it fills, so only the fd-leak check does work — and 2048/48 ≈ 43 batches is still far past the point where a per-batch fd leak would blow the +10 margin. Release lanes are unchanged, so the port-exhaustion coverage that matters is preserved.
Other factors
The tightened assertions are the only place new flakiness could enter. I traced whether bodyMismatches++ could fire on TCP-fragmented requests: if the body marker were split across data calls, the pre-existing objects[i].body dereference would already throw on undefined.body, so the new assertion adds no fragility beyond what the test's structure already assumes. serverReceived === issued will now fail loudly if a connection is dropped where Promise.allSettled previously swallowed it — that's the stated intent and matches REVIEW.md's "assert the strongest invariant". The test.todoIf(isCI && isMacOS) gate on the historically-flaky lane is preserved. PR reports stable timings across three runs on three lanes (Windows aarch64 release, Linux debug+ASAN, Linux release).
There was a problem hiding this comment.
LGTM — both earlier nits are addressed in the current diff (change-history comment gone, initialMaxFD dropped from the toEqual).
What was reviewed:
- Confirmed the removed "shutdown after timeout" case was byte-identical to "gently close" in the old code; the three surviving variants match the originals exactly.
- Verified
objects[]is only ever indexed by0..batch-1(both in the fetch loop and via the parsed marker on the server side), so sizing it tobatchis safe. - Checked the new
serverReceived === issued/bodyMismatches === 0assertions can't introduce flakiness the old test didn't already have — a mismatch previously lefttoClosenon-zero and hung onpendingClose, so this converts a 30s timeout into a fast failure. isASAN/isDebugthreshold branching matches harness exports and repo convention; release lanes keep the full 16k/8k count.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch-tcp-stress.test.ts: drops one of four test variants that was byte-identical to another, collapses the remaining three into a table-driven loop, sizes the objects[] array to batch (48) instead of total (up to 32768), branches PORT_EXHAUSTION_THRESHOLD on isASAN || isDebug, and replaces a console.log("Data is missing!") with counted assertions on serverReceived and bodyMismatches. No production code touched.
Security risks
None. Test file only; no auth, crypto, or user-input handling.
Level of scrutiny
Low-to-moderate. The two things that could go wrong with a stress-test cleanup are (a) silently weakening coverage and (b) introducing flakiness. On (a): release builds keep the exact same iteration count and the removed variant was verifiably a duplicate — I diffed the old "shutdown after timeout" and "gently close" bodies in the removed hunks and both are socket.end() / no-op. The debug/ASAN reduction is justified per REVIEW.md's "shrink the workload" guidance and the PR's reasoning (TIME_WAIT drains faster than accumulation at debug speed) is sound; 4096 requests across ~86 batches is ample for the getMaxFD() < initialMaxFD + 10 leak check. On (b): the new serverReceived === issued assertion codifies what await pendingClose.promise already required — if a body ever failed to match, the old test never wrote a response, toClose never reached 0, and it timed out at 30s. So the tightened assertion converts an existing timeout-failure into a fast failure with a clear message rather than adding a new failure mode.
Other factors
Both nits from my prior review pass are resolved in the current diff (the vacuous initialMaxFD: expect.any(Number) clause and the change-history comment). isASAN and isDebug are confirmed exports from test/harness.ts. The objects[] sizing change is provably correct: the fetch loop indexes objects[0..47] and the server-side index is parsed from a marker of 10 + i where i came from that same 0..47 range. test.todoIf(isCI && isMacOS) and the 30s per-test timeout are preserved unchanged.
There was a problem hiding this comment.
LGTM — both prior nits addressed (change-history comment removed, vacuous expect.any(Number) clause dropped).
What was reviewed:
- Confirmed the removed "shutdown after timeout" variant was byte-identical to "gently close" — no coverage lost.
- Verified the batch loop and server
datahandler only ever indexobjects[0..47], so sizing the array tobatchinstead oftotalis safe. - Release-lane threshold is unchanged; only debug/ASAN is reduced, with the fd-leak assertion still meaningful at 2k iterations.
- New
serverReceived === issuedassertion strictly tightens what was previously aconsole.log— no new flake vector vs. the pre-existing data-handler logic.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch-tcp-stress.test.ts. Four things: (1) drops one of four test variants that was byte-identical to another, (2) sizes the objects request-options array to batch (48) instead of total (up to 32k) since only indices 0..47 are ever read, (3) scales PORT_EXHAUSTION_THRESHOLD to 2k under isASAN || isDebug while leaving release/macOS thresholds untouched, and (4) replaces a diagnostic console.log with counted serverReceived/bodyMismatches asserted against issued. The three surviving variants are collapsed into a table + loop.
Security risks
None. No production code touched; test uses a local Bun.listen({ port: 0 }) server over loopback.
Level of scrutiny
Low-to-moderate. It's test infrastructure, but REVIEW.md is explicit that deleting/weakening tests needs justification. Checked each axis:
- Deleted variant: the pre-PR "shutdown after timeout" and "gently close" bodies were both
{ onServerWritten: s => s.end(), onFetchWritten: () => {} }— genuinely identical, so nothing is lost. - Threshold reduction: only applies under debug/ASAN, where per the PR's timings the file previously timed out 4/4 at 30s. Release lanes keep 16k/8k. The rationale (debug iterates ~30× slower so TIME_WAIT drains before the ephemeral range fills, leaving only the fd-leak check doing real work) is sound, and 2k × 48-per-batch iterations is well past the point where a per-batch fd leak would blow the
+10margin. - Array sizing: both consumers of
objects[i]— the fetch loop (i < batch) and the server handler (index parsed from a two-digit marker10..57) — stay within[0, batch). - Tightened assertion: the
datahandler's parse/match logic is unchanged from before; only the side effect on match changed fromconsole.logto a counter. If TCP fragmentation over loopback were splitting these ~30-byte bodies, the old test would already have been hanging (socket never markedread), so this doesn't introduce a new flake mode — it makes an existing silent failure loud.
Other factors
I left two nits on the previous revision (vacuous expect.any(Number) clause; change-history comment). Both were addressed in 74b3ba9 and 8e879b7 and the threads are resolved. No outstanding reviewer comments. No CODEOWNER on this path. The bug-hunting pass on the current revision found nothing.
… scale threshold under debug/ASAN The "shutdown after timeout" and "gently close" test bodies were byte-identical (both socket.end() then no-op), so one quarter of the file's wall time was re-running the same test. The request-options array was sized to `total` (32k entries on release) but the batch loop only ever indexes 0..47, so 32k-48 of those objects were dead weight built with per-entry string concat. Under debug/ASAN the loop iterates ~30x slower so TIME_WAIT sockets drain faster than they accumulate and the 16k threshold only costs time; use 2k there so the fd-leak assertion and every close path still run but the file completes instead of timing all four tests out at 30s. Also assert the server actually received every issued request and that no body mismatch was observed, instead of logging and carrying on, and collapse the variants into a table.
The PR description already records why the fourth variant was removed; per REVIEW.md comments carry durable content, not commit history.
initialMaxFD is always a Number; the >=0 assertion on the next line is the real capture check.
8e879b7 to
2b62faa
Compare
There was a problem hiding this comment.
LGTM — both earlier nits (the vacuous expect.any(Number) clause and the change-history comment) are addressed.
What was reviewed:
- Confirmed the dropped "shutdown after timeout" test was byte-identical to "gently close" — no coverage lost.
- Traced
objects[i]usage: only indices 0..47 are ever referenced (client loop and server-side BYTEMARKER parse), so sizing tobatchis correct. - Checked the new
serverReceived === issuedassertion for over-count risk on chunkeddataevents — the single-chunk assumption is pre-existing (old code would throw onobjects[NaN].body), not introduced here. - Release thresholds and
todoIf(isCI && isMacOS)are preserved; only debug/ASAN gets the 2k branch.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch-tcp-stress.test.ts. Four things: (1) removes one of two byte-identical test cases, (2) reduces the objects allocation from total (~32k) to batch (48) since only those indices are ever read, (3) branches PORT_EXHAUSTION_THRESHOLD on isASAN || isDebug to 2k so the file doesn't time out under bun bd test, and (4) replaces a diagnostic console.log with counted assertions (bodyMismatches === 0, serverReceived === issued, initialMaxFD >= 0). The four test bodies are collapsed into a three-entry variants table.
Security risks
None. No runtime code touched; the test uses a local Bun.listen server on 127.0.0.1 port 0.
Level of scrutiny
Low-to-moderate. The main REVIEW.md concern for this class of change is silently weakening a test. I checked each axis: the deleted test is a verified exact duplicate (both s.end() server + no-op client); the release-lane iteration count is unchanged; the debug/ASAN reduction is the repo-endorsed pattern (branch per build type rather than raise the timeout) and 2k iterations × 48/batch is still >40 batches, well past where a per-batch fd leak would blow the initialMaxFD + 10 bound. The assertions are strictly stronger than before — the old code just printed to stdout on a body mismatch.
Other factors
I previously left two nits on this PR (a vacuous expect.any(Number) matcher clause and a change-history code comment); the author removed both in follow-up commits and the current diff reflects that. I looked at whether the new serverReceived counter could flake via multi-chunk data events — it relies on the request arriving in one TCP chunk, but the pre-existing code already depended on that (it dereferences objects[i].body where i is parsed from the chunk with no buffering, so a split chunk would already have thrown). The PR does not introduce that fragility, and the author reports stability across three runs per lane. issued correctly tracks batch per outer iteration, matching what the inner loop actually sends even when remaining < batch on the final pass.
|
CI status: |
Speeds up
test/js/web/fetch/fetch-tcp-stress.test.tswithout weakening what it covers.What changed
socket.end()on the server, no-op on the client). That is one quarter of the file's wall time spent re-running the same work. The three remaining variants are collapsed into a table.objects[0..47], so allocatingtotalentries (32768 on release) with per-entry string concatenation was dead weight.bun bd test. At 2k the fd-leak assertion and every close path still run. Release builds keep the full 16k (8k on macOS) threshold.fetch()calls issued, so a dropped connection fails loudly instead of the oldconsole.log("Data is missing!"). Also assertsinitialMaxFDwas actually captured.Why this is correct
The port-exhaustion aspect of the test is only meaningful at release speed, so the release threshold is unchanged. Under debug/ASAN the loop is slow enough that the ephemeral-port window can never fill, so only the fd-leak assertion and the close-path coverage do anything; 2k is well past the point where a one-fd-per-batch leak would exceed the
+10margin. No coverage is dropped: the removed test was an exact duplicate.Running the three variants with
test.concurrentwas tried but gives no wall-time improvement on Windows aarch64 (each test went from ~7s to ~20s, same total) and makes the per-testgetMaxFD()check racy, so the file stays sequential.Timings
bun bd test)Stable across three consecutive runs on each.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.