Repository navigation
Conversation
…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
force-pushed
the
fix/cache-concurrent-restore
branch
from
October 19, 2026 22:27
c66fd7f to
07327e7
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #846
Motivation
When two
vp runprocesses hit the same cache entry at the same time, restoring the outputs can fail withFile exists (os error 17).tar'sunpackremoves an existing file and then recreates it withcreate_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_archivenow 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 withFile 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, whichcreate_output_archivenever writes, still go throughEntry::unpack_in.The temporary file-and-rename approach was suggested by @everton-dgn in #846, who also asked for the three points below.
main(File exists (os error 17), andpartial file).std::fs::renamefirst callsMoveFileExW, 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 withFILE_SHARE_DELETE, which std passes for every file it opens.tempfile'spersistcalls onlyMoveFileExW, with no retry, so the final step usesstd::fs::rename. Not handled: a process that opened the target withoutFILE_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 usessymlink_dirthere; the permission-bit test is Unix-only because Windows has no mode bits), but Windows is covered only by CI...,., absolute paths and prefixes are rejected. As inEntry::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 thantar, which silently skips..entries and extracts absolute paths inside the destination with the leading/removed;create_output_archivenever 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 onmain, so behavior is unchanged.