fix: write file cache entries atomically to avoid read races - #6011
Open
7487 wants to merge 1 commit into
Open
Conversation
FileCache.put opened the cache entry with "wb", truncating it in place.
A concurrent reader (e.g. another pytest-xdist worker sharing the cache
directory) could open the file between the truncate and the completed
write and fail to unpickle it ("Ran out of input"), causing flaky
Windows CI runs such as test_multi_repo_macro_references.
Write the gzip/pickle payload to a temp file in the cache directory and
os.replace it onto the entry path so readers only ever see complete
entries. The replace is best-effort: on Windows it can fail if a reader
still has the target open, in which case the existing entry is left
intact.
Fixes SQLMesh#6010
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: 7487 <1042653432@qq.com>
cmgoffena13
reviewed
Sep 2, 2026
| with os.fdopen(tmp_fd, "wb") as raw_fd: | ||
| with gzip.open(raw_fd, "wb", compresslevel=1) as fd: | ||
| pickle.dump(value, fd) | ||
| os.replace(tmp_name, self._cache_entry_path(name, entry_id)) |
Collaborator
There was a problem hiding this comment.
Your try wraps a lot here. I would keep the try/finally to unlink, but add in a try/except for the os.replace specifically, like this:
try:
with...
....
try:
os.replace
except OSError as ex:
warning...
finally:
try:
....
| pickle.dump(value, fd) | ||
| os.replace(tmp_name, self._cache_entry_path(name, entry_id)) | ||
| except OSError as ex: | ||
| # Storing an entry is best-effort; e.g. on Windows os.replace fails if a |
Collaborator
There was a problem hiding this comment.
I would just cut this down to: "Windows os.replace fails if a concurrent reader still has the target file open."
Collaborator
|
@7487 -- Would you please comment on the issue saying you're working on it. I can't assign an issue unless you've commented. Thanks! |
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.
Description
Fixes #6010
FileCache.putopened the cache entry with"wb", which truncates the file in place before the gzip/pickle write completes. A concurrent reader (e.g. another pytest-xdist worker sharing the cache directory) can open the file inside that window and hitpickle.loadEOF ("Ran out of input"), which is what intermittently broketests/lsp/test_reference_macro_find_all.py::test_multi_repo_macro_referenceson Windows CI.This change writes the payload to a temp file in the same cache directory and
os.replaces it onto the entry path, so readers only ever observe complete entries. Details:FileCache.__init__doesn't unlink it mid-write (fresh atime keeps it alive; orphaned temp files from a crashed process still get cleaned up by the atime threshold).os.replacefails with a sharing violation if a reader still has the target open. In that case we log a warning and keep the existing intact entry rather than raising (mirroring howgethandles unreadable entries).I didn't add the optional
getretry from the issue —getalready swallows unpickling errors andget_or_loadfalls back to the loader, and with atomic writes a partial read can no longer happen.Test Plan
test_file_cache_put_is_atomic, which verifies that a failedos.replaceleaves the existing entry untouched and cleans up the temp file.pytest tests/utils/test_cache.pypasses.getagainst a writer loopingputon the same entry: onmainthis produced ~2900 failed reads (partial file -> unpickle EOF); with this change, 0.Checklist
make styleand fixed any issues (ruff check, ruff format and mypy are clean on the touched files)tests/utils; remainingfast-testfailures in my env are missing optional engine deps, unrelated to this change)git commit -s) per the DCO