Repository navigation
feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) - #1074
Conversation
…crash (#445) Fresh re-cut of #1070 on the hardened main (1f37cd8), not a retarget: that PR was built on the history of the squash-merged #1069 and its code predates the quarantine sync, the identity checks and the fail-closed look that main has now. Its one own change is re-applied on that code, with its tests. test-support only, no caller, no new unsafe, no new package. Windows still answers Unsupported. - remove_empty_dir opens the child directory (no link followed), refuses one that has entries and moves it through the same quarantine directory; it is identified by the open directory and unlinked with AT_REMOVEDIR. A directory that fills after the check goes back (NotEmpty), through the step that syncs the quarantine directory like any put-back. - remove_leftover_file takes a .h1q-* directory by its generated name only (the prefix and exactly 32 lowercase hexadecimal digits, otherwise InvalidName before anything is opened), requires a directory of the effective user without access for group or others, authenticates the file in it again and unlinks it only if accepted. A file that does not authenticate and anything that is not a regular file (a link, a directory) stay and give Refused; an empty generated directory is removed. - On the hardened code the leftover removal syncs the quarantine directory it unlinks in and says Removed(NotConfirmed) if that fails; an entry that vanished before the unlink is reported like one that vanished before. Tests (own file): a directory swapped or filled after the check, an empty directory that wins the last window (Disturbed), a simulated crash and its recovery, the name grammar (short, long, upper case, not hexadecimal), a foreign object and a link left in the quarantine directory, a stranded entry (put-back blocked, proved to be there) finished by the recovery, and the failed sync of the held directory. The earlier permission-based stranded case is now Disturbed on main (a failed look proves nothing) and is not asserted here. Mutation-checked: no sync in the leftover removal, no refusal of a foreign entry, a loosened name grammar and an ignored authentication each fail a case. The PR number in the changelog entry is to be set to the number of this PR when it is opened. Issue #359.
Reviewer's GuideThis PR finishes the Unix directory-removal slice and crash-leftover recovery by routing directories through descriptor- and identity-checked quarantine handling, and by safely authenticating and synchronizing leftover cleanup. Extensive test-support coverage exercises race windows, crash and stranded states, malformed names, foreign objects, links, and durability failures; Windows remains unsupported. Sequence diagram for Unix directory removalsequenceDiagram
participant Caller
participant PinnedDir
participant Quarantine
participant Filesystem
Caller->>PinnedDir: remove_dir(name, observer)
PinnedDir->>Filesystem: open_dir(name)
Filesystem-->>PinnedDir: held directory identity
PinnedDir->>Filesystem: entries(0)
alt directory is not empty
Filesystem-->>PinnedDir: TooManyEntries
PinnedDir-->>Caller: NotEmpty
else directory is empty
PinnedDir->>Quarantine: create()
PinnedDir->>Filesystem: move entry with identity check
Quarantine->>Filesystem: unlinkat(AT_REMOVEDIR)
Quarantine-->>PinnedDir: removal outcome
PinnedDir-->>Caller: RemovalOutcome
end
Sequence diagram for Unix crash-leftover recoverysequenceDiagram
participant Caller
participant PinnedDir
participant Quarantine
participant Authenticator
participant Filesystem
Caller->>PinnedDir: remove_leftover(quarantine, authenticate)
PinnedDir->>Quarantine: open_existing(quarantine)
Quarantine->>Filesystem: validate generated name
Quarantine->>Filesystem: open_owned directory
Filesystem-->>Quarantine: directory handle and identity
Quarantine->>Filesystem: inspect file entry
alt foreign object, link, or authentication refused
Quarantine->>Authenticator: authenticate(file)
Authenticator-->>Quarantine: false or refused object
Quarantine-->>PinnedDir: Refused
else accepted regular file
Quarantine->>Authenticator: authenticate(file)
Authenticator-->>Quarantine: true
Quarantine->>Filesystem: unlink verified file
Quarantine->>Filesystem: sync quarantine directory
Filesystem-->>Quarantine: sync result
Quarantine-->>PinnedDir: Removed or NotConfirmed
else empty quarantine directory
Quarantine-->>PinnedDir: Vanished
end
PinnedDir-->>Caller: RemovalOutcome
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 35 minutes. View limit details
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
|
[check-pr-size] PR size is over the target tier (normal profile): 9 files, 776 meaningful lines, 9 commits — limit ≤8 files / ≤400 lines / ≤6 commits. Consider splitting into smaller, independently reviewable PRs. |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="402" />
<code_context>
+) -> Result<RemovalOutcome, PinnedError> {
+ let held = Quarantine::open_existing(&dir.handle, quarantine)?;
+ let step = leftover_step(&held, dir, authenticate, observer);
+ held.discard(&dir.handle);
+ settle(dir, step?, observer)
+}
</code_context>
<issue_to_address>
**Empty quarantine reappears after crashes**
When authentication refuses or errors, or inspection errors, while another actor removes the held entry before `held.discard`, `leftover_step` returns `Unchanged` or an error, but `held.discard` removes the empty quarantine directory. `settle` skips syncing the parent for `Unchanged`, and `step?` returns before `settle` on errors, so the caller gets `Refused` or an error while a crash can restore the empty quarantine directory.
Sync the parent whenever `held.discard` removes the quarantine directory, including when `leftover_step` is unchanged or returns an error, while preserving the operation result.
Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:572`.
</issue_to_address>
### Comment 2
<location path="crates/worldscript-secure-storage/src/pinned/unix/removal.rs" line_range="373-391" />
<code_context>
remove_moved(dir, target, observer)
}
+pub(in crate::pinned) fn remove_dir(
+ dir: &PinnedDir,
+ name: &OsStr,
+ observer: Observer<'_>,
+) -> Result<RemovalOutcome, PinnedError> {
+ let child = dir.open_dir(name)?;
+ match child.entries(0) {
+ Ok(_) => {}
+ Err(PinnedError::TooManyEntries) => return Ok(RemovalOutcome::NotEmpty),
+ Err(error) => return Err(error),
+ }
+ observer(RemovalStage::Checked)?;
+ let target = Target {
</code_context>
<issue_to_address>
**Stranded directories cannot be recovered**
When a crash or failed put-back leaves a directory inside a quarantine directory, a crash or blocked put-back during `remove_dir` leaves the directory as `x` in quarantine, but `leftover_step` inspects only `EntryKind::File` and treats the directory as foreign, so `remove_leftover_file` returns `Refused` and leaves the stranded quarantine entry in place.
Provide recovery for an empty stranded directory, or otherwise ensure the stranded-directory outcome has a supported cleanup path.
Also at `crates/worldscript-secure-storage/src/pinned/unix/removal.rs:413-417`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if the leftover cleanup identifies the wrong object or unlinks one that should have been retained, that file is permanently deleted; reverting prevents future removals but cannot restore files already unlinked. The implementation is test-support-only, which limits where it can run but does not make an executed deletion reversible.
Blocking findings: crates/worldscript-secure-storage/src/pinned/unix/removal.rs:402, crates/worldscript-secure-storage/src/pinned/unix/removal.rs:391
CodeAnt Nitpicks1 code suggestion1.
|
CodeAnt PR Risk: Low Risk
Assessed commit: |
…e the limits of the leftover recovery (#445) Carries the findings that were open on the superseded #1070 into this PR, checked against the code of main (most were already closed by the hardening it has: the rollback sync, the sync of the held directory, the identity-checked cleanup, the root-safe test). A directory that cannot be removed because it has entries goes back where it was and was reported as NotEmpty. If an actor won the last window by moving the authenticated empty directory away and putting a non-empty one in its place, the refusal was about that directory and said nothing about ours (Codex P2 on #1070). The unlink refusal now checks that what is inside is still the authenticated directory (held open, so its identity is unique); if not, it goes back under the name all the same and the outcome is Disturbed. A put-back that fails stays a failure that ends in Stranded or Disturbed as before. Stated, not changed: an empty foreign directory that wins the same window is still removed instead of the authenticated one (no Unix primitive unlinks "this inode"; it holds no data, and the removal reports Disturbed), and the leftover recovery reclaims files, so an empty directory left by a crashed directory removal stays in its .h1q-* directory (Sourcery on #1070). Both are in the changelog, the evidence document and the API documentation. Test: a non-empty directory swapped in at the last window gives Disturbed with the original directory intact and the swapped-in one back under the name. Mutation-checked: reporting NotEmpty for any directory fails it. test-support only, no caller, no new unsafe, no new package. Issue #359.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01917c1c65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d62328e5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…path, not its link count (#445) The hosted macOS evidence of this PR failed the removal of an empty directory: the outcome was Disturbed instead of Removed(Confirmed). The check after the unlink asked the open directory whether its link count had dropped, which holds on Linux, where a removed directory that is still open reports zero. macOS keeps reporting the old count (or cannot be asked), so the check could never confirm a removal there. The superseded #1070 never ran on a hosted OS (a stacked PR gets no hosted evidence), so this was the first time that path met macOS; the other directory cases passed there only because they expect Disturbed. For a directory on Apple targets the signal is now that the open descriptor no longer has a path: fcntl(F_GETPATH), rustix::fs::getpath, fails with "no such file" for a removed directory and still answers for one that an actor moved away and that is therefore not the one that was removed. Anything else, such as a path that is still there or another error, stays Disturbed (a removal that cannot be checked afterwards says nothing). Linux and files are unchanged. No new package and no new feature: getpath needs rustix's alloc, which std already enables. Whether macOS answers as described is not known from here; the hosted macOS run of this commit is the evidence, and the changelog and the evidence document state the signal as the one that run showed. test-support only, no caller, no new unsafe. Issue #359.
…ved and a live open directory (#445) Always fails on purpose so the hosted macOS log prints the answer; removed again in the next commit. Not part of the change.
…e the directory is removed (#445) Still temporary and removed again in the next commit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e294a2cf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the system cannot tell afterwards (#445) The hosted macOS evidence of this PR failed the removal of an empty directory (Disturbed instead of Removed). A first attempt to read the removal from fcntl(F_GETPATH) was also refused by macOS, so the answer was measured instead of guessed: a temporary probe on the hosted runner (commits b59fe38 and 8e294a2, removed here) compared an open directory that was removed, one that was only moved and one that was untouched. link count 2 / 2 / 2 (unchanged for the removed one) F_GETPATH the old path for the removed one (stale) openat(".."), statat(".") succeed for all three create an entry in it fails with ENOENT only for the removed one The removal of a directory counts only if the open directory can be asked afterwards whether it was the one that lost its name. Linux answers (a removed directory that is still open reports a link count of zero). On macOS the only signal is a write into a directory that, in the attack case, an actor moved away and that belongs to another path, and it fails for lack of permission on a read-only directory. That is not a check to build a success on. remove_empty_dir therefore answers Unsupported on any Unix system but Linux, after the checks that need no such signal (a link, a missing name, a directory that has entries) and before anything is moved. The directory cases move to a Linux-only test file; on the other systems the same call is pinned to Unsupported with nothing moved, and the generic directory test expects it. The leftover recovery (files) is unchanged and runs on both. Whether and how macOS gets directory removal is left to the decision about the production use. The changelog, the evidence document and the API documentation state the limit and the measured behaviour. test-support only, no caller, no new unsafe, no new package. Issue #359.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…s, look at a restored directory again, recover an empty directory leftover (#445) Three findings on 2989ba8, all valid. A leftover removal removes the emptied quarantine directory with discard, which changes the parent. When the removal itself changed nothing (a refusal) or failed, that change was never synced: Unchanged skipped the sync, and an error returned before the settle. A crash could then bring the empty directory back although the caller had been told the result (Sourcery, Codex P2 and a CodeAnt nitpick, independently). discard now reports whether it removed the directory; the parent is synced in that case for a refusal and for an error, and the result of the removal is kept. A directory that cannot be removed because it has entries goes back under its name, and was looked at before the rename. An actor that swaps another directory in between the look and the rename had it restored and reported as NotEmpty (Codex P2). It is now looked at under its name after it went back, against the identity of the open authenticated directory, and anything else is Disturbed. The earlier look is gone: it was the same check at a worse place. A crashed directory removal leaves an empty directory as x in its quarantine directory, and the leftover recovery only took files (Sourcery, on #1070 and again here). An empty directory holds nothing, so it needs no authentication and goes with the same unlink and the same check as the removal of a directory, on Linux; elsewhere it is refused, a directory with entries stays (NotEmpty), and a link or a FIFO stays (Refused). The temporary macOS probe thread (Codex P1) is obsolete: the probe was removed in the previous commit and the hosted macOS run is green. Tests: the parent sync for a refusal and an error with the entry taken, and not when the entry stays; the recovery of an empty directory after a crash during a directory removal, with the callback never asked; a directory with entries stays; Refused for an empty directory where the removal cannot be checked. Each of the three changes is mutation-checked. test-support only, no caller, no new unsafe, no new package. Issue #359.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
e2876f84-a6fe-490d-baec-746c2fe4bb44
📒 Files selected for processing: 8
CHANGELOG.mdcrates/worldscript-secure-storage/src/pinned/removal.rscrates/worldscript-secure-storage/src/pinned/tests/mod.rscrates/worldscript-secure-storage/src/pinned/tests/remove.rscrates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rscrates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rscrates/worldscript-secure-storage/src/pinned/unix/removal.rsdocs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
There was a problem hiding this comment.
Gates Passed
3 Quality Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
What this is
A fresh re-cut of #1070 on the hardened
main(1f37cd83), not a retarget. #1070 was stacked on the history of the squash-merged #1069 and its code predates whatmainhas now (quarantine-directory sync, identity checks, fail-closed look at a retained entry). Its one own change (24683a83) is re-applied on that code with its tests; one signed commit onmain. #1070 stays frozen until this PR is integrated and is then closed as superseded, not merged. No history was rewritten.test-supportonly: no caller, no newunsafe, no new package. Windows still answersUnsupported(its own slice, #1071), and so does the removal of a directory on every Unix system but Linux (see below). Slice H1a-2 part 3, part of #359 (Linear QNB-168).What it adds
remove_empty_dir(Linux only): opens the child directory (no link followed), refuses one with entries (NotEmpty), and moves it through the same quarantine directory; identified by the open directory, thenunlinkat(AT_REMOVEDIR). A directory that fills after the check goes back, through the step that syncs the quarantine directory like any put-back.remove_leftover_file: takes a.h1q-*directory by its generated name only (prefix plus exactly 32 lowercase hex digits, otherwiseInvalidNamebefore anything is opened), requires a directory of the effective user without group/other access, authenticates the file in it again and unlinks it only if accepted. A file that does not authenticate stays (Refused), and so does anything that is neither a regular file nor an empty directory (a link, a FIFO; a directory with entries isNotEmpty). An empty directory left by a crashed directory removal is removed without authentication (Linux), and an emptied generated quarantine directory is removed; the parent is synced whenever that removal changed it, whatever the result.What changed in the re-application (the point of the re-cut)
Removed(NotConfirmed)if that fails; an entry that vanished before the unlink is reported like one that vanished before. (feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) #1070's code had neither.)Quarantine::open_existingrecords the directory's identity, so its cleanup uses the same identity-checked removal as every other quarantine directory.mainthat case isDisturbed(a failed look proves nothing). The stranded recovery is tested with a blocked put-back, where the entry is proved to be there, and runs for root too.Unsupportedon Unix.Tests and proof (own file
remove_unix_dir.rs)A directory swapped or filled after the check; an empty and a non-empty directory that win the last window (
Disturbed, original intact; the non-empty one is not reported asNotEmpty); a simulated crash and its recovery with a refused and an accepted authentication; the name grammar (too short, too long, upper case, not hexadecimal); a foreign object and a link left in the quarantine directory; a stranded entry finished by the recovery; the failed sync of the held directory (NotConfirmed, through thecfg(test)seam that models the error; no crash is simulated). Mutation-checked, each caught: no sync in the leftover removal, no refusal of a foreign entry, a loosened name grammar (this one survived the first version of the tests and drove the extra name cases), an ignored authentication result, and reportingNotEmptyfor any directory.The 7 findings that were open on #1070, and where they stand here
main: a rolled-back or stranded failure syncs the parent (best effort, the cause is kept).Removed(NotConfirmed); tested with the failing-sync seam.discardremoves the name only if it still leads to the directory that was held (remove_if_named);rmdirtakes a name, so the race is narrowed, not closed.NotEmptyreported without rechecking the directoryDisturbed. Testa_non_empty_directory_swapped_in_the_last_window_is_not_reported_as_not_empty; mutation-checked.Disturbed; it holds no data. Part of the owner-gated same-user residual.xby a crashed directory removal holds nothing, so it needs no authentication and goes with the same unlink and check; a directory with entries stays (NotEmpty); a link or FIFO stays (Refused); other systems refuse it. Tests for each; mutation-checked.Findings raised on this PR itself (3 waves)
NotEmptyreported for a directory an actor swapped in (Codex P2 on feat(core): remove a directory on Unix and finish the leftovers of a crash (#445) #1070, fixed, test); the same check moved to after the rename (Codex P2 here, fixed, test).Why directory removal is Linux only (measured, not guessed)
The first hosted macOS run failed the removal of an empty directory (
Disturbedinstead ofRemoved): the check after the unlink asks the open directory whether its link count dropped, which holds on Linux only. A second attempt viafcntl(F_GETPATH)failed the same way, so I stopped guessing and ran a temporary probe on the hosted macOS runner (commitsb59fe385,8e294a2c, removed again in2989ba80) that compares an open directory that was removed, one that was only moved and one that was untouched: the link count stays 2 for all three,F_GETPATHkeeps the old path for the removed one,openat("..")andstatat(".")succeed for all three, and only creating an entry in the removed one fails (ENOENT). That last signal is a write into a directory that, in the attack case, an actor moved away, and it fails for lack of permission on a read-only directory, so it is not a check to build a success on.remove_empty_dirtherefore answersUnsupportedon any Unix system but Linux, after the checks that need no such signal (a link, a missing name, a directory with entries) and before anything is moved; the directory cases run on Linux and the other systems pinUnsupported. Whether and how macOS gets directory removal is left to the decision about the production use (H1b). The leftover recovery (files) is unaffected and runs on macOS.Limits, stated
The same-user residual is unchanged and not accepted for production: the
unlinkat(name)window, the link-count heuristic, and a foreign entry's identity without a held handle (see #359). A removed empty directory that wins the last window is the one case the removal reports asDisturbedafter the fact. Owner-gated before any production use (H1b).Local evidence, hosted CI and review pending
cargo fmt --check;cargo clippy --all-targets -D warningson the host and compile-only foraarch64-apple-darwinandx86_64-pc-windows-msvc; thepinned::unit tests on Linux (the macOS and Windows runs are the hosted evidence); canonicalpnpm run ci:prepush;pr:budgetagainstmain:CAUTION,OK, 8 commits (two of them the temporary macOS probe and its correction), 9 files, 775 meaningful lines. Hosted Windows/macOS/Ubuntu evidence, CI, CodeQL and independent review are pending; nothing from #1070 carries over.Part of #359.
Summary by CodeRabbit
New Features
Documentation