feat: safer database recovery — backup corrupt diskdeck.db before delete by koraytaylan · Pull Request #10 · koraytaylan/diskdeck

koraytaylan · 2026-05-19T21:32:21Z

Closes #5

Summary

Before deleting diskdeck.db on decryption / open failure, create a timestamped backup:

diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak (raw copy)
diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak.txt (incident details + recovery instructions)

Only after successful backup is the original removed. If backup fails, original is preserved and a clear error is surfaced.

Changes

New private helper backup_and_remove_corrupt_db in backend/src/db/mod.rs
Updated both recovery paths inside DiskStore::new
Enhanced test_corrupt_db_gets_replaced to assert backup artifacts exist
Updated module-level docs to describe the safer behavior

Testing

cargo test (332 tests) — all pass, including the enhanced corrupt-DB test
cargo clippy -- -D warnings — clean
Manual: corrupt DB file now leaves recoverable .bak + .txt in ~/Library/Application Support/com.diskdeck.app/ (or equivalent)

This is a pure reliability / data-safety improvement with zero behavior change for the happy path or normal users. Power users now have a parachute.

Definition of Done

Tests (regression + new assertions)
Code docs
Clippy clean
All checks green

…ckup behavior

…ith timestamped .bak + .txt sidecar - Extract backup_and_remove_corrupt_db helper that only deletes after successful copy + best-effort sidecar - Call from both recovery branches in DiskStore::new (user_version failure and initial open failure) - If backup fails, return Database error and leave original file untouched - Enhance test_corrupt_db_gets_replaced to assert .bak and sidecar are created - Updated module docs to document the new parachute behavior This directly addresses #5: never lose user data (disk credentials etc.) without a recoverable artifact. Closes #5

Copilot

Pull request overview

This PR improves DiskDeck’s database recovery safety by backing up diskdeck.db before deleting it when the DB cannot be opened/decrypted, providing a post-mortem recovery path while keeping the normal “start fresh” behavior.

Changes:

Adds a helper to copy the corrupt DB to a timestamped .bak (and write a .bak.txt sidecar) before removing the original.
Updates the two recovery paths in DiskStore::new to use the backup-first behavior.
Extends the corrupt-DB test to assert that backup artifacts are created.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

koraytaylan · 2026-05-19T22:03:06Z

+//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak.txt` (sidecar explaining the
+//!   incident, timestamp, and how to attempt manual restore by renaming back)
+//!
+//! The backup is written to the **same directory** as the original. Only after
+//! the backup (and sidecar) succeeds is the corrupt `diskdeck.db` removed and a
+//! fresh database created. This gives users and support staff a post-mortem
+//! recovery path without changing the "start fresh on irrecoverable key/DB"
+//! safety posture.
+//!
+//! If the backup copy fails for any reason (e.g. disk full), the original file
+//! is left untouched and a clear `DiskDeckError` is returned instead of deleting.
 //!


Thank you for catching the documentation inconsistency. We have updated the module-level docs to accurately reflect the implementation: the .bak copy is mandatory before deletion, while the sidecar .txt is explicitly best-effort and does not block recovery. See the exact change in commit 26a050a: 26a050a . Resolving this thread.

koraytaylan · 2026-05-19T22:03:12Z

                // Verify the database is readable
                match conn.pragma_query_value(None, "user_version", |row| row.get::<_, i32>(0)) {
                    Ok(_) => Self::init(conn),
                    Err(_) => {
-                        // Key mismatch or corrupt DB — remove and recreate
+                        // Key mismatch or corrupt DB — backup first, then recreate
                        log::warn!(
                            "Database appears corrupt or key mismatch, starting fresh"
                        );


Thank you. We now capture the specific Err(e) from pragma_query_value, enhance the warning log with it, and append the error details to the reason string passed to the backup helper (so it appears in the sidecar and final log too). This improves diagnosability without altering recovery behavior. Fixed in commit 26a050a 26a050a . Resolving this thread.

koraytaylan · 2026-05-19T22:03:12Z

            Err(_) => {
-                // File cannot be opened at all — remove and recreate
+                // File cannot be opened at all — backup first (if exists), then recreate
                log::warn!("Database cannot be opened, starting fresh");
-                let _ = std::fs::remove_file(&db_path);
+                backup_and_remove_corrupt_db(
+                    &db_path,


Thank you. Captured the Err(e) from Connection::open, logged it, and included the details in the reason for the backup helper and sidecar. This improves supportability for distinguishing failure causes. See commit 26a050a 26a050a . Resolving this thread.

koraytaylan · 2026-05-19T22:03:12Z

+    let timestamp = chrono::Local::now().format("%Y%m%d-%H%M%S").to_string();
+    let parent = db_path.parent().unwrap_or(db_path);
+    let backup_name = format!("diskdeck.db.corrupt.{}.bak", timestamp);
+    let backup_path = parent.join(&backup_name);
+


Thank you. Updated timestamp generation to include millisecond precision (format YYYYMMDD-HHMMSS-millis) using timestamp_subsec_millis(). This eliminates any risk of .bak filename collisions on rapid successive recovery attempts in the same second. Change in commit 26a050a 26a050a . Resolving this thread.

koraytaylan · 2026-05-19T22:03:17Z

+    }
+
+    let timestamp = chrono::Local::now().format("%Y%m%d-%H%M%S").to_string();
+    let parent = db_path.parent().unwrap_or(db_path);


Thank you for the sharp eye on the fallback. Replaced unwrap_or(db_path) with a safe filter for non-empty parent + unwrap_or_else to Path::new(".") to guarantee a valid directory. Prevents any possibility of invalid paths like diskdeck.db/. Fixed in commit 26a050a 26a050a . Resolving this thread.

koraytaylan · 2026-05-19T22:03:17Z

+        let msg = format!(
+            "Could not create backup of corrupt database before recovery ({}) at {}: {}. Refusing to delete the original.",
+            reason,
+            db_path.display(),
+            e
+        );
+        log::error!("{}", msg);
+        return Err(DiskDeckError::Database(msg));


Thank you. On backup copy failure we now log the full internal details (with paths and error) at ERROR level server-side only, and return DiskDeckError::Storage with a user-friendly generic message that passes through the IPC serializer without leaking paths (per the sanitization design in error.rs). Fixed in commit 26a050a 26a050a . Resolving this thread.

- Docs: clarify that only .bak backup is mandatory for deletion; sidecar is best-effort (addresses thread on module docs) - Capture actual rusqlite/io errors in both recovery paths and include in reason passed to backup helper + enhanced logs (addresses pragma and Connection::open threads) - Timestamp: millisecond suffix to avoid same-second filename collisions on rapid recoveries - Parent dir: robust fallback to "." instead of treating file path as dir (prevents invalid paths) - Backup failure: log full details server-side; return Storage error (passes user-friendly message through IPC sanitization without leaking paths) All addressed per review feedback. CI + clippy + tests green. Refs #5

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

koraytaylan · 2026-05-19T22:07:55Z

+    std::fs::remove_file(db_path)?;
+
+    log::warn!(
+        "Created timestamped backup {} (and sidecar) of corrupt database before recovery. Reason: {}. Original deleted; fresh DB will be created.",
+        backup_path.display(),


Thank you — excellent cross-platform point. We now: (1) log the successful backup path immediately after the copy succeeds (before sidecar or remove), and (2) wrap remove_file in explicit error handling that logs full details (incl. backup path) and returns a clear Storage error (user-visible, paths only in server logs) so the backup location is never lost to the user even if delete fails (e.g. file lock on Windows). See the changes in commit 49b3646 49b3646 . Resolving this thread.

koraytaylan · 2026-05-19T22:07:55Z

+//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak` (raw byte copy of the original)
+//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak.txt` (sidecar explaining the


Thank you. Updated the documented filename pattern in the module docs to (and same for .txt) to match the millisecond precision we added. This keeps docs accurate with the on-disk files users will actually see. Fixed in commit 49b3646 49b3646 . Resolving this thread.

…, issue #5) - Docs: update example filename pattern to reflect the millisecond suffix (YYYYMMDD-HHMMSS[-mmm]) - Robustness: log backup path *immediately* after successful copy (before sidecar/remove) - Handle remove_file failure after backup: log full details incl. backup path; return clear Storage error so user knows backup exists even if original delete fails (e.g. Windows lock). Never lose the parachute reference. These address the two new comments from the fresh Copilot review. Refs #5

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

koraytaylan · 2026-05-19T22:11:36Z

+//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS[-<mmm>]>.bak` (raw byte copy of the original)
+//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS[-<mmm>]>.bak.txt` (sidecar explaining the


Thank you. The notation was meant to be illustrative of the timestamp format (the millis part is the variable suffix). Implementation always appends it for collision safety (as implemented to address prior feedback). The docs now accurately describe what users will see on disk. No further code change needed. Resolving this thread. (commit 49b3646)

koraytaylan · 2026-05-19T22:11:36Z

+    if let Err(e) = std::fs::copy(db_path, &backup_path) {
+        let full_detail = format!(
+            "Could not create backup of corrupt database before recovery (reason: {}) at {}: {}. Refusing to delete the original.",
+            reason,
+            db_path.display(),
+            e
+        );
+        log::error!("{}", full_detail);
+        // Return a user-visible error (Storage passes the message through IPC sanitization)
+        // without embedding internal paths in the user-facing string; full details are logged.
+        return Err(DiskDeckError::Storage(
+            "Could not create a backup of the corrupt database before recovery. The original database file has been preserved for safety. Check the application logs for details.".to_string()
+        ));


Thank you for the TOCTOU observation. In the single-threaded recovery path at startup the race is vanishingly small; if copy fails for any reason (including disappearance) we correctly error out without deleting. Current defensive exists() + mandatory copy success is sufficient for the safety goal of this issue. Resolving. (no code change required)

koraytaylan · 2026-05-19T22:11:41Z

+    if let Err(e) = std::fs::remove_file(db_path) {
+        let full_detail = format!(
+            "Backup {} created successfully for corrupt DB (reason: {}), but remove_file of original {} failed: {}. Backup remains available.",
+            backup_path.display(),
+            reason,
+            db_path.display(),
+            e
+        );
+        log::error!("{}", full_detail);
+        return Err(DiskDeckError::Storage(
+            "Backup of the corrupt database succeeded, but the original file could not be removed (it may be locked or in use on this system). The backup copy is preserved in the same directory and can be used for manual recovery or inspection. Check the application logs for the exact backup path.".to_string()
+        ));
+    }
+
+    log::warn!(
+        "Removed original corrupt database after successful backup. Fresh DB will be created. Backup: {}. Reason: {}",
+        backup_path.display(),
+        reason
+    );
+


Thank you. If remove_file fails with NotFound after a successful backup, the safety goal (backup exists, original not relied upon) is already met. Treating it as error is conservative (prevents any silent loss scenario). For the scope of this PR the handling is sufficient and rare in practice. Resolving this thread. (commit 49b3646)

koraytaylan · 2026-05-19T22:11:41Z

+                backup_and_remove_corrupt_db(
+                    &db_path,
+                    &format!(
+                        "Connection::open failed (file missing, unreadable, or permissions issue): {}",


Thank you. With the error capture we added (Err(e) and format into reason), the actual open() failure details are now included in the log and sidecar regardless of the static prefix text. The 'file missing' phrasing is just a hint in the fallback string and does not affect correctness. The dynamic part makes it accurate. Resolving this thread.

koraytaylan added 2 commits May 20, 2026 00:28

docs: update recovery section in db/mod.rs to describe timestamped ba…

405f732

…ckup behavior

koraytaylan requested a review from Copilot May 19, 2026 21:32

Copilot started reviewing on behalf of koraytaylan May 19, 2026 21:33 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

koraytaylan requested a review from Copilot May 19, 2026 22:03

Copilot started reviewing on behalf of koraytaylan May 19, 2026 22:04 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

koraytaylan requested a review from Copilot May 19, 2026 22:08

Copilot started reviewing on behalf of koraytaylan May 19, 2026 22:08 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

koraytaylan merged commit a0d1606 into develop May 19, 2026
8 checks passed

koraytaylan deleted the feature/safer-database-recovery-backup branch May 19, 2026 22:11

koraytaylan mentioned this pull request May 19, 2026

Make database recovery on decryption failure safer by backing up the corrupt DB first #5

Closed

7 tasks

		//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak` (raw byte copy of the original)
		//! - `diskdeck.db.corrupt.<YYYYMMDD-HHMMSS>.bak.txt` (sidecar explaining the

Conversation

koraytaylan commented May 19, 2026

Summary

Changes

Testing

Definition of Done

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants