Skip to content

Replace the baseline with a rename that a concurrent reader cannot block - #5245

Open
stroxler wants to merge 1 commit into
mainfrom
export-D125499135
Open

stroxler wants to merge 1 commit into
mainfrom
export-D125499135

Conversation

@stroxler

Copy link
Copy Markdown
Contributor

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

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
@meta-cla meta-cla Bot added the cla signed label Oct 15, 2026
@meta-codesync

meta-codesync Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

@stroxler has exported this pull request. If you are a Meta employee, you can view the originating Diff in D125499135.

@github-actions

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@rchen152 rchen152 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants