test(serve): fix darwin flake in the respond-without-reading-body backpressure test - #36088
Conversation
…kpressure test The test added in #36072 used Connection: close. Once the handler responds without reading the body, uWS does shutdown(SHUT_WR) + close() back-to-back, and close() with unread body bytes in the kernel recv buffer sends RST on macOS. That RST races the still-armed 'drain' writeMore (EPIPE into sock.once('error', reject)) and can drop the response before the client reads it. 8/10 fail on darwin 14 x64 and darwin 26 aarch64. Switch to keep-alive so the server keeps the socket open after responding; detach_response()'s resume then observably lets the upload pump progress past the plateau, which is what the test was written to cover. Also resolve on the body content (not the header terminator) and drop the error-reject since no EPIPE is expected on this path. 50/50 pass on darwin aarch64, 20/20 on darwin x64, 10/10 on linux debug+ASAN; still fails on a bun without #36006 (sentBeforeGate == TOTAL).
|
Warning Review limit reached
Next review available in: 8 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 |
|
Reproduced on Self-review: approve; the one concern (unbounded resume poll making the |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake with a well-traced root cause (RST-on-close-with-unread-recv-data racing the response on darwin) and verified 50/50 + 20/20 on the previously-failing lanes. The inline nit about the unbounded poll is worth addressing but doesn't block: a regression there still fails via test timeout.
Extended reasoning...
Overview
This PR touches a single test in test/js/bun/http/serve.test.ts — the releases a paused request body when the handler responds without reading it case in the request body backpressure suite added by #36072, which has been flaking on darwin lanes since it landed. The shared pumpUploadUntilPlateau helper gains a connection parameter (defaulting to "close", so the five other callers at lines 3357/3407/3447/3488 are unaffected) and returns a getSent() closure. The flaky test switches to Connection: keep-alive, drops the sock.once('error', reject) (a no-op error handler is already installed by the helper at line 3283), resolves the response promise on the body sentinel instead of the header terminator, and adds a post-response poll asserting the upload progressed past the plateau — the direct observable of detach_response()'s resp.resume().
Security risks
None. Test-only diff, no src/ changes, no new external inputs.
Level of scrutiny
Low-to-moderate. This is a targeted CI de-flake with a mechanistic root-cause writeup: uWS try_end() → shutdown(SHUT_WR) → close() with unread body bytes in the kernel recv buffer sends RST on macOS, which races both the still-armed drain pump (EPIPE) and delivery of the already-written response. Switching to keep-alive removes the close, so no RST, and lets the resume run on a live socket — which the PR notes was a no-op on the old close path anyway (us_socket_resume early-returns on a closed fd). The verification table (7-8/10 fail → 50/50 and 20/20 pass on the actual darwin runners, still green on linux debug+ASAN, still fails under USE_SYSTEM_BUN=1) is exactly what REVIEW.md asks for.
Other factors
The one finding is a nit: the expect(getSent()).toBeGreaterThan(sentBeforeGate) at line 3551 is tautological given the unbounded while (getSent() === sentBeforeGate) poll immediately before it — sent is strictly increasing, so the loop only exits when the assertion already holds, and a regression manifests as a hang → test-runner timeout rather than a failing expect. Bounding the poll with a deadline would make the assertion load-bearing per REVIEW.md's "poll a bounded window" / "every assertion must be able to fail", but the test still catches the regression either way. I also confirmed the other five pumpUploadUntilPlateau callers pass three positional args and so keep Connection: close, and that dropping sock.once('error', reject) cannot leave an unhandled error because the helper installs sock.on('error', () => {}) before returning.
There was a problem hiding this comment.
LGTM — test-only de-flake with a clear root cause, and the bounded-poll nit from the earlier pass is addressed in 1057b9f.
What was reviewed:
pumpUploadUntilPlateau's newconnectionparam defaults to"close", so the other callers in this describe block are unchanged.getSent()closure correctly observes post-return mutations ofsentvia the still-armedsock.once('drain', writeMore).- Dropping
sock.once('error', reject)doesn't leave an unhandled-error path — the helper already installssock.on('error', () => {}), and'close'still resolves the response promise. - The 2s deadline on the post-response poll makes
expect(getSent()).toBeGreaterThan(sentBeforeGate)the load-bearing assertion for the resume.
Extended reasoning...
Overview
Test-only change to one subtest in test/js/bun/http/serve.test.ts (the request body backpressure > releases a paused request body when the handler responds without reading it case added by #36072), plus two backward-compatible additions to the shared pumpUploadUntilPlateau helper: a connection parameter defaulting to "close" and a getSent() accessor in the return object. No src/ changes.
The de-flake mechanism is well-argued in the PR description (RST-on-close-with-unread-recv-data racing the response delivery on darwin) and matches the referenced uWS try_end → shutdown+close sequence. Switching to Connection: keep-alive removes the RST race and, as a bonus, makes the test actually observe detach_response()'s resp.resume() on a live socket — the new bounded poll + toBeGreaterThan(sentBeforeGate) assertion is the direct observable of that.
Security risks
None. Test-only; no user-facing surface, no parsing of untrusted input, no network egress beyond localhost.
Level of scrutiny
Low. This is a targeted flake fix in test code, verified by the author on both failing darwin lanes (50/50 and 20/20 pass) and on linux debug+ASAN (10/10). The USE_SYSTEM_BUN=1 fail-before check confirms the test still exercises the same #36006 backpressure invariant. The helper's default parameter keeps the other five callers byte-identical in behavior.
Other factors
My previous review flagged the unbounded poll making the expect vacuous; the author addressed it in 1057b9f with a 2s deadline, and the thread is resolved. I checked that dropping the 'error' reject handler cannot produce an unhandled error event (the helper already swallows socket errors at line 3283) and that the response promise still has a terminal path via 'close'. The getSent() closure works because writeMore remains armed on 'drain' after the plateau helper returns, so sent continues to mutate once the server resumes reading. 2s is generous for a single drain-triggered write even under debug+ASAN.
2 participants
serve.test.tshas been red on the darwin lanes since #36072 landed (main #83238, also the PR's own build #83229 as a retry-flake). The subtest isrequest body backpressure > releases a paused request body when the handler responds without reading it, which #36072 added to cover thedetach_response()resume atRequestContext.rs:2382.Cause
The test sends a 32 MiB
PUTover a rawnet.SocketwithConnection: close, lets the server's pre-stream pause stall the upload, then has the handler respond without touchingreq.bodyand waits for the client to receive the response.Once the handler returns,
render_bytes()writes the response viatry_end()and uWS doesshutdown(SHUT_WR)immediately followed byclose()(HttpResponse.h:860-871). Because the socket was paused, the unread body bytes are still sitting in the kernel recv buffer, andclose()with unread recv data sends RST. On macOS that RST races two things:sock.once('drain', writeMore), which is still armed frompumpUploadUntilPlateau; when it fires and writes into the reset peer the socket emits EPIPE, which the test'ssock.once('error', reject)turns into a failure, and'data'handler sees them.On Linux the
'data'event reliably wins the race, so the test passed there; on darwin it loses ~70-80% of the time (verified ondarwin-test-arm64-3anddarwin-test-x64-2with the main #83238 binary).detach_response()'sresp.resume()is also a no-op on this path: for a simple text bodytry_end()has already closed the socket by the time it runs (us_socket_resumeearly-returns on a closed socket). So withConnection: closethe test was not actually observing the code it was written to cover.Fix
Switch this one test to
Connection: keep-alive. The server does not close after responding, so there is no RST, the response is delivered deterministically, anddetach_response()'s resume runs on a live socket. The test now additionally asserts that the client's upload progresses past the plateau after the response, which is the direct observable of the resume (without it the keep-alive socket would stay paused andgetSent()would never move).Also: resolve the response promise on the body content rather than the header terminator, and drop the
sock.once('error', reject); neither race applies on a keep-alive connection but both were latent flake on the old path. ThepumpUploadUntilPlateauhelper gains aconnectionparameter (defaulting to"close", so the other five callers are untouched) and returns agetSent()accessor.Verification
With the main #83238
bunbinary:darwin-test-arm64-3)darwin-test-x64-2)USE_SYSTEM_BUN=1 bun test test/js/bun/http/serve.test.ts -t 'releases a paused'still fails ontoBeLessThan(TOTAL)(no #36006 backpressure), so the test still exercises the samesrc/change. This is a test-only diff; fail-before is against a Bun without #36006, not against this PR's ownsrc/.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.