Repository navigation
test: read the whole line with the child's port in the large-page fault test - #42820
Jarred-Sumner wants to merge 1 commit into
Conversation
…lt test One read of the pipe can return part of the line. Accumulate until the newline, and say so if the child exits first. No-Verification-Needed: test-only change to test/js/bun/jsc/heapStats-mimalloc.test.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe server-port reader now accumulates stdout chunks until it receives a newline, reports premature process exit, and parses the first complete line as the port. ChangesServer-port output parsing
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This test-only change improves robustness when server output arrives in multiple chunks, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward test de-flake that buffers stdout to the newline framing.
What was reviewed:
- The accumulation loop: exits only once a
\nis present, soindexOf("\n")is never -1;donebefore newline throws with the partial buffer for diagnosis. - CRLF handling not needed — the test is
skipIf(!isLinux). - Pipe-drain deadlock: the child writes exactly one line to stdout and nothing after, so releasing the reader without further draining cannot back-pressure the server.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/jsc/heapStats-mimalloc.test.ts, changing how one test reads the port number from a spawned child server's stdout. The old code did a single reader.read() and parsed whatever chunk arrived; the new code loops with a streaming TextDecoder, accumulating until a newline appears, and throws a descriptive error if the stream ends first. It then parses only the substring before the first newline. Net diff is +9/-2 lines, test-only.
Security risks
None. This is a test-only change to how a subprocess's stdout is consumed. No production code, no untrusted input handling, no auth/crypto/network surface is touched.
Level of scrutiny
Low. The change is small, mechanical, and directly implements the pattern REVIEW.md prescribes for this exact situation: "Buffer raw socket/stdout chunks to the protocol's framing before asserting" and "Wire EVERY failure event … to reject the awaited promise." The loop invariant guarantees indexOf("\n") succeeds when reached, and the done branch converts a previously-silent parse of a partial chunk (or hang) into an actionable failure message. The test is Linux-only (skipIf(!isLinux || isASAN)), so Windows CRLF is not a concern. The child process only ever writes a single console.log(server.port) to stdout, so releasing the reader without continuing to drain cannot fill the pipe buffer and deadlock the server.
Other factors
The existing expect(port).toBeGreaterThan(0) guard is preserved, so the de-flake does not weaken what the original assertion protected. The added comment is a single-line why-comment explaining the non-obvious loop, matching repo comment style. No prior reviews or outstanding objections exist on the PR, and the bug hunt exited on dry_streak with no findings or ruled-out candidates.
This branch has not been deployed
1 participant
Follow-up to a review comment on #42808 that was still open when it merged.
The test added there ("a server that gets a request now and then does not fault its buffers in again for every request") read one chunk from the child's stdout and parsed it as the port. A read of a pipe can return part of a line, and then a fragment of the port would be parsed. It now accumulates until the newline, and throws with what it got if the child exits before it printed the line. Nothing else in that file reads a child's output that way (the other tests wait for the whole of
proc.stdout.text()).The file passes 8/8 five times with a release build; with the child changed to exit without printing, the test fails with "the server exited before it printed its port" instead of hanging or parsing
NaN.