From 6bd1e3fca38df00d7933d0a10821f8b0c4acb031 Mon Sep 17 00:00:00 2001 From: Codex Date: Wed, 9 Sep 2026 20:06:24 +0000 Subject: [PATCH] fix(storage): preserve concurrent Windows portable destinations (#1190) --- .github/workflows/test.yml | 6 + crates/graphforge-filesystem/src/lib.rs | 133 ++++++++++++++++++ .../src/project_portable_v2_export.rs | 28 +--- docs/book/architecture/portable-project-v2.md | 4 +- 4 files changed, 143 insertions(+), 28 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9661a7774..133ca4936 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -971,6 +971,12 @@ jobs: run: >- cargo test -p graphforge-filesystem --lib --no-fail-fast + - name: Verify portable publication refuses a concurrent destination + run: >- + cargo test -p graphforge-storage --lib + project_portable_v2_export::tests::destination_replacement_and_source_mutation_fail_closed + -- --exact --nocapture + - name: Run Windows native filesystem admission tests shell: bash run: >- diff --git a/crates/graphforge-filesystem/src/lib.rs b/crates/graphforge-filesystem/src/lib.rs index 30f201e85..6b986fed0 100644 --- a/crates/graphforge-filesystem/src/lib.rs +++ b/crates/graphforge-filesystem/src/lib.rs @@ -2282,6 +2282,44 @@ pub fn install_new_file( install_new_file_platform(directory, source_name, target_name, None) } +/// Atomically move a file or directory without replacing any existing destination. +/// +/// This never copies across volumes. The caller owns source admission and the +/// containing-directory durability barrier after a successful namespace change. +pub fn rename_no_replace(source: &Path, destination: &Path) -> io::Result<()> { + rename_no_replace_platform(source, destination) +} + +#[cfg(any(target_vendor = "apple", target_os = "linux", target_os = "redox"))] +fn rename_no_replace_platform(source: &Path, destination: &Path) -> io::Result<()> { + rustix::fs::renameat_with( + rustix::fs::CWD, + source, + rustix::fs::CWD, + destination, + rustix::fs::RenameFlags::NOREPLACE, + )?; + Ok(()) +} + +#[cfg(windows)] +fn rename_no_replace_platform(source: &Path, destination: &Path) -> io::Result<()> { + windows::rename_no_replace(source, destination) +} + +#[cfg(not(any( + target_vendor = "apple", + target_os = "linux", + target_os = "redox", + windows +)))] +fn rename_no_replace_platform(_source: &Path, _destination: &Path) -> io::Result<()> { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "atomic no-replace rename is unsupported", + )) +} + fn verify_single_component(name: &OsStr) -> io::Result<()> { let mut components = Path::new(name).components(); if !matches!(components.next(), Some(std::path::Component::Normal(_))) @@ -3442,6 +3480,26 @@ mod windows { Ok(()) } + pub(super) fn rename_no_replace(source: &Path, destination: &Path) -> io::Result<()> { + let source = wide(source.as_os_str())?; + let destination = wide(destination.as_os_str())?; + // SAFETY: both UTF-16 buffers are NUL-terminated and live for the call. + // Zero flags deliberately omit REPLACE_EXISTING and COPY_ALLOWED: + // an existing destination or cross-volume move must fail unchanged. + let succeeded = unsafe { + windows_sys::Win32::Storage::FileSystem::MoveFileExW( + source.as_ptr(), + destination.as_ptr(), + 0, + ) + }; + if succeeded == 0 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } + } + pub(super) fn identity(path: &Path) -> io::Result { file_identity(&open_identity_handle(path)?) } @@ -4242,6 +4300,81 @@ mod tests { assert!(!source.exists()); } + #[test] + fn no_replace_rename_preserves_file_and_directory_destinations() { + for directory in [false, true] { + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("source"); + let target = root.path().join("target"); + let contents = |path: &Path| { + if directory { + path.join("payload") + } else { + path.to_path_buf() + } + }; + if directory { + std::fs::create_dir(&source).unwrap(); + std::fs::create_dir(&target).unwrap(); + } + std::fs::write(contents(&source), b"source").unwrap(); + std::fs::write(contents(&target), b"sentinel").unwrap(); + assert_eq!( + rename_no_replace(&source, &target).unwrap_err().kind(), + io::ErrorKind::AlreadyExists + ); + assert_eq!(std::fs::read(contents(&source)).unwrap(), b"source"); + assert_eq!(std::fs::read(contents(&target)).unwrap(), b"sentinel"); + let absent = root.path().join("absent"); + rename_no_replace(&source, &absent).unwrap(); + assert!(!source.exists()); + assert_eq!(std::fs::read(contents(&absent)).unwrap(), b"source"); + } + } + + #[test] + fn no_replace_rename_concurrent_publish_has_one_winner() { + use std::sync::{Arc, Barrier}; + let root = tempfile::tempdir().unwrap(); + let target = root.path().join("target"); + let barrier = Arc::new(Barrier::new(3)); + let mut workers = Vec::new(); + for name in ["first", "second"] { + let source = root.path().join(name); + std::fs::write(&source, name.as_bytes()).unwrap(); + let target = target.clone(); + let barrier = Arc::clone(&barrier); + workers.push(std::thread::spawn(move || { + barrier.wait(); + (source.clone(), rename_no_replace(&source, &target)) + })); + } + barrier.wait(); + let results = workers + .into_iter() + .map(|worker| worker.join().unwrap()) + .collect::>(); + assert_eq!( + results.iter().filter(|(_, result)| result.is_ok()).count(), + 1 + ); + for (source, result) in results { + if let Err(error) = result { + assert_eq!(error.kind(), io::ErrorKind::AlreadyExists); + assert_eq!( + std::fs::read(&source).unwrap(), + source.file_name().unwrap().to_str().unwrap().as_bytes() + ); + } else { + assert!(!source.exists()); + assert_eq!( + std::fs::read(&target).unwrap(), + source.file_name().unwrap().to_str().unwrap().as_bytes() + ); + } + } + } + #[test] fn new_install_never_replaces_an_existing_entry() { let directory = tempfile::tempdir().unwrap(); diff --git a/crates/graphforge-storage/src/project_portable_v2_export.rs b/crates/graphforge-storage/src/project_portable_v2_export.rs index 474065092..dbb1e0b14 100644 --- a/crates/graphforge-storage/src/project_portable_v2_export.rs +++ b/crates/graphforge-storage/src/project_portable_v2_export.rs @@ -2275,34 +2275,8 @@ fn reject_destination(p: &Path) -> Result<(), ExportError> { } Ok(()) } -#[cfg(any(target_vendor = "apple", target_os = "linux", target_os = "redox"))] pub(crate) fn publish_no_replace(stage: &Path, destination: &Path) -> std::io::Result<()> { - rustix::fs::renameat_with( - rustix::fs::CWD, - stage, - rustix::fs::CWD, - destination, - rustix::fs::RenameFlags::NOREPLACE, - )?; - Ok(()) -} -#[cfg(windows)] -pub(crate) fn publish_no_replace(stage: &Path, destination: &Path) -> std::io::Result<()> { - // Windows rename is non-replacing. The destination was also checked before - // staging; any intervening creation makes this operation fail closed. - fs::rename(stage, destination) -} -#[cfg(not(any( - target_vendor = "apple", - target_os = "linux", - target_os = "redox", - windows -)))] -pub(crate) fn publish_no_replace(_: &Path, _: &Path) -> std::io::Result<()> { - Err(std::io::Error::new( - std::io::ErrorKind::Unsupported, - "atomic no-replace publication is unsupported on this platform", - )) + graphforge_filesystem::rename_no_replace(stage, destination) } fn parent(p: &Path) -> Result<(), ExportError> { fs::create_dir_all(p.parent().unwrap()).map_err(storage) diff --git a/docs/book/architecture/portable-project-v2.md b/docs/book/architecture/portable-project-v2.md index 7400b456a..060d9b351 100644 --- a/docs/book/architecture/portable-project-v2.md +++ b/docs/book/architecture/portable-project-v2.md @@ -251,7 +251,9 @@ canonical header order and PAX paths, exactly two end blocks, and the same no-replace publication. Until the final rename, neither representation exposes the requested destination. Cancellation, I/O failure, disk exhaustion, source mutation, or destination creation removes the private partial output and never -overwrites an existing destination. Re-running an unchanged plan produces the +overwrites an existing destination. The shared Rust filesystem primitive uses +native no-replace rename on Unix and Windows; Windows publication never enables +replacement or cross-volume copy. Re-running an unchanged plan produces the same package digest and bundle bytes; the two representations intentionally have distinct transport digests.