test(hot): skip transient file-access errors from Windows rm+rename gap - #34043
Conversation
On Windows, writeHotFileAtomicSync does rmSync(path) before renameSync(tmp, path) because renameSync EPERMs when --hot has the destination open. Between the rm and the rename, the entry file briefly does not exist; a --hot reload that lands in that window prints 'Module not found', 'ENOENT reading', or 'EPERM reading' (delete pending) instead of the expected 'error: N'. driveErrorReloadCycle now skips those lines and keeps reading. The rename's own watcher event drives the real reload, so re-saving on these lines would only race with it. Before: 29/50 pass on windows-x64, similar on windows-aarch64 After: 80/80 on both, full suite 15/15 on both, linux debug build unchanged
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted test-only de-flake for a Windows-specific race in the hot-reload harness.
What was reviewed:
- Checked the new skip regex
/Module not found|\w+ reading "/cannot match the realerror: <N>lines the test asserts on, so no false passes — a genuine module-not-found regression would still time out rather than pass. - Verified placement: the skip runs after the
error:filter and before the stale-counter/assertion branches, sotriggeredand buffer state stay consistent when a transient line is skipped. - Confirmed no production code touched; the added comment on
writeHotFileAtomicSyncjust cross-references the skip.
Extended reasoning...
Overview
Single-file test change to test/cli/hot/hot.test.ts. Adds a one-line regex guard inside driveErrorReloadCycle that skips transient Module not found / ENOENT reading / EPERM reading error lines that --hot can print on Windows when a reload lands in the gap between writeHotFileAtomicSync's rmSync and renameSync. Also expands two comments to document the cross-reference between the helper and the skip.
Security risks
None. Test-harness-only change; no production code, no external inputs.
Level of scrutiny
Low. This is a de-flake of a known-flaky Windows CI test. The PR description traces the root cause (rm+rename gap introduced in #30412), explains why the fix is a skip rather than a re-save (re-saving raced the rename's own watcher event and produced stale-sourcemap coords), and includes 80/80-run verification on both Windows arches plus a Linux no-regression check.
Other factors
- The regex is narrow enough: real assertion targets are
error: <number>which cannot containModule not foundor\w+ reading ". If the pattern ever fired on every reload (a real regression),reloadCounterwould never advance and the test would time out — it can't silently pass. - Loop state after
continueis correct:triggeredstays as-is, so a chunk containing only a transient error triggers noonReload, matching the PR's stated intent that the rename's own watcher event drives the next reload. - The 5-line comment technically exceeds CLAUDE.md's 3-line guideline, but this file already has several longer race-condition explainer comments in the same style; verifiers ruled this out as a non-issue.
- No prior reviews, no outstanding comments, no CODEOWNER on this test file.
There was a problem hiding this comment.
LGTM — small, well-scoped test-harness de-flake for a Windows-only race.
What was reviewed:
- Regex
/Module not found|\w+ reading "/cannot match the expectederror: <N>lines, so no false-positive skips. - Placement (after the target-count check, before the stale-counter branch and assertion) means remaining buffered lines are still processed via
continue. - The early buffer gate
/error: .*[0-9]\n/interacts fine — either the transient line buffers until a numeric error arrives, or it is skipped when lines are split.
Extended reasoning...
Overview
Test-only change to test/cli/hot/hot.test.ts. Adds a one-line regex skip in the shared driveErrorReloadCycle helper so transient Module not found / ENOENT reading "…" / EPERM reading "…" lines emitted by --hot during the Windows rmSync+renameSync gap in writeHotFileAtomicSync are ignored instead of failing the expect(line).toContain("error: N") assertion. Also expands the existing comment on writeHotFileAtomicSync to cross-reference the skip. No production code touched.
Security risks
None. Pure test-harness change; no runtime, auth, crypto, or user-facing code paths involved.
Level of scrutiny
Low. This is a targeted CI flake fix in a test helper, addressing a race that the same file's writeHotFileAtomicSync (added in #30412) itself introduced on Windows. The mechanism is clearly explained in both the PR description and the inline comment, and the verification table shows 80/80 pass on both Windows arches vs ~29/50 before. The regex is narrow enough that it cannot accidentally match the numeric error: <counter> lines the tests actually assert on, and \w+ reading " is specific to Bun's transpiler read-error format.
Other factors
- The
continuepreserves the rest of the buffered lines in the current chunk, so a real numeric error following the transient one in the same chunk is still processed. - The choice to skip (not re-save) is deliberate and explained: the rename's own watcher event already drives the next reload, and re-saving would race it — the PR notes an earlier revision that re-saved produced stale-sourcemap coords.
- No prior human or bot review comments to address; bug-hunting system found nothing.
|
Status: The remaining red lanes are unrelated pre-existing flakes that this diff cannot touch (only
Ready for review/merge. |
POSIX rename is atomic, so 'Module not found' / 'ENOENT reading' / 'EPERM reading' on the entry during these tests would be a real bug there and should fail loudly.
There was a problem hiding this comment.
LGTM — test-only Windows flake fix, gated so POSIX still fails loud on the same errors.
What was reviewed:
- Skip is placed after the
error:filter and before both the stale-counter re-save and theexpect(...toContain)assertion, so transient lines are neither re-saved (avoiding the race the PR notes) nor asserted on. - Regex
\w+ reading "is broad but only reachable on Windows insidedriveErrorReloadCycle, where all file writes are harness-driven — any such line is the rm+rename gap by construction. isWindowsexport exists intest/harness.ts; other tests in the file (including "preload not found") don't route through this helper and are unaffected.
Extended reasoning...
Overview
Modifies only test/cli/hot/hot.test.ts: imports isWindows from harness, adds a Windows-gated continue in driveErrorReloadCycle for stderr lines matching Module not found / \w+ reading ", and expands the existing comment on writeHotFileAtomicSync to cross-reference the skip. No runtime code is touched.
Security risks
None. Test harness change; no auth, crypto, network, or user-facing input handling.
Level of scrutiny
Low. This is a targeted de-flake of a known Windows CI failure, confined to a single test helper. The change is additive (one gated continue) and cannot alter behavior on non-Windows platforms. The mechanism is well-explained: writeHotFileAtomicSync on Windows must rmSync before renameSync (from #30412), leaving a brief window where a concurrent --hot reload sees the file missing and prints a resolver/transpiler error that the test previously asserted against as if it were the expected error: N line.
Other factors
- Placement is correct: the skip sits after the
line.includes("error:")gate and thereloadCounter >= targetCountcheck, but before the stale-counter re-save branch and theexpect(line).toContain(error: ${reloadCounter})assertion. Choosingcontinue(keep reading) over re-saving is deliberate — the rename's own watcher event drives the real reload, and the PR description notes an earlier re-saving revision introduced its own race. - The
isWindowsgate preserves signal on POSIX, whererename(2)is atomic and these errors would indicate a real regression — consistent with the repo's "never silently weaken an existing test" guidance. - The regex is intentionally loose (
\w+ reading "covers ENOENT/EPERM/etc.), but it only runs on lines already containingerror:in a helper whose sole callers write the entry file themselves viawriteHotFileAtomicSync, so there's no plausible non-transient match to mask. - CI confirms the fix:
hot.test.tspassed on all lanes across two builds including the previously-red Windows 11 aarch64; remaining red lanes are unrelated files this diff cannot affect. The commit history shows the gate was tightened from all-platforms to Windows-only in a follow-up commit, which is the right call.
…34166) `test/regression/issue/20144/20144.test.ts` has been flaking on the macOS aarch64 lanes (102 of the last 500 branch builds carry it in their annotations) and went fully red on darwin 14 aarch64 in build [72915](https://buildkite.com/bun/bun/builds/72915): ``` AssertionError: Expected values to be strictly equal: + 'SIGKILL' - 'SIGINT' ✗ should not time out [1010.48ms] ``` ### Cause The test spawns a child that runs `bun:ffi`'s `cc()` and then sends an IPC `"hej"` before entering `while (true)`. The parent replies with `SIGINT`, and the assertion checks the child exited via `SIGINT`. A `spawn` option of `timeout: 1000, killSignal: "SIGKILL"` is the hang guard. That 1s budget is spent almost entirely on child startup and the `cc()` compile, not on the property being tested. On an idle macOS box the round-trip is ~30 ms; on a local debug+ASAN build it is ~1.7 s, and under concurrent test-runner load on the macOS tart VM runners it crosses 1 s in release too, so `node:child_process` fires `SIGKILL` before `"hej"` ever arrives. There is no `src/` culprit in the window where the rate jumped: the first heavily affected build (72519) and the adjacent clean one (72517) share the same merge-base (`7d44148cfa`), and the only main commits between the surrounding bases are test-only (#34043, #34081). This is a latent tight timeout that CI load pushed over the edge. ### Fix * Raise the kill switch to `20_000` ms to match the other hang-guard tests in the repo (e.g. `test/js/bun/css/angle-serialization-hang.test.ts`). The regression from #20144 was a permanent hang, so the guard still fires on a real regression; it just stops racing startup. * Spawn with `bunExe()` / `bunEnv` so the child runs with the normalized test environment instead of inheriting the CI runner's full env. * Replace the `done` callback with `Promise.withResolvers`, wire `child.on("error")` to reject, and assert the exit shape with `expect(...).toEqual({ code: null, signal: "SIGINT" })` plus a check that the ready message was actually received. ### Verification ``` $ bun bd test test/regression/issue/20144/20144.test.ts (pass) should not time out [1720.56ms] ``` (Three consecutive passes on debug+ASAN; three on release. The debug duration alone shows the old 1 s guard was never safe.) <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
2 participants
Fixes
test/cli/hot/hot.test.tsgoing red on Windows lanes (build 72241, Windows 11 aarch64).Repro
Reproduces locally on both Windows x64 and aarch64: 21/50 fail rate on x64, similar on aarch64.
Cause
writeHotFileAtomicSyncon Windows doesrmSync(path)thenrenameSync(tmp, path)(added in #30412), becauserenameSyncover an existing file EPERMs while--hothas the destination open for reading. Between the rm and the rename the entry file briefly does not exist. A--hotreload that lands in that gap prints one of:error: Module not found '<path>'(resolver)error: ENOENT reading "<path>"(transpiler read)error: EPERM reading "<path>"(transpiler read, delete pending)driveErrorReloadCyclematched these atline.includes("error:")and then asserted they containerror: ${reloadCounter}, which fails.The "stale sourcemap" test hits this much more often than the sibling "sourcemap generation" test because its hot file self-writes
__filenameon every run, so there is almost always a reload in flight when the test's nextwriteHotFileAtomicSyncruns.Fix
driveErrorReloadCyclenow skips these transient file-access error lines and keeps reading. The rename's own watcher event drives the real reload, so re-saving from the harness on these lines would only race with it (an earlier revision that did re-save occasionally produced stale-sourcemap coords on x64 from exactly that race).Verification
hot.test.tssuite, both Windowsbun bd test hot.test.tsno test proof · iteration 1 · Platform-specific test-only change; deferring to CI.