Skip to content

fix(cache): restore cached outputs atomically so concurrent cache hits don't fail - #866

Open
tlq5l wants to merge 1 commit into
voidzero-dev:mainfrom
tlq5l:fix/cache-concurrent-restore
Open

tlq5l wants to merge 1 commit into
voidzero-dev:mainfrom
tlq5l:fix/cache-concurrent-restore

Conversation

@tlq5l

@tlq5l tlq5l commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #846

Motivation

When two vp run processes hit the same cache entry at the same time, restoring the outputs can fail with File exists (os error 17). tar's unpack removes an existing file and then recreates it with create_new, so another process can create the file between the two steps. The same remove-then-create window also lets a reader see a missing or half-written file.

extract_output_archive now writes each regular file to a temporary file in the target's directory and renames it over the target, with no lock. The rename can't fail with File exists, the last writer wins with the same bytes, and a reader never sees a missing or partly written file. A failed extraction leaves no temporary file behind. Entries that are not regular files, which create_output_archive never writes, still go through Entry::unpack_in.

The temporary file-and-rename approach was suggested by @everton-dgn in #846, who also asked for the three points below.

  • Concurrent readers. A new test reads every restored file in a loop on two threads while two other threads restore the archive 30 times each, and fails on any missing or partial file. A second test runs two concurrent restores for 50 rounds. Both fail on main (File exists (os error 17), and partial file).
  • Windows. On Windows, std::fs::rename first calls MoveFileExW, and when that fails with access denied (for example, because a reader has the target open or another process is replacing it), it retries with a POSIX-semantics rename (FileRenameInfoEx). That replaces a target a reader opened with FILE_SHARE_DELETE, which std passes for every file it opens. tempfile's persist calls only MoveFileExW, with no retry, so the final step uses std::fs::rename. Not handled: a process that opened the target without FILE_SHARE_DELETE, and Windows versions or file systems without POSIX-semantics rename; there the rename can still fail with access denied. I have no Windows machine: the code and tests are written to run on Windows (the symlink test uses symlink_dir there; the permission-bit test is Unix-only because Windows has no mode bits), but Windows is covered only by CI.
  • Path checks and metadata. Entry paths must be plain relative components; .., ., absolute paths and prefixes are rejected. As in Entry::unpack_in, entries that resolve outside the workspace through a symlinked directory are rejected, and that is checked before any missing directory is created. This is stricter than tar, which silently skips .. entries and extracts absolute paths inside the destination with the leading / removed; create_output_archive never writes such paths, so this only affects a tampered archive. Tests cover ../, dist/../../, an absolute path and the symlink case, including a missing directory below the symlink. Executable and other permission bits and the modification time are restored as before, and tests check both, including when replacing an existing file. Those tests also pass on main, so behavior is unchanged.

…s don't fail

Fixes voidzero-dev#846

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@tlq5l
tlq5l force-pushed the fix/cache-concurrent-restore branch from c66fd7f to 07327e7 Compare October 19, 2026 22:27

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent cache hits for the same task fail to restore outputs with "File exists (os error 17)"

1 participant