From e81a21e64567bf5e5bb822fa3cc41696b19123da Mon Sep 17 00:00:00 2001 From: aiongg Date: Mon, 12 Oct 2026 23:54:46 +0000 Subject: [PATCH] refactor(save): delegate atomic writes to craft-atomic The engine's atomic module keeps its functions and signatures and delegates to craft-libs/craft-atomic (pinned to e9546e19), so every save and export caller is unchanged. write_atomic_with also turns a panicking writer into an error, so its temporary file is removed. Recovery checkpoints and package output keep their own writers, which replace links instead of following them. On Unix, new files follow the umask and replaced files keep their permissions. --- Cargo.lock | 32 +++--- Cargo.toml | 1 + ROADMAP.md | 1 + crates/engine/Cargo.toml | 1 + crates/engine/src/atomic.rs | 213 +++++++++--------------------------- docs/target-app-parity.md | 2 +- 6 files changed, 74 insertions(+), 176 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ef525c84..56e81b3d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -725,7 +725,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "117725a109d387c937a1533ce01b450cbde6b88abceea8473c4d7a85853cda3c" dependencies = [ "lazy_static", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -796,6 +796,14 @@ dependencies = [ "libm", ] +[[package]] +name = "craft-atomic" +version = "0.1.0" +source = "git+https://github.com/storytold/craft-libs?rev=e9546e191dd62f2bc1d61eca45dbdc2f1b3c1d8e#e9546e191dd62f2bc1d61eca45dbdc2f1b3c1d8e" +dependencies = [ + "rustix 1.1.5", +] + [[package]] name = "craft-print" version = "0.1.0" @@ -964,6 +972,7 @@ name = "designcraft-engine" version = "0.7.1" dependencies = [ "caseless", + "craft-atomic", "craft-print", "designcraft-color", "designcraft-compose", @@ -1483,7 +1492,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -2740,7 +2749,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -3836,7 +3845,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -3849,7 +3858,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -4264,7 +4273,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix 1.1.5", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5236,7 +5245,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5349,15 +5358,6 @@ dependencies = [ "windows-targets 0.52.6", ] -[[package]] -name = "windows-sys" -version = "0.59.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" -dependencies = [ - "windows-targets 0.52.6", -] - [[package]] name = "windows-sys" version = "0.60.2" diff --git a/Cargo.toml b/Cargo.toml index b08a82fc..752bedc4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,7 @@ rust-version = "1.95" repository = "https://github.com/storytold/designcraft" [workspace.dependencies] +craft-atomic = { git = "https://github.com/storytold/craft-libs", version = "0.1.0", rev = "e9546e191dd62f2bc1d61eca45dbdc2f1b3c1d8e" } craft-print = { git = "https://github.com/storytold/craft-libs", version = "0.1.0", rev = "d991bf6792f8ade99c51358bf3c1d902c8cd96b5" } # internal designcraft-geom = { path = "crates/geom" } diff --git a/ROADMAP.md b/ROADMAP.md index 91a95161..a8023948 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -129,6 +129,7 @@ list) · [roadmap.md](docs/roadmap.md) (milestones, plan) · [parity-checklist.m Newest first. +- **2026-10-12** · The engine's atomic writer, used by saves and exports (documents, snippets, books, libraries, swatches, XML, interchange, MCP, desktop and CLI writes), delegates to craft-libs/craft-atomic. Failed writes retain the original; new files follow the umask and replaced files keep their permissions. Recovery checkpoints and package output keep their own no-follow writers. No readiness percentage change. - **2026-10-12** · macOS Dock Quit uses the same unsaved-document prompts as window and menu Quit. Repeated requests keep the current answers; Cancel retains every tab and unsaved recovery checkpoint, and completed Save/Don't Save answers exit through the ordinary event loop. No readiness percentage change. - **2026-10-12** · Orderly native exit writes the latest recovery state after finalizing current quit approvals; incomplete or stale plans keep their documents, and Undo to the saved state clears stale recovery. Unavailable or failed recovery is reported. Cancelable macOS Dock Quit remains open (#544). No readiness percentage change. - **2026-10-12** · Recovered documents keep a checkpoint until saved, closed or reverted, including a second crash before autosave. Restarted document IDs and explicit recovery folders preserve other snapshots; failed republishing retains the original. No readiness percentage change. diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 62578ba2..166d0566 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -32,6 +32,7 @@ quick-xml = { workspace = true } caseless = { workspace = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] +craft-atomic = { workspace = true } tempfile = { workspace = true } [dev-dependencies] diff --git a/crates/engine/src/atomic.rs b/crates/engine/src/atomic.rs index 7eed2f02..ba7d091c 100644 --- a/crates/engine/src/atomic.rs +++ b/crates/engine/src/atomic.rs @@ -1,172 +1,47 @@ -//! Crash-safe file replacement: how documents, exports and other user files reach the disk. -//! -//! [`write_atomic`] never writes the destination in place. It writes a uniquely named temporary -//! file in the same folder, flushes it (`sync_all`), renames it over the destination and then -//! flushes the folder (Unix) so the rename survives a power loss. When any step before the rename -//! fails, the temporary file is removed and the destination keeps its old bytes. -//! -//! - A replaced file keeps its permissions (Unix mode). -//! - A symbolic link keeps pointing at its file: the file it names is replaced. -//! - Windows: antivirus scanners and the search indexer briefly hold files open, which makes the -//! replacing rename fail with `PermissionDenied`; the rename is retried with a short backoff. -//! - A failed rename (cross-device, unwritable folder) is returned as an error; there is no -//! fallback to writing in place. -//! - Flushing the parent folder after replacement is best effort. A successful save confirms -//! that the new file is in place; it cannot guarantee survival of a subsequent power loss if -//! the file system rejects that folder flush. +//! How documents, exports and other user files reach the disk: through `craft-atomic`, which +//! never writes the destination in place. It stages the bytes in a temporary file beside the +//! destination, flushes it, renames it over the destination and flushes the folder (Unix), so a +//! failed save leaves the old file whole. A new file gets the permissions ordinary file creation +//! gives it (the umask applies), a replaced file keeps its permissions, a symbolic link keeps +//! pointing at the file it names, and Windows renames are retried while a scanner holds the +//! file. //! //! The web has no file system, so this module is not built for `wasm32`. use std::fs::File; -#[cfg(unix)] -use std::fs::OpenOptions; -use std::io::{self, Write as _}; -use std::path::{Path, PathBuf}; - -/// How many times a replacing rename is tried before its error is returned. -const RENAME_TRIES: u32 = if cfg!(windows) { 5 } else { 1 }; -/// The pause before the first rename retry; it doubles up to [`RENAME_MAX_DELAY_MS`]. -const RENAME_FIRST_DELAY_MS: u64 = 10; -const RENAME_MAX_DELAY_MS: u64 = 200; -/// Symbolic links followed before giving up (a loop, or an absurd chain). -const MAX_LINKS: usize = 40; +use std::io; +use std::path::Path; /// Atomically replace (or create) `path` with `bytes`. See the module docs. pub fn write_atomic(path: &Path, bytes: &[u8]) -> io::Result<()> { - write_atomic_with(path, |f| f.write_all(bytes)) + craft_atomic::atomic_write(path, bytes) } -/// [`write_atomic`] for a command's `path` parameter: the error names the path. +/// [`write_atomic`] for a command's `path` parameter. craft-atomic's errors name the path. pub(crate) fn write_file(path: &str, bytes: &[u8]) -> crate::Result<()> { - write_atomic(Path::new(path), bytes).map_err(|e| crate::EngineError::Other(format!("{path}: {e}"))) + write_atomic(Path::new(path), bytes).map_err(|e| crate::EngineError::Other(e.to_string())) } /// Atomically replace (or create) `path` with what `fill` writes into the temporary file. When /// `fill`, the flush or the rename fails, the temporary file is removed, `path` is untouched and -/// the error is returned. +/// the error is returned. A panic in `fill` is returned as an error too, so the temporary file is +/// removed then as well. pub fn write_atomic_with(path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { - let target = resolve_links(path)?; - target.file_name().ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, format!("{}: not a file path", path.display())))?; - let dir = match target.parent() { - Some(d) if !d.as_os_str().is_empty() => d.to_path_buf(), - _ => PathBuf::from("."), - }; - let existing = std::fs::metadata(&target).ok(); - if existing.is_some() { - check_writable(&target)?; - } - let mut tmp = create_temp(&dir, existing.as_ref())?; - if let Some(existing) = existing { - // Restore the exact mode before any document bytes are written, and flush it along with - // the content. The caller's umask may have restricted the initial temporary-file mode. - keep_permissions(tmp.as_file(), &existing)?; - } - fill(tmp.as_file_mut())?; - tmp.as_file().sync_all()?; - // Closing the file before renaming also allows replacement on Windows. TempPath removes the - // staging file on every error; NamedTempFile does the same if the writer unwinds. - let tmp = tmp.into_temp_path(); - retry_rename(RENAME_TRIES, || std::fs::rename(&tmp, &target))?; - // The new bytes are in place. A failed folder flush can't be undone and must not report the - // save as failed. - let _ = sync_dir(&dir); - Ok(()) -} - -/// Create a fresh temporary file beside the target (the same folder, so the rename stays on one -/// volume). A short randomized name supports long destination names and skips existing entries. -fn create_temp(dir: &Path, existing: Option<&std::fs::Metadata>) -> io::Result { - let mut builder = tempfile::Builder::new(); - builder.prefix(".designcraft-save-").suffix(".tmp"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - - // Private replacements are private from creation. New files retain fs::write's normal - // 0666 mode restricted by the caller's umask. - let mode = existing.map_or(0o666, |metadata| metadata.permissions().mode()); - builder.permissions(std::fs::Permissions::from_mode(mode)); - } - #[cfg(not(unix))] - let _ = existing; - builder.tempfile_in(dir) -} - -/// The file a chain of symbolic links ends at (`path` itself when it is not a link). A dangling -/// link resolves to the missing file it names, which is then created. -fn resolve_links(path: &Path) -> io::Result { - let mut p = path.to_path_buf(); - for _ in 0..MAX_LINKS { - match std::fs::symlink_metadata(&p) { - Ok(m) if m.file_type().is_symlink() => { - let to = std::fs::read_link(&p)?; - p = match p.parent() { - Some(dir) if to.is_relative() => dir.join(to), - _ => to, - }; - } - _ => return Ok(p), - } - } - Err(io::Error::new(io::ErrorKind::InvalidInput, format!("{}: too many levels of symbolic links", path.display()))) -} - -/// A rename replaces a file the user can't write (Unix only needs the folder to be writable); -/// refuse, as writing it in place would. Opening for writing without truncating changes nothing. -#[cfg(unix)] -fn check_writable(target: &Path) -> io::Result<()> { - OpenOptions::new().write(true).open(target).map(drop) -} - -/// Windows refuses to rename over a read-only file by itself. -#[cfg(not(unix))] -fn check_writable(_: &Path) -> io::Result<()> { - Ok(()) -} - -/// The replacement keeps the replaced file's mode (Unix; Windows files keep only a read-only flag, -/// and a read-only file can't be replaced). -#[cfg(unix)] -fn keep_permissions(tmp: &File, replaced: &std::fs::Metadata) -> io::Result<()> { - tmp.set_permissions(replaced.permissions()) -} - -#[cfg(not(unix))] -fn keep_permissions(_: &File, _: &std::fs::Metadata) -> io::Result<()> { - Ok(()) -} - -/// Run `rename` up to `tries` times, pausing with a doubling delay while it fails with -/// `PermissionDenied` (a scanner holding the file on Windows). Other errors return at once. -fn retry_rename(tries: u32, mut rename: impl FnMut() -> io::Result<()>) -> io::Result<()> { - let mut delay = RENAME_FIRST_DELAY_MS; - let mut left = tries.max(1); - loop { - left = left.saturating_sub(1); - match rename() { - Err(e) if e.kind() == io::ErrorKind::PermissionDenied && left > 0 => { - std::thread::sleep(std::time::Duration::from_millis(delay)); - delay = delay.saturating_mul(2).min(RENAME_MAX_DELAY_MS); - } - r => return r, - } - } -} - -/// Flush a folder's entries (Unix). Rust's portable file API cannot open a directory on Windows; -/// this does not establish power-loss durability for its directory entries. -#[cfg(unix)] -fn sync_dir(dir: &Path) -> io::Result<()> { - File::open(dir)?.sync_all() -} - -#[cfg(not(unix))] -fn sync_dir(_: &Path) -> io::Result<()> { - Ok(()) + craft_atomic::atomic_write_with(path, |file| { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| fill(file))) + .unwrap_or_else(|_| Err(io::Error::other("the file writer stopped unexpectedly"))) + }) } #[cfg(test)] mod tests { + #[cfg(unix)] + use std::fs::OpenOptions; + use std::io::Write as _; + use std::path::PathBuf; + + use craft_atomic::{RenameRetry, retry_rename}; + use super::*; fn dir(name: &str) -> PathBuf { @@ -206,7 +81,8 @@ mod tests { f.write_all(b"half a docu")?; Err(io::Error::other("disk full")) }); - assert_eq!(r.unwrap_err().to_string(), "disk full"); + let e = r.unwrap_err().to_string(); + assert!(e.contains("disk full") && e.contains("not changed"), "{e}"); assert_eq!(std::fs::read(&path).unwrap(), b"original"); assert_eq!(names(&d), ["doc.designcraft"], "the temporary file is removed"); // A folder that doesn't exist: an error, nothing written. @@ -257,11 +133,9 @@ mod tests { let d = dir("unwinding-writer"); let path = d.join("private.designcraft"); std::fs::write(&path, b"old").unwrap(); - let result = std::panic::catch_unwind(|| { - write_atomic_with(&path, |file| { - file.write_all(b"partial private document")?; - panic!("synthetic writer panic"); - }) + let result = write_atomic_with(&path, |file| { + file.write_all(b"partial private document")?; + panic!("synthetic writer panic"); }); assert!(result.is_err()); assert_eq!(std::fs::read(&path).unwrap(), b"old"); @@ -272,26 +146,47 @@ mod tests { #[test] fn the_rename_is_retried_only_while_access_is_denied() { let mut calls = 0; - let r = retry_rename(3, || { + let r = retry_rename(RenameRetry { attempts: 2, first_delay_ms: 0 }, || { calls += 1; if calls < 3 { Err(io::Error::from(io::ErrorKind::PermissionDenied)) } else { Ok(()) } }); assert!(r.is_ok() && calls == 3); calls = 0; - let r = retry_rename(3, || { + let r = retry_rename(RenameRetry { attempts: 2, first_delay_ms: 0 }, || { calls += 1; Err(io::Error::from(io::ErrorKind::PermissionDenied)) }); assert_eq!(r.map_err(|e| e.kind()), Err(io::ErrorKind::PermissionDenied)); assert_eq!(calls, 3, "gives up after the last try"); calls = 0; - let r = retry_rename(3, || { + let r = retry_rename(RenameRetry { attempts: 2, first_delay_ms: 0 }, || { calls += 1; Err(io::Error::from(io::ErrorKind::CrossesDevices)) }); assert!(r.is_err() && calls == 1, "other errors are not retried"); } + #[cfg(unix)] + #[test] + fn new_files_follow_the_umask_and_replaced_files_keep_their_mode() { + use std::os::unix::fs::PermissionsExt as _; + let d = dir("new-mode"); + let mode = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + let plain = d.join("plain"); + File::create(&plain).unwrap(); + let path = d.join("new.designcraft"); + write_atomic(&path, b"new").unwrap(); + assert_eq!(mode(&path), mode(&plain), "a new file gets the mode ordinary creation gives"); + let streamed = d.join("streamed.designcraft"); + write_atomic_with(&streamed, |f| f.write_all(b"new")).unwrap(); + assert_eq!(mode(&streamed), mode(&plain)); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o640)).unwrap(); + write_atomic(&path, b"replaced").unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"replaced"); + assert_eq!(mode(&path), 0o640, "a replaced file keeps its mode"); + let _ = std::fs::remove_dir_all(&d); + } + #[cfg(unix)] #[test] fn permissions_are_kept_and_unwritable_files_are_not_replaced() { diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index 78e968dc..3be484c5 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -288,7 +288,7 @@ It is evidence of breadth: each item exists. It is not a claim of InDesign-level - Missing fonts: highlighted pink on screen (never in output), listed by Preflight; Type › Find/Replace Font (fonts in the document, missing first; replace in text, cells, footnotes and styles); `font.list` / `font.replace`. - Missing glyphs, as in InDesign: a character the applied font lacks is drawn as that font's missing-glyph box (its .notdef; a drawn box when the font's is empty), on screen and in PDF (as a path, so PDF/A and PDF/UA exports take it), and Preflight lists the characters by font. Preferences › Composition › Draw Missing Glyphs from Fallback Fonts (`document.preferences {glyphFallback}`, stored with the document) draws them from other fonts instead; it is off in new documents and IDML imports, on in documents saved before it existed. A missing font's substitute always uses fallback fonts. - Damage-region repaint: after an edit the canvas re-renders only the frames whose composed text (or the items that) changed and patches its texture — pixel-identical to a full render; typing in a frame repaints in under a millisecond instead of a full-viewport render (22 ms measured on the sample). -- Native engine saves, Save a Copy, snippets, books, libraries, swatches and engine/MCP exports use same-folder atomic replacement, as do desktop Services writes and CLI raster exports. Failed writes before replacement retain the original; existing Unix permissions apply before staging document bytes, and already-open readers retain the old bytes. Parent-folder flushing is best effort, so this is not a power-loss durability guarantee. +- Native engine saves, Save a Copy, snippets, books, libraries, swatches and engine/MCP exports use same-folder atomic replacement through craft-libs/craft-atomic, as do desktop Services writes and CLI raster exports. Failed writes before replacement retain the original, and already-open readers retain the old bytes. On Unix a new file gets the permissions ordinary file creation gives it (the umask applies) and a replaced file keeps its permissions. Parent-folder flushing is best effort, so this is not a power-loss durability guarantee. - Save As prepares destination-relative data-source paths before writing and installs them only after a successful save. Failed Save As retains the document, saved state and history; Save a Copy rebases only the saved copy. Library and book collection edits likewise update live values only after persistence succeeds. - Crash recovery: unsaved documents are written every 30 s into a locked folder owned by that running app and reopened (unsaved, remembering their files) after a crash. Startup takes entries from ended runs and leaves live runs alone; writes and cleanup validate current folder and lock ownership. Failed isolation disables recovery with a warning. Recovered checkpoints remain available through another crash before autosave; restarting IDs and explicit recovery from another folder preserve unopened snapshots. Successful Save, Close or Revert clears the document's checkpoints, including manual recovery while autosave is disabled; failed republishing keeps the original. Concurrent older versions still use unlocked root entries; mixed-version isolation remains open. File › Revert and Save a Copy…; `file.recovery.*`, `file.revert`, `file.saveACopy`. - Window close, application/menu Quit and macOS Dock Quit ask about every unsaved document. Cancel or a cancelled/failed save keeps all tabs open; Don't Save approvals discard nothing until the complete plan is current. Later edits and new documents are checked before closing. Repeated native requests preserve an active dialog and partial approvals. Control-channel quit remains immediate.