From 8eeb9ba0f6112081a2b25ccffde1bdd0a2ad0361 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 30 Aug 2026 10:34:12 +0200 Subject: [PATCH 1/2] Offer the backup, and then the rebuild, when the index turns out to be damaged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NFR-R6 asks for an integrity check at startup and two offers behind it, and none of it existed. `PRAGMA integrity_check` appeared nowhere in the tree, `Catalog::open` was `open` → `configure` → `migrate` → `backfill` and nothing else, and corruption therefore surfaced as whatever rusqlite error the first unlucky query happened to produce — "database disk image is malformed" attached to a thumbnail refresh, elided into a 34px banner, over an empty grid saying "No images found · Check the library folder". Two messages that disagreed, and no way forward but deleting catalog.sqlite by hand. The property that makes the second offer real was already here and load- bearing: the catalog is an index, not a source of truth, rebuildable from sources plus sidecars (invariant §5.2.4, cited by schema.rs, trash.rs and lib.rs). And sync.rs already knew how to take a coherent snapshot of a WAL database. What was missing was the check, the type, and the conversation. Four pieces: **The type.** `CatalogError::Corrupt`, and — the part that makes it worth having — a hand-written `From` that classifies rather than wraps. `SQLITE_CORRUPT` and `SQLITE_NOTADB` become `Corrupt` wherever they arise, so a background job that trips over the damage first reports the same thing the startup check would have. `SQLITE_IOERR` and `SQLITE_BUSY` deliberately do not: a dropped network mount is a different problem, and telling someone to rebuild their index would be a wrong answer delivered confidently. **The check.** `Catalog::open_verified`, `quick_check` before the open rather than after, because opening runs migrations and a damaged catalog with an intact header would otherwise have structure rewritten on top of structure that is already wrong. Bound to `open_verified` and not to `open`: the check reads every page, which is affordable once at startup where a user can answer a question, and not affordable on the dozens of opens a session's background tasks make. **The backup.** NFR-R2's second clause, taken between `configure` and `migrate` in `Catalog::open`. A migration is the one routine operation that rewrites table structure, so it is the likeliest way this file becomes unreadable, and it is the last moment the pre-migration state exists to be copied. Three generations, through SQLite's backup API after a TRUNCATE checkpoint — never `fs::copy`, which on a WAL database backs up a state older than the catalog and possibly torn. A failure to take the copy is logged, not raised: a full disk must not be what makes a library unopenable. **The conversation.** The first line of the dialogue is that the photographs and the edits are safe, before the diagnosis, because that is the question the user is actually asking. Then the two offers, which are *not* interchangeable and are not presented as if they were: a restore keeps collections, and a rebuild cannot, because a manual collection is a set of images assembled by hand and nothing in the filesystem records it (docs/catalog.md §8.1). The labels say so, and the rebuild does not take the affirmative styling while a restore is on the table. One thing that is a fix rather than a feature: `show_catalog_now` now gates the scan. `Catalog::open` succeeds on a file whose header survived, so the scan that used to start immediately afterwards would write folder ETags and image rows into damaged pages in the seconds while the user was still reading the question — turning a file that had a backup into one where the backup is the only copy left. Restore also deletes the damaged catalog's `-wal` and `-shm`. That step is easy to leave out and fatal to leave out: a journal belonging to the old file, sitting beside the new one under the same name, is replayed into it on the next open. That is not a restore, it is a fresh corruption with the evidence gone. Tested by corrupting a fixture catalog — 500 images and a collection, then every page past the second overwritten — and driving both branches. The restore is asserted on the collection, because a collection is precisely what distinguishes the two paths; the rebuild on the damaged file being kept and the next open producing an empty catalog at the current schema. Plus the `SQLITE_NOTADB` presentation, a damaged backup being refused rather than installed, and a v1 catalog whose pre-migration backup comes back reading v1 rather than v11. Co-Authored-By: Claude Opus 5 (1M context) --- core/dr-catalog/src/error.rs | 62 ++- core/dr-catalog/src/lib.rs | 37 ++ core/dr-catalog/src/recovery.rs | 662 ++++++++++++++++++++++++++++++++ core/dr-catalog/src/sync.rs | 18 +- ui/dr-ui/src/lib.rs | 1 + ui/dr-ui/src/library_ui.rs | 53 ++- ui/dr-ui/src/recovery_ui.rs | 285 ++++++++++++++ ui/dr-ui/ui/app.slint | 43 +++ ui/dr-ui/ui/recovery.slint | 145 +++++++ 9 files changed, 1295 insertions(+), 11 deletions(-) create mode 100644 core/dr-catalog/src/recovery.rs create mode 100644 ui/dr-ui/src/recovery_ui.rs create mode 100644 ui/dr-ui/ui/recovery.slint diff --git a/core/dr-catalog/src/error.rs b/core/dr-catalog/src/error.rs index 0a8f0da..e88ec7f 100644 --- a/core/dr-catalog/src/error.rs +++ b/core/dr-catalog/src/error.rs @@ -1,14 +1,37 @@ -//! TRACES: NFR-ARCH-4 | NFR-R5 +//! TRACES: NFR-ARCH-4 | NFR-R5 | NFR-R6 //! Catalog errors. //! //! Typed and attached to the affected subject rather than panicking — a //! corrupt row or a failed job marks one image and lets the batch continue. +//! +//! # Why `From` is written by hand +//! +//! One class of SQLite failure is not about the statement that hit it: when +//! the file itself is damaged, *every* query fails, and which one the user +//! happened to trigger first says nothing. Before this, corruption reached the +//! interface as whatever `Sqlite(...)` the first failing query produced — +//! "database disk image is malformed" attached to a thumbnail refresh — and +//! there was nowhere to hang a recovery offer. +//! +//! So the conversion classifies rather than wraps: `SQLITE_CORRUPT` and +//! `SQLITE_NOTADB` become [`CatalogError::Corrupt`] wherever they arise, which +//! means a background job that trips over the damage reports the same thing +//! the startup check does (see [`crate::recovery`]). /// Something went wrong talking to the catalog. #[derive(Debug, thiserror::Error)] pub enum CatalogError { #[error("sqlite: {0}")] - Sqlite(#[from] rusqlite::Error), + Sqlite(#[source] rusqlite::Error), + + /// The catalog file is damaged. + /// + /// Its own variant because it is the one error with a *user-facing + /// remedy*: restore the NFR-R2 backup, or discard the index and rebuild it + /// from sources plus sidecars (NFR-R6, invariant §5.2.4). Every other + /// variant here is either a caller's mistake or a fact about one row. + #[error("the catalog file is damaged: {detail}")] + Corrupt { detail: String }, /// The catalog was written by a newer build. /// @@ -74,3 +97,38 @@ pub enum CatalogError { #[error("io: {0}")] Io(String), } + +impl From for CatalogError { + fn from(e: rusqlite::Error) -> Self { + if is_corruption(&e) { + // `to_string` rather than keeping the error: the detail is going + // into a dialog and into a log line, and the recovery path has no + // use for the rusqlite type once it knows the file is damaged. + CatalogError::Corrupt { + detail: e.to_string(), + } + } else { + CatalogError::Sqlite(e) + } + } +} + +/// Whether a SQLite failure means the *file* is damaged rather than the +/// statement wrong. +/// +/// `SQLITE_NOTADB` is included because it is what a truncated or overwritten +/// catalog produces — SQLite cannot read the header, so it declines to call it +/// a database at all. To a user those are the same accident, and the same two +/// offers answer both. +/// +/// Deliberately *not* included: `SQLITE_CANTOPEN` (a missing file, which +/// `Connection::open` fixes by creating one), `SQLITE_BUSY`, and +/// `SQLITE_IOERR` — a failing disk or a dropped network mount is a different +/// problem, and telling the user to rebuild their index would be a wrong +/// answer delivered confidently. +fn is_corruption(e: &rusqlite::Error) -> bool { + matches!( + e.sqlite_error_code(), + Some(rusqlite::ErrorCode::DatabaseCorrupt) | Some(rusqlite::ErrorCode::NotADatabase) + ) +} diff --git a/core/dr-catalog/src/lib.rs b/core/dr-catalog/src/lib.rs index 0ca953f..2cc9a86 100644 --- a/core/dr-catalog/src/lib.rs +++ b/core/dr-catalog/src/lib.rs @@ -21,6 +21,7 @@ //! - [`runner`] — the thing that drains it, driven by whoever owns the thread //! - [`trash`] — soft delete to a folder, then permanent delete //! - [`merge`] / [`sync`] — cross-device merging of collections and keywords +//! - [`recovery`] — backups, and the two offers made when this file is damaged //! //! # The one thing everything is designed around //! @@ -47,6 +48,7 @@ pub mod keywords; pub mod merge; pub mod query; pub mod rating; +pub mod recovery; pub mod runner; pub mod scan; pub mod schema; @@ -65,6 +67,7 @@ pub use keywords::{Coverage, Keyword, KeywordId, SelectionKeyword}; pub use merge::MergeReport; pub use query::{Query, Sort}; pub use rating::{Judgement, MAX_RATING}; +pub use recovery::Backup; // Not `runner::Budget`: `cache::Budget` already owns that name here and // means something else entirely (bytes on disk, not jobs in a slot). // Callers spell the work budget `runner::Budget`, where it is unambiguous. @@ -222,9 +225,23 @@ pub struct Catalog { impl Catalog { /// Open or create a catalog, migrating it forward if needed. + /// + /// Does **not** verify the file — see [`Self::open_verified`], and + /// [`recovery`] for why the check is bound to startup rather than to every + /// open. Damage this trips over on the way past is still reported as + /// [`CatalogError::Corrupt`] rather than as a stray SQLite error. pub fn open(path: &Path) -> Result { let conn = Connection::open(path)?; schema::configure(&conn)?; + // NFR-R2, and the reason it is *here*: a migration is the one routine + // operation that rewrites table structure, so it is the likeliest way + // this file becomes unreadable — and afterwards there is no + // pre-migration state left to copy. A failure to take the copy is + // logged rather than raised: a full disk must not be the thing that + // makes a library unopenable. + if let Err(e) = recovery::backup_before_migration(&conn, path) { + log::warn!("could not back up before migrating: {e}"); + } let from = schema::migrate(&conn)?; // A migration adds a column; it cannot know what the value should be // for rows that already existed. Backfilling on open is what stops @@ -235,6 +252,26 @@ impl Catalog { Ok(Catalog { conn }) } + /// TRACES: NFR-R6 + /// Open a catalog, checking the file first. + /// + /// What startup calls. On [`CatalogError::Corrupt`] the caller has a user + /// in front of it and must make the two offers [`recovery`] describes, + /// rather than reporting a SQLite message on a banner and carrying on into + /// a scan that would write into the damage. + /// + /// Checked *before* opening rather than after, because opening runs + /// migrations: a damaged catalog that happens to have an intact header + /// would otherwise be migrated — rewriting structure on top of structure + /// that is already wrong — before anybody asked whether it was sound. + pub fn open_verified(path: &Path) -> Result { + // A catalog that is not there yet is not damaged; `open` creates it. + if path.is_file() { + recovery::check_file(path)?; + } + Self::open(path) + } + /// An in-memory catalog, for tests and for a throwaway import preview. pub fn in_memory() -> Result { let conn = Connection::open_in_memory()?; diff --git a/core/dr-catalog/src/recovery.rs b/core/dr-catalog/src/recovery.rs new file mode 100644 index 0000000..cb1b185 --- /dev/null +++ b/core/dr-catalog/src/recovery.rs @@ -0,0 +1,662 @@ +//! TRACES: NFR-R2 | NFR-R6 +//! What to do once the index is already damaged. +//! +//! # Why this can be a small module +//! +//! Because of a property the rest of the catalog was built to keep: the +//! catalog is an *index*, not a source of truth (ARCH §6.12, invariant +//! §5.2.4). Sidecars beside the images hold the authoritative ratings, +//! keywords and edit graphs, for every catalogued image and whether or not a +//! remote account exists (FR-CAT-8). So the worst outcome available here is a +//! rescan — expensive, but not a loss. +//! +//! That is the second offer. The first is cheaper and loses nothing at all: a +//! backup, restored. +//! +//! # The one thing a rebuild does not recover +//! +//! **Collections.** A manual collection is a set of images the user assembled +//! by hand and nothing in the filesystem records it (`docs/catalog.md` §8.1) — +//! which is the whole reason the catalog file itself syncs. So the two offers +//! are not interchangeable, and the interface must not present them as if they +//! were: a restore keeps the user's collections, a rebuild does not. +//! +//! # When the check runs, and when it does not +//! +//! [`integrity_check`] reads every page of the database. That is affordable +//! once, at startup, where a failure has a user in front of it who can answer +//! a question — and it is *not* affordable on every [`Catalog::open`], which +//! this application does per background task, dozens of times a session. So +//! the check is bound to [`Catalog::open_verified`] rather than to `open`, +//! and the cheap half of the story — classifying `SQLITE_CORRUPT` and +//! `SQLITE_NOTADB` as [`CatalogError::Corrupt`] — happens for free on every +//! query through [`crate::error`]'s conversion. A background job that trips +//! over the damage first therefore reports the same thing the startup check +//! would have. +//! +//! [`Catalog::open`]: crate::Catalog::open +//! [`Catalog::open_verified`]: crate::Catalog::open_verified + +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use rusqlite::Connection; + +use crate::error::CatalogError; +use crate::schema; + +/// Directory backups live in, relative to the catalog file. +/// +/// Beside the catalog rather than in the cache directory, and that is the +/// point of the choice: this is the copy the user falls back on, and a cache +/// is a place the operating system is entitled to empty without asking +/// (see `library::data_root` for the same reasoning about sidecars). +const BACKUP_DIR: &str = "backups"; + +/// How many backups are kept. +/// +/// Small on purpose. A backup is a full copy of a catalog that is tens of +/// megabytes at 50k images, and the value of the third-oldest one is close to +/// zero: corruption is noticed at the next launch, not months later. What the +/// depth buys is protection against backing *up* the damage — if a corrupt +/// catalog is copied before anyone notices, the generation behind it is still +/// clean. +pub const KEEP_BACKUPS: usize = 3; + +/// Suffix given to a catalog that has been set aside as damaged. +/// +/// Kept rather than deleted. It costs disk this application would rather not +/// spend, and it is still the right call: `.sqlite` files have been recovered +/// by hand before, the user has not consented to a deletion, and NFR-R4's +/// instinct — never destroy what the user did not ask you to destroy — does +/// not stop applying at the catalog's edge. +const DAMAGED_SUFFIX: &str = "damaged"; + +/// One kept backup. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Backup { + pub path: PathBuf, + /// UTC seconds at which it was taken, read from the filename rather than + /// from the filesystem: a copy, a restore or a sync can rewrite an mtime, + /// and then the newest backup is not the one that looks newest. + pub taken_at: i64, + pub bytes: u64, +} + +/// Where backups for `catalog` are kept. +pub fn backup_dir(catalog: &Path) -> PathBuf { + catalog + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(BACKUP_DIR) +} + +/// Check the database this connection is attached to. +/// +/// `quick_check` rather than `integrity_check`: the difference is that +/// `quick_check` skips verifying that every index agrees with its table, which +/// is the expensive half and the half this application least needs — every +/// index here is derivable, and `REINDEX` fixes one without anybody being +/// asked a question. What is left still reads every page, and catches the +/// damage that matters: torn b-trees, a bad freelist, a truncated file. +/// +/// Returns [`CatalogError::Corrupt`] carrying what SQLite said, so the message +/// the user sees is the diagnosis rather than a paraphrase of it. +pub fn integrity_check(conn: &Connection) -> Result<(), CatalogError> { + // The argument caps how many problems are reported. One is enough: the + // answer is the same whether the file has one damaged page or nine + // hundred, and an unbounded check on a badly damaged file can run for a + // very long time producing a list nobody will read. + let mut stmt = conn.prepare("PRAGMA quick_check(1)")?; + let rows: Vec = stmt + .query_map([], |r| r.get(0))? + .collect::, _>>()?; + + // A healthy database answers with the single row "ok". + if rows.len() == 1 && rows[0] == "ok" { + return Ok(()); + } + Err(CatalogError::Corrupt { + detail: rows.join("; "), + }) +} + +/// Check a catalog file that is not currently open. +/// +/// Used before a restore: a backup is only worth swapping in if it is sound, +/// and swapping in a second damaged file — leaving the user with no catalog +/// and no offer left — is the failure this exists to prevent. +pub fn check_file(path: &Path) -> Result<(), CatalogError> { + if !path.is_file() { + return Err(CatalogError::Io(format!("{} is missing", path.display()))); + } + // Read-write rather than read-only, which reads oddly for a check. A + // backup carries the WAL journal mode in its header because it was copied + // page-for-page from a WAL database, and SQLite cannot open one read-only + // without a shared-memory file it is then not allowed to create. Nothing + // here writes; the connection is opened, read, and dropped. + let conn = Connection::open(path)?; + integrity_check(&conn) +} + +/// Take a backup of the open catalog. +/// +/// Returns the file written. Older generations beyond [`KEEP_BACKUPS`] are +/// pruned, newest kept. +/// +/// Goes through `crate::sync::copy_to` — SQLite's own backup API after a +/// TRUNCATE checkpoint — rather than copying the file. A WAL database is not +/// one file, and `fs::copy` of the main file alone would silently back up a +/// state that is older than the catalog and possibly torn, which is the one +/// failure mode a backup cannot afford. +pub fn backup(conn: &Connection, catalog: &Path) -> Result { + let dir = backup_dir(catalog); + std::fs::create_dir_all(&dir) + .map_err(|e| CatalogError::Io(format!("creating {}: {e}", dir.display())))?; + + let dest = dir.join(format!("catalog-{}.sqlite", now())); + // A second backup within the same second would otherwise land on the first + // one's name. Rare, and only reachable from tests and a retry, but the + // result would be a half-overwritten backup rather than two. + if dest.exists() { + std::fs::remove_file(&dest) + .map_err(|e| CatalogError::Io(format!("replacing {}: {e}", dest.display())))?; + } + + // Dropped immediately: the copy is complete when `copy_to` returns, and + // holding the connection open would leave a `-wal` beside a file whose + // whole purpose is to be a single self-contained artefact. + drop(crate::sync::copy_to(conn, &dest)?); + + prune(catalog); + Ok(dest) +} + +/// Back up before a migration, if there is anything to back up. +/// +/// Called from [`Catalog::open`](crate::Catalog::open) between `configure` and +/// `migrate`. NFR-R2 asks for this and the reasoning is narrower than "backups +/// are prudent": a migration is the one routine operation that rewrites table +/// structure, so it is the likeliest way a catalog becomes unreadable, and it +/// is the one moment where the pre-change state is still on disk to be copied. +/// Afterwards there is nothing left to take a copy *of*. +/// +/// A no-op in the two cases where it would cost without buying anything: a +/// catalog already at [`schema::SCHEMA_VERSION`], and a brand-new file at +/// version 0 with no tables in it yet. +pub fn backup_before_migration(conn: &Connection, catalog: &Path) -> Result<(), CatalogError> { + let from: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?; + if from == 0 || from >= schema::SCHEMA_VERSION { + return Ok(()); + } + let path = backup(conn, catalog)?; + log::info!( + "backed up catalog at v{from} to {} before migrating to v{}", + path.display(), + schema::SCHEMA_VERSION + ); + Ok(()) +} + +/// The backups available for `catalog`, newest first. +/// +/// Never fails: an unreadable or absent backup directory means there are no +/// backups, which is a fact about the offer to make rather than an error to +/// report on top of the corruption the user is already looking at. +pub fn backups(catalog: &Path) -> Vec { + let dir = backup_dir(catalog); + let Ok(entries) = std::fs::read_dir(&dir) else { + return Vec::new(); + }; + + let mut out: Vec = entries + .flatten() + .filter_map(|e| { + let path = e.path(); + let taken_at = timestamp_of(&path)?; + let bytes = e.metadata().ok()?.len(); + Some(Backup { + path, + taken_at, + bytes, + }) + }) + .collect(); + out.sort_by_key(|b| std::cmp::Reverse(b.taken_at)); + out +} + +/// Put a backup back in place of the damaged catalog. +/// +/// **Every connection to `catalog` must be closed first.** This replaces the +/// file underneath anything still holding it open, which on a live connection +/// is how a *second* corrupt catalog gets made. +/// +/// The order is deliberate: +/// +/// 1. The backup is checked. A restore that installs a second damaged file +/// leaves the user with nothing to try next. +/// 2. The damaged catalog is renamed aside, and its `-wal` and `-shm` are +/// **deleted**. This is the step that is easy to leave out and fatal to +/// leave out: a journal belonging to the old file, sitting beside the new +/// one under the same name, is replayed into it on the next open. That is +/// not a restore, it is a fresh corruption with the evidence gone. +/// 3. The backup is *copied* into place, not moved, so a failure here can be +/// retried against the same backup. +pub fn restore(catalog: &Path, backup: &Path) -> Result<(), CatalogError> { + check_file(backup)?; + set_aside(catalog)?; + std::fs::copy(backup, catalog).map_err(|e| { + CatalogError::Io(format!( + "restoring {} from {}: {e}", + catalog.display(), + backup.display() + )) + })?; + log::info!( + "restored {} from backup {}", + catalog.display(), + backup.display() + ); + Ok(()) +} + +/// Move a damaged catalog out of the way so the next open builds a fresh one. +/// +/// This is the rebuild path (NFR-R6's second offer) and also the first half of +/// a [`restore`]. Nothing else is needed to rebuild: the next +/// [`Catalog::open`](crate::Catalog::open) creates an empty catalog at the +/// current schema, and the ordinary scan repopulates it from sources and +/// sidecars — which is precisely invariant §5.2.4 being spent rather than +/// merely asserted. +/// +/// Returns where the damaged file was put, or `None` if there was no catalog +/// to move — a caller may be recovering from a file SQLite could not open +/// because it was never created. +pub fn set_aside(catalog: &Path) -> Result, CatalogError> { + let moved = if catalog.exists() { + let dest = with_suffix(catalog, DAMAGED_SUFFIX); + // An earlier damaged copy is replaced rather than accumulating: two of + // these is two full-size catalogs on the user's disk, and the older + // one has already been superseded by a recovery the user completed. + let _ = std::fs::remove_file(&dest); + // The rename first, so that a failure here leaves the journals with + // the file they belong to rather than orphaned beside a catalog that + // is still in use. + std::fs::rename(catalog, &dest).map_err(|e| { + CatalogError::Io(format!( + "setting aside {} as {}: {e}", + catalog.display(), + dest.display() + )) + })?; + log::warn!( + "catalog {} was damaged; kept as {}", + catalog.display(), + dest.display() + ); + Some(dest) + } else { + None + }; + + // Then the journals, whether or not there was a catalog to move: a `-wal` + // orphaned beside a missing database is replayed into whatever takes that + // name next, which would not be a restore but a fresh corruption with the + // evidence gone. + for sidecar in journals(catalog) { + if let Err(e) = std::fs::remove_file(&sidecar) { + if e.kind() != std::io::ErrorKind::NotFound { + return Err(CatalogError::Io(format!( + "removing stale journal {}: {e}", + sidecar.display() + ))); + } + } + } + + Ok(moved) +} + +/// Delete backups beyond [`KEEP_BACKUPS`]. +/// +/// Best-effort and silent about individual failures: failing to delete an old +/// backup is not a reason to fail the new one, which is already written. +fn prune(catalog: &Path) { + for old in backups(catalog).into_iter().skip(KEEP_BACKUPS) { + if let Err(e) = std::fs::remove_file(&old.path) { + log::warn!("could not prune backup {}: {e}", old.path.display()); + } + } +} + +/// The WAL and shared-memory files SQLite keeps beside a database. +fn journals(catalog: &Path) -> [PathBuf; 2] { + [with_suffix(catalog, "wal"), with_suffix(catalog, "shm")] +} + +/// `catalog.sqlite` plus `-suffix`, the way SQLite names its own sidecars. +/// +/// Appended to the whole filename rather than replacing the extension, so +/// `catalog.sqlite-wal` is what SQLite would look for and `catalog.sqlite- +/// damaged` sorts next to the catalog it came from. +fn with_suffix(catalog: &Path, suffix: &str) -> PathBuf { + let mut s = catalog.as_os_str().to_os_string(); + s.push("-"); + s.push(suffix); + PathBuf::from(s) +} + +/// Read the timestamp out of a backup's filename, or `None` if this is not one. +/// +/// Doubles as the filter that keeps [`backups`] from offering the user +/// something that is not a catalog — a stray file in the directory, or a `-wal` +/// left by a crash mid-backup. +fn timestamp_of(path: &Path) -> Option { + let name = path.file_name()?.to_str()?; + name.strip_prefix("catalog-")? + .strip_suffix(".sqlite")? + .parse() + .ok() +} + +/// Seconds since the epoch, or 0 if the clock is before it. +fn now() -> i64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::Catalog; + use std::io::{Seek, SeekFrom, Write}; + + /// A scratch directory that cleans up with the test. + fn tempdir(tag: &str) -> PathBuf { + let base = std::env::temp_dir().join(format!( + "dr-recovery-{tag}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + base + } + + /// A catalog on disk with enough rows to span several pages, closed. + /// + /// Closed matters: WAL means the rows are in `catalog.sqlite-wal` until + /// something checkpoints, and a test that corrupted the main file while + /// the data was still in the journal would be corrupting empty space. + fn fixture(path: &Path, images: i64) { + let cat = Catalog::open(path).unwrap(); + let c = cat.connection(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'local', 'lib')", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO collections(uuid, name, kind, created, revision, modified) + VALUES ('u1', 'Iceland', 0, 0, 1, 1)", + [], + ) + .unwrap(); + for i in 1..=images { + c.execute( + "INSERT INTO images(id, root_id, source_ref, added_at) + VALUES (?1, 1, ?2, 0)", + rusqlite::params![i, format!("DCIM/IMG_{i:05}.CR3")], + ) + .unwrap(); + } + crate::sync::checkpoint(c).unwrap(); + drop(cat); + } + + /// Scribble over everything past the first two pages. + /// + /// Past them rather than over them so that page 1 — the header and the + /// schema — survives: this produces a file SQLite is willing to open and + /// then finds damaged, which is the case `quick_check` exists for. Wiping + /// the header instead produces `SQLITE_NOTADB` at the first pragma, which + /// is a different branch and has its own test. + fn corrupt(path: &Path) { + let mut f = std::fs::OpenOptions::new().write(true).open(path).unwrap(); + let len = f.metadata().unwrap().len(); + assert!( + len > 8192, + "fixture is only {len} bytes; corrupting past page 2 would be a no-op" + ); + let junk = vec![0x5a_u8; (len - 8192) as usize]; + f.seek(SeekFrom::Start(8192)).unwrap(); + f.write_all(&junk).unwrap(); + f.sync_all().unwrap(); + } + + #[test] + fn a_healthy_catalog_passes() { + let cat = Catalog::in_memory().unwrap(); + integrity_check(cat.connection()).unwrap(); + } + + #[test] + fn a_corrupt_catalog_is_reported_as_corrupt_not_as_sqlite() { + // The whole point of the variant: this used to arrive as whatever + // rusqlite error the first failing query produced, with nowhere to + // hang a recovery offer. + let dir = tempdir("detect"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 500); + corrupt(&path); + + assert!(matches!( + Catalog::open_verified(&path), + Err(CatalogError::Corrupt { .. }) + )); + } + + #[test] + fn a_file_that_is_not_a_database_is_also_corrupt() { + // A truncated or overwritten catalog never reaches `quick_check`: the + // first pragma fails with SQLITE_NOTADB. Same accident to the user, + // same two offers, so it must classify the same way. + let dir = tempdir("notadb"); + let path = dir.join("catalog.sqlite"); + std::fs::write(&path, b"this is not a catalog, it is a text file\n").unwrap(); + + assert!(matches!( + Catalog::open(&path), + Err(CatalogError::Corrupt { .. }) + )); + } + + #[test] + fn restoring_a_backup_recovers_the_collections_a_rebuild_would_lose() { + // The first NFR-R6 branch, asserted on the thing that distinguishes it + // from the second: a collection exists nowhere but the catalog, so it + // is the evidence that the *contents* came back and not merely a + // readable file (docs/catalog.md §8.1). + let dir = tempdir("restore"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 500); + + { + let cat = Catalog::open(&path).unwrap(); + backup(cat.connection(), &path).unwrap(); + } + corrupt(&path); + assert!(matches!( + Catalog::open_verified(&path), + Err(CatalogError::Corrupt { .. }) + )); + + let newest = backups(&path).into_iter().next().expect("a backup exists"); + restore(&path, &newest.path).unwrap(); + + let cat = Catalog::open_verified(&path).unwrap(); + let name: String = cat + .connection() + .query_row("SELECT name FROM collections", [], |r| r.get(0)) + .unwrap(); + assert_eq!(name, "Iceland"); + let images: i64 = cat + .connection() + .query_row("SELECT count(*) FROM images", [], |r| r.get(0)) + .unwrap(); + assert_eq!(images, 500); + } + + #[test] + fn a_damaged_backup_is_refused_rather_than_installed() { + let dir = tempdir("badbackup"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 500); + { + let cat = Catalog::open(&path).unwrap(); + backup(cat.connection(), &path).unwrap(); + } + let newest = backups(&path).into_iter().next().unwrap(); + corrupt(&newest.path); + corrupt(&path); + + assert!(matches!( + restore(&path, &newest.path), + Err(CatalogError::Corrupt { .. }) + )); + // And the damaged catalog is still where it was, so the second offer + // is still available. + assert!(path.exists()); + } + + #[test] + fn setting_aside_leaves_a_fresh_catalog_to_rebuild_into() { + // The second NFR-R6 branch. What makes it a rebuild rather than a data + // loss is invariant §5.2.4, which lives outside this crate — what is + // testable here is that the damaged file is out of the way, kept, and + // that the next open succeeds on an empty catalog at the current + // schema, which is what a scan then fills. + let dir = tempdir("rebuild"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 500); + corrupt(&path); + + let kept = set_aside(&path).unwrap().expect("the catalog was there"); + assert!(kept.exists(), "the damaged catalog was deleted, not kept"); + assert!(!path.exists()); + + let cat = Catalog::open_verified(&path).unwrap(); + let images: i64 = cat + .connection() + .query_row("SELECT count(*) FROM images", [], |r| r.get(0)) + .unwrap(); + assert_eq!(images, 0); + let v: i64 = cat + .connection() + .query_row("PRAGMA user_version", [], |r| r.get(0)) + .unwrap(); + assert_eq!(v, schema::SCHEMA_VERSION); + } + + #[test] + fn a_stale_journal_does_not_follow_the_catalog_into_recovery() { + // The step that is easy to omit: a `-wal` belonging to the damaged + // file is replayed into whatever takes its name next. + let dir = tempdir("journal"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 500); + std::fs::write(with_suffix(&path, "wal"), b"stale").unwrap(); + + set_aside(&path).unwrap(); + assert!(!with_suffix(&path, "wal").exists()); + } + + #[test] + fn a_migration_is_backed_up_before_it_runs() { + // NFR-R2's second clause, against a real v1 catalog rather than a + // faked version number: the point is not that *a* file appears but + // that it holds the state from before the migration, which is the only + // state that is any use if the migration is what breaks it. + let dir = tempdir("premigrate"); + let path = dir.join("catalog.sqlite"); + { + let c = Connection::open(&path).unwrap(); + schema::configure(&c).unwrap(); + // `v1_for_attached` names the schema it targets, and "main" is a + // schema like any other — so this is the real v1, without needing + // `V1` itself to become visible outside its module. + c.execute_batch(&schema::v1_for_attached("main")).unwrap(); + c.pragma_update(None, "user_version", 1).unwrap(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'local', 'lib')", + [], + ) + .unwrap(); + crate::sync::checkpoint(&c).unwrap(); + } + assert!(backups(&path).is_empty()); + + Catalog::open(&path).unwrap(); + + let taken = backups(&path); + assert_eq!(taken.len(), 1, "no backup was taken before the migration"); + check_file(&taken[0].path).unwrap(); + let kept = Connection::open(&taken[0].path).unwrap(); + let v: i64 = kept + .query_row("PRAGMA user_version", [], |r| r.get(0)) + .unwrap(); + assert_eq!(v, 1, "the backup was taken after the migration, not before"); + } + + #[test] + fn opening_an_up_to_date_catalog_takes_no_backup() { + // Or every background task that opens the catalog would copy it. + let dir = tempdir("nobackup"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 10); + Catalog::open(&path).unwrap(); + assert!(backups(&path).is_empty()); + } + + #[test] + fn only_the_newest_generations_are_kept() { + let dir = tempdir("prune"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 10); + let cat = Catalog::open(&path).unwrap(); + + // Written by hand rather than by calling `backup` in a loop: the + // filename carries whole seconds, so real calls would collide. + std::fs::create_dir_all(backup_dir(&path)).unwrap(); + for t in 1..=KEEP_BACKUPS as i64 + 2 { + drop( + crate::sync::copy_to( + cat.connection(), + &backup_dir(&path).join(format!("catalog-{t}.sqlite")), + ) + .unwrap(), + ); + } + prune(&path); + + let kept = backups(&path); + assert_eq!(kept.len(), KEEP_BACKUPS); + // Newest first, and the newest is the highest timestamp. + assert_eq!(kept[0].taken_at, KEEP_BACKUPS as i64 + 2); + } + + #[test] + fn a_stray_file_in_the_backup_directory_is_not_offered_as_one() { + let dir = tempdir("stray"); + let path = dir.join("catalog.sqlite"); + fixture(&path, 10); + std::fs::create_dir_all(backup_dir(&path)).unwrap(); + std::fs::write(backup_dir(&path).join("notes.txt"), b"hello").unwrap(); + std::fs::write(backup_dir(&path).join("catalog-7.sqlite-wal"), b"x").unwrap(); + + assert!(backups(&path).is_empty()); + } +} diff --git a/core/dr-catalog/src/sync.rs b/core/dr-catalog/src/sync.rs index 975d82a..67022b6 100644 --- a/core/dr-catalog/src/sync.rs +++ b/core/dr-catalog/src/sync.rs @@ -52,6 +52,21 @@ pub fn checkpoint(conn: &Connection) -> Result<(), CatalogError> { /// coherent even with writers active. Callers should still prefer a quiet /// moment — this competes with background jobs for the write lock. pub fn snapshot_for_upload(conn: &Connection, dest: &Path) -> Result<(), CatalogError> { + let out = copy_to(conn, dest)?; + strip_face_crops(&out)?; + Ok(()) +} + +/// Checkpoint, then copy the whole database to `dest`, and hand back the +/// connection to the copy. +/// +/// Split out from [`snapshot_for_upload`] because [`crate::recovery`] wants +/// exactly this and none of what follows it there: an NFR-R2 backup is the +/// file the user may have to *live on*, so it keeps the face crops that an +/// upload strips. Sharing the copy rather than reimplementing it is what keeps +/// the WAL discipline in one place — a backup taken with `fs::copy` would be +/// the torn snapshot this module's header exists to warn about. +pub(crate) fn copy_to(conn: &Connection, dest: &Path) -> Result { checkpoint(conn)?; let mut out = Connection::open(dest)?; @@ -63,8 +78,7 @@ pub fn snapshot_for_upload(conn: &Connection, dest: &Path) -> Result<(), Catalog backup.run_to_completion(i32::MAX, std::time::Duration::ZERO, None)?; drop(backup); - strip_face_crops(&out)?; - Ok(()) + Ok(out) } /// Drop the stored face crops from a snapshot before it is uploaded. diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index aebae15..1f6b908 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -49,6 +49,7 @@ mod net_runtime; mod peaking; mod preset_store; mod presets; +mod recovery_ui; mod remote; mod segmentation; mod settings_store; diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index e1a9b19..f1c9b72 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -862,7 +862,13 @@ pub fn open( // Before the scan, not after it: the grid can be filled from disk now and // the scan is only ever going to add to it. - show_catalog_now(window, &ctl, &path, &coll_ctl); + // + // And it is the gate on the scan, not merely a prelude to it — a damaged + // catalog has a question on screen, and a scan writing into it while that + // question is unanswered is how the last good copy gets destroyed. + if !show_catalog_now(window, &ctl, &path, &coll_ctl) { + return; + } let rx = library::spawn_scan( conn.clone(), @@ -1095,7 +1101,7 @@ fn drain_scan( /// the same operation: a scan is the only request that both proves the server /// is reachable and brings the catalog up to date. Keeping them one function /// is what stops "retry" from quietly becoming a weaker probe than "rescan". -fn start_rescan( +pub(crate) fn start_rescan( window: &AppWindow, ctl: &Rc, coll_ctl: &Rc, @@ -1916,23 +1922,38 @@ fn schedule_reload(window: &AppWindow, ctl: &Rc) { /// state already says "Scanning…", and an error here would contradict a scan /// that is working perfectly. `Catalog::open` creates the file in that case, so /// what the grid reads is an empty catalog rather than a failure. -fn show_catalog_now( +/// Returns whether it is safe to go on and scan. +/// +/// `false` means the catalog is damaged and the recovery question is up. The +/// caller must not start a scan on that answer: `Catalog::open` succeeds on a +/// file whose header survived, so the scan would write ETags and image rows +/// into damaged pages while the user is still reading the question — turning a +/// file that had a backup into one where the backup is the only copy left. +pub(crate) fn show_catalog_now( window: &AppWindow, ctl: &Rc, catalog_path: &std::path::Path, coll_ctl: &Rc, -) { +) -> bool { if ctl.catalog.borrow().is_some() { - return; + return true; } - let cat = match Catalog::open(catalog_path) { + // Verified rather than plain: this is the once-per-launch moment where a + // full check is affordable and there is a user in front of it who can + // answer the question. See `dr_catalog::recovery` for why it is not on + // every open. + let cat = match Catalog::open_verified(catalog_path) { Ok(cat) => cat, + Err(dr_catalog::CatalogError::Corrupt { detail }) => { + crate::recovery_ui::offer(window, catalog_path, &detail); + return false; + } Err(e) => { // Not surfaced: the scan is the thing that has to work, and it is // still running. If it fails too, it reports for both of them. log::info!("no catalog to show before the scan: {e}"); - return; + return true; } }; @@ -1964,6 +1985,19 @@ fn show_catalog_now( crate::collections_ui::refresh_tree(window, coll_ctl, &cat); *ctl.catalog.borrow_mut() = Some(cat); load_window(window, ctl); + true +} + +/// Drop the open catalog, so the next `show_catalog_now` opens the file +/// again rather than returning early. +/// +/// Only recovery needs this, and it needs it for a specific reason: the file +/// under that connection has been replaced. A handle to the catalog that was +/// there before is a handle to a file that no longer has a name, and every +/// read through it would return the damaged pages the recovery just moved out +/// of the way. +pub(crate) fn forget_catalog(ctl: &Rc) { + *ctl.catalog.borrow_mut() = None; } /// What one cell of the outgoing model is worth keeping. @@ -5642,6 +5676,11 @@ pub fn wire( start_rescan(&w, &ctl, &coll_ctl); }); } + + // Last, and in its own module: the answers to a damaged catalog have + // nothing to do with the library view except that they run before it + // exists. + crate::recovery_ui::wire(window, &ctl, &coll_ctl); } /// Reload the grid after the filter changed. diff --git a/ui/dr-ui/src/recovery_ui.rs b/ui/dr-ui/src/recovery_ui.rs new file mode 100644 index 0000000..f8037cc --- /dev/null +++ b/ui/dr-ui/src/recovery_ui.rs @@ -0,0 +1,285 @@ +//! The two offers made when the catalog turns out to be damaged. +//! +//! `dr_catalog::recovery` owns the mechanism — the integrity check, the +//! backups, the restore, setting the damaged file aside. This module owns the +//! *conversation*: what a user is told has happened, which of the two answers +//! are available, and what runs afterwards. +//! +//! # The thing that has to be said first +//! +//! **The photographs are fine, and so are the edits.** A user told that their +//! library database is corrupt will assume they have lost their work, because +//! in every other photo application they would have. Here they have not: +//! sources are read-only to this application (NFR-R4), and ratings, keywords +//! and edit graphs live in sidecars beside the images for every catalogued +//! photograph, whether or not an account exists (FR-CAT-8, invariant §5.2.4). +//! That sentence is the first line of the dialogue, before the diagnosis, +//! because it is the answer to the question the user is actually asking. +//! +//! # Why the two answers are not interchangeable +//! +//! A restore brings back **collections**; a rebuild cannot. Every other thing +//! the catalog holds has authoritative backing outside it, which is what makes +//! a rebuild survivable — but a manual collection is a set of images the user +//! assembled by hand and nothing in the filesystem records it +//! (`docs/catalog.md` §8.1). So the labels say which one loses them, and the +//! rebuild is not given the affirmative styling while a restore is on offer. +//! +//! # Why the scan is held back +//! +//! `library_ui::open` shows the catalog and then starts a scan. On a damaged +//! catalog the scan is actively harmful: a plain `Catalog::open` on a file +//! whose header is intact succeeds, and the scan would then write folder +//! ETags and image rows into damaged pages — turning a recoverable file into +//! one whose backup is the only copy left, and doing it in the seconds while +//! the user is still reading the question. So `show_catalog_now` reports +//! whether it is safe to continue, and this module restarts the scan itself +//! once the file underneath has been replaced. + +use std::cell::RefCell; +use std::path::{Path, PathBuf}; +use std::rc::Rc; + +use dr_catalog::recovery; +use slint::ComponentHandle; + +use crate::library_ui::LibraryController; +use crate::AppWindow; + +/// What the open question is about. +/// +/// A thread-local rather than a field on `LibraryController`, because the +/// question is asked *before* that controller has a catalog and is answered by +/// callbacks wired at startup. Thread-local is sound here for the same reason +/// [`crate::memory`]'s registry is: everything below runs on the Slint event +/// loop thread, which is the only thread that has an `AppWindow` to show it +/// on. +thread_local! { + static PENDING: RefCell> = const { RefCell::new(None) }; +} + +/// The damaged catalog and what can be done about it. +struct Pending { + catalog: PathBuf, + /// Newest first. Empty is the ordinary case on a young install and is not + /// an error — it removes one offer, not both. + backups: Vec, +} + +/// Ask what should happen to a damaged catalog. +/// +/// Called from `library_ui::show_catalog_now` when the startup integrity check +/// fails. `detail` is what SQLite said, carried through verbatim: a diagnosis +/// the user can quote into a bug report is worth more than a reassurance they +/// cannot check. +pub(crate) fn offer(window: &AppWindow, catalog: &Path, detail: &str) { + let backups = recovery::backups(catalog); + log::error!( + "catalog {} failed its integrity check: {detail} ({} backup(s) available)", + catalog.display(), + backups.len() + ); + + window.set_recovery_title("This library's index is damaged".into()); + window.set_recovery_detail( + // Two facts and their order matters: what is safe, then what is lost. + "Your photographs and your edits are safe — they are in the files \ + themselves and in the sidecars beside them. What is damaged is only \ + DarkRoom's index of them, which can be rebuilt." + .into(), + ); + window.set_recovery_diagnosis(detail.into()); + + match backups.first() { + Some(newest) => { + window.set_recovery_can_restore(true); + window.set_recovery_restore_label( + format!( + "Restore the backup from {} · keeps your collections", + describe_age(newest.taken_at) + ) + .into(), + ); + } + None => { + window.set_recovery_can_restore(false); + window.set_recovery_restore_label(slint::SharedString::new()); + } + } + + window.set_recovery_rebuild_label( + if backups.is_empty() { + // Nothing to compare it against, so the label states the cost + // rather than the difference. + "Rebuild from your photographs · rescans the library" + } else { + "Rebuild from your photographs · loses your collections" + } + .into(), + ); + window.set_recovery_busy(false); + // The scan was held back, so the "Scanning…" the grid is showing behind + // this would be a lie the moment the question is dismissed. + window.set_library_scanning(false); + + PENDING.with(|p| { + *p.borrow_mut() = Some(Pending { + catalog: catalog.to_path_buf(), + backups, + }) + }); +} + +/// Close the question without answering it. +/// +/// Leaves the banner set, because the library genuinely does not work and a +/// dialogue that vanishes leaving no trace of why nothing loads is worse than +/// no dialogue at all. +fn dismiss(window: &AppWindow) { + PENDING.with(|p| *p.borrow_mut() = None); + window.set_recovery_title(slint::SharedString::new()); + window.set_library_scanning(false); + window.set_library_error( + "The library index is damaged. Rescan to rebuild it, or restore a backup.".into(), + ); +} + +/// Install the three answers. +/// +/// Called at the end of `library_ui::wire`, which is where every other +/// window-level callback in this area is installed. +pub(crate) fn wire( + window: &AppWindow, + ctl: &Rc, + coll_ctl: &Rc, +) { + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + let coll = coll_ctl.clone(); + window.on_recovery_restore(move || { + let Some(w) = weak.upgrade() else { return }; + answer(&w, &ctl, &coll, Answer::Restore); + }); + } + + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + let coll = coll_ctl.clone(); + window.on_recovery_rebuild(move || { + let Some(w) = weak.upgrade() else { return }; + answer(&w, &ctl, &coll, Answer::Rebuild); + }); + } + + { + let weak = window.as_weak(); + window.on_recovery_dismiss(move || { + let Some(w) = weak.upgrade() else { return }; + dismiss(&w); + }); + } +} + +/// Which of the two the user chose. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Answer { + Restore, + Rebuild, +} + +/// Carry out an answer, then get the library going again. +/// +/// Both answers end the same way — the file under `catalog_path` is one this +/// build can open — so both continue into the same two steps: open the catalog +/// for the grid, and start a scan. A rebuild needs the scan to have anything +/// at all; a restore needs it because the backup is by definition older than +/// the library. +fn answer( + window: &AppWindow, + ctl: &Rc, + coll_ctl: &Rc, + which: Answer, +) { + let Some(pending) = PENDING.with(|p| p.borrow_mut().take()) else { + return; + }; + window.set_recovery_busy(true); + + // Before the file moves, not after. There is normally no open catalog here + // — `show_catalog_now` returned before storing one — but "normally" is not + // a guarantee worth resting a file rename on, and a connection to a file + // that has just been renamed out from under it reads the damaged pages + // forever. + crate::library_ui::forget_catalog(ctl); + + // Synchronous, on the UI thread, and that is a considered choice rather + // than an oversight: this is a file copy of a catalog — tens of megabytes + // at 50k images — at a moment when there is nothing else on screen to + // block, no scan running, and no frame worth keeping smooth. Moving it to + // a worker would buy a spinner and cost the guarantee that nothing else + // touches the file while it is being replaced. + let outcome = match which { + Answer::Restore => match pending.backups.first() { + Some(b) => recovery::restore(&pending.catalog, &b.path), + None => Ok(()), + }, + Answer::Rebuild => recovery::set_aside(&pending.catalog).map(|_| ()), + }; + + if let Err(e) = outcome { + // The question stays up: the *other* answer may still work, and a + // failed restore in particular leaves the rebuild untouched. + log::error!("recovery failed: {e}"); + window.set_recovery_busy(false); + window.set_recovery_diagnosis(format!("That did not work: {e}").into()); + PENDING.with(|p| *p.borrow_mut() = Some(pending)); + return; + } + + window.set_recovery_busy(false); + window.set_recovery_title(slint::SharedString::new()); + window.set_library_error(slint::SharedString::new()); + + if crate::library_ui::show_catalog_now(window, ctl, &pending.catalog, coll_ctl) { + crate::library_ui::start_rescan(window, ctl, coll_ctl); + } +} + +/// "today", "3 days ago" — enough to choose by, without a date library. +/// +/// The user is deciding how much work a restore costs them, and the answer to +/// that is an *age*, not a timestamp: "yesterday" is immediately actionable +/// and "1756512000" is not. Whole days, because an hour's precision would +/// invite a confidence the backup schedule does not earn. +fn describe_age(taken_at: i64) -> String { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0); + let days = (now - taken_at).max(0) / 86_400; + match days { + 0 => "today".to_string(), + 1 => "yesterday".to_string(), + d => format!("{d} days ago"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_age_reads_as_an_age() { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() as i64; + assert_eq!(describe_age(now), "today"); + assert_eq!(describe_age(now - 86_400), "yesterday"); + assert_eq!(describe_age(now - 5 * 86_400), "5 days ago"); + // A clock that has gone backwards must not produce "-2 days ago". + assert_eq!(describe_age(now + 86_400), "today"); + } +} diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 7d21dbb..a40deb1 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -11,6 +11,7 @@ import { GestureRow } from "gestures.slint"; import { Button, PanelHeading, Label, Value, Caption, Panel, EmptyState, ProgressBar, ActivityRow } from "widgets.slint"; import { CollectionsPanel, CollectionRow, OfflinePrompt } from "collections.slint"; import { HistogramPanel, HistogramView } from "histogram.slint"; +import { RecoveryPrompt } from "recovery.slint"; import { PresetSheet, ScopeChips, ScopeKind } from "presets.slint"; import { FocusMarks, FocusPanel } from "peaking.slint"; import { SettingsPage } from "settings.slint"; @@ -365,6 +366,21 @@ export component AppWindow inherits Window { callback offline-prompt-release(); callback offline-prompt-dismiss(); + // The question a damaged catalog asks. Same shape as the prompt above and + // for the same reason: an empty title is what closes it, and every word in + // it is composed in Rust, which is the only side that knows what SQLite + // said and which backups exist. + in property recovery-title: ""; + in property recovery-detail: ""; + in property recovery-diagnosis: ""; + in property recovery-restore-label: ""; + in property recovery-can-restore: false; + in property recovery-rebuild-label: ""; + in property recovery-busy: false; + callback recovery-restore(); + callback recovery-rebuild(); + callback recovery-dismiss(); + in property library-root-label: ""; in-out property <[TimelineBar]> library-timeline; in property library-timeline-label: ""; @@ -1145,6 +1161,14 @@ in property panel-visible: true; // would leave the library from behind an open question — the // view changing underneath a modal, which reads as the app // having lost its place. + // + // The recovery question is asked first because it is drawn + // over everything, the offline prompt included: Back must + // reach the thing the user can actually see. + if (root.recovery-title != "") { + root.recovery-dismiss(); + return accept; + } if (root.offline-prompt-title != "") { root.offline-prompt-dismiss(); return accept; @@ -2664,5 +2688,24 @@ in property panel-visible: true; release() => { root.offline-prompt-release(); } dismiss() => { root.offline-prompt-dismiss(); } } + + // Last, and therefore over everything including the settings page and + // the offline prompt. Not a preference about layering: this is asked + // before the grid exists, and nothing else in the window is about a + // library that can be read. + RecoveryPrompt { + width: 100%; + height: 100%; + title: root.recovery-title; + detail: root.recovery-detail; + diagnosis: root.recovery-diagnosis; + restore-label: root.recovery-restore-label; + can-restore: root.recovery-can-restore; + rebuild-label: root.recovery-rebuild-label; + busy: root.recovery-busy; + restore() => { root.recovery-restore(); } + rebuild() => { root.recovery-rebuild(); } + dismiss() => { root.recovery-dismiss(); } + } } } diff --git a/ui/dr-ui/ui/recovery.slint b/ui/dr-ui/ui/recovery.slint new file mode 100644 index 0000000..cc5372b --- /dev/null +++ b/ui/dr-ui/ui/recovery.slint @@ -0,0 +1,145 @@ +// The question asked when the catalog turns out to be damaged. +// +// # Why this is a modal, when almost nothing else here is +// +// The house rule in this interface is to put the consequence in the button's +// label rather than to raise a dialogue — "Export 40", "Empty trash · 128" — +// and a genuine modal is kept for the two cases where the answer commits +// gigabytes. This is the third case, and it earns it for a different reason: +// there is nothing behind it to interact with. The grid cannot be drawn, the +// scan must not run (it would write into the damage), and every control in the +// window is about a library that cannot be read. A banner over an empty grid +// would be a question the user could scroll away from and then wonder why +// nothing worked. +// +// # Why the backdrop does not dismiss it +// +// Every other overlay here closes on a tap outside, and this one deliberately +// does not. A stray tap that loses the two offers leaves the application in a +// state with no way forward and no obvious way back to the question. There is +// a "Leave it for now" button instead, which says what it does. +// +// # Why the destructive answer is not the primary one +// +// A restore keeps the user's collections; a rebuild cannot, because a manual +// collection is a set of images the user assembled by hand and nothing in the +// filesystem records it (docs/catalog.md §8.1). So the two answers are not +// interchangeable, the difference is stated in the button rather than in a +// second dialogue after it, and the rebuild is the plain button even when it +// is the only one available. + +import { Theme } from "theme.slint"; +import { Button } from "widgets.slint"; + +export component RecoveryPrompt inherits Rectangle { + /// What went wrong, in the user's terms. Empty closes the prompt — one + /// source for "is this open", rather than a bool that can disagree with + /// the words beside it. + in property title; + /// What is safe and what is not, which is the part that determines whether + /// the next minute is frightening. + in property detail; + /// What SQLite actually said, kept because a bug report needs it and + /// because a diagnosis the user can read is worth more than a reassurance + /// they cannot check. + in property diagnosis; + /// The restore offer, naming the backup's date. Empty when there is no + /// backup to restore from, which is the case a fresh install is in. + in property restore-label; + in property can-restore: false; + /// The rebuild offer, naming what it costs — a full rescan, and the + /// collections it cannot bring back. + in property rebuild-label; + /// Set while a restore or rebuild is running, so neither can be started + /// twice against the same file. + in property busy: false; + + callback restore(); + callback rebuild(); + callback dismiss(); + + visible: root.title != ""; + background: #000000E0; + + // Swallows everything that misses the card, and answers nothing. See the + // header: losing this by a stray tap leaves nowhere to go. + TouchArea { } + + Rectangle { + width: min(460px, parent.width - 2 * Theme.gap-lg); + height: min(card.preferred-height, parent.height - 2 * Theme.gap-lg); + x: (parent.width - self.width) / 2; + y: (parent.height - self.height) / 2; + background: Theme.surface; + border-radius: Theme.radius; + border-width: 1px; + border-color: Theme.rule; + + TouchArea { } + + card := VerticalLayout { + padding: Theme.gap-lg; + spacing: Theme.gap; + + Text { + text: "Recover library"; + color: Theme.ink-faint; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + + Text { + text: root.title; + color: Theme.ink; + font-size: Theme.text-lg; + font-weight: 600; + wrap: word-wrap; + } + + Text { + text: root.detail; + color: Theme.ink-dim; + font-size: Theme.text; + wrap: word-wrap; + } + + // Wrapped rather than elided: this is the one line a bug report + // needs verbatim, and a truncated SQLite message is no message. + Text { + text: root.diagnosis; + color: Theme.ink-faint; + font-size: Theme.text-sm; + wrap: word-wrap; + } + + Rectangle { height: 1px; background: Theme.rule; } + + // Stacked, not a row: each label carries what its answer costs — + // a date, a count of photographs — and three of those side by side + // elide away exactly the part that lets the user choose. + if root.can-restore: Button { + text: root.busy ? "Working…" : root.restore-label; + primary: true; + enabled: !root.busy; + clicked => { root.restore(); } + } + + Button { + text: root.busy ? "Working…" : root.rebuild-label; + // Primary only when it is the only answer there is. A rebuild + // discards collections, so it does not get the emphasis while + // a restore that keeps them is on the table. + primary: !root.can-restore; + enabled: !root.busy; + clicked => { root.rebuild(); } + } + + Button { + text: "Leave it for now"; + enabled: !root.busy; + clicked => { root.dismiss(); } + } + } + } +} From 35954dfa1ddeaa97242c705921d533f5eeda3085 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 30 Aug 2026 10:34:12 +0200 Subject: [PATCH 2/2] Write a panic down where it can still be read, with nothing in it that names the user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NFR-OPS-2 is two sentences — local crash capture always, upload only on explicit opt-in — and what existed was one `log::error!` in the Android entry point and nothing at all on desktop. So a panic on desktop went to stderr and died with the terminal, and a panic on Android went to a logcat ring buffer that is gone long before anyone reports anything. What the user saw either way was a job that stopped or a control that went dead, with nothing to send. That matters more here than it would in most applications, because NFR-ARCH-4 says no worker error may panic the process and the code is written that way: errors are typed and attached to the image or job they belong to. A panic is therefore by construction a bug — an invariant this codebase believed and got wrong — and it was the one class of failure with no trace. `dr_plat::crash` writes a record to the XDG state directory: version, time, os, arch, thread, panic location, message, backtrace. In dr-plat rather than in either entry point because "where does this platform let an application keep state" is a platform question, and Android's answer is neither XDG nor `temp_dir` — `set_state_dir` takes it from `internal_data_path`, the same place `dr_sync::account::set_data_dir` gets its answer. The hook resolves the directory when it fires rather than when it is installed, which is what lets it go in before everything else and cover the startup it would otherwise miss. **The content rule is the substance of this, not the plumbing.** NFR-SEC-2 forbids credentials in logs and plain files; the same reasoning applies with more force to what this application is actually about, because a user's library is private and so is its shape. `/home/anna/Photos/2019 Divorce/` says something about a person, and a crash record is exactly the file someone attaches to a bug report while trying to be helpful. So `redact` runs over the message *and* the backtrace, and is deliberately blunt: anything containing a slash goes, `content://` and `primary:DCIM/...` included, since SAF names a library just as precisely as a path does; anything beside a word like `password` goes; a long opaque run with letters and digits in it goes, which is the shape of an app password nobody labelled. The one exception is a `.rs` path, which keeps its basename — a backtrace with no filenames is close to useless and `library.rs:1270` says nothing about anybody. Over-redaction costs legibility. Under-redaction costs a user something they cannot take back. Those are not comparable, so the boundary is not the place to be clever. NFR-SEC-5 — face data never in a crash report, under any configuration — is met structurally rather than by filtering: this module reads no catalog, opens no image, touches no account. A record is assembled from the panic hook's own arguments and `std::env::consts`, and there is no code path from here to an embedding. The message length cap is the backstop for a payload some other module formatted something large into. stderr is the one surface that still sees the message unredacted, deliberately: the previous hook is chained rather than replaced, so a developer watching a terminal does not lose the panic because the application started writing files. It is ephemeral, local, and never attached to a report. **No upload path, and not half of one** — no endpoint, no queue, no "send this later" flag. Opt-in upload needs a server to receive it and a consent flow stating what leaves the device (NFR-SEC-4, and the preview-and-consent step NFR-OPS-1 requires of the diagnostics bundle). Neither exists, and a transport built ahead of its consent is the shape of thing that later gets switched on by default. Leaves NFR-OPS-1 cheaper by three things it will want unchanged: `state_dir` (the log belongs at `state_dir()/log` beside `crash/`, so the diagnostics bundle has one directory to collect), `redact` (NFR-OPS-1's "automatic redaction of credentials and tokens" is this function), and `prune` (a size-capped rotation is this, counting bytes instead of files). Co-Authored-By: Claude Opus 5 (1M context) --- apps/darkroom-android/Cargo.toml | 4 + apps/darkroom-android/src/lib.rs | 19 +- apps/darkroom-desktop/Cargo.toml | 4 + apps/darkroom-desktop/src/main.rs | 8 + platform/dr-plat/src/crash.rs | 620 ++++++++++++++++++++++++++++++ platform/dr-plat/src/lib.rs | 7 + 6 files changed, 659 insertions(+), 3 deletions(-) create mode 100644 platform/dr-plat/src/crash.rs diff --git a/apps/darkroom-android/Cargo.toml b/apps/darkroom-android/Cargo.toml index 5699149..c51e112 100644 --- a/apps/darkroom-android/Cargo.toml +++ b/apps/darkroom-android/Cargo.toml @@ -19,6 +19,10 @@ dr-ui.workspace = true # For `account::set_data_dir`: only the platform entry point knows where Android # lets this app keep files, and it must be set before any store is opened. dr-sync.workspace = true +# For the panic hook, and for `crash::set_state_dir` — Android has no XDG +# directories, so the entry point is the only place that knows where a crash +# record may be written. +dr-plat.workspace = true # Directly, not just through dr-ui: `android_main` takes an `AndroidApp` and # calls `slint::android::init`, both of which come from this crate. The backend # feature comes from dr-ui's target-specific dependency. diff --git a/apps/darkroom-android/src/lib.rs b/apps/darkroom-android/src/lib.rs index f8c417f..5757a1f 100644 --- a/apps/darkroom-android/src/lib.rs +++ b/apps/darkroom-android/src/lib.rs @@ -39,9 +39,18 @@ fn android_main(app: slint::android::AndroidApp) { // worker thread that panics is invisible: the process survives, the // channel it was writing to closes, and the UI reports only that // something "failed unexpectedly" with no way to find out what. - std::panic::set_hook(Box::new(|info| { - log::error!("panic: {info}"); - })); + // + // This used to be one `log::error!` of the raw panic, which had two + // problems: logcat is a ring buffer that is gone by the time a user + // reports anything, and the raw message can carry a document URI naming + // their library or a credential a library interpolated into an error + // (NFR-SEC-2). `dr_plat::crash` writes a redacted record to disk and logs + // the redacted form. Nothing uploads it. + // + // Before `set_state_dir` on purpose: the hook resolves the directory when + // it fires, so installing it first covers the startup below rather than + // leaving it uncovered. + dr_plat::crash::install(env!("CARGO_PKG_VERSION")); log::info!("DarkRoom v{}", env!("CARGO_PKG_VERSION")); @@ -54,6 +63,10 @@ fn android_main(app: slint::android::AndroidApp) { match app.internal_data_path() { Some(dir) => { log::info!("data dir: {}", dir.display()); + // Crash records go beside the account data rather than under it: + // both are app-private and neither is a cache, which is the whole + // distinction that matters here (see `dr_plat::crash::state_dir`). + dr_plat::crash::set_state_dir(dir.join("state")); dr_sync::account::set_data_dir(dir); } None => log::error!("no internal data path; settings will not persist"), diff --git a/apps/darkroom-desktop/Cargo.toml b/apps/darkroom-desktop/Cargo.toml index 238b7f6..630f2f2 100644 --- a/apps/darkroom-desktop/Cargo.toml +++ b/apps/darkroom-desktop/Cargo.toml @@ -7,6 +7,10 @@ license.workspace = true [dependencies] dr-ui.workspace = true +# For the panic hook alone. Directly rather than through dr-ui, because it has +# to be installed before `dr_ui::run` — a panic during startup is exactly the +# one this exists to catch. +dr-plat.workspace = true anyhow.workspace = true env_logger.workspace = true log.workspace = true diff --git a/apps/darkroom-desktop/src/main.rs b/apps/darkroom-desktop/src/main.rs index 05713c8..149a5ff 100644 --- a/apps/darkroom-desktop/src/main.rs +++ b/apps/darkroom-desktop/src/main.rs @@ -10,6 +10,14 @@ fn main() -> anyhow::Result<()> { )) .init(); + // Immediately after the logger and before anything that could fail. Until + // now a panic on desktop went to stderr and died with the terminal, which + // means every panic a user has ever hit was unreportable: the process + // survives (the panicking worker does not), a control goes dead, and there + // is nothing on disk to say why. The record is local and stays local — + // there is no upload path, by design; see `dr_plat::crash`. + dr_plat::crash::install(env!("CARGO_PKG_VERSION")); + log::info!("DarkRoom v{}", env!("CARGO_PKG_VERSION")); let paths: Vec = std::env::args().skip(1).map(PathBuf::from).collect(); diff --git a/platform/dr-plat/src/crash.rs b/platform/dr-plat/src/crash.rs new file mode 100644 index 0000000..654ab3c --- /dev/null +++ b/platform/dr-plat/src/crash.rs @@ -0,0 +1,620 @@ +//! TRACES: NFR-OPS-2 | NFR-SEC-2 +//! Local crash capture. +//! +//! # What this is, and the half it deliberately is not +//! +//! NFR-OPS-2 is two sentences: *local crash capture always; upload only on +//! explicit opt-in.* Only the first is built here, and the second is not +//! half-built either — there is no upload path, no endpoint, no queue and no +//! "send this later" flag. Opt-in upload needs a server to receive it and a +//! consent flow that states what leaves the device (NFR-SEC-4, and the same +//! preview-and-consent step NFR-OPS-1 requires of the diagnostics bundle); +//! neither exists, and a transport built ahead of the consent is exactly the +//! shape of thing that later gets switched on by default. +//! +//! So a crash record is a file on the user's own disk. Nothing reads it but a +//! person. +//! +//! # Why a panic is worth writing down at all +//! +//! NFR-ARCH-4 says no worker error may panic the process, and the application +//! is built that way — errors are typed and attached to the image or job they +//! belong to. A panic is therefore, by construction, a *bug*: an invariant +//! this codebase believed and got wrong. Before this, one of those was +//! invisible on desktop (stderr, discarded with the terminal) and one line of +//! `log::error!` on Android. What the user saw was a job that stopped, or a +//! channel that closed and a control that went dead, with nothing to report. +//! +//! # What goes in a record, and what may never +//! +//! The rule the content is chosen under is NFR-SEC-2 — credentials never reach +//! logs or plain files — extended to the thing this application is actually +//! about: **a user's library is private, and its shape is private too.** A +//! path is not a neutral technical detail here. `/home/anna/Photos/2019 +//! Divorce/` names something about a person, and a crash record is a file that +//! gets attached to a bug report by someone trying to be helpful. +//! +//! Hence [`redact`], which is applied to the panic message *and* to the +//! backtrace before either is written, and which is deliberately blunt: it +//! removes anything that looks like a path or a URL, keeping only the basename +//! of `.rs` files so a backtrace is still readable. Over-redaction costs +//! legibility; under-redaction costs a user something they cannot take back. +//! +//! NFR-SEC-5 — face data never enters a diagnostics bundle or crash report +//! "under any configuration" — is met structurally rather than by filtering: +//! this module reads no catalog, opens no image, and touches no account. A +//! record is assembled from the panic hook's own arguments and from +//! [`std::env::consts`], and there is no code path from here to an embedding, +//! a crop, or a cluster. The message length cap is the backstop for the +//! remaining case — a panic payload that some *other* module formatted a large +//! value into. +//! +//! # What this leaves cheaper for NFR-OPS-1 +//! +//! The rotating on-disk log is unbuilt, and it wants three things that are +//! here: [`state_dir`] (the XDG state directory, resolved once, overridable +//! for Android where XDG does not exist), [`redact`] (NFR-OPS-1's "automatic +//! redaction of credentials and tokens" is the same function), and `prune` +//! (size-capped rotation is this counting files instead of bytes). A log +//! belongs at `state_dir().join("log")` beside `crash/`, and the diagnostics +//! bundle then has one directory to collect. + +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; +use std::time::{SystemTime, UNIX_EPOCH}; + +/// How many crash records are kept, newest first. +/// +/// Ten because the useful pattern in a crash record is usually a *repeat* — +/// the same panic three launches running is a far stronger report than one — +/// and because these are a few kilobytes each, so the cap exists to stop a +/// crash loop filling a disk rather than to save space. +pub const KEEP_RECORDS: usize = 10; + +/// The longest panic message written to a record. +/// +/// A backstop, not a redaction: [`redact`] handles what must not be written at +/// all. This bounds what a panic that formatted something enormous into its +/// message — a decoded buffer, a `Vec` of embeddings — can put on disk. +const MAX_MESSAGE: usize = 2000; + +/// Android's per-app directory, once the entry point has said what it is. +static STATE_DIR: OnceLock = OnceLock::new(); + +/// Declare the directory this application may keep state in. +/// +/// Only Android needs to call this, and it must call it before a crash rather +/// than before the hook is installed — [`install`] resolves the directory at +/// crash time precisely so that the hook can go in first, covering the startup +/// it would otherwise miss. Neither `XDG_STATE_HOME` nor `HOME` is set there, +/// and the fallback would resolve to a path the app cannot write. +/// +/// Later calls are ignored rather than racing, matching +/// `dr_sync::account::set_data_dir`, which the same entry point calls for the +/// same reason. +pub fn set_state_dir(dir: PathBuf) { + let _ = STATE_DIR.set(dir); +} + +/// Where this application keeps state that is neither configuration nor cache. +/// +/// `$XDG_STATE_HOME/darkroom`, falling back to `~/.local/state/darkroom`. +/// State rather than cache because a crash record must survive the sweep that +/// a cache directory exists to permit, and rather than config because it is +/// not something the user edits. +pub fn state_dir() -> PathBuf { + if let Some(d) = STATE_DIR.get() { + return d.clone(); + } + std::env::var_os("XDG_STATE_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| { + PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".local/state") + }) + .join("darkroom") +} + +/// Where crash records are written. +pub fn crash_dir() -> PathBuf { + state_dir().join("crash") +} + +/// Install the panic hook. +/// +/// Call once, as early as the entry point can — before the window, before any +/// store, before anything that could itself panic. The directory is resolved +/// lazily inside the hook, so installing this before +/// [`set_state_dir`] is correct rather than merely tolerated. +/// +/// The previously installed hook still runs afterwards. On desktop that is the +/// standard library's, which prints the panic to stderr, and a developer +/// watching a terminal should not lose that because the application started +/// writing files. stderr is the one surface that sees the message unredacted, +/// which is a considered exception: it is ephemeral, local, and never attached +/// to a bug report. +pub fn install(app_version: &str) { + let version = app_version.to_string(); + let previous = std::panic::take_hook(); + + std::panic::set_hook(Box::new(move |info| { + // A panic inside a panic hook aborts the process, which would replace + // a diagnosable crash with an undiagnosable one. Everything below is + // written to be infallible, and this is the admission that "written to + // be" is not the same as "is". + let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let record = compose(&version, info); + // Redacted, because this is a log file and NFR-SEC-2 governs it. + // The unredacted form goes to stderr below, through the hook this + // one chained onto. + log::error!("panic: {}", record.summary); + match write_record(&crash_dir(), &record) { + // The name, not the path: the log this line lands in is a + // sibling of the record, and printing the directory would put + // the user's home in a file they may hand to someone. + Ok(path) => log::error!( + "crash record written: {}", + path.file_name().unwrap_or_default().to_string_lossy() + ), + Err(e) => log::error!("could not write a crash record: {e}"), + } + })); + + previous(info); + })); +} + +/// The crash records on disk, newest first. +/// +/// For a future diagnostics bundle, and for a person looking for the file to +/// attach to a report. +pub fn records() -> Vec { + let Ok(entries) = std::fs::read_dir(crash_dir()) else { + return Vec::new(); + }; + let mut out: Vec<(i64, PathBuf)> = entries + .flatten() + .filter_map(|e| { + let path = e.path(); + Some((timestamp_of(&path)?, path)) + }) + .collect(); + out.sort_by_key(|(when, _)| std::cmp::Reverse(*when)); + out.into_iter().map(|(_, p)| p).collect() +} + +/// One crash, formatted. +struct Record { + /// The whole file. + body: String, + /// One redacted line, for the log. + summary: String, + when: i64, +} + +/// Build a record from what the panic hook was handed. +/// +/// Split from the writing so the *content* rules — what is included, what is +/// redacted, what is capped — are testable without a filesystem, and so a +/// future diagnostics bundle can reuse the same composition. +fn compose(version: &str, info: &std::panic::PanicHookInfo<'_>) -> Record { + let payload = info.payload(); + let message = payload + .downcast_ref::<&str>() + .copied() + .or_else(|| payload.downcast_ref::().map(|s| s.as_str())) + // A panic can carry any `Any`, and `panic_any` is used by some + // libraries. There is nothing to print, and saying so is better than + // an empty field that reads like a bug in this code. + .unwrap_or("(panic payload was not a string)"); + + let message = redact(&truncate(message, MAX_MESSAGE)); + let at = info + .location() + .map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column())) + // `location()` is a compile-time source path, not one of the user's, + // but it goes through the same redaction: a dependency built from a + // registry checkout carries the *builder's* home directory in it. + .map(|s| redact(&s)) + .unwrap_or_else(|| "unknown".to_string()); + let thread = std::thread::current() + .name() + .unwrap_or("unnamed") + .to_string(); + + let when = now(); + let summary = format!("{message} (at {at}, thread {thread})"); + + let body = format!( + "darkroom-crash 1\n\ + version: {version}\n\ + when: {when}\n\ + os: {}\n\ + arch: {}\n\ + thread: {thread}\n\ + at: {at}\n\ + message: {message}\n\ + backtrace:\n{}\n", + std::env::consts::OS, + std::env::consts::ARCH, + redact(&std::backtrace::Backtrace::force_capture().to_string()), + ); + + Record { + body, + summary, + when, + } +} + +/// Write one record into `dir`, then prune. +/// +/// Takes the directory rather than calling [`crash_dir`] so a test can drive +/// the real writing path without an environment variable and without a +/// `OnceLock` it cannot reset. +fn write_record(dir: &Path, record: &Record) -> std::io::Result { + std::fs::create_dir_all(dir)?; + // The pid distinguishes two processes crashing in the same second, which + // is not hypothetical: a background export and the app can be separate + // processes on desktop, and a crash loop retries fast. + let path = dir.join(format!("crash-{}-{}.txt", record.when, std::process::id())); + std::fs::write(&path, &record.body)?; + prune(dir, KEEP_RECORDS); + Ok(path) +} + +/// Delete all but the `keep` newest records in `dir`. +/// +/// Best-effort: failing to delete an old record is no reason to lose the new +/// one, which is already on disk. +fn prune(dir: &Path, keep: usize) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + let mut found: Vec<(i64, PathBuf)> = entries + .flatten() + .filter_map(|e| { + let path = e.path(); + Some((timestamp_of(&path)?, path)) + }) + .collect(); + found.sort_by_key(|(when, _)| std::cmp::Reverse(*when)); + for (_, path) in found.into_iter().skip(keep) { + let _ = std::fs::remove_file(path); + } +} + +/// Pull the timestamp back out of a record's filename. +/// +/// Doubles as the filter that keeps anything else in the directory — an +/// editor's backup file, a log — from being counted as a crash record or +/// pruned as one. +fn timestamp_of(path: &Path) -> Option { + let name = path.file_name()?.to_str()?; + let rest = name.strip_prefix("crash-")?.strip_suffix(".txt")?; + rest.split('-').next()?.parse().ok() +} + +/// Remove from `text` everything that would say something about the user. +/// +/// # The rule, and why it is this blunt one +/// +/// A token containing `/` is treated as a path or a URL and removed. Rust +/// source files are the exception and keep their basename, because a backtrace +/// with no filenames is close to useless and `library.rs:1270` says nothing +/// about anybody. A token that reads like a secret — a long opaque run, or the +/// value beside a word like `password` — is removed regardless of shape. +/// +/// Blunter than a list of known-sensitive shapes on purpose. The cost of +/// over-redaction is a diagnostic that is harder to read; the cost of +/// under-redaction is a directory listing of somebody's photographs in a file +/// they may attach to a public bug report. Those are not comparable, and this +/// is not the place to be clever about the boundary. +/// +/// Known limits, stated rather than hidden: a path with no `/` in it (a bare +/// filename) survives, and a person's name that some *other* module formatted +/// into a panic message survives. Both are bounded by the fact that nothing +/// here reads the catalog — see the module documentation — and the second is +/// why `MAX_MESSAGE` exists. +pub fn redact(text: &str) -> String { + let mut out: Vec = Vec::new(); + let mut redact_next = false; + + for token in text.split_inclusive(char::is_whitespace) { + // Whitespace is preserved exactly — a backtrace is read as a shape as + // much as as text — so the classification runs on the token without + // its trailing space and the space is put back. + let trailing: String = token + .chars() + .skip_while(|c| !c.is_whitespace()) + .collect::(); + let word = &token[..token.len() - trailing.len()]; + + if word.is_empty() { + out.push(token.to_string()); + continue; + } + + let replacement = if redact_next { + Some("".to_string()) + } else { + classify(word) + }; + redact_next = names_a_secret(word); + + out.push(match replacement { + Some(r) => format!("{r}{trailing}"), + None => token.to_string(), + }); + } + + out.join("") +} + +/// What one token should be replaced with, or `None` to keep it. +fn classify(word: &str) -> Option { + // `key=value` and `key: value` written as one token. Split at the first + // separator so `Authorization:Bearer` loses the half that matters. + if let Some((head, tail)) = word.split_once(['=', ':']) { + if names_a_secret(head) && !tail.is_empty() { + return Some(format!("{head}=")); + } + } + + // Strip the punctuation a sentence puts around a path — "opening + // /home/x/y.CR3:" — so the classification sees the path itself, then put + // nothing back: the punctuation is not worth the complexity of restoring + // it around a placeholder. + let bare = word.trim_matches(|c: char| matches!(c, '"' | '\'' | '(' | ')' | ',' | ';' | ':')); + + if bare.contains('/') { + // A Rust source path keeps its basename. Line and column are part of + // the same token in a backtrace ("src/library.rs:1270:9"), and they + // are kept: they are facts about this codebase. + if let Some(rs) = rust_source_tail(bare) { + return Some(format!("…/{rs}")); + } + return Some("".to_string()); + } + if bare.starts_with('~') { + return Some("".to_string()); + } + if looks_opaque(bare) { + return Some("".to_string()); + } + None +} + +/// The `library.rs:1270:9` tail of a path naming a Rust source file. +fn rust_source_tail(word: &str) -> Option<&str> { + let tail = word.rsplit('/').next()?; + // The extension is followed by `:line:col` in a backtrace and by nothing + // in a `Location`, so match on the extension rather than on the end. + if tail.contains(".rs") { + Some(tail) + } else { + None + } +} + +/// Whether this word introduces a value that must not be written down. +fn names_a_secret(word: &str) -> bool { + let w = word + .trim_matches(|c: char| !c.is_alphanumeric() && c != '_' && c != '-') + .to_ascii_lowercase(); + matches!( + w.as_str(), + "password" + | "passwd" + | "app-password" + | "app_password" + | "token" + | "secret" + | "bearer" + | "authorization" + | "apikey" + | "api-key" + | "api_key" + | "credential" + | "credentials" + | "cookie" + ) +} + +/// Whether a word looks like a key rather than like prose. +/// +/// A long run of the characters secrets are encoded in, containing both a +/// letter and a digit — which is what an app password, a bearer token or a +/// base64 blob looks like, and what an English word or a Rust identifier does +/// not. +fn looks_opaque(word: &str) -> bool { + word.len() >= 20 + && word + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '_' | '-' | '=')) + && word.chars().any(|c| c.is_ascii_digit()) + && word.chars().any(|c| c.is_ascii_alphabetic()) +} + +/// Cut `s` to `max` bytes on a character boundary, saying that it was cut. +fn truncate(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + format!("{}… ({} bytes truncated)", &s[..end], s.len() - end) +} + +/// Seconds since the epoch, or 0 if the clock is before it. +fn now() -> i64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_users_photograph_never_reaches_a_record() { + let r = redact("failed to open /home/anna/Photos/2019 Divorce/IMG_0042.CR3"); + assert!(!r.contains("anna"), "{r}"); + assert!(!r.contains("IMG_0042"), "{r}"); + assert!(!r.contains("Divorce"), "{r}"); + assert!(r.contains("failed to open"), "the diagnosis was lost: {r}"); + } + + #[test] + fn an_android_document_uri_is_a_path_too() { + // SAF hands out `content://` URIs and `primary:DCIM/...` refs rather + // than paths, and both name the user's library just as precisely. + let r = redact("no such image: content://com.android.providers/tree/primary%3ADCIM"); + assert!(!r.contains("DCIM"), "{r}"); + let r = redact("source_ref primary:DCIM/Camera/IMG_1.CR3 missing"); + assert!(!r.contains("IMG_1"), "{r}"); + } + + #[test] + fn a_server_url_is_removed_because_it_names_the_user() { + // A Nextcloud URL is the user's own server, often with their login in + // the path. NFR-SEC-3 is about the wire; this is about the disk. + let r = redact("PROPFIND https://cloud.example.org/remote.php/dav/files/anna/ failed"); + assert!(!r.contains("cloud.example.org"), "{r}"); + assert!(!r.contains("anna"), "{r}"); + assert!(r.contains("PROPFIND"), "{r}"); + } + + #[test] + fn a_credential_never_reaches_a_record() { + // NFR-SEC-2, which forbids credentials in logs and plain files. Both + // spellings: the value beside a naming word, and the value alone. + let r = redact("auth failed: password hunter2correcthorse"); + assert!(!r.contains("hunter2correcthorse"), "{r}"); + + let r = redact("Authorization: Bearer aGVsbG90aGVyZTEyMzQ1Njc4OTA="); + assert!(!r.contains("aGVsbG90aGVyZTEyMzQ1Njc4OTA"), "{r}"); + + let r = redact("rejected app-password=abcde-fghij-12345-klmno-pqrst"); + assert!(!r.contains("abcde-fghij"), "{r}"); + } + + #[test] + fn a_bare_secret_is_caught_by_its_shape() { + // Nextcloud app passwords arrive with no label at all when they are + // interpolated into a message by a library this codebase does not own. + let r = redact("login failed for aBcDe1FgHiJ2kLmNo3PqRsT4uV"); + assert!(!r.contains("aBcDe1FgHiJ"), "{r}"); + } + + #[test] + fn a_backtrace_keeps_the_frames_that_make_it_readable() { + // The whole reason the `.rs` exception exists: redacting these to + // `` leaves a backtrace of nothing but symbol names, and the + // line number is a fact about this codebase rather than about anyone. + let r = redact(" 3: dr_ui::library::run_scan\n at ./ui/dr-ui/src/library.rs:1270:9\n"); + assert!(r.contains("library.rs:1270:9"), "{r}"); + assert!(r.contains("dr_ui::library::run_scan"), "{r}"); + assert!(!r.contains("ui/dr-ui/src"), "{r}"); + // And the shape survives, because a backtrace is read as a shape. + assert!(r.contains('\n'), "{r}"); + assert!(r.starts_with(" 3:"), "{r}"); + } + + #[test] + fn ordinary_words_are_left_alone() { + // Over-redaction has a cost too: a record that says nothing is not + // safer, it is just useless. + let msg = "assertion failed: tier_desired was 3, expected 2"; + assert_eq!(redact(msg), msg); + } + + #[test] + fn an_enormous_payload_is_capped() { + // A panic that formatted a decoded buffer — or, the case NFR-SEC-5 + // cares about, an embedding — into its message. + let huge = "9".repeat(MAX_MESSAGE * 3); + let cut = truncate(&huge, MAX_MESSAGE); + assert!(cut.len() < huge.len()); + assert!(cut.contains("truncated"), "{cut}"); + } + + #[test] + fn truncation_does_not_split_a_character() { + let s = "é".repeat(100); + // 3 is mid-character for a 2-byte encoding. + let cut = truncate(&s, 3); + assert!(cut.starts_with('é')); + } + + #[test] + fn records_are_written_and_rotated() { + let dir = std::env::temp_dir().join(format!("dr-crash-test-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + + for i in 0..(KEEP_RECORDS as i64 + 5) { + write_record( + &dir, + &Record { + body: format!("darkroom-crash 1\nmessage: {i}\n"), + summary: String::new(), + // Distinct seconds, or the pid-suffixed names would + // collide and the rotation would have nothing to count. + when: 1_000 + i, + }, + ) + .unwrap(); + } + + let kept: Vec = std::fs::read_dir(&dir) + .unwrap() + .flatten() + .map(|e| e.path()) + .collect(); + assert_eq!(kept.len(), KEEP_RECORDS); + // The newest survive, not the oldest. + let newest = kept.iter().filter_map(|p| timestamp_of(p)).max(); + let oldest = kept.iter().filter_map(|p| timestamp_of(p)).min(); + assert_eq!(newest, Some(1_000 + KEEP_RECORDS as i64 + 4)); + assert_eq!(oldest, Some(1_000 + 5)); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_stray_file_is_neither_listed_nor_pruned() { + let dir = std::env::temp_dir().join(format!("dr-crash-stray-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("darkroom.log"), b"not a crash").unwrap(); + + for i in 0..(KEEP_RECORDS as i64 + 5) { + write_record( + &dir, + &Record { + body: String::new(), + summary: String::new(), + when: 2_000 + i, + }, + ) + .unwrap(); + } + + assert!(dir.join("darkroom.log").is_file(), "the log was pruned"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn the_state_directory_is_neither_config_nor_cache() { + // NFR-OPS-1 puts the log here too, and NFR-OPS-3 keeps preferences + // separate from both. The distinction is what stops a crash record + // being swept away by the thing that is allowed to sweep caches. + let dir = state_dir(); + let s = dir.to_string_lossy(); + assert!(s.ends_with("darkroom"), "{s}"); + assert!(!s.contains("/cache"), "{s}"); + } +} diff --git a/platform/dr-plat/src/lib.rs b/platform/dr-plat/src/lib.rs index 808ed80..77f0b68 100644 --- a/platform/dr-plat/src/lib.rs +++ b/platform/dr-plat/src/lib.rs @@ -4,6 +4,13 @@ //! construction, so `core/` contains no `#[cfg(target_os)]` (NFR-PORT-1, //! ARCH §10). +// Not a trait, and the one module here that is not. It belongs in this crate +// for the same reason the traits do: "where does this platform let an +// application keep state" is a platform question, and both entry points need +// the answer before either has a window. Resolving it in `apps/` would mean +// writing it twice, once per platform, which is the arrangement this crate +// exists to prevent. +pub mod crash; pub mod display; pub mod secrets; pub mod storage;