Repository navigation
Conversation
The walk memoizes Pattern.cacheKey() (every child() of a pattern shares the keys), joins the directory prefix once per directory and not twice per entry, and does not call path.resolve() per result or per queued path when there is no exclude array. The results and their order do not change.
fs.promises.glob and fs.glob wait one thread pool round trip per directory, one after the other. The walk now starts the readdir of the next 8 queued directories before it visits them, and reads the root while it stats it. The walk awaits the same promises from the readdir cache, so the walk order and the results do not change. A rejection of a readdir that nothing awaits is marked handled. The consumer still gets it from the walk's own readdir.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe glob implementation now uses prefixed directory entries, shared cache keys, guarded exclusions, and asynchronous directory prefetching. New tests cover path operations, ordering, errors, concurrency limits, and early iterator termination. ChangesGlob traversal optimization
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No merge-blocking behavior issue is confirmed in the optimized glob traversal. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:29 PM PT - Sep 16th, 2026
❌ @robobun, your commit 5f3557b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42963That installs a local version of the PR into your bun-42963 --bun |
|
Status How I reproduced it: Where the time went: CI: every job passes except one. PR: #42963 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked: joinPrefix(dir) + name against join(dir, name) for every path/fullpath shape the walk produces (., .., ../, /, C:\, normalized relative paths — the .. literal-segment branch still goes through join), the rejection path of a prefetched readdir (the cached inner promise is adopted by the async readdir() wrapper, so prefetchReaddir's $then leaves nothing unhandled and the walk's own readdir(path) still rejects), and the #addSubpattern/ResultSet.add skips (only taken when #isExcluded is the default, and the exclude-callback branch recomputes the same resolve(root, path)). None of these change results or order; the prefetch only ever adds cache entries earlier.
Extended reasoning...
Findings-present run: one inline nit on the maxInFlight <= 16 bound survived verification. This note records what else was traced and ruled out from reading src/js/internal/fs/glob.ts against the diff: the joinPrefix substitution (readdir names contain no ./../separator and path is always normalized because parent = join(path, "..") and entry paths are built from normalized prefixes), the unhandled-rejection question for prefetched readdirs (the async readdir() wrapper adopts the cached inner promise, so both the inner and the outer promise have handlers; the walk's later readdir(path) produces a fresh outer promise that rejects to the consumer, which the new sort-failure tests exercise for both the root and a directory inside the prefetch window — d3 is among the last 8 queue items on the sorted fixture), the memoized cacheKey (depends only on the shared glob strings and a non-negative index; "" past the end is ??=-stable), and the exclude fast-path (ResultSet.#isExcluded is only replaced via setup() for array excludes, and the callback branch now resolves the path itself). Prefetch reads directories the walk may later skip as excluded or cyclic, but that is an extra readdir mapped to [] on error, not a behavior change. No debug build was available in this checkout, so the tests were not executed here.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
There was a problem hiding this comment.
The latest push tightens the read-ahead test to the exact window, and this run found no bugs in the walker changes; since this is an async read-ahead in the node:fs glob walk with a correctness argument that lives in the PR text rather than in the code, a human look at that argument would still be worthwhile.
What was reviewed: the joinPrefix equivalence for every dir shape the walk produces (., relative, ..-prefixed, POSIX and drive/UNC roots), the shared #cacheKeys array across child() clones (keyed only on glob strings and index), the excludeNothing short-circuits including the relocated resolve() in the exclude-callback branch, and the #prefetch() window: it only inserts the same readdir promise into the cache earlier and skips entries whose parent Dirent is not a directory or whose patterns take the single-literal stat path. Also checked that prefetchReaddir attaches a rejection handler while the later awaited readdir(path) still surfaces the rejection, which the two new subprocess tests pin. One candidate was ruled out: the test comment explaining why maxInFlight is exactly 8 misdescribes the fixture (the root has 20 subdirectories), but the asserted value is correct because a popped item's readdir settles before its child is pushed and prefetched.
Extended reasoning...
Overview
The diff touches two files: src/js/internal/fs/glob.ts (the built-in node:fs glob walker ported from Node, ~110 lines changed) and test/js/node/fs/glob.test.ts (~147 lines added). The runtime change has four parts: memoized Pattern.cacheKey shared across child() clones, a joinPrefix(dir) + name replacement for per-entry path.join in both the sync and async walk loops, excludeNothing short-circuits in ResultSet.add and #addSubpattern, and a bounded (kPrefetchWindow = 8) read-ahead of readdir in the async glob() generator via Cache.prefetchReaddir and Cache.cachedDirent. The commit after the prior review (aaf77e04) converted the tests to describe.each and changed the in-flight assertion from <= 16 to exactly 8, which addresses the earlier inline nit.
Security risks
None specific to this change. The code handles filesystem paths from readdir results and user-supplied cwd/patterns, but the edits do not introduce new path parsing or trust boundaries: joinPrefix is only applied to names returned by readdir (never ./../separator-containing), and the literal-segment branch, where user-supplied .. can appear, still goes through join. The exclude callback path still resolves against #root before calling statSync. The prefetch only starts readdir on paths the walk would already read.
Level of scrutiny
Medium. The per-entry string work is mechanical and the equivalence argument for joinPrefix holds for the shapes the walk produces (join(dir, sep) normalizes to a trailing separator on both path.posix and path.win32, and drive/UNC roots stay intact). The read-ahead is the part that deserves a human eye: it relies on Cache.readdir being idempotent per path and on the walker awaiting the same cached promise, so result ordering cannot change. I traced readsDirectory against the firstPattern fast path in #iterateSubpatterns and they agree, and cachedDirent correctly returns undefined for pending stat promises and for null (stat failure) so ?.isDirectory() skips those. The rejection-handler design ($then(undefined, nullOnReject) on the prefetch while the walk awaits its own readdir(path)) is exercised by the two "fails to sort" subprocess tests, which would produce an unhandled-rejection exit without the handler.
Other factors
The new tests cover the async-equals-sync property across seven pattern shapes (including a literal-segment pattern and a missing-directory pattern), withFileTypes, early consumer break, and the exact read-ahead window. The vendored upstream test-fs-glob.mjs suite is unchanged and the PR claims it passes; that cannot be confirmed from this checkout without a build. The one ruled-out candidate is a misleading test comment about the fixture shape, which does not affect the assertion's correctness. No CODEOWNERS entry covers these files. Because this is a non-trivial change to a hot compat path with an order-sensitivity argument that a human should confirm they agree with, deferring rather than approving is the appropriate outcome.
This branch has not been deployed
2 participants
Problem
node:fsglob is a port of Node's JavaScript walker.fs.promises.glob("**/*.rs")oversrc/of this repo (1,525 results) takes 56 ms per call, andfs.globSynctakes 17 ms. Bun 1.3.14 took 9 ms and 7 ms.fs.promises.readdirper directory, in sequence. Each is a round trip to the thread pool, and the pool is idle while the JS thread matches entries.Pattern.cacheKey) and callspath.jointwice. Per result it callspath.resolve.Fix
Glob.#prefetch()starts thereaddirof the next 8 queued directories, and the rootreaddirstarts with the rootlstat. The walk awaits the same promises from the readdir cache, so the walk order and the results do not change.joinPrefix), and skipsresolve()without anexcludearray. That is the first commit. The read-ahead is the second.fs.promises.glob56 ms to 31 ms,fs.globSync17.1 ms to 15.2 ms.test/js/node/fs/glob.test.ts(new: async equals sync on a wide tree, the read-ahead is on and bounded, nojoinorresolveper entry). Alsotest-fs-glob.mjs(448 pass). Self-reviewed together with node:fs glob: compile plain patterns without minimatch #42874.Background
readdirand matches every child. ItsCachekeeps onereaddirpromise per directory.fs.promises.readdirruns on the thread pool.Notes
Numbers. Release builds of the same commit with and without this change, Linux x64,
bench/glob/node-fs-glob.mjsfrom #42874, median over fresh processes (7 for the tree, 21 for the directory), ms. The box was under load (load average 55), so compare rows, not absolute values.first,second: that call in the process.later: median of 100 more calls.**/*.rsoversrc/of this repo (1,525 results):"*.txt"in a directory of 20 files:The first call does not change here. That is the subject of #42874.
Why the read-ahead is safe. It only puts the
readdirpromise in the cache earlier. The matching code is untouched, so the walk order and the results are the same. It reads a queued directory only when the parent'sreaddirsaid it is a directory and a pattern for it lists it (readsDirectory). Each visited directory looks at the last 8 queue items, so a visit starts at most 8 reads, and only for items that no earlier visit started. On the test tree (61 directories, at most one subdirectory each) exactly 8 reads are in flight at the peak, and the test expects 8. A consumer that stops early leaves at most those reads unused.Cache.readdirmaps areaddirerror to[]. The promise can still reject whenfs.promises.readdiris patched and its result does not sort, soCache.prefetchReaddir()attaches a rejection handler to the promise that nothing awaits. The walk gets the same rejection from its ownreaddir(path), as before.Why
joinPrefixis equivalent.readdirnever returns.,..or a name with a separator, sojoin(dir, name)has nothing to normalize inname.join(dir, sep) + namegives the same string for every shapedirtakes here (.,a,../,..,/,C:\, a UNC root), checked againstpath.posixandpath.win32. The literal-segment branch (children = [stat], where the name can be..) still callsjoin.Why the shared cache keys are equivalent.
cacheKey(index)depends only on the glob strings and the index, and everychild()of a pattern has the same glob strings.Tests. Two of the new tests fail without the change: the in-flight
readdircount is 1 onmain, andmaincallspath.joinandpath.resolveat least once per entry for 200 files. The other tests pin behavior that must not change. Beyond the suite, a differential run of the old and the new module on 16,500 random cases (random trees with dotfiles and symlinks,excludefunction and array,withFileTypes,followSymlinks, othercwd, sync and async) gave the same results in the same order.#42870 (a results fix from upstream Node) removes three lines in each of the two walk loops, about ten lines below the
joinPrefixhunks. The changes do not overlap.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/glob.test.ts