Make garbage collection reachability path-aware#97
Merged
Conversation
Objects are stored at assets/{hash}/{manifest_key}.gz, so the storage unit
is hash plus path. Garbage collection judged reachability on the hash
alone, which leaked in both directions: an object stayed reachable as long
as any path shared its content, so removing one of two identical files
never freed the removed path's object, and renaming a file left its old
object reachable forever though nothing could address it.
Reachability is now computed as the set of base keys the manifest implies,
and a key is live if it is one of those or a .chunkN belonging to one. The
in-flight registry moves from hashes to the same base keys, so a claim on
one path no longer protects a different path that happens to share content.
remove_file and remove_subtree deleted only the base .gz key, orphaning
every chunk of a large file. They now delete the chunks too.
This keeps the existing layout rather than moving to content-addressed
storage, so no migration is needed. It does mean deduplication remains
per-path: two identical files at different paths still occupy two objects,
contrary to what the README implies. That claim should be corrected or the
layout changed; both are out of scope here.
An existing test caught a regression in an earlier draft: keys under the
prefix that do not have the shape of an asset were being collected. They
are now explicitly skipped, since s3lfs did not put them there in a form it
recognises and they are not ours to delete.
Five of the seven new tests fail on the parent commit. Three tests from the
in-flight registry change were updated for the key-based API.
Test suite: 61 failed / 561 passed before, 61 failed / 568 passed after.
Failure set identical.
Fixes #87
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 #87, taking the path-aware-GC option rather than content-addressed storage. Stacked on #96.
The mismatch
Objects live at
assets/{hash}/{manifest_key}.gz, so the storage unit is hash + path. GC judged reachability on the hash alone. Those disagree, and it leaked in both directions:a/x.binandb/x.binshare hashh; removea/x.bin.his still referenced byb/x.bin, soassets/h/a/x.bin.gzwas never collected.assets/h/oldpath.gzstayed reachable-by-hash forever, though nothing could address it.Reachability is now the set of base keys the manifest implies. A key is live if it is one of those, or a
.chunkNbelonging to one.Knock-on changes
The in-flight registry moves from hashes to base keys. Otherwise a claim on one path would protect a different path sharing its content — the same conflation, reintroduced through the registry added in #94.
remove_file/remove_subtreenow delete chunks. They deleted only the base.gz, orphaning every chunk of a large file. Previously those orphans were also uncollectable when the hash stayed referenced elsewhere; now they would be collected eventually, but--purge-from-s3should mean now.What this deliberately does not do
Deduplication stays per-path: two identical files at different paths still occupy two objects. The README's claim that identical files are stored only once remains inaccurate. Fixing that means content-addressed storage (
assets/{hash}.gz) and a migration, which is the option not taken here. Either the layout or the documentation should change; I'd suggest correcting the docs unless the storage saving matters.A regression an existing test caught
An earlier draft collected any key under the prefix that wasn't in the live set, including malformed ones.
test_cleanup_s3_short_key_pathsfailed and was right to: s3lfs didn't put those there in a form it recognises, so they are not ours to delete. Keys must now have the shape of an asset before being considered — restoring the intent of the oldlen(parts) < 3guard.Verification
Five of the seven new tests fail on the parent commit. Three tests from #94 were updated for the key-based API.
61 failed / 561 passed→61 failed / 568 passed, failure set identical. The 61 are the missing-python-binary failures fixed in #93.🤖 Generated with Claude Code
https://claude.ai/code/session_019UrDtdFKGwQZp8oYGwL2rj