Merge PR #8034: fix(format): make Windows native-save publication durable

crates/format/src/store.rs: the Windows tiles/blobs directory flush goes before main's manifest write (main publishes the manifest before removing stale previews), and that write takes the PR's fs argument; the PR's second manifest write is dropped.
This commit is contained in:
aiongg
2026-10-22 02:28:02 +00:00
6 changed files with 302 additions and 14 deletions
+3 -2
View File
@@ -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"
+35 -8
View File
@@ -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 => {
+45
View File
@@ -41,6 +41,9 @@ struct Faulty {
/// Fail the first N renames with PermissionDenied (a transient lock), then succeed.
locked_renames: Cell<u32>,
renames: Cell<u32>,
/// Failure of the durability barrier after publication (#7858).
fail_sync_dir: bool,
synced: Cell<u32>,
}
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);
}
+30 -4
View File
@@ -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<SaveStats> {
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<SaveStats> {
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;
+113
View File
@@ -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<u8>,
stage: FailAt,
armed: Cell<bool>,
synced: RefCell<Vec<PathBuf>>,
}
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();
}
}
+76
View File
@@ -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.