Repository navigation
fix(fs): walk directories in path order so prefetch matches consumers - #2821
Conversation
The default walk followed read_dir order (hash order for InMemoryFs), so a batch cut short by max_entries held listings tree/ls -R/find would not reach next. tree_prefetch_stays_within_entry_budget read 8-15 directories run to run and failed main's coverage job; it now reads 6 every time.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | 5692dc1 | Commit Preview URL Branch Preview URL |
Nov 03 2026, 08:14 AM |
![bashkit-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/249470897?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correct fix for the flaky prefetch: deterministic DFS in consumer order, with consumers verified (tree sorts + recurses ascending, find sorts + pushes reversed for ascending pop, ls -R iterates subdirs in listed order). No sandbox/security impact (sort only, no new I/O). One residual nit below.
| pending.push((super::vfs_join(&path, &entry.name), child_depth, None)); | ||
| } | ||
| // Visit children in name order (pushed reversed, popped first), | ||
| // the order tree/ls -R/find consume them: a batch cut short by |
There was a problem hiding this comment.
[P2] ls -R -t still consumes in mtime order while this prefetches in name order, so the "holds the next listings the caller needs" claim doesn't hold for that flag. Evidence: crates/bashkit/src/builtins/ls/list.rs:361-366 sorts sorted_entries by modified when sort_by_time, but list.rs:297 builds WalkCache::new(opts.recursive) unconditionally, and subdirs follows the time-sorted order. Correctness is unaffected (cache misses fall back to read_dir), just extra round trips on large ls -R -t runs. Consider scoping prefetch to the name-order case (e.g. WalkCache::new(opts.recursive && !opts.sort_by_time)) or softening this comment to "the order tree/ls -R (name order)/find consume them".
Review summary (PR #2821,
|
What changed
The default
FileSystem::walknow visits a directory's children in name order. Before, it usedread_dirorder, which is hash order forInMemoryFs.Why
tree,ls -Randfindconsume listings in path order. When a walk batch was cut short bymax_entries(for example,treecapping it at its remaining entry budget), it held arbitrary listings instead of the next ones the caller needed, so it read directories that were never printed. Because the order changed from run to run,tree_prefetch_stays_within_entry_budgetwas flaky, and it failed the coverage job on main at 6d66d43.Before / After
tree treewithmax_commands(50)on the round-trip fixture:read_dir= 8, 12, 13, 15 across runs (CI coverage failed at 15).read_dir= 6 on every run. The test now asserts<= 6.The new unit test
default_walk_visits_directories_in_path_orderfailed before the fix and passes after it.Risk
Checklist
Generated by Claude Code
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.