test: make test-fs-read-stream-pos.js deterministic (90s worst case → <100ms) - #36775
test: make test-fs-read-stream-pos.js deterministic (90s worst case → <100ms)#36775robobun wants to merge 2 commits into
Conversation
…safety timer The exit path of the upstream test is a pure timing race: the 1ms append interval must land between two consecutive ReadStream preads within a single stream instance (short chunk followed by another 'data' before 'end'). Upstream ships a 90s safety timer for when that race doesn't hit; on the 90s path the assertion block never runs and the test exits 0 without checking anything. Append synchronously in the 'data' handler when a short chunk arrives so the stream's next pread (scheduled after the handler returns) always has bytes to read. Same code path as the race, but the follow-up chunk is guaranteed, so the 90s timer and the runner workarounds from #36478 can go. Also assert the concatenated chunks are byte-for-byte the file's [streamStart, cur) range, which catches a wrong-position pread directly instead of only via broken-line heuristics.
WalkthroughChangesFilesystem stream test parallelization
Possibly related PRs
🚥 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 — test-only de-flake that strengthens the assertion instead of weakening it.
Reviewed: the 'data'-handler ordering argument (sync append lands before the next _read schedules its pread), the seed line guaranteeing the mustCallAtLeast on 'data' can no longer fail under I/O-heavy neighbours, and that bufs/streamStart/cur bookkeeping matches the new [streamStart, cur) byte-exact check across stream restarts. The two removed runner.node.mjs special cases have no other references; isParallelSafeTest collapses cleanly to the path check.
Extended reasoning...
Overview
Two files: test/js/node/test/parallel/test-fs-read-stream-pos.js (upstream Node parallel test) and scripts/runner.node.mjs (CI test runner). The test change replaces a 90 s timing-race exit path with a deterministic one — a synchronous append inside the 'data' handler after the first short chunk, so the same stream's next pread is guaranteed to see new bytes and hit the assertion block. It also seeds one line before the first stream and adds a byte-exact deepStrictEqual of the concatenated chunks against the file's [streamStart, cur) slice, which directly targets the nodejs/node#33940 wrong-position bug the test was written for. The runner change removes the 120 s per-file timeout override and the serial-only carve-out added in #36478, both now dead.
Security risks
None. Test-only and CI-runner-only; no runtime, native, or user-facing code touched.
Level of scrutiny
Low-to-moderate. It rewrites an upstream Node test, so the REVIEW.md bar is "when de-flaking, keep asserting the property the original assertion protected." That's met and exceeded: the original brokenLines heuristic is retained verbatim and a strictly stronger byte-exact check is added on top. The 90 s safety-timer path being removed was itself a silent no-op exit (asserted nothing), so nothing of value is lost. The ordering claim — 'data' fires synchronously from push, next _read goes through process.nextTick — is standard Readable semantics in flowing mode and the PR verified it empirically in both Bun and Node.
Other factors
bufsis reset only in'end'(which also nullsstream), andstreamStart = curis captured only whenstreamis null, so the newread.length === cur - streamStartinvariant holds by construction across stream restarts.- The seed
appendLine()guarantees ≥11 bytes before the first stream, so withhwm = 10a short tail chunk always exists in stream 1; combined with the in-handler append, theisLowassertion path is reached on the first stream every run (author confirmed on linux+windows). - No stale references remain: grepped for
test-fs-read-stream-pos— onlytest/expected-durations.json(94 ms, consistent with the new sub-100 ms runtime) and the test file itself.test/parallel-allowlist.jsonhas no per-file entry for it.isParallelSafeTestreduces to the plain path check with no dead branches. - Verification section is thorough: multi-platform release + debug+ASAN runs, Node cross-check, concurrent-neighbour stress, and a negative test simulating the original regression that fails the new assertion.
|
Updated 7:07 AM PT - Aug 2nd, 2026
❌ @robobun, your commit b8035fa has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36775That installs a local version of the PR into your bun-36775 --bun |
![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/test/parallel/test-fs-read-stream-pos.js`:
- Around line 18-39: Revert all local modifications to the vendored Node.js test
test-fs-read-stream-pos.js, including the writer changes, byte-range assertions,
short-read timing changes, and endTimer removal, restoring the upstream version.
Do not add fixes to this mirror; submit the deterministic change upstream to
nodejs/node first, then resync the mirror. If interim Bun coverage is required,
add a separate Bun-owned test outside the verbatim Node.js port.
🪄 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: 2eb6c9b8-d7ea-49cf-97bc-a0c5c8710d46
📒 Files selected for processing (2)
scripts/runner.node.mjstest/js/node/test/parallel/test-fs-read-stream-pos.js
💤 Files with no reviewable changes (1)
- scripts/runner.node.mjs
|
CI status:
Ready for a maintainer. |
There was a problem hiding this comment.
LGTM — test-only de-flake that strengthens assertions and removes now-dead runner workarounds.
What was reviewed:
- Traced the deterministic exit path: seeded 11-byte line + hwm=10 guarantees a short tail chunk on the first stream; synchronous
appendLine()in the short-chunk handler is ordered before the next_read(scheduled via nextTick), so the follow-up'data'event is guaranteed and the assertion block always runs. - Confirmed the new
deepStrictEqual(read, onDisk.subarray(streamStart, cur))directly catches nodejs/node#33940 (wrong-position pread), strictly stronger than the retained broken-line heuristic;mustCallAtLeastwrappers preserved. - Runner changes: the 120s override and serial exclusion in
scripts/runner.node.mjsare dead once the 90s safety timer and interval-starvation dependency are gone; default 20s CI timeout is ample for a <100ms test.
Extended reasoning...
Overview
Two files touched, both test infrastructure: test/js/node/test/parallel/test-fs-read-stream-pos.js (Node-parallel test for the nodejs/node#33940 ReadStream position bug) and scripts/runner.node.mjs (removes the 120s per-test timeout override and the parallel-safe exclusion added for this file in #36478). No production code is modified.
The test change replaces a timing race (1ms background writer must land between two consecutive preads) with a deterministic trigger: when a short chunk arrives, append a line synchronously inside the 'data' handler so the stream's next scheduled _read is guaranteed to find bytes. The 90s safety timer — which let the test exit 0 without asserting anything when the race never hit — is removed. The assertion block gains a direct byte-range equality check against the file on disk.
Security risks
None. This is a test file operating on a temp-directory scratch file plus a mechanical removal of two special-case entries from the CI runner script.
Level of scrutiny
Moderate for a test change. REVIEW.md's de-flaking rule ("keep asserting the property the original assertion protected") is the governing bar, and this PR clears it: the original broken-line assertion is retained, a strictly stronger deepStrictEqual on the exact byte range is added, the mustCallAtLeast wrappers are kept, and the previously-reachable "exit at 90s having asserted nothing" path is eliminated. The PR description includes fault-injection verification (a custom options.fs.read that rewinds after a short read fails the new assertion in both Bun and Node), timing measurements on both platforms including under I/O contention, and confirmation that upstream Node passes the modified test.
I checked the determinism argument: the seeded line hello at 1\n is 11 bytes vs hwm=10, so the first stream always produces a 10-byte chunk followed by a short tail; appendLine() runs synchronously in the handler before the next _read (which is nextTick-scheduled after push returns), so the follow-up pread always finds bytes and the isLow assertion block always executes. No hang path remains after removing the 90s timer.
Other factors
The runner deletions are exactly the two workarounds #36478 added for this file's timing dependency; with the dependency removed they are dead code and correctly cleaned up in the same PR. node --check and parallel-allowlist.test.ts were run per the description. The change is self-contained, well-reasoned, and thoroughly verified across platforms and runtimes.
1 participant
What
The exit path of the upstream test is a pure timing race: the 1 ms append interval must land between two consecutive
ReadStreampreads within one stream instance (a short chunk followed by another'data'before'end'). Upstream ships a 90 s safety timer for when the race doesn't hit. On that path the assertion block never runs, so the test exits 0 without checking anything.This PR appends synchronously inside the
'data'handler the moment a short chunk arrives. The stream's next_readis scheduled after the handler returns, so that pread is guaranteed to find the appended bytes. It is exactly the code path the race was trying to reach (short read, file grows, next pread from the same stream), just without the race. The 90 s safety timer and the two runner workarounds from #36478 are then dead and are removed.The assertion block now also checks that the concatenated chunks are byte-for-byte the file's
[streamStart, cur)range. nodejs/node#33940 was a wrong-position pread after a short read; this catches it directly (duplicated or skipped bytes fail the equality) instead of only via the broken-line heuristic.The 1 ms background writer and the
common.mustCallAtLeastwrappers are kept so the test still reads a growing file the way the original does.Why
Build #87550 spent 90 s on this file on Windows 2019 x64, and #36478 measured 1-40 s over 30 solo Windows runs. When it reaches 90 s the test has asserted nothing.
'data'runs synchronously from the read callback'spush, and_readis re-scheduled viaprocess.nextTickafterpushreturns, so a synchronous append inside the handler is ordered strictly before the next pread in both Bun'sreadStreamPrototype._readand Node'sReadStream.prototype._read. Forhwm = 10and 11/12-byte lines, the running file size is never a multiple of 10 whilecounter ≤ 105, so the first stream always produces a short tail chunk and the test exits in that stream.Orthogonal to #36548 / #36479: those address the event-loop ordering that made the race hard to hit; this makes the test independent of the race so neither the 120 s runner ceiling nor the serial scheduling is needed.
Verification
Release (
USE_SYSTEM_BUN=1 bun test/js/node/test/parallel/test-fs-read-stream-pos.js):test-fs-read-stream-fd-leak.jsneighboursmustCallAtLeastper #36478)bun bd test/js/node/test/parallel/test-fs-read-stream-pos.js(linux x64 debug+ASAN): 3.5-3.9 s before, 3.3-4.1 s after. Both are dominated by debug startup and lazynode:streamload; the test body itself is a handful of preads either way.Node passes the modified test in 60-111 ms on both platforms.
Assertion strength: an instrumented run confirms the
isLowbranch is reached in the first stream on every run on both platforms, and acreateReadStreamwith a customoptions.fs.readthat rewinds the position by one byte after a short read (simulating nodejs/node#33940) fails the newdeepStrictEqualin both Bun and Node.bun test test/internal/parallel-allowlist.test.ts: 2/2 pass.node --check scripts/runner.node.mjs: ok.no test proof · iteration 1 · docs-only change; test-proof not applicable