diff --git a/book/src/formats/pcraft.md b/book/src/formats/pcraft.md index 39385877a1..40ecbdd127 100644 --- a/book/src/formats/pcraft.md +++ b/book/src/formats/pcraft.md @@ -8,8 +8,11 @@ tiles/.zst blobs/.zst thumb.png composite/preview.png +save.lock ``` +`save.lock` exists only in directory bundles: a save holds an OS file lock on it while it writes objects, publishes the manifest and removes unreferenced objects, so concurrent saves of one bundle take turns. It is empty, and readers ignore it. + The manifest is versioned and describes the document tree. Tiles and blobs are content-addressed. Loaders verify ZIP entry bounds and CRCs; document content also uses stored hashes to detect mismatched or corrupted tile/blob content. Unsupported newer format versions fail explicitly. ## Implemented load limits diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 0c87720212..992e925ae9 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -8,6 +8,7 @@ //! blobs/.zst content-addressed binary data (ICC, EXIF, PSD blocks, smart objects) //! thumb.png optional thumbnail (caller-rendered) //! composite/preview.png optional flattened preview (caller-rendered) +//! save.lock directory bundles only: held while a save runs (empty) //! ``` //! //! A bundle is either a ZIP archive (STORE entries) or a directory with the @@ -15,7 +16,9 @@ //! already compressed/wrote, so only new tiles are encoded. Directory saves //! verify existing content-addressed objects once per writer and folder, //! rechecking files whose size or modification time changes; they replace -//! damaged or missing objects and garbage-collect unreferenced ones. +//! damaged or missing objects and garbage-collect unreferenced ones. A directory +//! save holds an OS file lock on `save.lock` throughout, so two writers (or two +//! processes) saving one bundle take turns instead of removing each other's objects. //! //! This crate sits at L3 next to the compositor, so it does not render. //! Callers pass previews in [`SaveOptions`]; `photocraft-io` does that. diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index 83e6d3fad0..b8d08e6429 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -18,6 +18,10 @@ use crate::{FormatError, LoadOptions, Result, SaveOptions}; pub(crate) const MANIFEST: &str = "manifest.json"; pub(crate) const THUMB: &str = "thumb.png"; pub(crate) const COMPOSITE: &str = "composite/preview.png"; +/// The file a directory save holds an exclusive lock on for its whole transaction (#4957). +pub(crate) const SAVE_LOCK: &str = "save.lock"; +/// How long a directory save waits for another save of the same bundle to finish. +const SAVE_LOCK_WAIT: std::time::Duration = std::time::Duration::from_secs(10); fn tile_path(h: &str) -> String { format!("tiles/{h}.zst") @@ -390,6 +394,8 @@ pub struct PcraftWriter { compressed_budget: usize, verified_directory: Option, reverify_budget: u64, + /// How long [`Self::save_dir`] waits for another save of the bundle ([`SAVE_LOCK_WAIT`]). + lock_wait: std::time::Duration, } impl Default for PcraftWriter { @@ -401,6 +407,35 @@ impl Default for PcraftWriter { compressed_budget: usize::MAX, verified_directory: None, reverify_budget: DEFAULT_REVERIFY_BUDGET, + lock_wait: SAVE_LOCK_WAIT, + } + } +} + +/// Hold `dir`'s [`SAVE_LOCK`] for a directory save, so two writers (two stores, or two +/// processes) can't interleave: each lists the existing objects, writes its own, publishes its +/// manifest and removes what that manifest doesn't reference. Interleaved, one writer's cleanup +/// could remove an object the other's winning manifest needs (#4957). The lock is the OS's +/// advisory file lock, released when the returned file is dropped or the process ends, so a +/// crash leaves no stale lock. Another save holding it is waited for up to `wait`; after that the +/// save fails without writing anything. A file system without file locks (some network shares) +/// saves unlocked, as before. +fn lock_bundle(dir: &Path, wait: std::time::Duration) -> Result> { + let path = dir.join(SAVE_LOCK); + validate_bundle_file(dir, &path)?; + let file = std::fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(&path)?; + let start = std::time::Instant::now(); + loop { + match file.try_lock() { + Ok(()) => return Ok(Some(file)), + Err(std::fs::TryLockError::WouldBlock) if start.elapsed() < wait => std::thread::sleep(std::time::Duration::from_millis(20)), + Err(std::fs::TryLockError::WouldBlock) => { + return Err(FormatError::Io(std::io::Error::new( + std::io::ErrorKind::WouldBlock, + format!("another save of {} is in progress; nothing was written", dir.display()), + ))); + } + Err(std::fs::TryLockError::Error(_)) => return Ok(None), } } } @@ -763,6 +798,12 @@ impl PcraftWriter { self.compressed.clear(); } + /// How long a directory save waits for another save of the same bundle to finish before + /// failing without writing anything (10 s by default; see [`Self::save_dir`]). + pub fn set_lock_wait(&mut self, wait: std::time::Duration) { + self.lock_wait = wait; + } + fn prepare(&mut self, doc: &Document, opts: &SaveOptions) -> Result { self.hash_cache.retain(|_, (w, _)| w.strong_count() > 0); self.blob_hash_cache.retain(|_, (w, _)| w.strong_count() > 0); @@ -844,10 +885,13 @@ impl PcraftWriter { /// Save into a directory bundle: reuses only valid existing objects, /// rewrites missing or damaged objects, then writes the manifest - /// atomically and removes unreferenced objects. + /// atomically and removes unreferenced objects. The whole save holds the bundle's + /// `save.lock`, so saves of one bundle from other writers or processes wait their turn. pub fn save_dir(&mut self, doc: &Document, dir: &Path, opts: &SaveOptions) -> Result { let canonical_dir = bundle_root(dir)?; let dir = canonical_dir.as_path(); + // Held until the end of the save: listing, writing, publishing and cleanup (#4957). + let _lock = lock_bundle(dir, self.lock_wait)?; let existing = list_objects(dir)?; // Reject a hostile existing entry before writing any object or manifest. This also // covers entries that this save would otherwise leave untouched or garbage-collect. diff --git a/crates/format/tests/roundtrip.rs b/crates/format/tests/roundtrip.rs index 725bc2c070..b123214a58 100644 --- a/crates/format/tests/roundtrip.rs +++ b/crates/format/tests/roundtrip.rs @@ -768,6 +768,34 @@ fn a_non_finite_float_is_refused_at_save_rather_than_breaking_every_later_load() assert_eq!(back, doc); } +/// #4957: a directory save holds the bundle's lock for its whole transaction. While another +/// writer holds it, a save waits, then fails without writing anything; once it is released the +/// save goes ahead. +#[test] +fn directory_save_waits_for_the_bundle_lock() { + let doc = rich_doc(ColorMode::Rgb, SampleType::U8); + let dir = temp_dir("lock"); + let other = std::fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(dir.join("save.lock")).unwrap(); + // A file system without file locks saves unlocked (`lock_bundle`), so there's nothing to test. + match other.try_lock() { + Ok(()) => {} + Err(std::fs::TryLockError::Error(e)) if e.kind() == std::io::ErrorKind::Unsupported => return, + Err(std::fs::TryLockError::Error(e)) => panic!("locking save.lock failed: {e}"), + Err(std::fs::TryLockError::WouldBlock) => panic!("save.lock is already locked"), + } + let mut w = PcraftWriter::new(); + w.set_lock_wait(std::time::Duration::from_millis(100)); + let e = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap_err(); + assert!(e.to_string().contains("in progress"), "{e}"); + assert!(!dir.join("manifest.json").exists(), "nothing written while locked"); + assert!(!dir.join("tiles").exists() || std::fs::read_dir(dir.join("tiles")).unwrap().count() == 0); + other.unlock().unwrap(); + w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + assert_eq!(load_path(&dir).unwrap(), doc); + drop(other); + std::fs::remove_dir_all(dir).unwrap(); +} + #[test] fn variables_follow_layer_id_remapping() { let mut doc = rich_doc(ColorMode::Rgb, SampleType::U8); diff --git a/docs/development.md b/docs/development.md index 71380fcadb..e2a7dd5a59 100644 --- a/docs/development.md +++ b/docs/development.md @@ -203,6 +203,7 @@ cargo run -p photocraft-cli -- commands --filter blur # the c - `manifest.json`: the versioned document tree, with migrations in `format/src/migrate.rs`. - `tiles/.zst` and `blobs/.zst`: zstd-compressed objects, content-addressed by the BLAKE3 hash of their uncompressed bytes. - `thumb.png` and `composite/preview.png`: previews. +- `save.lock` (directory bundles only): an empty file a save holds an OS file lock on, so two writers or processes saving one bundle take turns (#4957). Keep one `PcraftWriter` per open document: re-saving then only compresses and writes tiles that changed. Directory bundles verify objects on first encounter in a folder; later saves reuse them while their file size and modification time are unchanged, and re-verify changed objects, repair missing or damaged objects, and garbage-collect unreferenced ones. `format::Autosaver` writes snapshots into a recovery directory on a background thread. `list_recovery` / `recover` / `discard_recovery` implement crash recovery, and `format::RecoveryStore` is the lifecycle the desktop app uses: new documents autosave under per-launch keys (document ids restart every launch, so they never overwrite an older entry), and a recovered document adopts the entry it came from. That entry is replaced in place by the next autosave and removed only when the document is saved or closed, never just because it was recovered, so a second crash loses nothing. The web build has no crash recovery (no autosave services).