diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index 4f68ec88..9bf0e301 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -91,6 +91,17 @@ USAGE: #[global_allocator] static ALLOC: dhat::Alloc = dhat::Alloc; +/// Why `--library DIR` can't be opened; for a library open in another program, how to work with +/// that one instead (issue #99). +fn library_error(dir: &str, e: lightcraft_engine::EngineError) -> String { + match e { + lightcraft_engine::EngineError::LibraryInUse(why) => format!( + "{dir}: {why}\nTo work with the library while the app has it open, start the app with `--control PORT` and use `lightcraft-cli mcp --connect 127.0.0.1:PORT`." + ), + e => format!("{dir}: {e}"), + } +} + fn main() -> ExitCode { // `--features dhat-heap`: count allocations; the profile is written when `_heap` drops // (LIGHTCRAFT_DHAT_FILE, default dhat-heap.json). @@ -182,7 +193,7 @@ fn mcp(args: &[String]) -> Result<(), String> { let mut h = match &library { Some(dir) => { let mut h = Headless::default(); - let r = h.session.open_library(dir, demo).map_err(|e| e.to_string())?; + let r = h.session.open_library(dir, demo).map_err(|e| library_error(dir, e))?; eprintln!("lightcraft-cli mcp: opened library {dir} ({r:?})"); h } @@ -419,7 +430,7 @@ fn run(args: &[String]) -> Result<(), String> { let mut h = match &library { Some(dir) => { let mut h = Headless::default(); - h.session.open_library(dir, demo).map_err(|e| e.to_string())?; + h.session.open_library(dir, demo).map_err(|e| library_error(dir, e))?; h } None if demo => Headless::demo(), @@ -533,11 +544,17 @@ fn render(args: &[String]) -> Result<(), String> { .ok_or_else(|| format!("{output}: unknown extension (use .jpg .png .tif .webp .avif .dng or --opt format=…)"))?; } let e = export_photo(&mut s, lightcraft_engine::catalog::PhotoId(id), &o, 1)?; - std::fs::write(&output, &e.bytes).map_err(|err| format!("{output}: {err}"))?; - for (ext, bytes) in &e.sidecars { - let sc = Path::new(&output).with_extension(ext); - std::fs::write(&sc, bytes).map_err(|err| format!("{}: {err}", sc.display()))?; - eprintln!("lightcraft-cli: wrote {}", sc.display()); + // never over the input (or its sidecar), however it is spelled: `render IMG.jpg -o IMG.jpg` + let sidecars: Vec<(String, &Vec)> = + e.sidecars.iter().map(|(ext, bytes)| (Path::new(&output).with_extension(ext).to_string_lossy().to_string(), bytes)).collect(); + let guard = s.original_guard(); + for p in std::iter::once(&output).chain(sidecars.iter().map(|(p, _)| p)) { + guard.check(Path::new(p)).map_err(|err| format!("render: {err}"))?; + } + lightcraft_engine::export::write_file(&output, &e.bytes)?; + for (sc, bytes) in &sidecars { + lightcraft_engine::export::write_file(sc, bytes)?; + eprintln!("lightcraft-cli: wrote {sc}"); } eprintln!("lightcraft-cli: wrote {output} ({}×{})", e.width, e.height); Ok(()) @@ -580,7 +597,7 @@ fn snapshot(args: &[String]) -> Result<(), String> { let mut session = match &library { Some(dir) => { let mut s = Session::new().with_fs(); - s.open_library(dir, false).map_err(|e| format!("{dir}: {e}"))?; + s.open_library(dir, false).map_err(|e| library_error(dir, e))?; s } None if files.is_empty() => Session::with_demo().with_fs(), @@ -593,18 +610,8 @@ fn snapshot(args: &[String]) -> Result<(), String> { eprintln!("lightcraft-cli snapshot: imported {n} files in {:.0} ms", ti.elapsed().as_secs_f64() * 1e3); } let services = lightcraft_ui_egui::Services { - write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| { - if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| format!("{p}: {e}")) - })), - write: Some(Box::new(|p: &str, b: &[u8]| { - if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| format!("{p}: {e}")) - })), + write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)), + write: Some(Box::new(lightcraft_engine::export::write_file)), png: Some(Box::new(|img: &lightcraft_raster::Rgba8| { lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default() })), diff --git a/apps/lightcraft-cli/tests/cli.rs b/apps/lightcraft-cli/tests/cli.rs index 913200d2..5a4b7aab 100644 --- a/apps/lightcraft-cli/tests/cli.rs +++ b/apps/lightcraft-cli/tests/cli.rs @@ -86,6 +86,24 @@ fn render_subcommand() { assert!(String::from_utf8_lossy(&o.stderr).contains("unknown control")); } +/// Issue #93: `render IMG -o IMG` replaced its own input with the render. +#[test] +fn render_refuses_to_overwrite_its_input() { + let input = tmp("self-in.png"); + gradient_png(&input); + let before = std::fs::read(&input).unwrap(); + let dir = input.parent().unwrap(); + std::fs::create_dir_all(dir.join("sub")).unwrap(); + let other_spelling = dir.join("sub/../self-in.png"); + for out in [&input, &other_spelling] { + let o = + Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", "light.exposure=1"]).output().unwrap(); + assert!(!o.status.success()); + assert!(String::from_utf8_lossy(&o.stderr).contains("never writes over an original"), "{}", String::from_utf8_lossy(&o.stderr)); + } + assert_eq!(std::fs::read(&input).unwrap(), before, "the input is untouched"); +} + #[test] fn render_export_options() { let input = tmp("o-in.png"); @@ -242,3 +260,31 @@ fn devices_are_listed_and_imported_from() { let line: Value = serde_json::from_slice(o.stdout.split(|b| *b == b'\n').next().unwrap()).unwrap(); assert_eq!(line["result"]["candidates"].as_array().map(Vec::len), Some(1), "{line}"); } + +/// Issue #99: a library open in one process (here `mcp --library`) is refused by a second one, +/// with who has it and how to drive the running app instead; free again once the first exits. +#[test] +fn a_library_open_in_another_process_is_refused() { + let lib = tmp("locked-lib"); + let _ = std::fs::remove_dir_all(&lib); + let lib_s = lib.to_str().unwrap(); + let mut holder = + Command::new(BIN).args(["mcp", "--library", lib_s]).stdin(Stdio::piped()).stdout(Stdio::null()).stderr(Stdio::piped()).spawn().unwrap(); + let mut err = BufReader::new(holder.stderr.take().unwrap()); + let mut line = String::new(); + while !line.contains("opened library") { + line.clear(); + assert!(err.read_line(&mut line).unwrap() > 0, "mcp exited before opening the library"); + } + let (ok, _, stderr) = run_cli(&["--library", lib_s, "library.info"], None); + assert!(!ok); + assert!(stderr.contains("already open in lightcraft-cli") && stderr.contains(&format!("process {}", holder.id())), "{stderr}"); + assert!(stderr.contains("mcp --connect"), "{stderr}"); + + drop(holder.stdin.take()); // EOF: the server exits and lets go of the library + assert!(holder.wait().unwrap().success()); + let (ok, lines, stderr) = run_cli(&["--library", lib_s, "library.info"], None); + assert!(ok, "{stderr}"); + assert_eq!(lines[0]["ok"], true); + let _ = std::fs::remove_dir_all(&lib); +} diff --git a/apps/lightcraft-web/src/web.rs b/apps/lightcraft-web/src/web.rs index 09a19595..56b2868b 100644 --- a/apps/lightcraft-web/src/web.rs +++ b/apps/lightcraft-web/src/web.rs @@ -295,6 +295,7 @@ impl WebApp { files: Box::new(files.store()), on_disk: false, }; + let mut problem = None; match session.open_library_in(stores, true).cloned() { Ok(r) => log::info!( "lightcraft: library {} in {:.0} ms ({} photos; snapshot seq {}, {} ops replayed)", @@ -305,9 +306,17 @@ impl WebApp { r.replayed ), Err(e) => { - log::error!("opening the library failed: {e}; starting a temporary one"); - session = Session::with_demo(); + // never a silent demo session (issue #100): an empty one, and the window says why + log::error!("opening the library failed: {e}"); + session = Session::new(); originals.install(&mut session); + problem = Some(lightcraft_ui_egui::panels::library_problem::LibraryProblem { + can_retry: false, + ..lightcraft_ui_egui::panels::library_problem::LibraryProblem::new( + "the browser's storage for this page", + format!("{e}. Reload the page to try again."), + ) + }); } } // previews are large (≤ 2560 px, f32): keep few in a 32-bit address space @@ -318,6 +327,7 @@ impl WebApp { app.ui = ui; } app.ui = app.ui.sanitized(); + app.library_problem = problem; let n = opts.workers.unwrap_or_else(|| { let cores = window().map_or(1, |w| w.navigator().hardware_concurrency() as usize); cores.saturating_sub(1).clamp(1, 4) diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index c7a67e69..f6ac2f53 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -7,7 +7,9 @@ //! `~/Pictures/LightCraft Library`; a new library starts with the //! procedural demo photos unless `--no-demo` or files are given. Files and folders on the command //! line are imported (duplicates are skipped). `--memory` runs an in-memory session that writes -//! nothing (demo photos unless files are given; used by the README showcase scripts). +//! nothing (demo photos unless files are given; used by the README showcase scripts). A library +//! that can't be opened is never replaced silently: the window says why and asks what to do +//! (`lightcraft_ui_egui::panels::library_problem`). //! //! `--control ` (or `LIGHTCRAFT_CONTROL_PORT`) starts a localhost JSON-lines control server: //! `{"id":1,"method":"ui.inspect","params":{}}` → `{"id":1,"ok":true,"result":…}`. @@ -26,6 +28,7 @@ mod control_server; mod native_menu; use lightcraft_engine::Session; +use lightcraft_ui_egui::panels::library_problem::LibraryProblem; use lightcraft_ui_egui::{LightcraftApp, Services, UiState}; /// Reverse-DNS app id: Wayland app id, `.desktop` file and hicolor icon name. @@ -307,33 +310,28 @@ fn services() -> Services { .save_file() .map(|p| p.to_string_lossy().to_string()) })), - write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| { - if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| e.to_string()) - })), - write: Some(Box::new(|p: &str, b: &[u8]| { - if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| e.to_string()) - })), + // atomic (temp file + sync + rename): a failed write never leaves a truncated file + write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)), + write: Some(Box::new(lightcraft_engine::export::write_file)), png: Some(Box::new(|img: &lightcraft_raster::Rgba8| { lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default() })), } } -/// The persistent library session (or an in-memory one with `--memory` / if the library can't open). -fn open_session(in_memory: bool, dir: Option, seed_demo: bool) -> Session { - let fallback = || if seed_demo { Session::with_demo() } else { Session::new() }.with_fs().with_system_clock(); +/// The persistent library session, or with `--memory` an in-memory one (demo photos). +/// +/// If the library can't be opened the session is empty and in memory — never seeded with the +/// demo photos, never written anywhere — and the problem is returned: the window then says so +/// and offers Try Again / Choose Another Library… / Continue Without Saving / Quit (issue #100). +fn open_session(in_memory: bool, dir: Option, seed_demo: bool) -> (Session, Option) { if in_memory { - return fallback(); + return (if seed_demo { Session::with_demo() } else { Session::new() }.with_fs().with_system_clock(), None); } + let unopened = || Session::new().with_fs().with_system_clock(); let Some(dir) = dir else { - eprintln!("lightcraft: no library location (set --library or LIGHTCRAFT_LIBRARY); running in memory"); - return fallback(); + eprintln!("lightcraft: no library location (set --library or LIGHTCRAFT_LIBRARY)"); + return (unopened(), Some(LibraryProblem::new("", "There is no home folder to keep the library in. Choose a folder for it."))); }; let t0 = std::time::Instant::now(); let mut s = Session::new().with_fs().with_system_clock(); @@ -347,11 +345,11 @@ fn open_session(in_memory: bool, dir: Option, seed_demo: boo if torn > 0 { ", torn tail repaired" } else { "" }, t0.elapsed().as_secs_f64() * 1000.0 ); - s + (s, None) } Err(e) => { - eprintln!("lightcraft: can't open library {}: {e}; running in memory", dir.display()); - fallback() + eprintln!("lightcraft: can't open library {}: {e}", dir.display()); + (unopened(), Some(LibraryProblem::new(dir.to_string_lossy(), e.to_string()))) } } } @@ -428,7 +426,7 @@ fn main() -> eframe::Result { "LightCraft", options, Box::new(move |cc| { - let session = open_session(in_memory, library_dir, seed_demo && files.is_empty()); + let (session, problem) = open_session(in_memory, library_dir, seed_demo && files.is_empty()); let mut app = LightcraftApp::new(session, services()); if let Some(ui) = prefs { app.ui = ui; @@ -446,7 +444,11 @@ fn main() -> eframe::Result { let rx = control_server::start(port, cc.egui_ctx.clone()); app = app.with_control(rx); } - if !files.is_empty() { + if let Some(mut p) = problem { + // imported once the user has chosen where (into the library, or the temporary session) + p.pending_import = files; + app.library_problem = Some(p); + } else if !files.is_empty() { let _ = app.run("library.import", serde_json::json!({"paths": files})); app.ui.view = lightcraft_ui_egui::state::ViewMode::PhotoGrid; } diff --git a/crates/catalog/src/journal.rs b/crates/catalog/src/journal.rs index adc8348d..96187cfa 100644 --- a/crates/catalog/src/journal.rs +++ b/crates/catalog/src/journal.rs @@ -1,8 +1,9 @@ //! Crash-safe catalog persistence: an append-only op log plus periodic snapshots. //! //! Files (in a [`Store`]): -//! - `catalog.snap` — `{"format":"lightcraft-catalog","version":1,"seq":N,"catalog":{…}}`, the -//! state after op `N`; replaced atomically (temp + fsync + rename). +//! - `catalog.snap` — `{"format":"lightcraft-catalog","version":V,"seq":N,"catalog":{…}}`, the +//! state after op `N` in catalog format `V` ([`VERSION`]); replaced atomically (temp + fsync + +//! rename). //! - `catalog.log` — JSON lines, one per op applied after the snapshot: //! `{"seq":N,"crc":C,"op":{…}}` where `C` is the CRC-32 of the op's JSON text. Appends are //! fsynced before [`Journal::append`] returns. @@ -10,6 +11,12 @@ //! Loading = snapshot + replay of the records with `seq > snapshot seq`. Recovery rules: //! - a **torn final record** (crash mid-append: truncated line or bad CRC at the end) is dropped //! and the file is cut back to the last good record, so later appends start on a clean line; +//! - a **failed append** (disk full, a dropped network share) may leave part of its batch in the +//! file: [`Journal::append`] cuts the log back to the last whole record before returning the +//! error (or, if that fails too, before the next append), so the retry — the same ops with the +//! same `seq`s — starts on a clean line. Logs written by builds without that cut-back hold the +//! fragment glued to the first retried record on one line; the loader recognises the whole, +//! CRC-valid record at the end of such a line and skips only the fragment; //! - records already covered by the snapshot (crash between writing the snapshot and resetting //! the log) are skipped; //! - a bad record **followed by good ones** is real damage: replay stops there, the log is kept @@ -27,6 +34,32 @@ //! //! At most one snapshot is in flight; a synchronous [`Journal::snapshot`] and dropping the journal //! first wait for it, so an older snapshot can never replace a newer one. +//! +//! # Format versions +//! +//! The snapshot's `version` is the catalog format, and it covers the log beside it: the log has +//! no header of its own, and is only ever appended to under a snapshot of this build's format. +//! +//! | Version | Written by | Adds | +//! |---|---|---| +//! | (none) | early builds: a log without a snapshot | | +//! | 1 | up to v0.2.0 | | +//! | 2 | after v0.2.0 | `Op::SetBrowsed`, `Catalog.browsed`, `Photo.local_baseline` | +//! +//! Rules: +//! - **Bump [`VERSION`]** (and add a row above) in the change that adds an [`Op`] variant or a +//! serialized field to the catalog, a photo, an album, a stack or develop settings that an older +//! build would misread or silently drop. New fields must deserialize from older data +//! (`#[serde(default)]`), so newer builds keep reading every older format. +//! - **Opening an older format upgrades it**: right after loading, [`Journal::open`] writes a +//! snapshot in this build's format (also for a new library), before anything is appended — so a +//! library is never a mix, and older builds refuse it from then on instead of reading part of it. +//! [`LoadReport::upgraded_from`] says when that happened. +//! - **A newer format is refused, untouched**: a snapshot whose `version` is above [`VERSION`], +//! or a log record whose CRC matches (so it is exactly what was written) but whose op doesn't +//! parse, fails [`Journal::open`] with [`CatalogError::Newer`] before any file is modified — it +//! is never mistaken for a torn tail or damage. (Builds up to v0.2.0 refuse a v2 snapshot with +//! "unsupported format".) use crate::store::Store; use crate::{Catalog, CatalogError, Op, Result}; @@ -34,7 +67,9 @@ use crate::{Catalog, CatalogError, Op, Result}; pub const SNAPSHOT: &str = "catalog.snap"; pub const LOG: &str = "catalog.log"; const FORMAT: &str = "lightcraft-catalog"; -const VERSION: u32 = 1; +/// The catalog format this build writes (and the newest it reads). See the module docs → +/// *Format versions*; bump it whenever an [`Op`] variant or a serialized field is added. +pub const VERSION: u32 = 2; /// When [`Journal::wants_snapshot`] says it's time to compact the log. #[derive(Clone, Copy, Debug)] @@ -62,10 +97,16 @@ pub struct LoadReport { pub failed: usize, /// Bytes of a torn final record that were dropped. pub torn_bytes: u64, + /// Partial records left mid-log by a failed append that was retried (written by builds that + /// didn't cut the log back after a failed write): skipped, the retried records were replayed. + pub torn_fragments: usize, /// The damaged log was preserved under this name. pub damaged: Option, /// No catalog files existed (a new library). pub created: bool, + /// The library was in an older catalog format (`0` = before format versions: a log without a + /// snapshot) and was rewritten in this version's format ([`VERSION`]). + pub upgraded_from: Option, } /// Where persistence time goes (reported by `library.info` → `persistence`; printed to stderr @@ -140,6 +181,9 @@ pub struct Journal { pending: Option, /// After a failed background snapshot: don't retry before the log has this many records. retry_at_records: u64, + /// A failed append may have left part of its records after `log_bytes`, and cutting them off + /// failed too: the next append cuts the log back to `log_bytes` first. + tail_dirty: bool, } /// A background snapshot in flight. @@ -168,18 +212,58 @@ pub fn encode_record(seq: u64, op: &Op) -> String { format!("{{\"seq\":{seq},\"crc\":{crc},\"op\":{body}}}") } -/// Parse one log line (without the newline). `None` if it's malformed or the CRC doesn't match. +/// Parse one log line (without the newline). `None` if it's malformed, the CRC doesn't match, or +/// the op is one this version doesn't know (see [`Journal::open`]). pub fn decode_record(line: &str) -> Option<(u64, Op)> { - let rest = line.strip_prefix("{\"seq\":")?; - let (seq, rest) = rest.split_once(",\"crc\":")?; - let (crc, rest) = rest.split_once(",\"op\":")?; - let body = rest.strip_suffix('}')?; - let seq: u64 = seq.parse().ok()?; - let crc: u32 = crc.parse().ok()?; - if crc32fast::hash(body.as_bytes()) != crc { - return None; + match decode_line(line) { + Line::Record(seq, op) => Some((seq, op)), + Line::Newer(_) | Line::Bad => None, } - serde_json::from_str(body).ok().map(|op| (seq, op)) +} + +/// One log line, decoded. +enum Line { + Record(u64, Op), + /// The CRC matches — the line is exactly what was written — but the op doesn't parse: it + /// was written by a newer LightCraft (an op or field this version doesn't know), not damaged. + Newer(u64), + /// Malformed or a CRC mismatch: torn or damaged. + Bad, +} + +fn decode_line(line: &str) -> Line { + let parts = || { + let rest = line.strip_prefix("{\"seq\":")?; + let (seq, rest) = rest.split_once(",\"crc\":")?; + let (crc, rest) = rest.split_once(",\"op\":")?; + let body = rest.strip_suffix('}')?; + Some((seq.parse::().ok()?, crc.parse::().ok()?, body)) + }; + let Some((seq, crc, body)) = parts() else { return Line::Bad }; + if crc32fast::hash(body.as_bytes()) != crc { + return Line::Bad; + } + match serde_json::from_str(body) { + Ok(op) => Line::Record(seq, op), + Err(_) => Line::Newer(seq), + } +} + +/// The error for a library written by a newer LightCraft. +fn newer(what: String) -> CatalogError { + CatalogError::Newer(format!("{what}; this version reads catalog format v{VERSION} and older")) +} + +/// A line that doesn't decode but ends with a whole record: a fragment of a failed append followed +/// by the record that retried it (the retry re-encodes the same ops with the same `seq`s). Returns +/// the fragment's length and the record, if that record continues the log (`seq <= last + 1`). +/// The record's CRC must match, so damage is never mistaken for it. +fn resync(line: &[u8], last: u64) -> Option<(usize, (u64, Op))> { + const START: &[u8] = b"{\"seq\":"; + (1..line.len().saturating_sub(START.len() - 1)).filter(|&i| line[i..].starts_with(START)).find_map(|i| { + let rec = std::str::from_utf8(&line[i..]).ok().map(|l| l.trim_end_matches('\r')).and_then(decode_record)?; + (rec.0 <= last + 1).then_some((i, rec)) + }) } /// Write `catalog.snap` for the state after op `seq`, streaming the JSON into the store (no @@ -205,10 +289,16 @@ fn write_snapshot(store: &mut dyn Store, seq: u64, catalog: &Catalog) -> std::io Ok(SnapshotTiming { serialize_ms, write_sync_ms: (ms_since(t0) - serialize_ms).max(0.0), bytes, ..Default::default() }) } +/// Just the identification of `catalog.snap` (read before the catalog itself). #[derive(serde::Deserialize)] -struct SnapFile { +struct SnapHeader { format: String, version: u32, +} + +/// The whole `catalog.snap` (after [`SnapHeader`] was checked; `format` and `version` are ignored). +#[derive(serde::Deserialize)] +struct SnapFile { seq: u64, catalog: Catalog, } @@ -221,15 +311,20 @@ impl Journal { let snap = store.read(SNAPSHOT).map_err(io)?; let log = store.read(LOG).map_err(io)?; report.created = snap.is_none() && log.is_none(); - let (mut catalog, snapshot_seq) = match snap { + let (mut catalog, snapshot_seq, snapshot_version) = match snap { Some(bytes) => { - let s: SnapFile = serde_json::from_slice(&bytes).map_err(|e| CatalogError::Corrupt(format!("{SNAPSHOT}: {e}")))?; - if s.format != FORMAT || s.version > VERSION { - return Err(CatalogError::Corrupt(format!("{SNAPSHOT}: unsupported format {} v{}", s.format, s.version))); + // the header first: a newer snapshot may not parse as this version's catalog + let h: SnapHeader = serde_json::from_slice(&bytes).map_err(|e| CatalogError::Corrupt(format!("{SNAPSHOT}: {e}")))?; + if h.format != FORMAT { + return Err(CatalogError::Corrupt(format!("{SNAPSHOT}: not a LightCraft catalog (format {:?})", h.format))); } - (s.catalog, s.seq) + if h.version > VERSION { + return Err(newer(format!("{SNAPSHOT} is catalog format v{}", h.version))); + } + let s: SnapFile = serde_json::from_slice(&bytes).map_err(|e| CatalogError::Corrupt(format!("{SNAPSHOT}: {e}")))?; + (s.catalog, s.seq, Some(h.version)) } - None => (Catalog::new(), 0), + None => (Catalog::new(), 0, None), }; report.snapshot_seq = snapshot_seq; let mut j = Journal { @@ -242,6 +337,7 @@ impl Journal { stats: PersistStats::default(), pending: None, retry_at_records: 0, + tail_dirty: false, }; let log = log.unwrap_or_default(); @@ -261,7 +357,22 @@ impl Journal { good_end = next; continue; } - match line.and_then(decode_record) { + let mut record = line.and_then(decode_record); + if record.is_none() + && let Some((at, r)) = resync(&log[pos..line_end], j.seq) + { + // the torn start of a failed append, then the retried records (older builds + // appended them right after the fragment) + log::warn!("catalog log: skipped a partial record ({at} bytes) left by a failed write"); + report.torn_fragments += 1; + record = Some(r); + } + if record.is_none() + && let Some(Line::Newer(seq)) = line.map(decode_line) + { + return Err(newer(format!("{LOG} record {seq} holds a change this version doesn't know"))); + } + match record { Some((seq, op)) if seq <= j.seq => { // already in the snapshot let _ = op; @@ -277,7 +388,15 @@ impl Journal { } _ => { // bad record (or a gap): torn tail if nothing good follows, else damage - let rest_has_good = log[next..].split(|b| *b == b'\n').any(|l| std::str::from_utf8(l).ok().and_then(decode_record).is_some()); + let mut rest_has_good = false; + for l in log[next..].split(|b| *b == b'\n') { + match std::str::from_utf8(l).map(|l| decode_line(l.trim_end_matches('\r'))) { + Ok(Line::Record(..)) => rest_has_good = true, + // refused before anything is modified + Ok(Line::Newer(seq)) => return Err(newer(format!("{LOG} record {seq} holds a change this version doesn't know"))), + Ok(Line::Bad) | Err(_) => {} + } + } if rest_has_good { damaged_at = Some(pos); } @@ -301,10 +420,34 @@ impl Journal { report.torn_bytes = (log.len() - good_end) as u64; log::warn!("catalog log: dropped a torn final record ({} bytes)", report.torn_bytes); j.store.truncate(LOG, good_end as u64).map_err(io)?; + j.log_bytes = good_end as u64; } else if needs_newline { j.store.append(LOG, b"\n").map_err(io)?; + j.log_bytes = good_end as u64 + 1; + } else { + j.log_bytes = good_end as u64; + } + } + // Bring an older (or new, or snapshot-less) library to this version's format before + // anything is appended: a snapshot in this format makes older builds refuse the library + // instead of misreading ops or dropping fields they don't know. + if report.created { + // tiny: written directly, not counted as a compaction + let empty = format!("{{\"format\":\"{FORMAT}\",\"version\":{VERSION},\"seq\":0,\"catalog\":{}}}\n", catalog.to_snapshot()); + if let Err(e) = j.store.write_atomic(SNAPSHOT, empty.as_bytes()) { + log::warn!("catalog: can't write the new library's snapshot: {e}"); + } + } else if report.damaged.is_none() && snapshot_version != Some(VERSION) { + report.upgraded_from = Some(snapshot_version.unwrap_or(0)); + match j.snapshot(&catalog) { + Ok(()) => { + if let Some(v) = report.upgraded_from { + log::info!("catalog: upgraded from format v{v} to v{VERSION}"); + } + } + // nothing lost: the old files are intact and still load; retried on next open + Err(e) => log::warn!("catalog: can't write the format v{VERSION} snapshot: {e}"), } - j.log_bytes = good_end as u64; } catalog.revision = 0; Ok((j, catalog, report)) @@ -323,7 +466,20 @@ impl Journal { buf.push_str(&encode_record(seq, op)); buf.push('\n'); } - self.store.append(LOG, buf.as_bytes()).map_err(io)?; + if self.tail_dirty { + self.store.truncate(LOG, self.log_bytes).map_err(io)?; + self.tail_dirty = false; + } + if let Err(e) = self.store.append(LOG, buf.as_bytes()) { + // Part of the batch may be in the file. Cut it back to the last whole record, so the + // retry (same seqs) starts on a clean line instead of after a fragment that would + // read back as damage. + if let Err(t) = self.store.truncate(LOG, self.log_bytes) { + log::error!("catalog: can't cut the log back after a failed append: {t}"); + self.tail_dirty = true; + } + return Err(io(e)); + } if let Some(p) = self.pending.as_mut() { p.tail.extend_from_slice(buf.as_bytes()); p.tail_records += ops.len() as u64; @@ -343,12 +499,22 @@ impl Journal { /// Write a snapshot of `catalog` (which must reflect every appended op) and reset the log. pub fn snapshot(&mut self, catalog: &Catalog) -> Result<()> { + self.snapshot_with_unlogged(catalog, 0) + } + + /// [`Journal::snapshot`] of a catalog that also reflects `unlogged` ops applied after the + /// last appended one but never written to the log (their append failed). The snapshot counts + /// them (it holds the state after op `seq + unlogged`), so they are saved and must not be + /// appended afterwards. Once the snapshot is durable [`Journal::seq`] includes them, even if + /// resetting the log then fails (the error is still returned). + pub fn snapshot_with_unlogged(&mut self, catalog: &Catalog, unlogged: u64) -> Result<()> { // never two snapshot writers; the one in flight is older, so it must land first if let Err(e) = self.wait() { log::warn!("catalog: background snapshot failed ({e}); writing one now"); } let t0 = web_time::Instant::now(); - let mut timing = match write_snapshot(self.store.as_mut(), self.seq, catalog) { + let seq = self.seq + unlogged; + let mut timing = match write_snapshot(self.store.as_mut(), seq, catalog) { Ok(t) => t, Err(e) => { // nothing lost (the log is whole); don't retry on every append @@ -357,6 +523,9 @@ impl Journal { return Err(io(e)); } }; + // the snapshot is durable: it holds every op up to `seq` + self.seq = seq; + self.snapshot_seq = seq; let t2 = web_time::Instant::now(); // A crash here leaves old records in the log; they are skipped by seq on load. self.store.write_atomic(LOG, b"").map_err(io)?; @@ -365,10 +534,10 @@ impl Journal { timing.blocking_ms = timing.total_ms; timing.records = self.log_records; self.record_snapshot(timing); - self.snapshot_seq = self.seq; self.log_records = 0; self.log_bytes = 0; self.retry_at_records = 0; + self.tail_dirty = false; Ok(()) } @@ -459,6 +628,7 @@ impl Journal { self.log_records = p.tail_records; self.log_bytes = p.tail.len() as u64; self.retry_at_records = 0; + self.tail_dirty = false; Ok(()) } diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 4b9bdcdc..aad5949b 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -5,6 +5,11 @@ //! snapshot — crash-safe and diff-friendly (see [`journal`]); //! - **undo/redo**: the engine keeps inverse ops; //! - **determinism**: replaying the log reproduces the state exactly (property-tested). +//! +//! **Catalog format version** ([`journal::VERSION`], see [`journal`] → *Format versions*): adding +//! an [`Op`] variant or a serialized field means bumping it. Newer builds read every older format +//! (and upgrade it on open); older builds refuse a newer library with [`CatalogError::Newer`] +//! instead of reading part of it. #![forbid(unsafe_code)] #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] @@ -12,9 +17,11 @@ pub mod dates; pub mod journal; pub mod keywords; pub mod local; +pub mod lock; pub mod model; pub mod query; pub mod rules; +pub mod safe_file; pub mod stacks; pub mod store; @@ -26,6 +33,7 @@ pub use journal::{Journal, LoadReport, PersistStats, SnapshotPolicy, SnapshotTim pub use keywords::KeywordNode; use lightcraft_develop::DevelopSettings; pub use local::{DEFAULT_FORGET_DAYS, ForgetPlan, folder_of}; +pub use lock::{LibraryLock, LockError, LockOwner}; pub use model::*; pub use query::{DateGroup, Filter, RatingOp, Sort, SortKey}; pub use rules::{Match, Rule, RuleSet}; @@ -46,6 +54,10 @@ pub enum CatalogError { Corrupt(String), #[error("catalog storage: {0}")] Io(String), + /// The library was written by a newer LightCraft (a newer catalog format, or a change this + /// version doesn't know). Nothing was read into the session and nothing was modified. + #[error("this library was written by a newer version of LightCraft ({0}); update LightCraft to open it. The library was left unchanged.")] + Newer(String), } pub type Result = std::result::Result; @@ -643,6 +655,12 @@ mod tests; #[cfg(test)] mod tests_background; #[cfg(test)] +mod tests_format_version; +#[cfg(test)] mod tests_journal; #[cfg(test)] mod tests_local; +#[cfg(test)] +mod tests_lock; +#[cfg(test)] +mod tests_torn_append; diff --git a/crates/catalog/src/lock.rs b/crates/catalog/src/lock.rs new file mode 100644 index 00000000..879127c5 --- /dev/null +++ b/crates/catalog/src/lock.rs @@ -0,0 +1,140 @@ +//! One process per library: an exclusive OS lock on `/catalog.lock`, held for as long as +//! the library is open (issue #99). +//! +//! Two processes on the same library (the app and `lightcraft-cli --library`, two app instances, +//! two computers on a shared folder) would each keep their own catalog and `seq` counter, and +//! whichever wrote the last snapshot would silently drop the other's edits. So opening a library +//! takes the lock first, and a second opener gets [`LockError::InUse`] instead. +//! +//! - The lock is the OS's (`flock` on Unix, `LockFileEx` on Windows, via [`std::fs::File::try_lock`]): +//! it is released when the [`LibraryLock`] is dropped and, after a crash or a kill, by the OS when +//! the process ends. A `catalog.lock` file left behind is therefore never "stale": only a lock +//! that's actually held refuses the open. +//! - Who holds it (process id, computer, program, version) is written to `catalog.lock.owner`, for +//! the error message. It's a separate file because a Windows lock also blocks reading the +//! locked file. It's only a hint: it may be missing or left over from a crash. +//! - File systems without lock support (some network shares): the library opens unlocked, with a +//! warning — refusing it would lock the user out of their own library. Locks over SMB/NFS are +//! advisory and depend on the server; the owner file is then the only hint. +//! - Browser storage (OPFS) has no file locks: see the web host for its own guard. + +use std::fs::{File, OpenOptions, TryLockError}; +use std::io; +use std::path::{Path, PathBuf}; + +/// The lock file in the library directory. +pub const LOCK: &str = "catalog.lock"; +/// Who holds the lock (JSON, see [`LockOwner`]). +pub const OWNER: &str = "catalog.lock.owner"; + +/// Who has the library open (as written in [`OWNER`]). +#[derive(Clone, Debug, Default, PartialEq, serde::Serialize, serde::Deserialize)] +#[serde(default)] +pub struct LockOwner { + pub pid: u32, + /// Computer name, if known. + pub host: String, + /// The program (`LightCraft`, `lightcraft-cli`…) and its version. + pub program: String, + pub version: String, + /// When it opened the library (seconds since 1970). + pub since: u64, +} + +impl LockOwner { + /// This process, as `program`. + pub fn this_process(program: &str) -> LockOwner { + LockOwner { + pid: std::process::id(), + host: host_name(), + program: program.to_string(), + version: env!("CARGO_PKG_VERSION").to_string(), + since: web_time::SystemTime::now().duration_since(web_time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0), + } + } + + /// "LightCraft 0.3.0 (process 123 on studio-mac)". + pub fn describe(&self) -> String { + let program = if self.program.is_empty() { "another LightCraft" } else { self.program.as_str() }; + let version = if self.version.is_empty() { String::new() } else { format!(" {}", self.version) }; + let host = if self.host.is_empty() { + String::new() + } else if self.host == host_name() { + " on this computer".to_string() + } else { + format!(" on {}", self.host) + }; + format!("{program}{version} (process {}{host})", self.pid) + } +} + +/// The computer's name, best effort (empty if unknown). +fn host_name() -> String { + let env = ["COMPUTERNAME", "HOSTNAME"].iter().find_map(|k| std::env::var(k).ok().filter(|h| !h.trim().is_empty())); + env.or_else(|| std::fs::read_to_string("/etc/hostname").ok()).map(|h| h.trim().to_string()).filter(|h| !h.is_empty()).unwrap_or_default() +} + +#[derive(Debug, thiserror::Error)] +pub enum LockError { + /// Another process has the library open. + #[error("This library is already open in {}. Close it there first.", .0.as_ref().map(LockOwner::describe).unwrap_or_else(|| "another LightCraft program".into()))] + InUse(Option), +} + +/// The exclusive lock on a library directory; released on drop (and by the OS if the process +/// dies). +#[derive(Debug)] +pub struct LibraryLock { + dir: PathBuf, + /// `None` when the file system can't lock (the library is open unlocked). + _file: Option, + held: bool, +} + +impl LibraryLock { + /// Lock `dir` (which must exist) for this process. Fails only when another process holds it. + pub fn acquire(dir: &Path, program: &str) -> Result { + let unlocked = |why: io::Error| { + log::warn!("library {}: can't lock {LOCK} ({why}); opening it without protection against a second program", dir.display()); + LibraryLock { dir: dir.to_path_buf(), _file: None, held: false } + }; + let file = match OpenOptions::new().read(true).write(true).create(true).truncate(false).open(dir.join(LOCK)) { + Ok(f) => f, + Err(e) => return Ok(unlocked(e)), + }; + match file.try_lock() { + Ok(()) => {} + Err(TryLockError::WouldBlock) => { + let owner = std::fs::read(dir.join(OWNER)).ok().and_then(|b| serde_json::from_slice::(&b).ok()); + return Err(LockError::InUse(owner)); + } + Err(TryLockError::Error(e)) => return Ok(unlocked(e)), + } + let owner = LockOwner::this_process(program); + if let Ok(json) = serde_json::to_vec_pretty(&owner) + && let Err(e) = std::fs::write(dir.join(OWNER), json) + { + log::warn!("library {}: can't write {OWNER}: {e}", dir.display()); + } + Ok(LibraryLock { dir: dir.to_path_buf(), _file: Some(file), held: true }) + } + + /// The OS lock is held (`false`: the file system can't lock; opened unprotected). + pub fn held(&self) -> bool { + self.held + } + + pub fn dir(&self) -> &Path { + &self.dir + } +} + +impl Drop for LibraryLock { + /// Remove the owner note while still holding the lock (so it can't delete the next holder's), + /// then the file handle closes and the OS releases the lock. + fn drop(&mut self) { + if self.held { + let _ = std::fs::remove_file(self.dir.join(OWNER)); + } + } +} diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs new file mode 100644 index 00000000..1e482b81 --- /dev/null +++ b/crates/catalog/src/safe_file.rs @@ -0,0 +1,317 @@ +//! Crash-safe writes of user-visible files: exports, renders, DNG conversions, smart previews and +//! XMP sidecars all go through here (the catalog's own files use [`crate::store::FsStore`]). +//! +//! - [`write_atomic`] replaces a file so that after a crash, a full disk or an unplugged drive +//! either the old or the complete new content is there — never a truncated mix. The content +//! goes to a new temp file in the same folder (unique name, `create_new`), is synced to disk and +//! renamed over the target; then the folder is synced (POSIX). Any failure removes the temp +//! file and leaves the target as it was. +//! - [`write_new`] / [`write_new_unique`] write a file that must not exist yet: the same temp file +//! and sync, plus a read-back check, and the file then appears under its final name **without +//! replacing anything** (a hard link, which fails on a taken name; a check-then-rename where the +//! volume has no hard links). +//! - [`same_file`] tells whether two paths name the same file on disk (device + inode on Unix, +//! resolved paths elsewhere), for "is this target an original?" checks. + +use std::cell::Cell; +use std::fs::{self, File, OpenOptions}; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +thread_local! { + static FAIL_AFTER: Cell> = const { Cell::new(None) }; +} + +/// Fault injection for tests (in any crate): while the returned guard lives, every write made by +/// this module **on the calling thread** fails once `bytes` bytes of a file were written — as a +/// full disk or an unplugged drive would. +#[doc(hidden)] +pub fn fail_writes_after(bytes: u64) -> FaultGuard { + FAIL_AFTER.with(|c| c.set(Some(bytes))); + FaultGuard(()) +} + +/// Ends a [`fail_writes_after`] when dropped. +#[doc(hidden)] +pub struct FaultGuard(()); + +impl Drop for FaultGuard { + fn drop(&mut self) { + FAIL_AFTER.with(|c| c.set(None)); + } +} + +/// Counts what is written; fails past an injected limit. +struct Limited { + inner: W, + n: u64, + limit: Option, +} + +impl Write for Limited { + fn write(&mut self, buf: &[u8]) -> io::Result { + let buf = match self.limit { + Some(l) => { + let room = usize::try_from(l.saturating_sub(self.n)).unwrap_or(usize::MAX); + if room == 0 && !buf.is_empty() { + return Err(io::Error::other("write failed (injected fault)")); + } + buf.get(..room.min(buf.len())).unwrap_or(buf) + } + None => buf, + }; + let k = self.inner.write(buf)?; + self.n += k as u64; + Ok(k) + } + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +fn parent_of(path: &Path) -> &Path { + path.parent().filter(|d| !d.as_os_str().is_empty()).unwrap_or(Path::new(".")) +} + +/// Make a rename or a new name in `dir` durable (POSIX). Opening a directory fails on Windows, +/// where NTFS journals the rename itself, so errors are ignored. +pub fn sync_dir(dir: &Path) { + if let Ok(d) = File::open(dir) { + let _ = d.sync_all(); + } +} + +/// A new, empty temp file next to `path` (hidden, unique in this process and folder). +fn create_temp(path: &Path) -> io::Result<(PathBuf, File)> { + static N: AtomicU64 = AtomicU64::new(0); + if path.file_name().is_none() { + return Err(io::Error::new(io::ErrorKind::InvalidInput, format!("{} is not a file path", path.display()))); + } + let dir = parent_of(path); + for _ in 0..1000 { + let tmp = dir.join(format!(".lightcraft-{}-{}.tmp", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + match OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(f) => return Ok((tmp, f)), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("no free temp file name in {}", dir.display()))) +} + +/// Write a temp file next to `path` with what `fill` writes, synced to disk. On failure the temp +/// file is removed. Returns its path and length. +fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> { + let (tmp, file) = create_temp(path)?; + let r = (|| { + let limit = FAIL_AFTER.with(Cell::get); + let mut w = Limited { inner: io::BufWriter::with_capacity(1 << 20, file), n: 0, limit }; + fill(&mut w)?; + w.flush()?; + let n = w.n; + let f = w.inner.into_inner().map_err(|e| e.into_error())?; + f.sync_all()?; + Ok(n) + })(); + match r { + Ok(n) => Ok((tmp, n)), + Err(e) => { + let _ = fs::remove_file(&tmp); + Err(e) + } + } +} + +/// Replace (or create) `path` with `data` atomically and durably; see the module docs. The +/// folder must exist. +pub fn write_atomic(path: &Path, data: &[u8]) -> io::Result<()> { + write_atomic_with(path, &mut |w| w.write_all(data)).map(|_| ()) +} + +/// [`write_atomic`] with streamed content: `fill` writes it (an error aborts and leaves the old +/// file). Returns the length written. +pub fn write_atomic_with(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result { + let (tmp, n) = write_temp(path, fill)?; + if let Err(e) = fs::rename(&tmp, path) { + let _ = fs::remove_file(&tmp); + return Err(e); + } + sync_dir(parent_of(path)); + Ok(n) +} + +/// Write `data` as the new file `path`: never replaces an existing file (`AlreadyExists`), and +/// the file only appears once it is complete, synced and read back identical. +pub fn write_new(path: &Path, data: &[u8]) -> io::Result<()> { + write_new_unique(&mut std::iter::once(path.to_path_buf()), data).map(|_| ()) +} + +/// [`write_new`] under the first free name of `candidates` (all in the same folder, e.g. +/// `IMG_1.dng`, `IMG_1-2.dng`, …): the file is written once and then given the first name that +/// is not taken. Returns that name; `AlreadyExists` when every candidate is taken. +pub fn write_new_unique(candidates: &mut dyn Iterator, data: &[u8]) -> io::Result { + let mut candidates = candidates.peekable(); + let Some(first) = candidates.peek().cloned() else { + return Err(io::Error::new(io::ErrorKind::InvalidInput, "no file name to write")); + }; + let (tmp, _) = write_temp(&first, &mut |w| w.write_all(data))?; + let r = (|| { + if !same_content(&tmp, data)? { + return Err(io::Error::other("the written file reads back different (failing drive or connection?)")); + } + for path in candidates.by_ref().take(100_000) { + match publish_new(&tmp, &path) { + Ok(()) => { + sync_dir(parent_of(&path)); + return Ok(path); + } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: no free file name", first.display()))) + })(); + // after a hard link the temp name is a second name of the new file; after a rename it's gone + let _ = fs::remove_file(&tmp); + r +} + +/// Give the complete temp file `tmp` the name `path`, unless that is taken. +fn publish_new(tmp: &Path, path: &Path) -> io::Result<()> { + match fs::hard_link(tmp, path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => Err(e), + // no hard links on this volume (FAT/exFAT, some shares): check, then rename + Err(_) => { + if fs::symlink_metadata(path).is_ok() { + return Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{} exists", path.display()))); + } + fs::rename(tmp, path) + } + } +} + +/// Does the file at `path` hold exactly `data`? +fn same_content(path: &Path, data: &[u8]) -> io::Result { + use std::io::Read; + let mut f = File::open(path)?; + if f.metadata()?.len() != data.len() as u64 { + return Ok(false); + } + let mut buf = vec![0u8; 1 << 20]; + let mut at = 0usize; + loop { + let k = match f.read(&mut buf) { + Ok(0) => return Ok(at == data.len()), + Ok(k) => k, + Err(e) if e.kind() == io::ErrorKind::Interrupted => continue, + Err(e) => return Err(e), + }; + if data.get(at..at + k) != buf.get(..k) { + return Ok(false); + } + at += k; + } +} + +/// Do `a` and `b` name the same existing file (through links, `..`, case-insensitive names)? +pub fn same_file(a: &Path, b: &Path) -> bool { + let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return false }; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + ma.dev() == mb.dev() && ma.ino() == mb.ino() + } + #[cfg(not(unix))] + { + if ma.len() != mb.len() { + return false; + } + match (fs::canonicalize(a), fs::canonicalize(b)) { + (Ok(x), Ok(y)) => x == y || x.to_string_lossy().to_lowercase() == y.to_string_lossy().to_lowercase(), + _ => false, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp(tag: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("lc-safe-file-{tag}-{}", std::process::id())); + let _ = fs::remove_dir_all(&d); + fs::create_dir_all(&d).unwrap(); + d + } + + fn names(dir: &Path) -> Vec { + let mut v: Vec = fs::read_dir(dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect(); + v.sort(); + v + } + + #[test] + fn atomic_write_replaces_and_leaves_no_temp() { + let d = temp("replace"); + let p = d.join("out.jpg"); + write_atomic(&p, b"one").unwrap(); + write_atomic(&p, b"two two").unwrap(); + assert_eq!(fs::read(&p).unwrap(), b"two two"); + assert_eq!(names(&d), vec!["out.jpg"]); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn a_failure_mid_write_keeps_the_old_file() { + let d = temp("midwrite"); + let p = d.join("photo.jpg"); + fs::write(&p, b"the original bytes").unwrap(); + { + let _f = fail_writes_after(4); + let e = write_atomic(&p, &[7u8; 10_000]).unwrap_err(); + assert!(e.to_string().contains("injected"), "{e}"); + assert!(write_new(&d.join("new.dng"), &[1u8; 100]).is_err()); + } + assert_eq!(fs::read(&p).unwrap(), b"the original bytes", "the target is untouched"); + assert_eq!(names(&d), vec!["photo.jpg"], "no temp file and no partial new file"); + // a fill error aborts the same way + let r = write_atomic_with(&p, &mut |w| { + w.write_all(b"half")?; + Err(io::Error::other("encoder failed")) + }); + assert!(r.is_err()); + assert_eq!(fs::read(&p).unwrap(), b"the original bytes"); + assert_eq!(names(&d), vec!["photo.jpg"]); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn new_files_never_replace_anything() { + let d = temp("new"); + let p = d.join("a.dng"); + fs::write(&p, b"keep").unwrap(); + assert_eq!(write_new(&p, b"other").unwrap_err().kind(), io::ErrorKind::AlreadyExists); + assert_eq!(fs::read(&p).unwrap(), b"keep"); + let mut c = (1..).map(|k| if k == 1 { d.join("a.dng") } else { d.join(format!("a-{k}.dng")) }); + let got = write_new_unique(&mut c, b"fresh").unwrap(); + assert_eq!(got, d.join("a-2.dng")); + assert_eq!(fs::read(&got).unwrap(), b"fresh"); + assert_eq!(names(&d), vec!["a-2.dng", "a.dng"], "the temp file is gone"); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn same_file_sees_through_other_spellings() { + let d = temp("same"); + let p = d.join("x.jpg"); + fs::write(&p, b"x").unwrap(); + fs::write(d.join("y.jpg"), b"x").unwrap(); + fs::create_dir_all(d.join("sub")).unwrap(); + assert!(same_file(&p, &d.join("sub/../x.jpg"))); + assert!(!same_file(&p, &d.join("y.jpg"))); + assert!(!same_file(&p, &d.join("missing.jpg"))); + let _ = fs::remove_dir_all(&d); + } +} diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index 069878c7..a672d00e 100644 --- a/crates/catalog/src/store.rs +++ b/crates/catalog/src/store.rs @@ -28,9 +28,12 @@ pub trait Store: Send { self.write_atomic(name, &buf)?; Ok(buf.len() as u64) } - /// Append and make durable (fsync) before returning. + /// Append and make durable (fsync) before returning. On an error, part of `data` may have + /// been written (a full disk, a dropped network share): the journal cuts the file back to its + /// previous length with [`Store::truncate`] before appending again. fn append(&mut self, name: &str, data: &[u8]) -> io::Result<()>; - /// Cut a file to `len` bytes (drop a torn tail before appending again). + /// Cut a file to `len` bytes (drop a torn tail before appending again). A missing file is + /// left missing. fn truncate(&mut self, name: &str, len: u64) -> io::Result<()>; /// Human-readable location (diagnostics). fn describe(&self) -> String; @@ -132,15 +135,26 @@ impl Store for FsStore { self.appender = Some((name.to_string(), f)); } let Some((_, f)) = self.appender.as_mut() else { return Err(io::Error::other("appender not open")) }; - f.write_all(data)?; - f.sync_data() + let written = f.write_all(data).and_then(|()| f.sync_data()); + if written.is_err() { + // The handle may be dead (a network share that reconnected, a removed volume): the next + // attempt reopens the file. Part of `data` may be in the file; the caller cuts it back + // ([`Store::truncate`]) before appending again. + self.appender = None; + } + written } fn truncate(&mut self, name: &str, len: u64) -> io::Result<()> { if self.appender.as_ref().is_some_and(|(n, _)| n == name) { self.appender = None; } - let f = std::fs::OpenOptions::new().write(true).open(self.path(name))?; + let f = match std::fs::OpenOptions::new().write(true).open(self.path(name)) { + Ok(f) => f, + // nothing to cut (an append that failed before creating the file) + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; f.set_len(len)?; f.sync_all() } diff --git a/crates/catalog/src/tests_background.rs b/crates/catalog/src/tests_background.rs index 0e06936e..5235c3e1 100644 --- a/crates/catalog/src/tests_background.rs +++ b/crates/catalog/src/tests_background.rs @@ -180,7 +180,8 @@ fn failed_background_snapshot_loses_nothing_and_retries_later() { wait_written(&j); assert!(j.poll().is_err()); assert_eq!((j.snapshot_seq(), j.stats().failed_snapshots), (0, 1)); - assert!(g.files.get(SNAPSHOT).is_none()); + // still the (empty) snapshot written when the library was created + assert_eq!(g.snapshot_seq_on_disk(), Some(0)); assert_recovers(&g, &live); // no retry storm: the next attempt waits for another `max_records` records assert!(!j.wants_snapshot()); diff --git a/crates/catalog/src/tests_format_version.rs b/crates/catalog/src/tests_format_version.rs new file mode 100644 index 00000000..3f2e8816 --- /dev/null +++ b/crates/catalog/src/tests_format_version.rs @@ -0,0 +1,186 @@ +//! Issue #102: catalog format versions. Older formats load and are upgraded; a newer format is +//! refused with a clear error and left untouched (never read in part, truncated or set aside). + +use crate::journal::{LOG, SNAPSHOT, VERSION, encode_record}; +use crate::*; + +fn photo(c: &mut Catalog) -> Op { + let id = c.alloc_photo_id(); + Op::AddPhoto { photo: Box::new(Photo::new(id, Source::File { path: format!("/p/{}.jpg", id.0) }, "p.jpg", "JPEG", 4, 3, "2026-01-01")) } +} + +/// A catalog with two photos and the log line for a third op on top of it. +fn legacy_parts() -> (Catalog, String, Catalog) { + let mut c = Catalog::new(); + for _ in 0..2 { + let op = photo(&mut c); + c.apply(op).unwrap(); + } + let base = c.clone(); + let id = c.photos().next().unwrap().id; + let op = Op::SetRating { id, rating: 4 }; + c.apply(op.clone()).unwrap(); + (base, format!("{}\n", encode_record(3, &op)), c) +} + +fn snapshot_version(m: &MemStore) -> u64 { + let v: serde_json::Value = serde_json::from_slice(&m.get(SNAPSHOT).unwrap()).unwrap(); + v["version"].as_u64().unwrap() +} + +#[test] +fn v1_library_loads_and_is_upgraded() { + let (base, log, full) = legacy_parts(); + let m = MemStore::new(); + m.set(SNAPSHOT, format!("{{\"format\":\"lightcraft-catalog\",\"version\":1,\"seq\":2,\"catalog\":{}}}\n", base.to_snapshot()).into_bytes()); + m.set(LOG, log.into_bytes()); + let (mut j, c, r) = Journal::open(Box::new(m.clone())).unwrap(); + assert_eq!(c.to_snapshot(), full.to_snapshot()); + assert_eq!((r.replayed, r.upgraded_from), (1, Some(1))); + // rewritten in this format before anything is appended + assert_eq!(snapshot_version(&m), u64::from(VERSION)); + assert!(m.get(LOG).unwrap().is_empty()); + assert_eq!(j.seq(), 3); + j.append(&[Op::SetLabelName { label: ColorLabel::ALL[0], name: Some("x".into()) }]).unwrap(); + drop(j); + let (_, _, r) = Journal::open(Box::new(m)).unwrap(); + assert_eq!((r.replayed, r.upgraded_from), (1, None)); +} + +#[test] +fn versionless_log_only_library_is_upgraded() { + let mut c = Catalog::new(); + let op = photo(&mut c); + c.apply(op.clone()).unwrap(); + let m = MemStore::new(); + m.set(LOG, format!("{}\n", encode_record(1, &op)).into_bytes()); + let (_, loaded, r) = Journal::open(Box::new(m.clone())).unwrap(); + assert_eq!(loaded.to_snapshot(), c.to_snapshot()); + assert_eq!(r.upgraded_from, Some(0)); + assert_eq!(snapshot_version(&m), u64::from(VERSION)); +} + +/// A new library gets a snapshot in this format at once, so an older build never sees a bare log +/// it would read in part. +#[test] +fn new_library_is_written_in_this_format() { + let m = MemStore::new(); + let (_, _, r) = Journal::open(Box::new(m.clone())).unwrap(); + assert!(r.created); + assert_eq!(r.upgraded_from, None); + assert_eq!(snapshot_version(&m), u64::from(VERSION)); +} + +fn assert_refused_untouched(m: &MemStore) { + let before = m.files.lock().unwrap().clone(); + let e = Journal::open(Box::new(m.clone())).err().expect("refused"); + assert!(matches!(e, CatalogError::Newer(_)), "{e:?}"); + let msg = e.to_string(); + assert!(msg.contains("newer version of LightCraft") && msg.contains("left unchanged"), "{msg}"); + assert_eq!(*m.files.lock().unwrap(), before, "nothing modified, nothing renamed"); +} + +#[test] +fn newer_snapshot_is_refused_untouched() { + let (base, log, _) = legacy_parts(); + let m = MemStore::new(); + m.set( + SNAPSHOT, + format!("{{\"format\":\"lightcraft-catalog\",\"version\":{},\"seq\":2,\"catalog\":{}}}\n", VERSION + 1, base.to_snapshot()).into_bytes(), + ); + m.set(LOG, log.into_bytes()); + assert_refused_untouched(&m); + // even when its catalog doesn't parse as this version's + let m = MemStore::new(); + m.set( + SNAPSHOT, + format!("{{\"format\":\"lightcraft-catalog\",\"version\":{},\"seq\":2,\"catalog\":{{\"photos\":7}}}}\n", VERSION + 1).into_bytes(), + ); + assert_refused_untouched(&m); + let e = Journal::open(Box::new(m)).err().unwrap().to_string(); + assert!(e.contains(&format!("v{}", VERSION + 1)), "{e}"); +} + +/// A log record with a valid CRC whose op this version doesn't know: refused, not taken for a +/// torn tail (truncated) or damage (renamed) — wherever it is in the log. +#[test] +fn unknown_op_in_the_log_is_refused_untouched() { + let (base, log, _) = legacy_parts(); + let future = |seq: u64| { + let body = r#"{"op":"fromTheFuture","id":1,"what":"something new"}"#; + format!("{{\"seq\":{seq},\"crc\":{},\"op\":{body}}}\n", crc32fast::hash(body.as_bytes())) + }; + let snap = format!("{{\"format\":\"lightcraft-catalog\",\"version\":{VERSION},\"seq\":2,\"catalog\":{}}}\n", base.to_snapshot()); + let id = base.photos().next().unwrap().id; + let after = format!("{}\n", encode_record(5, &Op::SetRating { id, rating: 1 })); + for log in [format!("{log}{}", future(4)), format!("{log}{}{after}", future(4)), format!("{}{log}", future(3))] { + let m = MemStore::new(); + m.set(SNAPSHOT, snap.clone().into_bytes()); + m.set(LOG, log.into_bytes()); + assert_refused_untouched(&m); + } + // a known op with a field of an unknown shape is "newer" too + let body = r#"{"op":"setRating","id":1,"rating":"five stars"}"#; + let m = MemStore::new(); + m.set(SNAPSHOT, snap.into_bytes()); + m.set(LOG, format!("{log}{{\"seq\":4,\"crc\":{},\"op\":{body}}}\n", crc32fast::hash(body.as_bytes())).into_bytes()); + assert_refused_untouched(&m); +} + +/// A CRC mismatch is still damage / a torn tail (not "newer"). +#[test] +fn bad_crc_is_still_a_torn_tail() { + let (base, log, _) = legacy_parts(); + let m = MemStore::new(); + m.set( + SNAPSHOT, + format!("{{\"format\":\"lightcraft-catalog\",\"version\":{VERSION},\"seq\":2,\"catalog\":{}}}\n", base.to_snapshot()).into_bytes(), + ); + let body = r#"{"op":"fromTheFuture"}"#; + m.set(LOG, format!("{log}{{\"seq\":4,\"crc\":1,\"op\":{body}}}\n").into_bytes()); + let (_, _, r) = Journal::open(Box::new(m)).unwrap(); + assert!(r.torn_bytes > 0 && r.damaged.is_none(), "{r:?}"); +} + +/// Guard for the version rule (see `journal` → *Format versions*): adding an [`Op`] variant breaks +/// this match. When it does, bump `journal::VERSION`, add a row to the format table, and add the +/// variant here under the new version. +#[test] +fn op_variants_are_versioned() { + fn since(op: &Op) -> u32 { + match op { + Op::AddPhoto { .. } + | Op::RemovePhoto { .. } + | Op::SetRating { .. } + | Op::SetFlag { .. } + | Op::SetLabel { .. } + | Op::SetDevelop { .. } + | Op::SetMeta { .. } + | Op::SetDeleted { .. } + | Op::SetLocal { .. } + | Op::SetVersions { .. } + | Op::SetHistory { .. } + | Op::PushHistory { .. } + | Op::AddAlbum { .. } + | Op::RemoveAlbum { .. } + | Op::RenameAlbum { .. } + | Op::MoveAlbum { .. } + | Op::SetAlbumPhotos { .. } + | Op::SetAlbumCover { .. } + | Op::SetAlbumRules { .. } + | Op::AddStack { .. } + | Op::RemoveStack { .. } + | Op::SetStack { .. } + | Op::SetCaptured { .. } + | Op::SetAnalysis { .. } + | Op::SetFile { .. } + | Op::Relink { .. } + | Op::SetContent { .. } + | Op::SetLabelName { .. } + | Op::Batch { .. } => 1, + Op::SetBrowsed { .. } => 2, + } + } + let newest = since(&Op::SetBrowsed { folder: String::new(), at: None }); + assert_eq!(newest, VERSION, "the newest op's version must be the current format version"); +} diff --git a/crates/catalog/src/tests_journal.rs b/crates/catalog/src/tests_journal.rs index 6bf7c85c..0c9513e3 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/crates/catalog/src/tests_journal.rs @@ -283,7 +283,12 @@ fn streamed_snapshot_keeps_the_on_disk_format() { let (mut j, c, _) = open(&m); assert_eq!(c.to_snapshot(), full.to_snapshot()); j.snapshot(&c).unwrap(); - let legacy = format!("{{\"format\":\"lightcraft-catalog\",\"version\":1,\"seq\":{},\"catalog\":{}}}\n", j.seq(), c.to_snapshot()); + let legacy = format!( + "{{\"format\":\"lightcraft-catalog\",\"version\":{},\"seq\":{},\"catalog\":{}}}\n", + crate::journal::VERSION, + j.seq(), + c.to_snapshot() + ); assert_eq!(String::from_utf8(m.get(SNAPSHOT).unwrap()).unwrap(), legacy); assert_eq!(j.stats().last_snapshot.bytes, legacy.len() as u64); diff --git a/crates/catalog/src/tests_lock.rs b/crates/catalog/src/tests_lock.rs new file mode 100644 index 00000000..c885ec49 --- /dev/null +++ b/crates/catalog/src/tests_lock.rs @@ -0,0 +1,93 @@ +//! Issue #99: one process per library. + +use std::path::PathBuf; + +use crate::lock::{LOCK, OWNER}; +use crate::*; + +fn temp_dir(tag: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("lc-lock-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d +} + +#[test] +fn second_opener_is_refused_until_the_first_lets_go() { + let dir = temp_dir("twice"); + let first = LibraryLock::acquire(&dir, "LightCraft").unwrap(); + assert!(first.held()); + let owner: LockOwner = serde_json::from_slice(&std::fs::read(dir.join(OWNER)).unwrap()).unwrap(); + assert_eq!((owner.pid, owner.program.as_str()), (std::process::id(), "LightCraft")); + + let e = LibraryLock::acquire(&dir, "lightcraft-cli").unwrap_err(); + let LockError::InUse(Some(who)) = &e else { panic!("{e:?}") }; + assert_eq!(who.pid, std::process::id()); + let msg = e.to_string(); + assert!(msg.contains("already open in LightCraft") && msg.contains(&format!("process {}", std::process::id())), "{msg}"); + + drop(first); + assert!(!dir.join(OWNER).exists(), "the owner note goes with the lock"); + assert!(dir.join(LOCK).exists(), "the lock file itself may stay: only a held lock refuses"); + let again = LibraryLock::acquire(&dir, "lightcraft-cli").unwrap(); + assert!(again.held()); + drop(again); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Files left behind by a crash (lock file + owner note) don't lock anyone out. +#[test] +fn leftover_lock_files_are_not_a_lock() { + let dir = temp_dir("stale"); + std::fs::write(dir.join(LOCK), b"").unwrap(); + std::fs::write(dir.join(OWNER), br#"{"pid":999999,"host":"elsewhere","program":"LightCraft","version":"0.2.0","since":1}"#).unwrap(); + let l = LibraryLock::acquire(&dir, "LightCraft").unwrap(); + assert!(l.held()); + let owner: LockOwner = serde_json::from_slice(&std::fs::read(dir.join(OWNER)).unwrap()).unwrap(); + assert_eq!(owner.pid, std::process::id(), "the note now names this process"); + drop(l); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Another process holds the lock; when it is killed (a crash), the OS releases it. +#[test] +fn lock_held_by_another_process_is_released_when_it_dies() { + let dir = temp_dir("child"); + let exe = std::env::current_exe().unwrap(); + let mut child = std::process::Command::new(exe) + .args(["--exact", "tests_lock::lock_holder_child", "--ignored", "--test-threads=1"]) + .env("LC_LOCK_CHILD_DIR", &dir) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60); + while !dir.join("child-locked").exists() { + assert!(child.try_wait().unwrap().is_none(), "child exited before locking"); + assert!(std::time::Instant::now() < deadline, "child never locked"); + std::thread::sleep(std::time::Duration::from_millis(20)); + } + let e = LibraryLock::acquire(&dir, "LightCraft").unwrap_err(); + let LockError::InUse(Some(who)) = &e else { panic!("{e:?}") }; + assert_eq!(who.pid, child.id()); + assert!(e.to_string().contains("on this computer") || who.host.is_empty(), "{e}"); + + child.kill().unwrap(); // SIGKILL: no clean-up runs, like a crash + child.wait().unwrap(); + assert!(dir.join(OWNER).exists(), "the crashed holder's note is left behind"); + let l = LibraryLock::acquire(&dir, "LightCraft").expect("the OS released the dead process's lock"); + assert!(l.held()); + drop(l); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Run by [`lock_held_by_another_process_is_released_when_it_dies`] in a child process: take the +/// lock, say so, and wait to be killed. +#[test] +#[ignore = "helper process for lock_held_by_another_process_is_released_when_it_dies"] +fn lock_holder_child() { + let Some(dir) = std::env::var_os("LC_LOCK_CHILD_DIR") else { return }; + let _l = LibraryLock::acquire(std::path::Path::new(&dir), "lightcraft-cli").unwrap(); + std::fs::write(std::path::Path::new(&dir).join("child-locked"), b"").unwrap(); + std::thread::sleep(std::time::Duration::from_secs(120)); +} diff --git a/crates/catalog/src/tests_torn_append.rs b/crates/catalog/src/tests_torn_append.rs new file mode 100644 index 00000000..0b6cf05a --- /dev/null +++ b/crates/catalog/src/tests_torn_append.rs @@ -0,0 +1,223 @@ +//! Issue #101: an append that fails part-way (disk full, a dropped network share) and is then +//! retried must not read back as log damage, and must lose nothing. + +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; + +use crate::journal::LOG; +use crate::*; + +/// A store whose next append writes only a prefix of its data, then fails. +#[derive(Clone, Default)] +struct Torn { + files: MemStore, + /// The next append writes this many bytes, then returns an error. + torn_at: Arc>>, + /// `truncate` fails (the cut-back after a failed append can't be done right away). + truncate_fails: Arc, +} + +impl Torn { + fn tear_next_append(&self, bytes: usize) { + *self.torn_at.lock().unwrap() = Some(bytes); + } +} + +impl Store for Torn { + fn read(&mut self, name: &str) -> std::io::Result>> { + self.files.read(name) + } + fn write_atomic(&mut self, name: &str, data: &[u8]) -> std::io::Result<()> { + self.files.write_atomic(name, data) + } + fn append(&mut self, name: &str, data: &[u8]) -> std::io::Result<()> { + if let Some(n) = self.torn_at.lock().unwrap().take() { + self.files.append(name, &data[..n.min(data.len())])?; + return Err(std::io::Error::other("disk full")); + } + self.files.append(name, data) + } + fn truncate(&mut self, name: &str, len: u64) -> std::io::Result<()> { + if self.truncate_fails.load(Ordering::SeqCst) { + return Err(std::io::Error::other("share unavailable")); + } + self.files.truncate(name, len) + } + fn describe(&self) -> String { + "torn".into() + } +} + +fn add_photo(c: &mut Catalog) -> Op { + let id = c.alloc_photo_id(); + Op::AddPhoto { photo: Box::new(Photo::new(id, Source::File { path: format!("/p/{}.jpg", id.0) }, "p.jpg", "JPEG", 4, 3, "2026-01-01")) } +} + +/// Apply `ops` to the live catalog (like a command) and return them for the journal. +fn apply(c: &mut Catalog, ops: Vec) -> Vec { + for op in &ops { + c.apply(op.clone()).unwrap(); + } + ops +} + +/// Every tear position of a batch, then the retry (same ops, same seqs, like the engine's queue) +/// and more appends: reloading replays everything, reports no damage and no torn bytes. +#[test] +fn partial_append_then_retry_loads_cleanly() { + let probe = MemStore::new(); + let (mut j, mut c, _) = Journal::open(Box::new(probe.clone())).unwrap(); + let add = add_photo(&mut c); + let first = apply(&mut c, vec![add]); + j.append(&first).unwrap(); + let id = c.photos().next().unwrap().id; + let batch = apply(&mut c, vec![Op::SetRating { id, rating: 3 }, Op::SetFlag { id, flag: Flag::Pick }, Op::SetRating { id, rating: 4 }]); + let before = probe.get(LOG).unwrap().len(); + j.append(&batch).unwrap(); + let batch_len = probe.get(LOG).unwrap().len() - before; + + for tear in 1..batch_len { + let t = Torn::default(); + let (mut j, mut c, _) = Journal::open(Box::new(t.clone())).unwrap(); + let add = add_photo(&mut c); + let first = apply(&mut c, vec![add]); + j.append(&first).unwrap(); + let good_len = t.files.get(LOG).unwrap().len(); + let batch = apply(&mut c, vec![Op::SetRating { id, rating: 3 }, Op::SetFlag { id, flag: Flag::Pick }, Op::SetRating { id, rating: 4 }]); + t.tear_next_append(tear); + assert!(j.append(&batch).is_err(), "tear at {tear}"); + // cut back to the last whole record + assert_eq!(t.files.get(LOG).unwrap().len(), good_len, "tear at {tear}"); + j.append(&batch).unwrap(); + let more = apply(&mut c, vec![Op::SetLabel { id, label: Some(ColorLabel::ALL[0]) }]); + j.append(&more).unwrap(); + drop(j); + + let (_, loaded, r) = Journal::open(Box::new(t.clone())).unwrap(); + assert!(r.damaged.is_none(), "tear at {tear}: {r:?}"); + assert_eq!((r.torn_bytes, r.torn_fragments, r.replayed), (0, 0, 5), "tear at {tear}"); + assert_eq!(loaded.to_snapshot(), c.to_snapshot(), "tear at {tear}"); + } +} + +/// When cutting back fails too (the share is gone), the next append cuts back first; until that +/// works nothing more is appended after the fragment. +#[test] +fn failed_cut_back_is_done_before_the_next_append() { + let t = Torn::default(); + let (mut j, mut c, _) = Journal::open(Box::new(t.clone())).unwrap(); + let add = add_photo(&mut c); + let first = apply(&mut c, vec![add]); + j.append(&first).unwrap(); + let good_len = t.files.get(LOG).unwrap().len(); + let id = c.photos().next().unwrap().id; + let batch = apply(&mut c, vec![Op::SetRating { id, rating: 2 }, Op::SetRating { id, rating: 5 }]); + t.truncate_fails.store(true, Ordering::SeqCst); + t.tear_next_append(30); + assert!(j.append(&batch).is_err()); + assert_eq!(t.files.get(LOG).unwrap().len(), good_len + 30, "the fragment is still there"); + // still unreachable: the retry fails without writing anything after the fragment + assert!(j.append(&batch).is_err()); + assert_eq!(t.files.get(LOG).unwrap().len(), good_len + 30); + t.truncate_fails.store(false, Ordering::SeqCst); + j.append(&batch).unwrap(); + drop(j); + let (_, loaded, r) = Journal::open(Box::new(t.clone())).unwrap(); + assert!(r.damaged.is_none(), "{r:?}"); + assert_eq!(r.replayed, 3); + assert_eq!(loaded.to_snapshot(), c.to_snapshot()); +} + +/// Logs written before the fix: the fragment of the failed append is glued to the first retried +/// record on one line, followed by the rest of the retry. They load in full (the fragment is +/// skipped), instead of stopping at the fragment and setting the later records aside as damage. +#[test] +fn legacy_fragment_glued_to_the_retry_is_skipped() { + let m = MemStore::new(); + let (mut j, mut c, _) = Journal::open(Box::new(m.clone())).unwrap(); + let add = add_photo(&mut c); + let first = apply(&mut c, vec![add]); + j.append(&first).unwrap(); + let good = m.get(LOG).unwrap(); + let id = c.photos().next().unwrap().id; + let batch = apply( + &mut c, + vec![ + Op::SetRating { id, rating: 3 }, + Op::SetFlag { id, flag: Flag::Pick }, + Op::SetLabelName { label: ColorLabel::ALL[1], name: Some("Größe ✓".into()) }, + ], + ); + j.append(&batch).unwrap(); + let full = m.get(LOG).unwrap(); + let batch_bytes = &full[good.len()..]; + let later = apply(&mut c, vec![Op::SetRating { id, rating: 1 }]); + j.append(&later).unwrap(); + let later_bytes = m.get(LOG).unwrap()[full.len()..].to_vec(); + drop(j); + + // every tear (inside the first record, after it, inside a multi-byte character…) + for tear in 1..batch_bytes.len() { + if batch_bytes[tear - 1] == b'\n' { + continue; // a tear on a record boundary leaves no fragment + } + let mut log = good.clone(); + log.extend_from_slice(&batch_bytes[..tear]); + log.extend_from_slice(batch_bytes); + log.extend_from_slice(&later_bytes); + let t = MemStore::new(); + t.set(LOG, log); + let (_, loaded, r) = Journal::open(Box::new(t)).unwrap(); + assert!(r.damaged.is_none(), "tear at {tear}: {r:?}"); + assert_eq!(r.torn_fragments, 1, "tear at {tear}"); + assert_eq!(loaded.to_snapshot(), c.to_snapshot(), "tear at {tear}"); + } + + // a damaged record whose tail happens to be a whole record from the future is still damage + let mut log = good.clone(); + log.extend_from_slice(b"garbage"); + log.extend_from_slice(&later_bytes); + log.extend_from_slice(&later_bytes); + let t = MemStore::new(); + t.set(LOG, log); + let (_, _, r) = Journal::open(Box::new(t)).unwrap(); + assert!(r.damaged.is_some(), "{r:?}"); +} + +/// A snapshot can absorb ops whose append failed: it records them as saved (`seq` moves past +/// them), and appending after it continues from there. +#[test] +fn snapshot_with_unlogged_ops_saves_them() { + let t = Torn::default(); + let (mut j, mut c, _) = Journal::open(Box::new(t.clone())).unwrap(); + let add = add_photo(&mut c); + let first = apply(&mut c, vec![add]); + j.append(&first).unwrap(); + let id = c.photos().next().unwrap().id; + let queued = apply(&mut c, vec![Op::SetRating { id, rating: 2 }, Op::SetFlag { id, flag: Flag::Reject }]); + t.tear_next_append(10); + assert!(j.append(&queued).is_err()); + j.snapshot_with_unlogged(&c, queued.len() as u64).unwrap(); + assert_eq!(j.seq(), 3); + let after = apply(&mut c, vec![Op::SetRating { id, rating: 5 }]); + j.append(&after).unwrap(); + drop(j); + let (j, loaded, r) = Journal::open(Box::new(t.clone())).unwrap(); + assert_eq!((r.snapshot_seq, r.replayed), (3, 1)); + assert_eq!(j.seq(), 4); + assert_eq!(loaded.to_snapshot(), c.to_snapshot()); +} + +/// `FsStore::truncate` on a file that was never created (an append that failed first) is a no-op. +#[test] +fn fs_store_truncates_a_missing_file_quietly() { + let dir = std::env::temp_dir().join(format!("lc-torn-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let mut s = FsStore::open(&dir).unwrap(); + s.truncate(LOG, 0).unwrap(); + assert!(!dir.join(LOG).exists()); + s.append(LOG, b"abc\n").unwrap(); + s.truncate(LOG, 0).unwrap(); + assert_eq!(std::fs::read(dir.join(LOG)).unwrap(), b""); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/engine/src/cmd/convert.rs b/crates/engine/src/cmd/convert.rs index ddb2fdac..900c5257 100644 --- a/crates/engine/src/cmd/convert.rs +++ b/crates/engine/src/cmd/convert.rs @@ -10,22 +10,21 @@ use serde_json::{Value, json}; use super::{CommandSpec, bad, cmd, has_active, has_selection}; use crate::{Result, Session}; -/// A free `.dng` (then `-2.dng`…) next to `path`. -fn dng_path(path: &str) -> String { +/// `.dng`, then `-2.dng`, `-3.dng`… next to `path`. +fn dng_names(path: &str) -> impl Iterator { let p = Path::new(path); - let dir = p.parent().unwrap_or(Path::new("")); + let dir = p.parent().unwrap_or(Path::new("")).to_path_buf(); let stem = p.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_else(|| "photo".into()); - let mut out = dir.join(format!("{stem}.dng")); - let mut n = 2; - while out.exists() { - out = dir.join(format!("{stem}-{n}.dng")); - n += 1; - } - out.to_string_lossy().to_string() + (1u64..).map(move |n| if n == 1 { dir.join(format!("{stem}.dng")) } else { dir.join(format!("{stem}-{n}.dng")) }) } -/// Write the DNG for raw file `path` (develop settings in `packet`); returns the new path. -/// An empty `packet` writes no XMP. +/// Write the DNG for raw file `path` (develop settings in `packet`) next to it under a free name; +/// returns the new path. An empty `packet` writes no XMP. +/// +/// The DNG is checked before anyone relies on it (Convert to DNG relinks the photo to it, Copy as +/// DNG deletes the raw copy): the encoded file must decode to the same raw data, and it is +/// written to a temp file, synced, read back identical and only then given its name — never +/// replacing an existing file. On any failure no DNG is left behind and the raw stays in use. pub(crate) fn write_dng_for(s: &Session, path: &str, packet: String) -> std::result::Result { let bytes = match &s.media.file_bytes { Some(r) => r(path)?, @@ -35,9 +34,21 @@ pub(crate) fn write_dng_for(s: &Session, path: &str, packet: String) -> std::res drop(bytes); let dng = lightcraft_raw::write_dng(&raw, &lightcraft_raw::DngWriteOptions { xmp: (!packet.is_empty()).then_some(packet), ..Default::default() }) .map_err(|e| e.to_string())?; - let out = dng_path(path); - std::fs::write(&out, dng).map_err(|e| format!("{out}: {e}"))?; - Ok(out) + let back = lightcraft_raw::decode(&dng).map_err(|e| format!("{path}: the DNG written for it doesn't decode ({e}); the raw is kept"))?; + let same = match (&back.data, &raw.data) { + (lightcraft_raw::RawData::U16(a), lightcraft_raw::RawData::U16(b)) => a == b, + (lightcraft_raw::RawData::F32(a), lightcraft_raw::RawData::F32(b)) => { + a.len() == b.len() && a.iter().zip(b).all(|(x, y)| x.to_bits() == y.to_bits()) + } + _ => false, + }; + if (back.width, back.height, back.cpp) != (raw.width, raw.height, raw.cpp) || !same { + return Err(format!("{path}: the DNG written for it doesn't hold the same raw data; the raw is kept")); + } + drop((raw, back)); + let out = lightcraft_catalog::safe_file::write_new_unique(&mut dng_names(path), &dng) + .map_err(|e| format!("{path}: could not write its DNG ({e}); the raw is kept"))?; + Ok(out.to_string_lossy().to_string()) } fn convert(s: &mut Session, p: &Value) -> Result { @@ -133,7 +144,7 @@ fn edit_external(s: &mut Session, p: &Value) -> Result { }; let mut written = Vec::new(); let mut write = |path: &str, bytes: &[u8]| -> std::result::Result<(), String> { - std::fs::write(path, bytes).map_err(|e| format!("{path}: {e}"))?; + crate::export::write_file(path, bytes)?; written.push(path.to_string()); Ok(()) }; diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index 75173c84..11a11f6c 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -96,5 +96,18 @@ pub fn specs() -> Vec { save ), cmd!("export.deletePreset", "Delete Export Preset", [], None, "{name} — removes a user preset → presets", always, delete), + cmd!( + query "export.checkTarget", + "Check Output Path", + [], + None, + "{path} — fails when writing `path` would replace a photo's original (or its XMP sidecar) in the library; front ends call it before saving a render or screenshot to a user-given path → {path}", + always, + |s, p| { + let path = p.get("path").and_then(Value::as_str).ok_or_else(|| bad("export.checkTarget", "missing `path`"))?; + s.check_write_target(path).map_err(|e| bad("export.checkTarget", e))?; + Ok(json!({"path": path})) + } + ), ] } diff --git a/crates/engine/src/cmd/merge.rs b/crates/engine/src/cmd/merge.rs index 102ff0d3..a8869153 100644 --- a/crates/engine/src/cmd/merge.rs +++ b/crates/engine/src/cmd/merge.rs @@ -30,7 +30,8 @@ fn run(s: &mut Session, id: &str, p: &Value) -> Result { if let (Some(path), Some(img)) = (str_param(p, "previewPath"), &out.preview) { let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()) .map_err(|e| bad(id, e.to_string()))?; - std::fs::write(path, png).map_err(|e| bad(id, format!("{path}: {e}")))?; + s.check_write_target(path).map_err(|e| bad(id, e))?; + crate::export::write_file(path, &png).map_err(|e| bad(id, e))?; info["previewPath"] = json!(path); } if let Some(img) = &out.preview { diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 05bea65f..bf35f22e 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -1,8 +1,9 @@ //! Missing originals: photos whose file is no longer where the library expects it (moved, //! renamed, on an unplugged drive), and relinking them — one at a time (`photo.relink`) or by -//! searching a folder for files with the same name and size (`library.findMissing`). +//! searching a folder for files with the same name and size, or the same content +//! (`library.findMissing`). -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::Path; use lightcraft_catalog::{Catalog, Op, Photo, PhotoId, Source}; @@ -83,8 +84,29 @@ fn relink(s: &mut Session, p: &Value) -> Result { Ok(json!({"id": id.0, "path": abs})) } -/// Search `folder` (recursively) for each missing photo's file name — and its size when the -/// library knows it — and relink every match in one undoable step. +/// The stored content hash of a photo, without the `:dupN` suffix duplicates get. +fn stored_hash(p: &Photo) -> Option<&str> { + p.content_hash.as_deref().map(|h| h.split(':').next().unwrap_or(h)).filter(|h| !h.is_empty()) +} + +/// The content hash of the file at `path` (as import computes it), if it can be read. +fn file_hash(path: &str) -> Option { + std::fs::read(path).ok().map(|b| lightcraft_preview::hash_bytes(&b).to_string()) +} + +/// Search `folder` (recursively) for each missing photo's file and relink every match in one +/// undoable step. +/// +/// Matching, per photo: +/// 1. a file with the same name (any letter case), the same size and the same content hash; +/// 2. renamed: a file of the same size with the same content hash (only same-size files are +/// read, so this stays cheap); +/// 3. without a hash to confirm (or the file was edited in place): the one file with the same +/// name and size. Several such files are ambiguous: the photo is skipped and reported in +/// `ambiguous`, never guessed (two cameras' `IMG_0001.CR3` of one size are different photos). +/// +/// Files that are already some photo's original, or that another photo was just matched with, +/// are not used. fn find_missing(s: &mut Session, p: &Value) -> Result { const C: &str = "library.findMissing"; let folder = str_param(p, "folder").ok_or_else(|| bad(C, "missing `folder`"))?; @@ -93,27 +115,72 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { } let lost = missing(s); if lost.is_empty() { - return Ok(json!({"found": [], "missing": 0})); + return Ok(json!({"found": [], "missing": 0, "ambiguous": []})); } - // file name (lower case) → candidate paths + let in_use: HashSet = s.catalog.photos().filter_map(|p| checked_path(p).map(str::to_string)).collect(); + // file name (lower case) → candidate paths; size → paths (filled lazily) let mut by_name: HashMap> = HashMap::new(); - for f in crate::import::expand(&[folder.to_string()], None) { - if let Some(n) = Path::new(&f).file_name() { - by_name.entry(n.to_string_lossy().to_lowercase()).or_default().push(f); + let files: Vec = crate::import::expand(&[folder.to_string()], None).into_iter().filter(|f| !in_use.contains(f)).collect(); + for f in &files { + if let Some(n) = Path::new(f).file_name() { + by_name.entry(n.to_string_lossy().to_lowercase()).or_default().push(f.clone()); } } + let mut by_size: Option>> = None; + let mut hashes: HashMap> = HashMap::new(); + let mut hash_of = |f: &str| hashes.entry(f.to_string()).or_insert_with(|| file_hash(f)).clone(); + let size_of = |f: &str| std::fs::metadata(f).map(|m| m.len()).ok(); + let mut used: HashSet = HashSet::new(); let mut ops = Vec::new(); let mut found = Vec::new(); + let mut ambiguous = Vec::new(); for (id, old) in &lost { + let Some(ph) = s.catalog.photo(*id) else { continue }; let name = Path::new(old).file_name().map(|n| n.to_string_lossy().to_lowercase()).unwrap_or_default(); - let size = s.catalog.photo(*id).map(|p| p.file_size).unwrap_or(0); - let hit = by_name.get(&name).and_then(|c| { - c.iter().find(|f| size == 0 || std::fs::metadata(f).is_ok_and(|m| m.len() == size)).or_else(|| (c.len() == 1 && size == 0).then(|| &c[0])) - }); - if let Some(path) = hit { - ops.push(relink_op(*id, path)); - found.push(json!({"id": id.0, "from": old, "to": path})); + let size = ph.file_size; + let hash = stored_hash(ph).map(str::to_string); + let named: Vec<&String> = by_name + .get(&name) + .map(|c| c.iter().filter(|f| !used.contains(*f) && (size == 0 || size_of(f) == Some(size))).collect()) + .unwrap_or_default(); + // 1. same name and size, confirmed by the content hash + let mut hit: Option<(String, &str)> = + hash.as_deref().and_then(|h| named.iter().find(|f| hash_of(f).as_deref() == Some(h))).map(|f| (f.to_string(), "name")); + // 2. renamed: same size and content + if hit.is_none() + && let Some(h) = &hash + && size > 0 + { + let sizes = by_size.get_or_insert_with(|| { + let mut m: HashMap> = HashMap::new(); + for f in &files { + if let Some(n) = size_of(f) { + m.entry(n).or_default().push(f.clone()); + } + } + m + }); + hit = sizes + .get(&size) + .and_then(|c| c.iter().find(|f| !used.contains(*f) && hash_of(f).as_deref() == Some(h.as_str()))) + .map(|f| (f.clone(), "content")); } + // 3. same name and size only (no hash, or the file was edited in place): one candidate + // is taken, several are ambiguous and skipped rather than guessed + if hit.is_none() { + match named.len() { + 1 => hit = Some((named[0].clone(), "name")), + 0 => {} + _ => { + ambiguous.push(json!({"id": id.0, "from": old, "candidates": named})); + continue; + } + } + } + let Some((path, by)) = hit else { continue }; + ops.push(relink_op(*id, &path)); + found.push(json!({"id": id.0, "from": old, "to": path, "by": by})); + used.insert(path); } let still = lost.len() - found.len(); if !ops.is_empty() { @@ -124,7 +191,7 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { } } } - Ok(json!({"found": found, "missing": still})) + Ok(json!({"found": found, "missing": still, "ambiguous": ambiguous})) } pub fn specs() -> Vec { @@ -146,7 +213,7 @@ pub fn specs() -> Vec { "Find Missing Photos", [], None, - "{folder} — relink every missing photo whose file (same name and size) is somewhere in the folder → {found: [{id, from, to}], missing}", + "{folder} — relink every missing photo whose file is somewhere in the folder: same name and size (and content, when the library knows its hash), or — renamed — same size and content → {found: [{id, from, to, by: name|content}], missing, ambiguous: [{id, from, candidates}] (several same-name candidates and no hash to tell them apart: skipped)}", always, find_missing ), diff --git a/crates/engine/src/cmd/previews.rs b/crates/engine/src/cmd/previews.rs index 8da19ce3..44b2cebd 100644 --- a/crates/engine/src/cmd/previews.rs +++ b/crates/engine/src/cmd/previews.rs @@ -102,7 +102,7 @@ fn smart(s: &mut Session, p: &Value) -> Result { let dir = s.media.smart_dir.clone().ok_or_else(|| bad(C, "smart previews need a library on disk"))?; let discard = p.get("discard").and_then(Value::as_bool).unwrap_or(false); let ids = if p.get("ids").is_some() || !s.selection.ids.is_empty() { s.targets(p) } else { s.visible_cloned() }; - let (mut built, mut removed, mut failed) = (0usize, 0usize, Vec::new()); + let (mut built, mut repaired, mut removed, mut failed) = (0usize, 0usize, 0usize, Vec::new()); if !discard { // a chosen folder on another drive must be there: never recreate it on this one if s.smart_previews_dir.is_some() { @@ -123,20 +123,27 @@ fn smart(s: &mut Session, p: &Value) -> Result { } continue; } - if path.exists() { + // a complete proxy is kept; a missing or damaged one (cut short by a crash or a full + // drive) is (re)built + let damaged = path.exists(); + if damaged && crate::smart::is_valid(&path) { built += 1; continue; } + // atomic: a failed write leaves no partial proxy that would pass for a built one let r = s .source_now(id, crate::media::SourceLevel::Preview) .and_then(|src| crate::smart::encode(&src)) - .and_then(|b| std::fs::write(&path, b).map_err(|e| e.to_string())); + .and_then(|b| lightcraft_catalog::safe_file::write_atomic(&path, &b).map_err(|e| format!("{}: {e}", path.display()))); match r { - Ok(()) => built += 1, + Ok(()) => { + built += 1; + repaired += usize::from(damaged); + } Err(e) => failed.push(json!([id.0, e])), } } - Ok(json!({"built": built, "removed": removed, "failed": failed})) + Ok(json!({"built": built, "repaired": repaired, "removed": removed, "failed": failed})) } /// The smart previews folder: where it is, whether it is custom, what it holds, whether it can be @@ -205,7 +212,7 @@ fn smart_location(s: &mut Session, p: &Value) -> Result { /// Whether photo `id` has a smart preview. pub fn has_smart_preview(s: &Session, id: lightcraft_catalog::PhotoId) -> bool { match (&s.media.smart_dir, s.catalog.photo(id)) { - (Some(dir), Some(p)) => dir.join(crate::smart::file_name(p)).exists(), + (Some(dir), Some(p)) => crate::smart::is_valid(&dir.join(crate::smart::file_name(p))), _ => false, } } @@ -218,7 +225,7 @@ pub fn specs() -> Vec { "Build Smart Previews", [], None, - "{ids?, discard?: bool} — build (or discard) the smart previews of the selected photos (else all in view): compact proxies in the library that keep photos editable and exportable (at proxy size) while their originals are offline → {built, removed, failed}", + "{ids?, discard?: bool} — build (or discard) the smart previews of the selected photos (else all in view): compact proxies in the library that keep photos editable and exportable (at proxy size) while their originals are offline; a damaged proxy (cut short) is rebuilt → {built, repaired, removed, failed}", always, smart ), diff --git a/crates/engine/src/cmd/query.rs b/crates/engine/src/cmd/query.rs index 09fbbbdb..e7ffde14 100644 --- a/crates/engine/src/cmd/query.rs +++ b/crates/engine/src/cmd/query.rs @@ -113,7 +113,7 @@ pub fn specs() -> Vec { .map(|r| json!({"group": r.group, "tag": r.tag, "name": r.name, "value": r.value})) .collect(); // XMP: the sidecar if there is one, else the file's own packet - let packet = crate::sidecar::read_packet(path, ph.kind, s.xmp.naming).map(|(x, _)| x).or_else(|| lightcraft_meta::embedded(&bytes).xmp); + let packet = crate::sidecar::read_packet(path, ph.kind, s.sidecar_naming(ph.id)).map(|(x, _)| x).or_else(|| lightcraft_meta::embedded(&bytes).xmp); let xmp: Vec = packet .and_then(|x| lightcraft_meta::parse_xmp(&x).ok()) .map(|d| d.properties.into_iter().filter(|(k, _)| !k.starts_with("lc:")).map(|(k, v)| json!({"name": k, "value": v.join("; ")})).collect()) diff --git a/crates/engine/src/cmd/xmp.rs b/crates/engine/src/cmd/xmp.rs index 7b7d4325..b83e7275 100644 --- a/crates/engine/src/cmd/xmp.rs +++ b/crates/engine/src/cmd/xmp.rs @@ -9,14 +9,26 @@ use crate::{Result, Session}; fn save(s: &mut Session, p: &Value) -> Result { let mut written = Vec::new(); + let mut merged = Vec::new(); + let mut backups = Vec::new(); let mut failed = Vec::new(); + let owners = crate::sidecar::StemOwners::of(&s.catalog); for id in s.targets(p) { - match s.save_sidecar(id) { - Ok(path) => written.push(path.display().to_string()), + match s.save_sidecar_with(id, &owners) { + Ok(r) => { + let path = r.path.display().to_string(); + if r.merged { + merged.push(path.clone()); + } + if let Some(b) = r.backup { + backups.push(json!({"path": path, "backup": b.display().to_string()})); + } + written.push(path); + } Err(e) => failed.push(json!({"id": id.0, "error": e.to_string()})), } } - Ok(json!({"written": written, "failed": failed})) + Ok(json!({"written": written, "merged": merged, "backups": backups, "failed": failed})) } fn read(s: &mut Session, p: &Value) -> Result { @@ -63,7 +75,7 @@ pub fn specs() -> Vec { "Save Metadata to File", ["Photo"], Some("Cmd+S"), - "{ids?} — writes each photo's XMP sidecar (metadata + develop settings) next to the original → {written: [path], failed}", + "{ids?} — writes each photo's XMP sidecar (metadata + develop settings) next to the original, merged into an existing one (other apps' data kept; an unreadable one is backed up first) → {written: [path], merged: [path], backups: [{path, backup}], failed}", has_selection, save ), diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index 6ad24fca..6c30b1a0 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -987,6 +987,8 @@ pub struct PreparedExport { pub photo: lightcraft_catalog::PhotoId, pub file_name: String, work: Work, + /// The library's originals, which [`run_batch`] never writes over (shared by a batch). + guard: std::sync::Arc, } struct RenderWork { @@ -1009,12 +1011,30 @@ enum Work { }, } -/// Set up the export of photo `id` at 1-based position `seq` of a batch. +/// Set up the export of photo `id` at 1-based position `seq` of a batch. (For a whole batch use +/// [`prepare_batch`]: it looks at the library's originals once.) pub fn prepare_export( session: &mut crate::Session, id: lightcraft_catalog::PhotoId, o: &ExportOptions, seq: usize, +) -> Result { + let guard = std::sync::Arc::new(session.original_guard()); + prepare_guarded(session, id, o, seq, guard) +} + +/// [`prepare_export`] for each of `ids` in order (`{seq}` = position, from 1). +pub fn prepare_batch(session: &mut crate::Session, ids: &[lightcraft_catalog::PhotoId], o: &ExportOptions) -> Result, String> { + let guard = std::sync::Arc::new(session.original_guard()); + ids.iter().enumerate().map(|(i, id)| prepare_guarded(session, *id, o, i + 1, guard.clone())).collect() +} + +fn prepare_guarded( + session: &mut crate::Session, + id: lightcraft_catalog::PhotoId, + o: &ExportOptions, + seq: usize, + guard: std::sync::Arc, ) -> Result { let p = session.catalog.photo(id).ok_or("no such photo")?; let file_name = o.file_name_for(p, seq); @@ -1036,7 +1056,7 @@ pub fn prepare_export( size: (p.width as usize, p.height as usize), } }; - Ok(PreparedExport { photo: id, file_name, work }) + Ok(PreparedExport { photo: id, file_name, work, guard }) } impl PreparedExport { @@ -1088,14 +1108,16 @@ pub enum DngCompression { } /// The destination of a batch: a folder (with `ExportOptions::subfolder` and the conflict policy -/// applied), or one exact file path (single photo; overwritten). +/// applied to each file together with its sidecars), or one exact file path (single photo; an +/// ordinary file already there is replaced). Either way a catalogued original (or its sidecar) +/// is never written over: see [`crate::originals`]. #[derive(Clone, Debug, Default)] pub struct Destination { pub dir: String, pub exact: Option, } -/// Export `ids` in order ([`prepare_export`] + [`run_batch`]), stopping at the first error. +/// Export `ids` in order ([`prepare_batch`] + [`run_batch`]), stopping at the first error. pub fn export_batch( session: &mut crate::Session, ids: &[lightcraft_catalog::PhotoId], @@ -1104,15 +1126,34 @@ pub fn export_batch( write: &mut dyn FnMut(&str, &[u8]) -> Result<(), String>, exists: &dyn Fn(&str) -> bool, ) -> Result, String> { - let items = ids.iter().enumerate().map(|(i, id)| prepare_export(session, *id, o, i + 1)).collect::, _>>()?; + let items = prepare_batch(session, ids, o)?; run_batch(items, o, to, write, exists, true, &mut |_, _| true) } +/// Write an exported or rendered file on disk: its folder is created if needed, and the file is +/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic`]: a temp file, synced, then +/// renamed), so a failure part-way leaves any previous file intact and no truncated one. The +/// native writer behind exports, renders and screenshots (app, CLI, MCP). +pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> { + let p = std::path::Path::new(path); + if let Some(dir) = p.parent().filter(|d| !d.as_os_str().is_empty()) { + std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + } + lightcraft_catalog::safe_file::write_atomic(p, bytes).map_err(|e| format!("{path}: {e}")) +} + +/// The path of the sidecar with extension `ext` of the exported file `main`. +fn sidecar_path(main: &str, ext: &str) -> String { + std::path::Path::new(main).with_extension(ext).to_string_lossy().to_string() +} + /// Run prepared exports in order: pick each one's path, and hand the bytes (and sidecars) to -/// `write`. `exists` tells whether a path is taken (conflict policy). `progress(done, next file)` -/// is called before each photo; returning false cancels the rest. Returns one JSON object per -/// photo: `{path, width, height, bytes, sidecars}`, `{skipped: path}` or (unless -/// `stop_on_error`) `{photo, file, error}`. +/// `write`. `exists` tells whether a path is taken. The conflict policy applies to a file and its +/// sidecars as one: with Unique both get the same free name, with Skip the photo is skipped when +/// either is taken. A path that is a catalogued original (or its sidecar) is refused whatever the +/// policy. `progress(done, next file)` is called before each photo; returning false cancels the +/// rest. Returns one JSON object per photo: `{path, width, height, bytes, sidecars}`, +/// `{skipped: path}` or (unless `stop_on_error`) `{photo, file, error}`. pub fn run_batch( items: Vec, o: &ExportOptions, @@ -1132,7 +1173,7 @@ pub fn run_batch( if !progress(i, &item.file_name) { break; } - let (photo, name) = (item.photo, item.file_name.clone()); + let (photo, name, guard) = (item.photo, item.file_name.clone(), item.guard.clone()); let e = match item.run() { Ok(e) => e, Err(err) if stop_on_error => return Err(err), @@ -1141,14 +1182,18 @@ pub fn run_batch( continue; } }; + // the exported file and its sidecars + let group = |main: &str| std::iter::once(main.to_string()).chain(e.sidecars.iter().map(|(x, _)| sidecar_path(main, x))).collect::>(); let path = match to.exact.as_deref().filter(|_| single) { - Some(p) => p.to_string(), + Some(p) => Ok(p.to_string()), None => { - let mut path = join(&dir, &e.file_name); - let busy = |p: &str, taken: &std::collections::HashSet| taken.contains(p) || exists(p); - if busy(&path, &taken) { + let path = join(&dir, &e.file_name); + let busy = |main: &str| group(main).iter().any(|p| taken.contains(p) || exists(p)); + if !busy(&path) { + Ok(path) + } else { match o.conflict { - Conflict::Overwrite => {} + Conflict::Overwrite => Ok(path), Conflict::Skip => { out.push(json!({"skipped": path})); continue; @@ -1156,29 +1201,34 @@ pub fn run_batch( Conflict::Unique => { let (stem, ext) = e.file_name.rsplit_once('.').map_or((e.file_name.as_str(), None), |(a, b)| (a, Some(b))); let name = |n: usize| join(&dir, &ext.map_or(format!("{stem}-{n}"), |x| format!("{stem}-{n}.{x}"))); - path = (2..).map(name).find(|p| !busy(p, &taken)).expect("a free name"); + (2..1_000_000).map(name).find(|p| !busy(p)).ok_or_else(|| format!("{path}: no free file name")) } } } - path } }; - let written = write(&path, &e.bytes).and_then(|()| { - let mut sidecars = Vec::new(); - for (ext, bytes) in &e.sidecars { - let sc = std::path::Path::new(&path).with_extension(ext).to_string_lossy().to_string(); - write(&sc, bytes)?; - sidecars.push(sc); + let file = path.clone().unwrap_or_else(|_| name.clone()); + let written = path.and_then(|path| { + let files = group(&path); + // never over an original, whatever the conflict policy or the exact path said + for f in &files { + guard.check(std::path::Path::new(f))?; } - Ok(sidecars) + write(&path, &e.bytes)?; + let mut sidecars = Vec::new(); + for ((_, bytes), sc) in e.sidecars.iter().zip(files.iter().skip(1)) { + write(sc, bytes)?; + sidecars.push(sc.clone()); + } + Ok((path, files, sidecars)) }); match written { - Ok(sidecars) => { - taken.insert(path.clone()); + Ok((path, files, sidecars)) => { + taken.extend(files); out.push(json!({"path": path, "width": e.width, "height": e.height, "bytes": e.bytes.len(), "sidecars": sidecars})); } Err(err) if stop_on_error => return Err(err), - Err(err) => out.push(json!({"photo": photo.0, "file": path, "error": err})), + Err(err) => out.push(json!({"photo": photo.0, "file": file, "error": err})), } } Ok(out) diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 1109859e..8eb4c010 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -374,25 +374,14 @@ fn probe_all(probe: Option<&crate::media::FileProbe>, paths: &[String], progress } /// Copy `src` into `root`/`folders` as `name` (default: its own name), made unique with -1, -2…; -/// returns the new path. +/// returns the new path. The copy is verified ([`crate::import_move::copy_new`]: a new file, +/// synced and compared byte for byte); a bad one is removed and the photo reported as failed. fn copy_into(root: &Path, src: &str, folders: &[String], name: Option<&str>) -> Result { let dir = folders.iter().fold(root.to_path_buf(), |d, f| d.join(f)); - std::fs::create_dir_all(&dir).map_err(|e| format!("{}: {e}", dir.display()))?; let name = name .map(str::to_string) .unwrap_or_else(|| Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "photo".into())); - let (stem, ext) = match name.rsplit_once('.') { - Some((s, e)) => (s.to_string(), format!(".{e}")), - None => (name.clone(), String::new()), - }; - let mut dst = dir.join(&name); - let mut i = 1; - while dst.exists() { - dst = dir.join(format!("{stem}-{i}{ext}")); - i += 1; - } - std::fs::copy(src, &dst).map_err(|e| format!("copy {src}: {e}"))?; - Ok(dst.to_string_lossy().to_string()) + crate::import_move::copy_new(Path::new(src), &dir, &name).map(|p| p.to_string_lossy().to_string()) } /// What a [`scan`] needs from the session, so it can run on another thread (a folder on a network diff --git a/crates/engine/src/import_move.rs b/crates/engine/src/import_move.rs index 3c949e79..7221c595 100644 --- a/crates/engine/src/import_move.rs +++ b/crates/engine/src/import_move.rs @@ -169,6 +169,27 @@ pub(crate) fn place(src: &Path, dir: &Path, name: &str) -> Result Result { + fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + let (stem, ext) = match name.rsplit_once('.') { + Some((s, e)) if !s.is_empty() => (s.to_string(), format!(".{e}")), + _ => (name.to_string(), String::new()), + }; + for i in 0..100_000u32 { + let d = if i == 0 { dir.join(name) } else { dir.join(format!("{stem}-{i}{ext}")) }; + match copy_verified(src, &d) { + Ok(()) => return Ok(d), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(format!("copy {}: {e} (nothing was imported from it; the original is untouched)", src.display())), + } + } + Err(format!("copy {}: no free name in {}", src.display(), dir.display())) +} + /// Undo a [`place`]: remove what it created at the destination (the sources were never touched). pub(crate) fn rollback(p: &Placed) { for sc in p.sidecars.iter().filter(|s| s.created) { @@ -213,7 +234,7 @@ pub(crate) fn finish(p: &Placed) -> Result, String> { } /// A photo (by extension) still beside a stem-named sidecar, sharing its stem. -fn sibling_photo(sidecar: &Path) -> Option { +pub(crate) fn sibling_photo(sidecar: &Path) -> Option { let dir = sidecar.parent()?; let stem = sidecar.file_stem()?.to_string_lossy().to_string(); fs::read_dir(dir).ok()?.flatten().map(|e| e.path()).find_map(|f| { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 8774515f..2d68329f 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -24,6 +24,7 @@ pub mod library; pub mod media; pub mod memory; pub mod merge; +pub mod originals; pub mod preset_import; pub mod preset_luminar; pub mod presets; @@ -56,6 +57,9 @@ pub enum EngineError { /// be written. They stay queued and are written by the next successful save. #[error("saved in memory but not written to disk: {0}; LightCraft will retry")] NotSaved(String), + /// Another process (the app, `lightcraft-cli`, another computer) has the library open. + #[error("{0}")] + LibraryInUse(String), #[error("{0}")] Other(String), } @@ -378,15 +382,22 @@ impl Session { Ok(e.label) } - /// Apply an undo/redo op, first moving the files its renames imply (all or nothing). + /// Apply an undo/redo op, first moving the files its renames imply (all or nothing). Files + /// that can't be moved back after a failure are reported and the library follows them (a + /// logged change outside the undo history), so none goes missing. fn apply_with_files(&mut self, op: &Op) -> Result { + let fs = rename::RealFs; let moves = self.file_moves(op); - Session::move_files(&moves)?; + if let Err(e) = rename::move_all(&fs, &moves) { + return Err(EngineError::Other(format!("can't move the files back: {}", self.follow_stuck(op, e, false)))); + } match self.catalog.apply(op.clone()) { Ok(inv) => Ok(inv), Err(e) => { let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect(); - let _ = Session::move_files(&back); + if let Err(be) = rename::move_all(&fs, &back) { + return Err(EngineError::Other(format!("{e}; the files could not all be moved back: {}", be.message))); + } Err(e.into()) } } diff --git a/crates/engine/src/library.rs b/crates/engine/src/library.rs index 94ed050a..616cfb0f 100644 --- a/crates/engine/src/library.rs +++ b/crates/engine/src/library.rs @@ -23,7 +23,7 @@ use std::path::{Path, PathBuf}; -use lightcraft_catalog::{FsStore, Journal, LoadReport, Store}; +use lightcraft_catalog::{FsStore, Journal, LibraryLock, LoadReport, Store}; use lightcraft_develop::Preset; use serde::{Deserialize, Serialize}; @@ -70,6 +70,9 @@ pub struct Library { warnings_reported: usize, /// Settings files that couldn't be read or set aside: never overwritten this session. blocked: Vec<&'static str>, + /// Keeps other processes out of this library while it's open (last: released after the + /// journal's background snapshot has landed). + lock: Option, } /// Where a library's files live (see [`Session::open_library_in`]). @@ -177,6 +180,23 @@ fn poll_compaction(lib: &mut Library) { /// How long the frame loop waits before retrying a failed append (commands retry at once). const RETRY_BACKOFF: std::time::Duration = std::time::Duration::from_secs(2); +/// `a` and `b` name the same directory. +fn same_dir(a: &Path, b: &Path) -> bool { + match (std::fs::canonicalize(a), std::fs::canonicalize(b)) { + (Ok(a), Ok(b)) => a == b, + _ => a == b, + } +} + +/// This program, for the lock owner note ("LightCraft", "lightcraft-cli"). +fn program_name() -> String { + let exe = std::env::current_exe().ok().and_then(|p| p.file_stem().map(|s| s.to_string_lossy().to_string())); + match exe.as_deref() { + Some("lightcraft") | None => "LightCraft".into(), + Some(other) => other.to_string(), + } +} + /// Problems reading the settings files (`presets.json`, `prefs.json`, `view.json`) when the library /// opened (issue #103). A missing file is not a problem (defaults). A file that doesn't parse is /// kept as `.corrupt-` before the defaults are used (so the next save can't @@ -225,16 +245,47 @@ impl SettingsLoad { impl Session { /// Open (or create) the library at `dir` into this session, replacing its catalog. With /// `seed_demo`, a newly created library starts with the procedural demo photos. + /// + /// The library is locked for this session ([`lightcraft_catalog::lock`]): if another process + /// has it open, this fails with [`EngineError::LibraryInUse`] and nothing is read or changed. + /// Reopening the library this session already has open keeps its lock. pub fn open_library(&mut self, dir: impl AsRef, seed_demo: bool) -> Result<&LoadReport> { let dir = dir.as_ref().to_path_buf(); let open = || FsStore::open(&dir).map_err(|e| EngineError::Other(format!("can't open library {}: {e}", dir.display()))); let stores = LibraryStores { catalog: Box::new(open()?), files: Box::new(open()?), on_disk: true, dir: dir.clone() }; - self.open_library_in(stores, seed_demo) + // the library open in this session (same directory): hand its lock over + let reused = self.library.as_mut().filter(|l| same_dir(&l.dir, &dir)).and_then(|l| l.lock.take()); + let reusing = reused.is_some(); + let mut lock = match reused { + Some(l) => Some(l), + None => Some(LibraryLock::acquire(&dir, &program_name()).map_err(|e| EngineError::LibraryInUse(e.to_string()))?), + }; + if let Err(e) = self.open_stores(stores, seed_demo, &mut lock) { + if reusing && let Some(lib) = self.library.as_mut() { + lib.lock = lock.take(); + } + return Err(e); + } + self.loaded_report() } /// Open (or create) a library whose files live in `stores` (e.g. browser storage), replacing - /// this session's catalog. + /// this session's catalog. Not locked: the host guards against a second opener (see + /// [`Session::open_library`] for directories). pub fn open_library_in(&mut self, stores: LibraryStores, seed_demo: bool) -> Result<&LoadReport> { + self.open_stores(stores, seed_demo, &mut None)?; + self.loaded_report() + } + + fn loaded_report(&self) -> Result<&LoadReport> { + match &self.library { + Some(lib) => Ok(&lib.report), + None => Err(EngineError::Other("library closed while opening".into())), + } + } + + /// Open `stores`; the new library takes `lock` once nothing can fail any more. + fn open_stores(&mut self, stores: LibraryStores, seed_demo: bool, lock: &mut Option) -> Result<()> { let LibraryStores { dir, catalog, mut files, on_disk } = stores; self.media.smart_dir = on_disk.then(|| crate::smart::dir(&dir)); // the current library may be these same files: let its background snapshot land first @@ -328,6 +379,7 @@ impl Session { settings_warnings: settings.warnings, warnings_reported: 0, blocked: settings.blocked, + lock: lock.take(), }); // forget untouched Local records of folders not browsed for a while (journaled at once) if self.forget_local_days > 0 { @@ -341,10 +393,7 @@ impl Session { lib.forgot_local = Some(plan); } } - match &self.library { - Some(lib) => Ok(&lib.report), - None => Err(EngineError::Other("library closed while opening".into())), - } + Ok(()) } /// Write pending ops to the log (fsynced), compact when due, and save changed presets. @@ -413,16 +462,35 @@ impl Session { } /// Flush everything and write a snapshot (on quit). Ends an open interaction first. + /// + /// The snapshot is written even when appending the queued ops fails: it is a fresh file + /// holding everything in memory, so it saves those ops too (the log handle may be the only + /// thing that's broken). Fails only if nothing could be saved; the ops then stay queued. pub fn close_library(&mut self) -> Result<()> { if self.library.is_none() { return Ok(()); } let _ = self.end_interaction(); - self.persist()?; + let persisted = self.persist(); self.save_view(); + let unlogged = if persisted.is_err() { self.pending_log.len() as u64 } else { 0 }; let Some(lib) = self.library.as_mut() else { return Ok(()) }; - lib.journal.snapshot(&self.catalog)?; - Ok(()) + let before = lib.journal.seq(); + let snapshot = lib.journal.snapshot_with_unlogged(&self.catalog, unlogged); + if unlogged > 0 && lib.journal.seq() == before + unlogged { + // the snapshot holds the queued ops: they are saved, never append them again + log::info!("library: {unlogged} queued change(s) saved in the closing snapshot"); + self.pending_log.clear(); + lib.unsaved_error = None; + lib.retry_at = None; + lib.last_error = None; + } + match (persisted, snapshot) { + (_, Ok(())) => Ok(()), + // neither the log nor the snapshot took the queued ops + (Err(EngineError::NotSaved(e)), Err(s)) if lib.journal.seq() == before => Err(EngineError::NotSaved(format!("{e}; snapshot: {s}"))), + (_, Err(s)) => Err(s.into()), + } } fn view_json(&self) -> Vec { diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 4841533b..18e67faf 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -290,7 +290,7 @@ impl MediaCache { && !std::path::Path::new(path).exists() { let sp = dir.join(crate::smart::file_name(p)); - if sp.exists() { + if crate::smart::is_valid(&sp) { return SourceRef::Smart { path: sp }; } } diff --git a/crates/engine/src/merge.rs b/crates/engine/src/merge.rs index 24c65489..53c7f546 100644 --- a/crates/engine/src/merge.rs +++ b/crates/engine/src/merge.rs @@ -304,15 +304,13 @@ fn apply_auto(s: &mut DevelopSettings, src: &Rgb32f, info: &SourceInfo) { s.color.saturation = a.saturation; } -/// A free file name `dir/-.dng` (`-2`, `-3`… when taken). -fn output_path(first: &str, suffix: &str) -> String { +/// `dir/-.dng`, then `-2`, `-3`… (the candidates for a merge's output). +fn output_names(first: &str, suffix: &str) -> impl Iterator { let p = std::path::Path::new(first); let dir = p.parent().map(|d| d.to_path_buf()).unwrap_or_default(); let stem = p.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_else(|| "Merge".into()); - let cand = |k: u64| dir.join(if k == 1 { format!("{stem}-{suffix}.dng") } else { format!("{stem}-{suffix}-{k}.dng") }); - // (`u64::MAX` taken names can't exist; the last candidate is the fallback) - let free = (1..u64::MAX).map(cand).find(|c| !c.exists()).unwrap_or_else(|| cand(u64::MAX)); - free.to_string_lossy().to_string() + let suffix = suffix.to_string(); + (1u64..).map(move |k| dir.join(if k == 1 { format!("{stem}-{suffix}.dng") } else { format!("{stem}-{suffix}-{k}.dng") })) } /// Parse `merge.*` command parameters into a kind and finishing options. @@ -367,8 +365,11 @@ impl Session { /// apply Auto Settings / the auto crop. Returns `{id, path, …info}`. pub fn finish_merge(&mut self, job: &MergeJob, out: MergeOutput) -> Result { let first = &job.sources.first().ok_or_else(|| EngineError::Other("nothing merged".into()))?.1; - let path = output_path(first, job.kind.suffix()); - std::fs::write(&path, &out.dng).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + // a new file under a free name (never replacing one), complete and synced before it appears + let path = lightcraft_catalog::safe_file::write_new_unique(&mut output_names(first, job.kind.suffix()), &out.dng) + .map_err(|e| EngineError::Other(format!("could not write the merged DNG next to {first}: {e}")))? + .to_string_lossy() + .to_string(); let report = crate::import::import(self, std::slice::from_ref(&path), crate::import::ImportMode::Add)?; let id = report .imported diff --git a/crates/engine/src/originals.rs b/crates/engine/src/originals.rs new file mode 100644 index 00000000..59a5f6a2 --- /dev/null +++ b/crates/engine/src/originals.rs @@ -0,0 +1,102 @@ +//! Never write over an original: exports, renders and other files LightCraft writes for the user +//! must not land on a catalogued photo's own file (or its XMP sidecar), whatever the conflict +//! setting or exact path asked for. [`OriginalGuard`] is a snapshot of the library's originals +//! that answers "would writing here replace one?" by file identity (the same file under another +//! spelling, through `..`, a link or a case-insensitive volume counts), not by comparing strings. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use lightcraft_catalog::safe_file::same_file; +use lightcraft_catalog::{Catalog, Source}; + +/// The library's originals and their sidecars, looked up by file name. +#[derive(Debug, Default)] +pub struct OriginalGuard { + /// lower-case file name → (path on disk, what it is: "the original of IMG_1.CR3", …) + by_name: HashMap>, +} + +impl OriginalGuard { + /// The originals of every photo in `catalog`. + pub fn new(catalog: &Catalog) -> Self { + let mut g = Self::default(); + for p in catalog.photos() { + if let Source::File { path } = &p.source { + g.add(Path::new(path), &p.file_name); + } + } + g + } + + /// Protect `path` (an original named `label`) and its sidecars (`IMG_1.xmp`, `IMG_1.CR3.xmp`). + pub fn add(&mut self, path: &Path, label: &str) { + let Some(name) = path.file_name().map(|n| n.to_string_lossy().to_string()) else { return }; + let mut put = |file: String, p: PathBuf, what: String| self.by_name.entry(file.to_lowercase()).or_default().push((p, what)); + put(name.clone(), path.to_path_buf(), format!("the original of {label}")); + let stem = path.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default(); + put(format!("{stem}.xmp"), path.with_extension("xmp"), format!("the XMP sidecar of {label}")); + put(format!("{name}.xmp"), PathBuf::from(format!("{}.xmp", path.to_string_lossy())), format!("the XMP sidecar of {label}")); + } + + /// `Err` (a message for the user) when writing `target` would replace a protected file. + pub fn check(&self, target: &Path) -> Result<(), String> { + if std::fs::symlink_metadata(target).is_err() { + return Ok(()); // nothing there to replace + } + let mut names: Vec = target.file_name().map(|n| n.to_string_lossy().to_lowercase()).into_iter().collect(); + if let Some(n) = std::fs::canonicalize(target).ok().and_then(|c| c.file_name().map(|n| n.to_string_lossy().to_lowercase())) { + names.push(n); + } + for n in &names { + for (p, what) in self.by_name.get(n).into_iter().flatten() { + if same_file(p, target) { + return Err(format!( + "{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)", + target.display() + )); + } + } + } + Ok(()) + } +} + +impl crate::Session { + /// A guard over this library's originals (see [`OriginalGuard`]). + pub fn original_guard(&self) -> OriginalGuard { + OriginalGuard::new(&self.catalog) + } + + /// `Err` when writing `path` would replace a catalogued photo's original or sidecar. + pub fn check_write_target(&self, path: &str) -> Result<(), String> { + self.original_guard().check(Path::new(path)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use lightcraft_catalog::{Op, Photo, PhotoId}; + + #[test] + fn originals_and_their_sidecars_are_protected_by_identity() { + let d = std::env::temp_dir().join(format!("lc-originals-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(d.join("sub")).unwrap(); + let orig = d.join("IMG_1.jpg"); + std::fs::write(&orig, b"original").unwrap(); + std::fs::write(d.join("IMG_1.xmp"), b"").unwrap(); + std::fs::write(d.join("other.jpg"), b"other").unwrap(); + let mut c = Catalog::default(); + let p = Photo::new(PhotoId(1), Source::File { path: orig.to_string_lossy().to_string() }, "IMG_1.jpg", "JPEG", 10, 10, "2026-01-01"); + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let g = OriginalGuard::new(&c); + assert!(g.check(&orig).unwrap_err().contains("original of IMG_1.jpg")); + assert!(g.check(&d.join("sub/../IMG_1.jpg")).is_err(), "another spelling of the same file"); + assert!(g.check(&d.join("IMG_1.xmp")).unwrap_err().contains("sidecar")); + assert!(g.check(&d.join("other.jpg")).is_ok(), "an ordinary existing file"); + assert!(g.check(&d.join("new.jpg")).is_ok(), "a free name"); + let _ = std::fs::remove_dir_all(&d); + } +} diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 4b956c84..511aa8b4 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -11,7 +11,11 @@ //! //! Safety: //! - a target that exists on disk, or that another photo in the batch gets, receives a `-1`, `-2`… -//! suffix — no file is ever overwritten; +//! suffix — no file is ever overwritten. Whether an existing target is the photo itself (a +//! change of letter case on a case-insensitive volume) is decided by file identity, not by +//! comparing names, so on a case-sensitive volume `img_1.jpg` next to `IMG_1.JPG` is a collision +//! (issue #95); moves never replace a file even if one appears after the check (hard link + +//! unlink where the volume supports it); //! - the moves happen before the catalog changes; if one fails, the ones already done are moved //! back and nothing is committed; //! - the catalog op ([`Op::SetFile`]) is undoable: undo/redo move the files back and forth (see @@ -297,58 +301,251 @@ fn sidecars(path: &str) -> Vec<(PathBuf, SidecarNaming, bool)> { v } -/// Move `from` to `to` (never over an existing file) together with its sidecars. On error nothing -/// is left moved. -pub fn move_file(from: &str, to: &str) -> std::result::Result<(), String> { +/// The file-system primitives renaming uses. [`RealFs`] is the disk; tests substitute a model of +/// a case-insensitive or case-sensitive volume (and inject failures). +pub(crate) trait MoveFs { + /// Something (file, folder, link) answers to `p`. + fn exists(&self, p: &Path) -> bool; + fn is_file(&self, p: &Path) -> bool; + /// `a` and `b` name the same file (e.g. `IMG.JPG` and `img.jpg` on a case-insensitive + /// volume). Decided by file identity, never by comparing names: on a case-sensitive volume + /// they are two different photos. When in doubt: `false`. + fn same_file(&self, a: &Path, b: &Path) -> bool; + /// Rename `a` to `b`, failing (never replacing) when `b` exists. + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()>; + /// Copy `a` to a new file `b` (an independent copy, never a link), failing when `b` exists. + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()>; + fn remove_file(&self, p: &Path) -> std::io::Result<()>; + /// Another photo file beside the stem-named sidecar `sidecar` that shares its stem (e.g. + /// `IMG_1.JPG` for `IMG_1.xmp`), which still uses that sidecar. + fn stem_sibling(&self, sidecar: &Path) -> Option; +} + +/// The real file system. +pub(crate) struct RealFs; + +impl MoveFs for RealFs { + fn exists(&self, p: &Path) -> bool { + std::fs::symlink_metadata(p).is_ok() + } + + fn is_file(&self, p: &Path) -> bool { + p.is_file() + } + + fn same_file(&self, a: &Path, b: &Path) -> bool { + if a == b { + return true; + } + let (Ok(ma), Ok(mb)) = (std::fs::metadata(a), std::fs::metadata(b)) else { return false }; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + ma.dev() == mb.dev() && ma.ino() == mb.ino() + } + #[cfg(not(unix))] + { + // no stable file id in std here: `b` is `a` under another spelling when they look + // alike and the folder has no entry spelled exactly like `b` (on a case-sensitive + // folder holding both `IMG.JPG` and `img.jpg`, both entries are listed) + let (Some(dir), Some(name)) = (b.parent(), b.file_name()) else { return false }; + if a.parent() != Some(dir) || ma.len() != mb.len() || ma.modified().ok() != mb.modified().ok() { + return false; + } + let Ok(entries) = std::fs::read_dir(dir) else { return false }; + !entries.flatten().any(|e| e.file_name() == name) + } + } + + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, format!("{} already exists", b.display()))); + } + // A hard link fails when `b` exists — even if it appeared since the check above — so + // nothing is ever replaced. Volumes without hard links (FAT, some network shares) fall back + // to a plain rename after the check. + match std::fs::hard_link(a, b) { + Ok(()) => std::fs::remove_file(a).inspect_err(|_| { + // both names are the same file: dropping the new one loses nothing + let _ = std::fs::remove_file(b); + }), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => Err(e), + Err(_) => std::fs::rename(a, b), + } + } + + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + use std::io::Write; + let bytes = std::fs::read(a)?; + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(b)?; + f.write_all(&bytes).and_then(|_| f.sync_all()).inspect_err(|_| { + let _ = std::fs::remove_file(b); + }) + } + + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + std::fs::remove_file(p) + } + + fn stem_sibling(&self, sidecar: &Path) -> Option { + crate::import_move::sibling_photo(sidecar) + } +} + +/// `from` → `to` differs only in letter case and `to` is that very file (a case-insensitive +/// volume): the rename must go through a temporary name. +fn is_respelling(fs: &dyn MoveFs, from: &Path, to: &Path) -> bool { + from != to && from.to_string_lossy().to_lowercase() == to.to_string_lossy().to_lowercase() && fs.exists(to) && fs.same_file(from, to) +} + +/// Rename `a` to `b` without ever replacing a file; a change of letter case only goes through a +/// unique temporary name so case-insensitive volumes see a real change. +fn move_one(fs: &dyn MoveFs, a: &Path, b: &Path) -> std::io::Result<()> { + if !is_respelling(fs, a, b) { + return fs.rename_no_replace(a, b); + } + let tmp = (0..1000u32) + .map(|k| a.with_file_name(format!(".lc-rename-{}-{k}", std::process::id()))) + .find(|t| !fs.exists(t)) + .ok_or_else(|| std::io::Error::other("no free temporary name"))?; + fs.rename_no_replace(a, &tmp)?; + fs.rename_no_replace(&tmp, b).map_err(|e| match fs.rename_no_replace(&tmp, a) { + Ok(()) => e, + Err(e2) => std::io::Error::other(format!("{e}; it could not be given its old name back either ({e2}): it is now {}", tmp.display())), + }) +} + +/// Why [`move_file`] failed, and what it left behind. +#[derive(Clone, Debug, PartialEq)] +pub struct MoveError { + /// What went wrong, including every step that could not be undone and where that file is now. + pub message: String, + /// The file itself ended up at the new path: it moved, a sidecar didn't, and moving the file + /// back failed too. Its sidecars that did move stay with it. + pub moved: bool, +} + +impl std::fmt::Display for MoveError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + +/// Move `from` to `to` (never over an existing file) together with its sidecars. On error the +/// moves are undone; whatever can't be undone is named in the error (see [`MoveError`]). +pub fn move_file(from: &str, to: &str) -> std::result::Result<(), MoveError> { + move_file_with(&RealFs, from, to) +} + +/// [`move_file`] on the file system `fs`. +pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::result::Result<(), MoveError> { if from == to { return Ok(()); } + let fail = |message: String| MoveError { message, moved: false }; let (f, t) = (Path::new(from), Path::new(to)); - // a case-only change on a case-insensitive file system "exists" already: that is this file - let case_only = from.to_lowercase() == to.to_lowercase(); - if t.exists() && !case_only { - return Err(format!("{to} already exists")); + // `to` may be this very file spelled differently (case-insensitive volume); any other file + // there — e.g. `img.jpg` next to `IMG.JPG` on a case-sensitive volume — is never replaced + if fs.exists(t) && !is_respelling(fs, f, t) { + return Err(fail(format!("{to} already exists"))); } - let mut done: Vec<(PathBuf, PathBuf)> = Vec::new(); - let mv = |a: &Path, b: &Path| -> std::io::Result<()> { - if case_only { - // via a temporary name, so case-insensitive file systems see a real change - let tmp = a.with_file_name(format!(".lc-rename-{}", std::process::id())); - std::fs::rename(a, &tmp)?; - std::fs::rename(&tmp, b).inspect_err(|_| { - let _ = std::fs::rename(&tmp, a); - }) - } else { - std::fs::rename(a, b) - } - }; - mv(f, t).map_err(|e| format!("rename {from}: {e}"))?; - done.push((f.to_path_buf(), t.to_path_buf())); + move_one(fs, f, t).map_err(|e| fail(format!("rename {from}: {e}")))?; + // sidecars moved (or copied: `true`) + let mut done: Vec<(PathBuf, PathBuf, bool)> = Vec::new(); for (sc, naming, upper) in sidecars(from) { - if !sc.is_file() { + if !fs.is_file(&sc) { continue; } let mut dst = crate::sidecar::sidecar_path(to, naming); if upper { dst = dst.with_extension("XMP"); } - if dst.exists() && !case_only { + // a stem sidecar another file still uses (`IMG_1.xmp` of `IMG_1.CR3` and `IMG_1.JPG`) + // is copied, not taken away from it (issue #92) + let shared = naming == SidecarNaming::Stem && fs.stem_sibling(&sc).is_some(); + if fs.exists(&dst) && (shared || !is_respelling(fs, &sc, &dst)) { continue; // never overwrite: the old sidecar stays where it was } - if let Err(e) = mv(&sc, &dst) { - for (a, b) in done.iter().rev() { - let _ = std::fs::rename(b, a); + let r = if shared { fs.copy_no_replace(&sc, &dst) } else { move_one(fs, &sc, &dst) }; + if let Err(e) = r { + let mut message = format!("rename {}: {e}", sc.display()); + // the file first: if it can't go back, the sidecars that moved stay with it + if let Err(e) = move_one(fs, t, f) { + message.push_str(&format!("; {from} could not be moved back ({e}): it is now {to}")); + if !done.is_empty() { + let names: Vec = done.iter().map(|(_, b, _)| b.display().to_string()).collect(); + message.push_str(&format!(" with its sidecar {}", names.join(", "))); + } + return Err(MoveError { message, moved: true }); } - return Err(format!("rename {}: {e}", sc.display())); + for (a, b, copied) in done.iter().rev() { + if *copied { + // the original is still there: the copy just goes (a leftover copy loses nothing) + let _ = fs.remove_file(b); + } else if let Err(e) = move_one(fs, b, a) { + message.push_str(&format!("; sidecar {} could not be moved back ({e}): it is now {}", a.display(), b.display())); + } + } + return Err(fail(message)); } - done.push((sc, dst)); + done.push((sc, dst, shared)); } Ok(()) } +/// What a failed batch of moves ([`move_all`]) left behind. +#[derive(Debug)] +pub(crate) struct BatchMoveError { + /// The first failure and every move that could not be undone. + pub message: String, + /// Moves that stayed done, (from, to): those files are at `to` now. + pub stuck: Vec<(String, String)>, +} + +/// Move the files `moves` names, all or nothing: after a failure the earlier moves are undone. +/// Moves that can't be undone (e.g. the network share went away) are reported, not ignored. +pub(crate) fn move_all(fs: &dyn MoveFs, moves: &[(String, String)]) -> std::result::Result<(), BatchMoveError> { + let mut done: Vec<&(String, String)> = Vec::new(); + for m in moves { + let Err(e) = move_file_with(fs, &m.0, &m.1) else { + done.push(m); + continue; + }; + let mut message = e.message; + let mut stuck = Vec::new(); + if e.moved { + stuck.push(m.clone()); + } + for d in done.iter().rev() { + if let Err(e) = move_file_with(fs, &d.1, &d.0) { + message.push_str(&format!("; {} could not be moved back to {}: {}", d.1, d.0, e.message)); + if !e.moved { + stuck.push((*d).clone()); + } + } + } + return Err(BatchMoveError { message, stuck }); + } + Ok(()) +} + +/// The `SetFile` ops in `op` (nested batches included) that point a photo at one of `to`. +fn set_file_ops_to(op: &Op, to: &HashSet<&str>, out: &mut Vec) { + match op { + Op::Batch { ops } => ops.iter().for_each(|o| set_file_ops_to(o, to, out)), + Op::SetFile { source: Source::File { path }, .. } if to.contains(path.as_str()) => out.push(op.clone()), + _ => {} + } +} + impl Session { /// Plan renaming `ids` with `template` (sequence numbers from `start`), resolving collisions. pub fn plan_rename(&self, ids: &[PhotoId], template: &str, start: usize) -> Vec { + self.plan_rename_with(&RealFs, ids, template, start) + } + + /// [`Session::plan_rename`] on the file system `fs`. + pub(crate) fn plan_rename_with(&self, fs: &dyn MoveFs, ids: &[PhotoId], template: &str, start: usize) -> Vec { // photos sharing one file (virtual copies) rename once let mut seen_paths: HashSet = HashSet::new(); let mut taken: HashSet = HashSet::new(); // lower-case target paths/names claimed in this batch @@ -382,10 +579,12 @@ impl Session { let name = candidate(k); let tp = dir.join(&name).to_string_lossy().to_string(); let key = tp.to_lowercase(); - let same = key == path.to_lowercase(); + // this very file, maybe spelled differently (case-insensitive volume)? By + // identity: on a case-sensitive volume `img_1.jpg` may be another photo + let same = tp == *path || is_respelling(fs, Path::new(path), Path::new(&tp)); // free: not claimed in this batch and not on disk (unless it is this very file). // A file this batch moves away still counts as taken: simple and safe. - let on_disk = Path::new(&tp).exists() && !same; + let on_disk = fs.exists(Path::new(&tp)) && !same; if !taken.contains(&key) && !on_disk { taken.insert(key); break (name, Some(tp)); @@ -415,21 +614,16 @@ impl Session { /// Carry out `plans`: move the files (rolled back on failure), then commit one undoable op /// that updates the photos (and their virtual copies). + /// + /// When a failure can't be fully rolled back, the files that kept their new names are + /// committed as a smaller undoable rename, so the library points at them (nothing goes + /// missing), and the error lists them (old → new). pub fn apply_rename(&mut self, plans: &[RenamePlan]) -> Result { - let mut moved: Vec<(String, String)> = Vec::new(); - for pl in plans { - if let (Some(a), Some(b)) = (&pl.from_path, &pl.to_path) - && a != b - { - if let Err(e) = move_file(a, b) { - for (a, b) in moved.iter().rev() { - let _ = move_file(b, a); - } - return Err(EngineError::Other(e)); - } - moved.push((a.clone(), b.clone())); - } - } + self.apply_rename_with(&RealFs, plans) + } + + /// [`Session::apply_rename`] on the file system `fs`. + pub(crate) fn apply_rename_with(&mut self, fs: &dyn MoveFs, plans: &[RenamePlan]) -> Result { let mut ops = Vec::new(); let by_path: HashMap<&str, &RenamePlan> = plans.iter().filter_map(|p| p.from_path.as_deref().map(|f| (f, p))).collect(); for p in self.catalog.photos() { @@ -447,17 +641,66 @@ impl Session { } } let n = ops.len(); + let op = Op::Batch { ops }; + let moves: Vec<(String, String)> = plans + .iter() + .filter_map(|pl| match (&pl.from_path, &pl.to_path) { + (Some(a), Some(b)) if a != b => Some((a.clone(), b.clone())), + _ => None, + }) + .collect(); + if let Err(e) = move_all(fs, &moves) { + return Err(EngineError::Other(self.follow_stuck(&op, e, true))); + } if n > 0 - && let Err(e) = self.commit(&format!("Rename {n} Photo{}", if n == 1 { "" } else { "s" }), Op::Batch { ops }) + && let Err(err) = self.commit(&format!("Rename {n} Photo{}", if n == 1 { "" } else { "s" }), op) { - for (a, b) in moved.iter().rev() { - let _ = move_file(b, a); + let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect(); + if let Err(e) = move_all(fs, &back) { + return Err(EngineError::Other(format!("{err}; the renamed files could not all be moved back: {}", e.message))); } - return Err(e); + return Err(err); } Ok(n) } + /// After a failed batch of moves for `op`: point the photos whose files stayed moved at + /// their new paths (an undoable "partly" rename when `undoable`, otherwise a plain logged + /// change), so the library matches the disk. Returns the message for the user. + pub(crate) fn follow_stuck(&mut self, op: &Op, e: BatchMoveError, undoable: bool) -> String { + let mut msg = e.message; + if e.stuck.is_empty() { + return msg; + } + let to: HashSet<&str> = e.stuck.iter().map(|m| m.1.as_str()).collect(); + let mut keep = Vec::new(); + set_file_ops_to(op, &to, &mut keep); + let list: Vec = e.stuck.iter().map(|(a, b)| format!("{a} → {b}")).collect(); + let n = e.stuck.len(); + let s = if n == 1 { "" } else { "s" }; + let fix = Op::Batch { ops: keep }; + let applied = if undoable { + self.commit(&format!("Rename {n} Photo{s} (partly)"), fix) + } else { + match self.catalog.apply(fix.clone()) { + Ok(_) => { + self.pending_log.push(fix); + Ok(()) + } + Err(e) => Err(e.into()), + } + }; + match applied { + Ok(()) => msg + .push_str(&format!(". {n} file{s} could not be moved back and kept the new name; the library now points there: {}", list.join(", "))), + Err(err) => msg.push_str(&format!( + ". {n} file{s} could not be moved back and the library could not follow ({err}); relink with Find Missing Photos: {}", + list.join(", ") + )), + } + msg + } + /// File moves an undo/redo op implies: `SetFile` ops whose source path differs from the photo's /// current one. pub(crate) fn file_moves(&self, op: &Op) -> Vec<(String, String)> { @@ -479,26 +722,240 @@ impl Session { rec(self, op, &mut v); v } - - /// Move the files `moves` names; all or nothing. - pub(crate) fn move_files(moves: &[(String, String)]) -> Result<()> { - let mut done: Vec<&(String, String)> = Vec::new(); - for m in moves { - if let Err(e) = move_file(&m.0, &m.1) { - for (a, b) in done.iter().rev().map(|m| (&m.0, &m.1)) { - let _ = move_file(b, a); - } - return Err(EngineError::Other(format!("can't undo the rename: {e}"))); - } - done.push(m); - } - Ok(()) - } } #[cfg(test)] mod tests { use super::*; + use std::cell::RefCell; + + /// A model volume: files by path with contents; case-insensitive (default APFS/NTFS) or + /// case-sensitive (Linux, case-sensitive APFS). `fail` makes renames from a matching source fail. + pub(crate) struct FakeFs { + pub ci: bool, + pub files: RefCell)>>, + pub fail: RefCell>, + } + + impl FakeFs { + pub fn new(ci: bool, files: &[(&str, &str)]) -> FakeFs { + FakeFs { + ci, + files: RefCell::new(files.iter().map(|(p, c)| (p.to_string(), c.as_bytes().to_vec())).collect()), + fail: RefCell::new(Vec::new()), + } + } + fn idx(&self, p: &Path) -> Option { + let p = p.to_string_lossy(); + self.files.borrow().iter().position(|(f, _)| if self.ci { f.to_lowercase() == p.to_lowercase() } else { *f == p }) + } + /// The listing: (exact path, contents), sorted. + pub fn listing(&self) -> Vec<(String, String)> { + let mut v: Vec<_> = self.files.borrow().iter().map(|(p, c)| (p.clone(), String::from_utf8_lossy(c).to_string())).collect(); + v.sort(); + v + } + } + + impl MoveFs for FakeFs { + fn exists(&self, p: &Path) -> bool { + self.idx(p).is_some() + } + fn is_file(&self, p: &Path) -> bool { + self.exists(p) + } + fn same_file(&self, a: &Path, b: &Path) -> bool { + self.idx(a).is_some() && self.idx(a) == self.idx(b) + } + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.fail.borrow().iter().any(|f| a.to_string_lossy().contains(f.as_str())) { + return Err(std::io::Error::other("injected failure")); + } + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists")); + } + let i = self.idx(a).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + self.files.borrow_mut()[i].0 = b.to_string_lossy().to_string(); + Ok(()) + } + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists")); + } + let i = self.idx(a).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + let c = self.files.borrow()[i].1.clone(); + self.files.borrow_mut().push((b.to_string_lossy().to_string(), c)); + Ok(()) + } + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + let i = self.idx(p).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + self.files.borrow_mut().remove(i); + Ok(()) + } + fn stem_sibling(&self, sidecar: &Path) -> Option { + let stem = sidecar.with_extension("").to_string_lossy().to_string(); + self.files.borrow().iter().map(|(f, _)| f.clone()).find(|f| { + let p = Path::new(f); + p.with_extension("").to_string_lossy() == stem && !p.extension().is_some_and(|e| e.eq_ignore_ascii_case("xmp")) + }) + } + } + + fn file_photo(id: u64, path: &str) -> Photo { + let name = Path::new(path).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); + Photo::new(PhotoId(id), Source::File { path: path.into() }, &name, "JPG", 1, 1, "2026-01-01T00:00:00") + } + + /// Issue #95: a case-only rename is a real change on a case-insensitive volume and never + /// replaces a different file that differs only by case on a case-sensitive one. + #[test] + fn case_only_renames_never_replace_another_file() { + // case-sensitive: IMG_1.JPG and img_1.JPG are two photos + let fs = FakeFs::new(false, &[("/p/IMG_1.JPG", "first"), ("/p/img_1.JPG", "second"), ("/p/IMG_1.xmp", "sc1"), ("/p/img_1.xmp", "sc2")]); + assert!(move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap_err().message.contains("already exists")); + assert_eq!(fs.listing().len(), 4); + assert!(fs.listing().contains(&("/p/img_1.JPG".into(), "second".into()))); + // the planner picks a free name instead + let mut s = Session::new(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(1, "/p/IMG_1.JPG")) }).unwrap(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "img_1", 1); + assert_eq!(plans[0].to, "img_1-1.JPG"); + move_file_with(&fs, "/p/IMG_1.JPG", plans[0].to_path.as_deref().unwrap()).unwrap(); + assert_eq!( + fs.listing(), + vec![ + ("/p/img_1-1.JPG".into(), "first".into()), + ("/p/img_1-1.xmp".into(), "sc1".into()), + ("/p/img_1.JPG".into(), "second".into()), + ("/p/img_1.xmp".into(), "sc2".into()) + ] + ); + // case-sensitive, target free: a plain rename + let fs = FakeFs::new(false, &[("/p/IMG_2.JPG", "x")]); + move_file_with(&fs, "/p/IMG_2.JPG", "/p/img_2.JPG").unwrap(); + assert_eq!(fs.listing(), vec![("/p/img_2.JPG".into(), "x".into())]); + + // case-insensitive: the "existing" target is the photo itself → renamed via a temp name + let fs = FakeFs::new(true, &[("/p/IMG_1.JPG", "first"), ("/p/IMG_1.xmp", "sc1")]); + let mut s = Session::new(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(1, "/p/IMG_1.JPG")) }).unwrap(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "img_1", 1); + assert_eq!(plans[0].to, "img_1.JPG", "not a collision with itself"); + move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap(); + assert_eq!(fs.listing(), vec![("/p/img_1.JPG".into(), "first".into()), ("/p/img_1.xmp".into(), "sc1".into())]); + // case-insensitive, another file under another name: still a collision + let fs = FakeFs::new(true, &[("/p/A.JPG", "a"), ("/p/b.JPG", "b")]); + assert!(move_file_with(&fs, "/p/A.JPG", "/p/B.JPG").is_err()); + } + + /// Issue #105: a rollback that fails is reported, and the library follows the files that kept + /// their new names (an undoable partial rename), so nothing goes missing. + #[test] + fn failed_rollbacks_are_reported_and_followed() { + let fs = FakeFs::new(false, &[("/p/a.jpg", "A"), ("/p/a.xmp", "As"), ("/p/b.jpg", "B"), ("/p/c.jpg", "C")]); + let mut s = Session::new(); + for (i, f) in ["/p/a.jpg", "/p/b.jpg", "/p/c.jpg"].iter().enumerate() { + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(i as u64 + 1, f)) }).unwrap(); + } + let ids = [PhotoId(1), PhotoId(2), PhotoId(3)]; + let plans = s.plan_rename_with(&fs, &ids, "Trip-{seq}", 1); + // c can't be renamed (the share went away), and neither can Trip-1 be moved back + fs.fail.borrow_mut().extend(["c.jpg".to_string(), "Trip-1".to_string()]); + let err = s.apply_rename_with(&fs, &plans).unwrap_err().to_string(); + assert!(err.contains("rename /p/c.jpg"), "{err}"); + assert!(err.contains("/p/Trip-1.jpg could not be moved back"), "{err}"); + assert!(err.contains("/p/a.jpg → /p/Trip-1.jpg"), "lists what stayed renamed: {err}"); + assert_eq!( + fs.listing(), + vec![ + ("/p/Trip-1.jpg".into(), "A".into()), + ("/p/Trip-1.xmp".into(), "As".into()), + ("/p/b.jpg".into(), "B".into()), + ("/p/c.jpg".into(), "C".into()) + ] + ); + // the catalog matches the disk + let path = |s: &Session, id: u64| match &s.catalog.photo(PhotoId(id)).unwrap().source { + Source::File { path } => path.clone(), + Source::Demo { .. } => String::new(), + }; + assert_eq!(path(&s, 1), "/p/Trip-1.jpg"); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().file_name, "Trip-1.jpg"); + assert_eq!((path(&s, 2), path(&s, 3)), ("/p/b.jpg".into(), "/p/c.jpg".into())); + assert_eq!(s.undo.last().map(|e| e.label.as_str()), Some("Rename 1 Photo (partly)")); + + // a sidecar that won't move, and a file that won't go back: the file keeps its new name + // with the sidecars that did move, and says so + let fs = FakeFs::new(false, &[("/p/d.jpg", "D"), ("/p/d.xmp", "Ds"), ("/p/d.jpg.xmp", "Df")]); + fs.fail.borrow_mut().extend(["d.jpg.xmp".to_string(), "/p/e.jpg".to_string()]); + let e = move_file_with(&fs, "/p/d.jpg", "/p/e.jpg").unwrap_err(); + assert!(e.moved, "{e}"); + assert!(e.message.contains("it is now /p/e.jpg with its sidecar /p/e.xmp"), "{e}"); + assert_eq!(fs.listing(), vec![("/p/d.jpg.xmp".into(), "Df".into()), ("/p/e.jpg".into(), "D".into()), ("/p/e.xmp".into(), "Ds".into())]); + // … and when the file does go back, its sidecars follow it + let fs = FakeFs::new(false, &[("/p/d.jpg", "D"), ("/p/d.xmp", "Ds"), ("/p/d.jpg.xmp", "Df")]); + fs.fail.borrow_mut().push("d.jpg.xmp".to_string()); + let e = move_file_with(&fs, "/p/d.jpg", "/p/e.jpg").unwrap_err(); + assert!(!e.moved); + assert_eq!(fs.listing().len(), 3); + assert!(fs.listing().iter().all(|(p, _)| p.starts_with("/p/d."))); + } + + /// Issue #92 (comment): renaming one of `IMG_1.CR3` + `IMG_1.JPG` copies their shared + /// `IMG_1.xmp` instead of taking it away from the other; the last one renamed takes it. + #[test] + fn shared_stem_sidecar_is_copied_not_taken() { + let fs = FakeFs::new(false, &[("/p/IMG_1.CR3", "raw"), ("/p/IMG_1.JPG", "jpg"), ("/p/IMG_1.xmp", "shared"), ("/p/IMG_1.CR3.xmp", "own")]); + move_file_with(&fs, "/p/IMG_1.CR3", "/p/Trip_1.CR3").unwrap(); + assert_eq!( + fs.listing(), + vec![ + ("/p/IMG_1.JPG".into(), "jpg".into()), + ("/p/IMG_1.xmp".into(), "shared".into()), + ("/p/Trip_1.CR3".into(), "raw".into()), + ("/p/Trip_1.CR3.xmp".into(), "own".into()), + ("/p/Trip_1.xmp".into(), "shared".into()), + ] + ); + // the JPEG is the last one using it: it moves + move_file_with(&fs, "/p/IMG_1.JPG", "/p/Trip_2.JPG").unwrap(); + assert!(fs.listing().contains(&("/p/Trip_2.xmp".into(), "shared".into()))); + assert!(!fs.exists(Path::new("/p/IMG_1.xmp"))); + // a failure after the copy removes the copy and keeps the original + let fs = FakeFs::new(false, &[("/p/A.CR3", "raw"), ("/p/A.JPG", "jpg"), ("/p/A.xmp", "shared"), ("/p/A.CR3.xmp", "own")]); + fs.fail.borrow_mut().push("A.CR3.xmp".into()); + assert!(move_file_with(&fs, "/p/A.CR3", "/p/B.CR3").is_err()); + assert_eq!(fs.listing().len(), 4); + assert!(fs.listing().contains(&("/p/A.xmp".into(), "shared".into())) && !fs.exists(Path::new("/p/B.xmp"))); + } + + /// Issue #95 on the real disk, whichever kind of volume the temp folder is on. + #[test] + fn case_only_rename_on_disk() { + let dir = std::env::temp_dir().join(format!("lc-rename-case-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (upper, lower) = (dir.join("IMG_1.JPG"), dir.join("img_1.JPG")); + std::fs::write(&upper, b"first").unwrap(); + let case_sensitive = !lower.exists(); + let p = |q: &Path| q.to_string_lossy().to_string(); + if case_sensitive { + std::fs::write(&lower, b"second").unwrap(); + assert!(move_file(&p(&upper), &p(&lower)).is_err()); + assert_eq!(std::fs::read(&lower).unwrap(), b"second"); + assert_eq!(std::fs::read(&upper).unwrap(), b"first"); + } else { + move_file(&p(&upper), &p(&lower)).unwrap(); + let names: Vec = std::fs::read_dir(&dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect(); + assert_eq!(names, vec!["img_1.JPG".to_string()]); + assert_eq!(std::fs::read(&lower).unwrap(), b"first"); + } + // a plain rename never replaces + std::fs::write(dir.join("other.JPG"), b"other").unwrap(); + assert!(move_file(&p(&dir.join("other.JPG")), &p(&lower)).is_err()); + assert_eq!(std::fs::read(dir.join("other.JPG")).unwrap(), b"other"); + let _ = std::fs::remove_dir_all(&dir); + } #[test] fn folder_templates_stay_inside_the_destination() { diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index a49e5fee..69bf8f0f 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -5,12 +5,25 @@ //! creator, keywords, capture time and GPS in the standard namespaces, plus our complete develop //! settings (`lc:settings`, JSON), the pick/reject flag (`lc:flag`) and location (`lc:location`). //! +//! An existing sidecar is never replaced wholesale (issue #92): LightCraft's properties +//! ([`OWNED`], [`OWNED_IF_STATED`]) are merged into it ([`lightcraft_meta::merge_xmp`]) and +//! everything else — another editor's `crs:` develop settings, `xmpMM` history, unknown +//! namespaces — is kept byte for byte. A sidecar that can't be read as XMP is first copied to +//! `.xmp.bak-