Skip to content

fix(cleanup): unlink staged Unix descendant symlinks - #25

Merged
harry0703 merged 2 commits into
harry0703:mainfrom
ivg-design:fix/safe-special-entry-cleanup
Aug 21, 2026
Merged

harry0703 merged 2 commits into
harry0703:mainfrom
ivg-design:fix/safe-special-entry-cleanup

Conversation

@ivg-design

Copy link
Copy Markdown
Contributor

Context

5637f13 fixed 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 deleteWholeRoot execution 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:

  1. The selected root passes the existing protected-path, link, metadata, and identity checks.
  2. The root is moved into MangoDisk's private staging directory.
  3. The cancellable traversal reads each descendant with symlink_metadata.
  4. Any link-like descendant is treated as a fatal error.
  5. MangoDisk restores the staged root and reports the cleanup as partial/items skipped.

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_file on the link entry and returning success immediately:

  • whole-tree removal used by deleteWholeRoot, including Xcode DerivedData;
  • contents-only removal used by bulk cleanup of complete authorized directories.

The traversal continues to use symlink_metadata, so the link is identified without resolving its target. remove_file unlinks 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

  • A symlink selected directly as the cleanup target remains rejected by the existing preflight validation.
  • The containing directory must still pass the existing protected-path, canonical-path, metadata, and physical-identity checks.
  • The existing private same-volume staging boundary remains unchanged.
  • No symlink target is resolved, traversed, measured, or deleted.
  • FIFOs, Unix sockets, and other unsupported special entries retain the behavior introduced by 5637f13; this change does not duplicate or weaken that fix.
  • Only actual Unix symbolic links receive the new behavior. Other link-like entries continue to fail closed.
  • Windows reparse points and junctions retain the existing fail-closed behavior.
  • Existing cancellation checks before and between directory entries remain unchanged.

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:

  1. Whole-tree deletion with a descendant symlink to an external directory:
    • removes the approved directory tree;
    • leaves the external target and its file untouched;
    • does not add symlink bytes or a regular-file item to the outcome.
  2. Contents-only deletion with a descendant symlink to an external directory:
    • removes the symlink entry;
    • leaves the external target untouched;
    • preserves the expected retained directory skeleton;
    • does not add symlink bytes or a regular-file item to the outcome.

These tests exercise the filesystem primitives used by Xcode DerivedData cleanup without adding Xcode-specific policy or fixtures.

Validation

  • pnpm check
    • 58 frontend test files passed
    • 278 frontend tests passed
    • formatting, lint, type checking, frontend build, Rust formatting, Clippy, and workspace checks passed
  • cargo test --manifest-path src-tauri/Cargo.toml -p mangodisk-core
    • 372 passed
    • 0 failed
    • 61 ignored

Validation 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.

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.
Repository owner deleted a comment from wangxutech2010-wq Aug 21, 2026
@harry0703

Copy link
Copy Markdown
Owner

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.

@ivg-design

Copy link
Copy Markdown
Contributor Author

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:

  • Is set after a staged traversal successfully removes an entry.
  • Is preserved when parallel traversal outcomes are combined.
  • Causes rollback to report a partial deletion when any irreversible mutation occurred, even if released_bytes and affected_item_count remain zero.

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
before the root directory is removed. It verifies that:

  • The result is reported as partial.
  • The remaining directory is restored.
  • Byte and regular-file counts remain zero.
  • The symlink remains removed.
  • The external symlink target remains untouched.

Validation passes:

  • mangodisk-core: 373 passed, 61 ignored
  • Frontend: 278 passed
  • Full formatting, Clippy, workspace checks, and production build

@harry0703
harry0703 merged commit b4724c5 into harry0703:main Aug 21, 2026
2 of 4 checks passed
@harry0703

Copy link
Copy Markdown
Owner

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!

@harry0703

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants