Make whole-file cache writes atomic by lhoupert · Pull Request #2075 · fsspec/filesystem_spec

lhoupert · 2026-07-15T10:58:46Z

Addresses the whole-file-cache part of #639

Whole-file caches (simplecache, filecache) download cache misses directly to the final cache filename, and readers treat "file exists" as "file is ready", so a concurrent reader of a key whose download is still in flight can return a partially-written file.

This implements the fix @martindurant proposed in #639 ("replace get(remote, local) with get(remote, local.temp); mv(local.temp, local)"): every whole-file download now goes to a .part temp file in the cache directory followed by an atomic os.replace() .

A first commit fixes cold-cache cat_ranges/_cat_ranges bugs in the same methods it touches: is False/is None checks that never match what _check_file actually returns (so uncached files were never downloaded), repeated paths within one call resolving no local path, and on_error being forwarded into the target filesystem's _get_file (aiohttp rejects it with a TypeError).

For reviewers: the regression tests stagger reader starts across a throttled download: simultaneous starts would all miss the cache and write identical bytes to identical offsets, hiding the race. On unpatched master they reliably fail with truncated reads (e.g. 262144/1048576 bytes); they add ~3 s of runtime and passed 5/5 repeated local runs, with no regressions in the full suite. Scope is deliberately limited to the whole-file caches; blockcache/CachingFileSystem's sparse-mmap path has a related issue but needs a deeper redesign.

Author attestation

I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

Implementation and tests were AI-assisted, adapted from our production mitigation in EOPF-Explorer/data-model#220 where Claude helped me root-caused the race; I reviewed and verified every line of this diff.

SimpleCacheFileSystem.cat_ranges compared _check_file() results with "is False", but its _check_file returns None for missing entries, so uncached files were never downloaded and the subsequent local read failed. WholeFileCacheFileSystem._cat_ranges had the inverse problem ("is None" while the metadata-backed _check_file returns False), never resolved a local path for repeats of one path within a single call, and forwarded on_error into self.fs._get, which passes it on to the target filesystem's _get_file (aiohttp, for one, rejects it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The whole-file caching filesystems (filecache, simplecache) downloaded cache misses directly to the final cache filename, while readers treat the existence of that filename as "download complete". A concurrent reader of the same key could therefore observe - and return - a partially-written cache file (issue fsspec#639). All whole-file download paths (_open via _get_cached_file_before_open including its compression branch, cat, open_many, cat_ranges, _cat_file and _cat_ranges) now download to a temporary file created next to the final name and os.replace() it into place, so an incomplete download is never visible under the final filename. A lost race means the same bytes are fetched twice and the last rename wins; no locking is needed. Temporary files are removed on failure. The regression tests stagger reader starts across the duration of a throttled download: simultaneous starts would all miss the cache and write identical bytes to identical offsets, hiding the race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…loads Review feedback on fsspec#2075: make explicit in the code that the temp filename already contains a random token and is created with O_EXCL, so concurrent downloads of the same key cannot clash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Sanjays2402 · 2026-07-19T06:02:49Z

        if not fn:
            fn = os.path.join(self.storage[-1], sha)
-            await self.fs._get_file(path, fn, **kwargs)
+            await _atomic_get_file_async(self.fs, path, fn, **kwargs)


where does filecache record this download in CacheMetadata? _cat_file never calls _make_local_details or save_cache, so its second read is another cache miss and downloads the file again. The new test only checks returned bytes, so it doesn’t catch that.

Good catch @Sanjays2402 , thank you! You are right _cat_file never recorded the download, and my test only checked the returned bytes so it couldn't have caught it... I've tried to address it properly in the follow-up commits 5b94d78 and aeb6cc3:

misses in _cat_file/_cat_ranges now record the metadata (with an async-safe ukey lookup, since the sync ukey can't be called inside a running loop for asynchronous=True targets), followed by save_cache(), mirroring the sync cat() path (5b94d78)

The new tests count actual _get_file calls now: a second read, including from a fresh instance over the same cache_storage, should perform zero downloads. Happy to adjust if you'd prefer a different approach here.

Sanjays2402 · 2026-07-25T01:23:24Z

+def _temppath(lpath):
+    # mkstemp inserts a random token between prefix and suffix and creates the
+    # file with O_EXCL, so concurrent downloads of the same key never clash.
+    fd, tmp = tempfile.mkstemp(


mkstemp creates 0600 and os.replace keeps that mode, so cache files that used to land at 0644 (umask) are now owner-only. Anything sharing a cache_storage dir between users/processes running as different uids breaks after this. Worth a chmod to 0o666 & ~umask on the tmp file before the replace.

Only on POSIX, right?

Yeah, I think we should just use open and avoid the issue altogether (which means this function just generates a filename including a random component). The documentation should indicate that these files are not cleared up in case of early exit (except for an exception during replace).

Hi @Sanjays2402 and @martindurant , thank you for spotting this issue. It should be fixed in d9ffb53/9028ef9 where _temppath now only generates the name (..part), then the file is created by the plain open() of whatever downloads it, so mkstemp's fd and 0600 mode are gone.

I also added in the docs (features.rst and the module comment) that .part files are removed when the download or the final rename raises, but not on early exit, leftover .part files are ignored by the cache and safe to delete. I also added tests covering failed-download cleanup and that a stale .part file is never mistaken for a cache entry.

Sanjays2402 · 2026-07-25T15:30:06Z

+# visible under its final cache filename: readers treat the existence of that
+# filename as "download complete" (see issue #639). The rename is atomic, and
+# a concurrent duplicate download of the same path is harmless: both
+# temporary files hold identical bytes and the last rename wins. These


the "last rename wins" claim holds on posix but not on windows: os.replace onto an existing destination raises PermissionError if any other process has that file open, and that is exactly the state this design creates — reader A opens the cache file right after the first rename, downloader B for the same key then tries to replace it out from under A. pytest-win runs the new concurrency tests too, so this is reachable in CI. would it be enough to swallow OSError around the replace and just drop the temp file when the destination already exists (bytes are identical anyway)?

Thanks! I hadn't considered the Windows rename semantics at all... I went with your suggestion in 5b94d78, then refined it in aeb6cc3 with one change I'd welcome your view on: _replace_tempfile swallows the failed rename and drops the temp copy only when the destination holds identical bytes (filecmp), which I think covers the concurrent-duplicate case you describe.

Per review: mkstemp's fd handling and 0o600 mode are POSIX-specific choices, so _temppath now only generates a random name and the download's own open() creates the file. Document (module comment and features.rst) that .part files are removed when a download raises but can be left behind on abrupt process exit, and are safe to delete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… paths Follow-up from multi-agent verification of the review response: the docs now name the rename as well as the download as cleanup triggers (the clause the reviewer spelled out), the _temppath comment no longer misattributes mkstemp's drawbacks to portability, and two tests pin the newly documented guarantees (failing download leaves no .part nor final file and the next read succeeds; a stale .part file is never mistaken for a cache entry). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…Windows Two review comments from Sanjays2402: - filecache's _cat_file/_cat_ranges downloaded cache misses without recording them in CacheMetadata, so every async read of the same key was a fresh miss and downloaded the file again; _cat_file also passed the (detail, fn) tuple a metadata hit returns straight to open(). Misses now go through _make_local_details_async (async-safe uid lookup) followed by save_cache(), mirroring the sync cat() path; simplecache overrides it to keep its no-metadata behavior. - "last rename wins" does not hold on Windows, where os.replace onto a destination another process holds open raises PermissionError. The new _replace_tempfile discards the redundant temp copy instead when the destination already exists (identical bytes by construction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…stale replace Follow-up to the multi-agent review of the Sanjays2402 fixes: - _cat_ranges now strips protocols before metadata lookup/recording, so the downloads-once fix works for non-identity _strip_protocol targets (s3-style paths) and no longer persists unstripped junk keys. - The miss-path metadata/ukey lookup moved inside the gathered download coroutine, restoring on_error="return" semantics for missing paths. - New _check_file_async performs the check_files uid comparison and expiry check with the async-safe ukey (the sync comparison crashed for asynchronous=True targets once entries existed), and treats partial block-cache entries as misses instead of serving sparse bytes. - _replace_tempfile only discards the temp copy when the busy destination holds identical bytes; a differing destination (cache refresh racing a reader) raises instead of passing stale bytes off as fresh. - compression= set skips async metadata recording so the sync open() path can still self-heal with a decompressed download. - New tests cover non-identity strip + save_cache persistence (fresh instance, zero downloads), check_files round-trip, on_error, and the differing-bytes replace branch; all verified failing pre-fix by the review agents' repros. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Behavior-preserving cleanups from the simplification review: a single _tempfile context manager replaces the temp/replace/cleanup dance repeated across the four _atomic_* helpers; _make_local_details_async delegates to _make_local_details via an optional uid argument; _check_file_async returns only the local filename (neither caller used the detail); one _fetch_file_async helper carries the compression/ metadata guard for both _cat_file and _cat_ranges; tests reuse one fs-constructor helper and the leave-no-tempfiles assertion folded into the concurrency test, where it now covers both protocols and all trials. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

martindurant · 2026-07-28T13:33:35Z

+        # redundant and safe to discard; a differing destination (e.g. a
+        # cache refresh racing a reader of the stale copy) must surface
+        # the error rather than let stale bytes pass for fresh ones.
+        if not (os.path.exists(lpath) and filecmp.cmp(tmp, lpath, shallow=False)):


If I understand, this will always re-read the whole contents of both files (os.stat will never be identical), which could be a bad performance hit. Might be enough to look at the creation time of the two files? Is there any way to know if the target file is done (would that mean it gets closed and there is no error?) ?

Good point! What do you think about the last commit I pushed to address this 2b9ec83? There is no content reads any more. The temp file is pre-created empty so the download start is stamped on the cache filesystem's own clock, and on a failed rename stat checks decide: the temp copy is dropped only if the destination is a regular file of the same size with mtime at or after that start (written while this download ran, so a complete recent download of the same path). Claude suggested to use mtime rather than creation time because st_birthtime is not available on all platforms.

Is there any way to know if the target file is done (would that mean it gets closed and there is no error?) ?

Something else that the fact that a temporary file has been renamed with its final name? (the rename runs only after the download completes:

filesystem_spec/fsspec/implementations/cached.py

Lines 103 to 116 in 2b9ec83

@contextmanager

def _tempfile(lpath):

# yield a temp name to download into; rename it to lpath on success,

# remove it on error. The temp file is pre-created empty (with a plain

# open(), keeping umask-derived permissions) so the download start is

# stamped on the cache filesystem's own clock, comparable with the

# destination's mtime in _replace_tempfile.

tmp = _temppath(lpath)

try:

with open(tmp, "wb"):

pass

start = os.stat(tmp).st_mtime

yield tmp

_replace_tempfile(tmp, lpath, start)

Per review: filecmp re-read both files in full on every swallowed rename failure. Only complete downloads are ever renamed onto the final cache name, so stat checks suffice: the temp copy is discarded only when the busy destination is a regular file of the same size whose mtime is at or after the download start. The start stamp is the pre-created temp file's own mtime, so both timestamps come from the cache filesystem's clock (no wall-clock/filesystem skew), and only PermissionError is swallowed. An older destination (a cache refresh racing a reader of the stale copy), a size mismatch, or a non-file still raises rather than passing stale bytes off as fresh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lhoupert and others added 2 commits July 15, 2026 11:47

lhoupert changed the title ~~Atomic cache writes~~ Make whole-file cache writes atomic Jul 15, 2026

lhoupert mentioned this pull request Jul 15, 2026

race conditions in fsspec cache? #639

Open

martindurant reviewed Jul 15, 2026

View reviewed changes

Comment thread fsspec/implementations/cached.py Outdated

Sanjays2402 reviewed Jul 19, 2026

View reviewed changes

Sanjays2402 reviewed Jul 25, 2026

View reviewed changes

lhoupert and others added 5 commits July 28, 2026 11:48

martindurant reviewed Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make whole-file cache writes atomic - #2075

Make whole-file cache writes atomic#2075
lhoupert wants to merge 9 commits into
fsspec:masterfrom
lhoupert:atomic-cache-writes

lhoupert commented Jul 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Sanjays2402 Jul 19, 2026

Uh oh!

lhoupert Jul 28, 2026

Uh oh!

Sanjays2402 Jul 25, 2026

Uh oh!

martindurant Jul 25, 2026

Uh oh!

martindurant Jul 27, 2026

Uh oh!

lhoupert Jul 28, 2026

Uh oh!

Sanjays2402 Jul 25, 2026

Uh oh!

lhoupert Jul 28, 2026

Uh oh!

martindurant Jul 28, 2026

Uh oh!

lhoupert Jul 28, 2026

Uh oh!

lhoupert Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	@contextmanager
	def _tempfile(lpath):
	# yield a temp name to download into; rename it to lpath on success,
	# remove it on error. The temp file is pre-created empty (with a plain
	# open(), keeping umask-derived permissions) so the download start is
	# stamped on the cache filesystem's own clock, comparable with the
	# destination's mtime in _replace_tempfile.
	tmp = _temppath(lpath)
	try:
	with open(tmp, "wb"):
	pass
	start = os.stat(tmp).st_mtime
	yield tmp
	_replace_tempfile(tmp, lpath, start)

Uh oh!

Conversation

lhoupert commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Author attestation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhoupert commented Jul 15, 2026 •

edited

Loading