From f5ffe498837292aeab69a07dd2bfe84d14823e4b Mon Sep 17 00:00:00 2001 From: Steven Troxler Date: Thu, 15 Oct 2026 11:52:38 -0700 Subject: [PATCH] Replace the baseline with a rename that a concurrent reader cannot block Summary: `test_baseline_prune_ignores_diagnostics_below_threshold` fails intermittently on the Windows GitHub CI job (e.g. https://github.com/facebook/pyrefly/actions/runs/38726642921/job/116394023723). The server computes the prune, then logs `while writing baseline to ...: failed to persist temporary file: Access is denied. (os error 5)`, and the test times out waiting for the rewrite. `write_baseline_file` renames a temporary file over the baseline with `NamedTempFile::persist`, which on Windows calls `MoveFileExW(MOVEFILE_REPLACE_EXISTING)`. That fails with `ERROR_ACCESS_DENIED` whenever another handle has the baseline open at that moment, such as the server's own read of the baseline while it computes diagnostics, or the test polling the file. Windows users can lose a prune the same way. The baseline is now replaced with `std::fs::rename`. When `MoveFileExW` fails with `ERROR_ACCESS_DENIED`, the standard library retries the rename with `FILE_RENAME_FLAG_POSIX_SEMANTICS`, which replaces a file that others have open as long as their handles allow deletion; every handle the standard library opens does. Linux and macOS are unaffected, since a rename there never depends on open handles. On Windows, `NamedTempFile::new_in` marks the file it creates as temporary and only `persist` clears the mark, so the temporary file is created with `File::create_new` through `tempfile::Builder::make_in`, which keeps the cleanup on failure. On Unix that file is no longer private to its owner, so a new baseline gets the usual mode for a new file (the umask) instead of a fixed `0o644`. A rewrite still copies the permissions of the file it replaces. Differential Revision: D125499135 --- pyrefly/lib/error/baseline.rs | 66 ++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/pyrefly/lib/error/baseline.rs b/pyrefly/lib/error/baseline.rs index c897497ef1..2a9fb80b66 100644 --- a/pyrefly/lib/error/baseline.rs +++ b/pyrefly/lib/error/baseline.rs @@ -7,10 +7,10 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::fs; +use std::fs::File; use std::io::ErrorKind; use std::io::Write; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; use std::path::Path; use anyhow::Context; @@ -22,7 +22,7 @@ use ruff_text_size::Ranged; use similar::Algorithm; use similar::DiffOp; use similar::capture_diff_slices; -use tempfile::NamedTempFile; +use tempfile::Builder; use crate::config::config::BaselineMatchingMode; use crate::error::error::Error; @@ -64,11 +64,11 @@ pub fn write_baseline_file( ) -> Result { fn f(path: &Path, errors: &BaselineErrors, expected: Option<&str>) -> Result { let serialized = serde_json::to_vec_pretty(errors)?; - // A temporary file is created private to its owner, and the rename carries those - // bits onto the baseline. The baseline is checked in, so restore whatever the file - // being replaced had, or the usual mode when writing a new one. Metadata that is - // unavailable for any other reason stops the write, because carrying on would - // silently widen the permissions of a file whose own mode could not be read. + // The rename carries the temporary file's permissions onto the baseline. The baseline + // is checked in, so restore whatever the file being replaced had; a new baseline keeps + // the usual mode of a newly created file. Metadata that is unavailable for any other + // reason stops the write, because carrying on would silently widen the permissions of + // a file whose own mode could not be read. let replaced_permissions = match path.metadata() { Ok(metadata) => Some(metadata.permissions()), Err(e) if e.kind() == ErrorKind::NotFound => None, @@ -77,17 +77,14 @@ pub fn write_baseline_file( } }; // The temporary file goes in the baseline's own directory so that the rename - // stays within one filesystem, which is what makes it atomic. + // stays within one filesystem, which is what makes it atomic. It is created with + // `File::create_new` rather than `NamedTempFile::new_in`, which on Windows marks the + // file as temporary and clears the mark only when `persist` renames it. let directory = path.parent().unwrap_or_else(|| Path::new(".")); - let mut temporary = NamedTempFile::new_in(directory)?; + let mut temporary = Builder::new().make_in(directory, |path| File::create_new(path))?; temporary.write_all(&serialized)?; if let Some(permissions) = replaced_permissions { temporary.as_file().set_permissions(permissions)?; - } else { - #[cfg(unix)] - temporary - .as_file() - .set_permissions(std::fs::Permissions::from_mode(0o644))?; } temporary.as_file().sync_all()?; // Checked as late as possible, so that as little as possible can happen between @@ -99,7 +96,14 @@ pub fn write_baseline_file( { return Ok(BaselineWrite::Superseded); } - temporary.persist(path)?; + // `persist` would replace the baseline with `MoveFileExW`, which fails on Windows while + // another handle has the baseline open, as a concurrent read of it does. `fs::rename` + // retries that failure with POSIX rename semantics, which replace the file anyway as + // long as its other handles allow deletion, as every handle the standard library opens + // does. + fs::rename(temporary.path(), path)?; + // The temporary file is the baseline now, so there is nothing left to delete. + temporary.disable_cleanup(true); Ok(BaselineWrite::Written) } f(path, errors, expected) @@ -623,9 +627,29 @@ mod tests { assert_eq!(left_behind, vec![std::ffi::OsString::from("baseline.json")]); } - /// The baseline is checked in, so a rewrite must not narrow its permissions. The - /// temporary file the rewrite renames into place is created private to the user, and - /// those are the bits that would survive the rename if they were not corrected. + /// A rewrite replaces a baseline that a concurrent reader has open. Windows refuses to + /// rename a file over one that is open unless the rename uses POSIX semantics. + #[test] + fn test_write_baseline_file_replaces_a_baseline_held_open() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("baseline.json"); + std::fs::write(&path, "{ superseded }").unwrap(); + let _reader = File::open(&path).unwrap(); + + let baseline = BaselineErrors { + min_severity: Some(Severity::Warn), + errors: Vec::new(), + }; + write_baseline_file(&path, &baseline, None).unwrap(); + + let written: BaselineErrors = + serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(written.min_severity, Some(Severity::Warn)); + } + + /// The baseline is checked in, so a rewrite must keep its permissions rather than give it + /// those of the temporary file renamed into place. Group write access shows the + /// difference, because the usual umask of `022` removes it from a newly created file. #[cfg(unix)] #[test] fn test_write_baseline_file_preserves_permissions() { @@ -634,7 +658,7 @@ mod tests { let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("baseline.json"); std::fs::write(&path, "{}").unwrap(); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o664)).unwrap(); write_baseline_file( &path, @@ -647,7 +671,7 @@ mod tests { .unwrap(); let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o644, "rewrite must keep the baseline readable"); + assert_eq!(mode, 0o664, "rewrite must keep the baseline's permissions"); } /// A guarded rewrite goes through while the baseline is still the one it was computed