Repository navigation
test(fetch): deflake the textStream() drop-after-empty-decode case - #40947
Conversation
The server now waits until the client has consumed the first chunk before it writes the incomplete UTF-8 tail and destroys the socket. Under load, the HTTP thread could see all three chunks and the drop before the JS thread read anything. FetchTasklet coalesces them into one progress update with the failure set, the body becomes an errored stream, and the test saw received="" instead of "A".
|
Updated 12:19 AM PT - Aug 30th, 2026
❌ @robobun, your commit 3a91639 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40947That installs a local version of the PR into your bun-40947 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughChangesFetch stream reset handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change only stabilizes a fetch streaming test by coordinating when the server sends the incomplete UTF-8 tail and closes the connection; it does not alter product behavior. No actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the failure, the handshake-based fix, the preserved test behavior, and verification results. It does not use the template headings exactly, but it provides the required information and is sufficiently complete. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused test-only deflake that swaps a timing race for an explicit handshake without weakening the assertion.
What was reviewed:
- Confirmed the assertion (
{ code: "ECONNRESET", received: "A" }) is unchanged, so the property guarded by the original test is preserved. - Checked for hang risk: the
finallyresolvesconsumedFirstChunkon every exit, so the server callback always reachessock.destroy()andawait using servercan dispose; repeated.resolve()calls are no-ops. - Verified the pattern matches the repo's "await the condition, not time" rule — no sleep/setTimeout,
Promise.withResolvers()handshake as endorsed in test/CLAUDE.md.
Extended reasoning...
Overview
This PR touches a single test in test/js/web/fetch/body.test.ts — the "rejects a fetch textStream() when the connection drops after an empty decode" case. The old version wrote three chunked bytes back-to-back (separated only by setImmediate in writeChunk) and immediately destroyed the socket, which under CI load let the HTTP thread coalesce the body bytes and the failure into one progress update, discarding "A" before the client observed it. The fix introduces a Promise.withResolvers() handshake: the server writes "A", awaits the client's confirmation from inside the for await loop, then writes the two partial-UTF-8 bytes and destroys. A finally block resolves the handshake unconditionally so the server callback can always proceed to sock.destroy() even if the stream ends or errors early. No runtime code is touched.
Security risks
None. This is a test-only change to a local in-process net.createServer fixture on 127.0.0.1 with port: 0. No new network surface, no credentials, no untrusted input handling.
Level of scrutiny
Low. The diff is ~15 lines in one test, mechanical in nature, and follows the exact deflake pattern the repo's test guidelines prescribe ("await the actual observable condition", Promise.withResolvers() for single-callback synchronization, no sleep/setTimeout). The key REVIEW.md concern for deflakes — "keep asserting the property the original assertion protected" — is satisfied: the expect({ code, received }).toEqual({ code: "ECONNRESET", received: "A" }) line is byte-identical, and the drop still lands after the two empty-decode bytes, which is what the test guards (the #36180 stall-vs-reject regression).
Other factors
I checked rawChunkedServer and writeChunk to rule out a new deadlock: [Symbol.asyncDispose] waits on server.close(), which would hang if the connection stayed open, but the finally guarantees consumedFirstChunk resolves on every path so the server body always reaches sock.destroy(). Multiple .resolve() calls (once per loop iteration plus the finally) are safe no-ops. The added comment explains a non-obvious FetchTasklet coalescing race — the kind of context CLAUDE.md says is worth keeping. No CODEOWNERS entry covers this path, the hunt exited on dry_streak with no findings, and there are no outstanding reviewer objections in the timeline.
|
Status (build #108455): The one red lane is Reproduction of the flake, for reference: 24 parallel loops of the old test body fail 1400 of 3600 iterations with |
2 participants
Problem
test/js/web/fetch/body.test.ts> "rejects a fetch textStream() when the connection drops after an empty decode" fails in the parallel CI batch withreceived: ""instead of"A". The error code is the expectedECONNRESET. It passes when it runs alone."A",0xF0,0x9F(onesetImmediateapart) and then destroys the socket. Under load, the HTTP thread sees all of that before the JS thread has read anything.FetchTasklet::callbackcoalesces the updates into one progress update withfailset, andto_body_value(src/runtime/webcore/fetch/FetchTasklet.rs:1707) oron_body_received(:740) turns the body into an errored stream. The buffered"A"is discarded by design.Fix
Promise.withResolversresolved from thefor awaitbody is the handshake. Afinallyresolves it too, so the server still closes the socket if the stream ends early.0xF0,0x9Fdecode to nothing), which is what the test guards: the stall fixed in fetch: keep textStream() pulling when a native chunk decodes to nothing #36180 surfaced as a hang, not a rejection.bun testunder the same load: old 32 of 192 failures, new 0 of 192 (release) and 0 of 60 (debug build). The whole file passes withbun bd test.Background
res.textStream()on a fetch response is a nativeByteStreamsource in text mode. The HTTP thread delivers body bytes and the terminal result to the JS thread throughFetchTasklet.FetchTasklet::callbackposts at most one task at a time (has_schedule_callback). Results that arrive before the JS thread runs that task are merged: body bytes are appended toscheduled_response_buffer, and the latestfailorhas_morewins.BodyValue::Errorand the bytes are dropped. When it carries a failure after the head was already delivered,ByteStream::on_data(Err)rejects the pending pull or, with no pull pending,append(Err)clears the buffer (fetch: release the buffered response body and error the reader when a streaming response is aborted #32662). Whether the consumer sees the bytes before the error depends on timing.Notes
/tmp/repro-textstream.tsstyle loop (the test body, 150 iterations) run 24 times in parallel on a 16-core box. Failure rate 24% to 59% per process, all withreceived: ""andcode: "ECONNRESET". The same loop with the handshake: 0 failures in 3600 iterations.test.eachtable end with a clean0\r\n\r\n. A coalesced successful end delivers the bytes (TemporaryAndDone), so they are not affected.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/fetch/body.test.ts