Stop the hash cache returning a hash matching no version of a file#95
Merged
Conversation
Three related defects in the (size, mtime, inode) cache. File metadata was read before hashing and stored alongside the result afterwards. A file modified while it was being hashed produced an entry whose hash belonged to neither the old nor the new content, and which would be served again whenever those metadata recurred. The file is now re-stat'd after hashing and the entry dropped if anything moved. Filesystem mtime is frequently 1-second granular, so a file modified within the same tick as it was hashed cannot be distinguished from one that was not. Such entries are marked racy and revalidated rather than trusted on metadata alone. They are still written: refusing to cache them outright would make the cache useless for the common case of tracking files immediately after writing them, which was the first thing I tried and which broke five existing tests for exactly that reason. Recomputing once refreshes the entry with a timestamp comfortably after the mtime, and it is trusted from then on. This mirrors git's racily-clean handling. Both the manifest and the cache also wrote through a fixed temp filename. The rename is atomic but the content is not: two writers interleave into one temp file before either renames. Both now use unique temp names. Three of the six new tests fail on the parent commit. Test suite: 61 failed / 549 passed before, 61 failed / 555 passed after. Failure set identical; the 61 are the missing-"python"-binary failures fixed separately in #93. Fixes #90 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kmatzen
added a commit
that referenced
this pull request
Jul 19, 2026
Three related defects in the (size, mtime, inode) cache. File metadata was read before hashing and stored alongside the result afterwards. A file modified while it was being hashed produced an entry whose hash belonged to neither the old nor the new content, and which would be served again whenever those metadata recurred. The file is now re-stat'd after hashing and the entry dropped if anything moved. Filesystem mtime is frequently 1-second granular, so a file modified within the same tick as it was hashed cannot be distinguished from one that was not. Such entries are marked racy and revalidated rather than trusted on metadata alone. They are still written: refusing to cache them outright would make the cache useless for the common case of tracking files immediately after writing them, which was the first thing I tried and which broke five existing tests for exactly that reason. Recomputing once refreshes the entry with a timestamp comfortably after the mtime, and it is trusted from then on. This mirrors git's racily-clean handling. Both the manifest and the cache also wrote through a fixed temp filename. The rename is atomic but the content is not: two writers interleave into one temp file before either renames. Both now use unique temp names. Three of the six new tests fail on the parent commit. Test suite: 61 failed / 549 passed before, 61 failed / 555 passed after. Failure set identical; the 61 are the missing-"python"-binary failures fixed separately in #93. Fixes #90 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
kmatzen
added a commit
that referenced
this pull request
Jul 19, 2026
* Add TLA+ specs for manifest, GC, and chunked upload protocols
Three models of s3lfs's concurrency-sensitive protocols, checked with TLC
2.19. Each spec exposes candidate fixes as constants so alternatives can be
compared by model checking rather than by argument.
S3lfsManifest -- concurrent read-modify-write of .s3_manifest.yaml.
Re-reading under the lock and sharing one lock file are each necessary and
only jointly sufficient; fixing either alone leaves lost updates reachable.
The CWD-relative temp_dir at core.py:158 means the paths that do reload
correctly are not actually safe today.
S3lfsGC -- cleanup_s3 racing a concurrent upload. Re-validating the manifest
under the lock before deleting does not fix the race: the whole mark/sweep
cycle fits inside the uploader's upload-then-commit window. An in-flight
registry claimed before upload checks clean, but depends on the lock fix.
S3lfsChunks -- partial chunked upload. A trailing gap makes checkout report
success with a truncated file, since the count is inferred from
len(chunk_keys) and nothing verifies a hash. Storing the count or verifying
the hash converts corruption into a loud failure; only committing the
manifest after all chunks land keeps the bad state from being recorded.
Models are small (2 processes / 2 paths / 3 chunks) -- enough to exhibit the
counterexamples, not to establish correctness at scale. No production code
is changed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Anchor the manifest lock to the manifest, not the working directory
The portalocker lock guarding .s3_manifest.yaml was derived from temp_dir,
which defaults to the relative path ".s3lfs_temp". The manifest path itself
is resolved absolutely at the git root, so two processes started from
different working directories guarded one manifest with two different lock
files and did not exclude each other at all.
Derive the lock from the resolved manifest directory instead.
Verified with two processes holding the lock from different working
directories: before, their critical sections interleave
(enter 0, enter 1, exit 0, exit 1); after, they serialize. This is the
SHARED_LOCK precondition in specs/S3lfsManifest.tla -- with it false, even
the code paths that correctly reload under the lock still lose updates.
Keeping the lock under .s3lfs_temp/ rather than beside the manifest is an
organizational choice, not a correctness one: it keeps the repository root
clean and matches the existing .gitignore entries. Both locations are
equally visible to file enumeration, which uses rglob("*") with no
exclusion list (core.py:1878) and today also enumerates .git/** and the
manifest itself. That is a separate pre-existing defect.
Test suite: 89 failed / 503 passed before, 61 failed / 531 passed after,
run sequentially on the same machine. No new failures. The remaining 61
are pre-existing and environmental (the tests shell out to "python",
absent here). The 28 resolved are all
test_cli_integration.py::TestS3LFSCLIInProcess, which drive CLI commands
from varying working directories.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Add S3lfsNamespace: derive lock identity instead of assuming it
S3lfsManifest models mutual exclusion as an input (SHARED_LOCK \in BOOLEAN).
A spec that is told whether the lock works can only score fixes already
thought of -- it cannot evaluate where the lock should live, because
placement is precisely what the boolean abstracts away.
S3lfsNamespace resolves the lock from a (base, name) pair the way the code
does, so mutual exclusion follows from whether two processes land on the
same file. It also carries the directory tree, because s3lfs's metadata
lives in the tree s3lfs enumerates with rglob("*") (core.py:1878).
Two results the earlier specs could not express:
- manifest_root and manifest_temp are equivalent for NoLostUpdate. The
choice between them is organizational, not correctness. The lock fix was
originally justified by claiming .s3lfs_temp/ keeps the lock out of file
enumeration; that is false, and this spec contradicts it. The commit
message for that change has been corrected.
- NoInternalFileTracked fails for every lock policy when the tracked target
is the repository root, because the manifest itself is in the enumerated
subtree. Confirmed against the real code: enumerating from the repo root
returns .s3_manifest.yaml, the lock file, and all of .git/**. This is a
standing defect, unrelated to locking, and is not yet fixed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Exclude git and s3lfs internals from filesystem enumeration
_resolve_filesystem_paths enumerates with rglob("*"), which matches
dotfiles and has no exclusion list. Tracking the repository root therefore
walked into .git/ and also collected s3lfs's own bookkeeping.
Measured before this change, on a fresh repo holding two user files:
`track .` processed 22 files and reported "Successfully processed 22 files"
with no warning. The manifest gained 18 .git/** entries plus
.s3_manifest.yaml and .s3lfs_temp/.s3lfs.lock. On a real repository that
means .git/objects/** is uploaded to the bucket -- the full history,
including content deleted in later commits. After the change the same
repository yields exactly the two user files.
Skip anything under .git/ or the s3lfs temp directory, plus the manifest,
the hash cache, and any *.s3lfs.lock. The check is made relative to the
git root so that a repository stored beneath an unrelated directory named
.git is not misclassified.
This is the NoInternalFileTracked invariant in specs/S3lfsNamespace.tla,
which fails for every lock placement when the tracked target is the
repository root, because the manifest itself sits in the enumerated
subtree. It is a standing defect, independent of the lock fix that
prompted the spec.
Test suite: 61 failed / 531 passed before, 61 failed / 535 passed after,
run sequentially on the same machine. Failure set byte-identical; the
four additional passes are the new regression tests. The 61 remain
pre-existing and environmental (tests shell out to "python", absent here).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Re-read the manifest under the lock before saving
An S3LFS instance loads the manifest once at construction and mutates that
copy for the life of the process. Writers that saved without re-reading
wrote their stale snapshot back over anything another process had
committed, dropping entries whose S3 objects then became unreferenced --
and which a later cleanup_s3 would delete.
remove_file and remove_subtree now reload under the lock before mutating.
remove_subtree additionally did its matching under one lock acquisition,
popped entries outside the lock entirely, and saved under a second
acquisition; matching, mutation, and save now share one critical section,
with S3 deletion moved out of it since it is network I/O.
track_modified_files and track_modified_files_cached each ended with a
redundant save_manifest(). parallel_upload_chunked already commits the
manifest correctly, reloading under the lock and merging only the keys it
uploaded, so the trailing save could only overwrite a concurrent commit
that landed in between. Both are removed rather than given a reload.
Measured against the previous commit: a process holding a manifest
snapshot, a second process committing new.bin, then the first removing an
unrelated old.bin left the manifest empty -- new.bin erased. It now
survives. This is the NoLostUpdate counterexample in
specs/S3lfsManifest.tla, whose Reload_Lock configuration is the one that
checks clean; with the lock path already fixed, that configuration now
describes the code.
The new tests fail on the parent commit (2 of 3) and pass here.
Test suite: 61 failed / 535 passed before, 61 failed / 538 passed after,
run sequentially on the same machine. Failure set byte-identical; the
three additional passes are the new tests. The 61 remain pre-existing and
environmental (tests shell out to "python", absent here).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Correct the test-suite claims in this branch; verify lock reentrancy
Two verification results, one of which invalidates earlier commit messages.
The lock-path fix (cd925ce) claimed to resolve 28 cli_integration
failures. That is false. The baseline it was measured against had been
extracted with `git archive`, which omits .git, and those 28 tests require
a git repository -- they were failing for that reason alone. Re-measured
with .git present in both trees, 385fe1b gives 61 failed / 531 passed and
1c355de gives 61 failed / 538 passed. The delta is exactly the seven tests
added with the fixes, so all three code fixes have zero test-suite delta.
Each remains verified by its own probe or regression test; only the claim
of suite movement was wrong.
Separately, _lock_context is not reentrant, and re-reading the manifest
under the lock widened remove_subtree's critical section. Rather than
model this -- reentrancy is a property of the call graph, so a spec would
only restate the assumptions used to write it -- the lock was instrumented
with a per-thread depth counter and the suite run against it. Zero
reentrant acquisitions, with the instrumented run reproducing the
uninstrumented counts exactly, and no hang, which also rules out
cross-instance nesting. Exercised paths only; the 61 environmental
failures leave their paths unverified.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Record a chunked upload only once every chunk has landed; verify on checkout
parallel_upload_chunked wrote the manifest entry at prep time, before any
chunk was uploaded, and per-chunk failures were caught and skipped. The
finally block then committed the entry regardless, so a partial upload
produced a manifest entry pointing at an incomplete object.
That turns into silent corruption at checkout, because
_discover_chunks_for_file infers the chunk count from the objects that
happen to exist and reads indices 0..n-1. A file missing its tail
reassembles into a shorter but otherwise well-formed file, and nothing
downstream noticed: _finalize_file did not verify anything. An interior
gap fails loudly on a 404; a trailing gap did not fail at all.
A file now earns its manifest entry only after all of its chunks upload,
and incomplete files are reported rather than silently dropped. Interrupted
uploads benefit too: previously SIGINT could leave a committed entry for a
file whose upload never finished.
_finalize_file additionally verifies the reassembled file against the
manifest hash, removing the output and raising on mismatch. This is defence
in depth for chunks lost after a correct commit -- lifecycle expiry,
out-of-band deletion, or the GC race modelled in specs/S3lfsGC.tla. The
download loop now catches per-file failures so one bad file does not
abandon the rest, and files whose chunks never all arrive are reported and
their partial chunks removed instead of leaking into .s3lfs_temp.
Measured on a 4-chunk file whose trailing 3 chunks fail: before, the
manifest recorded big.bin and the run printed "Uploaded 1 file(s)"; after,
the manifest is empty and the run warns "big.bin (1/4 chunks)".
This is the CommitAndVerify configuration in specs/S3lfsChunks.tla, the
only one satisfying both NoSilentCorruption and ManifestImpliesChunks.
Two existing tests in test_block_parallel.py used placeholder strings
("hash1", "h1") as manifest hashes, which cannot match real content. They
now carry real digests. Note hash_file is content-only despite a docstring
claiming it also covers the relative path.
Test suite: 61 failed / 538 passed before, 61 failed / 542 passed after.
Failure set identical; the four additional passes are the new tests, three
of which fail on the parent commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Fix mypy findings and run CI for stacked pull requests
Two mypy errors in the chunk work: the incomplete-file reporter bound its
loop variable to `e`, which Python deletes at the end of the enclosing
except block, and the new test's mock store needed an annotation.
Also widen the pull_request trigger to any base branch. CI previously ran
only for PRs targeting main, so a PR based on another branch got no checks
at all.
flake8 fails in some local environments with
AttributeError("module 'ast' has no attribute 'Str'"); that is pyflakes
6.0.0 against Python 3.14 and reproduces identically on the base commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Protect in-flight uploads from garbage collection
cleanup_s3 was mark-and-sweep with the manifest lock released between mark
and sweep and an unbounded input() confirmation in the middle. An uploader
PUTs chunks before publishing its manifest entry, so an object can exist in
S3 while nothing references it; a sweep landing in that window deleted it
and left the manifest pointing at a missing object.
Re-validating the manifest under the lock before deleting does not fix
this, and specs/S3lfsGC.tla rejects it: the whole mark/sweep cycle fits
inside the uploader's upload-then-commit window, during which the manifest
genuinely does not reference the object.
Uploaders now claim each hash in a registry before any of its bytes reach
S3, and release the claim only after the manifest is written. GC unions the
registry into its live set at mark time and re-checks under the lock
immediately before deleting. This is the INFLIGHT configuration in
specs/S3lfsGC.tla, which checks clean over the full state space.
The release ordering carries the argument. Releasing before the manifest
write would reopen the exact window the registry closes.
A crashed uploader leaves claims behind, so claims age out after 24h. That
timeout bounds the leak from a crash and plays no part in closing the race
-- deliberately narrower than an object-age grace period, where the timeout
would be the whole correctness argument.
Asset key parsing also moves off an unanchored str.replace, which corrupted
the parse when the repo prefix appeared again inside a file path (part of
#91).
Measured: with the uploader held between its chunk PUT and its manifest
commit, a concurrent full cleanup_s3 previously deleted the object and left
a dangling manifest reference. It now survives. Six of the seven new tests
fail on the parent commit.
Test suite: 61 failed / 542 passed before, 61 failed / 549 passed after.
Failure set identical; the seven additional passes are the new tests. The
61 are the missing-"python"-binary failures fixed separately in #93.
Fixes #85
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Stop the hash cache returning a hash matching no version of a file (#95)
Three related defects in the (size, mtime, inode) cache.
File metadata was read before hashing and stored alongside the result
afterwards. A file modified while it was being hashed produced an entry
whose hash belonged to neither the old nor the new content, and which would
be served again whenever those metadata recurred. The file is now re-stat'd
after hashing and the entry dropped if anything moved.
Filesystem mtime is frequently 1-second granular, so a file modified within
the same tick as it was hashed cannot be distinguished from one that was
not. Such entries are marked racy and revalidated rather than trusted on
metadata alone. They are still written: refusing to cache them outright
would make the cache useless for the common case of tracking files
immediately after writing them, which was the first thing I tried and which
broke five existing tests for exactly that reason. Recomputing once
refreshes the entry with a timestamp comfortably after the mtime, and it is
trusted from then on. This mirrors git's racily-clean handling.
Both the manifest and the cache also wrote through a fixed temp filename.
The rename is atomic but the content is not: two writers interleave into
one temp file before either renames. Both now use unique temp names.
Three of the six new tests fail on the parent commit.
Test suite: 61 failed / 549 passed before, 61 failed / 555 passed after.
Failure set identical; the 61 are the missing-"python"-binary failures
fixed separately in #93.
Fixes #90
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* Fix path normalization in remove, off-thread construction, and a docstring (#96)
* Stop the hash cache returning a hash matching no version of a file
Three related defects in the (size, mtime, inode) cache.
File metadata was read before hashing and stored alongside the result
afterwards. A file modified while it was being hashed produced an entry
whose hash belonged to neither the old nor the new content, and which would
be served again whenever those metadata recurred. The file is now re-stat'd
after hashing and the entry dropped if anything moved.
Filesystem mtime is frequently 1-second granular, so a file modified within
the same tick as it was hashed cannot be distinguished from one that was
not. Such entries are marked racy and revalidated rather than trusted on
metadata alone. They are still written: refusing to cache them outright
would make the cache useless for the common case of tracking files
immediately after writing them, which was the first thing I tried and which
broke five existing tests for exactly that reason. Recomputing once
refreshes the entry with a timestamp comfortably after the mtime, and it is
trusted from then on. This mirrors git's racily-clean handling.
Both the manifest and the cache also wrote through a fixed temp filename.
The rename is atomic but the content is not: two writers interleave into
one temp file before either renames. Both now use unique temp names.
Three of the six new tests fail on the parent commit.
Test suite: 61 failed / 549 passed before, 61 failed / 555 passed after.
Failure set identical; the 61 are the missing-"python"-binary failures
fixed separately in #93.
Fixes #90
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Fix path normalization in remove, off-thread construction, and a docstring
Three independent items from the collected-nits issue.
remove_file built its manifest key from the raw user argument rather than
path_resolver.to_manifest_key, so an absolute path reported a tracked file
as untracked. It also built its S3 deletion key from the raw argument,
which would not match the key the object was stored under. Both now use the
normalised manifest key. Note "./data/x.bin" already worked, since
Path.as_posix collapses it; absolute paths were the broken case.
S3LFS.__init__ installed a SIGINT handler unconditionally. signal.signal
only works on the main thread of the main interpreter and raises ValueError
elsewhere, so constructing an S3LFS from a worker thread failed outright.
The handler is now best-effort, which is appropriate for a library
component that does not own process-wide signal handling.
hash_file's docstring claimed the digest covers "content and relative
path". It is content-only -- the same bytes at two paths produce the same
digest. The claim implied a path-collision resistance that does not exist.
Two of the six new tests fail on the parent commit.
Test suite: 61 failed / 555 passed before, 61 failed / 561 passed after.
Failure set identical.
Refs #91
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* Make the partial-upload test deterministic
The mock decided whether to fail a chunk from how many chunks were already
in the store. Uploads run concurrently, so with enough workers every chunk
passed that check before any was recorded and the simulated failure never
fired -- the test passed locally and failed on CI, which has more cores.
Decide from the chunk index instead, which does not depend on ordering.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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 #90. Stacked on #94.
Three related defects in the
(size, mtime, inode)hash cache.1. Metadata captured before hashing
File metadata was read before computing the hash and stored alongside the result after. A file modified during hashing produced an entry whose hash belonged to neither the old nor the new content — and which would be served again whenever those metadata recurred.
The file is now re-stat'd after hashing and the entry dropped if anything moved.
2. mtime granularity
Filesystem mtime is frequently 1-second granular, so a file modified within the same tick as it was hashed is indistinguishable from one that was not.
Such entries are now marked racy and revalidated rather than trusted on metadata alone.
The design detail worth reviewing
My first attempt simply refused to cache recently-modified files. That is wrong, and the existing suite said so — it broke five cache tests. Tracking files immediately after writing them is the primary workflow, so never caching within the racy window makes the cache useless exactly when it matters.
The entry is therefore still written. Recomputing once refreshes it with a timestamp comfortably after the mtime, and it is trusted from then on — one extra hash per file, once, rather than either a wrong answer or a permanently cold cache. This mirrors git's racily-clean handling.
test_racy_entry_becomes_trusted_after_revalidationpins the self-healing, since "revalidate" that never settles would be a silent performance cliff.3. Shared temp filenames
Both
save_manifestandsave_cachewrote through a fixed temp name (.s3_manifest.tmp). The rename is atomic; the content is not. Two writers interleave into the same temp file before either renames, so the surviving file can be a byte-level mix of two documents. Both now use unique temp names.Verification
Three of the six new tests fail on the parent commit.
61 failed / 549 passed→61 failed / 555 passed, failure set identical. The 61 are the missing-python-binary failures fixed separately in #93.Not addressed
load_cacheskips reading when the on-disk mtime matches what it last wrote, so two processes saving within one mtime tick can still lose an update — the same shape as the manifest read-modify-write bug, in the cache. It needs the same treatment and is not attempted here.cleanup_stale_cachealso still exists and is never called from the CLI.🤖 Generated with Claude Code
https://claude.ai/code/session_019UrDtdFKGwQZp8oYGwL2rj