From 851ca348a71ff7063e4b7ad7609d290500845c2c Mon Sep 17 00:00:00 2001 From: denstu473 <63150090+denstu473@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:24:23 +0200 Subject: [PATCH 1/2] fix(format): persist Windows save publication before acknowledgement Flush NTFS directory publication after replacing a saved file. Report a failed barrier as uncertain durability instead of successful Save. Flush directory-bundle objects before the manifest, including on same-writer retries, and stop before garbage collection when publication fails. Add regression tests for failed barriers, retained old objects, cached object retries, and directory handles. Keep existing Windows metadata preservation and the separate Unix flush-error policy. Validation: 130 format tests, 657 I/O corpus tests, clippy, layers, wasm, 12 acknowledged-save power cuts, six publication/cleanup cuts, and 1,368 native fault cases with exact archive comparisons. Existing preview-only failure ordering remains tracked by #7548/#7549. Fixes #7858. --- crates/format/src/atomic.rs | 41 ++++++-- crates/format/src/atomic/tests.rs | 41 ++++++++ crates/format/src/store.rs | 32 +++++- crates/format/src/store/durability_tests.rs | 104 ++++++++++++++++++++ docs/windows-save-durability.md | 76 ++++++++++++++ 5 files changed, 282 insertions(+), 12 deletions(-) create mode 100644 crates/format/src/store/durability_tests.rs create mode 100644 docs/windows-save-durability.md diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index 3a4aeca6c0..095ae30266 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`]. @@ -105,7 +106,20 @@ impl AtomicFs for RealFs { { 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 +186,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 +252,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 +332,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..c2912803dd 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 { @@ -67,6 +70,10 @@ impl AtomicFs for Faulty { RealFs.remove(path) } 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 +83,38 @@ 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); +} + +#[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"); @@ -124,6 +163,7 @@ 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); } @@ -134,6 +174,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 7da5dd6b53..564ab5156e 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -525,7 +525,9 @@ 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<()> { +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))?; @@ -562,6 +564,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()); @@ -807,6 +813,11 @@ impl PcraftWriter { /// rewrites missing or damaged objects, then writes the manifest /// atomically and removes unreferenced objects. 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(); let existing = list_objects(dir)?; @@ -862,7 +873,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 { @@ -876,7 +887,7 @@ 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. @@ -891,7 +902,17 @@ impl PcraftWriter { } } } - 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)?; // 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. @@ -1011,3 +1032,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..963524d3ca --- /dev/null +++ b/crates/format/src/store/durability_tests.rs @@ -0,0 +1,104 @@ +//! 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; + +#[derive(Clone, Copy, Debug)] +enum FailAt { + Tiles, + Blobs, + Manifest, +} + +struct PublicationFault { + root: PathBuf, + old_manifest: Vec, + stage: FailAt, + armed: Cell, + synced: RefCell>, +} + +impl AtomicFs for PublicationFault { + fn write_new(&self, path: &Path, bytes: &[u8]) -> io::Result<()> { + RealFs.write_new(path, bytes) + } + fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.rename(from, to) + } + fn remove(&self, path: &Path) -> io::Result<()> { + RealFs.remove(path) + } + fn copy_metadata(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.copy_metadata(from, to) + } + 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) + } +} + +#[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. From c82674439f756d28c6748c0eb3ad425881e9726d Mon Sep 17 00:00:00 2001 From: denstu473 <63150090+denstu473@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:15:43 +0200 Subject: [PATCH 2/2] fix(ci): run native Windows tests for format changes Include the native format crate in Windows platform selection so publication barriers and retry regressions execute in the unit and integration CI legs. Document the durability helpers and regression tests flagged by review. Validated 20 platform-selection scenarios and all 130 native Windows format tests, plus formatting, Clippy, layering and the format WebAssembly check. --- .github/workflows/ci.yml | 5 +++-- crates/format/src/atomic.rs | 2 ++ crates/format/src/atomic/tests.rs | 4 ++++ crates/format/src/store.rs | 2 ++ crates/format/src/store/durability_tests.rs | 9 +++++++++ 5 files changed, 20 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba8243353d..b933deb816 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 095ae30266..635bf924a2 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -101,6 +101,8 @@ 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)] { diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index c2912803dd..853a11293d 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -69,6 +69,7 @@ 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 { @@ -107,6 +108,7 @@ fn a_failed_publication_flush_is_reported_and_can_be_retried() { 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() { @@ -152,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"); @@ -167,6 +170,7 @@ fn write_failure_halfway_keeps_original() { 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"); diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index 564ab5156e..40b7c45e7e 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -525,6 +525,8 @@ fn validate_bundle_file(root: &Path, path: &Path) -> Result<()> { plain_file_or_missing(path) } +/// 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; diff --git a/crates/format/src/store/durability_tests.rs b/crates/format/src/store/durability_tests.rs index 963524d3ca..5f60db2948 100644 --- a/crates/format/src/store/durability_tests.rs +++ b/crates/format/src/store/durability_tests.rs @@ -6,6 +6,7 @@ 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, @@ -13,6 +14,7 @@ enum FailAt { Manifest, } +/// Real filesystem operations with recorded barriers and a single failure at the chosen stage. struct PublicationFault { root: PathBuf, old_manifest: Vec, @@ -22,18 +24,23 @@ struct PublicationFault { } 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 { @@ -48,6 +55,8 @@ impl AtomicFs for PublicationFault { } } +/// 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);