Skip to content

fs.glob: do not skip sibling entries after a seen child path - #42870

Open
robobun wants to merge 1 commit into
mainfrom
robobun/46cd92b2/fs-glob-seen-cache-siblings
Open

robobun wants to merge 1 commit into
mainfrom
robobun/46cd92b2/fs-glob-seen-cache-siblings

Conversation

@robobun

@robobun robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Remove the check at both sites. That is the whole upstream change (nodejs/node@0ea2c86b5b70).
  • Safe because the check only ended the loop early. cache.add still stops a second traversal of the same path and pattern suffix, so the walk ends.
  • Verified: test/js/node/fs/glob.test.ts (9 new tests) and test/js/node/test/parallel/test-fs-glob.mjs (449 tests). The second file is now the v26.8.2 copy, with the upstream test for this bug. With src/ from main, 9 and 1 of them fail. Results match Node.js v26.8.2.
  • Self-reviewed: 5 concerns raised, 5 addressed.

Background

  • The walker keeps a queue of (path, pattern) items. It lists each path and matches every child.
  • The seen cache maps a path to the pattern suffixes that already started a traversal there. Two patterns that end in *.ts share a key.
  • In the array case the walker traverses the ./ pattern first, so the cache holds src/helpers with *.ts. Then src/*.ts lists src. The check fires on the child helpers, and the walker never reaches index.ts.
Notes

Repro 1. Fixture: src/index.ts, src/helpers/a.ts, src/helpers/b.ts. globSync(["src/*.ts", "./src/helpers/*.ts"], { cwd }):

Runtime Result
Bun 1.3.14 ./src/helpers/a.ts, ./src/helpers/b.ts, src/index.ts
Bun 1.4.3-canary, Node.js v26.3.0 src/helpers/a.ts, src/helpers/b.ts
This branch, Node.js v26.8.2 src/helpers/a.ts, src/helpers/b.ts, src/index.ts

The brace form {src/*.ts,./src/helpers/*.ts} behaves the same on 1.4.x. The reverse order of the array does not trigger the bug.

Repro 2. Fixture: a/b/c/d/, a/c/d/c/, a/x, a/z. globSync("a/**/../*", { cwd }) returns a/b, a/b/c, a/b/c/d, a/c/d and a/c/d/c on main and on Node.js v26.3.0. This branch and Node.js v26.8.2 return those plus a, a/c, a/x and a/z. Bun 1.3.14 returned [] for this pattern.

Real tree. Over Bun's src/ directory, the results of **/*.ts, js/**/../*, **/node/../*.rs and {js,runtime}/**/*.{ts,rs} are identical to Node.js v26.8.2. js/**/../* goes from 123 to 198 results.

Upstream test. The re-vendored test-fs-glob.mjs is byte-identical to Node.js v26.8.2. The new case (glob - seen cache) spawns a child with --expose-internals and requires internal/fs/glob, which Bun supports. It fails on main with the upstream assertion (missing a/c from sync results) and passes here. The same file passes on Node.js v26.8.2 and fails on v26.3.0.

Not changed here (tracked in #42876). The walker drops results in more cases. Node.js v26.8.2 and the v27 nightly of 2026-09-15 (native glob engine) return the same results as this branch, so this PR leaves them alone.

  • ["x/**/y/*", "./x/y/**/y/*", "x/y/*"] over x/y/g and x/y/z/y/f returns only x/y/z/y/f. main returns all three paths, because the early return stops the walker before Cache.add sees the merged pattern. This is the one known class of input where this branch returns less than main. It did not occur in 70,000 random pattern arrays. In 30,000 arrays built to share a tail, Node.js v26.8.2 lost a path against v26.3.0 in 4 and gained paths in 4,571. In 6,000 such arrays, this branch lost a path against main in 0 and gained paths in 936.
  • ["a/*/*", "a/**/c/*"] over the fixture of test-fs-glob.mjs does not return a/c/d/c/b (the same on main). Cache.add reports a repeat when any one index of the pattern is already in the cache, and it marks the other indexes although nothing walked them.
  • a/**/../{x,z} over a/b/, a/x, a/z returns only a/x (the same on main). The next === ".." branch queues a continuation with #subpatterns.set only if no other pattern already queued that path.

The readdir sort. The port sorts each readdir result, and its comment says that the bookkeeping is sensitive to entry order. This PR does not touch it, because the statement is still true. With every readdir result shuffled, 21 of 2661 random single patterns gave order-dependent results on Node.js v26.3.0 and 0 on v26.8.2. But on v26.8.2, !(symlink)/?/**/../../!(symlink) over the fixture of test-fs-glob.mjs returns a for only 44 of 80 orders.

Other suites. test-fs-glob-throw.mjs, test-path-glob.js and test/regression/issue/24007.test.ts pass.


no test proof · iteration 0 · 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

The children loop in the sync walker and in the async walker returned
from the whole method when a child path was already in the seen cache.
Every later sibling was dropped:

- ['src/*.ts', './src/helpers/*.ts'] did not return 'src/index.ts'.
  Bun 1.3 returned it.
- 'a/**/../*' did not return 'a', 'a/c', 'a/x' or 'a/z'.

Remove the check at both sites, as Node.js did in v26.8.0 and v24.21.0:
nodejs/node@0ea2c86

Update test/js/node/test/parallel/test-fs-glob.mjs to the v26.8.2 copy,
which adds the upstream test for this bug.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 5 days. After that, they cost $0.25 per reviewed file.

Or wait 3 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 08b136ce-2041-4e50-b091-c2fff82a2367

📥 Commits

Reviewing files that changed from the base of the PR and between a105560 and 59d6f4f.

📒 Files selected for processing (3)
  • src/js/internal/fs/glob.ts
  • test/js/node/fs/glob.test.ts
  • test/js/node/test/parallel/test-fs-glob.mjs

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

@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Status for #42870.

Reproduction, on bun 1.4.3-canary.1+09bb54630 and on main (Linux x64):

const fs = require("node:fs"), os = require("node:os"), path = require("node:path");
const cwd = fs.mkdtempSync(path.join(os.tmpdir(), "glob-"));
fs.mkdirSync(path.join(cwd, "src/helpers"), { recursive: true });
for (const f of ["src/index.ts", "src/helpers/a.ts", "src/helpers/b.ts"]) fs.writeFileSync(path.join(cwd, f), "");
console.log(fs.globSync(["src/*.ts", "./src/helpers/*.ts"], { cwd }).sort());
// bun 1.4.3-canary, Node.js v26.3.0: [ "src/helpers/a.ts", "src/helpers/b.ts" ]
// this branch, Node.js v26.8.2:      [ "src/helpers/a.ts", "src/helpers/b.ts", "src/index.ts" ]

USE_SYSTEM_BUN=1 bun test test/js/node/fs/glob.test.ts fails the 9 new tests. bun bd test test/js/node/fs/glob.test.ts passes all 43 tests with this branch. test/js/node/test/parallel/test-fs-glob.mjs (now the Node.js v26.8.2 copy) fails 1 of 449 tests on main and passes all 449 with this branch.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked that the deletion at both walker sites (src/js/internal/fs/glob.ts:640 sync, :856 async) is exactly the upstream nodejs/node@ 0ea2c86b change with no drift between the two copies, and that the re-vendored glob - seen cache case in test/js/node/test/parallel/test-fs-glob.mjs still fails on the base branch even though Bun's port re-sorts dirents (glob.ts:118, :276), which overrides the child's pinned ['c','b','x','z'] readdir order — so it exercises a different traversal order than Node, but not a vacuous one.

Extended reasoning...

Two findings were already confirmed inline (result loss with merged brace/array patterns once the early return is gone, and the .. continuation #subpatterns claim now firing for already-seen children). This note only records what else was examined: the sync and async walkers were diffed against each other and against the cited upstream commit and match; the vendored Node test's readdir monkeypatch is partially neutralized by Bun's sortDirents (glob.ts:118/276), but the PR's own claim that it fails on main under the sorted order is consistent with the trace, so it was not treated as a bug. The pre-existing Cache.add suffix-merging gap is upstream behavior and out of scope for this change.

One verified lower-impact observation (a convention, logging or cleanup point) was not posted.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/js/internal/fs/glob.ts — Callers passing a pattern array or brace group that contains **/.. can lose the second pattern's .. continuation and get fewer results than on the base branch. With the early return gone, the .. block at glob.ts:696-700 now runs for children the seen cache already holds, and it claims the #subpatterns slot for path or parent with a single-index child. A later pattern reaching the same block sees has() true and silently skips its own continuation instead of appending. Fix: make the .. block push onto the existing list the way #addSubpattern does at glob.ts:547-551 (both sync :696-700 and async :916-920), rather than skipping when the key is present.

    Extended reasoning...

    On the base branch the early return at the old glob.ts:640 aborted the child loop for a seen child, so that pattern never reached the .. block and never set #subpatterns[path]. Now it does. #subpatterns.set(path, [pattern.child({nextIndex+1})]) at :697 and :700 is guarded only by !this.#subpatterns.has(path). Unlike #addSubpattern (:547-551), which pushes when the key exists, this block discards the second pattern's continuation entirely. In a pattern array (or brace group, which is expanded to an array) where two patterns both contain **/.. at different positions, or the same pattern is queued with two indexes, the first pattern to run the block wins the slot; the second pattern's {nextIndex+1} child for path/parent is never queued. The dismissing finder only checked that #addSubpattern(entryPath, ...) cannot collide with path/parent; it did not consider that the newly reachable .. block itself is the writer that now fills the slot first. The PR author lists a/**/../{x,z} returning only a/x as a symptom of exactly this skip, and calls it pre-existing,…

    Verification: nit (verified regression vs base, but only on a narrow input class: a pattern array / brace group with two or more **/.. patterns that have different targets plus a pattern that pre-marks a child directory with the same **/.. suffix; upstream Node v26.8.2 has the same limitation after its identical fix). Triggering condition: a .. continuation of one array pattern is claimed via… | normal…

Comment thread src/js/internal/fs/glob.ts
@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:15 PM PT - Sep 15th, 2026

✅ @robobun, your commit 59d6f4fc80eee648f6fad89581056186fe45f305 passed in Build #116325! 🎉


🧪   To try this PR locally:

bunx bun-pr 42870

That installs a local version of the PR into your bun-42870 executable, so you can run:

bun-42870 --bun

@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

I ran both findings (Linux x64, results sorted).

Cache.add finding. The first repro is confirmed, and the trace in the comment is correct. ["x/**/y/*", "./x/y/**/y/*", "x/y/*"] over x/y/g and x/y/z/y/f:

Runtime Result
main (1.4.3-canary), Node.js v26.3.0 x/y/g, x/y/z, x/y/z/y/f
This branch, Node.js v26.8.2, Node.js v27 nightly (native glob engine) x/y/z/y/f

The second repro is not a regression. ["**/x/*", "./d/x/x/*", "d/x/*"] over d/x/x/f1 and d/x/f2 returns only d/x/x/f1 on main and on Node.js v26.3.0 too.

How often the first case occurs, with each result compared to the run without the fix:

Input Runs Gained paths Lost a path
70,000 random arrays of 2 or 3 patterns Node.js v26.3.0 and v26.8.2 279 0
30,000 arrays of 2 to 4 patterns that share a tail Node.js v26.3.0 and v26.8.2 4,571 4
6,000 arrays that share a tail main and this branch 936 0

I tried the suggested change: Cache.add returns the indexes that are new, and the walker continues with only those. It fixes both repros, but alone it is not safe. In the 6,000 arrays it lost a path in 14, for example ["./x/**/../*", "*/y/**/../*"] lost x. The narrowed pattern reaches the shortcut for a single literal segment with .., and setDirentName renames the Dirent that the readdir cache shares, so a later listing of . sees .. in place of x. With a second change (the shortcut does not rename the shared Dirent), it lost a path in 0, and the arrays with a wrong result went from 377 to 7.

Those are two deliberate divergences from Node.js, which returns the same results as this branch in v26.8.2 and in the nightly. So this PR stays the upstream change. #42876 has the repros, the three causes and the measurements, and the PR description now names this class of input. I can open the follow-up PR if a maintainer wants the divergence.

.. continuation finding. I could not reproduce a loss against main. a/**/../{x,z}, ["a/**/../x", "a/**/../z"], and three-pattern variants with a ./a/b/**/../x pattern that marks a/b first, return the same paths on main, on this branch and on Node.js v26.8.2. In 3,000 random arrays with .. segments and in the 6,000 arrays above (their tails include ../* and **/../*), this branch lost a path against main in 0. The dropped continuation itself is real on every one of these runtimes. It is repro 3 in #42876.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant