ci(runner): run test-fs-read-stream-pos.js serially with a 120s ceiling

[robobun]

Fixes test/js/node/test/parallel/test-fs-read-stream-pos.js going red on main (seen on builds 84352, 84676, 85400, 85630, 85664, 85692, 85720-85866 across win2019, alpine/ubuntu/debian aarch64, and debian x64-asan).

Cause

The test's exit path is a pure timing race: a setInterval(append, 1) writer must land a write between two consecutive ReadStream preads within a single stream instance (partial chunk → another 'data' before 'end'). Upstream ships it with a 90-second safety timer for exactly that reason. In addition, each stream's 'data' handler is wrapped in common.mustCallAtLeast(1), so any stream that observes zero bytes fails the process on exit.

Two recent PRs together pushed both failure modes over the runner's 20 s ceiling:

• win: high-resolution event-loop timer via waitable timer + IOCP #34834 raised Windows event-loop timer resolution from ~15 ms to ~1 ms. With the old resolution the writer and reader both fired on the same ~15 ms tick and each stream covered ~1 write (so the race hit in ≤25 cycles); with 1 ms resolution each stream covers ~10 writes, the partial chunk is always last, and the per-stream hit rate drops to ~1/500. 30 solo runs on a Windows box: 1 s–40 s, max 40 s, 5 runs >20 s.
• ci: run the fast, non-flaky test files as one bun test --parallel batch per shard #36175 removed the cpuCount: 2 clamp on the linux/windows test agents, taking the parallel-safe phase from width 1 to width 3. Running alongside test-fs-read-stream-fd-leak.js (50× createReadStream at 2 ms) starves the 1 ms appender long enough for a later stream to be created with start == EOF, producing zero 'data' events and failing mustCallAtLeast with "Mismatched function calls. Expected at least 1, actual 0." (e.g. build 85400 debian aarch64, attempts 1–2).

Neither changes what the test is actually checking (ReadStream position tracking while a file is being appended, nodejs/node#33940), which has never failed here; all observed reds are the race not being reached inside the runner's 20 s window, or the per-stream mustCallAtLeast tripping under I/O contention.

Change

In scripts/runner.node.mjs:

• isParallelSafeTest now returns false for this file, so it runs in the serial phase. With no I/O neighbours the 1 ms writer keeps its cadence and every stream sees at least one write, so the mustCallAtLeast failure cannot occur.
• getNodeParallelTestTimeout gives it 120 s, so on the occasions the race does not hit within 90 s the test's own safety timer fires and the process exits 0 (Node's python runner also allows 120 s per test/parallel file).

Same assertions and code path; only the runner's scheduling of this file changes.

Verification

• Windows x64 solo, 30 consecutive runs with the release canary: 30/30 pass, times 1 s–40 s (median ~6 s), none reached the 90 s safety timer, none exited non-zero.
• node scripts/runner.node.mjs --include js/node/test/parallel/test-fs-read-stream-pos.js on linux: file is scheduled in the serial phase (not under "Running N parallel-safe tests"), passes.
• bun test test/internal/parallel-allowlist.test.ts: 2/2 pass.

The event-loop ordering difference (Bun drains queued thread-pool completions before yielding to due timers, Node yields to the timer phase between poll iterations) is a pre-existing behaviour not introduced by either of the two PRs above; changing it is out of scope for unbreaking this test.

---

no test proof · iteration 0 · build/CI scripts only; test-proof not applicable

[coderabbitai]

Walkthrough

Changes

Node test runner

Layer / File(s)	Summary
Parallel eligibility and timeout rules scripts/runner.node.mjs	Parallel-safe tests are limited to the Node and Bun parallel directories; test-fs-read-stream-pos.js runs serially and receives a 120-second timeout override.

Possibly related PRs

• oven-sh/bun#33622: Updates the runner’s test scheduling and parallel-safe execution rules.
• oven-sh/bun#35944: Excludes specific tests from the parallel phase.
• oven-sh/bun#36175: Refactors parallel test classification and batching.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly states the key change: running test-fs-read-stream-pos.js serially with a 120s timeout.
Description check	✅ Passed	The description covers what changed and how it was verified, though it uses custom headings instead of the template labels.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — CI runner scheduling tweak only, no runtime code touched.

What was reviewed:

• getNodeParallelTestTimeout: new 120s entry follows the existing .includes() pattern and is placed before the !isCI/asan fallbacks, so it applies on all lanes.
• isParallelSafeTest: the refactor from return A || B to if (!A && !B) return false; …; return true is behavior-preserving for every other path; only test-fs-read-stream-pos.js is newly routed to the serial phase.
• Confirmed .endsWith("test-fs-read-stream-pos.js") won't accidentally catch other test files (no other test-fs-read-stream-pos* exists).

Extended reasoning...

Overview

This PR touches only scripts/runner.node.mjs, the CI test runner. It adds two narrowly scoped overrides for a single upstream Node parallel test (test-fs-read-stream-pos.js): a 120 s per-file timeout in getNodeParallelTestTimeout, and an exclusion from the parallel-safe phase in isParallelSafeTest so it runs serially. No production/runtime code, no test assertions, and no test files are modified.

Security risks

None. This is CI orchestration JavaScript that decides scheduling and timeouts for test files; it does not touch auth, crypto, network, or any code that ships in the Bun binary.

Level of scrutiny

Low. The change is mechanical and mirrors four existing per-file timeout overrides directly above it (test-dns, test-cluster-, -docker-, test-stdin-pipe-large) using the same testPath.includes(...) idiom. The isParallelSafeTest change is a small, behavior-preserving De Morgan refactor of the existing predicate plus one .endsWith() exclusion, with a comment explaining the I/O-contention failure mode. Both edits carry explanatory comments consistent with the file's style.

Other factors

• The new timeout entry sits before the !isCI / -asan- fallbacks, so it takes effect on every lane — matching the intent (the flake was observed on win2019, aarch64, and asan).
• The PR description gives a detailed root-cause analysis (timer-resolution change + concurrency-width change) and verification (30 solo Windows runs, serial-phase scheduling confirmed, parallel-allowlist.test.ts passes). Per REVIEW.md, this is a de-flake that keeps the original assertion intact rather than weakening it — only the runner's scheduling changes.
• No prior review comments on the timeline to address.

[claude]

LGTM — CI runner scheduling tweak that follows the existing per-test override pattern.

What was reviewed:

• getNodeParallelTestTimeout: new 120s override sits before the !isCI/asan fallbacks so it applies in CI, matching neighbouring .includes() clauses.
• isParallelSafeTest: refactor is behaviour-preserving for all other paths; the endsWith check runs on the already-normalized forward-slash path so it matches on Windows too.
• Confirmed test/js/node/test/parallel/test-fs-read-stream-pos.js exists and both call sites (serialTests/parallelSafeTests partition, per-test timeout) consume the new values.

Extended reasoning...

Overview

Two small edits to scripts/runner.node.mjs, the CI test runner: (1) a 120 s timeout override for test-fs-read-stream-pos.js in getNodeParallelTestTimeout, and (2) an exclusion of that same file from the parallel-safe phase in isParallelSafeTest. No runtime, test, or product code is touched.

Security risks

None. This only changes how the CI harness schedules and times out one Node parallel test. No user input, no auth, no crypto, no permissions.

Level of scrutiny

Low. This is CI infrastructure — a per-file scheduling override that mirrors several existing entries in the same function (test-dns, test-cluster-, -docker-, test-stdin-pipe-large). The isParallelSafeTest refactor from a single boolean expression to early-return + exclusion + return true is trivially equivalent for every other path. The exclusion uses p.endsWith("test-fs-read-stream-pos.js") on the backslash-normalized path, so it works on Windows as well as POSIX.

Other factors

The 120 s ceiling is placed before the !isCI and -asan- 60 s fallbacks, so it takes effect on CI (where the flake occurs) and leaves headroom for the test's own 90 s safety timer to fire and exit 0. The PR description gives a well-traced root cause (timer resolution change + parallel width change) and cites 30 solo verification runs. The comments added inline are load-bearing (explain why the exclusion exists) rather than narrative. The referenced test file exists in-tree. No outstanding reviewer comments.

[robobun]

Self-review flagged that the in-code comment attributed the aarch64 solo variance to #34834, which is a Windows-only libuv change. Reworded in af28a9a to scope the measured 1-40s to Windows and cite the aarch64 >20s observations by CI build number instead.

Build 85882 (first CI run): test-fs-read-stream-pos.js passed on every lane. The one red (bun-install-registry.test.ts peer hoisting on win11 aarch64) is unrelated to this runner-only change and reported separately.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

scripts/runner.node.mjs (1)

641-650: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the parallel allowlist path-boundary aware.

Line [643] uses includes(), so any path containing js/node/test/parallel/ or js/bun/test/parallel/ as a substring can be treated as parallel-safe even when it is not under an approved test directory. Use a normalized, path-segment-bounded comparison while preserving support for absolute paths.

🤖 Prompt for 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.

In `@scripts/runner.node.mjs` around lines 641 - 650, Update isParallelSafeTest to
recognize approved parallel directories only when the normalized path has a
path-segment boundary around js/node/test/parallel or js/bun/test/parallel,
rather than matching arbitrary substrings. Preserve support for absolute paths
and keep the existing test-fs-read-stream-pos.js serial override unchanged.

🤖 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.

Outside diff comments:
In `@scripts/runner.node.mjs`:
- Around line 641-650: Update isParallelSafeTest to recognize approved parallel
directories only when the normalized path has a path-segment boundary around
js/node/test/parallel or js/bun/test/parallel, rather than matching arbitrary
substrings. Preserve support for absolute paths and keep the existing
test-fs-read-stream-pos.js serial override unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1cc88cf1-eb1f-4479-86c3-70661dcae81e

📥 Commits

Reviewing files that changed from the base of the PR and between e70ddff and af28a9a.

📒 Files selected for processing (1)

• scripts/runner.node.mjs

[claude]

LGTM — CI-runner-only de-flake that moves one Node parallel test to the serial phase and gives it a 120s ceiling.

What was reviewed:

• isParallelSafeTest refactor is behavior-preserving for all other paths; the new endsWith check runs on the backslash-normalized path so Windows matches.
• Confirmed getNodeParallelTestTimeout is applied at the spawnBun call site regardless of serial/parallel phase, so the 120s override still takes effect after the test is moved out of the parallel-safe set.
• No other test file matches the test-fs-read-stream-pos substring; the timeout override is placed before the !isCI / -asan- fallbacks so it wins in all environments.

Extended reasoning...

Overview

Two additions to scripts/runner.node.mjs: a 120 000 ms entry in getNodeParallelTestTimeout for test-fs-read-stream-pos, and an exclusion in isParallelSafeTest so that file runs in the serial phase instead of the width-N parallel-safe phase. No runtime, test, or fixture code is touched — this is purely CI test-runner scheduling.

Security risks

None. The change only affects how the CI harness schedules and times out one upstream Node test file.

Level of scrutiny

Low. This is a targeted CI de-flake following the exact pattern already used for test-dns, test-cluster-, -docker-, and test-stdin-pipe-large in the same function. The isParallelSafeTest rewrite from a one-line || to guard-then-exclude is mechanically equivalent for every other path.

Other factors

• Verified the timeout function is invoked at runner.node.mjs:919 for all node-test spawns, not just the parallel-safe branch, so moving the file to serial does not lose the 120 s override.
• The endsWith("test-fs-read-stream-pos.js") check operates on p (already replaceAll("\\\\", "/")), so it matches on Windows.
• Only one file in the tree matches test-fs-read-stream-pos, so the .includes substring in the timeout table is unambiguous.
• PR description documents empirical verification (30 solo Windows runs, serial-phase scheduling check, parallel-allowlist.test.ts still green) and a well-reasoned root cause tied to #34834 and #36175. The inline comments capture the durable "why" per repo convention.

[claude]

LGTM — CI runner scheduling tweak only, no runtime code touched.

What was reviewed:

• isParallelSafeTest refactor is behavior-preserving for all other paths (same two includes checks, inverted to early-return) with one new exclusion.
• Timeout override follows the exact pattern of neighboring entries in getNodeParallelTestTimeout; 120s correctly exceeds the test's own 90s safety timer.
• Confirmed getNodeParallelTestTimeout is called with title (forward-slash normalized), so the .includes match works on Windows too.

Extended reasoning...

Overview

Two small edits to scripts/runner.node.mjs, the CI test runner:

1. getNodeParallelTestTimeout: adds a testPath.includes("test-fs-read-stream-pos") → 120_000 entry alongside the existing dns/cluster/docker/stdin overrides.
2. isParallelSafeTest: converts the single-expression return into an early-return guard, then excludes test-fs-read-stream-pos.js so it runs in the serial phase.

No runtime, bindings, or test-file changes. The refactor of isParallelSafeTest preserves the prior semantics exactly for every other path — the two p.includes(...) checks are the same, just De Morgan'd into an early return false.

Security risks

None. This is CI orchestration script logic that only affects which phase a test file is scheduled in and how long the runner waits before killing it.

Level of scrutiny

Low. This is a de-flake change to test-runner scheduling, following the established pattern already used in both functions (string-match on test path → override). The PR description provides a detailed root-cause analysis tying the flake to #34834 (Windows timer resolution) and #36175 (parallel width unclamp), and CI build 85882 confirms the target test now passes on every lane. The repo review guide's caution about "raising per-test timeouts to make a slow test pass" doesn't apply here — the test isn't slow, its exit condition is a probabilistic race that upstream Node already guards with a 90s safety timer, and 120s simply lets that timer fire instead of the runner's 20s SIGKILL.

Other factors

• Checked that getNodeParallelTestTimeout is invoked with title (already forward-slash normalized elsewhere in the runner), so .includes("test-fs-read-stream-pos") matches on Windows paths.
• p.endsWith("test-fs-read-stream-pos.js") is applied after replaceAll("\\\\", "/"), so it's separator-safe.
• No CODEOWNERS on scripts/runner.node.mjs; no outstanding reviewer comments.
• The in-code comments are on the longer side but carry non-obvious context (why this specific test races, which PRs changed the timing, which builds observed it) that would otherwise require re-deriving from CI archaeology.