Repository navigation
Conversation
…tory once The scanner skipped a directory it could not open or read without a word, so a suite under an unreadable directory was silently absent and the run exited 0. Each such directory is now reported with its errno, a summary line names the count, and the run exits 1. The scanner also followed symlinks back into the tree until the path buffer overflowed, which ran the same files many times. It now records (st_dev, st_ino) of every directory it opens and skips one it has seen.
|
Warning Review limit reached
On-demand reviews are free for the next 15 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 12:13 AM PT - Sep 6th, 2026
✅ @robobun, your commit 3ce764607a1987f2be8779d20835f970d9b39e78 passed in 🧪 To try this PR locally: bunx bun-pr 41465That installs a local version of the PR into your bun-41465 --bun |
…hout an inode number
There was a problem hiding this comment.
I re-reviewed after the latest push — all five of my earlier inline findings are addressed (from_code_int for the Windows errno type, st_ino == 0 falls through without dedup, ENOENT is skipped in report_unreadable_dir, non-Sys resolver errors are now reported, and the /etc/passwd read is guarded). No new issues found. A maintainer should still weigh in on the exit-code policy the PR flags (fail vs. warn on unreadable dirs), and note that both new tests skip on Windows so the (st_dev, st_ino) dedup and error-reporting paths there rely on rust:check-all only.
What was reviewed
report_unreadable_dir/report_dir_read_error: ENOENT early-return, non-Sysfallthrough,from_code_intcross-platform (WindowsSystemErrnois#[repr(u16)],E::ENOENTalias exists at windows_errno.rs:258).mark_visited:st_ino != 0guard; root seed viafstatatinserting a single(dev, 0)on FAT is harmless since children go through the guarded path.test_command.rs:unreadable_dirsis captured beforedrop(scanner)and gates exit code alongside existing failure conditions.- Tests:
hasNobodyUser()try/catch,finallyrestores chmod, output asserted before exit code.
Extended reasoning...
Overview
The PR adds two behaviors to the bun test file-discovery scanner: (1) (st_dev, st_ino) tracking to avoid re-scanning directories reached via symlink loops, and (2) reporting + nonzero exit when a directory cannot be opened or read. Changes are in src/runtime/cli/test/Scanner.rs (~60 lines added), a small hook in src/runtime/cli/test_command.rs to surface the count and set exit code 1, and two new tests appended to the existing scanner block in test/cli/test/bun-test.test.ts.
Security risks
None identified. This is local filesystem traversal for test discovery; no network, auth, or untrusted-input parsing is involved. The added fstat/fstatat calls use existing bun_sys wrappers and their errors degrade to "scan anyway".
Level of scrutiny
Moderate. My prior review raised five findings (three blocking, two optional); commits 3fc9c3b and 59cea25 address all of them, verified against the current diff and the underlying types (bun_resolver::Error is Copy, Windows SystemErrno is #[repr(u16)] so the as c_int cast is valid, and E::ENOENT is an associated const on Windows so get_errno() == E::ENOENT holds cross-platform). I did not find new issues on re-read.
Other factors
Two things keep this from an approve: the PR author explicitly leaves the exit-code policy (fail vs. warn on unreadable directories, where Node/Jest/vitest exit 0) for a maintainer to decide, and both new tests are gated off Windows — so the Windows-specific concerns I raised earlier (dangling junctions, FAT/exFAT st_ino == 0) are addressed in code but have no test coverage on that platform. No CODEOWNERS entry matches these paths.
|
Status: draft. The symlink half of this PR has a regression. Do not merge head 3ce7646. Measured on this head merged with main 95690fc (debug build, Linux x64), against stock 1.4.3-canary.1+367d939d9. The head does end the walk that never finishes:
The regression: the visited set keeps only the first path that reaches a directory. A path filter that names another path to the same directory matches nothing. mkdir -p r/packages/v2 && cd r
echo 'import {test,expect} from "bun:test"; test("s",()=>expect(1).toBe(1));' > packages/v2/t.test.js
ln -s packages/v2 latest
bun test packages/v2/
The scanner is breadth-first. It opens The suites I ran do not cover this. The unreadable-directory half is not affected. Next: rework the symlink handling so that a filter matches every path it matches on main, with these trees as tests. |
Problem
bun testskips a directory it cannot open, prints nothing, and exits 0. The cause islet Ok(child_fd) = opened else { continue }insrc/runtime/cli/test/Scanner.rs:207.Ran 2 tests across 902 files.Two links to the root never finish.Fix
error: could not scan "<path>" for testsplus the errno line. The run exits 1.(st_dev, st_ino). A directory seen before is skipped.test/cli/test/bun-test.test.ts(two new cases, stock 1.4.3 fails both). The two-link trees printRan 1 test across 1 file.Background
bun test packages/v2/) is a substring match on the path the scanner recorded.runuser.Downsides
latest -> packages/v2,bun test packages/v2/exits 1 withThe following filters did not match any test files. Main runs the test.fstatper scanned directory, counted from the code. Not measured.Notes
Measured on head 3ce7646 merged with main 95690fc (debug build, Linux x64), against stock 1.4.3-canary.1+367d939d9:
examples/one/lib -> ../..andexamples/two/lib -> ../..Ran 1 test across 1 file.(3 of 3 runs)a/loop -> ..andalias -> aRan 1 test across 1 file.(3 of 3 runs)packages/a/vendor/b <-> packages/b/vendor/aRan 2 tests across 82 files.Ran 2 tests across 2 files.(3 of 3 runs)packages/v2/andlatest -> packages/v2,bun test packages/v2/v2/andlinks/latest -> ../v2,bun test links/latest/The last three rows are the regression. The scanner is breadth-first, so a link nearer the root is opened before the real directory and the real directory is skipped. The suites below do not cover it: none uses a path filter on a tree with a directory symlink.
Other designs were not weighed before this was written, for example a cycle check against the ancestors of the current directory (
src/glob/GlobWalker.rskeepsfollowed_linksfor that). The rework starts there.Probe on bun 1.4.3, as a non-root user, with
ok/a.test.tsreadable andlocked/b.test.tsunder achmod 000directory:Symlink loop probe on 1.4.3:
sub/a.test.ts,sub/deeper/b.test.ts,sub/loop -> ..,sub/deeper/self -> .givesRan 2 tests across 902 files.The file count and the wall time grow with the path limit.A directory that is gone when it is opened (ENOENT, a dangling link) is skipped without a report.
Root errors other than ENOENT and ENOTDIR were already logged under
BUN_DEBUG_jest. They are now also reported to the user and counted.On Windows
fstaton a directory handle fillsst_devfrom the volume serial number andst_inofrom the NTFS file index (read fromsrc/sys/lib.rs, not run on Windows). A filesystem that reportsst_ino == 0gets no deduplication. Iffstatfails the directory is scanned as before.Self-review: 5 concerns raised, 4 addressed (Windows compile of the errno conversion,
st_ino == 0, ENOENT on a dangling junction, non-errno resolver errors). The fifth is the exit-code policy. The self-review saysnode --test, Jest and vitest skip an unreadable directory and exit 0 (not rerun by me), and it recommends a split: the symlink handling and the error report in one change, the exit code in its own change. It did not find the filter regression above.Suites run with the debug build on the head merged with main:
test/cli/test/bun-test.test.ts,pass-with-no-tests.test.ts,test-shard.test.ts,path-ignore-patterns.test.ts,test-changed.test.ts(164 pass, 6 todo, 0 fail). Commit 3ce7646 was pushed before a local run. CI passed on it afterwards.no test proof · iteration 1 · 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