From 7c7d8f759dd1a64f181d4aa6ea7f7164e9dda6d4 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 03:05:54 +0200 Subject: [PATCH 1/8] Prevent manifest fixture overwrites (#61) Persist staged manifest fixtures without replacing an existing target. Treat a concurrent creation as success and cover that no-clobber path. --- test_support/src/manifest.rs | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index c0b03f58d..e1ee76cfb 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -99,8 +99,12 @@ fn write_manifest_content(file: &mut NamedTempFile, manifest_path: &Utf8Path) -> }) } +/// Persist a staged manifest without overwriting an existing target. +/// +/// A concurrently created target is tolerated because it already fulfils the +/// manifest-exists contract. fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::Result<()> { - match file.persist(manifest_path.as_std_path()) { + match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => Ok(()), Err(e) => Err(io::Error::new( @@ -180,7 +184,7 @@ mod tests { use super::*; use anyhow::{Context, Result}; use camino::Utf8Path; - use std::io; + use std::io::{self, Write}; use tempfile::TempDir; #[test] @@ -261,4 +265,31 @@ mod tests { ); Ok(()) } + + #[test] + fn persisting_manifest_tolerates_existing_file_without_overwriting() -> Result<()> { + let temp = TempDir::new().context("create temp dir")?; + let temp_path = Utf8Path::from_path(temp.path()) + .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; + let manifest_path = temp_path.join("manifest.yml"); + let existing_contents = b"existing manifest contents"; + fs::write(manifest_path.as_std_path(), existing_contents) + .context("create existing manifest")?; + + let mut staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + staged_file + .write_all(b"replacement manifest contents") + .context("write staged manifest")?; + + persist_manifest_file(staged_file, &manifest_path) + .context("persist staged manifest without overwriting")?; + + let contents = + fs::read(manifest_path.as_std_path()).context("read existing manifest contents")?; + anyhow::ensure!( + contents == existing_contents, + "manifest contents changed: {contents:?}" + ); + Ok(()) + } } From e1bebf37524f599dcf035dcd1a6e5f95259c43f7 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 15:55:11 +0200 Subject: [PATCH 2/8] Reject raced manifest directories (#61) Verify targets after no-clobber persistence reports `AlreadyExists` so existing files remain acceptable while directory targets are rejected. Share manifest test workspace setup and cover the raced-directory path. --- test_support/src/manifest.rs | 90 ++++++++++++++++++++++++++---------- 1 file changed, 65 insertions(+), 25 deletions(-) diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index e1ee76cfb..7f94db1f3 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -101,12 +101,20 @@ fn write_manifest_content(file: &mut NamedTempFile, manifest_path: &Utf8Path) -> /// Persist a staged manifest without overwriting an existing target. /// -/// A concurrently created target is tolerated because it already fulfils the -/// manifest-exists contract. +/// A concurrently created non-directory target is tolerated because it already +/// fulfils the manifest-exists contract. fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::Result<()> { match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), - Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => Ok(()), + Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists && fs::is_dir(manifest_path) => { + Err(io::Error::new( + io::ErrorKind::IsADirectory, + format!("Manifest path points to a directory, expected a file: {manifest_path}"), + )) + } + Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists && fs::exists(manifest_path) => { + Ok(()) + } Err(e) => Err(io::Error::new( e.error.kind(), format!( @@ -183,19 +191,31 @@ mod tests { use super::*; use anyhow::{Context, Result}; - use camino::Utf8Path; + use camino::{Utf8Path, Utf8PathBuf}; + use rstest::{fixture, rstest}; use std::io::{self, Write}; use tempfile::TempDir; - #[test] - fn existing_directory_manifest_path_is_rejected() -> Result<()> { + type TempManifestWorkspace = Result<(TempDir, Utf8PathBuf)>; + + #[fixture] + fn temp_manifest_workspace() -> TempManifestWorkspace { let temp = TempDir::new().context("create temp dir")?; let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; + .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))? + .to_owned(); + Ok((temp, temp_path)) + } + + #[rstest] + fn existing_directory_manifest_path_is_rejected( + temp_manifest_workspace: TempManifestWorkspace, + ) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; let dir = temp.path().join("dir"); fs::create_dir(&dir).context("create directory placeholder")?; - let Err(err) = ensure_manifest_exists(temp_path, Utf8Path::new("dir")) else { + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("dir")) else { anyhow::bail!("existing directory should be rejected"); }; anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); @@ -207,16 +227,16 @@ mod tests { Ok(()) } - #[test] - fn read_only_parent_reports_target_path() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; + #[rstest] + fn read_only_parent_reports_target_path( + temp_manifest_workspace: TempManifestWorkspace, + ) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; let parent = temp.path().join("parent"); fs::write(&parent, b"file").context("write placeholder parent file")?; let manifest = parent.join("manifest.yml"); - let Err(err) = ensure_manifest_exists(temp_path, Utf8Path::new("parent/manifest.yml")) + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) else { anyhow::bail!("non-directory parent should error"); }; @@ -229,11 +249,11 @@ mod tests { Ok(()) } - #[test] - fn creates_missing_parent_directory_and_manifest() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; + #[rstest] + fn creates_missing_parent_directory_and_manifest( + temp_manifest_workspace: TempManifestWorkspace, + ) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; // Parent directory does not exist beforehand. let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); @@ -244,7 +264,7 @@ mod tests { ); let manifest_path = - ensure_manifest_exists(temp_path, cli_file).context("create manifest when missing")?; + ensure_manifest_exists(&temp_path, cli_file).context("create manifest when missing")?; anyhow::ensure!(manifest_path == expected_path, "manifest path should match"); anyhow::ensure!(fs::exists(&manifest_path), "manifest file should exist"); anyhow::ensure!( @@ -266,11 +286,11 @@ mod tests { Ok(()) } - #[test] - fn persisting_manifest_tolerates_existing_file_without_overwriting() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; + #[rstest] + fn persisting_manifest_tolerates_existing_file_without_overwriting( + temp_manifest_workspace: TempManifestWorkspace, + ) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; let manifest_path = temp_path.join("manifest.yml"); let existing_contents = b"existing manifest contents"; fs::write(manifest_path.as_std_path(), existing_contents) @@ -292,4 +312,24 @@ mod tests { ); Ok(()) } + + #[rstest] + fn persisting_manifest_rejects_existing_directory( + temp_manifest_workspace: TempManifestWorkspace, + ) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + fs::create_dir(manifest_path.as_std_path()).context("create manifest directory")?; + let staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + + let Err(err) = persist_manifest_file(staged_file, &manifest_path) else { + anyhow::bail!("directory target should be rejected"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + anyhow::ensure!( + err.to_string().contains(manifest_path.as_str()), + "message: {err}" + ); + Ok(()) + } } From 1ab0dcd6f1819f8d274b9a766ee079678f8d2922 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 19:34:19 +0200 Subject: [PATCH 3/8] Document manifest existence guarantees Describe preservation of existing and concurrently created file targets, directory rejection, atomic staging, and the controlled proof boundary. --- test_support/src/manifest.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index 7f94db1f3..36e4c54d3 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -15,8 +15,14 @@ pub fn manifest_yaml(body: &str) -> String { /// Resolve `cli_file` relative to `temp_dir` and ensure it exists. /// /// When `cli_file` is relative, it is joined with `temp_dir` and the returned -/// path is absolute and UTF‑8. If the resulting path does not exist, a minimal -/// manifest is written to that location. +/// path is absolute and UTF‑8. If the resulting path already names a +/// non-directory target, that path is returned without modifying its contents. +/// If another actor creates a non-directory target after the initial existence +/// check but before persistence, that target is likewise returned unchanged. +/// Directory targets return an [`io::ErrorKind::IsADirectory`] error. When a +/// manifest must be created, staging occurs atomically in the target directory. +/// These guarantees describe the controlled implementation ordering and do not +/// establish behaviour for every filesystem or scheduler. /// /// # Errors /// From 4395bf89c38df01643f5c4a9b4392ea3021547ab Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 20:05:33 +0200 Subject: [PATCH 4/8] Cover manifest creation races (#61) Document the no-clobber contract and exercise all controlled target creation orderings through a test-only persistence seam. Keep generated staged content, existing files, and directory errors under bounded property coverage without asserting arbitrary scheduler behaviour. --- test_support/src/manifest.rs | 180 +++------------- test_support/src/manifest/tests.rs | 327 +++++++++++++++++++++++++++++ 2 files changed, 357 insertions(+), 150 deletions(-) create mode 100644 test_support/src/manifest/tests.rs diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index 36e4c54d3..cc7ed0f52 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -45,6 +45,14 @@ pub fn manifest_yaml(body: &str) -> String { /// assert!(manifest.exists()); /// ``` pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { + ensure_manifest_exists_impl(temp_dir, cli_file, |file, _| Ok(file)) +} + +fn ensure_manifest_exists_impl( + temp_dir: &Utf8Path, + cli_file: &Utf8Path, + before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, +) -> io::Result { let manifest_path = resolve_manifest_path(temp_dir, cli_file)?; if fs::is_dir(&manifest_path) { @@ -58,10 +66,23 @@ pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::R return Ok(manifest_path); } - create_manifest_file(temp_dir, manifest_path.as_ref())?; + create_manifest_file(temp_dir, manifest_path.as_ref(), before_persist)?; Ok(manifest_path) } +/// Run manifest creation with a deterministic action before persistence. +/// +/// This test-only seam models controlled target creation after the initial +/// existence check; it does not introduce scheduling into production code. +#[cfg(test)] +fn ensure_manifest_exists_with_before_persist( + temp_dir: &Utf8Path, + cli_file: &Utf8Path, + before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, +) -> io::Result { + ensure_manifest_exists_impl(temp_dir, cli_file, before_persist) +} + fn resolve_manifest_path(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { let manifest_path = if cli_file.is_absolute() { cli_file.to_owned() @@ -79,12 +100,16 @@ fn resolve_manifest_path(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result Ok(manifest_path) } -fn create_manifest_file(temp_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Result<()> { +fn create_manifest_file( + temp_dir: &Utf8Path, + manifest_path: &Utf8Path, + before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, +) -> io::Result<()> { let dest_dir = manifest_path.parent().unwrap_or(temp_dir); ensure_parent_directory(manifest_path, dest_dir)?; let mut file = create_temp_file(dest_dir, manifest_path)?; write_manifest_content(&mut file, manifest_path)?; - persist_manifest_file(file, manifest_path) + persist_manifest_file(before_persist(file, manifest_path)?, manifest_path) } fn create_temp_file(dest_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Result { @@ -192,150 +217,5 @@ fn find_existing_ancestor<'a>( } #[cfg(test)] -mod tests { - //! Unit tests for manifest fixture creation. - - use super::*; - use anyhow::{Context, Result}; - use camino::{Utf8Path, Utf8PathBuf}; - use rstest::{fixture, rstest}; - use std::io::{self, Write}; - use tempfile::TempDir; - - type TempManifestWorkspace = Result<(TempDir, Utf8PathBuf)>; - - #[fixture] - fn temp_manifest_workspace() -> TempManifestWorkspace { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))? - .to_owned(); - Ok((temp, temp_path)) - } - - #[rstest] - fn existing_directory_manifest_path_is_rejected( - temp_manifest_workspace: TempManifestWorkspace, - ) -> Result<()> { - let (temp, temp_path) = temp_manifest_workspace?; - let dir = temp.path().join("dir"); - fs::create_dir(&dir).context("create directory placeholder")?; - - let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("dir")) else { - anyhow::bail!("existing directory should be rejected"); - }; - anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); - let msg = err.to_string(); - let dir_str = dir - .to_str() - .ok_or_else(|| anyhow::anyhow!("dir path is not valid UTF-8"))?; - anyhow::ensure!(msg.contains(dir_str), "message: {msg}"); - Ok(()) - } - - #[rstest] - fn read_only_parent_reports_target_path( - temp_manifest_workspace: TempManifestWorkspace, - ) -> Result<()> { - let (temp, temp_path) = temp_manifest_workspace?; - let parent = temp.path().join("parent"); - fs::write(&parent, b"file").context("write placeholder parent file")?; - let manifest = parent.join("manifest.yml"); - - let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) - else { - anyhow::bail!("non-directory parent should error"); - }; - anyhow::ensure!(err.kind() == io::ErrorKind::AlreadyExists); - let msg = err.to_string(); - let manifest_str = manifest - .to_str() - .ok_or_else(|| anyhow::anyhow!("manifest path is not valid UTF-8"))?; - anyhow::ensure!(msg.contains(manifest_str), "message: {msg}"); - Ok(()) - } - - #[rstest] - fn creates_missing_parent_directory_and_manifest( - temp_manifest_workspace: TempManifestWorkspace, - ) -> Result<()> { - let (_temp, temp_path) = temp_manifest_workspace?; - - // Parent directory does not exist beforehand. - let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); - let expected_path = temp_path.join(cli_file); - anyhow::ensure!( - !fs::exists(&expected_path), - "precondition: path should not exist" - ); - - let manifest_path = - ensure_manifest_exists(&temp_path, cli_file).context("create manifest when missing")?; - anyhow::ensure!(manifest_path == expected_path, "manifest path should match"); - anyhow::ensure!(fs::exists(&manifest_path), "manifest file should exist"); - anyhow::ensure!( - fs::exists( - manifest_path - .parent() - .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? - ), - "parent directory should be created" - ); - - // Sanity check that content was written, not an empty file. - let contents = - fs::read_to_string(manifest_path.as_std_path()).context("read manifest contents")?; - anyhow::ensure!( - contents.contains("netsuke_version:"), - "unexpected manifest contents: {contents}" - ); - Ok(()) - } - - #[rstest] - fn persisting_manifest_tolerates_existing_file_without_overwriting( - temp_manifest_workspace: TempManifestWorkspace, - ) -> Result<()> { - let (temp, temp_path) = temp_manifest_workspace?; - let manifest_path = temp_path.join("manifest.yml"); - let existing_contents = b"existing manifest contents"; - fs::write(manifest_path.as_std_path(), existing_contents) - .context("create existing manifest")?; - - let mut staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; - staged_file - .write_all(b"replacement manifest contents") - .context("write staged manifest")?; - - persist_manifest_file(staged_file, &manifest_path) - .context("persist staged manifest without overwriting")?; - - let contents = - fs::read(manifest_path.as_std_path()).context("read existing manifest contents")?; - anyhow::ensure!( - contents == existing_contents, - "manifest contents changed: {contents:?}" - ); - Ok(()) - } - - #[rstest] - fn persisting_manifest_rejects_existing_directory( - temp_manifest_workspace: TempManifestWorkspace, - ) -> Result<()> { - let (temp, temp_path) = temp_manifest_workspace?; - let manifest_path = temp_path.join("manifest.yml"); - fs::create_dir(manifest_path.as_std_path()).context("create manifest directory")?; - let staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; - - let Err(err) = persist_manifest_file(staged_file, &manifest_path) else { - anyhow::bail!("directory target should be rejected"); - }; - anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); - anyhow::ensure!( - err.to_string().contains(manifest_path.as_str()), - "message: {err}" - ); - Ok(()) - } -} +#[path = "manifest/tests.rs"] +mod tests; diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs new file mode 100644 index 000000000..37fef71cd --- /dev/null +++ b/test_support/src/manifest/tests.rs @@ -0,0 +1,327 @@ +//! Unit tests for manifest fixture creation. + +use super::*; +use anyhow::{Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use proptest::{prelude::*, test_runner::TestCaseError}; +use rstest::{fixture, rstest}; +use std::io::{self, Write}; +use tempfile::TempDir; + +type TempManifestWorkspace = Result<(TempDir, Utf8PathBuf)>; + +#[fixture] +fn temp_manifest_workspace() -> TempManifestWorkspace { + let temp = TempDir::new().context("create temp dir")?; + let temp_path = Utf8Path::from_path(temp.path()) + .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))? + .to_owned(); + Ok((temp, temp_path)) +} + +#[rstest] +fn existing_directory_manifest_path_is_rejected( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let dir = temp.path().join("dir"); + fs::create_dir(&dir).context("create directory placeholder")?; + + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("dir")) else { + anyhow::bail!("existing directory should be rejected"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + let msg = err.to_string(); + let dir_str = dir + .to_str() + .ok_or_else(|| anyhow::anyhow!("dir path is not valid UTF-8"))?; + anyhow::ensure!(msg.contains(dir_str), "message: {msg}"); + Ok(()) +} + +#[rstest] +fn read_only_parent_reports_target_path( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let parent = temp.path().join("parent"); + fs::write(&parent, b"file").context("write placeholder parent file")?; + let manifest = parent.join("manifest.yml"); + + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) else { + anyhow::bail!("non-directory parent should error"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::AlreadyExists); + let msg = err.to_string(); + let manifest_str = manifest + .to_str() + .ok_or_else(|| anyhow::anyhow!("manifest path is not valid UTF-8"))?; + anyhow::ensure!(msg.contains(manifest_str), "message: {msg}"); + Ok(()) +} + +#[rstest] +fn creates_missing_parent_directory_and_manifest( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + + let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); + let expected_path = temp_path.join(cli_file); + anyhow::ensure!( + !fs::exists(&expected_path), + "precondition: path should not exist" + ); + + let manifest_path = + ensure_manifest_exists(&temp_path, cli_file).context("create manifest when missing")?; + anyhow::ensure!(manifest_path == expected_path, "manifest path should match"); + anyhow::ensure!(fs::exists(&manifest_path), "manifest file should exist"); + anyhow::ensure!( + fs::exists( + manifest_path + .parent() + .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? + ), + "parent directory should be created" + ); + + let contents = + fs::read_to_string(manifest_path.as_std_path()).context("read manifest contents")?; + anyhow::ensure!( + contents.contains("netsuke_version:"), + "unexpected manifest contents: {contents}" + ); + Ok(()) +} + +#[rstest] +fn existing_file_manifest_path_is_returned_unchanged( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + let existing_contents = b"existing manifest contents"; + fs::write(manifest_path.as_std_path(), existing_contents) + .context("create existing manifest")?; + + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("return existing manifest")?; + + anyhow::ensure!(returned_path == manifest_path, "manifest path should match"); + let contents = fs::read(manifest_path.as_std_path()).context("read existing manifest")?; + anyhow::ensure!(contents == existing_contents, "manifest contents changed"); + Ok(()) +} + +#[rstest] +fn raced_file_manifest_path_is_returned_unchanged( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let expected_path = temp_path.join("manifest.yml"); + let competing_contents = b"competing manifest contents"; + + let returned_path = ensure_manifest_exists_with_before_persist( + &temp_path, + Utf8Path::new("manifest.yml"), + |file, manifest_path| { + fs::write(manifest_path.as_std_path(), competing_contents)?; + Ok(file) + }, + ) + .context("tolerate manifest created before persistence")?; + + anyhow::ensure!(returned_path == expected_path, "manifest path should match"); + let contents = fs::read(expected_path.as_std_path()).context("read competing manifest")?; + anyhow::ensure!(contents == competing_contents, "manifest contents changed"); + Ok(()) +} + +#[rstest] +fn raced_directory_manifest_path_is_rejected( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let expected_path = temp_path.join("manifest.yml"); + + let Err(err) = ensure_manifest_exists_with_before_persist( + &temp_path, + Utf8Path::new("manifest.yml"), + |file, manifest_path| { + fs::create_dir(manifest_path.as_std_path())?; + Ok(file) + }, + ) else { + anyhow::bail!("directory created before persistence should be rejected"); + }; + + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + anyhow::ensure!( + err.to_string().contains(expected_path.as_str()), + "message: {err}" + ); + Ok(()) +} + +#[rstest] +fn persisting_manifest_tolerates_existing_file_without_overwriting( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + let existing_contents = b"existing manifest contents"; + fs::write(manifest_path.as_std_path(), existing_contents) + .context("create existing manifest")?; + + let mut staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + staged_file + .write_all(b"replacement manifest contents") + .context("write staged manifest")?; + + persist_manifest_file(staged_file, &manifest_path) + .context("persist staged manifest without overwriting")?; + + let contents = + fs::read(manifest_path.as_std_path()).context("read existing manifest contents")?; + anyhow::ensure!( + contents == existing_contents, + "manifest contents changed: {contents:?}" + ); + Ok(()) +} + +#[rstest] +fn persisting_manifest_rejects_existing_directory( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + fs::create_dir(manifest_path.as_std_path()).context("create manifest directory")?; + let staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + + let Err(err) = persist_manifest_file(staged_file, &manifest_path) else { + anyhow::bail!("directory target should be rejected"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + anyhow::ensure!( + err.to_string().contains(manifest_path.as_str()), + "message: {err}" + ); + Ok(()) +} + +#[derive(Clone, Copy, Debug)] +enum TargetState { + Missing, + ExistingFile, + RacedFile, + ExistingDirectory, + RacedDirectory, +} + +fn target_state_strategy() -> impl Strategy { + prop_oneof![ + Just(TargetState::Missing), + Just(TargetState::ExistingFile), + Just(TargetState::RacedFile), + Just(TargetState::ExistingDirectory), + Just(TargetState::RacedDirectory), + ] +} + +fn property_workspace() -> Result<(TempDir, Utf8PathBuf), TestCaseError> { + let temp = TempDir::new().map_err(|error| TestCaseError::fail(error.to_string()))?; + let temp_path = Utf8Path::from_path(temp.path()) + .ok_or_else(|| TestCaseError::fail("temporary path is not UTF-8"))? + .to_owned(); + Ok((temp, temp_path)) +} + +fn replace_staged_manifest( + staged_file: NamedTempFile, + manifest_path: &Utf8Path, + contents: &[u8], +) -> io::Result { + drop(staged_file); + let destination_directory = manifest_path.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("Manifest path has no parent directory: {manifest_path}"), + ) + })?; + let mut replacement = NamedTempFile::new_in(destination_directory.as_std_path())?; + replacement.write_all(contents)?; + Ok(replacement) +} + +// This property checks controlled creation orderings through the injected hook. +// It does not model arbitrary operating-system scheduling or filesystems. +proptest! { + #![proptest_config(ProptestConfig::with_cases(16))] + + #[test] + fn manifest_existence_contract_holds_for_bounded_target_states( + state in target_state_strategy(), + staged_name in "[a-z]{1,12}", + competing_contents in proptest::collection::vec(any::(), 1..25), + ) { + let (_temp, temp_path) = property_workspace()?; + let cli_file = Utf8Path::new("manifest.yml"); + let expected_path = temp_path.join(cli_file); + let staged_contents = manifest_yaml(&format!( + "targets:\n - name: {staged_name}\n command: \"echo hi\"\n" + )) + .into_bytes(); + + match state { + TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents) + .map_err(|error| TestCaseError::fail(error.to_string()))?, + TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path()) + .map_err(|error| TestCaseError::fail(error.to_string()))?, + TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} + } + + let expected_staged_contents = staged_contents.clone(); + let hook_contents = competing_contents.clone(); + let result = ensure_manifest_exists_with_before_persist( + &temp_path, + cli_file, + move |file, manifest_path| { + let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; + match state { + TargetState::RacedFile => { + fs::write(manifest_path.as_std_path(), hook_contents)?; + } + TargetState::RacedDirectory => fs::create_dir(manifest_path.as_std_path())?, + TargetState::Missing + | TargetState::ExistingFile + | TargetState::ExistingDirectory => {} + } + Ok(replacement) + }, + ); + + match state { + TargetState::Missing => { + let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; + prop_assert_eq!(&returned_path, &expected_path); + let contents = fs::read(expected_path.as_std_path()) + .map_err(|error| TestCaseError::fail(error.to_string()))?; + prop_assert_eq!(contents, expected_staged_contents); + } + TargetState::ExistingFile | TargetState::RacedFile => { + let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; + prop_assert_eq!(&returned_path, &expected_path); + let contents = fs::read(expected_path.as_std_path()) + .map_err(|error| TestCaseError::fail(error.to_string()))?; + prop_assert_eq!(contents, competing_contents); + } + TargetState::ExistingDirectory | TargetState::RacedDirectory => { + let error = result.expect_err("directory target should be rejected"); + prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); + prop_assert!(error.to_string().contains(expected_path.as_str())); + } + } + } +} From b11649175dfcc1b44343819306ab424b81e5f890 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 12 Aug 2026 00:12:54 +0200 Subject: [PATCH 5/8] Propagate manifest target inspection errors (#61) Inspect manifest paths through a fallible test-support filesystem wrapper so inaccessible metadata is never mistaken for an absent target. Exercise the exported manifest helper with a scoped pre-persist hook and document the controlled no-clobber contract for fixture authors. --- docs/developers-guide.md | 21 +++- test_support/src/fs.rs | 46 ++++++++ test_support/src/fs_tests.rs | 32 ++++- test_support/src/manifest.rs | 182 ++++++++++++++++++++--------- test_support/src/manifest/tests.rs | 73 ++++++++---- 5 files changed, 266 insertions(+), 88 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 3b4f21a24..b71b5e03d 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1712,10 +1712,7 @@ is not obvious from the name: absent or unreadable path returns `false` rather than surfacing the underlying metadata error. Fixture code must use this wrapper for directory predicates rather than calling `std::fs::metadata(...).is_dir()` or - `Path::is_dir` directly. `test_support/src/manifest.rs` is an existing caller: - `ensure_manifest_exists` uses it both to reject a directory where a manifest - file is expected, and to accept a destination directory that is already - present. + `Path::is_dir` directly. - `try_is_file(path) -> io::Result` is the fallible counterpart to the boolean predicates: `Ok(true)` when the path is a regular file, `Ok(false)` when it is absent (`NotFound` is folded into the boolean result), and `Err` @@ -1749,6 +1746,22 @@ ambient boundary stays where the lint expects it. Prefer that shape — pass in what the operation needs and keep the handle here — over widening an exclusion to a module that wants a raw `File`. +### `test_support::ensure_manifest_exists` + +`test_support::ensure_manifest_exists` (`test_support/src/manifest.rs`) never +overwrites an existing non-directory target. If another actor creates a +non-directory target after the initial existence check but before persistence, +no-clobber persistence leaves that target unchanged and returns its path, which +satisfies the existence contract. An existing directory, including one created +at the controlled pre-persist point, returns `io::ErrorKind::IsADirectory`. + +When a manifest is missing, its generated contents are written to a temporary +file staged in the destination directory before persistence. The tests inject +the pre-persist action to cover controlled creation orderings; they do not claim +to model arbitrary scheduler or filesystem interleavings. The fallible +`test_support::fs::inspect_path` probe treats `NotFound` as absence and +propagates every other metadata error. + ### Shared Makefile contract helpers `tests/support/makefile.rs` is a shared module for integration tests that diff --git a/test_support/src/fs.rs b/test_support/src/fs.rs index 36009846a..dc2f1b605 100644 --- a/test_support/src/fs.rs +++ b/test_support/src/fs.rs @@ -21,6 +21,17 @@ use std::io; use std::path::Path; use std::time::SystemTime; +/// The state observed when inspecting a filesystem path. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PathState { + /// The path does not exist. + Absent, + /// The path exists and is a directory. + Directory, + /// The path exists and is not a directory. + NonDirectory, +} + /// Write `contents` to `path`, creating or truncating the file. /// /// # Errors @@ -117,6 +128,41 @@ pub fn is_dir(path: impl AsRef) -> bool { fs::metadata(path).is_ok_and(|metadata| metadata.is_dir()) } +/// Inspect whether `path` is absent, a directory, or another target. +/// +/// Only [`io::ErrorKind::NotFound`] becomes [`PathState::Absent`]. All other +/// metadata failures are returned so callers do not mistake an unreadable path +/// for an absent one. +/// +/// # Errors +/// +/// Propagates the underlying metadata failure for any error other than +/// [`io::ErrorKind::NotFound`]. +/// +/// # Examples +/// +/// ``` +/// use test_support::fs::{PathState, inspect_path}; +/// +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let file = dir.path().join("file"); +/// test_support::fs::write(&file, "contents").expect("write file"); +/// assert_eq!(inspect_path(&file).expect("inspect file"), PathState::NonDirectory); +/// assert_eq!(inspect_path(dir.path()).expect("inspect directory"), PathState::Directory); +/// assert_eq!( +/// inspect_path(dir.path().join("absent")).expect("inspect absent path"), +/// PathState::Absent +/// ); +/// ``` +pub fn inspect_path(path: impl AsRef) -> io::Result { + match fs::metadata(path) { + Ok(metadata) if metadata.is_dir() => Ok(PathState::Directory), + Ok(_) => Ok(PathState::NonDirectory), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(PathState::Absent), + Err(error) => Err(error), + } +} + /// Return `true` when `path` is a regular file, surfacing unexpected errors. /// /// Unlike `Path::is_file`, an I/O failure other than `NotFound` is propagated diff --git a/test_support/src/fs_tests.rs b/test_support/src/fs_tests.rs index 3b5c9d793..eb549cb98 100644 --- a/test_support/src/fs_tests.rs +++ b/test_support/src/fs_tests.rs @@ -5,7 +5,7 @@ //! `module_max_lines` cap; included from there via `#[path]` so the tests //! stay a child module of `fs`. -use super::{create_dir_all, try_is_file, write}; +use super::{PathState, create_dir_all, inspect_path, try_is_file, write}; use rstest::{fixture, rstest}; use std::io; @@ -72,6 +72,36 @@ fn try_is_file_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyh Ok(()) } +#[rstest] +fn inspect_path_distinguishes_absent_directory_and_non_directory( + temp_dir: TempDir, +) -> anyhow::Result<()> { + let temp = temp_dir?; + let file = temp.path().join("regular-file"); + write(&file, b"fixture")?; + + anyhow::ensure!(inspect_path(temp.path())? == PathState::Directory); + anyhow::ensure!(inspect_path(&file)? == PathState::NonDirectory); + anyhow::ensure!(inspect_path(temp.path().join("absent"))? == PathState::Absent); + Ok(()) +} + +#[rstest] +fn inspect_path_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> { + let temp = temp_dir?; + let file = temp.path().join("regular-file"); + write(&file, b"fixture")?; + + let Err(error) = inspect_path(file.join("child")) else { + anyhow::bail!("traversing through a regular file should fail"); + }; + anyhow::ensure!( + error.kind() != io::ErrorKind::NotFound, + "traversal through a file should not be reported as absence, got {error:?}" + ); + Ok(()) +} + #[rstest] fn create_dir_all_accepts_an_existing_directory(temp_dir: TempDir) -> io::Result<()> { let temp = temp_dir?; diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index cc7ed0f52..ae9cdc6f7 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -6,6 +6,9 @@ use cap_std::{ambient_authority, fs_utf8}; use std::io; use tempfile::NamedTempFile; +#[cfg(test)] +use std::cell::RefCell; + /// Prefix the provided manifest body with the standard Netsuke version header. #[must_use] pub fn manifest_yaml(body: &str) -> String { @@ -21,6 +24,8 @@ pub fn manifest_yaml(body: &str) -> String { /// check but before persistence, that target is likewise returned unchanged. /// Directory targets return an [`io::ErrorKind::IsADirectory`] error. When a /// manifest must be created, staging occurs atomically in the target directory. +/// Target inspection is fallible: metadata errors other than `NotFound` +/// propagate to the caller. /// These guarantees describe the controlled implementation ordering and do not /// establish behaviour for every filesystem or scheduler. /// @@ -45,42 +50,44 @@ pub fn manifest_yaml(body: &str) -> String { /// assert!(manifest.exists()); /// ``` pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { - ensure_manifest_exists_impl(temp_dir, cli_file, |file, _| Ok(file)) -} - -fn ensure_manifest_exists_impl( - temp_dir: &Utf8Path, - cli_file: &Utf8Path, - before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, -) -> io::Result { let manifest_path = resolve_manifest_path(temp_dir, cli_file)?; - if fs::is_dir(&manifest_path) { - return Err(io::Error::new( - io::ErrorKind::IsADirectory, - format!("Manifest path points to a directory, expected a file: {manifest_path}"), - )); + match inspect_manifest_target(&manifest_path)? { + fs::PathState::Absent => create_manifest_file(temp_dir, manifest_path.as_ref())?, + fs::PathState::Directory => return Err(manifest_path_is_directory_error(&manifest_path)), + fs::PathState::NonDirectory => return Ok(manifest_path), } - if fs::exists(&manifest_path) { - return Ok(manifest_path); - } - - create_manifest_file(temp_dir, manifest_path.as_ref(), before_persist)?; Ok(manifest_path) } -/// Run manifest creation with a deterministic action before persistence. +fn inspect_manifest_target(manifest_path: &Utf8Path) -> io::Result { + fs::inspect_path(manifest_path).map_err(|error| { + io::Error::new( + error.kind(), + format!("Failed to inspect manifest path {manifest_path}: {error}"), + ) + }) +} + +fn manifest_path_is_directory_error(manifest_path: &Utf8Path) -> io::Error { + io::Error::new( + io::ErrorKind::IsADirectory, + format!("Manifest path points to a directory, expected a file: {manifest_path}"), + ) +} + +/// Install a deterministic action before a staged manifest is persisted. /// /// This test-only seam models controlled target creation after the initial -/// existence check; it does not introduce scheduling into production code. +/// existence check; it does not introduce scheduling into production code. Its +/// guard restores the preceding hook when it is dropped. #[cfg(test)] -fn ensure_manifest_exists_with_before_persist( - temp_dir: &Utf8Path, - cli_file: &Utf8Path, - before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, -) -> io::Result { - ensure_manifest_exists_impl(temp_dir, cli_file, before_persist) +fn install_before_persist_hook( + hook: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result + 'static, +) -> BeforePersistHookGuard { + let previous = BEFORE_PERSIST_HOOK.with(|hook_slot| hook_slot.replace(Some(Box::new(hook)))); + BeforePersistHookGuard { previous } } fn resolve_manifest_path(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { @@ -100,16 +107,20 @@ fn resolve_manifest_path(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result Ok(manifest_path) } -fn create_manifest_file( - temp_dir: &Utf8Path, - manifest_path: &Utf8Path, - before_persist: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result, -) -> io::Result<()> { +fn create_manifest_file(temp_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Result<()> { let dest_dir = manifest_path.parent().unwrap_or(temp_dir); ensure_parent_directory(manifest_path, dest_dir)?; let mut file = create_temp_file(dest_dir, manifest_path)?; write_manifest_content(&mut file, manifest_path)?; - persist_manifest_file(before_persist(file, manifest_path)?, manifest_path) + #[cfg(test)] + { + persist_manifest_file(run_before_persist_hook(file, manifest_path)?, manifest_path) + } + + #[cfg(not(test))] + { + persist_manifest_file(file, manifest_path) + } } fn create_temp_file(dest_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Result { @@ -137,14 +148,17 @@ fn write_manifest_content(file: &mut NamedTempFile, manifest_path: &Utf8Path) -> fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::Result<()> { match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), - Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists && fs::is_dir(manifest_path) => { - Err(io::Error::new( - io::ErrorKind::IsADirectory, - format!("Manifest path points to a directory, expected a file: {manifest_path}"), - )) - } - Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists && fs::exists(manifest_path) => { - Ok(()) + Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => { + match inspect_manifest_target(manifest_path)? { + fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), + fs::PathState::NonDirectory => Ok(()), + fs::PathState::Absent => Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" + ), + )), + } } Err(e) => Err(io::Error::new( e.error.kind(), @@ -159,19 +173,22 @@ fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::R } fn ensure_parent_directory(manifest_path: &Utf8Path, dest_dir: &Utf8Path) -> io::Result<()> { - if fs::exists(dest_dir) { - // If the path exists but is not a directory, report a clear error that - // includes the final manifest path. Returning AlreadyExists mirrors the - // semantics that the desired directory “exists” but is unusable. - if fs::is_dir(dest_dir) { - return Ok(()); + match fs::inspect_path(dest_dir).map_err(|error| { + io::Error::new( + error.kind(), + format!("Failed to inspect manifest parent directory for {manifest_path}: {error}"), + ) + })? { + fs::PathState::Directory => return Ok(()), + fs::PathState::NonDirectory => { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Failed to create manifest parent directory for {manifest_path}: parent path exists and is not a directory", + ), + )); } - return Err(io::Error::new( - io::ErrorKind::AlreadyExists, - format!( - "Failed to create manifest parent directory for {manifest_path}: parent path exists and is not a directory", - ), - )); + fs::PathState::Absent => {} } let base = find_existing_ancestor(dest_dir, manifest_path)?; @@ -204,16 +221,65 @@ fn find_existing_ancestor<'a>( let mut ancestors = dest_dir.ancestors(); ancestors.next(); // Skip self - ancestors - .find(|candidate| fs::exists(candidate)) - .ok_or_else(|| { + for candidate in ancestors { + match fs::inspect_path(candidate).map_err(|error| { io::Error::new( - io::ErrorKind::NotFound, + error.kind(), format!( - "Failed to locate an existing ancestor for manifest directory {manifest_path}", + "Failed to inspect manifest ancestor {candidate} for {manifest_path}: {error}" ), ) - }) + })? { + fs::PathState::Directory => return Ok(candidate), + fs::PathState::NonDirectory => { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Failed to create manifest parent directory for {manifest_path}: ancestor {candidate} is not a directory", + ), + )); + } + fs::PathState::Absent => {} + } + } + + Err(io::Error::new( + io::ErrorKind::NotFound, + format!("Failed to locate an existing ancestor for manifest directory {manifest_path}"), + )) +} + +#[cfg(test)] +type BeforePersistHook = Box io::Result>; + +#[cfg(test)] +thread_local! { + static BEFORE_PERSIST_HOOK: RefCell> = const { RefCell::new(None) }; +} + +#[cfg(test)] +#[must_use] +struct BeforePersistHookGuard { + previous: Option, +} + +#[cfg(test)] +impl Drop for BeforePersistHookGuard { + fn drop(&mut self) { + BEFORE_PERSIST_HOOK.with(|hook_slot| *hook_slot.borrow_mut() = self.previous.take()); + } +} + +#[cfg(test)] +fn run_before_persist_hook( + file: NamedTempFile, + manifest_path: &Utf8Path, +) -> io::Result { + let hook = BEFORE_PERSIST_HOOK.with(|hook_slot| hook_slot.borrow_mut().take()); + match hook { + Some(installed_hook) => installed_hook(file, manifest_path), + None => Ok(file), + } } #[cfg(test)] diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs index 37fef71cd..f4c8a191a 100644 --- a/test_support/src/manifest/tests.rs +++ b/test_support/src/manifest/tests.rs @@ -40,7 +40,7 @@ fn existing_directory_manifest_path_is_rejected( } #[rstest] -fn read_only_parent_reports_target_path( +fn non_directory_parent_propagates_target_inspection_error( temp_manifest_workspace: TempManifestWorkspace, ) -> Result<()> { let (temp, temp_path) = temp_manifest_workspace?; @@ -51,7 +51,10 @@ fn read_only_parent_reports_target_path( let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) else { anyhow::bail!("non-directory parent should error"); }; - anyhow::ensure!(err.kind() == io::ErrorKind::AlreadyExists); + anyhow::ensure!( + err.kind() != io::ErrorKind::NotFound, + "target inspection error should not be treated as absence: {err}" + ); let msg = err.to_string(); let manifest_str = manifest .to_str() @@ -121,16 +124,17 @@ fn raced_file_manifest_path_is_returned_unchanged( let (_temp, temp_path) = temp_manifest_workspace?; let expected_path = temp_path.join("manifest.yml"); let competing_contents = b"competing manifest contents"; + anyhow::ensure!( + fs::inspect_path(&expected_path)? == fs::PathState::Absent, + "precondition: manifest path should be absent" + ); - let returned_path = ensure_manifest_exists_with_before_persist( - &temp_path, - Utf8Path::new("manifest.yml"), - |file, manifest_path| { - fs::write(manifest_path.as_std_path(), competing_contents)?; - Ok(file) - }, - ) - .context("tolerate manifest created before persistence")?; + let _hook = install_before_persist_hook(move |file, manifest_path| { + fs::write(manifest_path.as_std_path(), competing_contents)?; + Ok(file) + }); + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("tolerate manifest created before persistence")?; anyhow::ensure!(returned_path == expected_path, "manifest path should match"); let contents = fs::read(expected_path.as_std_path()).context("read competing manifest")?; @@ -144,15 +148,16 @@ fn raced_directory_manifest_path_is_rejected( ) -> Result<()> { let (_temp, temp_path) = temp_manifest_workspace?; let expected_path = temp_path.join("manifest.yml"); + anyhow::ensure!( + fs::inspect_path(&expected_path)? == fs::PathState::Absent, + "precondition: manifest path should be absent" + ); - let Err(err) = ensure_manifest_exists_with_before_persist( - &temp_path, - Utf8Path::new("manifest.yml"), - |file, manifest_path| { - fs::create_dir(manifest_path.as_std_path())?; - Ok(file) - }, - ) else { + let _hook = install_before_persist_hook(|file, manifest_path| { + fs::create_dir(manifest_path.as_std_path())?; + Ok(file) + }); + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) else { anyhow::bail!("directory created before persistence should be rejected"); }; @@ -164,6 +169,27 @@ fn raced_directory_manifest_path_is_rejected( Ok(()) } +#[rstest] +fn before_persist_hook_does_not_escape_its_scope( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + { + let _hook = install_before_persist_hook(|_, _| { + Err(io::Error::other( + "hook should be removed when its guard drops", + )) + }); + } + + let expected_path = temp_path.join("manifest.yml"); + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("create manifest after hook scope ends")?; + + anyhow::ensure!(returned_path == expected_path, "manifest path should match"); + Ok(()) +} + #[rstest] fn persisting_manifest_tolerates_existing_file_without_overwriting( temp_manifest_workspace: TempManifestWorkspace, @@ -284,10 +310,7 @@ proptest! { let expected_staged_contents = staged_contents.clone(); let hook_contents = competing_contents.clone(); - let result = ensure_manifest_exists_with_before_persist( - &temp_path, - cli_file, - move |file, manifest_path| { + let _hook = install_before_persist_hook(move |file, manifest_path| { let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; match state { TargetState::RacedFile => { @@ -299,8 +322,8 @@ proptest! { | TargetState::ExistingDirectory => {} } Ok(replacement) - }, - ); + }); + let result = ensure_manifest_exists(&temp_path, cli_file); match state { TargetState::Missing => { From 5e0105489139e287ccaddb0d81a6169e0a24122a Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 12 Aug 2026 04:36:16 +0200 Subject: [PATCH 6/8] Deduplicate filesystem error tests (#61) Share the file-traversal assertion while keeping the named tests for the `inspect_path` and `try_is_file` public contracts. Forward Whitaker's root configuration to its isolated driver so its documented scoped exclusions remain effective. --- Makefile | 2 +- test_support/src/fs_tests.rs | 45 +++++++++++++++++------------------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/Makefile b/Makefile index c0844da19..4a85f0bd6 100644 --- a/Makefile +++ b/Makefile @@ -100,7 +100,7 @@ lint-clippy: ## Run rustdoc and Clippy with warnings denied RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) clippy $(CLIPPY_FLAGS) lint-whitaker: ## Run the Whitaker Dylint suite with warnings denied - RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package netsuke-build -- --all-targets --all-features + DYLINT_TOML="$$(cat dylint.toml)" RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package netsuke-build -- --all-targets --all-features # Run from the crate directory as well so Whitaker loads the narrow # `test_support::fs` exemption from test_support/dylint.toml. cd test_support && DYLINT_TOML="$$(cat dylint.toml)" RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package test_support -- --all-targets --all-features diff --git a/test_support/src/fs_tests.rs b/test_support/src/fs_tests.rs index eb549cb98..471f1a55b 100644 --- a/test_support/src/fs_tests.rs +++ b/test_support/src/fs_tests.rs @@ -8,6 +8,7 @@ use super::{PathState, create_dir_all, inspect_path, try_is_file, write}; use rstest::{fixture, rstest}; use std::io; +use std::path::Path; /// Temporary workspace for the filesystem helper tests. /// @@ -21,6 +22,24 @@ fn temp_dir() -> TempDir { tempfile::tempdir() } +fn assert_traversal_through_file_propagates_non_not_found( + temp_dir: TempDir, + inspect: impl FnOnce(&Path) -> io::Result, +) -> anyhow::Result<()> { + let temp = temp_dir?; + let file = temp.path().join("regular-file"); + write(&file, b"fixture")?; + + let Err(error) = inspect(&file.join("child")) else { + anyhow::bail!("traversing through a regular file should fail"); + }; + anyhow::ensure!( + error.kind() != io::ErrorKind::NotFound, + "traversal through a file should not be reported as absence, got {error:?}" + ); + Ok(()) +} + #[rstest] fn try_is_file_reports_a_regular_file_as_a_file(temp_dir: TempDir) -> anyhow::Result<()> { let temp = temp_dir?; @@ -58,18 +77,7 @@ fn try_is_file_reports_a_directory_as_not_a_file(temp_dir: TempDir) -> anyhow::R #[rstest] fn try_is_file_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> { - let temp = temp_dir?; - let file = temp.path().join("regular-file"); - write(&file, b"fixture")?; - - let Err(error) = try_is_file(file.join("child")) else { - anyhow::bail!("traversing through a regular file should fail"); - }; - anyhow::ensure!( - error.kind() != io::ErrorKind::NotFound, - "traversal through a file should not be reported as absence, got {error:?}" - ); - Ok(()) + assert_traversal_through_file_propagates_non_not_found(temp_dir, |path| try_is_file(path)) } #[rstest] @@ -88,18 +96,7 @@ fn inspect_path_distinguishes_absent_directory_and_non_directory( #[rstest] fn inspect_path_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> { - let temp = temp_dir?; - let file = temp.path().join("regular-file"); - write(&file, b"fixture")?; - - let Err(error) = inspect_path(file.join("child")) else { - anyhow::bail!("traversing through a regular file should fail"); - }; - anyhow::ensure!( - error.kind() != io::ErrorKind::NotFound, - "traversal through a file should not be reported as absence, got {error:?}" - ); - Ok(()) + assert_traversal_through_file_propagates_non_not_found(temp_dir, |path| inspect_path(path)) } #[rstest] From 290953e29c0bde6103ff75a99c1733d6dc1ff1f8 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 12 Aug 2026 12:20:09 +0200 Subject: [PATCH 7/8] Harden manifest fixture diagnostics (#61) Report non-directory parent components as `NotADirectory` and cover both internal branches. Confirm raced persistence removes staged files, improve the property assertion, and document the filesystem and hook contracts. --- docs/developers-guide.md | 6 ++++ test_support/src/fs.rs | 2 ++ test_support/src/manifest.rs | 9 ++++-- test_support/src/manifest/tests.rs | 52 +++++++++++++++++++++++++++++- 4 files changed, 65 insertions(+), 4 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index b71b5e03d..1add5ea61 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1713,6 +1713,12 @@ is not obvious from the name: underlying metadata error. Fixture code must use this wrapper for directory predicates rather than calling `std::fs::metadata(...).is_dir()` or `Path::is_dir` directly. +- `PathState` and `inspect_path(path) -> io::Result` provide a + fallible target-state probe. `PathState::Absent` means metadata returned + `NotFound`, `PathState::Directory` means the target is a directory, and + `PathState::NonDirectory` means it exists but is not a directory. The probe + follows symlinks, so a dangling symlink is `Absent` even when its directory + entry exists; metadata errors other than `NotFound` are propagated. - `try_is_file(path) -> io::Result` is the fallible counterpart to the boolean predicates: `Ok(true)` when the path is a regular file, `Ok(false)` when it is absent (`NotFound` is folded into the boolean result), and `Err` diff --git a/test_support/src/fs.rs b/test_support/src/fs.rs index dc2f1b605..a2afd5754 100644 --- a/test_support/src/fs.rs +++ b/test_support/src/fs.rs @@ -133,6 +133,8 @@ pub fn is_dir(path: impl AsRef) -> bool { /// Only [`io::ErrorKind::NotFound`] becomes [`PathState::Absent`]. All other /// metadata failures are returned so callers do not mistake an unreadable path /// for an absent one. +/// [`std::fs::metadata`] follows symbolic links, so a dangling symbolic link is +/// reported as [`PathState::Absent`] even though its directory entry exists. /// /// # Errors /// diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index ae9cdc6f7..c0067737f 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -81,7 +81,10 @@ fn manifest_path_is_directory_error(manifest_path: &Utf8Path) -> io::Error { /// /// This test-only seam models controlled target creation after the initial /// existence check; it does not introduce scheduling into production code. Its -/// guard restores the preceding hook when it is dropped. +/// action is one-shot: [`run_before_persist_hook`] removes it before execution, +/// so it runs at most once per guard even across multiple +/// [`ensure_manifest_exists`] calls. The guard restores the preceding hook when +/// it is dropped. #[cfg(test)] fn install_before_persist_hook( hook: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result + 'static, @@ -182,7 +185,7 @@ fn ensure_parent_directory(manifest_path: &Utf8Path, dest_dir: &Utf8Path) -> io: fs::PathState::Directory => return Ok(()), fs::PathState::NonDirectory => { return Err(io::Error::new( - io::ErrorKind::AlreadyExists, + io::ErrorKind::NotADirectory, format!( "Failed to create manifest parent directory for {manifest_path}: parent path exists and is not a directory", ), @@ -233,7 +236,7 @@ fn find_existing_ancestor<'a>( fs::PathState::Directory => return Ok(candidate), fs::PathState::NonDirectory => { return Err(io::Error::new( - io::ErrorKind::AlreadyExists, + io::ErrorKind::NotADirectory, format!( "Failed to create manifest parent directory for {manifest_path}: ancestor {candidate} is not a directory", ), diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs index f4c8a191a..e70d3c178 100644 --- a/test_support/src/manifest/tests.rs +++ b/test_support/src/manifest/tests.rs @@ -19,6 +19,21 @@ fn temp_manifest_workspace() -> TempManifestWorkspace { Ok((temp, temp_path)) } +fn assert_no_staged_manifest_files(temp_path: &Utf8Path, expected_path: &Utf8Path) -> Result<()> { + for entry_result in temp_path + .read_dir_utf8() + .context("inspect manifest workspace")? + { + let workspace_entry = entry_result.context("inspect manifest workspace entry")?; + anyhow::ensure!( + workspace_entry.path() == expected_path, + "leftover staged manifest file: {}", + workspace_entry.path() + ); + } + Ok(()) +} + #[rstest] fn existing_directory_manifest_path_is_rejected( temp_manifest_workspace: TempManifestWorkspace, @@ -63,6 +78,36 @@ fn non_directory_parent_propagates_target_inspection_error( Ok(()) } +#[rstest] +fn non_directory_parent_components_are_reported_as_not_a_directory( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let parent = temp_path.join("parent"); + fs::write(parent.as_std_path(), b"file").context("write placeholder parent file")?; + let manifest_path = parent.join("manifest.yml"); + + let Err(parent_err) = ensure_parent_directory(&manifest_path, &parent) else { + anyhow::bail!("non-directory parent should be rejected"); + }; + anyhow::ensure!(parent_err.kind() == io::ErrorKind::NotADirectory); + anyhow::ensure!(parent_err.to_string().contains(manifest_path.as_str())); + + let ancestor = temp_path.join("ancestor"); + fs::write(ancestor.as_std_path(), b"file").context("write placeholder ancestor file")?; + let ancestor_manifest_path = ancestor.join("child/manifest.yml"); + let dest_dir = ancestor_manifest_path + .parent() + .context("manifest path missing parent")?; + let Err(ancestor_err) = find_existing_ancestor(dest_dir, &ancestor_manifest_path) else { + anyhow::bail!("non-directory ancestor should be rejected"); + }; + anyhow::ensure!(ancestor_err.kind() == io::ErrorKind::NotADirectory); + let ancestor_message = ancestor_err.to_string(); + anyhow::ensure!(ancestor_message.contains(ancestor_manifest_path.as_str())); + Ok(()) +} + #[rstest] fn creates_missing_parent_directory_and_manifest( temp_manifest_workspace: TempManifestWorkspace, @@ -139,6 +184,7 @@ fn raced_file_manifest_path_is_returned_unchanged( anyhow::ensure!(returned_path == expected_path, "manifest path should match"); let contents = fs::read(expected_path.as_std_path()).context("read competing manifest")?; anyhow::ensure!(contents == competing_contents, "manifest contents changed"); + assert_no_staged_manifest_files(&temp_path, &expected_path)?; Ok(()) } @@ -166,6 +212,7 @@ fn raced_directory_manifest_path_is_rejected( err.to_string().contains(expected_path.as_str()), "message: {err}" ); + assert_no_staged_manifest_files(&temp_path, &expected_path)?; Ok(()) } @@ -341,7 +388,10 @@ proptest! { prop_assert_eq!(contents, competing_contents); } TargetState::ExistingDirectory | TargetState::RacedDirectory => { - let error = result.expect_err("directory target should be rejected"); + prop_assert!(result.is_err(), "directory target should be rejected"); + let error = result + .err() + .ok_or_else(|| TestCaseError::fail("directory target should be rejected"))?; prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); prop_assert!(error.to_string().contains(expected_path.as_str())); } From aeacfc64032b56b63ff9c7e009dbc92dd7bf092b Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 02:22:15 +0200 Subject: [PATCH 8/8] Refactor manifest race handling (#61) Centralize raced-target classification and property-test error conversion without changing manifest creation or error-propagation behaviour. --- test_support/src/manifest.rs | 24 ++++++++++++++---------- test_support/src/manifest/tests.rs | 20 ++++++++++---------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index c0067737f..f78e3a351 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -77,6 +77,19 @@ fn manifest_path_is_directory_error(manifest_path: &Utf8Path) -> io::Error { ) } +fn handle_raced_manifest_target(manifest_path: &Utf8Path) -> io::Result<()> { + match inspect_manifest_target(manifest_path)? { + fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), + fs::PathState::NonDirectory => Ok(()), + fs::PathState::Absent => Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" + ), + )), + } +} + /// Install a deterministic action before a staged manifest is persisted. /// /// This test-only seam models controlled target creation after the initial @@ -152,16 +165,7 @@ fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::R match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => { - match inspect_manifest_target(manifest_path)? { - fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), - fs::PathState::NonDirectory => Ok(()), - fs::PathState::Absent => Err(io::Error::new( - io::ErrorKind::AlreadyExists, - format!( - "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" - ), - )), - } + handle_raced_manifest_target(manifest_path) } Err(e) => Err(io::Error::new( e.error.kind(), diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs index e70d3c178..523ab6925 100644 --- a/test_support/src/manifest/tests.rs +++ b/test_support/src/manifest/tests.rs @@ -302,15 +302,16 @@ fn target_state_strategy() -> impl Strategy { Just(TargetState::RacedDirectory), ] } - +fn test_case_error(error: &io::Error) -> TestCaseError { + TestCaseError::fail(error.to_string()) +} fn property_workspace() -> Result<(TempDir, Utf8PathBuf), TestCaseError> { - let temp = TempDir::new().map_err(|error| TestCaseError::fail(error.to_string()))?; + let temp = TempDir::new().map_err(|error| test_case_error(&error))?; let temp_path = Utf8Path::from_path(temp.path()) .ok_or_else(|| TestCaseError::fail("temporary path is not UTF-8"))? .to_owned(); Ok((temp, temp_path)) } - fn replace_staged_manifest( staged_file: NamedTempFile, manifest_path: &Utf8Path, @@ -327,7 +328,6 @@ fn replace_staged_manifest( replacement.write_all(contents)?; Ok(replacement) } - // This property checks controlled creation orderings through the injected hook. // It does not model arbitrary operating-system scheduling or filesystems. proptest! { @@ -349,9 +349,9 @@ proptest! { match state { TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents) - .map_err(|error| TestCaseError::fail(error.to_string()))?, + .map_err(|error| test_case_error(&error))?, TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path()) - .map_err(|error| TestCaseError::fail(error.to_string()))?, + .map_err(|error| test_case_error(&error))?, TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} } @@ -374,17 +374,17 @@ proptest! { match state { TargetState::Missing => { - let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; + let returned_path = result.map_err(|error| test_case_error(&error))?; prop_assert_eq!(&returned_path, &expected_path); let contents = fs::read(expected_path.as_std_path()) - .map_err(|error| TestCaseError::fail(error.to_string()))?; + .map_err(|error| test_case_error(&error))?; prop_assert_eq!(contents, expected_staged_contents); } TargetState::ExistingFile | TargetState::RacedFile => { - let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; + let returned_path = result.map_err(|error| test_case_error(&error))?; prop_assert_eq!(&returned_path, &expected_path); let contents = fs::read(expected_path.as_std_path()) - .map_err(|error| TestCaseError::fail(error.to_string()))?; + .map_err(|error| test_case_error(&error))?; prop_assert_eq!(contents, competing_contents); } TargetState::ExistingDirectory | TargetState::RacedDirectory => {