From c20b81b25168057e9cbffa2f45c36ec02dacf756 Mon Sep 17 00:00:00 2001 From: Valentin Klein Date: Mon, 12 Oct 2026 05:31:23 +0200 Subject: [PATCH] Autosave: best-effort cleanup after commit; sweep orphaned recovery bundles (#3489) (#3502) save_dir removed unreferenced objects with `remove_file(..)?` after the manifest commit, so a vanished or locked file made a completed save (autosave) report failure. The cleanup is now best-effort. RecoveryStore::new sweeps recovery data nothing could load or clean up: bundles without a sidecar (crash during a first autosave) and *.pcsave.tmp files of other processes, only once untouched for 24 hours so another instance's in-progress save is never touched. Co-authored-by: Claude Opus 5.5 --- crates/format/src/autosave.rs | 121 +++++++++++++++++++++++++++++++- crates/format/src/store.rs | 8 ++- crates/format/tests/autosave.rs | 18 +++++ 3 files changed, 144 insertions(+), 3 deletions(-) diff --git a/crates/format/src/autosave.rs b/crates/format/src/autosave.rs index 114cdf2c..47674136 100644 --- a/crates/format/src/autosave.rs +++ b/crates/format/src/autosave.rs @@ -238,7 +238,9 @@ impl RecoveryStore { static STORES: AtomicU64 = AtomicU64::new(0); let t = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_nanos()).unwrap_or(0); let session = format!("{t:x}_{:x}_{:x}", std::process::id(), STORES.fetch_add(1, Ordering::Relaxed)); - RecoveryStore { dir: recovery_dir.into(), session, savers: HashMap::new(), adopted: HashMap::new() } + let dir = recovery_dir.into(); + sweep_orphans(&dir, ORPHAN_AGE); + RecoveryStore { dir, session, savers: HashMap::new(), adopted: HashMap::new() } } /// Load every recoverable document, newest first, with the entry it came from. Nothing is @@ -283,6 +285,81 @@ impl RecoveryStore { } } +/// How long recovery data must have been left untouched before [`sweep_orphans`] may remove +/// it. Far longer than any save takes, so another running instance's in-progress first +/// autosave (bundle written, sidecar not yet) is never mistaken for an orphan. +#[cfg(not(target_arch = "wasm32"))] +const ORPHAN_AGE: std::time::Duration = std::time::Duration::from_secs(24 * 60 * 60); + +/// Best-effort removal of recovery data nothing can ever load or clean up: +/// - `.pcraft` bundle folders with no `.json` sidecar (a crash during a document's +/// first autosave: the sidecar is written after the bundle, and `list_recovery` skips them); +/// - `*.pcsave.tmp` files left by an interrupted atomic write (see [`crate::atomic::temp_name`]) +/// that another process created. +/// +/// Only entries untouched for at least `min_age` are removed. Bundles with a sidecar, symlinks, +/// and anything outside `recovery_dir` are never touched; errors are ignored. +#[cfg(not(target_arch = "wasm32"))] +fn sweep_orphans(recovery_dir: &Path, min_age: std::time::Duration) { + let Ok(rd) = std::fs::read_dir(recovery_dir) else { return }; + let mut bundles = Vec::new(); + for e in rd.flatten() { + let Ok(ft) = e.file_type() else { continue }; + let path = e.path(); + if ft.is_file() { + sweep_temp_file(&path, min_age); + continue; + } + if !ft.is_dir() { + continue; + } + let name = e.file_name(); + let Some(key) = name.to_str().and_then(|n| n.strip_suffix(".pcraft")) else { continue }; + if key.is_empty() || sanitize(key) != key { + continue; + } + // Anything at the sidecar's path (even a broken one) keeps the bundle. + let has_sidecar = std::fs::symlink_metadata(recovery_dir.join(format!("{key}.json"))).is_ok(); + let parts = [path.clone(), path.join("tiles"), path.join("blobs")]; + if !has_sidecar && parts.iter().all(|p| untouched_for(p, min_age, true)) { + let _ = std::fs::remove_dir_all(&path); + } else { + bundles.extend(parts); + } + } + for dir in bundles { + let Ok(rd) = std::fs::read_dir(&dir) else { continue }; + for e in rd.flatten() { + if e.file_type().is_ok_and(|ft| ft.is_file()) { + sweep_temp_file(&e.path(), min_age); + } + } + } +} + +/// Remove `path` if it is an atomic-write temp file of another process, untouched for `min_age`. +#[cfg(not(target_arch = "wasm32"))] +fn sweep_temp_file(path: &Path, min_age: std::time::Duration) { + let Some(name) = path.file_name().and_then(|n| n.to_str()) else { return }; + // `.{stem}.{pid}-{n}.pcsave.tmp` + let Some(rest) = name.strip_prefix('.').and_then(|n| n.strip_suffix(".pcsave.tmp")) else { return }; + let Some((_, owner)) = rest.rsplit_once('.') else { return }; + let Some(pid) = owner.split_once('-').and_then(|(pid, _)| pid.parse::().ok()) else { return }; + if pid != std::process::id() && untouched_for(path, min_age, false) { + let _ = std::fs::remove_file(path); + } +} + +/// Whether `path` was last modified at least `min_age` ago. Unreadable times count as recent; +/// a missing path counts as untouched only when `missing_ok`. +#[cfg(not(target_arch = "wasm32"))] +fn untouched_for(path: &Path, min_age: std::time::Duration, missing_ok: bool) -> bool { + match std::fs::symlink_metadata(path) { + Ok(m) => m.modified().ok().and_then(|t| SystemTime::now().duration_since(t).ok()).is_some_and(|age| age >= min_age), + Err(e) => missing_ok && e.kind() == std::io::ErrorKind::NotFound, + } +} + fn remove_entry(dir: &Path, key: &str) -> Result<()> { let bundle = dir.join(format!("{key}.pcraft")); if bundle.exists() { @@ -318,4 +395,46 @@ mod tests { let msg = err.to_string(); assert!(msg.contains("could not start"), "{msg}"); } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn sweep_removes_orphaned_bundles_and_foreign_temp_files_only() { + let dir = std::env::temp_dir().join(format!( + "pcraft-sweep-{}-{}", + std::process::id(), + SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_nanos()).unwrap_or(0) + )); + let doc = + Arc::new(Document::new("Kept", photocraft_doc::Size { width: 4, height: 4 }, photocraft_color::ColorMode::Rgb, photocraft_color::SampleType::U8)); + let saver = Autosaver::new(&dir, "kept"); + saver.request(doc.clone(), 1, None, SaveOptions::default()); + assert!(saver.flush().is_some_and(|r| r.is_ok())); + // A first autosave that crashed before its sidecar was written. + let orphan = dir.join("doc-crashed-1.pcraft"); + std::fs::create_dir_all(orphan.join("tiles")).unwrap(); + std::fs::write(orphan.join("tiles/abc.zst"), b"partial").unwrap(); + // Stale temp files: another process's (swept) and this process's (kept, may be in flight). + let foreign = [dir.join(".kept.json.4294967294-3.pcsave.tmp"), dir.join("kept.pcraft/.manifest.json.4294967294-0.pcsave.tmp")]; + let own = dir.join(format!(".kept.json.{}-9.pcsave.tmp", std::process::id())); + for p in foreign.iter().chain([&own]) { + std::fs::write(p, b"tmp").unwrap(); + } + // Not ours to judge: unparsable temp names, unrelated files. + let other = dir.join("notes.txt"); + std::fs::write(&other, b"user file").unwrap(); + + // Fresh data is never swept. + sweep_orphans(&dir, std::time::Duration::from_secs(3600)); + assert!(orphan.is_dir() && foreign.iter().all(|p| p.is_file())); + + sweep_orphans(&dir, std::time::Duration::ZERO); + assert!(!orphan.exists(), "a bundle without a sidecar is swept"); + assert!(foreign.iter().all(|p| !p.exists()), "another process's temp files are swept"); + assert!(own.is_file(), "this process's temp files are kept"); + assert!(other.is_file()); + let entries = list_recovery(&dir); + assert_eq!(entries.len(), 1, "the complete entry survives"); + assert_eq!(recover(&entries[0]).unwrap(), *doc); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index 5c283560..f90e8ebe 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -680,10 +680,14 @@ impl PcraftWriter { } } write_atomic(&dir.join(MANIFEST), &p.manifest)?; + // The manifest is committed: the save has succeeded. Removing objects it no longer + // references is best-effort housekeeping (a vanished or locked file must not turn a + // complete save into a reported failure); a leftover is retried on the next save. for path in existing { if !p.objects.contains_key(&path) { - std::fs::remove_file(dir.join(&path))?; - stats.objects_removed += 1; + if std::fs::remove_file(dir.join(&path)).is_ok() { + stats.objects_removed += 1; + } if let Some(cache) = &mut self.verified_directory { cache.objects.remove(&path); } diff --git a/crates/format/tests/autosave.rs b/crates/format/tests/autosave.rs index 95e4e9da..29905d7a 100644 --- a/crates/format/tests/autosave.rs +++ b/crates/format/tests/autosave.rs @@ -236,3 +236,21 @@ fn sidecars_cannot_point_at_other_bundles() { assert_eq!(entries.iter().map(|e| e.info.key.as_str()).collect::>(), ["k"]); std::fs::remove_dir_all(dir).unwrap(); } + +/// Once the manifest is committed the save has succeeded: an unreferenced object that cannot be +/// removed (here a folder squatting on a tile name, like a locked or vanished file) must not turn +/// the directory save (autosave's path) into a reported failure. +#[test] +fn failed_cleanup_after_commit_does_not_fail_the_save() { + let dir = temp_dir("cleanup-best-effort").join("doc.pcraft"); + std::fs::create_dir_all(&dir).unwrap(); + let doc = rich_doc(ColorMode::Rgb, SampleType::U8); + let mut w = PcraftWriter::new(); + w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + let stuck = dir.join("tiles").join("0000000000000000000000000000000000000000000000000000000000000000.zst"); + std::fs::create_dir_all(stuck.join("inner")).unwrap(); + let stats = w.save_dir(&doc, &dir, &SaveOptions::default()).expect("the committed save reports success"); + assert_eq!(stats.objects_removed, 0, "nothing was actually removed"); + assert_eq!(load_path(&dir).unwrap(), doc); + std::fs::remove_dir_all(dir.parent().unwrap()).unwrap(); +}