Repository navigation
fix(fs): cut round trips for rg, find and tree walkers on remote backends - #2816
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | 022e2ba | Commit Preview URL Branch Preview URL |
Nov 03 2026, 05:33 AM |
cdcdb1c to
3220240
Compare
|
Generated by Claude Code |
…ends - Indexed search reaches builtins: MountableFs forwards SearchCapable (with mount path mapping), the per-execution VFS hands out a lease-checked provider (TM-ISO-027). - rg keeps the index with ignore rules on, loading rules only along directories that lead to a match; the walk reads ignore files the listing shows instead of probing four names per directory. - find, du, grep -r, tar, cp -r use read_dir metadata instead of a per-entry stat; grep -r resolves only root and symlinked dirs; an empty index answer is final. - FileSystem::walk(root, max_depth) bulk listing hook with a read_dir default, forwarded through wrappers; used by find and du. - FollowFs copy/chmod/set_modified_time fast path. - CountingFs test/bench backend, round-trip budget tests, remote_fs bench.
…once
FileSystem::walk now takes WalkOptions { max_depth, max_entries } and
returns a bounded batch (10k entries default, never a split listing).
find and du refill batches on demand through WalkCache and drop listings
as they descend. rg charges each file to the aggregate-input budget as it
reads and stops at the first refusal instead of loading every file first.
TM-DOS-139.
3220240 to
d9c52b6
Compare
![bashkit-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/249470897?s=60&v=4){kind=small}
| self.inner.search(query) | ||
| } | ||
|
|
||
| fn capabilities(&self) -> crate::SearchCapabilities { |
There was a problem hiding this comment.
[P2] capabilities() bypasses the execution lease while search() enforces it, so a provider retained past completion still answers metadata queries. Either check the lease here too or document that capabilities are intentionally lease-free. Evidence: crates/bashkit/src/execution_capability.rs:511-521 (LeasedSearchProvider::search calls ensure_active, capabilities just forwards to inner). Fix: map a revoked scope to default/empty capabilities or add a comment stating capabilities carry no authority.
There was a problem hiding this comment.
Fixed in 022e2ba: LeasedSearchProvider::capabilities returns default (no content search) once the lease is revoked. retained_search_provider_fails_after_completion now asserts it.
Generated by Claude Code
| } | ||
|
|
||
| impl super::SearchProvider for MountedSearchProvider { | ||
| fn search(&self, query: &super::SearchQuery) -> Result<super::SearchResults> { |
There was a problem hiding this comment.
[P2] MountedSearchProvider::search does not re-check has_mount_below at query time, so a mount added between search_provider() and search() (parallel executions share the MountableFs) yields a partial-index answer that callers treat as definitive. Evidence: guard exists only in search_provider (crates/bashkit/src/fs/mountable.rs:543), while search at :508 translates the root and delegates unconditionally; compare walk at :636 which re-checks per call. Fix: re-validate inside search (return empty/default to force walk fallback) or document that mounts are immutable during execution.
There was a problem hiding this comment.
Fixed in 022e2ba. MountableFs keeps a mount generation that is bumped on every mount and unmount. A provider records the generation it was handed out at, and search fails if it changed, so callers fall back to the walk. Test: search_provider_refuses_after_mount_table_changes.
Generated by Claude Code
| if inputs.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| // An index that answered with no (in-scope) match is still an answer: |
There was a problem hiding this comment.
[P2] An in-scope but empty/stale index answer is treated as definitive (crates/bashkit/src/builtins/grep.rs:1624 "still an answer"), yet I found no documented completeness/consistency contract on SearchProvider/SearchCapable requiring that. A remote backend with an eventually-consistent or partial index will then silently miss matches where the old walk found them. Fix: document in fs/traits.rs (and docs/filesystem.md) that providers must return complete results for the queried root or return Err/empty-provider so callers fall back to walking.
There was a problem hiding this comment.
Documented in 022e2ba. SearchCapable::search_provider (fs/search.rs), docs/filesystem.md and knowledge/foundations/vfs.md now say that answers must be complete for the queried root. A backend whose index may be stale or partial returns no provider, or an error from search, and then rg/grep -r walk.
Generated by Claude Code
| @@ -0,0 +1,120 @@ | |||
| # remote_fs bench, bashkit checkout (598ebb4 + this change) | |||
There was a problem hiding this comment.
[P2] Generated Criterion reports are committed (crates/bashkit/benches/results/criterion-remote_fs-*.md, e.g. the table with per-command round-trip counts). These are machine- and run-specific artifacts that will churn on every bench run. Fix: gitignore benches/results/ (or criterion/) and keep at most a hand-written summary in the PR/docs.
There was a problem hiding this comment.
Keeping these: the repo contract (AGENTS.md, Benches) requires saving criterion results under crates/bashkit/benches/results/ as criterion-<bench>-<moniker>-<timestamp>.md, so future runs have a baseline to diff against. Each run adds a new timestamped file, and existing files are never rewritten.
Generated by Claude Code
Review summary (head
|
Requested by Михайло · project thread
What changed
Tree-walking builtins now make far fewer
FileSystemcalls, which is what decides speed when each call is a network round trip (Everruns: ~10 ms).rg/grep. In a defaultBashsession the index was never used:MountableFsand the per-execution scoped VFS both returnedNonefromas_search_capable.MountableFsnow forwards the provider of the backend serving the whole subtree (none if another mount sits below it) and maps result paths back under the mount point. The scoped VFS hands out a lease-checked provider: everysearchfails once the execution is revoked (TM-ISO-027, new regression test).rgkeeps the index with ignore rules on (ask 1). Index matches are filtered through the walk's own rules, loaded only along the directories that lead to a match; ignored/hidden directories are never entered.--max-depthnow applies to index matches. A missing operand falls back to the walk so the error matches.rgwalk lists a directory first and opens only the ignore files the listing shows (it used tostat+readfour names per directory andstat.gitin every ancestor of every directory).findreusesDirEntry.metadata(ask 2):lstatonly for roots; symlinks still getread_link+ targetstat. The listing read for%n/-lsis reused when descending.FileSystem::walk(root, max_depth) -> Vec<DirListing>(ask 3), default = oneread_dirper directory (walk_with_read_dir), forwarded byFollowFs,MountableFs(no nested mount),RootFs(outside system trees),ReadOnlyFsand the per-execution wrappers. Used byfind(when no-exec*/-delete/-prune/-quit) anddu. Returns listings per directory rather than flat(path, metadata)pairs so callers know which listings are complete, including empty dirs.grep -rtakes sizes from listings and resolves only root/symlinked dirs (was anlstatper path component per directory); an index answer with zero matches is final (it used to re-walk the whole tree).du,taruse listing metadata.FollowFscopy/chmod/set_modified_timetake a one-statfast path instead of resolving every component, which dropscp -rfrom ~12 to ~2.4 lookups per file.MountableFs::read_dirlists mount points with the mounted root's real metadata.bashkit::testing::CountingFs(doc-hidden): call-counting backend with optional index, bulk walk and busy-wait latency, for tests and benches.Memory bounds (trees with millions of files)
FileSystem::walktakesWalkOptions { max_depth, max_entries }and returns a bounded batch (10k entries by default, never a split listing).findandduask for the next batch as they descend and drop listings once used, so they hold about one batch whatever the tree size. If an answer is missing the requested directory, the walk falls back toread_dirinstead of repeating walks.rgcharges each file's text to the aggregate-input budget as it reads it and stops at the first refusal. Before, it read every file and only then checked the budget (TM-DOS-140).walk_batches_are_bounded_*(100-entry backend page: same output, peak ≤ page + one listing),rg_stops_reading_at_input_budget(stops after fewer than 200 of 1,000 files), andwalk_cacheunit tests.Why
Everruns runs Bashkit over a remote filesystem;
rg -l markkit cratesandfind /workspace -path '*markkit*'hit the 30 s timeout on a 3.7k-file checkout.Before / After
cargo bench --bench remote_fs -- --counts, bashkit checkout (3,720 files), backend with an index. Full tables incl. plain and bulk-walk backends:crates/bashkit/benches/results/criterion-remote_fs-linux-x86_64-1793656706.md.rg -l markkit cratesrg -l SearchCapable crates(13 hits)grep -rln markkit cratesfind /workspace -path '*markkit*'find crates -lsdu -s cratescp -r crates/bashkit/src/builtins …tar -cf … crates/bashkit/src/builtinsrg -l markkit crates(no index)Acceptance tests (
tests/integration/remote_fs_round_trip_tests.rs, 1,000-file tree):rg PATTERN dirmakes exactly onesearchand reads only the 20 matching files;find dir -type fmakes oneread_dirper directory (111) and at most one metadata lookup (the root). Differential tests assert index vs walk and bulk-walk vsread_dirgive byte-identical output across.gitignore/.ignore/.rgignore, parent ignore files, re-includes, hidden dirs,--max-depth,-g, symlinks,-L,-maxdepth/-mindepth,-printf %n.Not changed, noted:
ls -R/treeare already oneread_dirper directory (no bulk walk: they skip dot dirs / stop at an entry budget, so an eager default walk would read more);**globs list each directory twice (TODO in test). Bench wall times are CPU-bound byInMemoryFs::read_dir, which scans every entry per call; worth a separate fix.Risk
grep -r/rgnow trust an empty index answer (documented onSearchCapable::search_provider: returnNoneif the index may be stale). One unit test that relied on the old re-walk was updated.read_dirmetadata to equallstat; custom backends must keep them consistent (documented indocs/filesystem.md).cargo test --workspace --lib --bins --tests --features http_client,ssh,sqlitepasses exceptssh_supabase_connects(external host, unrelated); doc tests pass; fmt/clippy clean.Checklist
<a href="
''https://backend.blacksmith.sh/track/enable-autofix?expires=1796249005&installation_model_id=447267&pr_number=2816&ref=codesmith_pr_footer&repository=everruns%2Fbashkit&return_to=https%3A%2F%2Fgithub.com%2Feverruns%2Fbashkit%2Fpull%2F2816&signature=cc2259c6436b37e855854845a48e5afa09035dd8b0726876f8dfc1b20c4962fd"><picture><sourcemedia="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark-3x.png" width="190" height="28">``Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Generated by Claude Code