Repository navigation
perf(core): change only directory permissions before deleting a tree - #762
Merged
Merged
Conversation
Unlinking an entry on Unix needs write and search permission on its parent directory, never on the entry. remove_tree_if_present used to lstat and chmod every file, directory, and symlink before remove_dir_all walked the tree again. The new riftri_storage::make_tree_removable walks directories only, takes child types from readdir, and chmods a directory only when it lacks an owner read, write, or search bit. Symlinks are never followed or changed. Windows keeps the per-entry pass, because the read-only attribute blocks deletion there.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Kinfe123
added a commit
that referenced
this pull request
Oct 30, 2026
## Problem `riftri gc --apply` and `riftri repair` re-read whole journal directories inside per-item loops: - `retire_finished_journals` called `locked_retirable_lineage` once per retirable add, and each call read every removal, move, and compaction journal: retirable adds x lifecycle journals. - Each candidate base went through `resume_collection`, which called `protected_base_paths` twice (before removing the marker and before quarantining), each reading every add, removal, and compaction journal: candidates x 2 x journals. - Finishing an interrupted retirement read every removal journal once per `.retired` marker. - Candidate bases were measured one at a time, although `status` already measures bases with the 4-worker `tree_usages`. - `repair` read the whole removal, move, or compaction store once per pending operation to find that one journal by id, and `resume_removal` read every add journal twice (once to validate, once to find its add). ## Change - **Retirement** (`retire_locked_lineages`): finished lineages are retired in batches of up to 64. A batch try-locks each add (never waiting, as before), fires the existing `JournalRetirementLocked` hook per lock, then reads the removal, move, and compaction journals once (`LockedLifecycleJournals`). Each lineage is then decided exactly as before by `locked_retirable_lineage`, which now filters that shared read and still reads its own add journal. A lineage's lock is released when it is done. - **Interrupted retirements**: one removal-journal read serves every `.retired` marker (read lazily, only if a marker exists). - **Collection** (`collect_locked_batch`): `resume_collection`'s body is split into its three journaled steps (`remove_collection_marker`, `quarantine_collected_base`, `delete_quarantined_base`) with no change to what each step checks or writes; `resume_collection` (used by recovery and `repair`) still runs them for one journal with a fresh `protected_base_paths` read per step. `gc --apply` records intents and takes base locks for up to 16 planned bases in sorted base order, then runs each step for the whole batch with one protection read per step: the first after all locks are held, the second after every marker removal and before any quarantine. Cancelled members release their lock immediately, the rest after deletion. Only the first lock of a batch waits; a busy base starts the next batch, so collection never waits for a lock while holding another. - **Candidate sizing** uses `tree_usages`, in plan order. - **Repair**: new `load_listed(id)` on the removal, move, and compaction stores reads one journal by name with exactly `load_all`'s per-file semantics (in-flight retry, a journal retired since the listing is `None`, any other failure is an error). `resume_removal` reads the add journals once and uses them for both the validation and the lookup. - Unit-test builds use batches of 2 bases and 4 lineages (`cfg(test)` constants), so the existing unit and Hegel fixtures, not only the new tests, cross batch boundaries. New test-only race point `CollectionMarkerRemoved`. Docs: architecture.md (base collection) and D043 describe the batches. CHANGELOG entry appended under Unreleased > Changed. ## Safety: every decision still reads journals after its lock - Retirement: each add in a batch is locked before the batch's single read, so every lineage is decided from journals read under its own lock, as the per-add read was. A journal for an add of this batch is written under that add's lock (the premise the per-add re-read already relied on), and the batch holds it until that lineage is done. An add of a later batch is decided from that batch's read, which follows its own locks. Which adds are tried still comes from the unlocked snapshot, as before. - Collection, first check: read once per batch after every base lock in the batch is held, so each base's check is under its lock, as before. Intents are persisted before the lock, as before. - Collection, second check (the one that matters for racing adds): read once per batch after every member's marker removal, so for each base it still follows that base's marker removal and is still under its lock. `a_collection_batch_rechecks_references_before_quarantine` records an add intent for one base from a hook fired after the first marker removal of the batch, i.e. after the first read: the second read sees it, the base gets its marker back and is kept, and the other base is collected. - A reference recorded after the second read was already possible between the check and the rename; the add waits for the base lock and rebuilds the quarantined base, as before. Within a batch, quarantine renames for all members happen right after the second read and before any deletion, so that window stays a few renames long. - Deadlocks: collection only waits for a base lock while holding no other lock (first member of a batch; later members use a non-blocking try). Retirement never waits for add locks. - Repair's by-name reads happen under the same add lock the whole-store reads did. One difference: `load_all()?` failed the whole `repair` when any *other* journal of that kind became unreadable mid-pass; a by-name read fails only for the journal being resumed. The inventory at the start of the pass still reads every journal and fails closed as before. ### Concurrency and error behavior that changed (documented in code and docs) - Lock hold time: a base lock can now be held while up to 15 other bases of its batch are processed, so an add that wants a base in the batch may wait for that batch rather than for one base. Cancelled members release their lock as soon as they are cancelled. Retirement holds up to 64 add locks at once; a concurrent `repair` counts those adds as busy and skips them, as it does for any held lock. - Errors: a candidate that fails before its batch runs (path or base validation, lock open or validation errors) still stops the run only after the bases ahead of it are collected, so `collection_deletes_only_real_marked_bases` (Hegel, asserts no pending collections after a refusal) passes unchanged. An error *inside* a batch (I/O failure, an injected `fail_after`, or a base changed under the lock) stops the run as before, but other members of that batch may be left at `intent-recorded` or `marker-removed` instead of not started. That is exactly the state an interruption leaves; the next `gc --apply` (which resumes pending collections first) or `riftri repair` resumes them through the unchanged `resume_collection`. - Progress lines for multi-base runs now interleave by step (all `intent-recorded`, then `marker-removed`, ...) instead of per base. ## Tests - `garbage_collection_reads_each_journal_a_bounded_number_of_times` (new): 4 live worktrees, 6 finished lineages on 6 trees, with the test batch sizes (two retirement and three collection batches). Counts journal file opens with the existing `JournalOpen` test hook and asserts the exact total: every journal once, each retired add once more, the removals still present once per retirement batch, the live adds once for the plan and twice per collection batch (58 here; 40 with the production batch sizes; `origin/main`: 95). Reads grow with batches, not with lineages x journals or bases x journals. - `repair_reads_each_pending_removal_alone` (new): 3 pending removals among 6 adds and 6 removals; asserts the exact total, in which each pending removal costs `2 + adds` opens (its by-name read, its record reload, one add read) instead of `removals + 2 * adds + 1`. - `a_collection_batch_rechecks_references_before_quarantine` (new, described above). - `a_busy_base_starts_the_next_collection_batch` (new): with a batch's second base locked by the test, the first base is collected without waiting, the busy base is untouched while locked, and after release it starts the next batch and all three bases are collected with no pending journals. - Existing gc, retirement (including the `JournalRetirementLocked` race test), repair, recovery, kill-fuzz, concurrency, and Hegel collection-state, lifecycle, and state tests pass unchanged. ## Measurements macOS 26.2 arm64 laptop, APFS, release builds of `origin/main` (401e244) and this branch, user shim removed from `PATH`, temporary `HOME`/`RIFTRI_CACHE_DIR`. Fixture: a 10-file repository with 20 live worktrees on the first commit and 50 finished lineages (`worktree add --detach` then `worktree remove`) spread over 20 later commits: 120 journals, 50 retirable adds, 20 collectible bases. Each run starts from an APFS clone (`cp -c -R -p`) of that state at the same path; binaries alternate ABBA, n=12 each. For `repair`, 20 of the 50 completed removal journals are rewound to `worktree-removed` (a crash after Git removed the view, before the base was released). Journal file opens, counted with the `JournalOpen` test hook in a unit test that builds the same shape through the core API (`origin/main` sources plus the same test for "before"): | | before | after | | --- | ---: | ---: | | `gc --apply` (retire 100 journals, collect 20 bases) | 2,265 | 320 | | `repair` (resume 20 pending removals) | 4,320 | 1,940 | Wall time (median, min / max): | | before | after | | --- | ---: | ---: | | `riftri --no-progress gc --apply` | 1252 ms (1180 / 1855) | 1190 ms (1115 / 1422) | | `riftri --no-progress repair` | 499 ms (474 / 1080) | 412 ms (400 / 535) | Wall time is dominated by durable journal writes, base deletion, and Git processes at this size; the read counts are the scaling result. Laptop timings swing between runs, so treat them as one observation. ## Gates `cargo fmt --all --check`; `cargo clippy --workspace --all-targets --all-features -- -D warnings` on the host and with `--target x86_64-pc-windows-msvc` and `--target x86_64-unknown-linux-gnu`; `cargo test --workspace` (1,144 passed, 0 failed, 26 ignored; the Hegel termination tests ran and passed, nothing skipped). Independent of #762: a throwaway local merge of both branches merges `worktree.rs` and the docs cleanly and passes (workspace clippy; `riftri-storage` tests; `native_cow_removal` and `native_cow_compaction`; the 136 riftri-core unit tests matching gc, collect, retire, garbage, removal, repair, or compaction). The only conflict is `CHANGELOG.md`, because both PRs append to the end of Unreleased > Changed; keep both entries in merge order. ## Not changed, on purpose - `resume_removal` still reads every add journal once per pending removal: `validate_removal_against_add_journals` checks the removal against all of them, under the add lock. - `resume_move` and `resume_compaction` still find their add with `load_all().find(..)` (one read per pending operation); a by-name read would stop failing closed on other unreadable add journals there, which I left alone. - Pending collections resumed at the start of `gc --apply` and by `repair` still run one at a time with fresh reads per step; they are rare and usually single. - `retire_vanished_add_journal` and `superseding_add_journal` read whole stores per affected journal; they run only for vanished or superseded adds. ## Not verified locally - Linux and Windows runtime (CI covers Btrfs, XFS, OverlayFS, ReFS); `--target` clippy only. - Production batch sizes crossing a boundary in a unit test: covered by the `cfg(test)` sizes instead; the measured release fixture (20 bases) ran two production collection batches, and its 50 lineages one retirement batch.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every tree Riftri deletes goes through
remove_tree_if_present(worktree.rs): a removal or compaction quarantine, a collected base, an add's rollback scratch or base staging, and an unfinished base before a rebuild. It first calledNativeCowCloner::make_tree_owner_writable, whichlstats andchmods every file, directory, and symlink in the tree (and reads the process umask), and thenremove_dir_allwalked the whole tree again. On Unix the per-file work is never needed: unlinking an entry takes write and search permission on its parent directory, not on the entry. For a view with a large ignorednode_modules, that was onelstatand onechmodper file before deletion started.Change
riftri_storage::make_tree_removable(path), used only byremove_tree_if_present.tree_removal.rs): walks directories only. Child types come fromreaddir(DirEntry::file_type, which usesd_typeon APFS, Btrfs, and XFS), so files and symlinks are never stat'ed or chmod'ed. Each child directory islstated once (not followed) and chmod'ed tomode | 0o700only when it lacks an owner read, write, or search bit. Read is included becauseremove_dir_allmust list a directory before emptying it. Like the old walk, it collects child names and closes the handle before descending, so a deep tree holds one directory handle at a time.refs::make_tree_owner_writable, because the read-only attribute blocks deletion itself.make_tree_owner_writableandmake_tree_read_onlyare unchanged and still used where they matter (restoring checkout permissions on a cloned view, sealing a base, OverlayFS).docs/architecture.mddescribes the deletion preparation; CHANGELOG entry appended under Unreleased > Changed.Safety
remove_tree_if_present:resume_collection(quarantined base),prepare_base(an incomplete base before rebuild),retire_superseded_add_journalandrollback_decoded(scratch view and base staging),remove_native_worktreeandresume_removal(removal quarantine),resume_compaction,restore_original_after_compaction, andcancel_unactivated_compaction(replacement and dropped view), andremove_unfinished_base. None needs a file to be writable afterwards; each deletes the tree it prepared.make_tree_read_only(0o555directories,0o444/0o555files), andresume_collectionrestores owner rwx on the base root before quarantining it. Deleting a base now chmods its nested directories (111 of 112 in the benchmark base) and no files. The new integration test collects a base with nested read-only directories.lstat, a child is descended into only when bothreaddirandlstatsay it is a real directory, andchmodis applied only to such directories.remove_dir_all(std,openat/unlinkatwithO_NOFOLLOW) does not follow links either. Tests check that a link to an outside read-only directory and one to an outside read-only file leave both targets' modes and contents unchanged.uchg/schg(the onlychflagscall isnodumpin a checkout_clone unit test). If a user's view holds auchgfile, deletion fails both before and after this change: before, at thechmod(whichuchgrefuses) before anything was deleted; now, at that file'sunlink, afterremove_dir_allmay have deleted earlier entries. Every caller deletes only at a journaled point of no return (quarantine after Git unregistered, a collected base, rollback staging), and a retry orriftri repairfinishes a partial delete, exactly as for any other interruptedremove_dir_all(architecture.md already documents this for removal and compaction). The same holds for Linuxchattr +i. macOS ACLs are untouched either way.chmodfails as before. A file owned by someone else no longer has to be chmod'ed, so deletion no longer fails on that when the directory permits it. That chmod failure was incidental, not a safety check; dirtiness and force-snapshot checks run before any of these paths are deleted.Tests
tree_removal::tests(riftri-storage, new):read_only_tree_is_deleted_without_touching_files_or_link_targets: nested0o555directories,0o444files, a relative link inside, links to an outside0o555directory and0o444file. Thread-local counters record everylstatandchmod: exactly the three directories missing owner bits are chmod'ed, only the four directories are stat'ed, files keep0o444, an already-writable directory keeps its mode,remove_dir_allthen succeeds, and the outside targets keep their modes and contents.writable_tree_needs_no_permission_changes: zerochmodcalls and onelstatper directory.missing_owner_read_or_search_is_restored: directories at0o300and0o600.a_link_or_file_root_is_left_alone.deep_removal_walk_bounds_directory_handles(native_deep_walk.rs, new): the walk succeeds on a 129-level read-only tree withRLIMIT_NOFILElowered to 64. (std'sremove_dir_allitself keeps one handle per level, so it cannot run under that limit; the test checks the walk, and the parent deletes the tree.)removal_and_collection_delete_read_only_trees_without_following_links(native_cow_removal.rs, new): a cleanremove_worktreeof a view holding an ignorednode_moduleswith0o444files,0o555/0o500directories, and links out of the view, thengarbage_collect --applyof its nested read-only base. Verified to fail when the walk is disabled.Measurements
macOS 26.2 arm64 laptop, APFS, release builds of
origin/main(401e244) and this branch, user shim removed fromPATH, temporaryHOMEandRIFTRI_CACHE_DIR. Each iteration runs both binaries in alternating order (ABBA), n=12 each. Laptop timings swing between runs; treat the times as one observation, the call counts as the result.Repository: 5,000 tracked files (100 directories of 50, two levels deep) and
.gitignore=node_modules/.riftri worktree removeof a clean view plus 20,000 ignored files in 801 directories, medianriftri gc --applycollecting the 5,000-file base, medianPermission calls in the deletion pass, from the code paths applied to the measured trees (
findcounts; the unit-test counters confirm the new path's counts on fixtures):lstat/chmodlstat/chmod0o755)0o555)Steps per iteration and binary (a Python driver runs them and times only the measured command):
Gates
cargo fmt --all --check;cargo clippy --workspace --all-targets --all-features -- -D warningson the host and with--target x86_64-pc-windows-msvcand--target x86_64-unknown-linux-gnu;cargo test --workspace(1,146 passed, 0 failed, 26 ignored; the Hegel termination tests ran and passed, nothing skipped).Not verified locally