Skip to content

fix: write file cache entries atomically to avoid read races - #6011

Open
7487 wants to merge 1 commit into
SQLMesh:mainfrom
7487:fix/atomic-file-cache-put
Open

fix: write file cache entries atomically to avoid read races#6011
7487 wants to merge 1 commit into
SQLMesh:mainfrom
7487:fix/atomic-file-cache-put

Conversation

@7487

@7487 7487 commented Sep 2, 2026

Copy link
Copy Markdown

Description

Fixes #6010

FileCache.put opened 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 hit pickle.load EOF ("Ran out of input"), which is what intermittently broke tests/lsp/test_reference_macro_find_all.py::test_multi_repo_macro_references on 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:

  • The temp file is prefixed with the cache version so the stale-file cleanup in 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).
  • The store is best-effort: on Windows os.replace fails 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 how get handles unreadable entries).

I didn't add the optional get retry from the issue — get already swallows unpickling errors and get_or_load falls back to the loader, and with atomic writes a partial read can no longer happen.

Test Plan

  • Added test_file_cache_put_is_atomic, which verifies that a failed os.replace leaves the existing entry untouched and cleans up the temp file.
  • pytest tests/utils/test_cache.py passes.
  • Stress-checked with 2 readers looping get against a writer looping put on the same entry: on main this produced ~2900 failed reads (partial file -> unpickle EOF); with this change, 0.

Checklist

  • I have run make style and fixed any issues (ruff check, ruff format and mypy are clean on the touched files)
  • I have added tests for my changes (if applicable)
  • All existing tests pass (ran tests/utils; remaining fast-test failures in my env are missing optional engine deps, unrelated to this change)
  • My commits are signed off (git commit -s) per the DCO

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>
Comment thread sqlmesh/utils/cache.py
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))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:
    ....

Comment thread sqlmesh/utils/cache.py
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would just cut this down to: "Windows os.replace fails if a concurrent reader still has the target file open."

@cmgoffena13

Copy link
Copy Markdown
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!

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.

CI Fix - Windows Tests Race Condition

2 participants