[Detail Bug] Restore on NFS can delete restored file content when rollback reports failure

[detail-app]

Summary

• Context: The write() function in src/restore_fs.rs atomically restores file content and handles race conditions where the target changes to a non-file between staging and exchange.
• Bug: v0.2.5 introduced a regression where the cleanup guard now deletes the user's restored file content on NFS when an undo exchange "fails but succeeds" on the server.
• Actual vs. expected: The restored content should be preserved when undo fails (v0.2.4 behavior), but v0.2.5 deletes it on NFS.
• Impact: Regression from safe to unsafe behavior on NFS. User's staged restore content is deleted, potentially requiring re-extraction from backup (if still retained).

Code with bug

if !metadata.is_file() {
    let undo_result = atomic_undo_exchange(&parent, &temp_name, &name);
    temp_is_staged_file =
        entry_is_file_with_identity(&parent, &temp_name, staged_identity); // <-- BUG 🔴 set before checking if undo failed
    if let Err(error) = undo_result {
        anyhow::bail!(
            "restore target '{}' changed to a non-file and Undo could not roll \
             back the replacement: {}. The target may already contain restored \
             content; inspect it and the displaced entry at '{}', if present, \
             before retrying",
            target.relative().display(),
            error,
            target.parent().join(&temp_name).display()
        );
    }
    anyhow::bail!(
        "restore target '{}' changed to a non-file before it could be updated",
        target.relative().display()
    );
}

Evidence

Scenario: NFS rename "fails but succeeds"

The Linux rename(2) man page explicitly warns:

BUGS: On NFS filesystems, you cannot assume that if the operation failed, the file was not renamed. If the server does the rename operation and then crashes, the retransmitted RPC which will be processed when the server is up again causes a failure.

This means on NFS, renameat2 with RENAME_EXCHANGE can return an error to the client while the exchange actually succeeded on the server.

Execution trace

Normal case (local filesystem):

1. Exchange succeeds: temp_name = directory, name = file with restored content
2. temp_is_staged_file = false
3. Undo exchange fails (local atomic failure): temp_name still = directory
4. entry_is_file_with_identity(temp_name) returns FALSE
5. Cleanup: err && temp_is_staged_file = TRUE && FALSE = skip
6. Result: name contains restored content (correct)

NFS case:

1. Exchange succeeds: temp_name = directory, name = file with restored content
2. temp_is_staged_file = false
3. Undo exchange appears to fail (EIO) but actually succeeds on server
4. Server state: temp_name = file with restored content, name = directory
5. entry_is_file_with_identity(temp_name) returns TRUE (queries server, finds staged file)
6. Cleanup: err && temp_is_staged_file = TRUE && TRUE = delete temp_name
7. Result: Restored content is deleted

Test verification

The existing test existing_write_reports_partial_mutation_when_non_file_rollback_fails passes because it simulates a local filesystem failure where the undo exchange truly fails (temp_name remains a directory). It does not test the NFS scenario where the undo exchange "fails but succeeds."

cargo test --bin undo existing_write_reports_partial_mutation_when_non_file_rollback_fails
# test passes, but only validates local filesystem behavior

This is a regression

The v0.2.5 change made this NFS edge case worse. The previous code was safe.

Old code (v0.2.4, lines 218-226):

if !metadata.is_file() {
    if atomic_exchange(&parent, &temp_name, &name).is_ok() {
        temp_contains_new_content =
            parent.symlink_metadata(&temp_name).is_ok_and(|metadata| {
                metadata.is_file()
                    && metadata.dev() == staged_identity.0
                    && metadata.ino() == staged_identity.1
            });
    }
    anyhow::bail!(...);
}

New code (v0.2.5, lines 225-238):

if !metadata.is_file() {
    let undo_result = atomic_undo_exchange(&parent, &temp_name, &name);
    temp_is_staged_file =
        entry_is_file_with_identity(&parent, &temp_name, staged_identity);
    if let Err(error) = undo_result {
        anyhow::bail!(...);
    }
    anyhow::bail!(...);
}

Behavior comparison in NFS "failed but succeeded" scenario:

Code version	Undo returns	Flag set?	Cleanup runs?	User data preserved?
v0.2.4 (old)	Err	No (.is_ok() is false)	No	Yes
v0.2.5 (new)	Err	Yes (identity check runs)	Yes	No

The old code only set the cleanup flag inside if atomic_exchange().is_ok(). On NFS, when the undo "fails but succeeds," .is_ok() returns false, so the flag assignment is skipped and cleanup never runs. The user's restored content is preserved.

The new code unconditionally runs the identity check and sets the flag before checking if undo failed. On NFS, when the undo "fails but succeeds," the identity check finds the staged file at temp_name, sets the flag to true, and cleanup deletes it.

The v0.2.5 changelog claims: "Restore rollback failures now preserve uncertain or displaced entries and explicitly report when restored content may already be present at the target."

This claim is false on NFS. The new code deletes the restored content on NFS when rollback fails, while the old code preserved it.

Why has this bug gone undetected?

1. Rare trigger conditions: Requires (a) NFS filesystem, (b) race condition where target becomes non-file between staging and exchange, (c) NFS server/network failure during undo exchange.

2. No filesystem-specific testing: Tests simulate local atomic rename semantics, not NFS's non-atomic behavior.

3. Regression misdiagnosed as improvement: The v0.2.5 change was intended to improve error handling, but the unconditional identity check introduced this regression on NFS.

4. Apparent success: The error message correctly warns the user to inspect both entries, but the cleanup guard has already deleted one of them.

Recommended fix

Move the identity check inside the error handling so cleanup never fires when the undo result is uncertain:

if !metadata.is_file() {
    let undo_result = atomic_undo_exchange(&parent, &temp_name, &name);
    if let Err(error) = undo_result {
        temp_is_staged_file = false; // <-- FIX 🟢 state is uncertain, don't clean up
        anyhow::bail!(
            "restore target '{}' changed to a non-file and Undo could not roll \
             back the replacement: {}. The target may already contain restored \
             content; inspect it and the displaced entry at '{}', if present, \
             before retrying",
            target.relative().display(),
            error,
            target.parent().join(&temp_name).display()
        );
    }
    temp_is_staged_file =
        entry_is_file_with_identity(&parent, &temp_name, staged_identity);
    anyhow::bail!(
        "restore target '{}' changed to a non-file before it could be updated",
        target.relative().display()
    );
}

This ensures that when the undo fails, the cleanup guard remains false and no automatic deletion occurs. The user must manually inspect both entries as the error message instructs.

Addressing objections

"Undo doesn't officially support NFS"

The storage documentation (lines 68-71) states:

"Network and FUSE filesystems vary: server-side writes may not emit local notifications. Undo's startup/resume scan catches visible differences when it runs, but that is not a guarantee of real-time capture."

This warns about event capture, not restore operations. The documentation does not state that restore operations are unsupported or unsafe on NFS. Users with home directories on NFS (common in corporate/academic environments) will reasonably expect restore to work safely.

Furthermore, the v0.2.5 changelog does not mention NFS as an exclusion. The claimed improvement ("Restore rollback failures now preserve uncertain or displaced entries") is contradicted by this regression.

"The content exists in backups anyway"

Two problems with this argument:

1. Retention limits: The storage documentation (lines 35-36) states defaults of "seven days of history and a 1 GiB cap." Backups may have expired or been cleaned up by the time the user retries.

2. User expectation: When a restore operation reports "The target may already contain restored content," the user expects to find it there. The cleanup guard silently violates this expectation.

"This scenario is extremely rare"

The probability is low, but:

• NFS is common in corporate and academic environments
• Race conditions are hard to control or predict
• The regression makes a previously-safe code path unsafe
• The impact is deletion of user data without warning

A low-probability regression with data deletion is still a bug worth fixing, especially when the fix is simple and restores the previous safe behavior.

History

This bug was introduced in commit d8e9466 (@dantelex, 2026-07-30). The commit refactored the rollback logic to improve error reporting when a non-file replacement is detected and rollback fails. The bug slipped in because the temp_is_staged_file flag is now set unconditionally after calling atomic_undo_exchange, even before checking if the operation failed. In the previous v0.2.4 code, the flag was only set when atomic_exchange().is_ok(), which prevented cleanup from running when the undo exchange reported failure (even if it actually succeeded on NFS).