pcraft directory saves: re-verify a rolling share of cached objects; stronger file signature (#821)

Follow-up to #752. After a directory save verifies an object, later saves trust it while its
file signature is unchanged. A file damaged in place with the same length and modification
time (bit rot, or a tool that restores timestamps) was trusted until the writer was recreated.

- The signature also carries inode, device and change time on Unix, which an in-place rewrite
  changes even when the modification time is restored.
- Each directory save re-reads and re-checks cached objects, oldest-checked first, up to
  DEFAULT_REVERIFY_BUDGET (16 MiB of compressed data; at least one object), so over enough
  saves every object is read again and a damaged one is rewritten. Repeat saves of small
  documents stay a full check; large ones pay a bounded cost per save.
  PcraftWriter::set_reverify_budget tunes it (0 = trust signatures, u64::MAX = check all).
- PcraftWriter's doc comment states what the cache trusts.
- Tests: a same-size, same-mtime tamper is repaired on the next save (fails with the budget
  at 0 on Windows, where there is no change time), and with a 1-byte budget the rolling
  re-check reaches the damaged object within one pass over the objects.

Co-authored-by: Claude Opus 5.5 <[email protected]>

crates/format/src/lib.rs

@@ -43,7 +43,7 @@ pub use autosave::{Autosaver, RecoveryEntry, discard_recovery, list_recovery, re
 pub use convert::MAX_GROUP_DEPTH;
 pub use manifest::{FORMAT_VERSION, Manifest};
 pub use read::read_file;
-pub use store::{PcraftWriter, SaveStats};
+pub use store::{DEFAULT_REVERIFY_BUDGET, PcraftWriter, SaveStats};
 
 /// File extension of the native format.
 pub const EXTENSION: &str = "pcraft";

crates/format/src/store.rs

@@ -313,27 +313,60 @@ fn tile_le(t: &Tile, sample: SampleType) -> std::borrow::Cow<'_, [u8]> {
     }
 }
 
+/// Default for [`PcraftWriter::set_reverify_budget`]: compressed bytes of already-verified
+/// objects re-read per directory save.
+pub const DEFAULT_REVERIFY_BUDGET: u64 = 16 * 1024 * 1024;
+
 /// Incremental `.pcraft` writer. Keep one per open document: it remembers
 /// tile hashes (by `Arc` identity), compressed objects, and objects it has
 /// verified in the current directory, so repeated saves stay cheap.
-#[derive(Default)]
+///
+/// **What the directory cache trusts.** A directory save reuses an existing object file only
+/// after decompressing it and checking its content hash. After that, the writer remembers the
+/// file's signature (length and modification time, plus inode, device and change time on Unix)
+/// and skips the full check on later saves while the signature is unchanged. A file damaged in
+/// place without changing its signature (bit rot, or a tool that restores timestamps where
+/// there is no change time) would be trusted, so each save also re-verifies a rolling share of
+/// the cached objects, oldest-checked first, up to [`DEFAULT_REVERIFY_BUDGET`] bytes (see
+/// [`Self::set_reverify_budget`]). Over enough saves every object is read again, and a damaged
+/// one is rewritten.
 pub struct PcraftWriter {
     hash_cache: HashMap<usize, (Weak<Tile>, Hash)>,
     /// Compressed objects by bundle path (ZIP mode).
     compressed: HashMap<String, Arc<Vec<u8>>>,
     verified_directory: Option<DirectoryVerificationCache>,
+    reverify_budget: u64,
+}
+
+impl Default for PcraftWriter {
+    fn default() -> Self {
+        Self { hash_cache: HashMap::new(), compressed: HashMap::new(), verified_directory: None, reverify_budget: DEFAULT_REVERIFY_BUDGET }
+    }
 }
 
 #[derive(Default)]
 struct DirectoryVerificationCache {
     directory: PathBuf,
-    objects: HashMap<String, FileSignature>,
+    /// Directory saves so far; an object's `checked_at` is the save that last read it fully.
+    generation: u64,
+    objects: HashMap<String, Verified>,
 }
 
+#[derive(Clone, Copy)]
+struct Verified {
+    signature: FileSignature,
+    checked_at: u64,
+}
+
+/// What a file looked like when it was verified. Any change means "verify again".
 #[derive(Clone, Copy, PartialEq, Eq)]
 struct FileSignature {
     len: u64,
     modified: SystemTime,
+    /// Inode, device and status-change time (s, ns): an in-place rewrite changes the change
+    /// time even when a tool restores the modification time.
+    #[cfg(unix)]
+    unix: (u64, u64, i64, i64),
 }
 
 struct Prepared {
@@ -366,7 +399,17 @@ impl Object {
 }
 
 fn file_signature(metadata: &std::fs::Metadata) -> Option<FileSignature> {
-    Some(FileSignature { len: metadata.len(), modified: metadata.modified().ok()? })
+    #[cfg(unix)]
+    let unix = {
+        use std::os::unix::fs::MetadataExt;
+        (metadata.ino(), metadata.dev(), metadata.ctime(), metadata.ctime_nsec())
+    };
+    Some(FileSignature {
+        len: metadata.len(),
+        modified: metadata.modified().ok()?,
+        #[cfg(unix)]
+        unix,
+    })
 }
 
 fn path_signature(path: &Path) -> Result<Option<FileSignature>> {
@@ -450,6 +493,13 @@ impl PcraftWriter {
         Self::default()
     }
 
+    /// How many compressed bytes of already-verified objects each directory save re-reads and
+    /// re-checks (oldest-checked first; at least one object when the budget is non-zero). `0`
+    /// trusts unchanged signatures entirely; `u64::MAX` re-verifies everything on every save.
+    pub fn set_reverify_budget(&mut self, bytes: u64) {
+        self.reverify_budget = bytes;
+    }
+
     fn prepare(&mut self, doc: &Document, opts: &SaveOptions) -> Result<Prepared> {
         self.hash_cache.retain(|_, (w, _)| w.strong_count() > 0);
         convert::check_nesting(&doc.layers)?;
@@ -529,14 +579,24 @@ impl PcraftWriter {
         }
         let canonical_dir = std::fs::canonicalize(dir)?;
         if self.verified_directory.as_ref().is_none_or(|cache| cache.directory != canonical_dir) {
-            self.verified_directory = Some(DirectoryVerificationCache { directory: canonical_dir, objects: HashMap::new() });
+            self.verified_directory = Some(DirectoryVerificationCache { directory: canonical_dir, ..Default::default() });
         }
         let existing = list_objects(dir)?;
+        let generation = match &mut self.verified_directory {
+            Some(cache) => {
+                cache.generation = cache.generation.saturating_add(1);
+                cache.generation
+            }
+            None => 0,
+        };
+        let recheck = self.rolling_recheck(&p.objects);
         for (path, obj) in &p.objects {
             let object_path = dir.join(path);
             let signature = path_signature(&object_path)?;
-            let already_verified =
-                signature.is_some_and(|signature| self.verified_directory.as_ref().is_some_and(|cache| cache.objects.get(path) == Some(&signature)));
+            let already_verified = !recheck.contains(path)
+                && signature.is_some_and(|signature| {
+                    self.verified_directory.as_ref().is_some_and(|cache| cache.objects.get(path).is_some_and(|v| v.signature == signature))
+                });
             if existing.contains(path) && already_verified {
                 if matches!(obj, Object::Tile(..)) {
                     stats.tiles_reused += 1;
@@ -549,7 +609,7 @@ impl PcraftWriter {
                     if let Some(signature) = signature
                         && let Some(cache) = &mut self.verified_directory
                     {
-                        cache.objects.insert(path.clone(), signature);
+                        cache.objects.insert(path.clone(), Verified { signature, checked_at: generation });
                     }
                     if matches!(obj, Object::Tile(..)) {
                         stats.tiles_reused += 1;
@@ -562,10 +622,16 @@ impl PcraftWriter {
                 Object::Blob(_) => stats.blobs_written += 1,
             }
             write_atomic(&object_path, &obj.compressed())?;
-            if let Some(signature) = path_signature(&object_path)?
-                && let Some(cache) = &mut self.verified_directory
-            {
-                cache.objects.insert(path.clone(), signature);
+            let written = path_signature(&object_path)?;
+            if let Some(cache) = &mut self.verified_directory {
+                match written {
+                    Some(signature) => {
+                        cache.objects.insert(path.clone(), Verified { signature, checked_at: generation });
+                    }
+                    None => {
+                        cache.objects.remove(path);
+                    }
+                }
             }
         }
         for (name, data) in &p.previews {
@@ -589,6 +655,29 @@ impl PcraftWriter {
         Ok(stats)
     }
 
+    /// The cached objects this save re-verifies even though their signature is unchanged:
+    /// oldest-checked first, until their recorded sizes reach the re-verify budget (always at
+    /// least one, so an object larger than the budget is still reached in turn).
+    fn rolling_recheck(&self, objects: &BTreeMap<String, Object>) -> HashSet<String> {
+        let mut out = HashSet::new();
+        let Some(cache) = &self.verified_directory else { return out };
+        if self.reverify_budget == 0 {
+            return out;
+        }
+        let mut candidates: Vec<(&String, &Verified)> = cache.objects.iter().filter(|(path, _)| objects.contains_key(*path)).collect();
+        candidates.sort_by(|a, b| a.1.checked_at.cmp(&b.1.checked_at).then_with(|| a.0.cmp(b.0)));
+        let mut spent = 0u64;
+        for (path, verified) in candidates {
+            let next = spent.saturating_add(verified.signature.len);
+            if !out.is_empty() && next > self.reverify_budget {
+                break;
+            }
+            spent = next;
+            out.insert(path.clone());
+        }
+        out
+    }
+
     /// Save to `path`: a directory bundle if `path` is an existing directory
     /// or ends with a path separator, otherwise a ZIP file (written atomically).
     pub fn save_path(&mut self, doc: &Document, path: &Path, opts: &SaveOptions) -> Result<SaveStats> {

crates/format/tests/corruption.rs

@@ -175,6 +175,77 @@ fn directory_bundle_resave_repairs_tampered_objects() {
     std::fs::remove_dir_all(dir).unwrap();
 }
 
+/// Damages one object in place without changing its length or modification time, the case a
+/// size-and-mtime cache alone would trust. Returns the damaged file's path.
+fn damage_in_place_keeping_size_and_mtime(dir: &std::path::Path) -> std::path::PathBuf {
+    let object = std::fs::read_dir(dir.join("tiles")).unwrap().next().unwrap().unwrap().path();
+    let original = std::fs::read(&object).unwrap();
+    let mtime = std::fs::metadata(&object).unwrap().modified().unwrap();
+    // Flip one byte of the compressed payload; try positions until the bundle no longer loads.
+    for at in (original.len() / 2..original.len()).chain(0..original.len() / 2) {
+        let mut damaged = original.clone();
+        damaged[at] ^= 0x5a;
+        std::fs::write(&object, &damaged).unwrap();
+        std::fs::File::options().write(true).open(&object).unwrap().set_modified(mtime).unwrap();
+        if load_path(dir).is_err() {
+            assert_eq!(std::fs::metadata(&object).unwrap().len(), original.len() as u64);
+            assert_eq!(std::fs::metadata(&object).unwrap().modified().unwrap(), mtime);
+            return object;
+        }
+    }
+    panic!("no single-byte change made the bundle fail to load");
+}
+
+#[test]
+fn same_size_same_mtime_tamper_is_repaired_on_the_next_save() {
+    let doc = rich_doc(ColorMode::Rgb, SampleType::U8);
+    let dir = temp_dir("tamper-same-size");
+    let mut writer = PcraftWriter::new();
+    writer.save_dir(&doc, &dir, &SaveOptions::default()).unwrap();
+    // A second save trusts every object it verified in the first one.
+    writer.save_dir(&doc, &dir, &SaveOptions::default()).unwrap();
+
+    damage_in_place_keeping_size_and_mtime(&dir);
+    // The sample bundle is far below DEFAULT_REVERIFY_BUDGET, so the rolling re-check reads
+    // every cached object again (and on Unix the change time differs as well).
+    let stats = writer.save_dir(&doc, &dir, &SaveOptions::default()).unwrap();
+    assert_eq!(stats.tiles_written, 1);
+    assert_eq!(load_path(&dir).unwrap(), doc);
+    std::fs::remove_dir_all(dir).unwrap();
+}
+
+#[test]
+fn rolling_reverify_reaches_every_object_with_a_tiny_budget() {
+    let doc = rich_doc(ColorMode::Rgb, SampleType::U8);
+    let dir = temp_dir("tamper-rolling");
+    let mut writer = PcraftWriter::new();
+    writer.save_dir(&doc, &dir, &SaveOptions::default()).unwrap();
+    let objects = ["tiles", "blobs"].iter().map(|d| std::fs::read_dir(dir.join(d)).unwrap().count()).sum::<usize>();
+
+    // One object per save: the oldest-checked one.
+    writer.set_reverify_budget(1);
+    damage_in_place_keeping_size_and_mtime(&dir);
+    let mut repaired = false;
+    for _ in 0..=objects {
+        let stats = writer.save_dir(&doc, &dir, &SaveOptions::default()).unwrap();
+        if stats.tiles_written == 1 {
+            repaired = true;
+            break;
+        }
+    }
+    assert!(repaired, "a damaged object was not re-verified within {objects} saves");
+    assert_eq!(load_path(&dir).unwrap(), doc);
+
+    // A zero budget re-reads nothing whose signature is unchanged; a fresh writer still verifies
+    // everything on its first save.
+    let mut fresh = PcraftWriter::new();
+    fresh.set_reverify_budget(0);
+    damage_in_place_keeping_size_and_mtime(&dir);
+    assert_eq!(fresh.save_dir(&doc, &dir, &SaveOptions::default()).unwrap().tiles_written, 1);
+    assert_eq!(load_path(&dir).unwrap(), doc);
+    std::fs::remove_dir_all(dir).unwrap();
+}
+
 #[test]
 fn deflated_zip_entries_are_readable() {
     // Re-zip with DEFLATE (as a user's zip tool might) — must still load.