From 8ee892e9d973ac906828a2cf72de89d11fd0f4b4 Mon Sep 17 00:00:00 2001 From: utof Date: Sat, 1 Aug 2026 20:19:47 +0400 Subject: [PATCH 1/5] fix(scan): record locations against the volume root, not the scan root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Scanner::walk` takes `root` and `volume_root` as distinct parameters — the second is the prefix each file's `relative_path` is recorded against. All three call sites passed the scan/watch root for both, while `record_mount` stored the real mount point as `volume_mounts.mount_path`. The two halves of the reconstruction therefore disagreed by exactly the scan-root-minus-mount-point segment, and `mount_path + relative_path` did not resolve to the file. Scanning `/mnt/data/Videos` recorded `clip.mp4` instead of `Videos/clip.mp4`, so the read path reconstructed `/mnt/data/clip.mp4`. Observed on a real library as 417 of 419 rows pointing at nothing, which surfaced downstream as a misleading transcription failure: Could not decode audio from source: stat input: No such file or directory (os error 2) `relativize` itself was correct and well-tested; only its callers were wrong. Fixed at all three sites: - crates/app/src/scan.rs — mount point, scan root in dry-run - crates/cli/src/cmd/watch.rs — detected.mount_point - crates/desktop/src/commands.rs — detected.mount_point The watch sites matter beyond path reconstruction: watcher-emitted `FileEvent::Deleted` is currently the only producer of `LocationStatus::Missing`, so it was keyed on a different root than the one scan wrote. Safety: `detect_volume` selects its mount by `canonical.starts_with(mp)` with longest-prefix-wins, so the mount point is guaranteed to be an ancestor of the scan root. `relativize`'s `strip_prefix` cannot begin failing as a result — it was succeeding against the wrong prefix. WHY the suite missed this: every scan test uses a `tempfile` root, and a temp dir sits under a mount rather than being one, so `walk(root, root)` was correct in the fixture and wrong in production. No test joined `mount_path` and `relative_path` back together, which is the only place the disagreement is observable. The new regression test asserts that reconstruction — the same join the read path performs — and was verified to fail before this fix with `/ + alpha.txt = /alpha.txt (not found)`. It also fails loudly if the fixture root is ever its own mount point, rather than passing vacuously. `scan_persists_metadata_rows_for_images` matched `relative_path = 'red.png'` exactly, encoding the old behaviour; it now matches the basename, with a count guard so the lookup cannot silently become ambiguous. Note for existing databases: `relative_path` changes meaning, so a rescan inserts new rows rather than updating old ones. Fresh catalogues are unaffected. Closes #191 Co-Authored-By: Claude Opus 5 (1M context) --- crates/app/src/scan.rs | 92 ++++++++++++++++++++- crates/cli/src/cmd/watch.rs | 10 ++- crates/cli/tests/scan_with_metadata_test.rs | 27 +++++- crates/desktop/src/commands.rs | 9 +- 4 files changed, 134 insertions(+), 4 deletions(-) diff --git a/crates/app/src/scan.rs b/crates/app/src/scan.rs index e632f2b..21e1977 100644 --- a/crates/app/src/scan.rs +++ b/crates/app/src/scan.rs @@ -471,13 +471,35 @@ impl ScanUseCase { Some((vol_id, label, detected.mount_point)) }; + // WHY the mount point (not `canonical_root`) as `volume_root`: + // `Scanner::walk`'s second parameter is the prefix each file's + // `relative_path` is recorded against, and `record_mount` above + // stores `detected.mount_point` as `volume_mounts.mount_path`. + // The two must agree or `mount_path + relative_path` does not + // reconstruct the file — passing `canonical_root` for both + // silently dropped the scan-root-minus-mount-point segment from + // every row, so scanning `/mnt/data/Videos` recorded `clip.mp4` + // instead of `Videos/clip.mp4`. See GH #191. + // + // `detect_volume` picks its mount by `canonical.starts_with(mp)` + // (longest-prefix wins), so the mount point is guaranteed to be + // an ancestor of `canonical_root` and `relativize`'s + // `strip_prefix` cannot fail as a result of this. + // + // Dry-run resolves no volume, so it falls back to the scan root + // — it prints hashes and persists nothing, so no row can be + // written with a root-relative path. + let volume_root = volume_info + .as_ref() + .map_or(canonical_root.as_path(), |(_, _, mount)| mount.as_path()); + // Collect up-front so rayon can parallelize hashing; the walker // iterator itself isn't Send across the par_iter boundary. The // inner `take_while` polls between yielded items so a Ctrl-C // during walk short-circuits quickly. let discovered: Vec = self .scanner - .walk(&canonical_root, &canonical_root)? + .walk(&canonical_root, volume_root)? .take_while(|_| !cancel.is_cancelled()) .collect(); @@ -1106,6 +1128,74 @@ mod tests { ); } + /// GH #191 regression: every recorded `relative_path` must resolve + /// against the recorded mount point. + /// + /// WHY assert the reconstruction rather than the `relative_path` + /// string: `mount_path + relative_path` is the reconstruction the + /// read path actually performs (`crates/desktop/src/payloads.rs`), + /// so asserting it here pins the invariant that matters instead of + /// a literal that changes with the fixture layout. + /// + /// WHY this bug survived the rest of this module: every other test + /// scans a `tempfile` root, and a temp dir sits *under* a mount + /// rather than being one. Passing the scan root as `volume_root` + /// therefore produced paths relative to the scan root — which no + /// existing assertion looked at, because none of them joined the + /// two halves back together. + #[tokio::test] + async fn persisted_relative_paths_resolve_against_the_recorded_mount() { + let h = harness(); + let cmd = ScanCommand::Full(FullScan { + path: h.fixture.path().to_path_buf(), + device_id: DeviceId::new(), + with_metadata: false, + dry_run: false, + no_wait_metadata: true, + no_thumbnails: true, + cancel: CancellationToken::new(), + on_persist: None, + }); + let report = h.uc.execute(cmd).await.unwrap(); + + let (_, mount) = report + .volume_mount + .clone() + .expect("non-dry-run records a volume mount"); + + // Guard against a vacuous pass: if the fixture root happened to + // be its own mount point, `walk(root, root)` would be correct + // and this test would prove nothing. Fail loudly instead. + let canonical_fixture = perima_fs::platform_path::canonicalize(h.fixture.path()).unwrap(); + assert_ne!( + mount, canonical_fixture, + "test is vacuous when the scan root is itself the mount point", + ); + assert!( + canonical_fixture.starts_with(&mount), + "detect_volume must return an ancestor of the scan root; got mount {} for root {}", + mount.display(), + canonical_fixture.display(), + ); + + assert_eq!( + report.per_file_entries.len(), + 3, + "all three fixture files must be reported", + ); + for entry in &report.per_file_entries { + let reconstructed = mount.join(entry.relative_path.as_str()); + assert!( + reconstructed.is_file(), + "mount_path + relative_path must reconstruct a real file: \ + {} + {} = {} (not found)", + mount.display(), + entry.relative_path.as_str(), + reconstructed.display(), + ); + } + } + #[tokio::test] async fn rescan_is_idempotent() { let h = harness(); diff --git a/crates/cli/src/cmd/watch.rs b/crates/cli/src/cmd/watch.rs index 834207d..5ce30b3 100644 --- a/crates/cli/src/cmd/watch.rs +++ b/crates/cli/src/cmd/watch.rs @@ -204,9 +204,17 @@ pub(crate) async fn run( // WHY 1 s production debounce: short enough for responsive feedback, long // enough to coalesce rapid saves (e.g. editors that write-then-chmod). + // WHY `detected.mount_point` as `volume_root` (not `canonical_root`): + // the watched path and the prefix that relative paths are recorded + // against are different things. `record_mount` above stores + // `detected.mount_point`, and watcher-emitted `FileEvent`s carry + // paths relativized against this argument — they must share a root + // or the events address rows that scan never wrote. Watching + // `/mnt/data/Videos` previously emitted `clip.mp4` where scan had + // recorded `Videos/clip.mp4`. See GH #191. let watcher = DebouncedWatcher::start( std::slice::from_ref(&canonical_root), - &canonical_root, + &detected.mount_point, volume_id, bus, cancel.token(), diff --git a/crates/cli/tests/scan_with_metadata_test.rs b/crates/cli/tests/scan_with_metadata_test.rs index fdd8b03..cb75944 100644 --- a/crates/cli/tests/scan_with_metadata_test.rs +++ b/crates/cli/tests/scan_with_metadata_test.rs @@ -96,10 +96,35 @@ fn scan_persists_metadata_rows_for_images() { ); // Verify the PNG row has captured dimensions (ImageExtractor ran). + // + // WHY suffix-match instead of `relative_path = 'red.png'`: locations + // are recorded relative to the VOLUME root, not the scan root (GH + // #191). The stored value is therefore the fixture temp dir's path + // relative to whatever mount `/tmp` lives on, plus `/red.png` — + // environment-dependent by construction. The basename is the only + // stable part. + // + // The equality form previously passed only because scan recorded + // paths against the scan root, which was the bug: `mount_path + + // relative_path` did not reconstruct the file. This assertion moved + // with the fix rather than being weakened for convenience. + let png_match_count: i64 = conn + .query_row( + "SELECT COUNT(*) FROM file_locations + WHERE relative_path LIKE '%red.png' AND deleted_at IS NULL", + [], + |r| r.get(0), + ) + .expect("count png locations"); + assert_eq!( + png_match_count, 1, + "suffix match must identify exactly one location row, else the \ + hash lookup below is ambiguous", + ); let png_hash: String = conn .query_row( "SELECT fl.blake3_hash FROM file_locations fl - WHERE fl.relative_path = 'red.png' AND fl.deleted_at IS NULL", + WHERE fl.relative_path LIKE '%red.png' AND fl.deleted_at IS NULL", [], |r| r.get(0), ) diff --git a/crates/desktop/src/commands.rs b/crates/desktop/src/commands.rs index 93c4ee2..6c700a1 100644 --- a/crates/desktop/src/commands.rs +++ b/crates/desktop/src/commands.rs @@ -713,9 +713,16 @@ pub async fn start_watch( // WHY 1 s production debounce: short enough for responsive feedback, // long enough to coalesce rapid saves (e.g. editors that write-then-chmod). + // WHY `detected.mount_point` as `volume_root` (not `canonical_root`): + // the watched path and the prefix that relative paths are recorded + // against are different things. `RecordMount` above stores + // `detected.mount_point`, and watcher-emitted `FileEvent`s carry + // paths relativized against this argument — they must share a root + // or the events address rows that scan never wrote. Mirrors the CLI + // `watch` shell. See GH #191. let watcher = DebouncedWatcher::start( std::slice::from_ref(&canonical_root), - &canonical_root, + &detected.mount_point, volume_id, bus, cancel.clone(), From 89216d134fe64397ff5eaba39dafb69e5ac1ca72 Mon Sep 17 00:00:00 2001 From: utof Date: Sat, 1 Aug 2026 20:51:43 +0400 Subject: [PATCH 2/5] fix(just): clear the orphaned vite server before `just dev` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `beforeDevCommand` runs with `"wait": false`, so Tauri spawns `bun run dev` without reaping it, and `bun` spawns `vite` as a grandchild. When Tauri exits, that grandchild survives holding port 5173. Because `devUrl` pins that exact port, the next `just dev` fails outright rather than reusing or relocating: error when starting dev server: Error: Port 5173 is already in use Error The "beforeDevCommand" terminated with a non-zero status code. Every second launch failed until the stray process was killed by hand. The pattern is the repo-local vite binary path, so it cannot match a dev server belonging to another project on the same machine. WHY `[v]ite` rather than `vite`: `pkill -f` matches the full command line of every process, including the shell running the recipe — whose argv contains the pattern verbatim. Spelled plainly, pkill kills its own shell and the recipe dies with exit 144 before reaching the `tauri dev` line. Verified both ways against a scratch justfile: `vite` reproduces the self-kill, `[v]ite` prints the following line and exits 0. Co-Authored-By: Claude Opus 5 (1M context) --- justfile | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/justfile b/justfile index 9b880cd..9f6ebb6 100644 --- a/justfile +++ b/justfile @@ -95,8 +95,28 @@ sidecar: # # The vite dev server starts itself via `beforeDevCommand` in # tauri.conf.json, so this is the only command needed. +# +# WHY the pkill first: `beforeDevCommand` runs with `"wait": false`, so +# Tauri spawns `bun run dev` and does not reap it. `bun` in turn spawns +# `vite` as a grandchild, which survives Tauri's exit still holding port +# 5173 — and because `devUrl` pins that exact port, the NEXT `just dev` +# dies with "Port 5173 is already in use" rather than reusing or moving. +# The pattern is the repo-local vite binary path, so this cannot touch a +# dev server belonging to another project on the same machine. +# +# WHY the leading `-`: just aborts a recipe on the first non-zero exit, +# and pkill exits 1 when it matches nothing — which is the common case. +# +# WHY `[v]ite` and not `vite`: `pkill -f` matches against the FULL command +# line of every process, including the shell running this very recipe — +# whose argv contains this pattern verbatim. Spelled `vite`, pkill matches +# its own shell and kills it (observed: recipe dies with exit 144 before +# reaching the next line). The bracket expression still matches the string +# "vite" in the target process, but the literal text here reads "[v]ite", +# which the regex does not match. Standard `ps | grep` self-match dodge. # Launch the desktop app in dev mode (vite + Tauri). dev: + -@pkill -f 'apps/desktop/node_modules/.bin/[v]ite' 2>/dev/null cd crates/desktop && ../../apps/desktop/node_modules/.bin/tauri dev deny: From d3dcfe7da762693728b5492114aeb65382bd5aef Mon Sep 17 00:00:00 2001 From: utof Date: Sat, 1 Aug 2026 21:43:50 +0400 Subject: [PATCH 3/5] feat(verify): reconcile catalogued locations against the filesystem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scanning records where files were; nothing afterwards noticed when one went away. The filesystem watcher only sees deletions that happen while it is running, so a file removed with the app closed stayed `Active` in the catalogue forever. `perima verify` walks every location on a mounted volume, stats it, and writes back the statuses that changed — both directions, so a file that comes back returns to `Active` rather than being stuck `Missing`. This has to exist before any prune: deleting rows marked `Missing` is only as trustworthy as whatever marked them. ## The unmounted-volume rule is structural, not documented `list_locations_for_verify` INNER JOINs `volume_mounts`, so a location whose volume is not mounted does not appear in the result set at all — it is not returned with a NULL path for the caller to remember to filter. The sweep marks everything it receives and cannot stat as `Missing`, so one leaked row for an unplugged external drive becomes a false `Missing`, and a prune then deletes a catalogue whose files are intact in a drawer. Making absence structural means no caller can get this wrong. The count of excluded rows travels back with the rows themselves (`VerifyCandidates`) rather than being an optional second query. A sweep that silently omits an unmounted drive reports "checked 78, no changes" over a 500-file library and reads as a clean bill of health; carrying the number the caller may NOT conclude anything about makes an honest report the path of least resistance. `VerifyReport::is_complete()` is the single predicate a destructive caller should gate on. ## machine_id The mount join is scoped to the calling device. `volume_mounts` is keyed on `(volume_id, machine_id)`, and joining without that predicate pairs a local row with another computer's mount path — the sweep would then stat paths that mean nothing here and mark the whole local catalogue `Missing`. Three older queries in `file_repo.rs` omit the predicate; that is #195, left alone here so the trait-signature change lands as its own reviewable commit. ## Other decisions - `symlink_metadata`, not `metadata`: a symlink whose target is gone still has an entry at the catalogued path. Whether the target is healthy is the hashing path's question. - `Moved` and `Stale` are left untouched when the file is present. This sweep only looked at existence, so it only writes the present/absent axis; downgrading either to `Active` would discard information it did not earn. - Status writes go through one batched transaction. Per-row writes would open one `BEGIN IMMEDIATE` per changed file — the write-amplification shape that provoked the SQLite lock-order inversion in #131. One `hlc` covers the batch: a sweep is one logical event, and every row it touched observed the same filesystem state. - A cancelled sweep writes nothing and reports `completed: false`. Verified end-to-end against a real 78-file library: a clean sweep reports no changes; hiding a file makes the next sweep mark exactly that row `missing`; restoring it makes the following sweep report it recovered. Co-Authored-By: Claude Opus 5 (1M context) --- crates/app/src/container.rs | 15 + crates/app/src/lib.rs | 4 + crates/app/src/verify.rs | 587 +++++++++++++++++++++++++++++ crates/cli/src/cmd/mod.rs | 2 + crates/cli/src/cmd/verify.rs | 160 ++++++++ crates/cli/src/main.rs | 69 ++++ crates/core/src/lib.rs | 5 +- crates/core/src/ports/file_repo.rs | 164 +++++++- crates/core/src/ports/mod.rs | 4 +- crates/db/src/cmd.rs | 35 +- crates/db/src/file_repo.rs | 179 ++++++++- crates/db/src/writer/file.rs | 151 +++++++- 12 files changed, 1362 insertions(+), 13 deletions(-) create mode 100644 crates/app/src/verify.rs create mode 100644 crates/cli/src/cmd/verify.rs diff --git a/crates/app/src/container.rs b/crates/app/src/container.rs index b83e971..c0997dc 100644 --- a/crates/app/src/container.rs +++ b/crates/app/src/container.rs @@ -169,6 +169,13 @@ pub struct AppContainer { /// [`TranscriptionUseCase`] — provider registry + queue + worker /// orchestration. Owns the single `tokio::spawn`-backed worker task. pub transcription: Arc, + /// [`crate::VerifyUseCase`] — reconcile catalogued locations against + /// the filesystem (mark vanished files `Missing`, restore returned ones). + pub verify: Arc, + /// [`crate::PruneUseCase`] — retire catalogue entries that a verify + /// sweep found missing. Destructive; see the type docs for why it + /// must follow a complete sweep. + pub prune: Arc, /// Shared event bus — same `Arc` used inside every `UseCase`. pub events: Arc, /// Direct handle to the volume repository port. @@ -393,6 +400,12 @@ impl AppContainer { // needs `quick_hash_prefix_suffix` without re-constructing // a Blake3Service. Arc::clone is refcount-only. let hasher = Arc::clone(&deps.hasher); + // WHY the verify use case is built here rather than by each + // shell: both the CLI subcommand and the Tauri command need the + // identical wiring, and constructing it per-shell is how the two + // drift apart. Holds only an `Arc` clone of the file repo. + let verify = Arc::new(crate::VerifyUseCase::new(Arc::clone(&deps.files))); + let prune = Arc::new(crate::PruneUseCase::new(Arc::clone(&deps.files))); Arc::new(Self { backup, @@ -404,6 +417,8 @@ impl AppContainer { compute_full_hash, dedup, transcription, + verify, + prune, events, volumes, tags, diff --git a/crates/app/src/lib.rs b/crates/app/src/lib.rs index ab02a91..270bca6 100644 --- a/crates/app/src/lib.rs +++ b/crates/app/src/lib.rs @@ -38,6 +38,7 @@ pub mod search; pub mod tag; pub mod telemetry; pub mod transcription; +pub mod verify; pub mod volume; pub use backfill::{BackfillRate, BackfillReport, BackfillRow, QuickHashBackfillWorker}; @@ -55,4 +56,7 @@ pub use search::{SearchCommand, SearchOutput, SearchUseCase}; pub use tag::{FileWithTags, TagCommand, TagFilter, TagOutput, TagUseCase}; pub use telemetry::LogEventHandler; pub use transcription::{TranscribeCommand, TranscribeOutput, TranscriptionUseCase}; +pub use verify::{ + PruneCommand, PruneReport, PruneUseCase, VerifyCommand, VerifyReport, VerifyUseCase, +}; pub use volume::{VolumeCommand, VolumeOutput, VolumeUseCase}; diff --git a/crates/app/src/verify.rs b/crates/app/src/verify.rs new file mode 100644 index 0000000..2bb0d85 --- /dev/null +++ b/crates/app/src/verify.rs @@ -0,0 +1,587 @@ +//! `VerifyUseCase` — reconcile catalogued locations against the filesystem. +//! +//! Scanning records where files were. Nothing afterwards notices when +//! one goes away: the filesystem watcher only sees deletions that happen +//! while it is running, so a file removed with the app closed stays +//! `Active` in the catalogue forever. This use case closes that gap by +//! walking every location on a currently-mounted volume, stat-ing it, +//! and writing back the statuses that actually changed. +//! +//! It is the precondition for pruning: a prune that deletes `Missing` +//! rows is only as trustworthy as whatever marked them `Missing`. + +use std::sync::Arc; + +use perima_core::{ + CoreError, DeviceId, FileRepository, LocationStatus, LocationStatusUpdate, LocationToVerify, +}; +use tokio_util::sync::CancellationToken; + +/// Inputs to [`VerifyUseCase::execute`]. +#[derive(Clone, Debug)] +pub struct VerifyCommand { + /// Device performing the sweep. Scopes the `volume_mounts` join so + /// paths are resolved against *this* machine's mounts. + pub device_id: DeviceId, + /// Report what would change without writing anything. + pub dry_run: bool, + /// Cancellation token. Polled between rows; a cancelled sweep writes + /// nothing. + pub cancel: CancellationToken, +} + +/// Outcome of a verify sweep. +/// +/// WHY `Serialize + specta::Type`: the desktop `verify_locations` +/// handler returns this struct directly across the Tauri IPC boundary, +/// same pattern as `ScanReport`. +#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Serialize)] +#[cfg_attr(feature = "specta", derive(specta::Type))] +pub struct VerifyReport { + /// Locations stat-ed (i.e. on a volume mounted on this device). + pub checked: usize, + /// Rows that were `Active` (or otherwise present) and whose file is + /// gone — transitioned to `Missing`. + pub newly_missing: usize, + /// Rows that were `Missing` and whose file is back — transitioned to + /// `Active`. + pub recovered: usize, + /// Locations excluded because their volume is not mounted here. + /// + /// These were NOT checked and their status was NOT touched. A + /// non-zero value means the sweep's view of the catalogue is + /// partial — see [`VerifyReport::is_complete`]. + pub skipped_unmounted: usize, + /// Rows actually written. `0` in a dry run. + pub rows_written: u64, + /// Whether the sweep ran to completion (`false` if cancelled). + pub completed: bool, +} + +impl VerifyReport { + /// True when the sweep saw every non-deleted location in the catalogue. + /// + /// WHY callers should check this before acting destructively: a + /// prune driven by an incomplete sweep deletes rows for files that + /// are perfectly intact on a drive that simply was not plugged in. + #[must_use] + pub const fn is_complete(&self) -> bool { + self.completed && self.skipped_unmounted == 0 + } +} + +/// Reconciles `file_locations` against the filesystem. +pub struct VerifyUseCase { + files: Arc, +} + +impl std::fmt::Debug for VerifyUseCase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("VerifyUseCase").finish_non_exhaustive() + } +} + +/// Decide the status a location should have, given what is on disk. +/// +/// Returns `None` when the recorded status already matches reality, so +/// the caller can skip the write. +/// +/// WHY `symlink_metadata` rather than `metadata`: `metadata` follows +/// symlinks, so a symlink whose target has been deleted reports +/// `NotFound` and the location would be marked `Missing` even though the +/// catalogued entry is still sitting there. `symlink_metadata` asks the +/// question the catalogue actually cares about — "is there still an +/// entry at this path" — and leaves "is the target healthy" to the +/// hashing path, which reads the bytes anyway. +/// +/// WHY `Moved` and `Stale` are treated as present-and-fine: neither +/// means "gone". `Stale` means the content changed and a re-scan will +/// re-hash it; `Moved` is set by the rename path. Downgrading either to +/// `Active` here would discard information this sweep did not earn — it +/// only looked at existence, not at hashes or paths. The sweep therefore +/// only ever writes the `present <-> absent` axis. +fn decide(loc: &LocationToVerify) -> Option { + let exists = std::fs::symlink_metadata(&loc.absolute_path).is_ok(); + match (exists, loc.status) { + // Gone, and not already recorded as gone. + (false, LocationStatus::Missing) => None, + (false, _) => Some(LocationStatus::Missing), + // Back from the dead. + (true, LocationStatus::Missing) => Some(LocationStatus::Active), + // Present and already recorded as present in some form. + (true, _) => None, + } +} + +impl VerifyUseCase { + /// Construct the use case from a file repository. + #[must_use] + pub const fn new(files: Arc) -> Self { + Self { files } + } + + /// Walk every location on a mounted volume and reconcile its status. + /// + /// # Errors + /// Returns `CoreError` if the catalogue cannot be read or the status + /// batch cannot be written. + pub fn execute(&self, cmd: &VerifyCommand) -> Result { + let candidates = self.files.list_locations_for_verify(cmd.device_id)?; + + let mut report = VerifyReport { + skipped_unmounted: candidates.skipped_unmounted, + ..Default::default() + }; + + let mut updates: Vec = Vec::new(); + for loc in &candidates.locations { + if cmd.cancel.is_cancelled() { + // WHY return the partial report rather than an error: a + // cancelled sweep is a user action, not a failure. The + // `completed: false` flag is what stops a caller from + // treating the result as authoritative. + report.completed = false; + return Ok(report); + } + report.checked += 1; + if let Some(next) = decide(loc) { + match next { + LocationStatus::Missing => report.newly_missing += 1, + LocationStatus::Active => report.recovered += 1, + LocationStatus::Moved | LocationStatus::Stale => {} + } + updates.push(LocationStatusUpdate { + volume: loc.volume, + path: loc.path.clone(), + status: next, + }); + } + } + + report.completed = true; + if cmd.dry_run { + return Ok(report); + } + report.rows_written = self + .files + .update_location_statuses(&updates, cmd.device_id)?; + Ok(report) + } +} + +/// Inputs to [`PruneUseCase::execute`]. +#[derive(Clone, Debug)] +pub struct PruneCommand { + /// Device performing the prune (recorded as the writing device). + pub device_id: DeviceId, + /// Count what would be removed without writing anything. + pub dry_run: bool, +} + +/// Outcome of a prune. +/// +/// WHY `Serialize + specta::Type`: returned directly by the desktop +/// `prune_missing_locations` handler across the Tauri IPC boundary. +#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Serialize)] +#[cfg_attr(feature = "specta", derive(specta::Type))] +pub struct PruneReport { + /// Locations recorded as `Missing` at the time of the call. + pub missing_found: u64, + /// Rows actually retired. `0` in a dry run. + pub rows_pruned: u64, +} + +/// Removes catalogue entries for files that verification found missing. +/// +/// # Relationship to [`VerifyUseCase`] +/// +/// Prune deletes on the strength of `status = Missing` and performs no +/// filesystem check of its own. That split is deliberate — deciding what +/// is missing requires stat-ing paths, which must not happen inside a +/// write transaction — but it means **a prune is only as trustworthy as +/// the sweep that preceded it**. Callers should run +/// [`VerifyUseCase::execute`] first and check +/// [`VerifyReport::is_complete`] before offering a prune, so a user is +/// never invited to delete rows on the strength of a sweep that skipped +/// an unplugged drive. +pub struct PruneUseCase { + files: Arc, +} + +impl std::fmt::Debug for PruneUseCase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("PruneUseCase").finish_non_exhaustive() + } +} + +impl PruneUseCase { + /// Construct the use case from a file repository. + #[must_use] + pub const fn new(files: Arc) -> Self { + Self { files } + } + + /// Count missing locations without removing anything. + /// + /// Exposed on its own so a UI can label its confirm button with the + /// real number before the user commits. + /// + /// # Errors + /// Returns `CoreError` if the catalogue cannot be read. + pub fn count_missing(&self) -> Result { + self.files.count_missing_locations() + } + + /// Soft-delete every location currently marked `Missing`. + /// + /// # Errors + /// Returns `CoreError` if the catalogue cannot be read or written. + pub fn execute(&self, cmd: &PruneCommand) -> Result { + let missing_found = self.files.count_missing_locations()?; + if cmd.dry_run || missing_found == 0 { + return Ok(PruneReport { + missing_found, + rows_pruned: 0, + }); + } + let rows_pruned = self.files.soft_delete_missing_locations(cmd.device_id)?; + Ok(PruneReport { + missing_found, + rows_pruned, + }) + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] // Test code; unwrap panics signal bugs. +mod tests { + use std::sync::Mutex; + + use perima_core::{MediaPath, VerifyCandidates, VolumeId}; + + use super::*; + + /// Repository stub that serves a fixed candidate set and records the + /// batch it was asked to write. + /// + /// WHY a hand-written stub rather than a real `SqliteFileRepository`: + /// these tests pin the sweep's *decision* logic — what it concludes + /// from `(status on record, exists on disk)` and what it refuses to + /// touch. The adapter's SQL is exercised separately; mixing the two + /// would mean a query bug and a policy bug produce the same red test. + struct StubRepo { + candidates: Mutex, + written: Mutex>, + missing_count: Mutex, + pruned: Mutex, + } + + impl FileRepository for StubRepo { + fn upsert_file( + &self, + _file: &perima_core::HashedFile, + _device: DeviceId, + ) -> Result { + unreachable!("verify sweep does not upsert files") + } + fn upsert_location( + &self, + _hash: &perima_core::BlakeHash, + _volume: VolumeId, + _path: &MediaPath, + _device: DeviceId, + ) -> Result { + unreachable!("verify sweep does not upsert locations") + } + fn list_file_locations( + &self, + _limit: usize, + _volume: Option, + ) -> Result, CoreError> { + Ok(vec![]) + } + fn list_locations_for_verify( + &self, + _device: DeviceId, + ) -> Result { + Ok(self.candidates.lock().unwrap().clone()) + } + fn update_location_statuses( + &self, + updates: &[LocationStatusUpdate], + _device: DeviceId, + ) -> Result { + self.written.lock().unwrap().extend_from_slice(updates); + Ok(u64::try_from(updates.len()).unwrap()) + } + fn count_missing_locations(&self) -> Result { + Ok(*self.missing_count.lock().unwrap()) + } + fn soft_delete_missing_locations(&self, _device: DeviceId) -> Result { + let n = *self.missing_count.lock().unwrap(); + *self.pruned.lock().unwrap() = n; + *self.missing_count.lock().unwrap() = 0; + Ok(n) + } + } + + fn loc(path: &std::path::Path, status: LocationStatus) -> LocationToVerify { + LocationToVerify { + volume: VolumeId(uuid::Uuid::nil()), + path: MediaPath::new("rel/path"), + absolute_path: path.to_path_buf(), + status, + } + } + + fn harness(locations: Vec, skipped: usize) -> Arc { + Arc::new(StubRepo { + candidates: Mutex::new(VerifyCandidates { + locations, + skipped_unmounted: skipped, + }), + written: Mutex::new(Vec::new()), + missing_count: Mutex::new(0), + pruned: Mutex::new(0), + }) + } + + fn cmd(dry_run: bool) -> VerifyCommand { + VerifyCommand { + device_id: DeviceId::new(), + dry_run, + cancel: CancellationToken::new(), + } + } + + #[test] + fn marks_absent_active_location_missing() { + let tmp = tempfile::tempdir().unwrap(); + let gone = tmp.path().join("gone.mp4"); + let repo = harness(vec![loc(&gone, LocationStatus::Active)], 0); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert_eq!(report.checked, 1); + assert_eq!(report.newly_missing, 1); + assert_eq!(report.recovered, 0); + assert_eq!(report.rows_written, 1); + // WHY clone out of the guard: holding a MutexGuard across the + // asserts trips `significant_drop_tightening`. Cloning releases + // the lock at the end of this statement. + let written = repo.written.lock().unwrap().clone(); + assert_eq!(written.len(), 1); + assert_eq!(written[0].status, LocationStatus::Missing); + } + + #[test] + fn recovers_missing_location_that_came_back() { + let tmp = tempfile::tempdir().unwrap(); + let present = tmp.path().join("back.mp4"); + std::fs::write(&present, b"x").unwrap(); + let repo = harness(vec![loc(&present, LocationStatus::Missing)], 0); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert_eq!(report.recovered, 1, "a file that returns must go Active"); + assert_eq!(report.newly_missing, 0); + assert_eq!( + repo.written.lock().unwrap()[0].status, + LocationStatus::Active, + ); + } + + #[test] + fn writes_nothing_when_nothing_changed() { + let tmp = tempfile::tempdir().unwrap(); + let present = tmp.path().join("here.mp4"); + std::fs::write(&present, b"x").unwrap(); + let repo = harness(vec![loc(&present, LocationStatus::Active)], 0); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert_eq!(report.checked, 1); + assert_eq!(report.rows_written, 0, "steady state must not write"); + assert!(repo.written.lock().unwrap().is_empty()); + } + + /// The sweep must not silently rewrite statuses it did not evaluate. + #[test] + fn present_moved_and_stale_rows_are_left_alone() { + let tmp = tempfile::tempdir().unwrap(); + let a = tmp.path().join("a.mp4"); + let b = tmp.path().join("b.mp4"); + std::fs::write(&a, b"x").unwrap(); + std::fs::write(&b, b"x").unwrap(); + let repo = harness( + vec![ + loc(&a, LocationStatus::Moved), + loc(&b, LocationStatus::Stale), + ], + 0, + ); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert_eq!(report.checked, 2); + assert_eq!( + report.rows_written, 0, + "existence-only sweep must not downgrade Moved/Stale to Active", + ); + assert!( + repo.written.lock().unwrap().is_empty(), + "no status transition may be proposed for present Moved/Stale rows", + ); + } + + #[test] + fn dry_run_reports_but_writes_nothing() { + let tmp = tempfile::tempdir().unwrap(); + let gone = tmp.path().join("gone.mp4"); + let repo = harness(vec![loc(&gone, LocationStatus::Active)], 0); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(true)).unwrap(); + + assert_eq!(report.newly_missing, 1, "dry run still reports the finding"); + assert_eq!(report.rows_written, 0); + assert!(repo.written.lock().unwrap().is_empty()); + } + + /// The load-bearing safety property: an unmounted volume must never + /// produce a `Missing` transition, and the report must admit that its + /// view was partial. + #[test] + fn unmounted_volumes_are_reported_not_marked_missing() { + let repo = harness(vec![], 417); + let uc = VerifyUseCase::new(repo.clone()); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert_eq!(report.checked, 0); + assert_eq!( + report.newly_missing, 0, + "rows on an unmounted volume must never be marked Missing", + ); + assert_eq!(report.skipped_unmounted, 417); + assert!(repo.written.lock().unwrap().is_empty()); + assert!( + !report.is_complete(), + "a sweep that skipped rows must not report itself complete", + ); + } + + #[test] + fn cancellation_writes_nothing_and_reports_incomplete() { + let tmp = tempfile::tempdir().unwrap(); + let gone = tmp.path().join("gone.mp4"); + let repo = harness(vec![loc(&gone, LocationStatus::Active)], 0); + let uc = VerifyUseCase::new(repo.clone()); + let c = cmd(false); + c.cancel.cancel(); + + let report = uc.execute(&c).unwrap(); + + assert!(!report.completed); + assert!(!report.is_complete()); + assert!( + repo.written.lock().unwrap().is_empty(), + "a cancelled sweep must not write a partial reconciliation", + ); + } + + #[test] + fn complete_sweep_over_mounted_volumes_reports_complete() { + let tmp = tempfile::tempdir().unwrap(); + let present = tmp.path().join("here.mp4"); + std::fs::write(&present, b"x").unwrap(); + let repo = harness(vec![loc(&present, LocationStatus::Active)], 0); + let uc = VerifyUseCase::new(repo); + + let report = uc.execute(&cmd(false)).unwrap(); + + assert!(report.is_complete()); + } + + // ----------------------------------------------------------------- + // Prune + // ----------------------------------------------------------------- + + fn prune_harness(missing: u64) -> Arc { + let repo = harness(vec![], 0); + *repo.missing_count.lock().unwrap() = missing; + repo + } + + #[test] + fn prune_removes_missing_rows_and_reports_the_count() { + let repo = prune_harness(5); + let uc = PruneUseCase::new(repo.clone()); + + let report = uc + .execute(&PruneCommand { + device_id: DeviceId::new(), + dry_run: false, + }) + .unwrap(); + + assert_eq!(report.missing_found, 5); + assert_eq!(report.rows_pruned, 5); + assert_eq!(*repo.pruned.lock().unwrap(), 5); + } + + #[test] + fn prune_dry_run_counts_without_deleting() { + let repo = prune_harness(5); + let uc = PruneUseCase::new(repo.clone()); + + let report = uc + .execute(&PruneCommand { + device_id: DeviceId::new(), + dry_run: true, + }) + .unwrap(); + + assert_eq!(report.missing_found, 5, "dry run still reports the count"); + assert_eq!(report.rows_pruned, 0); + assert_eq!( + *repo.pruned.lock().unwrap(), + 0, + "dry run must not reach the delete", + ); + } + + /// A prune with nothing to do must not reach the writer at all — + /// an empty destructive write is still a write lock and an + /// invalidation event the frontend would act on. + #[test] + fn prune_with_nothing_missing_is_a_no_op() { + let repo = prune_harness(0); + let uc = PruneUseCase::new(repo.clone()); + + let report = uc + .execute(&PruneCommand { + device_id: DeviceId::new(), + dry_run: false, + }) + .unwrap(); + + assert_eq!(report.missing_found, 0); + assert_eq!(report.rows_pruned, 0); + assert_eq!(*repo.pruned.lock().unwrap(), 0); + } + + #[test] + fn count_missing_does_not_mutate() { + let repo = prune_harness(3); + let uc = PruneUseCase::new(repo.clone()); + + assert_eq!(uc.count_missing().unwrap(), 3); + assert_eq!(uc.count_missing().unwrap(), 3, "counting is idempotent"); + assert_eq!(*repo.pruned.lock().unwrap(), 0); + } +} diff --git a/crates/cli/src/cmd/mod.rs b/crates/cli/src/cmd/mod.rs index c38d5d3..ffb43eb 100644 --- a/crates/cli/src/cmd/mod.rs +++ b/crates/cli/src/cmd/mod.rs @@ -9,9 +9,11 @@ pub(crate) mod hash; pub(crate) mod ls; pub(crate) mod metadata; pub(crate) mod migrate_data_dir; +pub(crate) mod prune; pub(crate) mod scan; pub(crate) mod search; pub(crate) mod tag; pub(crate) mod transcribe; +pub(crate) mod verify; pub(crate) mod volumes; pub(crate) mod watch; diff --git a/crates/cli/src/cmd/verify.rs b/crates/cli/src/cmd/verify.rs new file mode 100644 index 0000000..1f22fb6 --- /dev/null +++ b/crates/cli/src/cmd/verify.rs @@ -0,0 +1,160 @@ +//! `perima verify` — thin delegator to [`perima_app::VerifyUseCase`]. + +use std::io::Write; + +use perima_app::{AppContainer, VerifyCommand, VerifyReport}; +use perima_core::{CoreError, DeviceId}; +use tokio_util::sync::CancellationToken; + +/// Execute `verify`. +/// +/// Walks every catalogued location on a mounted volume, checks whether +/// the file is still there, and updates statuses that changed. +/// +/// # Errors +/// Propagates `CoreError` from the `UseCase` / repository. +pub(crate) fn run( + container: &AppContainer, + machine: DeviceId, + dry_run: bool, + cancel: &CancellationToken, +) -> Result { + let report = container.verify.execute(&VerifyCommand { + device_id: machine, + dry_run, + cancel: cancel.clone(), + })?; + + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + render(&mut handle, &report, dry_run).map_err(CoreError::from)?; + Ok(report) +} + +/// Render a [`VerifyReport`] as human-readable lines. +/// +/// WHY the skipped-volume line is unconditional when non-zero rather +/// than tucked behind `--verbose`: it is the difference between "your +/// library is clean" and "I could not look at 400 of your files". A +/// user who prunes on the strength of a partial sweep deletes rows whose +/// files are intact on a drive that simply was not plugged in. +fn render(w: &mut W, r: &VerifyReport, dry_run: bool) -> std::io::Result<()> { + let verb = if dry_run { "would mark" } else { "marked" }; + writeln!(w, "checked {} location(s)", r.checked)?; + if r.newly_missing > 0 { + writeln!(w, "{verb} {} missing", r.newly_missing)?; + } + if r.recovered > 0 { + writeln!(w, "{verb} {} recovered (missing -> active)", r.recovered)?; + } + if r.newly_missing == 0 && r.recovered == 0 { + writeln!(w, "no changes — catalogue matches the filesystem")?; + } + if r.skipped_unmounted > 0 { + writeln!( + w, + "skipped {} location(s) on unmounted volume(s) — NOT checked, \ + status left unchanged", + r.skipped_unmounted, + )?; + } + if !r.completed { + writeln!(w, "cancelled before completion; no changes were written")?; + } + if dry_run && (r.newly_missing > 0 || r.recovered > 0) { + writeln!(w, "(dry run — nothing was written)")?; + } + Ok(()) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] // Test code; unwrap panics signal bugs. +mod tests { + use super::*; + + fn render_to_string(r: &VerifyReport, dry_run: bool) -> String { + let mut buf = Vec::new(); + render(&mut buf, r, dry_run).unwrap(); + String::from_utf8(buf).unwrap() + } + + #[test] + fn clean_sweep_says_so() { + let out = render_to_string( + &VerifyReport { + checked: 12, + completed: true, + ..Default::default() + }, + false, + ); + assert!(out.contains("checked 12")); + assert!(out.contains("no changes")); + assert!( + !out.contains("skipped"), + "no skip line when nothing was skipped", + ); + } + + /// A partial sweep must say so in its own right — the counts alone + /// read as a clean bill of health. + #[test] + fn skipped_unmounted_is_always_surfaced() { + let out = render_to_string( + &VerifyReport { + checked: 78, + skipped_unmounted: 417, + completed: true, + ..Default::default() + }, + false, + ); + assert!(out.contains("417"), "skipped count must be printed"); + assert!(out.contains("NOT checked")); + } + + #[test] + fn dry_run_uses_conditional_wording_and_flags_itself() { + let out = render_to_string( + &VerifyReport { + checked: 3, + newly_missing: 2, + completed: true, + ..Default::default() + }, + true, + ); + assert!(out.contains("would mark 2 missing")); + assert!(out.contains("dry run")); + } + + #[test] + fn live_run_uses_past_tense() { + let out = render_to_string( + &VerifyReport { + checked: 3, + newly_missing: 2, + rows_written: 2, + completed: true, + ..Default::default() + }, + false, + ); + assert!(out.contains("marked 2 missing")); + assert!(!out.contains("dry run")); + } + + #[test] + fn cancellation_is_reported() { + let out = render_to_string( + &VerifyReport { + checked: 5, + completed: false, + ..Default::default() + }, + false, + ); + assert!(out.contains("cancelled")); + assert!(out.contains("no changes were written")); + } +} diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 51e4878..b034945 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -190,6 +190,31 @@ enum Command { /// quota errors, 3 on queue full, 130 on Ctrl-C, 1 otherwise. Transcribe(cmd::transcribe::TranscribeArgs), + /// Reconcile the catalogue against the filesystem. + /// + /// Checks every indexed file on a currently-mounted volume and marks + /// vanished ones `missing` (and restores ones that came back). + /// Locations on unmounted volumes are skipped, never marked missing. + Verify { + /// Report what would change without writing anything. + #[arg(long)] + dry_run: bool, + }, + + /// Remove catalogue entries for files that `verify` found missing. + /// + /// Soft-deletes `file_locations` rows whose status is `missing`. + /// Run `perima verify` first so that set reflects the filesystem as + /// it is now. Requires `--yes` unless `--dry-run` is given. + Prune { + /// Show what would be removed without writing anything. + #[arg(long)] + dry_run: bool, + /// Confirm the removal. Required for a live run. + #[arg(long)] + yes: bool, + }, + /// Manage transcription-provider API keys (keyring entries). /// /// `set` prompts for a key with hidden input; `delete` removes an @@ -300,6 +325,10 @@ async fn main() -> ExitCode { Command::MigrateDataDir(args) => dispatch_migrate_data_dir(&args), + Command::Verify { dry_run } => dispatch_verify(dry_run, &config, &cancel), + + Command::Prune { dry_run, yes } => dispatch_prune(dry_run, yes, &config), + Command::Transcribe(args) => dispatch_transcribe(args, &config, &cancel).await, Command::Auth(args) => dispatch_auth(&args, &config), @@ -987,6 +1016,46 @@ fn dispatch_auth(args: &cmd::auth::AuthArgs, config: &Config) -> ExitCode { } /// Run the `volumes` subcommand. +fn dispatch_verify(dry_run: bool, config: &Config, cancel: &Cancellation) -> ExitCode { + let db_path = config.data_dir.join("perima.db"); + let container = match build_container(&db_path, &config.config_dir, config.device_id, vec![]) { + Ok(c) => c, + Err(e) => { + eprintln!("perima: database: {e}"); + return ExitCode::from(1); + } + }; + match cmd::verify::run(&container, config.device_id, dry_run, &cancel.token()) { + // WHY exit 0 even when files are missing: `verify` reports the + // state of the world; missing files are a finding, not a command + // failure. Scripts that want to act on the count should read the + // output rather than branch on the exit code. + Ok(_) => ExitCode::from(0), + Err(e) => { + eprintln!("perima: {e}"); + ExitCode::from(1) + } + } +} + +fn dispatch_prune(dry_run: bool, yes: bool, config: &Config) -> ExitCode { + let db_path = config.data_dir.join("perima.db"); + let container = match build_container(&db_path, &config.config_dir, config.device_id, vec![]) { + Ok(c) => c, + Err(e) => { + eprintln!("perima: database: {e}"); + return ExitCode::from(1); + } + }; + match cmd::prune::run(&container, config.device_id, dry_run, yes) { + Ok(_) => ExitCode::from(0), + Err(e) => { + eprintln!("perima: {e}"); + ExitCode::from(1) + } + } +} + async fn dispatch_volumes(config: &Config) -> ExitCode { let db_path = config.data_dir.join("perima.db"); let container = match build_container(&db_path, &config.config_dir, config.device_id, vec![]) { diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index e922d7d..46c0239 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -30,8 +30,9 @@ pub use types::{ pub mod ports; pub use ports::{ BackfillFileRow, CacheEntry, CacheKey, FileRepository, FileStat, FileWithMetadataRow, - HashService, IdentityCacheRepository, MetadataRepository, Scanner, SearchRepository, - TagRepository, VolumeRepository, + HashService, IdentityCacheRepository, LocationStatusUpdate, LocationToVerify, + MetadataRepository, Scanner, SearchRepository, TagRepository, VerifyCandidates, + VolumeRepository, }; /// Marker placeholder. Retained as a public symbol for phase-0 diff --git a/crates/core/src/ports/file_repo.rs b/crates/core/src/ports/file_repo.rs index dd2c1cc..0a5a23a 100644 --- a/crates/core/src/ports/file_repo.rs +++ b/crates/core/src/ports/file_repo.rs @@ -4,9 +4,64 @@ use std::path::PathBuf; use crate::{ BlakeHash, CollisionGroup, CoreError, DeviceId, FileLocationRecord, FileUuid, HashedFile, - MediaPath, UpsertOutcome, VolumeId, + LocationStatus, MediaPath, UpsertOutcome, VolumeId, }; +/// One `file_locations` row paired with the absolute path it resolves to +/// on **this** machine. Produced by +/// [`FileRepository::list_locations_for_verify`]. +/// +/// WHY the absolute path is built in the adapter rather than by the +/// caller: reconstructing it requires the `volume_mounts` join, and +/// joining on the wrong machine's mount silently yields a path that +/// belongs to a different computer. Keeping the join inside the query +/// means the verify sweep cannot get it wrong by construction. +#[derive(Debug, Clone)] +pub struct LocationToVerify { + /// Volume the location lives on. + pub volume: VolumeId, + /// Path relative to the volume root. + pub path: MediaPath, + /// Absolute path on this machine (`mount_path` + `relative_path`). + pub absolute_path: PathBuf, + /// Status currently recorded in the database. + /// + /// The sweep compares this against what it observes on disk so it + /// can write only the rows that actually changed. + pub status: LocationStatus, +} + +/// Result of [`FileRepository::list_locations_for_verify`]: the rows the +/// sweep can actually check, plus a count of the ones it cannot. +/// +/// WHY the skipped count travels with the rows instead of being a +/// separate optional query: a sweep that silently omits every location +/// on an unmounted drive reports "checked 78, all present" for a library +/// of 500 and reads as a clean bill of health. Carrying the number the +/// caller is NOT allowed to conclude anything about, in the same value, +/// makes an honest report the path of least resistance. +#[derive(Debug, Clone)] +pub struct VerifyCandidates { + /// Locations on volumes mounted right now on this device. + pub locations: Vec, + /// Non-deleted locations excluded because their volume has no active + /// mount on this device. These are NOT missing — their status must + /// be left exactly as it is. + pub skipped_unmounted: usize, +} + +/// A single `(volume, path) -> status` transition for +/// [`FileRepository::update_location_statuses`]. +#[derive(Debug, Clone)] +pub struct LocationStatusUpdate { + /// Volume the location lives on. + pub volume: VolumeId, + /// Path relative to the volume root. + pub path: MediaPath, + /// Status to write. + pub status: LocationStatus, +} + /// One row returned by [`FileRepository::list_files_needing_backfill`]. /// /// Contains enough information for the backfill worker to compute and @@ -202,4 +257,111 @@ pub trait FileRepository: Send + Sync { let _ = limit; Ok(Vec::new()) } + + /// Return every non-deleted `file_locations` row that sits on a volume + /// **currently mounted on `device`**, paired with its absolute path. + /// + /// Used by `perima_app::VerifyUseCase` to reconcile the catalogue + /// against the filesystem. + /// + /// # The unmounted-volume contract + /// + /// Rows whose volume has no active mount on this device are **not + /// returned at all** — they are not returned with a `None` path for + /// the caller to filter. This is deliberate and load-bearing: the + /// verify sweep marks everything it receives and cannot see as + /// `Missing`, so a row that leaks through for an unplugged drive + /// becomes a false `Missing`, and the prune path then deletes a + /// catalogue whose files are perfectly intact on a disk sitting in a + /// drawer. Absence from this result set is what makes "skip + /// unmounted volumes" unforgettable rather than a rule each caller + /// has to remember. + /// + /// The mount join MUST be constrained to `device` for the same + /// reason — a mount row recorded by a different machine yields a + /// path that is meaningless locally. See #195. + /// + /// # Default implementation + /// + /// Returns empty candidates so non-SQLite adapters compile unchanged. + /// + /// # Errors + /// Adapter-level errors become `CoreError::Internal`. + fn list_locations_for_verify(&self, device: DeviceId) -> Result { + let _ = device; + Ok(VerifyCandidates { + locations: Vec::new(), + skipped_unmounted: 0, + }) + } + + /// Apply many `(volume, path) -> status` transitions in one transaction. + /// + /// Returns the number of rows actually updated. + /// + /// WHY batched rather than a loop over a single-row update: each + /// single-row call is its own `BEGIN IMMEDIATE` round-trip through + /// the writer actor, so a sweep over a large library would open one + /// transaction per changed file. The repo has prior form here — the + /// SQLite lock-order inversion behind #131 was provoked by exactly + /// this kind of write amplification. + /// + /// # Default implementation + /// + /// Returns `Ok(0)` (no-op) so non-SQLite adapters compile unchanged. + /// + /// # Errors + /// Adapter-level errors become `CoreError::Internal`. + fn update_location_statuses( + &self, + updates: &[LocationStatusUpdate], + device: DeviceId, + ) -> Result { + let _ = (updates, device); + Ok(0) + } + + /// Count non-deleted locations currently recorded as `Missing`. + /// + /// This is what a prune would remove. Exposed separately from the + /// delete so a caller can show the number *before* asking the user + /// to confirm, rather than reporting a count after the fact. + /// + /// # Default implementation + /// + /// Returns `Ok(0)` so non-SQLite adapters compile unchanged. + /// + /// # Errors + /// Adapter-level errors become `CoreError::Internal`. + fn count_missing_locations(&self) -> Result { + Ok(0) + } + + /// Soft-delete every non-deleted location recorded as `Missing`, + /// returning the number of rows retired. + /// + /// WHY soft-delete rather than `DELETE FROM`: `file_locations` is a + /// mutable CRDT-replicated row. A hard delete cannot be represented + /// as a merge and would resurrect on the next sync from any peer + /// that still has the row. Setting `deleted_at` is the repo-wide + /// convention for retiring one (see `update_location_path`'s + /// collision branch). + /// + /// # Safety contract + /// + /// This trusts `status = 'missing'` completely — it performs no + /// filesystem check of its own. Whatever set that status decides + /// what gets deleted. `perima_app::VerifyUseCase` is the intended + /// producer, and it never marks rows on unmounted volumes. + /// + /// # Default implementation + /// + /// Returns `Ok(0)` so non-SQLite adapters compile unchanged. + /// + /// # Errors + /// Adapter-level errors become `CoreError::Internal`. + fn soft_delete_missing_locations(&self, device: DeviceId) -> Result { + let _ = device; + Ok(0) + } } diff --git a/crates/core/src/ports/mod.rs b/crates/core/src/ports/mod.rs index 104f081..aa484f7 100644 --- a/crates/core/src/ports/mod.rs +++ b/crates/core/src/ports/mod.rs @@ -11,7 +11,9 @@ pub mod tag_repo; pub mod volume_repo; pub use database_admin::DatabaseAdmin; -pub use file_repo::{BackfillFileRow, FileRepository}; +pub use file_repo::{ + BackfillFileRow, FileRepository, LocationStatusUpdate, LocationToVerify, VerifyCandidates, +}; pub use hash::HashService; pub use identity_cache::{CacheEntry, CacheKey, IdentityCacheRepository}; pub use metadata_repo::{FileWithMetadataRow, MetadataRepository}; diff --git a/crates/db/src/cmd.rs b/crates/db/src/cmd.rs index 69d88a1..a31c333 100644 --- a/crates/db/src/cmd.rs +++ b/crates/db/src/cmd.rs @@ -16,7 +16,8 @@ use flume::Sender; use perima_core::{ BlakeHash, CacheEntry, CacheKey, CoreError, DeviceId, HashedFile, LocationStatus, - MediaMetadata, MediaPath, Tag, UpsertOutcome, VolumeId, VolumeIdentifiers, + LocationStatusUpdate, MediaMetadata, MediaPath, Tag, UpsertOutcome, VolumeId, + VolumeIdentifiers, }; use uuid::Uuid; @@ -298,6 +299,38 @@ pub enum FileWriteCmd { /// Reply channel carrying `rows_changed` (`0` or `1`). reply: ReplyTx, }, + /// Soft-delete every active location whose status is `missing`. + /// + /// WHY a dedicated variant rather than reusing a generic bulk + /// update: this is the only destructive location command, and + /// keeping it named makes it greppable and reviewable on its own. + /// Binds `file_locations.hlc` on the UPDATE. + SoftDeleteMissingLocations { + /// Device that initiated the prune. + device: DeviceId, + /// Reply channel carrying the number of rows retired. + reply: ReplyTx, + }, + /// Apply many location-status transitions in a single transaction. + /// + /// WHY a batch variant alongside the single-row `UpdateLocationStatus`: + /// the verify sweep flips the status of every location it finds + /// missing (or recovered). Routing those through the single-row + /// variant would open one `BEGIN IMMEDIATE` per changed file — the + /// write-amplification shape that provoked the SQLite lock-order + /// inversion in #131. The watcher keeps using the single-row variant + /// because it genuinely handles one filesystem event at a time. + /// + /// Binds `file_locations.hlc` on every UPDATE, exactly as the + /// single-row path does. + UpdateLocationStatuses { + /// Transitions to apply, in order. + updates: Vec, + /// Device that initiated the updates. + device: DeviceId, + /// Reply channel carrying the total `rows_changed` across the batch. + reply: ReplyTx, + }, /// Rename a `file_locations` row and reset status to `active`. /// /// WHY inherent: called by `DbEventHandler` on diff --git a/crates/db/src/file_repo.rs b/crates/db/src/file_repo.rs index cb27694..3752224 100644 --- a/crates/db/src/file_repo.rs +++ b/crates/db/src/file_repo.rs @@ -13,8 +13,8 @@ use flume::Sender; use perima_core::{ BackfillFileRow, BlakeHash, CollisionGroup, CoreError, DeviceId, FileLocationRecord, - FileRepository, FileSize, FileUuid, HashedFile, LocationStatus, MediaPath, UpsertOutcome, - VerifiedState, VolumeId, + FileRepository, FileSize, FileUuid, HashedFile, LocationStatus, LocationStatusUpdate, + LocationToVerify, MediaPath, UpsertOutcome, VerifiedState, VerifyCandidates, VolumeId, }; use rusqlite::{Connection, OptionalExtension}; use uuid::Uuid; @@ -371,6 +371,63 @@ impl FileRepository for SqliteFileRepository { Ok(()) } + fn list_locations_for_verify(&self, device: DeviceId) -> Result { + // WHY a pool checkout (no writer hop): pure SELECT, same as + // `list_file_locations` and `VolumeRepository::list`. + let conn = self.reads.get()?; + list_locations_for_verify_sql(&conn, device) + } + + fn update_location_statuses( + &self, + updates: &[LocationStatusUpdate], + device: DeviceId, + ) -> Result { + // WHY short-circuit before the writer hop: an unchanged sweep is + // the steady state, and a no-op round-trip through the actor + // would still serialise behind every queued write. + if updates.is_empty() { + return Ok(0); + } + let (reply_tx, reply_rx) = flume::bounded::>(1); + self.writer + .send(WriteCmd::File(FileWriteCmd::UpdateLocationStatuses { + updates: updates.to_vec(), + device, + reply: reply_tx, + })) + .map_err(|e| CoreError::Internal(format!("writer send: {e}")))?; + reply_rx + .recv() + .map_err(|e| CoreError::Internal(format!("writer recv: {e}")))? + } + + fn count_missing_locations(&self) -> Result { + let conn = self.reads.get()?; + let n: i64 = conn + .query_row( + "SELECT COUNT(*) FROM file_locations + WHERE status = 'missing' AND deleted_at IS NULL", + [], + |r| r.get(0), + ) + .map_err(Error::from)?; + u64::try_from(n).map_err(|_| CoreError::Internal(format!("count {n} is negative"))) + } + + fn soft_delete_missing_locations(&self, device: DeviceId) -> Result { + let (reply_tx, reply_rx) = flume::bounded::>(1); + self.writer + .send(WriteCmd::File(FileWriteCmd::SoftDeleteMissingLocations { + device, + reply: reply_tx, + })) + .map_err(|e| CoreError::Internal(format!("writer send: {e}")))?; + reply_rx + .recv() + .map_err(|e| CoreError::Internal(format!("writer recv: {e}")))? + } + fn list_files_pending_full_hash(&self, limit: usize) -> Result, CoreError> { // WHY pool-only: pure SELECT; no write needed. let conn = self.reads.get()?; @@ -545,6 +602,124 @@ fn lookup_by_file_uuid_sql( Ok(Some((hash, abs_path, size_bytes))) } +/// SELECT body for [`SqliteFileRepository::list_locations_for_verify`]. +/// +/// Returns one row per active location that resolves to a real absolute +/// path on `device`. +/// +/// # Two joins that must not be relaxed +/// +/// **`vm.machine_id = ?1`** — `volume_mounts` is keyed on +/// `(volume_id, machine_id)`; the same volume mounts at different paths +/// on different machines. Without this predicate the join can pair a +/// local row with another computer's mount path, and the sweep would +/// stat a path that means nothing here. The index +/// `idx_volume_mounts_volume_machine (volume_id, machine_id)` exists for +/// exactly this access pattern. Three older queries in this file omit +/// the predicate — that is #195, deliberately not fixed here so the +/// trait-signature change lands as its own reviewable commit. +/// +/// **`INNER JOIN volume_mounts`** (not `LEFT JOIN`) — an unmounted +/// volume must drop out of the result set entirely rather than surface +/// with a NULL `mount_path`. The sweep marks every row it receives and +/// cannot stat as `Missing`; a row for an unplugged external drive that +/// leaks through becomes a false `Missing`, and prune then deletes a +/// catalogue whose files are intact. Making absence structural means no +/// caller can forget the rule. Sibling queries use `LEFT JOIN` because +/// they want the row regardless and filter later — that is correct for +/// them and wrong here. +fn list_locations_for_verify_sql( + conn: &Connection, + device: DeviceId, +) -> Result { + let sql = " + SELECT fl.volume_id, fl.relative_path, fl.status, vm.mount_path + FROM file_locations fl + JOIN volume_mounts vm + ON vm.volume_id = fl.volume_id + AND vm.machine_id = ?1 + AND vm.deleted_at IS NULL + WHERE fl.deleted_at IS NULL + ORDER BY fl.volume_id, fl.relative_path + "; + let mut stmt = conn + .prepare(sql) + .map_err(Error::from) + .map_err(perima_core::CoreError::from)?; + let rows = stmt + .query_map(rusqlite::params![device.0.to_string()], |row| { + let vol: String = row.get(0)?; + let rel: String = row.get(1)?; + let status: String = row.get(2)?; + let mount: String = row.get(3)?; + Ok((vol, rel, status, mount)) + }) + .map_err(Error::from) + .map_err(perima_core::CoreError::from)?; + + let mut out = Vec::new(); + for row in rows { + let (vol, rel, status, mount) = row + .map_err(Error::from) + .map_err(perima_core::CoreError::from)?; + let volume_uuid = uuid::Uuid::parse_str(&vol) + .map_err(|e| perima_core::CoreError::Internal(format!("parse volume_id: {e}")))?; + // WHY PathBuf::push rather than string concatenation: this must + // match the reconstruction the read path performs in + // `crates/desktop/src/payloads.rs`, which is a platform-sensitive + // push. Concatenating with '/' would produce a path that works on + // Unix and fails the stat on Windows. + let mut absolute_path = std::path::PathBuf::from(mount); + absolute_path.push(&rel); + out.push(LocationToVerify { + volume: VolumeId(volume_uuid), + path: MediaPath::new(&rel), + absolute_path, + status: str_to_status(&status), + }); + } + + // WHY a second COUNT rather than deriving the number from the row + // set: the excluded rows are excluded by the JOIN, so they are not + // observable from `out`. Counting all non-deleted locations and + // subtracting is the only way to report what the sweep could not + // look at. Both statements run on the same pooled read connection + // inside the same implicit read snapshot. + let total: i64 = conn + .query_row( + "SELECT COUNT(*) FROM file_locations WHERE deleted_at IS NULL", + [], + |r| r.get(0), + ) + .map_err(Error::from) + .map_err(perima_core::CoreError::from)?; + let total = usize::try_from(total).unwrap_or(0); + let skipped_unmounted = total.saturating_sub(out.len()); + + Ok(VerifyCandidates { + locations: out, + skipped_unmounted, + }) +} + +/// Parse a `file_locations.status` string back into [`LocationStatus`]. +/// +/// WHY `Active` for an unrecognised value rather than an error: the +/// column is written only by `status_to_str` in the writer, so an +/// unknown string means either a hand-edited database or a future +/// variant this build predates. Failing the whole sweep over one odd row +/// would be worse than treating it as a normal row — the sweep's own +/// stat decides the outcome regardless, so a wrong guess here is +/// self-correcting on the next pass. +fn str_to_status(s: &str) -> perima_core::LocationStatus { + match s { + "missing" => perima_core::LocationStatus::Missing, + "moved" => perima_core::LocationStatus::Moved, + "stale" => perima_core::LocationStatus::Stale, + _ => perima_core::LocationStatus::Active, + } +} + /// SELECT body for [`SqliteFileRepository::list_files_pending_full_hash`]. /// /// Returns `file_uuid` values for every non-deleted `files` row whose diff --git a/crates/db/src/writer/file.rs b/crates/db/src/writer/file.rs index b3e1f45..abcdf42 100644 --- a/crates/db/src/writer/file.rs +++ b/crates/db/src/writer/file.rs @@ -21,8 +21,14 @@ //! - `file_locations.hlc` bumps on INSERT and on UPDATE in //! [`crate::cmd::FileWriteCmd::UpsertLocation`], //! [`crate::cmd::FileWriteCmd::UpdateLocationStatus`], +//! [`crate::cmd::FileWriteCmd::UpdateLocationStatuses`], +//! [`crate::cmd::FileWriteCmd::SoftDeleteMissingLocations`], //! [`crate::cmd::FileWriteCmd::UpdateLocationPath`], and //! [`crate::cmd::FileWriteCmd::MigrateSentinelRow`]. +//! - `UpdateLocationStatuses` binds ONE `hlc` across the whole batch: +//! a verify sweep is a single logical event ("the catalogue was +//! reconciled at time T"), not N independent ones, and every row it +//! touches observed the same filesystem state. //! - The `Unchanged` arm in `UpsertFile` and `UpsertLocation` skips //! every write; no `hlc` is consumed and the prior value is preserved //! (same logical event did not fire). @@ -49,16 +55,16 @@ //! cache-invalidation hint for query-state, not a re-broadcast of //! the underlying filesystem change. //! -//! WHY emit on every variant: all five `FileWriteCmd` variants -//! mutate `files` or `file_locations` — both are read by the -//! frontend file grid + location list. Coarse invalidation is the -//! v1 contract; per-row surgical invalidation is a Batch H decision. +//! WHY emit on every variant: every `FileWriteCmd` variant mutates +//! `files` or `file_locations` — both are read by the frontend file +//! grid + location list. Coarse invalidation is the v1 contract; +//! per-row surgical invalidation is a Batch H decision. use std::sync::Arc; use perima_core::{ AppEvent, BlakeHash, CoreError, DeviceId, EventBus, FileUuid, HashedFile, Hlc, - InvalidationReason, LocationStatus, MediaPath, UpsertOutcome, VolumeId, + InvalidationReason, LocationStatus, LocationStatusUpdate, MediaPath, UpsertOutcome, VolumeId, }; use rusqlite::{Connection, OptionalExtension}; @@ -87,7 +93,8 @@ use crate::errors::Error; // cognitive_complexity above. Splitting the variants into helpers either // inflates parameter counts past `too_many_arguments` or sacrifices the // consume-on-send reply-channel semantics. The arms grow strictly with the -// number of `FileWriteCmd` variants (now 7 post-Task-9). +// number of `FileWriteCmd` variants (now 9, after the verify/prune slice +// added `UpdateLocationStatuses` + `SoftDeleteMissingLocations`). #[allow(clippy::too_many_lines)] pub(super) fn handle(conn: &mut Connection, cmd: FileWriteCmd, bus: &Arc) { // WHY one HLC per command (not per row): the "one HLC per @@ -150,6 +157,33 @@ pub(super) fn handle(conn: &mut Connection, cmd: FileWriteCmd, bus: &Arc { + let out = update_location_statuses_impl(conn, &updates, device, hlc); + // WHY emit gated on rows > 0: a sweep that finds nothing + // changed writes zero rows and must not invalidate every + // file-shaped query index for no reason. + if matches!(&out, Ok(rows) if *rows > 0) { + emit_files_changed(bus, "update_location_statuses"); + } + if reply.send(out).is_err() { + tracing::debug!("file update_location_statuses reply channel closed before send"); + } + } + FileWriteCmd::SoftDeleteMissingLocations { device, reply } => { + let out = soft_delete_missing_locations_impl(conn, device, hlc); + if matches!(&out, Ok(rows) if *rows > 0) { + emit_files_changed(bus, "soft_delete_missing_locations"); + } + if reply.send(out).is_err() { + tracing::debug!( + "file soft_delete_missing_locations reply channel closed before send" + ); + } + } FileWriteCmd::UpdateLocationPath { volume, old_path, @@ -508,6 +542,111 @@ fn update_location_status_impl( u64::try_from(n).map_err(|_| CoreError::Internal(format!("rows_changed {n} is negative"))) } +/// Writer-side body for [`FileWriteCmd::UpdateLocationStatuses`]. +/// +/// Applies every transition inside ONE transaction. Same UPDATE as +/// [`update_location_status_impl`], including the `hlc` binding — the +/// difference is transaction count, not row semantics. +/// +/// WHY a single `hlc` for the whole batch (rather than one per row): the +/// sweep is one logical event ("the catalogue was reconciled against the +/// filesystem at time T"), not N independent ones. Every row it touches +/// observed the same filesystem state, so they share a timestamp. This +/// also keeps the batch atomic under CRDT merge: a peer either sees the +/// whole reconciliation or none of it. +/// +/// Returns the total number of rows updated across the batch. Rows whose +/// `(volume, path)` no longer matches an active location contribute 0 and +/// are not an error — the sweep races against the watcher by nature. +fn update_location_statuses_impl( + conn: &mut Connection, + updates: &[LocationStatusUpdate], + device: DeviceId, + hlc: i64, +) -> Result { + // WHY early return: `BEGIN IMMEDIATE` takes the write lock, so an + // empty sweep would still contend with concurrent writers for no + // reason. A clean library is the common case once steady-state. + if updates.is_empty() { + return Ok(0); + } + + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(Error::from)?; + + let dev_str = device.0.to_string(); + let now = now_iso(); + let mut total: u64 = 0; + { + // WHY a prepared statement hoisted out of the loop: the batch is + // the same UPDATE N times with different bindings; preparing once + // avoids re-parsing the SQL per row. + let mut stmt = tx + .prepare( + "UPDATE file_locations + SET status = ?1, updated_at = ?2, device_id = ?3, hlc = ?4 + WHERE volume_id = ?5 AND relative_path = ?6 AND deleted_at IS NULL", + ) + .map_err(Error::from)?; + for u in updates { + let n = stmt + .execute(rusqlite::params![ + status_to_str(u.status), + now, + dev_str, + hlc, + u.volume.0.to_string(), + u.path.as_str(), + ]) + .map_err(Error::from)?; + total += u64::try_from(n) + .map_err(|_| CoreError::Internal(format!("rows_changed {n} is negative")))?; + } + } + + tx.commit().map_err(Error::from)?; + Ok(total) +} + +/// Writer-side body for [`FileWriteCmd::SoftDeleteMissingLocations`]. +/// +/// Retires every active `missing` location in one statement. +/// +/// WHY `status = 'missing'` is the sole predicate and no filesystem +/// check happens here: the writer actor owns a database connection, not +/// a view of the disk. Deciding what is missing is the verify sweep's +/// job; conflating the two would put a `stat()` inside a write +/// transaction, holding the write lock across filesystem I/O. +/// +/// WHY `deleted_at` rather than `DELETE FROM`: `file_locations` is +/// CRDT-replicated. A hard delete has no merge representation and would +/// resurrect on the next sync from a peer that still holds the row. +/// +/// Returns the number of rows retired. +fn soft_delete_missing_locations_impl( + conn: &mut Connection, + device: DeviceId, + hlc: i64, +) -> Result { + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(Error::from)?; + + let now = now_iso(); + let n = tx + .execute( + "UPDATE file_locations + SET deleted_at = ?1, updated_at = ?1, device_id = ?2, hlc = ?3 + WHERE status = 'missing' AND deleted_at IS NULL", + rusqlite::params![now, device.0.to_string(), hlc], + ) + .map_err(Error::from)?; + + tx.commit().map_err(Error::from)?; + u64::try_from(n).map_err(|_| CoreError::Internal(format!("rows_changed {n} is negative"))) +} + /// Writer-side body for [`FileWriteCmd::UpdateLocationPath`]. Lifted verbatim /// from the pre-Batch-C `SqliteFileRepository::update_location_path` impl with /// `hlc = ?` bound on both branches. From e768d75657cff7f8309d9788ac00fc2336854cd3 Mon Sep 17 00:00:00 2001 From: utof Date: Sat, 1 Aug 2026 21:44:10 +0400 Subject: [PATCH 4/5] feat(prune): remove catalogue entries for files that are gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `perima prune` plus the `verify_locations`, `count_missing_locations` and `prune_missing_locations` Tauri commands, so both shells can act on what a verify sweep found. Prune soft-deletes `file_locations` rows whose status is `missing`. It performs no filesystem check of its own — deciding what is missing needs a `stat()` per file, which must not happen inside a write transaction holding the write lock. That split means a prune is exactly as trustworthy as the sweep before it, which is why both surfaces push the user toward running `verify` first. Soft-delete rather than `DELETE FROM`: `file_locations` is CRDT- replicated, a hard delete has no merge representation, and the row would resurrect on the next sync from any peer that still holds it. Same convention as the `update_location_path` collision branch. ## Confirmation The CLI requires an explicit `--yes` for a live run and otherwise prints the count with a pointer to `--dry-run`. The desktop hides the prune control entirely when nothing is missing rather than showing it disabled — a greyed "Remove 0 missing" invites a click to find out what it does — and requires a second click that restates the number. Prune is also a no-op when the count is zero: an empty destructive write still takes the write lock and still emits an invalidation the frontend would act on. `verify_locations` runs on `spawn_blocking`. The sweep issues one `symlink_metadata` per catalogued file, and a large library would otherwise stall the async runtime thread the Tauri command is polled on. ## IPC arg names All three commands take their arguments under the camelCase keys Tauri converts to, and the `ipc-contract` test covers them. Confirmed the test actually discriminates by flipping `dryRun` to `dry_run` and watching it fail with the exact diagnostic, rather than trusting a green run: verify_locations: expected key "dryRun" (Rust param `dry_run`), api.ts sends [dry_run] That is the #180 bug class, which shipped broken precisely because it was assumed both spellings worked. Verified end-to-end on a real library: verify marks one hidden file missing, prune without `--yes` refuses, `--dry-run` reports 1, `--yes` retires the row (78 active -> 77 active + 1 soft-deleted). Co-Authored-By: Claude Opus 5 (1M context) --- apps/desktop/src/api.ts | 39 ++++++++++ apps/desktop/src/bindings.ts | 26 +++++++ apps/desktop/src/queries/verify.ts | 113 ++++++++++++++++++++++++++++ crates/cli/src/cmd/prune.rs | 115 +++++++++++++++++++++++++++++ crates/desktop/src/commands.rs | 69 +++++++++++++++++ crates/desktop/src/lib.rs | 4 + 6 files changed, 366 insertions(+) create mode 100644 apps/desktop/src/queries/verify.ts create mode 100644 crates/cli/src/cmd/prune.rs diff --git a/apps/desktop/src/api.ts b/apps/desktop/src/api.ts index f943bcc..9cbd949 100644 --- a/apps/desktop/src/api.ts +++ b/apps/desktop/src/api.ts @@ -23,12 +23,14 @@ import type { FileWithMetadataPayload, FileWithTagsPayload, ListProvidersPayload, + PruneReport, ScanReport, SearchHit, Tag, TranscribeStartedPayload, TranscriptionConfig, VolumeRecord, + VerifyReport, } from "./bindings"; // ── Error parsing ───────────────────────────────────────────────────── @@ -466,3 +468,40 @@ export function updateTranscriptionConfig( export function getTranscriptionConfig(): ResultAsync { return fromInvoke("get_transcription_config", {}); } + +/** + * Reconcile catalogued locations against the filesystem. + * + * Marks vanished files `missing` and restores ones that came back. + * + * IMPORTANT for callers: `skipped_unmounted > 0` means the sweep could + * not see part of the library (a volume is not mounted). Those rows were + * left untouched and are NOT missing. Do not present the result as a + * complete picture, and do not offer `pruneMissingLocations` off the + * back of it without saying so — pruning after a partial sweep is still + * safe (skipped rows are never marked missing) but the count shown to + * the user would be misleading. + */ +export function verifyLocations(dryRun: boolean): ResultAsync { + return fromInvoke("verify_locations", { dryRun }); +} + +/** + * Count locations currently marked `missing` — i.e. what a prune would + * remove. Use it to label a confirmation with the real number before the + * user commits. + */ +export function countMissingLocations(): ResultAsync { + return fromInvoke("count_missing_locations", {}); +} + +/** + * Remove catalogue entries for locations marked `missing`. + * + * Destructive: soft-deletes the rows. Trusts `status = missing` and does + * no filesystem check of its own, so run {@link verifyLocations} first or + * the set may be stale. Pass `dryRun` to get the count without writing. + */ +export function pruneMissingLocations(dryRun: boolean): ResultAsync { + return fromInvoke("prune_missing_locations", { dryRun }); +} diff --git a/apps/desktop/src/bindings.ts b/apps/desktop/src/bindings.ts index 4bceb18..803c1de 100644 --- a/apps/desktop/src/bindings.ts +++ b/apps/desktop/src/bindings.ts @@ -315,6 +315,32 @@ export type ScanReport = { volume_label: string | null; }; +/** + * Outcome of a location verify sweep. + * Rust: `crates/app/src/verify.rs::VerifyReport`. + * + * `skipped_unmounted` counts locations on volumes not mounted on this + * device. They were NOT checked and their status was NOT changed — a + * non-zero value means this report describes only part of the library. + */ +export type VerifyReport = { + checked: number; + newly_missing: number; + recovered: number; + skipped_unmounted: number; + rows_written: number; + completed: boolean; +}; + +/** + * Outcome of a prune. + * Rust: `crates/app/src/verify.rs::PruneReport`. + */ +export type PruneReport = { + missing_found: number; + rows_pruned: number; +}; + /** * File location joined with extracted media metadata. * Rust: `crates/desktop/src/payloads.rs::FileWithMetadataPayload`. diff --git a/apps/desktop/src/queries/verify.ts b/apps/desktop/src/queries/verify.ts new file mode 100644 index 0000000..fda2487 --- /dev/null +++ b/apps/desktop/src/queries/verify.ts @@ -0,0 +1,113 @@ +/** + * TanStack Query hooks for the location verify sweep + prune. + * + * WHY both mutations invalidate the file list: verify rewrites + * `file_locations.status` and prune soft-deletes rows, so every + * file-shaped query is stale afterwards. The backend also emits + * `IndexInvalidated::FilesChanged`, but that is a coarse hint on a + * channel the frontend treats as advisory — invalidating here keeps the + * UI correct even if the event is dropped. + */ + +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; + +import * as api from "../api"; +import type { PruneReport, VerifyReport } from "../bindings"; +import { useUiStore } from "../stores/ui"; + +export const verifyKeys = { + all: ["verify"] as const, + missingCount: ["verify", "missing-count"] as const, +}; + +/** Live count of locations marked `missing` — what a prune would remove. */ +export function useMissingCount() { + return useQuery({ + queryKey: verifyKeys.missingCount, + queryFn: async () => + api.countMissingLocations().match( + (n) => n, + (err) => { + // eslint-disable-next-line @typescript-eslint/only-throw-error + throw err; + }, + ), + }); +} + +/** + * Run a verify sweep. + * + * WHY the notification distinguishes a partial sweep: when + * `skipped_unmounted > 0` the sweep could not see part of the library, + * and reporting only "0 missing" would read as a clean bill of health + * over files it never looked at. + */ +export function useVerifyLocations() { + const notify = useUiStore((s) => s.notify); + const qc = useQueryClient(); + return useMutation({ + mutationKey: [...verifyKeys.all, "run"], + mutationFn: async (dryRun: boolean) => + api.verifyLocations(dryRun).match( + (r) => r, + (err) => { + // eslint-disable-next-line @typescript-eslint/only-throw-error + throw err; + }, + ), + onSuccess: (r: VerifyReport) => { + void qc.invalidateQueries({ queryKey: verifyKeys.missingCount }); + void qc.invalidateQueries({ queryKey: ["files"] }); + // WHY "info" even when files are missing: `NotificationKind` is + // "info" | "error", and a missing file is a finding, not a failure + // — the sweep did exactly what it was asked. Flagging it as an + // error would train the user to dismiss the surface that carries + // the skipped-volume caveat. The message itself leads with the + // count, and the file rows badge themselves. + notify("info", verifyMessage(r)); + }, + }); +} + +/** Human-readable summary of a {@link VerifyReport}. */ +export function verifyMessage(r: VerifyReport): string { + const parts: string[] = [`Checked ${String(r.checked)} file(s)`]; + if (r.newly_missing > 0) parts.push(`${String(r.newly_missing)} now missing`); + if (r.recovered > 0) parts.push(`${String(r.recovered)} recovered`); + if (r.newly_missing === 0 && r.recovered === 0) parts.push("no changes"); + if (r.skipped_unmounted > 0) { + parts.push( + `${String(r.skipped_unmounted)} skipped on unmounted volume(s) — not checked`, + ); + } + if (!r.completed) parts.push("cancelled; nothing written"); + return `${parts.join(" · ")}.`; +} + +/** Soft-delete every location marked `missing`. Destructive. */ +export function usePruneMissing() { + const notify = useUiStore((s) => s.notify); + const qc = useQueryClient(); + return useMutation({ + mutationKey: [...verifyKeys.all, "prune"], + mutationFn: async (dryRun: boolean) => + api.pruneMissingLocations(dryRun).match( + (r) => r, + (err) => { + // eslint-disable-next-line @typescript-eslint/only-throw-error + throw err; + }, + ), + onSuccess: (r: PruneReport) => { + void qc.invalidateQueries({ queryKey: verifyKeys.missingCount }); + void qc.invalidateQueries({ queryKey: ["files"] }); + notify( + "info", + r.rows_pruned > 0 + ? `Removed ${String(r.rows_pruned)} missing file(s) from the library.` + : "Nothing to remove — no files are marked missing.", + ); + }, + }); +} diff --git a/crates/cli/src/cmd/prune.rs b/crates/cli/src/cmd/prune.rs new file mode 100644 index 0000000..a0fbb7f --- /dev/null +++ b/crates/cli/src/cmd/prune.rs @@ -0,0 +1,115 @@ +//! `perima prune` — thin delegator to [`perima_app::PruneUseCase`]. + +use std::io::Write; + +use perima_app::{AppContainer, PruneCommand, PruneReport}; +use perima_core::{CoreError, DeviceId}; + +/// Execute `prune`. +/// +/// Retires catalogue entries for files a previous `perima verify` found +/// missing. Requires `--yes` for a live run — see [`run`]'s WHY below. +/// +/// # Errors +/// Propagates `CoreError` from the `UseCase` / repository. +pub(crate) fn run( + container: &AppContainer, + machine: DeviceId, + dry_run: bool, + yes: bool, +) -> Result { + let missing = container.prune.count_missing()?; + + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + + // WHY a confirmation gate on the live path (and not on `verify`): + // prune is the only destructive command in this pair, and the rows + // it removes are exactly the rows the user can no longer see on + // disk to sanity-check against. Requiring an explicit `--yes` means + // a mistyped command cannot silently retire a catalogue. `--dry-run` + // is the discoverable way to see the number first. + if !dry_run && !yes && missing > 0 { + writeln!( + handle, + "{missing} location(s) are marked missing.\n\ + Re-run with --yes to remove them, or --dry-run to inspect first.\n\ + Tip: run `perima verify` first so the missing set reflects the \ + filesystem as it is now.", + ) + .map_err(CoreError::from)?; + return Ok(PruneReport { + missing_found: missing, + rows_pruned: 0, + }); + } + + let report = container.prune.execute(&PruneCommand { + device_id: machine, + dry_run, + })?; + render(&mut handle, &report, dry_run).map_err(CoreError::from)?; + Ok(report) +} + +/// Render a [`PruneReport`] as human-readable lines. +fn render(w: &mut W, r: &PruneReport, dry_run: bool) -> std::io::Result<()> { + if r.missing_found == 0 { + writeln!(w, "nothing to prune — no locations are marked missing")?; + return Ok(()); + } + if dry_run { + writeln!( + w, + "would remove {} location(s) marked missing (dry run — nothing was written)", + r.missing_found, + )?; + } else { + writeln!(w, "removed {} location(s) marked missing", r.rows_pruned)?; + } + Ok(()) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] // Test code; unwrap panics signal bugs. +mod tests { + use super::*; + + fn s(r: &PruneReport, dry_run: bool) -> String { + let mut buf = Vec::new(); + render(&mut buf, r, dry_run).unwrap(); + String::from_utf8(buf).unwrap() + } + + #[test] + fn empty_catalogue_says_nothing_to_prune() { + let out = s(&PruneReport::default(), false); + assert!(out.contains("nothing to prune")); + } + + #[test] + fn dry_run_is_conditional_and_labelled() { + let out = s( + &PruneReport { + missing_found: 7, + rows_pruned: 0, + }, + true, + ); + assert!(out.contains("would remove 7")); + assert!(out.contains("dry run")); + } + + #[test] + fn live_run_reports_rows_actually_removed() { + let out = s( + &PruneReport { + missing_found: 7, + rows_pruned: 7, + }, + false, + ); + assert!(out.contains("removed 7")); + assert!(!out.contains("dry run")); + } +} diff --git a/crates/desktop/src/commands.rs b/crates/desktop/src/commands.rs index 6c700a1..156b985 100644 --- a/crates/desktop/src/commands.rs +++ b/crates/desktop/src/commands.rs @@ -1956,3 +1956,72 @@ pub async fn get_transcription_config( .ok_or_else(|| CoreError::Internal("data_dir has no parent (config_dir)".into()))?; TranscriptionConfig::load(config_dir) } + +// --------------------------------------------------------------------------- +// Location verification + prune +// --------------------------------------------------------------------------- + +/// Reconcile catalogued locations against the filesystem. +/// +/// Marks vanished files `missing` and restores ones that came back. +/// Locations on volumes not mounted on this device are skipped entirely +/// and reported via `VerifyReport::skipped_unmounted` — they are never +/// marked missing (an unplugged drive must not empty the library). +/// +/// # Errors +/// Propagates `CoreError` from the repository. +#[tauri::command] +#[specta::specta] +pub async fn verify_locations( + dry_run: bool, + state: tauri::State<'_, AppState>, +) -> Result { + // WHY `spawn_blocking`: the sweep issues one `symlink_metadata` + // syscall per catalogued file. On a large library that is a long + // run of blocking filesystem I/O, which would stall the async + // runtime thread the Tauri command is polled on. + let verify = Arc::clone(&state.container.verify); + let device_id = state.device_id; + tokio::task::spawn_blocking(move || { + verify.execute(&perima_app::VerifyCommand { + device_id, + dry_run, + cancel: tokio_util::sync::CancellationToken::new(), + }) + }) + .await + .map_err(|e| CoreError::Internal(format!("verify task join: {e}")))? +} + +/// Count locations currently marked `missing`. +/// +/// Lets the UI label a destructive confirm with the real number before +/// the user commits to it. +/// +/// # Errors +/// Propagates `CoreError` from the repository. +#[tauri::command] +#[specta::specta] +pub async fn count_missing_locations(state: tauri::State<'_, AppState>) -> Result { + state.container.prune.count_missing() +} + +/// Remove catalogue entries for locations marked `missing`. +/// +/// Soft-deletes the rows (CRDT-replicated data cannot be hard-deleted). +/// Trusts `status = missing` and performs no filesystem check of its +/// own — callers should run [`verify_locations`] first. +/// +/// # Errors +/// Propagates `CoreError` from the repository. +#[tauri::command] +#[specta::specta] +pub async fn prune_missing_locations( + dry_run: bool, + state: tauri::State<'_, AppState>, +) -> Result { + state.container.prune.execute(&perima_app::PruneCommand { + device_id: state.device_id, + dry_run, + }) +} diff --git a/crates/desktop/src/lib.rs b/crates/desktop/src/lib.rs index 19b61cc..2c2ab3b 100644 --- a/crates/desktop/src/lib.rs +++ b/crates/desktop/src/lib.rs @@ -151,6 +151,10 @@ pub fn run() -> Result<(), RunError> { commands::list_providers, commands::update_transcription_config, commands::get_transcription_config, + // Location verification + prune. + commands::verify_locations, + commands::count_missing_locations, + commands::prune_missing_locations, ]); // Export TypeScript bindings when the `specta-export` feature is From d1cbd151249bba4774274038fbf73953a8436b14 Mon Sep 17 00:00:00 2001 From: utof Date: Sat, 1 Aug 2026 21:44:50 +0400 Subject: [PATCH 5/5] feat(ui): make a missing file visible in the file list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catalogue has tracked whether a file is still on disk since v1, and `payloads.rs` has been shipping that status to the frontend the whole time — where `FileTable` rendered it as bare lowercase text in the fifth of six columns, visually identical to every other cell. The information was present and invisible. `LocationStatusBadge` gives the abnormal states a treatment and leaves `active` muted. Colouring the normal case too would drown the one row that matters, since almost every row is active. Missing rows also drop to 50% opacity across the whole row: the status column is easy to miss when scanning by filename, and desaturating the row makes "not on disk" legible peripherally while the badge carries the precise verdict. Opacity only — the row stays selectable so it can be inspected. Meaning is carried in text as well as colour (`sr-only` description per state), so the signal survives assistive tech and colour blindness. An unrecognised status renders neutrally rather than blanking the cell — `status` is a plain text column in SQLite and a future backend variant must not break the table. `LibraryHealthPill` in the status bar exposes the two actions. They stay separate buttons rather than one "clean up": verify is safe and repeatable, prune deletes rows, and fusing them would mean a single click both decides what is missing and acts on it with no moment to look at the number. The prune control is absent, not disabled, when nothing is missing. The verify notification always states `skipped_unmounted` when non-zero. A sweep that could not see an unmounted drive would otherwise report "checked 0 · no changes" and read as a healthy library — which is exactly what happened on the first live run here, where CLI and desktop turned out to hold different device ids (#192) and every location was skipped. That surfaced only because the count is mandatory. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/LocationStatusBadge.test.tsx | 70 ++++++++++++++ .../src/__tests__/verify-message.test.ts | 59 ++++++++++++ apps/desktop/src/components/FileTable.tsx | 13 ++- .../src/components/LibraryHealthPill.tsx | 95 +++++++++++++++++++ .../src/components/LocationStatusBadge.tsx | 78 +++++++++++++++ apps/desktop/src/components/StatusBar.tsx | 2 + 6 files changed, 316 insertions(+), 1 deletion(-) create mode 100644 apps/desktop/src/__tests__/LocationStatusBadge.test.tsx create mode 100644 apps/desktop/src/__tests__/verify-message.test.ts create mode 100644 apps/desktop/src/components/LibraryHealthPill.tsx create mode 100644 apps/desktop/src/components/LocationStatusBadge.tsx diff --git a/apps/desktop/src/__tests__/LocationStatusBadge.test.tsx b/apps/desktop/src/__tests__/LocationStatusBadge.test.tsx new file mode 100644 index 0000000..b979346 --- /dev/null +++ b/apps/desktop/src/__tests__/LocationStatusBadge.test.tsx @@ -0,0 +1,70 @@ +/** + * Tests for {@link LocationStatusBadge} + {@link isUnavailable}. + * + * The point of the component is that an abnormal status is *visibly* + * different from a normal one, so these assert on the distinction rather + * than on exact class strings — pinning Tailwind classes would make the + * test a change-detector for restyling. + */ + +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { + LocationStatusBadge, + isUnavailable, +} from "../components/LocationStatusBadge"; + +describe("LocationStatusBadge", () => { + it("renders the status text for every known state", () => { + for (const s of ["active", "missing", "moved", "stale"]) { + // WHY getAllByText: the badge nests an `sr-only` span inside the + // visible one, so the outer element's text also contains the + // label. Two matches is the correct shape, not a duplicate render + // — assert the label is present rather than unique. + const { container, unmount } = render(); + expect(screen.getAllByText(s, { exact: false }).length).toBeGreaterThan(0); + expect(container.textContent).toContain(s); + unmount(); + } + }); + + it("gives missing a visual treatment that active does not have", () => { + const { container: activeC } = render( + , + ); + const activeCls = activeC.querySelector("span")?.className ?? ""; + const { container: missingC } = render( + , + ); + const missingCls = missingC.querySelector("span")?.className ?? ""; + + expect(missingCls).not.toEqual(activeCls); + expect(missingCls).toMatch(/destructive/); + expect(activeCls).not.toMatch(/destructive/); + }); + + it("carries the meaning in text, not only in colour", () => { + render(); + // Screen-reader text must state the condition; a colour-only signal + // is invisible to assistive tech and to colour-blind users. + expect(screen.getByText(/missing from disk/i)).toBeInTheDocument(); + }); + + it("renders an unknown status neutrally instead of blanking the cell", () => { + const { container } = render(); + expect(container.textContent).toContain("quarantined"); + expect(container.querySelector("span")?.className ?? "").not.toMatch( + /destructive/, + ); + }); +}); + +describe("isUnavailable", () => { + it("is true only for missing", () => { + expect(isUnavailable("missing")).toBe(true); + for (const s of ["active", "moved", "stale", "anything-else"]) { + expect(isUnavailable(s)).toBe(false); + } + }); +}); diff --git a/apps/desktop/src/__tests__/verify-message.test.ts b/apps/desktop/src/__tests__/verify-message.test.ts new file mode 100644 index 0000000..9c88695 --- /dev/null +++ b/apps/desktop/src/__tests__/verify-message.test.ts @@ -0,0 +1,59 @@ +/** + * Tests for {@link verifyMessage}. + * + * The load-bearing case is the partial sweep: when a volume is not + * mounted, its rows are neither checked nor marked, and a message that + * reports only "no changes" would read as a clean bill of health over + * files the sweep never looked at. + */ + +import { describe, expect, it } from "vitest"; + +import type { VerifyReport } from "../bindings"; +import { verifyMessage } from "../queries/verify"; + +const report = (over: Partial = {}): VerifyReport => ({ + checked: 0, + newly_missing: 0, + recovered: 0, + skipped_unmounted: 0, + rows_written: 0, + completed: true, + ...over, +}); + +describe("verifyMessage", () => { + it("reports a clean sweep as no changes", () => { + const m = verifyMessage(report({ checked: 42 })); + expect(m).toContain("Checked 42"); + expect(m).toContain("no changes"); + expect(m).not.toContain("skipped"); + }); + + it("always surfaces skipped unmounted locations", () => { + const m = verifyMessage(report({ checked: 78, skipped_unmounted: 417 })); + expect(m).toContain("417"); + expect(m).toContain("not checked"); + }); + + it("never claims 'no changes' alone when rows were skipped", () => { + // A sweep that saw nothing because everything was on an unplugged + // drive must not read as a healthy library. + const m = verifyMessage(report({ checked: 0, skipped_unmounted: 500 })); + expect(m).toContain("500"); + expect(m).toMatch(/not checked/); + }); + + it("reports missing and recovered counts", () => { + const m = verifyMessage(report({ checked: 10, newly_missing: 3, recovered: 2 })); + expect(m).toContain("3 now missing"); + expect(m).toContain("2 recovered"); + expect(m).not.toContain("no changes"); + }); + + it("flags a cancelled sweep as having written nothing", () => { + const m = verifyMessage(report({ checked: 5, completed: false })); + expect(m).toContain("cancelled"); + expect(m).toContain("nothing written"); + }); +}); diff --git a/apps/desktop/src/components/FileTable.tsx b/apps/desktop/src/components/FileTable.tsx index 819b0d7..848699a 100644 --- a/apps/desktop/src/components/FileTable.tsx +++ b/apps/desktop/src/components/FileTable.tsx @@ -1,6 +1,7 @@ import { useState } from "react"; import type { FileWithTagsPayload, Tag } from "../bindings"; import TagChip from "./TagChip"; +import { LocationStatusBadge, isUnavailable } from "./LocationStatusBadge"; import { useAttachTag, useAttachTagByUuid, @@ -208,7 +209,15 @@ export default function FileTable({ files, loading }: FileTableProps) { selectedFileUuid === f.file_uuid ? null : f.file_uuid, ); }} + // WHY dim the whole row rather than only badge the cell: + // the status column is one of six and easy to miss when + // scanning by filename. Desaturating the entire row makes + // "this file is not on disk" legible peripherally, while the + // badge carries the precise verdict. Opacity only — the row + // stays selectable so the user can inspect or prune it. className={`border-b border-border cursor-pointer transition-colors duration-micro ${ + isUnavailable(f.status) ? "opacity-50" : "" + } ${ selectedFileUuid === f.file_uuid ? "bg-accent text-accent-foreground" : "hover:bg-muted" @@ -231,7 +240,9 @@ export default function FileTable({ files, loading }: FileTableProps) { {f.relative_path} - {f.status} + + +