Repository navigation
Conversation
A debug build needs 1 to 15 s for the 1000 streams of one flood. The tests ran with the 5 s default timeout. The bucket also regains 33 tokens per second, so a flood of 1200 resets that takes more than 6 s never empties it, and no GOAWAY comes. - flood() writes the pairs once more when the server answered the first write without a GOAWAY. A PING behind the pairs shows that. - The tests that have to send a whole bucket of 1000 resets get a 60 s timeout on debug builds. Other builds keep the default. - Two tests do not need a whole bucket. They send 100 streams now. - The flood that must be older than the server's GOAWAY cannot be written once more. It has 1700 streams, enough for a wait of 20 s.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe HTTP/2 conformance tests update reset-flood timing and batch sizes. They use debug-dependent waits and timeouts, revise reset and upload counts, and check reset accounting around malformed headers and GOAWAY. ChangesHTTP/2 reset conformance
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The changes improve debug-build flood-test tolerance while preserving reset-accounting checks. No actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on linux x64 with the debug build from
|
|
The automated review above has no actionable items, and no review threads are open. One correction to its walkthrough: this PR adds no test coverage. The refused header block test and the GOAWAY tests exist on main. The PR changes the flood helper, the timeouts on debug builds, and three stream counts. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- flood() writes the pairs a second time only when the first write took more than 6 s. A faster build must answer 1200 resets with the GOAWAY, as before. A bucket of 1500 passed the previous commit on a release build and fails now. - The tests with the default timeout keep their 10 s waits. Only the six tests with the debug timeout wait 20 s on a debug build. - The flood that must be older than the server's GOAWAY has 1700 streams on debug builds only. Other builds keep 1300.
|
The review has four comments. a888e0f changes the code for two of them, and the other two have an answer in their threads. All four threads are resolved.
The regenerated summary above has no actionable items. |
1200 resets pass a bucket of 1000 only if it regained 200 tokens, which takes 7 refills of 33. The comment said 199 tokens and 7 s at 33 per second.
This branch has not been deployed
1 participant
Problem
stream-reset floodstests intest/js/node/http2/h2-conformance.test.tsfail withthis test timed out after 5000msortimed out waiting for frame.RateLimit::drain,src/runtime/api/bun/h2/connection.rs:170). A flood of 1200 resets that takes more than 6 s never empties it.Fix
flood()sends aPINGafter the pairs. It writes the pairs again only if the ack comes with noGOAWAYafter more than 6 s. A faster build must send theGOAWAY.bun bd test test/js/node/http2/h2-conformance.test.ts(84 pass). With one CPU, main fails 6 to 7 of 14 tests in 4 of 4 runs. This branch passes 3 of 3.Background
GOAWAY(ENHANCE_YOUR_CALM).bun test --timeoutwith 90 s or more. Only local runs use the 5 s default.GOAWAY. Considered one larger write: two writes leave the first write unchanged.Notes
Reproduction (linux x64, debug build from
bun bd, main at 9f70da0):bun bd test test/js/node/http2/h2-conformance.test.ts -t "stream-reset floods": main fails 2 of 9 runs, withthis test timed out after 5000msin the first test (at 5204 ms and 7186 ms). In the other 7 runs that test takes 3.9 to 5.2 s. This branch (a888e0f) passes 3 of 3 runs, one of them with the first test at 7057 ms.taskset -c <cpu> ./build/debug/bun-debug test test/js/node/http2/h2-conformance.test.ts -t "stream-reset floods". Main: 7, 6, 6, 6 of 14 fail, withtimed out waiting for frameandUnhandled error between testsin every run. This branch: 14 pass, 3 of 3 runs.The arithmetic. The bucket is empty at reset
1001 + 33 * k, wherekis the count of whole seconds since the connection started.k <= 6. So a build that takes the pairs in less than 6 s must send theGOAWAY, andflood()fails if it does not. A probe that sends the same frames with one CPU: the first write took 13.6 s (RST_STREAM) and 10.5 s (WINDOW_UPDATE) and got noGOAWAY. The second write got it after 2.0 s and 5.3 s.k1 + k2 <= 42, which is 21 s for each write. The wait is 20 s on a debug build.server-sent resets stay charged after the server has sent its GOAWAYresets streams that must exist before the GOAWAY, so a second write is not possible. On a debug build it has 1700 streams (other builds: 1300, as before).1001 + 33 * 21 = 1694, which covers a wait of 20 s. In the probe with 1300 streams and one CPU, the reset phase took 6.4 s and needed 1199 of them.The upper bound. With
streamResetBurst: 1500in the first test (a bucket that is too large), the test fails on the release build and on the debug build withthe server took the whole flood and sent no GOAWAY. Theflood()of the first commit wrote a second time without the 6 s rule and passed that case on the release build.The six tests with the debug timeout send a whole bucket: the
RST_STREAMflood with default options, the three floods of server-sent resets,a header block refused before dispatch is not charged, andserver-sent resets stay charged after the server has sent its GOAWAY. The last five use the bucket for server-sent resets (sent_reset_limit,connection.rs:390), which no option changes. The other eight tests keep the default timeout and their 10 s waits.The two smaller tests.
a flood under the burst keeps the session serving requests: 100 pairs, not 500. The burst is the default of 1000 as before.resets of streams opened after the server's GOAWAY are charged:streamResetBurst: 50and 100 pairs, not the default burst and 1200 pairs. The flood still uses stream ids below the held stream.Times per test, debug build (ms,
-t "stream-reset floods". Main: 3 runs, and 4 runs with one CPU. Branch at a888e0f: 3 runs, and 3 runs with one CPU):RST_STREAMflood, default optionsstreamResetBurstWINDOW_UPDATE0WINDOW_UPDATEpast 2^31-1DATAafterEND_STREAMRST_STREAMfor a closed streamDATAThe whole file takes 58 s on main (1 run) and 52 to 81 s on this branch (3 runs, the slowest under a host load average of 564).
Release build (canary of main at 9f70da0): the block passes 3 of 3 runs in 2.7 to 3.2 s (main: 2.4 to 3.6 s). The file passes 3 of 3 runs.
A build with no reset limit (bun 1.4.3-canary at 367d939, before #36230): the same 8 tests fail with the old file and with the new file, and the same 6 pass. The five
flood()tests now fail in 23 to 382 ms withthe server took the whole flood and sent no GOAWAY. Before, each of them ran into the 5 s timeout, and its wait rejected later asUnhandled error between tests. The block takes 16.0 s (before: 40.9 s).More load. One run of the first commit with a busy loop on the same CPU: the six tests with the debug timeout pass (9.7 to 32.7 s each). One test with the default timeout,
streamResetBurst sets where the flood is detected, timed out at 5 s.Why not one larger write. On the release build, one write of 4000 or more pairs (144 KB) gives the raw client
ECONNRESETand noGOAWAY(15 of 15 runs). 3500 pairs or fewer give theGOAWAY(18 of 18 runs). I did not look for the cause, and I cannot run Windows or macOS to see where that limit is there. The first write stays at 1200 pairs (43 KB) on every build.Seen, not changed here.
stream release after a queued END_STREAMcases in the same file take 1.7 to 4.9 s and ran into the 5 s timeout in 2 of 4 runs of the whole file. test: deflake the h2 stream-release cases on debug builds #42357 is open for those cases.server streams answered with end("ok") behind another stream's stalled response are released, withExpected: <= 3andReceived: 6. The 14 flood tests passed in all 13 runs.Unhandled error between tests. This is the same on main.Not run locally. Windows and macOS.
[auto-merge] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file