Trustworthy garbage collection: complete reference discovery (#1469) + race-safe deletion (#1445) #1478

Activity

1. dimitri-yatsenko

   added

   enhancementIndicates new improvementsIndicates new improvements

   on Jul 2, 2026
2. dimitri-yatsenko commented on Jul 2, 2026

   

   dimitri-yatsenko

   on Jul 2, 2026

   MemberAuthor

   More actions

   Refinement: two addressing schemes = two GC models; branching safely deferred

   Confirmed in code (builtin_codecs/schema.py::_build_path + storage.generate_token): schema-addressed paths are {schema}/{table}/{pk}/{field}{ext} plus a unique per-write token (default length 8). So schema-addressed is not one-file-per-row — the token enables multiple versions of the same logical row's object (immutable writes; a recompute/update lands at a new token path).

   That gives two distinct GC models, to be specified separately:

   • Hash-addressed (<blob@> / <attach@> / <hash@>) — content-addressed, deduped, many entries → one object. Orphan ⟺ reference count reaches zero across all rows. The harder determination, and where the Two-phase, transaction-safe garbage collection (quarantine -> grace -> purge) #1445 scan→delete race bites hardest.
   • Schema-addressed (<object@> / <npy@>) — token-per-write, row-owned versions. Orphan ⟺ a token path no current row references (superseded tokens fall out naturally). Essentially per-row; no cross-entry sharing.

   Both are discovered by the same referenced_paths codec hook (#1469): it returns the path(s) the current stored value references, so live tokens are retained and superseded ones become orphans — for both models.

   Branching is deliberately deferred until #1469 and #1445 are addressed, and that's safe:

   • Schema-addressed's token is globally unique per write → already branching-compatible (two branches computing the same PK write distinct token paths, no collision).
   • Hash-addressed is content-addressed/immutable → branch-shareable by nature.
   • So branching requires no addressing change; it only widens GC's reference scope later (a hash object is orphaned only when no branch references it), which layers onto this design without invalidating it.

   Net: proceed on the current addressing model — #1469 (discovery) → #1445 (two-phase deletion) — and revisit only GC's reference scope when branching lands.
3. dimitri-yatsenko commented on Jul 2, 2026

   

   dimitri-yatsenko

   on Jul 2, 2026

   Last edited by dimitri-yatsenko

   MemberAuthor

   More actions

   Heads-up while implementing this (PR #1479): the fix also closes a latent data-loss path in built-in schema-addressed storage — <object@> and <npy@>, not just custom codecs.

   A second root cause (in addition to the codec-name blindness): a row's stored metadata references the object file ({schema}/{table}/{pk}/{field}_{token}), but list_schema_paths enumerated the enclosing directory. The referenced and stored path sets therefore never matched, so every live schema-addressed object was classified as an orphan — meaning gc.collect() could delete live <object@>/<npy@> files. It stayed hidden because the existing scan tests only asserted referenced >= 1, never orphaned == 0.

   PR #1479 fixes it by enumerating files (matching the referenced paths, with per-token granularity) and deleting the single orphaned file (+ pruning empty parents). Net: this broadens the severity — any pipeline that ran gc.collect() over schema-addressed columns was exposed, not only custom-codec users.
4. MilagrosMarin commented on Jul 17, 2026

   

   MilagrosMarin

   on Jul 17, 2026

   Last edited by MilagrosMarin

   Contributor

   More actions

   Adding three concrete findings from a v2.3.1 code audit that slot into the umbrella framing (completeness in #1469 territory, race-safety in #1445 territory). Not proposing to replace the two-phase plan — surfacing them so they inform the redesign.

   Completeness layer (post-#1469)

   Even with codec-driven discovery in place, two paths silently truncate the reference set:

   1. _references() swallows per-table exceptions (gc.py:158-178)

   try:
       for row in table.proj(attr_name).cursor(as_dict=True):
           try:
               for path, _ in codec.referenced_paths(row[attr_name]):
                   referenced.add(path)
           except Exception as e:
               logger.warning(...)  # partial reference set returned
   except Exception as e:
       logger.warning(...)  # entire table's refs dropped

   Any transient error mid-scan (a Lost connection to MySQL server during query mid-cursor, an intermittent server error, an unexpected exception in codec.referenced_paths) is logged as a warning; the loop moves on and _references() returns a partial set with no completeness signal to the caller. collect(dry_run=False) then computes orphaned = stored - PARTIAL_referenced and deletes those files at gc.py:355-375 — a live file becomes orphaned by a mid-scan hiccup.

   Concrete failure: table with 500 hash-referenced rows, connection drops after row 20 → 480 live files land in orphaned_hash_paths → permanently deleted, with one WARNING line in the log.

   2. Codec.referenced_paths() returns [] on bad input (codecs.py:252-260)

   def referenced_paths(self, stored):
       if isinstance(value, str):
           try:
               value = json.loads(value)
           except (json.JSONDecodeError, TypeError, ValueError):
               return []          # silent zero refs
       if isinstance(value, dict) and "path" in value:
           return [(value["path"], value.get("store"))]
       return []                   # dict without "path" → silent zero refs

   Corrupted metadata (a partial-write JSON blob, a JSONB double-encoding, a custom codec whose format drifts) contributes zero references, and its live backing file becomes an orphan candidate. The tests at test_gc.py:117-121, 168-169 enshrine this behavior, so it's intentional — but under dry_run=False it's a silent-data-loss path.

   Race-safety layer (#1445 territory)

   3. collect() scans references before listing stored files (gc.py:333-340)

   Current order:

   hash_paths_referenced = self.hash_references()            # line 333
   schema_paths_referenced = self.schema_references()        # line 334
   for schema in schemas:
       hash_paths_stored.update(self.list_hash_paths(...))   # line 336-340
       schema_paths_stored.update(self.list_schema_paths(...))

   A concurrent writer whose put_buffer() + INSERT sequence completes between the reference scan and the stored listing gets its file classified as orphan and deleted — DB row survives pointing at a deleted file. Reversing the order (list stored first, then scan references) narrows the window but doesn't close it (a writer whose upload completes before listing but whose INSERT commits after the reference scan is still exposed) — which is why the full quarantine → grace → purge in #1445 is the durable fix.

   Why they interact

   The naïve "raise on partial scan" fix for finding #1 collides with finding #2: as it stands, the cursor loop at gc.py:170-176 swallows anything raised by codec.referenced_paths(), so tightening one without the other trades silent data loss for aborted collections. A coordinated fix probably needs:

   • Codec.referenced_paths() to distinguish "genuinely no refs" from "couldn't determine" (e.g., a specific exception, a sentinel, or a two-return signature).
   • _references() to propagate an incomplete-scan flag to collect(), which refuses to delete when it's set.

   Both feed the completeness contract in the umbrella framing.

   Not a fix

   Docs-only surface; the two-phase redesign in #1445 is a superset. Just flagging that the completeness layer needs its own tightening (findings 1 & 2) even after #1469 lands, and finding 3 is a one-line reorder that could be a stopgap for the race window until the quarantine work merges.
5. dimitri-yatsenko commented on Jul 17, 2026

   

   dimitri-yatsenko

   on Jul 17, 2026

   MemberAuthor

   More actions

   Progress: the first child — #1469 (complete reference discovery) — is done and merged in #1479 (codec-driven reference discovery + file-level orphan matching; #1479 also closed #1486/#1487). Checked off above.

   Remaining before this umbrella closes:

   • Two-phase, transaction-safe garbage collection (quarantine -> grace -> purge) #1445 — two-phase quarantine → grace → purge deletion. Its precondition (Garbage collection deletes external-store files belonging to existing rows (custom codecs) #1469) is now satisfied, but the state machine itself is unstarted. (Note: fix(gc): don't act on partial storage listings #1506 is a scan-completeness hardening follow-up, not the two-phase API.)
   • The datajoint-docs work — new reference/specs/garbage-collection.md spec, plus the codec-api.md / custom-codec / how-to updates listed above.
6. MilagrosMarin

   added a commit that references this issue on Jul 18, 2026

   fix(gc): don't act on partial storage listings

   ...

   edb9545
7. dimitri-yatsenko

   added this to the v2.3.3 milestone on Jul 21, 2026
8. github-actions commented on Sep 5, 2026

   

   github-actionsBot

   on Sep 5, 2026 – with GitHub Actions

   More actions

   This issue is stale because it has been open for 45 days with no activity.
9. github-actions

   added

   staleIndicates issues, pull requests, or discussions are inactiveIndicates issues, pull requests, or discussions are inactive

   on Sep 5, 2026
10. dimitri-yatsenko

    modified the milestones: v2.3.3, v2.4 on Sep 9, 2026
11. github-actions

    removed

    staleIndicates issues, pull requests, or discussions are inactiveIndicates issues, pull requests, or discussions are inactive

    on Sep 10, 2026