fix: write file cache entries atomically to avoid read races - #6011
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>
| 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)) |
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:
....
There was a problem hiding this comment.
Done in 21588c5 — kept the try/finally for the temp-file unlink and moved the except OSError to wrap just the os.replace. One side effect worth noting: a failure while writing the temp file (e.g. disk full) now propagates instead of being logged, which I think is the right behavior anyway since nothing was stored.
| 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 |
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."
|
@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! |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: 7487 <1042653432@qq.com>
2 participants
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