Repository navigation
Conversation
|
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 (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe HTTP test suite now uses shared raw-socket helpers and concurrent tests. It expands coverage for abort cleanup, streaming body limits, late promise settlement, WebSocket upgrades, and ChangesHTTP lifecycle test coverage
Merge Risk: ⚪ Minimal · up to This localized test-only change speeds up the HTTP leak tests and strengthens their assertions; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, verification results, test behavior, and related context. It does not use the template headings, but it provides the required information and is mostly complete. Comment |
|
Status: rebased on main after #40137 landed (cbf0b30, plus dd75f31 from review), and ready for a maintainer. The diff is test-only. How the slowness was confirmed: ran the old file under the debug build (14.3s) and release (2.5s). A probe showed Rebase: #40137 added an Guards: the 413 late-settle case stays in a child process. The in-process abort loop holds its parked promises, so it times out on bun 1.4.0 (before #39743) and passes on the current build. CI: the rebased head ran in builds 106986 (cbf0b30) and 106992 (dd75f31, which adds only a fail-fast error path to the http2 loop). This file passes on all 11 lanes in both. The red jobs are unrelated and fail the same way on main: the x64-asan shard ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also looked at the other deleted fixture (serve-pending-promise-abort-leak-fixture.ts) moving in-process — that one was a leak-only scenario (never a crash), and its invariant (pendingRequests returns to 0 after aborts) is preserved by await stopAndAssertDrained(server), so no coverage is lost there. The test.concurrent conversion also checks out: every test constructs its own port: 0 server or child process with no shared mutable state between them.
Extended reasoning...
The confirmed inline finding covers the 413 late-settle fixture losing subprocess crash isolation. The second deleted fixture (serve-pending-promise-abort-leak-fixture.ts) is a different case worth distinguishing for the reviewer: its original comment described a leak ("Previously the RequestContext leaked"), not a UAF, and the observable it asserted — server.pendingRequests === 0 after 100 aborts — is exactly what the in-process replacement asserts via stopAndAssertDrained, so that migration does not weaken a safety net. I also checked the test.concurrent conversion for shared state between cases; each test owns an isolated server or subprocess, and the module-level helpers are pure, so concurrency is safe. The toBeLessThanOrEqual(limit) change strengthens the prior toBeLessThan(overflowTotal) bound rather than loosening it.
|
Updated 1:59 PM PT - Aug 27th, 2026
❌ @robobun, your commit dd75f31 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40539That installs a local version of the PR into your bun-40539 --bun |
There was a problem hiding this comment.
LGTM — thanks for restoring the 413 late-settle guard to a subprocess in d229a6c; that addresses the safety-net concern from my earlier review.
Checked that the remaining in-process 413 test never late-settles (only holds capturedResolve), so no UAF risk was reintroduced there. The inlined abort-loop test replaces a leak (not crash) fixture, so stopAndAssertDrained hanging into the timeout is the correct failure mode. Verified the fixture's pendingAfterSettle read is ordered after the follow-up fetch() resolves on a sync-returned response, and that openRawSocket's post-connect error swallowing is scoped to the documented reset-during-write case with pre-connect errors still rejecting.
Extended reasoning...
Overview
Test-only refactor of test/js/bun/http/serve-pending-promise-abort-leak.test.ts and its fixtures. The goal is to cut a 20s ASAN-lane file down to ~4s by removing broken GC-poll waits (which spun 100 rounds after #39743 made the 413 path free contexts synchronously), converting independent tests to test.concurrent, and replacing shape-only assertions with exact-value checks (full 413 response bytes, DOMException name+message, WebSocket event ordering, {exitCode, signalCode} objects). One fixture (serve-pending-promise-abort-leak-fixture.ts, a leak repro) is deleted and inlined; the other (serve-413-streaming-late-resolve-fixture.ts, a UAF repro) is rewritten but kept as a spawned subprocess.
Security risks
None. This touches only test code — no production paths, no auth/crypto, no user-facing surface. The tests bind to port: 0 on loopback and spawn only bunExe() with bunEnv.
Level of scrutiny
Moderate for a test refactor, focused on the REVIEW.md rule against silently weakening safety nets. My prior review flagged that the 413 late-settle UAF regression guard had been moved in-process, which would let an ASAN abort take down the whole runner. Commit d229a6c directly reverted that: the fixture file is retained and spawned via Bun.spawn, with the crash-isolation rationale now stated in both the fixture header and the test comment. The other fixture that was inlined guarded a leak (not a crash), and its new failure mode — await server.stop() never resolving, tripping the test timeout — is an appropriate signal that doesn't need subprocess isolation.
Other factors
The change aligns tightly with REVIEW.md test conventions: Promise.withResolvers for callback bridging, test.concurrent for independent per-server tests, composite .toEqual over scattered .toBe, signalCode === null on hang-guard subprocesses, no per-test timeout overrides, Buffer.alloc(n, fill) retained, and event-driven waits (socket close, stop() resolution) replacing Bun.gc(true)+sleep polls. No CODEOWNERS entries cover these paths. No outstanding third-party CHANGES_REQUESTED reviews.
There was a problem hiding this comment.
LGTM — test-only refactor, and the earlier concern about subprocess isolation for the 413 late-settle UAF case is addressed (d229a6c restored the child fixture; 75bd203 further tightens the abort loop by holding the parked promises so GC cannot mask a missing abort teardown).
What was reviewed: verified the 413 late-settle scenario still runs in a spawned fixture with {exitCode, signalCode} asserted; the inlined abort-loop test is leak-only (hangs stop() on regression, no crash) so in-process is safe; each test.concurrent owns its own port: 0 server or child with no shared mutable state; GC/sleep polling replaced by awaiting socket close / server.stop(); swallowed .catch(() => {}) outcomes now assert AbortError.
Extended reasoning...
Overview
This PR refactors test/js/bun/http/serve-pending-promise-abort-leak.test.ts and its fixtures to cut runtime (14.3s → 4.5s debug) and tighten assertions. The serve-pending-promise-abort-leak-fixture.ts child is deleted and its 100-abort loop inlined in-process (now holding each parked promise in an array so GC cannot collect the NativePromiseContext cell and mask a missing abort-path teardown). The serve-413-streaming-late-resolve-fixture.ts is rewritten to drop the broken Bun.gc(true) + sleep poll (which since #39743 always ran to its 100-round cap) in favor of awaiting the client socket close, and now emits richer per-case JSON. The main test file converts every test to test.concurrent, extracts shared openRawSocket/readUntilClose/chunkedPostHead helpers, replaces .catch(() => {}) with asserted AbortError outcomes, asserts exact 413 response bytes and {exitCode, signalCode} on children, adds WebSocket open/upgrade/close event ordering, and drops per-test timeout overrides. No runtime code is touched.
Security risks
None. This is a test-only change with no production code paths, no external network contact (all servers are local port: 0), no credentials, and no changes to TLS/auth/crypto surfaces.
Level of scrutiny
Moderate. The diff is large (~1000 lines) but mechanical: it applies the repo's own test conventions from REVIEW.md and test/CLAUDE.md — await observable conditions instead of GC+sleep, test.concurrent for independent server-owning tests, no per-test timeouts, one .toEqual on combined objects, signalCode === null on hang/crash-guard children, exact error names/messages over toContain. The one substantive design question — whether moving crash-regression scenarios out of child processes weakens the safety net — was raised in the first review round; d229a6c restored the 413 late-settle child fixture, and the remaining inlined case (the abort loop) is leak-only, where regression manifests as server.stop() never resolving rather than a process crash, so in-process is appropriate.
Other factors
The PR has iterated responsively: the initial push moved the UAF fixture in-process, review flagged it, d229a6c restored it, and 75bd203 further hardened the abort loop by pinning the handler promises so the test proves the abort path (not GC) reclaims the context. I checked that concurrent tests share no module-level mutable state (each declares its own server, signals, and arrays inside the test body), that readUntilClose cannot hang on error (close fires regardless, and openRawSocket already installed the post-connect error swallow), and that the deleted fixture's assertions (pending === 0, abortCount === iterations) are all preserved or strengthened in the inlined version. No CHANGES_REQUESTED reviews are outstanding from other reviewers.
…ghten its assertions The streaming 413 fixture waited for pendingRequests to reach 1. Since #39743 the 413 path frees the context at once, so that wait never ended and each case spun through 100 rounds of Bun.gc(true) plus a 10ms sleep. The fixture keeps its child process (the late settle was a heap-use-after-free before #30077) but now waits for the client socket to close, which is after uSockets freed the server socket, and prints one JSON line per case. The abort-loop fixture was leak-only, so it moves in-process, including the http2 variant from #40137. It holds every parked promise, so GC cannot free a context and server.stop() resolves only if each abort did. On bun 1.4.0, which predates #39743, the test times out. Every test owns its server or child, so all 17 run as test.concurrent. The per-test timeouts are gone. Assertions check values: the full 413 response, the body rejection message, pendingRequests 0 and no error() call after a late settle, the exact AbortError, the WebSocket event order, and toEqual on each child's output. Debug build under the same load: 17.0s to 5.4s.
eebd2ad to
cbf0b30
Compare
…fore the handler ran The HTTP/1.1 branch rejects handlerEntered on a socket error, but the HTTP/2 branch absorbed a fetch failure into the outcome it awaits only after handlerEntered, so a TLS or ALPN failure hung until the timeout.
There was a problem hiding this comment.
LGTM — dd75f31 wires the h2 fetch settlement into failHandler, addressing the last open point.
What was reviewed: the outcome.then(failHandler) fix — it races correctly since failHandler is a no-op once signalHandler resolved handlerEntered; confirmed the 413 late-settle scenario still runs in a child fixture (crash isolation kept); checked the in-process abort loop holds every parked promise so stopAndAssertDrained cannot pass via GC; verified no per-test timeouts remain and every raw-socket await has an error path to reject it.
Extended reasoning...
Overview
Test-only refactor of test/js/bun/http/serve-pending-promise-abort-leak.test.ts and its 413 fixture. The abort-loop subprocess fixture is deleted and moved in-process (both HTTP/1.1 and the new HTTP/2 variant from #40137), the 413 fixture drops its sleep+Bun.gc(true) poll loop in favor of awaiting the socket close, all 17 tests become test.concurrent, per-test timeouts are removed, and assertions are tightened to exact 413 bytes / AbortError names / toEqual composites / { exitCode, signalCode }. No runtime code is touched.
Security risks
None. The diff is confined to test/; no auth, crypto, parsing of untrusted input, or production code paths are modified.
Level of scrutiny
Moderate for a test refactor, focused on the REVIEW.md test rules: not weakening a crash safety net, awaiting observable conditions rather than time, and wiring every failure event to reject the awaited promise. Two concerns were raised on earlier pushes and both were addressed by commits — the 413 late-settle scenario was restored to a subprocess fixture (crash isolation preserved), and dd75f31 now rejects handlerEntered in the h2 branch if the fetch settles before the handler ran, so a TLS/ALPN failure surfaces immediately instead of hanging. I re-read the fix: outcome.then(name => failHandler(...)) fires unconditionally, but once signalHandler has resolved handlerEntered the reject is a no-op, so the happy path (ac.abort() → "AbortError") is unaffected.
Other factors
The in-process abort loop pushes every parked Promise<Response> into held[], so GC cannot free the RequestContext and stopAndAssertDrained is a real leak check (the PR notes it times out on 1.4.0). No CODEOWNERS entry covers these paths. No outstanding third-party CHANGES_REQUESTED reviews. Exit reason was dry_streak with zero findings this run. The change follows test/CLAUDE.md conventions closely (concurrent, port: 0, Promise.withResolvers, composite toEqual, no sleep polling), so approving.
Problem
test/js/bun/http/serve-pending-promise-abort-leak.test.tstook 20.3s on debian 13 x64-asan (build 106056) and about 3s on every other lane.streaming 413fixture: it waited forpendingRequeststo reach 1, but since Bun.serve: tear down an aborted request's context at abort instead of waiting for GC #39743 the 413 frees the context at once, so each case ran 100 rounds ofBun.gc(true)plus a 10ms sleep. Two more children ran 100 aborts each (HTTP/1.1 and, since Bun.serve: HTTP/2 support viahttp2: true#40137, HTTP/2), then polled with GC.Fix
server.stop()resolves only if each abort did (on bun 1.4.0, before Bun.serve: tear down an aborted request's context at abort instead of waiting for GC #39743, it times out). All 17 tests own their server (port: 0) or child and run astest.concurrent. Per-test timeouts are gone.pendingRequests0 and noerror()call after a late settle, the exactAbortError, the WebSocket event order,toEqualon child output.bun bd test test/js/bun/http/serve-pending-promise-abort-leak.test.tson the same loaded machine, main 17.0s to 5.4s (debug), 17 pass. Earlier CI (16 tests, before the rebase): x64-asan 8.6s to 12.7s, other lanes 84ms to 543ms. On 1.4.0, 13 of 16 fail.Background
pendingRequestscounts liveRequestContexts. A gracefulserver.stop()resolves once it is 0 and every connection is gone, so awaiting it is a leak check when the test holds the promise (GC cannot free the context first).Promise<Response>fromfetch()gets a native reaction. Before Bun.serve: tear down an aborted request's context at abort instead of waiting for GC #39743 the context lived until it fired or GC collected the promise. Now abort, the 413 end path, and upgrade release it directly.Content-LengthovermaxRequestBodySizehits the streaming 413 inon_buffered_body_chunk, which closes the connection.Notes
Per-test timing under the debug build, before:
After (concurrent, so per-test numbers overlap):
Exact values the new assertions pin down (debug and release agree): the 413 response is
HTTP/1.1 413 Payload Too Large\r\nConnection: close\r\n\r\n(noDate, no body),pendingRequestsis 0 right after the client reads it,req.text()rejects with "Request body exceeded maxRequestBodySize", a late reject does not invokeerror(), a follow-up GET on a new connection returns 200 "follow-up", the abort error forreq.text(),for await (req.body)andreq.textStream()is aDOMExceptionnamedAbortErrorwith message "The connection was closed.", and the server seesopen, thenupgrade()returns true, thenclose 1000.The abort loop keeps 100 iterations. In the debug build the first connects pay a one-time warmup of about 500ms, then each iteration costs about 5ms, so cutting the count would save little.
The 413 fixture prints one JSON line per case:
settle,response,bodyError,pendingAfter413,outcome,pendingAfterSettle,errorCalls,followUp. The test asserts both lines with onetoEqual, then{ exitCode, signalCode }.The first revision of this PR ran the 413 late settle in-process. Review pointed out that this dropped the crash isolation the fixture existed for, so d229a6c restored the child.
Guard check on the pre-fix binary (bun 1.4.0, 34cbb9a, one day before #39743): the first revision's in-process abort loop passed there, because nothing referenced the parked promises and the 1s idle GC timer (plus level-1 GC after each
expect()in CI) collected them, which frees the contexts through theNativePromiseContextdestructor. With the promises held (75bd203) the test times out on 1.4.0 and passes on the current build. The whole file on 1.4.0: 13 fail, 3 pass (the 3 do not guard #39743 or #40034).Rebase on #40137 (cbf0b30): that PR added an
--http2variant to the abort-loop fixture this PR deletes. The variant is ported into the in-process test astest.concurrent.each([false, true]): the HTTP/2 case serves withtlsandhttp2: true, and each iteration aborts afetch(url, { protocol: "http2" })once the handler ran, which sends RST_STREAM. Main's file on this build and load: 17.0s and 17.5s (413 child 8.4s, abort children 3.3s and 2.3s). This branch: 5.4s to 6.3s. The branch was rebuilt as one commit on top of main; the earlier commits 5ee60e8, d229a6c, 75bd203 are squashed into it.Related: #33704 is a stale, conflicting sweep that only adds
test.concurrentto this file.[stamp-90s] gate passed · iteration 4 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 4
evidence per changed file