diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 735986c355..b06e15449d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,8 @@ concurrency: jobs: # Which OSes the test job runs on. main, manual runs and PRs labelled `full-ci` get Linux and # Windows. Other PRs get Linux, plus Windows when they touch code whose behaviour differs by - # platform: the app shell, UI, text and fonts, GPU, pen input, build scripts or the lockfile. + # platform: the app shell, UI, text and fonts, GPU, pen input, native saves, build scripts or + # the lockfile. Format changes must exercise the Windows-only publication barriers (#8034). # macOS is tested only by release.yml. platforms: timeout-minutes: 10 @@ -78,7 +79,7 @@ jobs: else echo "web=false" >> "$GITHUB_OUTPUT" fi - if git diff --name-only "$BASE" HEAD | grep -Eq '^(apps/|crates/(ui-egui|text|gpu|tablet|heif)/|Cargo\.lock$|(.*/)?build\.rs$|\.github/workflows/ci\.yml$)'; then + if git diff --name-only "$BASE" HEAD | grep -Eq '^(apps/|crates/(ui-egui|text|gpu|tablet|heif|format)/|Cargo\.lock$|(.*/)?build\.rs$|\.github/workflows/ci\.yml$)'; then echo "legs=$all" >> "$GITHUB_OUTPUT" else echo "legs=[$linux]" >> "$GITHUB_OUTPUT" diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index 3a4aeca6c0..635bf924a2 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -2,8 +2,9 @@ //! //! [`atomic_write`] never truncates the destination. It writes a temporary file in the **same //! directory**, flushes it to stable storage (`sync_all`), renames it over the destination and, -//! on Unix, fsyncs the directory so the rename itself survives a power loss. If anything fails, -//! the destination keeps its previous bytes and the temporary file is removed. +//! flushes the directory so the rename itself survives a power loss. Failures before replacement +//! keep the destination's previous bytes. On Windows a failed publication flush reports that the +//! new file is in place but its durability could not be confirmed (#7858). //! //! - **Windows:** antivirus scanners and the search indexer briefly hold files open, which makes //! the replacing rename fail with `PermissionDenied`. The rename is retried with exponential @@ -59,7 +60,7 @@ pub trait AtomicFs { fn rename(&self, from: &Path, to: &Path) -> io::Result<()>; /// Remove a (temporary) file. fn remove(&self, path: &Path) -> io::Result<()>; - /// Flush a directory entry change (Unix); a no-op elsewhere. + /// Flush a directory entry change on Unix and Windows; a no-op elsewhere. fn sync_dir(&self, dir: &Path) -> io::Result<()>; /// Copy the metadata a replacement should keep — mode, extended attributes, ACL — from the /// file about to be replaced onto the temporary file. Best effort; see [`meta`]. @@ -100,12 +101,27 @@ impl AtomicFs for RealFs { fn remove(&self, path: &Path) -> io::Result<()> { std::fs::remove_file(path) } + /// Flush directory publication using a directory handle on Unix and Windows. + /// Windows requests only `FILE_ADD_FILE`; opening or flushing the handle can fail the save. fn sync_dir(&self, dir: &Path) -> io::Result<()> { #[cfg(unix)] { sync_file(&std::fs::File::open(dir)?) } - #[cfg(not(unix))] + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt; + + // FILE_ADD_FILE is the directory equivalent of FILE_WRITE_DATA. Requesting only + // this right permits FlushFileBuffers without requiring GENERIC_WRITE's additional + // directory permissions. BACKUP_SEMANTICS opens a directory, not a volume: no + // administrative privilege or volume-wide flush is needed. + const FILE_ADD_FILE: u32 = 0x0002; + const FILE_FLAG_BACKUP_SEMANTICS: u32 = 0x0200_0000; + let directory = std::fs::OpenOptions::new().access_mode(FILE_ADD_FILE).custom_flags(FILE_FLAG_BACKUP_SEMANTICS).open(dir)?; + directory.sync_all() + } + #[cfg(not(any(unix, windows)))] { let _ = dir; Ok(()) @@ -172,8 +188,8 @@ impl RenameRetry { /// Atomically replace (or create) `path` with `bytes`. See the module docs. /// -/// Errors keep their [`io::ErrorKind`] and say what happened and that the original file is -/// unchanged. +/// Errors keep their [`io::ErrorKind`]. Before replacement they say the original is unchanged; +/// a Windows publication-flush error says the new file may not survive a power loss. pub fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { atomic_write_with(&RealFs, RenameRetry::platform(), path, bytes) } @@ -238,8 +254,10 @@ pub fn atomic_write_with_progress(fs: &dyn AtomicFs, retry: RenameRetry, path: & let _ = fs.remove(&tmp); return Err(rename_error(e, path)); } - // The new bytes are in place; a failed directory fsync can't be undone and must not report - // the save as failed, so it is best effort. + #[cfg(windows)] + fs.sync_dir(&dir).map_err(|e| publication_error(e, path))?; + // Keep the existing Unix policy here; #5420 separately changes its error handling. + #[cfg(not(windows))] let _ = fs.sync_dir(&dir); Ok(()) } @@ -316,6 +334,15 @@ fn context(e: io::Error, msg: String) -> io::Error { const UNCHANGED: &str = "the original file was not changed"; +/// Replacement already happened: never claim that the original file is unchanged or undo it. +#[cfg(windows)] +pub(crate) fn publication_error(error: io::Error, path: &Path) -> io::Error { + io::Error::new( + error.kind(), + format!("wrote {}, but could not confirm the save is durable; it may not survive a power loss or crash: {error}", path.display()), + ) +} + fn write_error(e: io::Error, path: &Path, dir: &Path) -> io::Error { let msg = match e.kind() { io::ErrorKind::PermissionDenied | io::ErrorKind::ReadOnlyFilesystem => { diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index d4381705a0..853a11293d 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -41,6 +41,9 @@ struct Faulty { /// Fail the first N renames with PermissionDenied (a transient lock), then succeed. locked_renames: Cell, renames: Cell, + /// Failure of the durability barrier after publication (#7858). + fail_sync_dir: bool, + synced: Cell, } impl AtomicFs for Faulty { @@ -66,7 +69,12 @@ impl AtomicFs for Faulty { fn remove(&self, path: &Path) -> io::Result<()> { RealFs.remove(path) } + /// Count publication barriers and optionally fail before the real directory flush. fn sync_dir(&self, dir: &Path) -> io::Result<()> { + self.synced.set(self.synced.get() + 1); + if self.fail_sync_dir { + return Err(io::Error::other("injected publication flush failure")); + } RealFs.sync_dir(dir) } fn copy_metadata(&self, from: &Path, to: &Path) -> io::Result<()> { @@ -76,6 +84,39 @@ impl AtomicFs for Faulty { const NO_RETRY: RenameRetry = RenameRetry { attempts: 0, first_delay_ms: 0 }; +/// A successful rename does not establish power-loss durability. A failed Windows publication +/// barrier must leave the new bytes in place, report uncertainty, and allow a subsequent retry. +#[cfg(windows)] +#[test] +fn a_failed_publication_flush_is_reported_and_can_be_retried() { + let d = TempDir::new("publication-flush"); + let p = d.0.join("document.pcraft"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_sync_dir: true, ..Default::default() }; + + let error = atomic_write_with(&fs, NO_RETRY, &p, b"new document").unwrap_err(); + assert_eq!(fs.synced.get(), 1, "{error}"); + assert_eq!(error.kind(), io::ErrorKind::Other); + assert!(error.to_string().contains("document.pcraft"), "{error}"); + assert!(error.to_string().contains("may not survive"), "{error}"); + assert!(!error.to_string().contains(UNCHANGED), "the replacement already happened: {error}"); + assert_eq!(std::fs::read(&p).unwrap(), b"new document"); + assert_no_temp(&d.0); + + atomic_write(&p, b"retried document").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"retried document"); + assert_no_temp(&d.0); +} + +/// A real Windows directory handle must flush successfully and preserve directory-open errors. +#[cfg(windows)] +#[test] +fn windows_directory_flush_opens_a_directory_and_reports_open_errors() { + let d = TempDir::new("sync-directory"); + RealFs.sync_dir(&d.0).unwrap(); + assert_eq!(RealFs.sync_dir(&d.0.join("missing")).unwrap_err().kind(), io::ErrorKind::NotFound); +} + #[test] fn replaces_and_creates() { let d = TempDir::new("ok"); @@ -113,6 +154,7 @@ fn creates_missing_parent_folders() { assert_eq!(std::fs::read(&p).unwrap(), b"x"); } +/// A partial temporary write must preserve the original and never reach publication or its flush. #[test] fn write_failure_halfway_keeps_original() { let d = TempDir::new("write"); @@ -124,9 +166,11 @@ fn write_failure_halfway_keeps_original() { assert!(e.to_string().contains("not changed"), "{e}"); assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); assert_eq!(fs.renames.get(), 0, "never renames after a failed write"); + assert_eq!(fs.synced.get(), 0, "nothing was published"); assert_no_temp(&d.0); } +/// A failed replacement must preserve the original and skip the publication barrier. #[test] fn rename_failure_keeps_original() { let d = TempDir::new("rename"); @@ -134,6 +178,7 @@ fn rename_failure_keeps_original() { std::fs::write(&p, ORIGINAL).unwrap(); let fs = Faulty { fail_rename: Some(io::ErrorKind::Other), ..Default::default() }; assert!(atomic_write_with(&fs, NO_RETRY, &p, b"new").is_err()); + assert_eq!(fs.synced.get(), 0, "a failed replacement never reaches the publication barrier"); assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); assert_no_temp(&d.0); } diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index b8d08e6429..03a85cbc83 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -587,7 +587,11 @@ fn validate_bundle_file(root: &Path, path: &Path) -> Result<()> { plain_file_or_missing(path) } -fn write_bundle_file(root: &Path, path: &Path, data: &[u8]) -> Result<()> { +/// Publish a validated bundle entry through a flushed temporary file and replacing rename. +/// On Windows, propagate a failed directory barrier even though the new entry is already visible. +fn write_bundle_file(root: &Path, path: &Path, data: &[u8], fs: &dyn crate::atomic::AtomicFs) -> Result<()> { + #[cfg(not(windows))] + let _ = fs; validate_bundle_file(root, path)?; let parent = path.parent().ok_or_else(|| unsafe_bundle_entry(path))?; let name = path.file_name().ok_or_else(|| unsafe_bundle_entry(path))?; @@ -624,6 +628,10 @@ fn write_bundle_file(root: &Path, path: &Path, data: &[u8]) -> Result<()> { // itself, never its referent, even if one appears after this check. validate_bundle_file(root, path)?; crate::atomic::retry_rename(crate::atomic::RenameRetry::platform(), || std::fs::rename(&tmp, path))?; + #[cfg(windows)] + { + fs.sync_dir(parent).map_err(|e| crate::atomic::publication_error(e, path))?; + } #[cfg(unix)] { let _ = std::fs::File::open(parent).and_then(|d| d.sync_all()); @@ -888,6 +896,11 @@ impl PcraftWriter { /// 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 { + self.save_dir_with(doc, dir, opts, &crate::atomic::RealFs) + } + + /// The same save path with an injectable publication barrier, for failure/GC ordering tests. + fn save_dir_with(&mut self, doc: &Document, dir: &Path, opts: &SaveOptions, fs: &dyn crate::atomic::AtomicFs) -> 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). @@ -945,7 +958,7 @@ impl PcraftWriter { Object::Tile(..) => stats.tiles_written += 1, Object::Blob(_) => stats.blobs_written += 1, } - write_bundle_file(dir, &object_path, &obj.compressed())?; + write_bundle_file(dir, &object_path, &obj.compressed(), fs)?; let written = path_signature(&object_path)?; if let Some(cache) = &mut self.verified_directory { match written { @@ -959,12 +972,22 @@ impl PcraftWriter { } } for (name, data) in &p.previews { - write_bundle_file(dir, &dir.join(name), data)?; + write_bundle_file(dir, &dir.join(name), data, fs)?; } // Pin all GC directories before removing stale previews or objects. // Unix cleanup then stays within these directories even if a pathname is swapped. let gc = BundleGc::new(dir)?; - write_bundle_file(dir, &dir.join(MANIFEST), &p.manifest)?; + #[cfg(windows)] + { + // A previous attempt may have published an object and then failed its directory + // flush. The retry can reuse those valid bytes (including cached ones), so flush + // both object directories again before publishing a manifest that references them. + // This also persists the fixed subdirectories of a newly created bundle. + for sub in ["tiles", "blobs"] { + fs.sync_dir(&dir.join(sub)).map_err(|e| crate::atomic::publication_error(e, &dir.join(sub)))?; + } + } + write_bundle_file(dir, &dir.join(MANIFEST), &p.manifest, fs)?; // Omitted previews belong to the previous manifest until the new one commits. // After commit, a locked leftover is only housekeeping, just like unused objects. for stale in [THUMB, COMPOSITE] { @@ -1162,3 +1185,6 @@ pub(crate) fn write_atomic(path: &Path, data: &[u8]) -> Result<()> { crate::atomic::atomic_write(path, data)?; Ok(()) } + +#[cfg(all(test, windows))] +mod durability_tests; diff --git a/crates/format/src/store/durability_tests.rs b/crates/format/src/store/durability_tests.rs new file mode 100644 index 0000000000..5f60db2948 --- /dev/null +++ b/crates/format/src/store/durability_tests.rs @@ -0,0 +1,113 @@ +//! Exercise the real directory save, including cached-object reuse and garbage collection. +use super::*; +use crate::atomic::{AtomicFs, RealFs}; +use photocraft_color::{ColorMode, SampleType}; +use photocraft_doc::{Layer, Size}; +use std::cell::{Cell, RefCell}; +use std::io; + +/// The object or manifest directory barrier at which to inject one publication failure. +#[derive(Clone, Copy, Debug)] +enum FailAt { + Tiles, + Blobs, + Manifest, +} + +/// Real filesystem operations with recorded barriers and a single failure at the chosen stage. +struct PublicationFault { + root: PathBuf, + old_manifest: Vec, + stage: FailAt, + armed: Cell, + synced: RefCell>, +} + +impl AtomicFs for PublicationFault { + /// Keep temporary-file writes and file flushes real; only directory barriers are injected. + fn write_new(&self, path: &Path, bytes: &[u8]) -> io::Result<()> { + RealFs.write_new(path, bytes) + } + /// Publish through the real rename so a barrier error occurs after replacement. + fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.rename(from, to) + } + /// Use real cleanup so the test can detect premature removal of old objects. + fn remove(&self, path: &Path) -> io::Result<()> { + RealFs.remove(path) + } + /// Preserve normal replacement metadata behavior during failure injection. + fn copy_metadata(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.copy_metadata(from, to) + } + /// Record every barrier and fail the selected stage once, allowing a same-writer retry. + fn sync_dir(&self, dir: &Path) -> io::Result<()> { + self.synced.borrow_mut().push(dir.to_path_buf()); + let fail = match self.stage { + FailAt::Tiles => dir == self.root.join("tiles"), + FailAt::Blobs => dir == self.root.join("blobs"), + FailAt::Manifest => dir == self.root && std::fs::read(dir.join(MANIFEST))? != self.old_manifest, + }; + if fail && self.armed.replace(false) { + return Err(io::Error::other("injected publication flush failure")); + } + RealFs.sync_dir(dir) + } +} + +/// Failed object or manifest publication retains old objects; a retry flushes reused objects +/// before the manifest and resumes garbage collection only after durable publication. +#[test] +fn failed_bundle_publication_retains_old_objects_and_same_writer_retry_is_durable() { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + for stage in [FailAt::Tiles, FailAt::Blobs, FailAt::Manifest] { + let id = NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("pcraft-durability-{}-{id}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + let dir = std::fs::canonicalize(dir).unwrap(); + let mut old = Document::new("old", Size::new(1, 1), ColorMode::Rgb, SampleType::U8); + let mut layer = Layer::raster("pixels", old.pixel_format()); + layer.surface_mut().unwrap().write_pixel(0, 0, &[1.0, 0.0, 0.0, 1.0]); + old.layers.push(layer); + old.metadata.exif = Some(Arc::new(vec![1; 100])); + let mut new = old.clone(); + new.name = "new".into(); + new.layers[0].surface_mut().unwrap().write_pixel(0, 0, &[0.0, 0.0, 1.0, 1.0]); + new.metadata.exif = Some(Arc::new(vec![2; 100])); + + let mut writer = PcraftWriter::new(); + writer.save_dir(&old, &dir, &SaveOptions::default()).unwrap(); + let old_objects: Vec<_> = list_objects(&dir).unwrap().into_iter().map(|p| (p.clone(), std::fs::read(dir.join(p)).unwrap())).collect(); + assert!(old_objects.len() >= 2); + let fs = PublicationFault { + old_manifest: std::fs::read(dir.join(MANIFEST)).unwrap(), + root: dir.clone(), + stage, + armed: Cell::new(true), + synced: RefCell::new(Vec::new()), + }; + + let error = writer.save_dir_with(&new, &dir, &SaveOptions::default(), &fs).unwrap_err(); + assert!(!fs.armed.get(), "the {stage:?} failure fired"); + assert!(error.to_string().contains("may not survive"), "{error}"); + let expected = if matches!(stage, FailAt::Manifest) { &new } else { &old }; + assert_eq!(&crate::load_path(&dir).unwrap(), expected); + for (path, bytes) in &old_objects { + assert_eq!(&std::fs::read(dir.join(path)).unwrap(), bytes, "old object removed before durable publication: {path}"); + } + + fs.synced.borrow_mut().clear(); + writer.save_dir_with(&new, &dir, &SaveOptions::default(), &fs).unwrap(); + assert_eq!(crate::load_path(&dir).unwrap(), new); + let synced = fs.synced.borrow(); + let publication = synced.iter().rposition(|p| p == &dir).unwrap(); + for sub in ["tiles", "blobs"] { + assert!( + synced.iter().position(|p| p == &dir.join(sub)).is_some_and(|position| position < publication), + "retry must flush reused {sub} before its manifest" + ); + } + assert!(old_objects.iter().all(|(p, _)| !dir.join(p).exists()), "cleanup resumes after the durable retry"); + std::fs::remove_dir_all(dir).unwrap(); + } +} diff --git a/docs/windows-save-durability.md b/docs/windows-save-durability.md new file mode 100644 index 0000000000..a4dc5c70de --- /dev/null +++ b/docs/windows-save-durability.md @@ -0,0 +1,76 @@ +# Windows native-save durability + +An acknowledged overwrite must reopen as the complete new document after a Windows/NTFS +power loss. [Issue #7858](https://github.com/storytold/photocraft/issues/7858) demonstrated +that flushing a temporary file before replacement did not establish this guarantee: a fresh +Windows boot recovered the previous ZIP or directory bundle after Save returned success. + +## Publication barrier + +The shared atomic writer still writes and flushes a sibling temporary file, preserves the +destination's metadata, and replaces the destination. It now flushes the parent directory +on Windows before acknowledging success. `RealFs::sync_dir` opens a directory using +`FILE_FLAG_BACKUP_SEMANTICS` and `FILE_ADD_FILE`, then calls `File::sync_all`, which reaches +`FlushFileBuffers`. This uses ordinary directory permissions and does not open or flush a +volume or require administrative privileges. No unsupported `REPLACEFILE_WRITE_THROUGH` +flag is used. + +Windows references: + +- [Directory handles](https://learn.microsoft.com/en-us/windows/win32/fileio/obtaining-a-handle-to-a-directory) +- [File access rights](https://learn.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants) +- [Flushing cached data and directory structure](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/0de7dc40-9627-437e-a4df-c4696cdc3d02) +- [Filesystem-specific behavior](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/4e3695bd-7574-4f24-a223-b4679c065b63): the directory-structure guarantee applies to NTFS. +- [ReplaceFileW flags](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-replacefilew) + +A publication-flush error occurs after replacement. It preserves the underlying error kind +and says that the new data was written but may not survive a crash. It does not claim the +old file is unchanged, attempt an unsafe rollback, or return success. Existing callers then +retain the document's dirty state and recovery data. + +## Directory bundles and retries + +Every published bundle file receives a Windows directory flush. Before replacing the +manifest, the writer also flushes `tiles/` and `blobs/`, including when it reuses cached +objects. A previous attempt can leave valid objects in place after a failed publication +flush; validating their bytes alone does not establish their durability. These barriers +make a retry using the same writer safe without recompressing every unchanged object. + +The manifest's publication barrier completes before obsolete objects are collected. An +error stops the save before that cleanup, preserving the objects referenced by the old +manifest. Garbage collection remains best effort after a successful commit. + +The private `save_dir_with` seam exercises this complete ordering with injected tile, +blob and manifest publication failures. The tests check the reported error, loaded +document, preservation of old objects, barriers on reused objects, and successful retry. + +## Verification + +Run the format tests and repository checks: + +```sh +cargo test -p photocraft-format +cargo clippy -p photocraft-format --all-targets -- -D warnings +cargo xtask layers +cargo xtask wasm +cargo xtask test-corpus -p io +``` + +Unit tests cannot prove power-loss durability. The VM test uses a disposable NTFS disk and +synthetic old/new documents at 8-, 16- and 32-bit depth, in ZIP and directory form. Flush the +old baseline during setup, call the production save API, wait for its successful response, +then abruptly power off the guest. Preserve and hash the crashed disk before recovery. +Boot Windows fresh against those exact bytes and compare the complete loaded document +and every expected bundle entry with the new revision. Do not restore a RAM snapshot for +recovery. Repeat each combination and retain failing unchanged-build controls. + +Keep volume flushes and clean unmounts in setup or diagnostic controls only; they are not +part of the production save implementation. Archive comparisons must account for the +build's generator-version stamp while still comparing all document payloads exactly. + +This change addresses Windows/NTFS publication. The existing Unix flush-error policy is +handled separately by [#5420](https://github.com/storytold/photocraft/pull/5420). Preview +replacement/deletion ordering on failed directory saves remains tracked by +[#7548](https://github.com/storytold/photocraft/issues/7548) and +[#7549](https://github.com/storytold/photocraft/issues/7549). VM tests do not establish +physical-controller or firmware-cache behavior, or guarantees on other filesystems.