Repository navigation
Conversation
…table The scanner already sorted entries for the top-level directory when the resolver had it cached, but subdirectories visited from the BFS queue were processed in raw getdents/hash-map order. That order depends on the filesystem (ext4 htree hash vs APFS vs tmpfs insertion order), so the same project could run its test files in a different order on different machines. Because bun test runs every file in one process with a shared module registry, a different file order means different cross-file module side effects, which turned issue #6655 into a pass-on-one-machine / hang-on-another situation. Fix by always collecting a directory's entries, sorting by lowercased basename, and visiting in that order (both for the scan root and every queued subdirectory). The inline readdir iterator path and has_iterated bookkeeping are no longer needed.
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit 2454c7a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36636That installs a local version of the PR into your bun-36636 --bun |
WalkthroughChangesThe scanner now performs deterministic, case-insensitive directory traversal across platforms. A new end-to-end test verifies breadth-first execution order and successful completion. Deterministic test scanning
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/runtime/cli/test/Scanner.rs`:
- Around line 235-241: Update the comparator in the entry_ptrs sort_by call to
use the original basename as a tie-breaker whenever base_lowercase() values
compare equal. Preserve lowercase ordering first, then compare each Entry’s
original basename so entries differing only by case sort deterministically.
- Around line 235-245: Update process_entries_sorted to avoid infallible
allocation when collecting entry pointers: reserve the required Vec capacity
with a fallible allocation API, return AllocError on failure, and preserve
sorting and processing behavior on success. Change all three
process_entries_sorted call sites to propagate the allocation error as
ScanError::OutOfMemory.
In `@test/cli/test/bun-test.test.ts`:
- Around line 1702-1710: In the test named “runs test files in stable sorted
order regardless of filesystem readdir order,” remove the narrative regression
comments and the explanatory name-order comment, keeping only the exact issue
URL comment for issue 6655; apply the same cleanup to the additional referenced
lines.
🪄 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: 0854b47e-7241-4204-9c96-7e5094cf78f0
📒 Files selected for processing (2)
src/runtime/cli/test/Scanner.rstest/cli/test/bun-test.test.ts
…er hooks readdir The test scanner was the only non-() implementer of DirEntryIterator. With process_entries_sorted doing a collect-then-sort after read_directory returns, nothing observes entries during readdir anymore, so the trait, the &T blanket impl, the !IS_VOID branch in add_entry_with_store, and the generic I parameter threaded through readdir/read_directory_with_iterator are all dead. Fold read_directory_with_iterator into read_directory and drop the iterator arg at every call site.
There was a problem hiding this comment.
LGTM — the dead DirEntryIterator plumbing I flagged earlier is now fully removed in 2454c7a, and the rest is unchanged.
What was reviewed:
- Traced the BFS in
scan()against the new test's expected order (root→pkg→lib,src; lib pushes inner; src files emit before inner's) — matches. - Confirmed no remaining references to
DirEntryIterator/read_directory_with_iterator/has_iterated; theIS_VOIDhits inPackage.rsare unrelated. - Checked that
process_entries_sortedis the only path callingnext()now, so no double-processing on cached vs. fresh directories. - Return-type widening on
read_directory(&mut→&'static mut) is compatible with all existing call sites.
Extended reasoning...
Overview
This PR makes bun test file discovery order deterministic by sorting every scanned directory's entries by lowercased basename (with a case-sensitive tie-break). Previously only the root directory was sorted (and only when the resolver had it cached); subdirectories were processed in raw getdents/hash-map iteration order, which is filesystem-dependent.
Five files touched: Scanner.rs extracts the existing root-level sort into process_entries_sorted() and calls it for every directory in the BFS loop (both POSIX and Windows arms); fs.rs/lib.rs/resolver.rs drop the now-dead DirEntryIterator trait, its ()/&T impls, the !IS_VOID branch, the I generic on readdir/add_entry_with_store, and fold read_directory_with_iterator into read_directory; bun-test.test.ts adds a 17-file nested-directory test asserting exact BFS-plus-sorted execution order.
Security risks
None. This is file-discovery ordering in the test runner; no untrusted input parsing, no auth/crypto, no network.
Level of scrutiny
Moderate. The core change is a straightforward bugfix — the root-level sort already existed and expressed the intent; this extends it to subdirectories, which is what #26851's regression test already assumed. The resolver API deletions are mechanical dead-code cleanup that I requested in my previous review and are confined to plumbing whose only non-trivial consumer was the scanner itself. The one signature change (read_directory now returns &'static mut instead of an elided-to-&mut self lifetime) is a widening; every existing caller (grep confirms 6) is compatible.
Other factors
- My prior review's only finding (dead
DirEntryIteratorplumbing) was fully addressed in 2454c7a; grep confirms zero remaining references. - All three CodeRabbit findings were addressed or withdrawn (tie-break added, comment trimmed, OOM propagation declined with a sound rationale).
- The new test uses
tempDir/bunEnv, drains all pipes concurrently, asserts stdout before exitCode, and has a load-bearing filename set documented with the htree-hash rationale. I hand-traced the BFS queue against the expected array and it matches exactly. - The removed
has_iteratedflag existed only to skip the manual root-entries pass when the readdir iterator had already fired; with the iterator gone,process_entries_sortedis now the sole caller ofnext()on every path, so there's no double-visit risk. process_entries_sortedcollects raw*mut Entry(EntryStore-backed, process-lifetime) before iterating, soself.next()'s call intoentry.kind(real_fs, ...)cannot invalidate the pointers being walked.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
should the module resolver itself do the sorting? maybe we need a bool that's like "is_unsorted" and then a function to get the sorted dir entries list and then if is_unsorted is true we sort it first?
|
I kept the sort in the scanner because the scanner is (currently) the only thing that iterates a
If the goal is to keep the "hash-map iteration order is unstable" detail out of the scanner, I'd lean toward a non-cached helper on impl DirEntry {
pub fn sorted_entries(&self) -> Vec<*mut Entry> {
let mut v: Vec<*mut Entry> = self.data.values().copied().collect();
v.sort_by(|a, b| unsafe {
(**a).base_lowercase().cmp((**b).base_lowercase())
.then_with(|| (**a).base().cmp((**b).base()))
});
v
}
}Then |
|
CI on 2454c7a is green for this diff: the new scanner-order test passed on every lane, and the only hard-red job ( Holding for the |
What
bun testdiscovers test files with a BFS directory walk. The root directory's entries were already being sorted (when the resolver had it cached), but every subdirectory visited from the BFS queue was processed in rawgetdents/ hash-map iteration order. That order is filesystem-dependent (ext4 htree hash, APFS, tmpfs insertion order, NTFS), so the same project ran its test files in a different order on different machines.This change sorts every directory's entries by lowercased basename before visiting them, so discovery order is the same everywhere. The inline-readdir iterator callback and the
has_iteratedbookkeeping are no longer needed and are removed.Why this matters for #6655
The reproduction in #6655 has two test files in
packages/server-winston-mongodb/src/:Log.entity.test.tscallsgetModelForClass(LogEntity)at module-load time, which caches a typegoose model bound to mongoose's default (never-connected) connectionMongoDBTransport.test.tslater callsgetModelForClass(LogEntity, { existingConnection })and, because of typegoose's model cache, gets the already-cached model on the wrong connection, somodel.create(...)buffers forever and the test times outNode reproduces the same hang when those two modules are evaluated in that order in one process:
Because
bun testruns every file in one process with a shared module registry, whether the hang shows up depends entirely on which file runs first. On filesystems wheregetdentsreturnedLog.entity.test.tsbeforeMongoDBTransport.test.ts(APFS, some Docker layouts), the test hung; on ext4 here,MongoDBTransport.test.tshappened to come first and it passed. Copying the package to a fresh "standalone" directory changes the directory-entry order again, which is why workspace vs standalone looked like the trigger.With this fix the discovery order is stable, so that project now fails the same way everywhere instead of appearing to be a workspace/module-resolution bug. The underlying hang is typegoose's cache, not Bun; the fix on that project's side is either
getModelForClass(LogEntity, { options: { disableCaching: true } })or callingdeleteModel('LogEntity')between files.Verification
New test in
test/cli/test/bun-test.test.tscreates 17 files across nested subdirectories whose ext4 htree-hash order is non-alphabetical and asserts the exact BFS-plus-sorted execution order. Fails on main, passes with this change.Related: #6655, #26851
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/test/bun-test.test.ts
Deletions
The scanner was the only non-
()implementer ofDirEntryIterator. With the collect-then-sort model nothing hooksreaddirper-entry anymore, so the following are removed as dead:DirEntryIteratortrait and its()/&Timpls (src/resolver/fs.rs)!I::IS_VOIDbranch inadd_entry_with_storeI: DirEntryIteratorgeneric anditeratorargument onreaddir/add_entry_with_storeread_directory_with_iterator(folded intoread_directory)ScannerDirIterand thehas_iteratedflag (src/runtime/cli/test/Scanner.rs)