Protect in-flight uploads from garbage collection#94
Merged
Conversation
kmatzen
force-pushed
the
gc-inflight-registry
branch
from
July 19, 2026 03:36
c7658de to
b075f15
Compare
This was referenced Jul 19, 2026
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>
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>
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>
_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>
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>
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>
…heckout
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>
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>
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>
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>
…tring (#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>
kmatzen
force-pushed
the
gc-inflight-registry
branch
from
July 19, 2026 05:16
d578a61 to
624d62d
Compare
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>
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 #85. Stacked on #84, which it depends on — the registry's guarantee assumes the manifest lock actually provides mutual exclusion.
The race
cleanup_s3is mark-and-sweep with the lock released between mark and sweep, and an unboundedinput()confirmation in the middle. An uploader PUTs chunks before publishing its manifest entry, so an object can sit in S3 referenced by nothing. A sweep landing in that window deletes it, leaving the manifest pointing at an object that no longer exists.Why the obvious fix was rejected
Re-validating the manifest under the lock immediately before deleting looks right.
specs/S3lfsGC.tlasays otherwise:The entire cycle fits inside the upload-then-commit window, during which the manifest genuinely does not reference the object. No amount of re-reading it helps.
What this does
Uploaders claim each hash in a registry (
.s3lfs_temp/.s3lfs_inflight.yaml) 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.The release ordering carries the whole argument: releasing before the manifest write would reopen the exact window the registry exists to close.
This is the
INFLIGHTconfiguration, which checks clean over the full state space (87 distinct states, queue drained).Measured
With the uploader held between its chunk PUT and its manifest commit, and a full
cleanup_s3run in that window:['asset.bin']['asset.bin']['asset.bin']Six of the seven new tests fail on the parent commit.
Trade-off worth reviewing
A crashed uploader leaves claims behind, so claims age out after 24h (
INFLIGHT_TTL_SECONDS). That timeout bounds the leak from a crash and plays no part in closing the race.This is deliberately narrower than the alternative I considered — an object-age grace period, where the timeout is the correctness argument and a slow upload exceeding it silently reintroduces the bug. Here, exceeding the TTL only means an abandoned object becomes collectable, which is the desired outcome anyway.
Also included
Asset key parsing moves off an unanchored
str.replace, which corrupted the parse when the repo prefix appeared again inside a file path (part of #91). The re-validation step needs correct parsing, so it was in scope.Not addressed
cleanup_s3still deletes serially rather than batching viadelete_objects, and a mid-sweep crash leaves a partial sweep with no resume state. Neither affects correctness.🤖 Generated with Claude Code
https://claude.ai/code/session_019UrDtdFKGwQZp8oYGwL2rj