From ee4b78f64679fc5007cc917ce8aa1d2ef0027e2b Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 17:37:48 -0400 Subject: [PATCH] Issue #99: Library: exclusive lock so two processes can't open one library Two processes on one library each kept their own catalog and seq counter; the last snapshot silently dropped the other's edits. - lightcraft_catalog::lock: LibraryLock holds an exclusive OS lock (std File::try_lock: flock / LockFileEx) on /catalog.lock for the session; who holds it goes to catalog.lock.owner (separate file: a Windows lock also blocks reads). Released on drop and by the OS when the process dies, so leftover files from a crash never lock anyone out. File systems without lock support open unprotected with a warning. - Session::open_library takes the lock before reading anything and fails with EngineError::LibraryInUse ("this library is already open in LightCraft 0.x (process N on host)"); reopening the session's own library hands its lock over. - lightcraft-cli (mcp/run/snapshot --library) adds how to drive the running app instead (--control + mcp --connect). - Tests: in-process, leftover files, a killed child process, two engine sessions, CLI e2e (mcp --library holds it, run --library is refused). Existing tests that reopened a library while it was open now drop the first session. - docs: parity row LRC-LIB-CATALOG-LOCK, mcp.md, web.md (no guard across browser tabs yet), whats-new. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/src/main.rs | 17 +++- apps/lightcraft-cli/tests/cli.rs | 28 ++++++ crates/catalog/src/lib.rs | 4 + crates/catalog/src/lock.rs | 140 +++++++++++++++++++++++++++++ crates/catalog/src/tests_lock.rs | 93 +++++++++++++++++++ crates/engine/src/lib.rs | 3 + crates/engine/src/library.rs | 63 +++++++++++-- crates/engine/src/tests_import.rs | 1 + crates/engine/src/tests_library.rs | 31 +++++++ crates/engine/src/tests_prefs.rs | 2 + docs/mcp.md | 5 +- docs/parity.md | 7 +- docs/web.md | 4 + docs/whats-new.md | 4 + 14 files changed, 388 insertions(+), 14 deletions(-) create mode 100644 crates/catalog/src/lock.rs create mode 100644 crates/catalog/src/tests_lock.rs diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index 4f68ec88..152326c0 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(), @@ -580,7 +591,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(), diff --git a/apps/lightcraft-cli/tests/cli.rs b/apps/lightcraft-cli/tests/cli.rs index 913200d2..12fe9db0 100644 --- a/apps/lightcraft-cli/tests/cli.rs +++ b/apps/lightcraft-cli/tests/cli.rs @@ -242,3 +242,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/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 4b9bdcdc..813a0857 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -12,6 +12,7 @@ pub mod dates; pub mod journal; pub mod keywords; pub mod local; +pub mod lock; pub mod model; pub mod query; pub mod rules; @@ -26,6 +27,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}; @@ -646,3 +648,5 @@ mod tests_background; mod tests_journal; #[cfg(test)] mod tests_local; +#[cfg(test)] +mod tests_lock; 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/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/engine/src/lib.rs b/crates/engine/src/lib.rs index dcbfd9ed..30c7e4d2 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -56,6 +56,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), } diff --git a/crates/engine/src/library.rs b/crates/engine/src/library.rs index 6fcb08d2..81730abf 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}; @@ -64,6 +64,9 @@ pub struct Library { pub forgot_local: Option, presets_written: String, view_written: Vec, + /// 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`]). @@ -171,6 +174,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(), + } +} + fn read_json(store: &mut dyn Store, name: &str) -> Option { store.read(name).ok().flatten().and_then(|b| serde_json::from_slice(&b).ok()) } @@ -178,16 +198,47 @@ fn read_json(store: &mut dyn Store, name: &str) 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 @@ -277,6 +328,7 @@ impl Session { forgot_local: None, presets_written, view_written, + lock: lock.take(), }); // forget untouched Local records of folders not browsed for a while (journaled at once) if self.forget_local_days > 0 { @@ -290,10 +342,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. diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index 6be7ce9a..4828eca2 100644 --- a/crates/engine/src/tests_import.rs +++ b/crates/engine/src/tests_import.rs @@ -590,6 +590,7 @@ fn smart_previews_folder_is_chosen_per_library() { assert_eq!(s.execute("photo.smartPreview", &json!({})).unwrap()["smartPreview"], true); // the setting is saved with the library + drop(s); // one session per library (issue #99) let mut again = Session::new().with_fs(); again.open_library(&lib, false).unwrap(); assert_eq!(again.execute("library.smartPreviewsLocation", &json!({})).unwrap()["path"], to.to_string_lossy().as_ref()); diff --git a/crates/engine/src/tests_library.rs b/crates/engine/src/tests_library.rs index fd09fdc8..45a56b2e 100644 --- a/crates/engine/src/tests_library.rs +++ b/crates/engine/src/tests_library.rs @@ -113,6 +113,7 @@ fn close_writes_snapshot_and_view_state() { let expect = s.catalog.to_snapshot(); s.close_library().unwrap(); assert_eq!(std::fs::metadata(dir.join("catalog.log")).unwrap().len(), 0); + drop(s); // one session per library (issue #99) let s2 = open(&dir, true); let r = &s2.library.as_ref().unwrap().report; @@ -178,6 +179,7 @@ fn new_library_without_seed_is_empty_and_compacts() { assert!(p["snapshots"].as_u64() >= Some(1), "{p}"); assert!(p["lastSnapshot"]["bytes"].as_u64() > Some(0), "{p}"); assert!(p["lastSnapshot"]["totalMs"].as_f64() >= p["lastSnapshot"]["serializeMs"].as_f64(), "{p}"); + drop(s); // one session per library (issue #99) let s2 = open(&dir, true); assert_eq!(s2.catalog.len(), 0, "an existing library is never seeded"); assert_eq!(s2.catalog.albums().count(), 1); @@ -376,3 +378,32 @@ fn scale_100k_library_queries() { worst.sort_by(|a, b| b.1.total_cmp(&a.1)); eprintln!("slowest: {:?}", &worst[..3]); } + +/// Issue #99: a library is open in one session at a time. A second opener is refused (nothing +/// read or written), the owning session can reopen it, and it is free again once closed. +#[test] +fn a_library_open_elsewhere_is_refused() { + let dir = temp_dir("locked"); + let mut s = open(&dir, true); + s.execute("photo.rate", &json!({"rating": 3})).unwrap(); + let log = std::fs::read(dir.join("catalog.log")).unwrap(); + + let mut other = Session::new(); + let e = other.open_library(&dir, true).unwrap_err(); + assert!(matches!(e, crate::EngineError::LibraryInUse(_)), "{e:?}"); + assert!(e.to_string().contains("already open in"), "{e}"); + assert!(other.library.is_none()); + assert_eq!(std::fs::read(dir.join("catalog.log")).unwrap(), log, "the refused opener wrote nothing"); + + // the session that has it open may reopen it (Settings → Open Library on the same folder) + s.close_library().unwrap(); + s.open_library(&dir, true).unwrap(); + assert!(other.open_library(&dir, true).is_err(), "still locked after reopening"); + s.execute("photo.rate", &json!({"rating": 4})).unwrap(); + let expect = s.catalog.to_snapshot(); + + drop(s); + other.open_library(&dir, true).unwrap(); + assert_eq!(other.catalog.to_snapshot(), expect); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/engine/src/tests_prefs.rs b/crates/engine/src/tests_prefs.rs index 8dc2256c..06cad3a6 100644 --- a/crates/engine/src/tests_prefs.rs +++ b/crates/engine/src/tests_prefs.rs @@ -126,11 +126,13 @@ fn preferences_persist_with_the_library() { assert_eq!(r["persistent"], true); // export presets are library preferences too s.execute("export.savePreset", &json!({"name": "Proof", "params": {"format": "jpeg", "percent": 25}})).unwrap(); + drop(s); // one session per library (issue #99) let mut s = Session::new().with_fs(); s.open_library(&lib, false).unwrap(); assert_eq!(s.export_params(&json!({"preset": "Proof"})).unwrap(), json!({"format": "jpeg", "percent": 25})); // older prefs.json files (snake_case `last_export`, no import section) still load std::fs::write(lib.join("prefs.json"), r#"{"xmp": {"autoWrite": false}, "last_export": {"format": "png"}}"#).unwrap(); + drop(s); let mut s = Session::new().with_fs(); s.open_library(&lib, false).unwrap(); assert_eq!(s.last_export, Some(json!({"format": "png"}))); diff --git a/docs/mcp.md b/docs/mcp.md index 9b4c65f1..492e655d 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -15,7 +15,10 @@ It runs in one of two modes: Options: `--library DIR` opens (or creates) a persistent LightCraft library — the same crash-safe format the desktop app uses (`~/Pictures/LightCraft Library` by default there) — so ratings, edits and albums -survive between sessions (with `--demo`, a new library is seeded with the demo photos); +survive between sessions (with `--demo`, a new library is seeded with the demo photos). A library +is open in one program at a time (`catalog.lock` in the library folder): while the desktop app has +it open, `--library` on the same folder fails with "This library is already open in LightCraft +(process N …)" — use connect mode to work with the running app instead; `--demo` starts the headless session with the procedurally generated demo library; `--compact` lists only the helper tools (see below). In connect mode the server starts even when the app is not running yet and connects on the first call (and reconnects if the app restarts). diff --git a/docs/parity.md b/docs/parity.md index 2f24d599..16f83ea8 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -47,10 +47,10 @@ and prints the summary; `cargo xtask parity --write` refreshes the summary table | X. Cross-cutting behaviours (BEHAV) | 15 | 2 | 2 | 1 | 8/8 (100%) | 6/7 (86%) | | Y. Menus | 81 | 0 | 4 | 8 | 47/47 (100%) | 23/23 (100%) | | Z. Keyboard shortcuts (desktop) | 74 | 3 | 3 | 1 | 49/52 (94%) | 22/23 (96%) | -| Lightroom Classic extras | 30 | 9 | 40 | 9 | — | 20/21 (95%) | -| **Total** | 386 | 28 | 91 | 37 | 194/200 (97%) | 138/147 (94%) | +| Lightroom Classic extras | 31 | 9 | 40 | 9 | — | 21/22 (95%) | +| **Total** | 387 | 28 | 91 | 37 | 194/200 (97%) | 139/148 (94%) | -Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **79.2%** of 505 in-scope rows — P0 98.5% of 200 · P1 95.9% of 147 · P2 39.2% of 158. +Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **79.2%** of 506 in-scope rows — P0 98.5% of 200 · P1 95.9% of 148 · P2 39.2% of 158. ## Top gaps @@ -713,6 +713,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06- | LRC-LIB-SURVEY | Survey view | P2 | ✅ | `cmd:view.survey` | N | | LRC-LIB-REFVIEW | Reference view | P2 | ✅ | `cmd:view.reference`, `cmd:photo.setReference`, `crates/ui-egui/src/panels/compare.rs` (`show_reference`) | View ▸ Reference View (⇧R): the reference photo beside the active one (edited with the Edit panel); photo menu ▸ Set as Reference Photo | | LRC-LIB-CATALOG-PANEL | Catalog sets | P1 | ✅ | `cmd:library.source` (`missing`) | all, recently added, picks, recently deleted, missing photos (when any) | +| LRC-LIB-CATALOG-LOCK | Catalog open in one program at a time | P1 | ✅ | `cmd:app.openLibrary`, `crates/catalog/src/lock.rs`, `crates/engine/src/library.rs` (`open_library`) | exclusive OS lock on `catalog.lock` for the session (app and `lightcraft-cli --library`); a second opener is refused with who has it (`catalog.lock.owner`: program, process, computer); released by the OS on a crash, so never stale; file systems without locks open unprotected with a warning; no read-only mode; the browser build (OPFS) has no guard across tabs yet | | LRC-LIB-FOLDERS | Disk folder tree | P1 | ✅ | `cmd:library.browse`, `cmd:folder.rename`, `cmd:folder.move`, `crates/ui-egui/src/panels/left.rs` (`local_section`, `folder_tree`) | standard places + the browsed folder + Browse Folder…, each an expandable tree of subfolders, breadcrumb navigation; context menu: Rename Folder…, Move Folder To…, Show in Finder (on disk, photos relinked; like the file manager, not an undo step) | | LRC-LIB-COLLECTIONS | Collections & sets | P1 | ✅ | `cmd:album.create`, `cmd:album.createSmart`, `cmd:album.toggleTarget`, `cmd:album.setTarget`, `cmd:album.clearQuick` | albums + folders (sets), smart albums (rule editor), Quick Collection and target album (B in the grids adds / removes the selection; album menu ▸ Set as Target Album; the target is marked +) | | LRC-LIB-SMARTCOLL | Smart-collection rules | P1 | ✅ | `cmd:album.createSmart`, `cmd:album.setRules`, `cmd:album.ruleFields`, `cmd:dialog.smartAlbum`, `crates/catalog/src/rules.rs`, `crates/ui-egui/src/panels/rules_editor.rs` | rule editor: match all / any / none, nested groups (⌥+ or + Group), 26 fields (rating, flag, label, type, edits, keywords, any text, filename, format, title, caption, camera, lens, location, creator, copyright, copyright status, capture / import / edit date, ISO, aperture, focal length, megapixels, GPS, virtual copy, album) with text / number / date / in-the-last operators; live match count; also usable as a library filter (`ruleSet`) | diff --git a/docs/web.md b/docs/web.md index 6efe1345..ecd2cff3 100644 --- a/docs/web.md +++ b/docs/web.md @@ -80,6 +80,10 @@ same-origin or send CORP/CORS headers (the bundle has no third-party resources). the background within a frame or two, each file replaced atomically, in modification order (`apps/lightcraft-web/src/files.rs`). View state and UI prefs are saved every second when they change (a tab can close without notice). + - Known limitation: the browser storage has no file locks, so two tabs of the same origin open + the same library and the last one to write a snapshot wins (the desktop app and the CLI lock + a library folder: `catalog.lock`, issue #99). Use one tab at a time; a guard through the Web + Locks API (`navigator.locks`) is still to do. - `originals/`: the bytes of every imported photo. The catalog refers to them as `web//`; the main thread keeps recently used originals in memory (≤ 768 MB) and loads the rest on demand (the active photo is prefetched). diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..14f5c477 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -38,6 +38,10 @@ - Smaller, faster catalogs: photos you only looked at in Local (never added, rated or edited) are forgotten once their folder has not been browsed for 30 days — your files and sidecars stay, and browsing the folder shows them again. Change the period (or turn it off) in Settings → Performance. +- A library is open in one program at a time (issue #99): opening a library that LightCraft or `lightcraft-cli` already + has open — on this computer or another one sharing the folder — says who has it ("already open in LightCraft (process + 123 on studio-mac)") instead of letting both write and silently drop each other's edits. A crash never leaves the + library locked: the lock is the operating system's and goes away with the program. ### Editing - Auto Sync: edits apply to every selected photo. Auto B&W mix. Automatic versions.