Uh oh!
There was an error while loading. Please reload this page.
fix(gc): don't act on partial storage listings - #1506
Conversation
Three compounding completeness bugs in GarbageCollector.list_hash_paths and list_schema_paths that could cause collect(dry_run=False) to silently delete live external-store files: 1. str.replace(full_root, '') strips every occurrence — corrupts the relative path when the store-root string recurs (e.g., S3 bucket name == schema name). Uses prefix slice instead. 2. fs.size() error handling diverged: list_hash_paths dropped the entry silently (invisible to orphan detection); list_schema_paths kept it at 0 silently. Now both keep at 0 and log a warning. 3. fs.walk errors returned a partial-or-empty stored dict with only a log-warning signal — indistinguishable from a clean store. collect() then computed orphaned = stored - referenced against incomplete data and permanently deleted the 'orphans'. Adds a _scan_errors tracker on the collector; collect(dry_run=False) refuses to delete when set, with a clear DataJointError. New scan_errors list added to the stats dict (empty on clean runs). Tests: 3 new TestScanErrorGuard cases; full local suite green (42/42 test_gc.py, 319/319 unit, 626 integration — no new failures). Follow-up to the completeness discussion on datajoint#1478.
There was a problem hiding this comment.
Reviewed the full diff and traced list_*_paths, collect(), and _full_path(). This is a solid, well-scoped safety fix — the exception ordering (FileNotFoundError before Exception so a missing section isn't misrecorded as a scan error), the guard placement (reset at the top of collect(), checked before any deletion, dry_run=True reports without raising), and the copy-on-return of scan_errors are all correct. The fs.size→0 unification fails in the safe direction (keeps the file visible rather than silently dropping it).
Two non-blocking suggestions, both inline. Neither needs to hold up merge — they'd be equally fine as a quick follow-up, consistent with the deferred-scope note on the two datajoint-python#1478 findings.
- The prefix-slice in change 1 relies on an invariant that isn't enforced for custom storage adapters.
- Change 1 (the headline path-corruption bug) has no regression test.
Details on each below.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
Following up on my earlier review — I'm withdrawing both suggestions:
Prefix-slice / custom adapters: Retracted. I traced the deletion path:
list_*_pathsretains only the derived relative key, and deletion reconstructs the target via_full_path(key), so a mis-derived key would round-trip to a non-existent path rather than touch a live file — no data-loss risk. The invariant also holds by construction for every built-in backend, and the prefix-slice is a strict improvement over the previous.replace(). My original framing overstated the severity; nothing to change here.Regression test for the recurring-root case: Withdrawing this too — the fix is straightforward and the scenario is narrow; not worth holding the PR or churning the suite over.
The exception ordering (FileNotFoundError before Exception), the scan-error guard before deletion, and the safe-direction fs.size handling are all correct. LGTM — approving. Nice, well-scoped safety fix.
Uh oh!
There was an error while loading. Please reload this page.
Fixes three compounding completeness bugs in
GarbageCollector.list_hash_pathsandlist_schema_pathsthat could causecollect(dry_run=False)to silently delete live external-store files. Follow-up to the completeness-layer discussion on #1478.Changes
1.
str.replace(full_root, "")corrupts paths when the root string recursBoth
list_*_pathsderived relative paths asfile_path.replace(full_root, "").lstrip("/")(gc.py:209, 256).str.replacereplaces every occurrence, so iffull_rootreappears later infile_path— e.g., S3 bucket "store" + schema "store" →full_root="store/"recurs in"store/_hash/store/xxx"— the middle occurrence gets stripped too. The resulting relative key doesn't match the reference-side shape, so a live file falls into the orphan set. Real live files aren't destroyed (the corrupted key failsbackend.exists()), but real orphans in affected schemas never get reclaimed. Usesfile_path[len(full_root):](prefix slice) instead.2.
fs.sizehandling diverged silently between the two functionslist_hash_pathswrappedstored[relative_path] = fs.size(file_path)inexcept Exception: pass— dropping the file fromstoredentirely, so an orphan invisible tocollect()and quietly retained forever.list_schema_pathsusedexcept Exception: stored[relative_path] = 0— keeping it visible but counting 0 bytes towardbytes_freed. Neither logged. Both now keep the entry at 0 and log a warning.3.
fs.walkerrors returned a partial listing with no signal tocollect()An S3 permission drop, expired token, transient 5xx, NFS unmount, etc. all present to
collect()as an empty (or partial)storeddict.orphaned = stored - referencedthen computes toset(), and the stats dict reportsorphaned_hash_paths: 0— indistinguishable from a genuinely clean store. A user running nightly GC would see "nothing to reclaim" while their listing was silently failing.Adds a
_scan_errors: list[str]instance attribute onGarbageCollector.list_*_pathsappend to it when a walk fails (in addition to the existinglogger.warning).collect()resets it at the start and, before deleting, refuses with a clearDataJointErrorif it's non-empty. The stats dict now includesscan_errors: list[str]— empty on clean runs.Behavior changes
collect(dry_run=False)raisesDataJointErrorwhen a listing walk failed. Real-store tests unaffected; would only surface in tests that mockfs.walkto raise (none currently do).fs.sizeerrors on the hash side now keep the entry at 0 (was: drop). More conservative for orphan detection.scan_errors: list[str]field in the returned stats dict. Additive; existing tests that check specific keys still work.Verification
Applied and tested locally in an isolated worktree:
tests/integration/test_gc.py: 39/39 pass unchanged → 42/42 pass with fix + 3 newTestScanErrorGuardtests (reset-between-calls, refuse-to-delete-on-scan-error, scan_errors-in-stats).test_tls.py, 5 pre-existing errors intest_university.py— both unrelated togc.py).Scope note
Findings #1 (
_referencesswallows per-table exceptions) and #2 (Codec.referenced_pathssilent[]on bad input) from #1478 are structurally similar but touch different code paths (_references()and the codec API surface). Kept out of this PR to keep the scope tight — they could adopt the same_scan_errorsmechanism in a follow-up.