From 2a004529dd14d5de678dbb8d67077500acef4e11 Mon Sep 17 00:00:00 2001 From: LabDennis Date: Mon, 12 Oct 2026 13:13:35 +0200 Subject: [PATCH] M14: Preserve recovery snapshots without readable metadata (#1378) * M14: Preserve recovery snapshots without readable metadata Recovery writes the PDF before its metadata sidecar. A failed sidecar write or interrupted save could leave a usable snapshot that the next listing silently deleted. List PDF snapshots without deleting recovery files. When metadata is missing, unreadable, corrupt or refers to another key, retain the snapshot and recover it without an original path so saving requires a destination. Keep the existing password flow for encrypted snapshots. Add regression coverage for failed metadata writes, repeated listing, missing/corrupt/mismatched metadata and encrypted recovery. Update the M14 progress documentation. Validation: the failed-write regression fails before the fix; all 11 recovery tests pass after it. Formatting and the 31-crate WASM check pass. The full workspace and strict Clippy gates remain red; see the PR body for the failing tests and diagnostics. * Recovery: replace damaged metadata once its snapshot is recovered; one progress-log entry Recovering a snapshot whose sidecar was missing, unreadable or for another key now writes fresh metadata for it (recording whether it is encrypted), so the damaged file doesn't linger. clear() also removes sidecars whose PDF is gone; snapshots themselves are never removed by listing. The header and M14 milestone-row edits are replaced by a single progress-log entry. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: LabDennis <341377186+LabDennis@users.noreply.github.com> Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/ui-egui/src/recovery.rs | 81 ++++++++++++++++++------ crates/ui-egui/tests/recovery.rs | 103 +++++++++++++++++++++++++++++-- 3 files changed, 163 insertions(+), 22 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 2d58f5fb6..b152728d7 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -140,6 +140,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md ## Progress log +- **2026-10-12 (community, M14, recovery metadata):** A recovery snapshot whose metadata is missing, damaged or names another snapshot is no longer deleted when the folder is listed: it is offered for recovery without an original path (Save asks for a destination), and encrypted snapshots still ask for their password. Recovering one writes fresh metadata in place of the damaged file, and clearing the folder also removes metadata left without a snapshot. Before, a failed metadata write meant the next start deleted the finished snapshot. Synthetic regressions cover the failed write, missing, corrupt, mismatched and encrypted cases. Estimates unchanged. - **2026-10-12 (M7, Arabic ActualText):** Add text records each shaped line in typing order with `/ActualText`, so copy and search preserve mixed Arabic, Latin, digits and brackets. Edit text keeps the logical order supplied by `/ActualText` instead of reversing Arabic or Hebrew a second time. Adjacent visual glyph runs retain their own bidi recovery. Font-independent tests cover Arabic marks, numbers, Latin words, reopen and complete replacement; automation checks saved editor text and unchanged rendering. Font fallback and bidi typing remain separate gaps; estimates unchanged. - **2026-10-12 (M5, Fill & Sign text size):** The active Fill & Sign toolbar now chooses 4–300 pt for new text and dates; the in-place editor and saved annotation use that size, which persists in local settings. `fill_sign_add` accepts the same `font_size`. Feature and readiness estimates unchanged. - **2026-10-12 (community, M3, Greek search):** Case-insensitive Find treats normal, final and capital sigma (`σ`, `ς`, `Σ`) as the same letter, so `κόσμος` and `ΚΌΣΜΟΣ` find each other. Case-sensitive and whole-word options, glyph ranges and match limits retain their behavior. Synthetic text-search regressions cover both directions. The tab-menu regression selects the platform's Reveal label (Finder on macOS), restoring the macOS test without changing the menu. Estimates unchanged. diff --git a/crates/ui-egui/src/recovery.rs b/crates/ui-egui/src/recovery.rs index e6ae0193a..7a8b8d464 100644 --- a/crates/ui-egui/src/recovery.rs +++ b/crates/ui-egui/src/recovery.rs @@ -3,8 +3,8 @@ //! While documents have unsaved changes, their working file is written every //! `AUTOSAVE_SECS` to a recovery folder in the user's data directory. Each entry is //! `.pdf` plus `.json` (name, original path, time). Saving, discarding or closing a -//! document removes its entry, and a clean quit leaves the folder empty. If entries are found at -//! startup, the previous session ended unexpectedly and the app offers to recover them. +//! document removes its entry. At startup the app offers snapshots even when their metadata is +//! missing or damaged; those recover without an original path, so saving asks for a destination. //! //! Encrypted documents are stored encrypted (the working file is), so recovery never writes //! plaintext of a protected document to disk. The web build has no recovery store yet. @@ -76,41 +76,77 @@ impl RecoveryStore { let _ = std::fs::remove_file(self.dir.join(format!("{key}.pdf"))); } - /// Complete entries, newest first. Incomplete leftovers (bytes without metadata) are removed. + /// Snapshots, newest first. Listing never removes files; missing metadata uses the file name. pub fn list(&self) -> Vec { let Ok(dir) = std::fs::read_dir(&self.dir) else { return Vec::new() }; let mut out = Vec::new(); for entry in dir.flatten() { let path = entry.path(); - match path.extension().and_then(|e| e.to_str()) { - Some("json") => { - let meta = std::fs::read(&path).ok().and_then(|b| serde_json::from_slice::(&b).ok()); - match meta { - Some(m) if self.dir.join(format!("{}.pdf", m.key)).is_file() => out.push(m), - _ => { - let _ = std::fs::remove_file(&path); - } - } - } - Some("pdf") if !path.with_extension("json").is_file() => { - let _ = std::fs::remove_file(&path); - } - _ => {} + if path.extension().and_then(|e| e.to_str()) != Some("pdf") || !entry.file_type().is_ok_and(|t| t.is_file()) { + continue; } + let Some(key) = path.file_stem().and_then(|s| s.to_str()) else { continue }; + let meta = std::fs::read(path.with_extension("json")) + .and_then(|b| serde_json::from_slice::(&b).map_err(std::io::Error::other)) + .and_then(|m| if m.key == key { Ok(m) } else { Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "snapshot key mismatch")) }); + out.push(meta.unwrap_or_else(|e| { + log::warn!("recovering snapshot without metadata: {}: {e}", path.display()); + let saved_at = entry + .metadata() + .and_then(|m| m.modified()) + .ok() + .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .map_or(0, |d| d.as_secs()); + RecoveryMeta { + key: key.to_string(), + name: format!("{key}.pdf"), + path: None, + saved_at, + // This is only a display hint; opening the PDF still detects encryption. + encrypted: false, + } + })); } out.sort_by_key(|m| std::cmp::Reverse(m.saved_at)); out } + /// Rewrite `meta`'s sidecar when it is missing, unreadable or names another key (a snapshot + /// recovered without metadata), so damaged metadata doesn't stay behind. The PDF is untouched. + pub fn repair_meta(&self, meta: &RecoveryMeta) { + let path = self.dir.join(format!("{}.json", meta.key)); + let readable = std::fs::read(&path).ok().and_then(|b| serde_json::from_slice::(&b).ok()).is_some_and(|m| m.key == meta.key); + if readable { + return; + } + let written = serde_json::to_vec_pretty(meta) + .map_err(std::io::Error::other) + .and_then(|json| crate::editing::write_atomically(&path.to_string_lossy(), &json)); + if let Err(e) = written { + log::warn!("couldn't replace recovery metadata {}: {e}", path.display()); + } + } + pub fn read(&self, key: &str) -> std::io::Result> { std::fs::read(self.dir.join(format!("{key}.pdf"))) } - /// Remove every entry (clean quit, or "Discard all"). + /// Remove every entry (clean quit, or "Discard all"), and metadata left without a snapshot. pub fn clear(&self) { for m in self.list() { self.remove(&m.key); } + let Ok(dir) = std::fs::read_dir(&self.dir) else { return }; + for entry in dir.flatten() { + let path = entry.path(); + // Only sidecars whose PDF is gone: they hold no document content. + if path.extension().and_then(|e| e.to_str()) == Some("json") + && entry.file_type().is_ok_and(|t| t.is_file()) + && !path.with_extension("pdf").exists() + { + let _ = std::fs::remove_file(&path); + } + } } } @@ -191,6 +227,15 @@ impl PdfCraftApp { let Some(id) = self.views.last().map(|v| v.id) else { return }; self.session.mark_recovered(id, meta.path.clone()); self.recovery_keys.insert(id, meta.key.clone()); + // A snapshot recovered without readable metadata gets a fresh sidecar now, so the + // damaged one doesn't linger until the next autosave (or forever, if none comes). + if let Some(store) = &self.recovery { + let mut meta = meta.clone(); + if let Some(doc) = self.session.get(id) { + meta.encrypted = doc.info.encrypted; + } + store.repair_meta(&meta); + } } /// Delete recovery entries the user chose not to recover. diff --git a/crates/ui-egui/tests/recovery.rs b/crates/ui-egui/tests/recovery.rs index 8d25965ff..63761e16a 100644 --- a/crates/ui-egui/tests/recovery.rs +++ b/crates/ui-egui/tests/recovery.rs @@ -232,13 +232,108 @@ fn a_cancelled_recovery_password_does_not_attach_to_the_next_unlocked_document() } #[test] -fn incomplete_entries_are_ignored_and_cleaned_up() { +fn incomplete_entries_are_preserved_until_explicitly_discarded() { let s = store("partial"); std::fs::create_dir_all(s.dir()).unwrap(); - std::fs::write(s.dir().join("orphan.pdf"), b"%PDF-1.7").unwrap(); + let bytes = fixture(1); + std::fs::write(s.dir().join("orphan.pdf"), &bytes).unwrap(); std::fs::write(s.dir().join("broken.json"), b"{ not json").unwrap(); - assert!(s.list().is_empty()); - assert_eq!(files_in(&s), 0); + for _ in 0..2 { + let entries = s.list(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].key, "orphan"); + assert_eq!(entries[0].path, None); + assert_eq!(s.read("orphan").unwrap(), bytes); + assert_eq!(files_in(&s), 2, "listing must not remove any recovery files"); + } + s.clear(); + assert!(!s.dir().join("orphan.pdf").exists()); + assert_eq!(files_in(&s), 0, "metadata without a snapshot holds no work and is cleared too"); +} + +#[test] +fn a_failed_metadata_write_keeps_the_completed_snapshot_recoverable() { + let s = store("metadata-write-failure"); + let meta = pdfcraft_ui_egui::RecoveryMeta { + key: "snapshot".into(), + name: "work.pdf".into(), + path: Some("original.pdf".into()), + saved_at: 1, + encrypted: false, + }; + // A directory at the sidecar path makes its atomic rename fail after the PDF was written. + std::fs::create_dir_all(s.dir().join("snapshot.json")).unwrap(); + let bytes = fixture(2); + assert!(s.write(&meta, &bytes).is_err()); + assert_eq!(s.read("snapshot").unwrap(), bytes); + let entries = s.list(); + assert_eq!(s.read("snapshot").unwrap(), bytes, "a failed sidecar write must not lose the PDF"); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].key, "snapshot"); + assert_eq!(entries[0].path, None, "an unknown original path must require Save As"); +} + +#[test] +fn missing_corrupt_or_mismatched_metadata_recovers_the_snapshot_without_an_original_path() { + for case in ["missing", "corrupt", "mismatched"] { + let s = store(case); + crashed_session(&s, fixture(3), None, Some("original.pdf")); + let mut meta = s.list().remove(0); + let saved = s.read(&meta.key).unwrap(); + let sidecar = s.dir().join(format!("{}.json", meta.key)); + match case { + "missing" => std::fs::remove_file(&sidecar).unwrap(), + "corrupt" => std::fs::write(&sidecar, b"{ not json").unwrap(), + _ => { + meta.key = "other-snapshot".into(); + std::fs::write(&sidecar, serde_json::to_vec(&meta).unwrap()).unwrap(); + } + } + let mut h = harness(s.clone()); + h.get_by_label("Recover").click(); + h.run_steps(4); + let app = h.state(); + assert_eq!(app.views.len(), 1, "{case}"); + let doc = app.session.get(app.views[0].id).unwrap(); + assert!(doc.dirty); + assert_eq!(doc.path, None); + assert_eq!(doc.info.pages.len(), 2, "the edit survived missing metadata: {case}"); + let recovered = s.list(); + assert_eq!(recovered.len(), 1); + assert_eq!(s.read(&recovered[0].key).unwrap(), saved); + // The damaged sidecar was replaced with metadata for the recovered snapshot. + let sidecar: pdfcraft_ui_egui::RecoveryMeta = serde_json::from_slice(&std::fs::read(&sidecar).unwrap()).unwrap(); + assert_eq!((sidecar.key.as_str(), sidecar.path.as_deref()), (recovered[0].key.as_str(), None), "{case}"); + } +} + +#[test] +fn an_encrypted_snapshot_without_metadata_still_requires_its_password() { + let s = store("encrypted-orphan"); + crashed_session(&s, encrypted(fixture(2), "pw"), Some("pw"), Some("original.pdf")); + let meta = s.list().remove(0); + let saved = s.read(&meta.key).unwrap(); + std::fs::remove_file(s.dir().join(format!("{}.json", meta.key))).unwrap(); + let mut h = harness(s.clone()); + h.get_by_label("Recover").click(); + h.run_steps(3); + assert!(h.state().views.is_empty()); + let field = h.get_by_role(Role::PasswordInput); + field.focus(); + field.type_text("pw"); + h.run_steps(2); + h.key_press(Key::Enter); + h.run_steps(4); + let app = h.state(); + assert_eq!(app.views.len(), 1); + let doc = app.session.get(app.views[0].id).unwrap(); + assert!(doc.dirty); + assert_eq!(doc.path, None); + assert_eq!(doc.info.pages.len(), 1); + assert_eq!(s.read(&meta.key).unwrap(), saved, "recovery must not rewrite encrypted bytes"); + let sidecar: pdfcraft_ui_egui::RecoveryMeta = + serde_json::from_slice(&std::fs::read(s.dir().join(format!("{}.json", meta.key))).unwrap()).unwrap(); + assert!(sidecar.encrypted, "the replacement metadata records that the snapshot is encrypted"); } /// Through the control channel, the reply to a click comes after the click's effects: state read