Repository navigation
Conversation
The two tests save main.js in place. fs.writeFileSync writes over the file and then resizes it, which is two watcher events. When --hot reloads between them, the second load runs twice and prints "collected" twice, or the third load parses the new file followed by the tail of the old one. The three saves now have the same length and differ in one digit, so a read always gets one whole version. The second load does its work once. The 20,000 promises are made before the first catch(), so the collected cell goes to one of them and not to a promise that catch() returns.
Contributor
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 22 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
Comment |
Collaborator
Author
|
Status: ready for review. This PR changes tests only. How the failure was reproduced: a probe did the syscalls of
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
does not take a promise of the program's, %s, for that of the entry pointfails on Linux in 75 of the last 400 CI builds:"collected"twice on stdout (47), orerror: Expected ";" but found ")"on stderr (30).main.jsin place.fs.writeFileSyncwrites over the file, then callsftruncate(src/runtime/node/node_fs.rs:7376): two watcher events. A reload between them runs the second load twice, or parses the new file plus the old tail.rejected and handledcase. It fails only 6 of 20 runs: the promisecatch()returns can get the collected cell.Fix
catch().bun bd test test/cli/hot/hot.test.tspasses. With 50 ms between the two calls, the old tests fail 8 of 8 and the new pass 8 of 8. A build from before --hot: fix a use-after-free of the entry point's promise #44350 fails the new tests 6 of 6.Background
bun --hotruns the entry point again when a watched file changes. The watcher waits 100 µs for more events (src/watcher/INotifyWatcher.rs:70). A later event is a second reload (Duplicate Output Issue After Repeated Saves withbun --hot#13511).--hotread the entry point's promise after its collection.writeHotFileAtomicSync(write, then rename). On Windows it removes the file first, and a reload in that gap prints an error. A rename is also two events.Notes
This PR changes tests only. No file under
src/changes.Source of the CI numbers. Annotations of the 400 most recent finished builds, 122998 to 123628 (2026-10-02 to 2026-10-06). The two tests fail in 97 of them. Every hit passed on the retry.
"collected"twiceExpected ";" but found ")"first loadThe Windows timeouts are not changed by this PR. There the first save, made right after
first load, never reloads. #40017 (open) describes a change that Windows misses right after the entry point first runs. I did not run Windows, so I did not confirm that it is the same failure.What a reader sees during
fs.writeFileSync. One process rewrites a file in a loop with a 201-byte and a 44-byte content. A second process reads it in a loop. With Bun as the writer, 82,652 of 727,000 reads returned the 44 new bytes followed by the last 157 old bytes, and none returned an empty file. With Node 26.3.0 as the writer (O_TRUNC), 459,284 of 860,000 reads returned an empty file and 2 returned such a mix.The stderr of the second signature is that content. The second load is longer than the third.
console.log("third load"); process.exit(0);is 43 bytes, and byte 43 of the second load is the"fs")ofrequire("fs").readFile(__filename, () => {.Both signatures from a delay between the two calls. A probe did the syscalls of
fs.writeFileSyncby hand (openSyncwithoutO_TRUNC,writeSync, a busy wait,ftruncateSync). Release build (367d939), the same three saves with a second load that has no collections and no promises, 40 runs per row:collectedtwiceThe parse error is the one from CI, character for character. The same probe around the real tests, debug build of
main, 8 tests per cell:The debug build starts a reload late, so a second event that is a few milliseconds behind often still joins the first reload.
Without the delay I could not make the old tests fail on this machine (ext4, 12 cores): 560 runs of the write pattern on a release build, pinned to one CPU next to busy loops for some of them, all passed. In CI the second event comes late often enough.
Why one digit. Two versions of the same length that differ in one byte have no state in between: a read that sees part of a write still sees one of the two. That holds for the write strategy of today (no
O_TRUNC) and for a truncate followed by a write, where the state in between is an empty file that loads and prints nothing.Why the second load has a guard.
--hotcan still reload twice for one save. The older tests in this file accept that too (expect(reloadCounter).toBeGreaterThanOrEqual(3), anddriveErrorReloadCyclesaves again when it sees the previous error). #30617 (open) makes the watcher wait longer before it posts a reload. This PR does not depend on it.Why a rename does not remove the second reload. A rename over the entry point gives a
MOVED_TOevent on the directory and aDELETE_SELFevent on the replaced file.src/jsc/hot_reloader.rs:1051handles the case that they land in separate reads, and each posts a reload.The
catch()change.Promise.reject(e).catch(() => {})makes two promises: the rejected one and the one thatcatch()returns, which is fulfilled a moment later. The old loop made them in turn, so the collected cell went to either kind. A fulfilled promise in that cell looks like an entry point that loaded, and the unfixed build passes. "Rejected and handled" case, release build from before #44350 (367d939), 20 runs per row:catch()callscatch()callsThe
pendingcase fails every run in all four rows: both of its promises stay pending.Other runs. The two new tests under 16 busy loops on 12 cores, debug build: 16 of 16 pass. The whole file on the debug build: 16 pass, 0 fail.
Not changed.
holds the promise of the entry point itself, which it looks at on every tickalso saves in place. Its second version is longer than the first, it waits only for the line of the second load, and it has no failure in the 400 builds.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts