Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 that shrinks the workload without weakening what's asserted.
What was reviewed:
- 8MB still overflows the Linux/macOS loopback send buffer with a paused client, so the pin assertion (
detachedWhilePending === false) remains load-bearing; the resizable tests already relied on 8MB for this andRESIZABLE_CHUNK_SIZEis unchanged in value. describe.concurrent: each test bindsport: 0, allocates its own payload, and captures results in local closures — no shared state between tests.- The five tests that were timing out assert byte-delivery only and pass whether or not backpressure engages, so shrinking the payload doesn't hollow them out.
Extended reasoning...
Overview
Test-only change to test/js/node/http/node-http-pinned-write.test.ts: CHUNK_SIZE 64MB → 8MB, describe → describe.concurrent, and RESIZABLE_CHUNK_SIZE rewritten as CHUNK_SIZE (same value, 8MB). No runtime code touched.
Security risks
None. Test file only; no auth, crypto, or untrusted-input handling changed.
Level of scrutiny
Low-to-moderate. The main risk with a de-flake like this is silently weakening the property the test protects (REVIEW.md: "When de-flaking, keep asserting the property the original assertion protected"). I checked each assertion:
- The one test that asserts the pin actually engages (
detachedWhilePending === false) uses a pausednet.Socketclient, so the server's kernel send buffer must fill before the client reads anything. 8MB exceeds the Linux/macOS loopback send buffer (the PR measured the threshold at ~4MB, and the pre-existingRESIZABLE_CHUNK_SIZEtests already used 8MB for exactly this reason). Windows was alreadyskipIf'd for this test before the change; the updated comment just documents that. - The five tests that were timing out in CI assert byte-for-byte delivery (sha1 match, ordering) and don't depend on backpressure engaging — shrinking the payload doesn't change what they prove.
RESIZABLE_CHUNK_SIZE = CHUNK_SIZEis a no-op in value.
For describe.concurrent: every test creates its own server on port: 0, its own payload buffers, and captures results in per-test locals (detachedWhilePending, detachedAfterSecondWrite, etc.). The subprocess tests are fully isolated. No shared mutable state, so concurrent execution is safe. This matches the repo guidance to use test.concurrent for independent subprocess-spawning suites.
Other factors
The PR description reports 15/15 passes under debug+ASAN and a wall-time drop from 15.6s → 7.7s. The change follows the "shrink the workload, don't raise the timeout" rule from REVIEW.md. No prior reviewer comments to address.
|
CI on build #134155, head 755b735:
The diff is green on every lane that ran it. |
|
Closing this PR because it has been inactive for more than 90 days. |
…iB, not 64 MiB Most tests in this file sent 64 MiB so that the kernel left a tail of the write pending. With the default loopback buffers the kernel takes 2.5 to 4 MiB of the first write. One run of the file moved 626 MiB through the loopback interface. The tests now set SO_SNDBUF to 64 KiB on the server's socket with setSocketOptions from bun:internal-for-testing. The kernel then takes 128 KiB of a 1 MiB write, so a tail is pending with 1 MiB. The two resizable ArrayBuffer tests use the same size. The WebAssembly.Memory test is not changed. It still sends 32 MiB.
robobun
changed the title
9c75eed to
755b735
Compare
|
Updated 10:00 PM PT - Oct 27th, 2026
❌ @robobun, your commit 755b735 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 36376That installs a local version of the PR into your bun-36376 --bun |
|
I reopened this PR. The stale sweep closed it, and it had no review. The change is different now. In July, On current main the file still sends 64 MiB in most tests, and one run moves 626 MiB through loopback. With this change one run moves 43 MiB. The PR body has the measurements. |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
test/js/node/http/node-http-pinned-write.test.ts:13— Maintainers can get a green run from the pin-release tests without a pinned tail ever existing, so a broken unpin path would go unnoticed after this change. Cutting CHUNK_SIZE from 64 MiB to 8 MiB at test/js/node/http/node-http-pinned-write.test.ts:13 leaves about a 2x margin over the 2.5-4 MiB the Linux loopback kernel absorbs, and none of the child or in-process pin tests assert that res.write() returned false before checking detached. Fix: make every pin-state assertion depend on the backpressure precondition, e.g. record the res.write() return value and assert it is false in the emitted JSON, which covers the 3 sites listed.Why this was flagged
The child at test/js/node/http/node-http-pinned-write.test.ts:213 writes an 8 MiB payload to a flowing (not paused) client and asserts detachedAfterClose === true at line 242. transfer() detaches whenever no pin is held, so the assertion is also true when the kernel accepted the whole write and nothing was ever pinned. The same holds for detachedAfterWrite at line 274 (asserted at lines 318 and 336) and detachedAfterSecondWrite at line 357 (asserted at line 365). On the base branch the 64 MiB payload is 16x larger than any default loopback send+receive buffer, so a tail was always pending; with 8 MiB the margin is about 2x on Linux and smaller on macOS where autotuned buffers are used, and nothing in the tests checks that res.write() reported backpressure. Only the first test (line 101, detachedWhilePending toBe(false)) fails if no tail is held; the other three keep passing with a broken close/second-write unpin path once the kernel absorbs the write.
Verification: nit. Trigger: a Linux/macOS host whose loopback kernel accepts a full 8 MiB nonblocking send(). src/runtime/server/NodeHTTPResponse.rs:2063-2067 returns when the kernel takes the whole write, so nothing is pinned. test/js/node/http/node-http-pinned-write.test.ts:213 and :352 discard the res.write() return value and only check
detached === true. The diff drops the margin at :13 from 64 MiB to 8 MiB. -
🟡
test/js/node/http/node-http-pinned-write.test.ts:26— Developers running this file locally on a debug or ASAN build may now see the child-process tests time out at the 5 s default, which did not happen when the tests ran serially. describe.concurrent starts all ten tests at once: the seven in-process tests serialize their 8-16 MiB hashing and copies on the one JS thread, and the three spawned debug Bun children compete with them, so each test's wall clock now includes the others' work. Fix: keep the subprocess tests concurrent but shrink the per-test workload so the slowest test stays well under the default timeout without adding per-test timeouts, e.g. cap SO_SNDBUF via setSocketOptions and send about 1 MiB as the PR description describes.Why this was flagged
test/js/node/http/node-http-pinned-write.test.ts:26 switches the block to describe.concurrent with CHUNK_SIZE still 8 MiB at line 13. The three child-process tests (lines 195, 304, 324) each spawn bunExe() and move 8 MiB through loopback while the seven in-process tests hash and transfer 8 to 16 MiB each on the same JS thread. Bun's per-test timeout is wall-clock from test start, and no timeout is set in the file, so locally the default 5000 ms applies; CI passes --timeout from scripts/runner.node.mjs:1949 (90 s, 270 s for ASAN), so CI is not the population at risk. The PR description reports that with describe.concurrent each child-process test went from 2.5 s to 4.4 s on the debug build, and lists not using describe.concurrent as a decision, yet this checkout enables it. On the base branch the tests ran serially so each got its own 5 s window. A developer on a slower machine or under load gets 'test timed out' failures that the serial base does not produce.
Verification: Triggers when a developer runs the file locally with
bun bd test(debug/ASAN build, default 5 s timeout) on a machine where the concurrent load pushes a child-process test past 5 s. test/js/node/http/node-http-pinned-write.test.ts:26 is nowdescribe.concurrent(...), CHUNK_SIZE at line 13 is 8 MiB, and no test in the file sets a timeout.
|
This review ran on 9c75eed, the head before the rework (8 MiB,
The suggested assertion cannot show a held tail. The pin on an One finding is correct for a different reason, and main has the same condition. |
Problem
test/js/node/http/node-http-pinned-write.test.tssend 64 MiB, so that the kernel leaves a tail ofres.write()pending. One run moves 626 MiB through loopback.test timed outin CI (5 failing in the parallel batch (450.13s)). The last 100 CI builds show no failure, but the load is unchanged.Fix
setSocketOptions(req.socket, 1, SEND_BUFFER)with 64 KiB.CHUNK_SIZEis 1 MiB, also for the two resizableArrayBuffertests.WebAssembly.Memorytest sends 32 MiB of that.bun bd test test/js/node/http/node-http-pinned-write.test.ts, 11 pass, 10 of 10 runs.bun test --debug-aotgives the same 11 passes.Background
res.write()of more than 16 KiB does not copy the bytes that the kernel did not take.NodeHTTPResponseholds them by reference untildrain. These tests check that state.SO_SNDBUFis the size of a socket's kernel send buffer. A set value also stops the kernel's auto-tuning.setSocketOptions(bun:internal-for-testing) accepts anode:httprequest socket since WebSocket: real closing handshake, one failure path,wsrewritten on native transports (consolidates the open WebSocket PRs) #45223.websocket-server-backpressure-buffer.test.tsuses the same two sizes.Notes
Measurements: debug build with ASAN of 8d5672c, linux x64, one run of the file.
lo, whole filelo, the ten changed testsloin/proc/net/dev, read before and after the run. Peak RSS is the largest process of the run. A test file with one small test peaks at 470 MiB on this build.SO_SNDBUF, the kernel takes a whole 1 MiB write and no tail is held.a client disconnect while a large write is draining releases the pin: a probe with the same shape has a tail pending when the client destroys the socket in 20 of 20 rounds. Without the cap at 1 MiB: 0 of 20.tryWriteBody partial: 130893 / 1048576and130932 / 1048576.in the parallel batchannotation for other files, so the scan is not empty.WebAssembly.Memorytest keeps 512 pages (32 MiB) and its 30 s timeout. Its comments describe what the allocator does with the freed 32 MiB block. test: remove the Windows skips that are not needed #46479 and Copy WebAssembly.Memory bytes for native borrows that outlive the call #42192 change that block.describe.concurrent. On the debug build it cuts the file from 14.6 s to 8.4 s, but each child-process test goes from 2.5 s to 4.4 s. The default timeout is 5 s.test/aot/buns-own-tests-as-compiled-codewithAOT_PROGRAMS=node-http-pinned-write: on this debug build the compiled run needs 417 to 636 s, more than the 390 s limit of the driver, with and without this change. Started by hand, it gives 11 passes with and without this change.res.write()returnsfalse,res.writableLengthis above 0 andreq.socket.bytesWrittencounts the whole chunk, also when the kernel takes the whole write. The pin on anArrayBufferis the only sign. The first test asserts it (detachedWhilePending === false) with the sameSEND_BUFFERandCHUNK_SIZE, so it fails on Linux and macOS if these sizes stop leaving a tail.describe.concurrent. At that timesetSocketOptionsdid not accept anode:httpsocket. The PR got no review and the stale sweep closed it.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-pinned-write.test.ts