fix(cleanup): unlink staged Unix descendant symlinks - #25
Conversation
Remove Unix symlink entries encountered inside approved staged directory trees without following their targets. Keep direct target validation and Windows reparse-point handling fail-closed, with regression coverage for whole-tree and contents-only cleanup.
|
Thanks for the careful investigation and the detailed PR. I reproduced the descendant-symlink case locally, and the external target remained untouched as expected. I also ran three alternating baseline/PR performance rounds with 10,000 small files. The median differences across the affected deletion paths were approximately +1.0%, +3.0%, and -1.8%, so I did not find a meaningful performance regression. Deleting 10,000 descendant symlinks took about 507 ms in the Release build. I did find one cancellation edge case that should be addressed before merging: if a descendant symlink is unlinked and cancellation or another error occurs before the root directory is removed, the remaining tree is restored, but the result is still reported as a pre-mutation, non-partial failure because the symlink contributes zero bytes and zero affected regular-file items. In that state, the symlink has already been permanently removed. I reproduced this deterministically by cancelling immediately after the symlink was unlinked. Could you track that irreversible mutation separately, without changing the regular-file byte/item accounting, and add a regression test for it? Thanks again for the contribution and for keeping the change focused. |
|
Good catch with this edge case! I’ve pushed an update in e08414d that tracks irreversible filesystem mutation independently from the regular-file byte and item counters. The new internal marker:
The marker is intentionally generic rather than symlink-specific, so the same reporting gap cannot occur when another uncounted entry, such as an empty directory, has already been removed. Regular-file byte and item accounting remains unchanged. I also added a deterministic Unix regression test that cancels immediately after a descendant symlink is unlinked but
Validation passes:
|
|
Thanks for the quick follow-up and the thoughtful fix. I reviewed the update and verified the merged result on macOS, including the descendant symlink cleanup, cancellation reporting, and the 10,000-file cleanup benchmark. The Windows checks and native build also passed. Everything looks good, and the PR has now been merged. I really appreciate the careful investigation and focused contribution! |
|
This improvement is now available in MangoDisk 1.0.6: https://github.com/harry0703/MangoDisk/releases/tag/v1.0.6 Thanks again for digging into the descendant-symlink case so carefully and following through on the cancellation edge case. The final change keeps external link targets protected and makes partial cleanup results more accurate. I really appreciate the contribution. |
Context
5637f13fixed the reported Unix FIFO hang by rejecting special files before deletion and using non-blocking, no-follow identity handles. This pull request builds on that change and addresses a separate cleanup failure exposed by the same investigation.An approved directory cleanup currently rolls back when its staged tree contains an ordinary descendant symbolic link. This affects Xcode DerivedData in particular, because DerivedData commonly contains symlinks created by Xcode, Swift Package Manager, and versioned build products.
Current behavior
The Xcode DerivedData rule owns the complete DerivedData root and uses the
deleteWholeRootexecution path. The permanent-delete flow correctly validates the selected root, captures its physical identity, and moves it into a private same-volume staging directory before recursive removal.The failure occurs during that final staged traversal:
symlink_metadata.This is appropriate for a symlink supplied as the top-level deletion target, where following or accepting the target would cross the user's approved boundary. It is unnecessarily restrictive for a symlink entry already contained by an approved, privately staged directory tree. The entry itself belongs to that tree and can be unlinked without traversing its target.
Leaving the link in place is not useful for whole-root cleanup: the containing directory cannot be removed while the entry remains, so the current behavior causes the entire otherwise-valid cleanup to roll back.
Change
On Unix, both cancellable staged traversal modes now handle a descendant symbolic link by calling
fs::remove_fileon the link entry and returning success immediately:deleteWholeRoot, including Xcode DerivedData;The traversal continues to use
symlink_metadata, so the link is identified without resolving its target.remove_fileunlinks the Unix directory entry; it does not recurse into or delete the target, including when the target is a directory.Symlink entries continue to contribute zero released bytes and zero affected regular-file items. If unlinking fails, the error still propagates through the existing staged-removal failure and rollback path.
Safety boundaries preserved
5637f13; this change does not duplicate or weaken that fix.Why unlink instead of skip
Scanning may omit symlink targets from byte and file accounting, but execution still has to remove the symlink entry to complete deletion of the approved directory. Skipping the entry would leave the staged tree non-empty and prevent removal of the selected root. Unlinking the entry is therefore the minimal operation that satisfies the cleanup request without expanding its ownership boundary.
Tests
Added Unix regression coverage for both staged traversal modes:
These tests exercise the filesystem primitives used by Xcode DerivedData cleanup without adding Xcode-specific policy or fixtures.
Validation
pnpm checkcargo test --manifest-path src-tauri/Cargo.toml -p mangodisk-coreValidation was run on macOS/Apple Silicon. The implementation is Unix-gated, and Windows reparse-point behavior is unchanged; Windows validation was not available locally for this follow-up and should be confirmed by CI.
Scope
This pull request changes one Core filesystem file. It does not change the Xcode cleanup rule, the UI, cleanup selection policy, or the FIFO/special-file fix already merged in
5637f13.