From 08c2acda26889f013c5d2451ef9fdadfdb15b010 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 16:44:09 -0400 Subject: [PATCH] Issue #101: catalog.log: cut a partial append back before retrying A failed append (disk full, dropped share) could leave part of its batch in catalog.log; the retry (same seqs) then shared a line with the fragment and the next load treated it as mid-log damage, setting later edits aside. - Journal::append cuts the log back to the last whole record on error (or, if that fails too, before the next append). - FsStore drops a failed append handle so the next attempt reopens the file; truncate of a never-created file is a no-op. - The loader recognises a CRC-valid record glued after a fragment (logs written by older builds) and skips only the fragment. - log_bytes now counts the newline added to an unterminated last record. - close_library writes the closing snapshot even when appending fails; the snapshot absorbs the queued ops (Journal::snapshot_with_unlogged). Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/journal.rs | 75 +++++++- crates/catalog/src/lib.rs | 2 + crates/catalog/src/store.rs | 24 ++- crates/catalog/src/tests_torn_append.rs | 223 ++++++++++++++++++++++++ crates/engine/src/library.rs | 25 ++- crates/engine/src/tests_persist.rs | 48 ++++- docs/parity.md | 2 +- docs/whats-new.md | 4 + 8 files changed, 388 insertions(+), 15 deletions(-) create mode 100644 crates/catalog/src/tests_torn_append.rs diff --git a/crates/catalog/src/journal.rs b/crates/catalog/src/journal.rs index adc8348d..8582673f 100644 --- a/crates/catalog/src/journal.rs +++ b/crates/catalog/src/journal.rs @@ -10,6 +10,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 @@ -62,6 +68,9 @@ 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). @@ -140,6 +149,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. @@ -182,6 +194,18 @@ pub fn decode_record(line: &str) -> Option<(u64, Op)> { serde_json::from_str(body).ok().map(|op| (seq, op)) } +/// 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 /// whole-file string). The bytes are exactly /// `{"format":"lightcraft-catalog","version":1,"seq":N,"catalog":}\n`, @@ -242,6 +266,7 @@ impl Journal { stats: PersistStats::default(), pending: None, retry_at_records: 0, + tail_dirty: false, }; let log = log.unwrap_or_default(); @@ -261,7 +286,17 @@ 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); + } + match record { Some((seq, op)) if seq <= j.seq => { // already in the snapshot let _ = op; @@ -301,10 +336,13 @@ 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; } - j.log_bytes = good_end as u64; } catalog.revision = 0; Ok((j, catalog, report)) @@ -323,7 +361,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 +394,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 +418,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 +429,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 +523,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..88bb719e 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -646,3 +646,5 @@ mod tests_background; mod tests_journal; #[cfg(test)] mod tests_local; +#[cfg(test)] +mod tests_torn_append; 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_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/library.rs b/crates/engine/src/library.rs index 6fcb08d2..aef79dd4 100644 --- a/crates/engine/src/library.rs +++ b/crates/engine/src/library.rs @@ -362,16 +362,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/tests_persist.rs b/crates/engine/src/tests_persist.rs index 90f76cad..4fb3f023 100644 --- a/crates/engine/src/tests_persist.rs +++ b/crates/engine/src/tests_persist.rs @@ -17,6 +17,8 @@ struct Flaky { fail: Arc, /// Only snapshots fail (appends work). snapshots_only: bool, + /// Only appends fail (a dead log handle; fresh files can still be written). + appends_only: bool, } impl Flaky { @@ -39,7 +41,7 @@ impl Store for Flaky { self.files.read(name) } fn write_atomic(&mut self, name: &str, data: &[u8]) -> std::io::Result<()> { - if self.failing() { + if self.failing() && !self.appends_only { return Err(std::io::Error::other("disk full")); } self.files.write_atomic(name, data) @@ -145,3 +147,47 @@ fn failed_snapshot_does_not_fail_the_command() { store.set_failing(false); assert_eq!(open(&store, false).catalog.albums().count(), 2); } + +/// Issue #101: quitting while the log can't be appended to still saves the queued changes in the +/// closing snapshot (a fresh file), and reports success; they are not appended again later. +#[test] +fn close_saves_queued_changes_in_the_snapshot_when_appends_fail() { + let store = Flaky { appends_only: true, ..Default::default() }; + let mut s = open(&store, true); + let id = s.selection.active.unwrap(); + store.set_failing(true); + assert!(matches!(s.execute("photo.rate", &json!({"rating": 4})), Err(EngineError::NotSaved(_)))); + assert!(matches!(s.execute("photo.flag", &json!({"flag": "pick"})), Err(EngineError::NotSaved(_)))); + assert_eq!(s.unsaved().map(|u| u.0), Some(2)); + s.close_library().unwrap(); + assert_eq!(s.unsaved(), None); + assert!(s.pending_log.is_empty()); + let expect = s.catalog.to_snapshot(); + drop(s); + store.set_failing(false); + let mut s2 = open(&store, true); + assert_eq!(s2.catalog.to_snapshot(), expect); + let p = s2.catalog.photo(id).unwrap(); + assert_eq!((p.rating, p.flag), (4, Flag::Pick)); + // later edits continue after the absorbed ops + s2.execute("photo.rate", &json!({"rating": 2})).unwrap(); + let expect = s2.catalog.to_snapshot(); + drop(s2); + assert_eq!(open(&store, true).catalog.to_snapshot(), expect); +} + +/// When neither the log nor the snapshot can be written, closing fails with `NotSaved` and the +/// changes stay queued (a later retry can still save them). +#[test] +fn close_fails_when_nothing_can_be_saved() { + let store = Flaky::default(); + let mut s = open(&store, true); + store.set_failing(true); + assert!(s.execute("photo.rate", &json!({"rating": 4})).is_err()); + let e = s.close_library().unwrap_err(); + assert!(matches!(e, EngineError::NotSaved(_)), "{e:?}"); + assert_eq!(s.unsaved().map(|u| u.0), Some(1)); + store.set_failing(false); + s.close_library().unwrap(); + assert_eq!(s.unsaved(), None); +} diff --git a/docs/parity.md b/docs/parity.md index 2f24d599..5e28b554 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -488,7 +488,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | Id | Feature | Tier | Status | Evidence | Missing / notes | |---|---|---|---|---|---| -| LR-BEHAV-AUTOSAVE | Instant autosave | P0 | ✅ | `crates/catalog/src/journal.rs`, `crates/engine/src/library.rs` | every change fsynced before the command returns; a failed write fails the command ("saved in memory but not written to disk"), keeps the change queued and retries; top-bar warning until saved | +| LR-BEHAV-AUTOSAVE | Instant autosave | P0 | ✅ | `crates/catalog/src/journal.rs`, `crates/engine/src/library.rs` | every change fsynced before the command returns; a failed write fails the command ("saved in memory but not written to disk"), keeps the change queued and retries; top-bar warning until saved; a write that fails part-way is cut back to the last whole record so the retry never reads back as damage (logs from older builds with such a fragment load in full); quitting saves queued changes in the closing snapshot even when the log can't be appended to | | LR-BEHAV-UNDO | Global undo | P0 | ✅ | `cmd:edit.undo`, `crates/engine/src/tests.rs` (`rating_flag_undo_redo`) | covers ratings, albums, deletes, edits | | LR-BEHAV-MULTISELECT | Multi-selection | P0 | ✅ | `cmd:library.select` (`replace`/`add`/`toggle`/`range`), `cmd:library.selectAll` | | | LR-BEHAV-BATCH | Batch apply to selection | P0 | ✅ | `cmd:photo.rate`, `cmd:develop.paste`, `cmd:preset.apply`, `cmd:app.export` | | diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..7003a4d5 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -10,6 +10,10 @@ - 23 new built-in presets: Portrait, Landscape, Urban, Food, Seasons, Vintage and B&W toners. ### Reliability +- A save that fails part-way (a full disk, a network share that drops) no longer looks like a damaged catalog + afterwards (issue #101): the partial write is cut off before LightCraft retries, so the next launch replays every + change. Catalogs already holding such a fragment load in full. Quitting while the catalog log can't be written + still saves your queued changes in the closing snapshot. - Canon CR2 photos from the EOS 7D, 50D, 60D, 550D, 600D, 1200D, 1300D, 5D Mark II and 1D Mark IV (and other models whose sensor starts on a green-blue row) no longer come out magenta (issue #85): the colour-filter layout is read from each file instead of assumed.