Repository navigation
Conversation
…tighten assertions All four tests arm their fault inside their own subprocess, so the parent's process-wide fault table is never touched and the tests can run concurrently. Workload reductions where the assertion is N-agnostic: - close-tracking: N 8->4, body 32K->8K (assertion is closed === N) - Bun.serve abort: body 64K->16K, abort 1024->512 - seeded fuzz: bodyLen 8K->4K (iteration count unchanged) The TLS proxy pipe/destroy test keeps its original 256KB/4-request workload; smaller sizes let the server finish draining before the client destroy arrives, so the drain race would no longer be exercised. Assertions: replace bare "OK" with structured JSON (closed count, per-client received-past-threshold, response status) and assert stderr is empty rather than any-string. The fuzz test aggregates all eight iterations into one toEqual so a failure shows which (bytes, after) pair diverged. Local debug+ASAN wall time: ~21.6s -> ~9.9s over 3 runs each.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but given #33704 previously dropped this file from a concurrency pass and this PR reduces workload sizes on tests that exist to hit backpressure/drain races, it's worth a human confirming the reduced sizes still exercise the target windows and that the new closed: 4 / stderr: "" assertions hold up across CI lanes.
What was reviewed:
- Confirmed every
fault.setcall is subprocess-scoped, sotest.concurrentdoesn't share the parent fault table; the parent-side upstream server (test 1) and http client (fuzz test) don't conflict. - Checked the new
closed: 4assertion in the TLS test —proxy.close()callback fires only after all connections drop, so server-sideresclose events should have run by then. - Verified the reduced Bun.serve (16 KB / abort at 512) and close-tracking (8 KB / destroy at first byte) workloads still leave multi-KB of queued 1-byte sends pending at abort time.
Extended reasoning...
Overview
Test-only change to test/js/node/http/node-http-syscall-fault.test.ts: converts four subprocess-spawning tests to test.concurrent, reduces body/N workload on three of them, and tightens assertions (structured JSON output instead of "OK", stderr: "" instead of expect.any(String), aggregated fuzz results).
Security risks
None — test-only, no runtime code touched.
Level of scrutiny
Medium. It's test-only, but these tests exist specifically to catch UAF / drain-race regressions in the socket write path, and the PR both reduces the workload that creates the race window and adds new assertions (closed: 4, received: [true×4], stderr: "") that were not previously enforced. REVIEW.md is explicit that weakening a test's ability to catch its target failure mode, or adding assertions that could flake, both warrant scrutiny.
Other factors
- #33704 previously attempted
test.concurrenton this file and backed it out after debug-ASAN issues. The author explains the difference (smaller per-test workload so only the unchanged TLS test dominates), which is plausible, but it means CI history has already flagged this file as concurrency-sensitive. - The author kept the TLS test at 256 KB after empirically finding smaller sizes miss the destroy-vs-drain race. The same empirical check isn't stated for the Bun.serve (64 KB → 16 KB) or close-tracking (32 KB → 8 KB) reductions — arithmetically there's still ~15 KB / ~8 KB of queued 1-byte sends at abort time, which should be ample, but a maintainer familiar with what these tests were added for should confirm.
- The new
stderr: ""assertion is a strict tightening;BUN_DEBUG_QUIET_LOGS=1is set so debug spew shouldn't leak, but any incidental stderr on a specific CI lane would now fail the test. - 10 local debug+ASAN runs passed per the description; no CI results visible in the timeline yet.
Deferring so a human can sanity-check the workload reductions against the original repros and let CI weigh in on the new strict assertions.
WalkthroughChangesThe HTTP syscall-fault tests now run concurrently, use structured JSON subprocess output, validate exact lifecycle results, support threshold-based client aborts, and reliably manage randomized backpressure subprocesses. HTTP fault-injection test coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http/node-http-syscall-fault.test.ts`:
- Around line 244-249: Update the fuzz-server subprocess flow around Bun.spawn
and the result assertions to drain proc.stderr concurrently with the existing
stdout and process-exit handling, then assert the combined subprocess result
includes empty stderr before cleanup. Preserve the existing results/body-length
assertion and ensure the same behavior is applied to the related logic spanning
the referenced subprocess handling block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fee24af-0d73-48e6-8508-db5321ede270
📒 Files selected for processing (1)
test/js/node/http/node-http-syscall-fault.test.ts
Matches the other three tests in this file: start draining stderr once the port line is read so the pipe cannot back up, and assert it is empty after the server has been SIGTERM'd and exited.
…B lower bound Fold the fuzz subprocess's stderr into the thrown error when a client iteration rejects so a fault-injection-triggered server error shows up in CI output instead of a bare client-side socket error. Add an in-tree comment anchoring the 256 KB upstream body size in the TLS proxy test; smaller sizes let the proxy finish draining before the client destroy lands and silently skip the queued-drain race.
|
Updated 7:57 AM PT - Jul 24th, 2026
✅ @robobun, your commit 08add6eb4481066895423d1cec4cee4b74868e8d passed in 🧪 To try this PR locally: bunx bun-pr 35424That installs a local version of the PR into your bun-35424 --bun |
|
Self-review pass addressed in 08add6e:
Local debug+ASAN: 5 runs at 9.5-12.5s, no flakes. Waiting on CI. |
There was a problem hiding this comment.
I didn't find bugs, but deferring to a human on this one: it revisits a concurrency change that #33704 explicitly backed out after ASAN issues, and the workload reductions (N 8→4, body sizes halved/quartered) are a coverage judgment call on race-sensitive tests — worth a maintainer confirming the reduced sizes still exercise the intended failure modes across CI lanes.
What was reviewed:
- Confirmed all four tests arm
fault.setonly inside their subprocess, sotest.concurrentdoesn't share the parent's fault table; the in-parentupstreamserver in the TLS test useslisten(0)and no fault. - Assertions are strictly tightened (
stderr: ""replacesexpect.any(String); structured JSON replaces bare"OK"); nothing is weakened. - Fuzz-test restructure drains stderr, kills/awaits unconditionally, and surfaces stderr on reject — CodeRabbit's comment is addressed.
Extended reasoning...
Overview
Test-only change to test/js/node/http/node-http-syscall-fault.test.ts: converts four subprocess-based syscall-fault tests to test.concurrent, reduces per-test workload (request count and body sizes) on three of them, and tightens assertions (structured JSON output, stderr: "", aggregated fuzz results). The TLS pipe/destroy test keeps its original 256 KB workload with a comment documenting why. No production code is touched.
Security risks
None. Test-only; subprocesses run local bunExe() fixtures over loopback with port: 0.
Level of scrutiny
Medium. Although test-only, these tests exist to catch backpressure/drain/destroy races in the HTTP server paths, and the PR reduces the workload that provokes those races. REVIEW.md flags "never silently weaken an existing test" — the author addresses this explicitly (TLS test kept at swept lower bound; other three have N-agnostic assertions), but whether 8 KB / N=4 under 1-byte sends still reliably reaches the same drain-queue depth as 32 KB / N=8 is empirical. The PR description also notes #33704 previously dropped this file from a concurrency pass after debug-ASAN verification surfaced issues; the author's mitigation is precisely these workload reductions, which makes them the load-bearing change.
Other factors
- Assertions are unambiguously stronger:
expect.any(String)→""on stderr, bare"OK"→ structured{ closed, received }/{ status, readPastThreshold }, per-iterationtoBe→ aggregatedtoEqualwith(bytes, after)context. - Concurrency safety checks out: every
fault.setis inside a subprocess fixture; the parent'safterEach(fault.clear)is a no-op; the one in-parent server (TLS test'supstream) never arms a fault and uses an ephemeral port. - CodeRabbit's stderr-drain comment on the fuzz test was addressed in 325e6f7..08add6e and the thread is resolved.
- Local verification: 3+10 debug+ASAN runs at ~10s, no flakes. CI build #79576 was still in progress at review time.
Given the prior #33704 back-out and the coverage-vs-speed tradeoff being the crux of the change, a maintainer sign-off is appropriate here rather than auto-approval.
|
Closing this PR because it has been inactive for more than 90 days. |
What does this PR do?
test/js/node/http/node-http-syscall-fault.test.tstakes ~31s on the debian 13 x64-asan lane (build #79247). All four tests arm theirsocketFaultInjectionrule inside their own subprocess, so the parent's process-wide fault table is never touched and the tests can safely run concurrently.Speed
test.concurrenton all four tests (every fault is subprocess-scoped; theafterEach(fault.clear)in the parent is a no-op).N8 → 4, body 32 KB → 8 KB (assertion isclosed === N)bodyLen8 KB → 4 KB (iteration count unchanged at 8)Assertions
"OK"outputs with structured JSON: proxy-sideclosedcount and per-clientreceived > thresholdfor the pipe/destroy test;{ status, readPastThreshold }for the Bun.serve abort test.stderr: ""instead ofexpect.any(String)so unexpected subprocess diagnostics fail the test.toEqualso a failure shows which(bytes, after)pair diverged instead of just the first.How did you verify your code works?
Local debug+ASAN, 3 runs each side:
bun bd test --timeout=60000 <file>10 additional runs of the after version all pass (9.6-10.6s, no flakes).
note on #33704
#33704 dropped this file from its concurrency pass after debug-ASAN verification surfaced issues. The difference here is the N/body reductions on the non-TLS tests: at the original N=8 / 32 KB / 64 KB sizes, four concurrent ASAN subprocesses each running thousands of 1-byte
bsd_sendevent-loop turns contend hard enough to overrun per-test timeouts. With the smaller workloads only the TLS test (unchanged) dominates and the others finish well inside it.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.