fix(restore): clear chattr +i mount-guard fallback in --cleanup-guards

[tis24dev]

Problem

During restore, when a storage mountpoint is offline, ProxSave applies a read-only bind-mount guard; if the bind mount can't be created it falls back to chattr +i on the mountpoint directory, so PVE/PBS can't silently write into the root filesystem (/).

--cleanup-guards previously only undid the bind-mount guards. The chattr +i fallback was left in place — and unlike a bind mount, the immutable flag persists across reboots and is never cleared automatically, so a user had to run chattr -i by hand (and often didn't know to).

Change

Record each chattr-guarded mountpoint in a host-side index (/var/lib/proxsave/guards/chattr-targets) and make --cleanup-guards reverse it:

• Record on a successful chattr +i at both apply sites (PBS + PVE), written atomically on the real filesystem (the index is host state, not part of the staged restore tree), deduped, with strict validation (datastore mount root only; reject newline/control chars).
• Clear: clearImmutableGuards reads the index and runs chattr -i, but only on targets that are not currently mounted — a live mount shadows the flagged directory, so clearing it would touch the wrong inode (mirrors how hidden bind-mount guards are left in place). Symlinks are resolved and the allowlist re-checked before clearing, so a parent-component symlink can't escape /mnt|/media|/run/media.
• Pending gate: keep the guard directory + index while any immutable target is still pending (mounted / unresolvable / failed) so a later run — once the storage is unmounted — can finish the job; remove them only when nothing is pending and no bind-mount guards remain.
• UX: surface the skip-while-mounted case at Info level, and point users at the manual recovery (lsattr -d / chattr -i) when the guard directory was deleted by hand.

PVE guard apply now uses the existing mountGuardMkdirAll / mountGuardIsPathOnRootFilesystem seams (default behavior unchanged) so the record wiring can be tested without a writable /mnt/pve.

Behavior notes

• Bringing the storage back online is enough to use it again (a real mount stacks on top of the guard); --cleanup-guards just removes the leftover guard.
• To clear a flag while the storage is mounted: unmount → --cleanup-guards (or chattr -i) → remount.
• If the guard directory was already deleted manually, there's no record to clear — documented manual recovery via lsattr -d / chattr -i.

Tests & validation

• New mutation-aware tests (record dedup/validation, real-FS atomic writer perms, PVE + PBS wiring, clear/skip/dry-run/non-datastore-reject/chattr-failure/missing-index/symlink-escape/pending-keeps-index, end-to-end round-trip).
• Full internal/orchestrator suite green with -race -shuffle=on -count=2; cmd/... + internal/cli/... green.
• Adversarially reviewed across correctness, security, and test-rigor over two rounds; all findings closed and mutation-proven.

Docs

Updated RESTORE_GUIDE.md, RESTORE_TECHNICAL.md, CLI_REFERENCE.md, TROUBLESHOOTING.md: distinguish bind-mount vs chattr +i guard lifecycle (the latter survives reboot), fix the "real mount overlays the guard" wording, and document the cleanup / mounted-target / deleted-directory recovery procedures.

Greptile Summary

This PR closes the gap where --cleanup-guards only reversed bind-mount guards but left chattr +i immutable flags in place — a regression that persists across reboots and requires manual recovery. The fix records each successfully-applied chattr +i fallback target in a host-side index (/var/lib/proxsave/guards/chattr-targets) and makes --cleanup-guards reverse them with appropriate safety checks.

• Record path: both PBS and PVE guard-apply sites call recordImmutableGuardTarget after a successful chattr +i, writing atomically (temp-file + rename) with dedup, strict allowlist validation, and control-character rejection.
• Clear path: clearImmutableGuards re-validates each recorded target against the datastore-root allowlist, skips currently-mounted targets (the immutable flag is on the shadowed inode, not the live mount), resolves symlinks and re-checks the allowlist, and runs chattr -i on the resolved path.
• Pending gate: the guard directory and index are kept while any target is still pending (mounted, unresolvable, or failed), so a later run after unmounting can finish the job.

Confidence Score: 5/5

Safe to merge — the chattr guard record/clear logic is well-contained, comprehensively tested, and fails safely at every step.

The change correctly closes a real lifecycle gap (immutable flags persisting after reboots). Guard application and cleanup are both best-effort and non-fatal, so recording or clearing failures degrade gracefully to the pre-PR manual-recovery path rather than aborting the restore. The allowlist double-check (once on the raw target, again on the resolved path) prevents symlink-escape. Test coverage is extensive and mutation-aware across all code paths. Two previously-noted edge cases (no fsync before rename; isMounted checked on the unresolved path before symlink resolution) are low-impact theoretical concerns that the PR explicitly acknowledges as acceptable trade-offs.

No files require special attention. The core logic in guards_cleanup.go and mount_guard_chattr_index.go is well-covered by the new test suite.

Important Files Changed

Filename	Overview
internal/orchestrator/mount_guard_chattr_index.go	New file: index record/read/write helpers with allowlist validation, control-char rejection, dedup, atomic write, and size cap. Uses os.OpenRoot for path confinement on the record-time read path.
internal/orchestrator/guards_cleanup.go	Adds clearImmutableGuards and three new injectable seams; correctly integrates the pending count with the existing bind-mount guard loop and removal gate. The isMounted check uses the unresolved target path (a known symlink edge-case, previously flagged).
internal/orchestrator/mount_guard_apply.go	One-line addition: recordImmutableGuardTarget called after successful chattr +i in the PBS fallback path, correctly placed after the early-return on failure.
internal/orchestrator/pve_staged_apply.go	Switches two os.MkdirAll / isPathOnRootFilesystem call sites to the injectable mountGuard* seams; adds recordImmutableGuardTarget call after successful chattr +i, correctly placed after the continue on failure.
internal/orchestrator/mount_guard_chattr_index_test.go	489-line test file covering dedup, injection-rejection, perm verification, parse tolerance, PBS/PVE wiring, and all clearImmutableGuards branches including dry-run, skip-mounted, chattr-failure, symlink-escape, pending-gate, and round-trip.
internal/orchestrator/mount_guard.go	mountGuardBaseDir promoted from const to var solely for test redirection; production behavior unchanged.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant R as Restore
    participant GA as Guard Apply (PBS/PVE)
    participant IDX as chattr-targets index
    participant CG as --cleanup-guards

    R->>GA: process offline storage mountpoint
    GA->>GA: attempt bind-mount guard
    alt bind-mount succeeds
        GA-->>R: bind-mount guard active
    else bind-mount fails → chattr fallback
        GA->>GA: chattr +i guardTarget
        GA->>IDX: recordImmutableGuardTarget(target)
        IDX->>IDX: atomic write (temp+rename, 0o600)
    end

    Note over R,IDX: Later, after storage is back online

    CG->>IDX: clearImmutableGuards: read index
    loop each recorded target
        CG->>CG: isConfirmableDatastoreMountRoot(target)?
        CG->>CG: isMounted(target)?
        alt currently mounted
            CG-->>CG: skip (pending++), keep index
        else not mounted
            CG->>CG: EvalSymlinks(target) → resolved
            CG->>CG: isConfirmableDatastoreMountRoot(resolved)?
            CG->>CG: chattr -i resolved
        end
    end
    alt "pending == 0 and no bind-mount guards"
        CG->>IDX: RemoveAll(mountGuardBaseDir)
    else "pending > 0"
        CG-->>CG: keep index for next run
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant R as Restore
    participant GA as Guard Apply (PBS/PVE)
    participant IDX as chattr-targets index
    participant CG as --cleanup-guards

    R->>GA: process offline storage mountpoint
    GA->>GA: attempt bind-mount guard
    alt bind-mount succeeds
        GA-->>R: bind-mount guard active
    else bind-mount fails → chattr fallback
        GA->>GA: chattr +i guardTarget
        GA->>IDX: recordImmutableGuardTarget(target)
        IDX->>IDX: atomic write (temp+rename, 0o600)
    end

    Note over R,IDX: Later, after storage is back online

    CG->>IDX: clearImmutableGuards: read index
    loop each recorded target
        CG->>CG: isConfirmableDatastoreMountRoot(target)?
        CG->>CG: isMounted(target)?
        alt currently mounted
            CG-->>CG: skip (pending++), keep index
        else not mounted
            CG->>CG: EvalSymlinks(target) → resolved
            CG->>CG: isConfirmableDatastoreMountRoot(resolved)?
            CG->>CG: chattr -i resolved
        end
    end
    alt "pending == 0 and no bind-mount guards"
        CG->>IDX: RemoveAll(mountGuardBaseDir)
    else "pending > 0"
        CG-->>CG: keep index for next run
    end

Loading

Comments Outside Diff (2)

1. internal/orchestrator/guards_cleanup.go, line 287-320 (link)

   Resolved-path mount status not re-checked after symlink resolution

   The isMounted(target) guard at line 261 checks the original (possibly symlink) path. After cleanupResolveTarget returns a different resolved path, the code runs chattr -i resolved without verifying that resolved is also not currently mounted. On Linux, mount follows symlinks: if /mnt/pve/store is a symlink to /mnt/actual-store, mounting storage there records /mnt/actual-store in mountinfo, so isMounted("/mnt/pve/store") returns false even though the storage is live. The subsequent chattr -i /mnt/actual-store would then act on the live mount's top-level inode rather than the shadowed underlying directory — exactly the scenario isMounted was added to prevent.

   Proxmox itself does not create symlinks for storage mountpoints, so this is not triggered in normal usage, but the defensive-programming rationale used elsewhere in this function applies equally here. Adding an isMounted(resolved) check (with the same skip-and-pending logic) would close the gap.

   

2. internal/orchestrator/mount_guard_chattr_index.go, line 497-517 (link)

   Index durability: no fsync before rename

   writeImmutableGuardIndex writes to a temp file and atomically renames it, but never calls tmp.Sync(). If the kernel crashes or loses power between the rename completing and the OS flushing dirty pages, the renamed file can be empty or contain zeros — the guard directory would exist but the index would be unreadable, silently returning 0 targets on the next --cleanup-guards run. The PR documentation correctly calls this "best-effort", and the failure mode (manual chattr -i required) is the same as the pre-PR situation, so the trade-off is reasonable. Calling tmp.Sync() after the last Write and before Close would eliminate the gap at the cost of an extra fdatasync.

   

_{Reviews (2): Last reviewed commit: "fix(orchestrator): resolve gosec G301/G3..." | Re-trigger Greptile}

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad4020ae-2ec0-45be-8ad8-cef985d4d982

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cleanup-guards-clear-immutable

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 73.48485% with 35 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/orchestrator/mount_guard_chattr_index.go	73.23%	12 Missing and 7 partials ⚠️
internal/orchestrator/guards_cleanup.go	75.00%	13 Missing and 1 partial ⚠️
internal/orchestrator/pve_staged_apply.go	50.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!