fix(#1469): codec-driven GC reference discovery + file-level orphan matching#1479
Open
dimitri-yatsenko wants to merge 20 commits into
Open
fix(#1469): codec-driven GC reference discovery + file-level orphan matching#1479dimitri-yatsenko wants to merge 20 commits into
dimitri-yatsenko wants to merge 20 commits into
Conversation
…rphan matching Garbage collection deleted external-store files belonging to LIVE rows in two ways, both fixed here: 1. Custom-codec blindness. gc.scan recognized schema-addressed columns by hardcoded codec name (object/npy), so a custom SchemaCodec subclass (e.g. a NetCDF codec, #1469) was never scanned — its live files were reported orphaned and collect() deleted them. Recognition is now by type (isinstance(codec, SchemaCodec)), and reference extraction moves to a codec-owned hook, Codec.referenced_paths(stored). Custom SchemaCodec subclasses inherit correct behavior for free; fully custom codecs override. 2. Path-format mismatch (also hit built-in <object@>/<npy@>). A row's stored metadata references an object FILE ({schema}/{table}/{pk}/{field}_{token}), but list_schema_paths enumerated the enclosing DIRECTORY, so the referenced and stored path sets never matched and live objects were flagged orphaned. list_schema_paths now enumerates files (matching the referenced paths, with per-token granularity) and delete_schema_path removes the single orphaned file and prunes empty parent dirs. Delete-then-GC semantics unchanged (files survive row delete by design; GC reclaims). Adds Codec.referenced_paths to the base contract (default recognizes standard {path, store} metadata). Tests: recognition of custom SchemaCodec subclasses; end-to-end guard that collect() keeps a surviving row's file and reclaims only the deleted row's file (both fail on pre-fix code). Existing object/npy/hash suites still pass. Discovery layer of the trustworthy-GC work (#1478); #1445 builds on it.
The file-level orphan matching introduced earlier in this PR broke
DIRECTORY-valued objects (e.g. Zarr stores via <object@>): a row's metadata
references the directory PREFIX, while list_schema_paths enumerates the chunk
FILES under it — exact set difference therefore flagged every chunk of a LIVE
store as an orphan (empirically confirmed), and collect() would delete live
data. Folder objects also write a {path}.manifest.json sidecar, which the
listing skipped — orphaned folders would leak their manifests forever.
Orphan detection is now coverage-based via _is_covered(): a stored file is
referenced when it IS a referenced path (single-file object), lies UNDER a
referenced path (directory object), or is the .manifest.json sidecar of a
referenced object. Manifests are now listed (so orphaned ones are reclaimed);
lookup is O(path-depth) ancestor-prefix set membership per file.
Note the pre-existing code also mishandled directory objects (the PK-level
directory holding the manifest was orphan-flagged, so collect() would rm -r
the live object) — neither version was safe; this makes the new file-level
model correct for both single-file and directory objects.
Tests: a live folder object's chunks+manifest are never flagged (fails before
this commit); after deleting one of two rows, collect() reclaims exactly the
orphaned folder's chunks+manifest and preserves the survivor's. Full gc suite
34/34.
Member
Author
|
Amendment pushed ( A deep re-analysis of this PR found a critical flaw in its original form: the file-level orphan matching broke directory-valued objects (Zarr-style The amendment makes orphan detection coverage-based ( For context: the pre-PR code also mishandled directory objects, differently — the PK-level directory (holding the manifest) was orphan-flagged, so old One related, pre-existing hazard deliberately not addressed here (tracked for the #1478/#1445 follow-up): |
Review finding: after the codec-driven refactor in this PR, the two extraction helpers had no production callers — the scan functions call attr.codec.referenced_paths() — and were line-identical to each other AND to the Codec.referenced_paths base default (triple duplication). Deleted both (plus the now-unused json import); their unit tests are retargeted at the codec API, which is the single remaining implementation, with an added case verifying a custom SchemaCodec subclass inherits the extraction. GC suite 35/35.
Review finding (Q2): the schema-addressed walk skipped any path CONTAINING the substring '_hash' — a user table whose name merely contains it (e.g. probe_hash) was silently excluded from the listing, so its orphans were never reclaimed. The skip now matches the first store-relative path segment exactly. Also introduces HASH_STORAGE_PREFIX in hash_registry (the writer) and imports it in gc.py, so scanner and writer share one source of truth. The constant's comment documents the finding behind it: the layout is a fixed convention — the 'hash_prefix' store key accepted by settings is currently consumed ONLY as a reserved-namespace declaration for <filepath@> validation, not by the hash writer or GC (tracked separately). Regression test: a table named gc_probe_hash is listed, and its orphan is reclaimed after delete while the live row's file survives. GC suite 36/36; hash-storage suite unaffected.
…prefix keys Resolves the validate-and-ignore state of the per-store hash_prefix / schema_prefix / filepath_prefix keys (#1486) within this PR, per review: - The keys are now documented for what they are — NAMESPACE DECLARATIONS for store cohabitation, not relocation settings (settings.py docstring + HASH_STORAGE_PREFIX comment). DataJoint's storage layout stays fixed. - GC now gives filepath_prefix real teeth on the read side: list_schema_paths skips the declared filepath namespace, so user-managed <filepath@> files under it can never be classified as orphans or collected. This closes the disclosed cohabitation data-loss hazard for stores that declare the prefix; undeclared cohabitation remains a documented hazard for the two-phase GC fail-safe. Test: a stray user file under the declared prefix is excluded from listing and survives collect(dry_run=False); the same file IS an orphan candidate without the declaration (pinning the documented hazard). GC suite 37/37. Fixes #1486.
…is overridden
Follow-up to the namespace-declaration resolution, with a corrected picture:
get_store_spec DEFAULTS hash_prefix to '_hash' and schema_prefix to '_schema'
(settings.py:633-634), so filepath validation already reserved _hash/ in the
default case. The real hole: OVERRIDING hash_prefix (e.g. 'content') replaced
the default and silently un-reserved the actual _hash/ namespace — the key is
declarative and does not relocate hash storage, so the true layout lost its
protection exactly when a user misread the key as a relocation knob. filepath
validation now always reserves HASH_STORAGE_PREFIX in addition to declared
values.
Also documents the second nuance: schema_prefix's default '_schema' is
vestigial — schema-addressed objects live in root-level {schema}/ directories,
so the reservation protects a location nothing writes to (settings docstring).
Unit test: with hash_prefix overridden, filepath still rejects _hash/... AND
the declared override. filepath unit tests 9/9; settings 80/80; GC suite
37/37.
Direction reversed per maintainer review against the documentation: the
per-store prefix settings are LAYOUT-CONTROLLING, as documented in
how-to/configure-storage ('Hash-addressed section (configured via
hash_prefix, default _hash/)'; 'Schema-addressed section (configured via
schema_prefix, default _schema/)') and migrate-to-v20 (path format
{location}/{hash_prefix}/{schema}/{hash}). The code was out of spec twice:
the hash writer hardcoded _hash/ and the schema writer wrote to root-level
{schema}/... ignoring schema_prefix entirely.
Now all components consume the same settings, so sections can be relocated
per store without drift:
- put_hash/build_hash_path take hash_prefix from the store spec.
- build_object_path (codec + staged-insert callers) takes schema_prefix from
the store spec; new layout {schema_prefix}/{partition}/{schema}/{table}/...
(default section _schema/ per the documented default).
- gc.list_stored_hashes scans the configured hash section;
gc.list_schema_paths skips the configured hash and filepath sections. The
schema walk deliberately covers the whole store (minus reserved sections)
so root-level objects from stores written before schema_prefix was honored
remain listable and reclaimable; coverage-based orphan matching handles
mixed layouts because metadata records full paths.
- filepath validation reverts to spec-driven reservation: overriding
hash_prefix now moves writer and reservation together, closing the
override desynchronization differently than the previous hardcoded fix.
- HASH_STORAGE_PREFIX removed; DEFAULT_HASH_PREFIX/DEFAULT_SCHEMA_PREFIX
remain as builder defaults only. Settings docstring rewritten to the
layout-controlling semantics, with the note that changing a prefix on a
store holding data leaves old objects readable (metadata paths) but
outside the scanned sections until restored.
Tests: end-to-end pin that custom hash_prefix/schema_prefix are honored by
writers AND scanned/collected by GC; partition-path expectation updated to
the sectioned layout; put_hash tests updated to spec-driven setup; filepath
override test inverted (override is reserved, former default released).
Suites: gc 39/39, object/npy/chaining green, hash-storage 14/14, codecs +
settings + full unit 323 passed, both backends for integration.
…ngle source Review finding: build_hash_path/build_object_path carried default prefix parameters (DEFAULT_HASH_PREFIX/DEFAULT_SCHEMA_PREFIX) duplicating the defaults that settings already applies. _apply_common_store_defaults runs for EVERY store spec — built-in and plugin protocols alike — before get_store_spec returns, so the fallbacks were dead code and a second place for the default to drift. The prefixes are now required keyword-only parameters; both constants are deleted; all production callers index the spec directly (spec['hash_prefix'] / spec['schema_prefix']) with a loud KeyError, rather than a silent wrong-layout write, as the failure mode for any spec that bypassed get_store_spec. Direct builder calls in tests pass the prefix explicitly, which is the honest contract. Suites: hash-storage/object/unit 377 passed; gc/npy/chaining green.
…nge safety)
Empirically found while validating the prefix-change story: after hash_prefix
is changed on a populated store, the old hash objects sit OUTSIDE the currently
configured hash section, so the schema-addressed walk (whole store minus the
CURRENT hash+filepath sections) enumerates them. They are hash references, not
schema references, so coverage against schema_paths_referenced alone missed
them and collect() would DELETE live hash-addressed data. Reads were never
affected — metadata records each object's full path.
scan() now treats a stored file as live if covered by EITHER reference set
(schema OR hash), both being full relative paths from row metadata. This makes
the read-side invariant ('the path in metadata is the truth') hold on the GC
side too: relocating a prefix leaves existing objects readable AND safe from
collection, at the documented cost that objects under a former prefix are not
reclamation candidates until the setting is restored.
Regression test: with hash_prefix relocated from _hash to content, a live
blob object is absent from both orphan sets and survives collect(dry_run=
False); the row still fetches. GC suite 39/39.
…d checking
Docstring-only accuracy pass over gc.py (no behavior change):
- list_stored_hashes: no longer claims it 'scans the _hash/ directory' — it
walks the configured hash section (hash_prefix, default _hash/); documents
that only the current prefix is walked and that scan() still protects
objects under a former prefix. Returns/path-format comments parameterized.
- scan(): orphaned_hashes / orphaned_paths documented as full relative store
PATHS (they were described as 'content hashes' — the code passes them
straight to the deleter as paths); notes the either-set liveness rule and
the cross-section safety property; schema-addressed described as any
SchemaCodec incl. custom subclasses.
- list_schema_paths: single-file path now shows the {schema_prefix} section;
added a Scope paragraph (whole store minus hash+filepath sections; legacy
root-level and post-prefix-change hash objects both stay listable, with
scan() disambiguating).
- scan_hash/schema_references: Returns clarified as full relative metadata
paths (directory objects recorded as the prefix); schema scan noted as
type-based over SchemaCodec.
- collect(): notes orphan identity comes from scan(), so live cross-section
objects and the filepath_prefix namespace are never deleted.
- Stale inline comments (_hash/{schema}/... path format, 'No _hash/ directory
yet', the shallow-file floor guard) updated. GC suite 39/39.
Review decision: format_stats presumed a single display format and was never called by the library — scan()/collect() return plain dicts. Users inspect or render those dicts however they like, so the helper only added surface area. Removed the function and its three unit tests. GC suite 36/36. Docs that showed dj.gc.format_stats(stats) are updated to inspect the stats dict directly (datajoint-docs).
…to each schema
Both storage sections embed the schema in the path
({hash_prefix}/{schema}/... and {schema_prefix}/{schema}/..., plus the legacy
root-level {schema}/...), and hash deduplication is per-schema — so a schema's
orphan set is fully determined by its own references vs. its own stored files,
with no cross-schema interaction. GC now uses this:
- list_stored_hashes / list_schema_paths take an optional schema_name that
confines the walk to that schema's subtree(s) (the hash section subtree, and
for schema-addressed both the schema_prefix section and the legacy
root-level layout). schema_name=None keeps the whole-store walk.
- scan() iterates the passed schemas, listing each against ITS OWN subtree and
computing orphans per schema, then aggregates. collect() inherits this.
Consequence — the safety win: orphan detection is limited to the schemas you
pass. Objects of other schemas sharing the store are never listed and can never
be misclassified as orphans or deleted. This removes the previous requirement
to pass EVERY schema on a store at once; any subset is now safe. The per-schema
either-set liveness check (schema OR hash references) is retained for the
empty-/former-hash_prefix cases.
Tests: two schemas sharing one store — scoped listings don't leak across
schemas; scan(a) reports zero orphans while b holds only live data; collect(a)
reclaims a's orphan and leaves every b object intact. GC suite 39/39;
hash/object/npy/chaining 96 passed.
Per review, GC is now strictly per-schema and the schema-addressed walk is
confined to the schema's own section — a much smaller, safer surface:
- list_stored_hashes / list_schema_paths take schema_name as a REQUIRED
keyword-only argument (no more whole-store mode). list_schema_paths walks
ONLY {schema_prefix}/{schema}/; list_stored_hashes ONLY {hash_prefix}/{schema}/.
- Because those two top-level sections are mutually exclusive (store
validation) and the filepath section is a third, the schema walk can never
enter the hash or filepath sections. So all of this is deleted:
* the hash-section skip (and the _hash-substring guard it needed),
* the filepath-section skip,
* the schema-OR-hash union coverage added for the prefix-change corner —
a stranded hash object simply cannot appear in a schema-only walk.
Orphan coverage is now schema references alone.
- GC's blast radius shrinks to exactly {hash_prefix}/{schema}/ and
{schema_prefix}/{schema}/: user <filepath@> content and other schemas'
objects are structurally out of reach. Resolves the cohabitation hazard for
the common case without a separate fail-safe.
- collect() builds bytes_freed size maps per schema (guarded by orphan counts
so the mocked unit tests don't invoke real listers).
Legacy note: 2.3.0-and-earlier stores wrote schema-addressed objects at
root-level {schema}/... (schema_prefix ignored, #1487). Such a store should
set schema_prefix='' so writes and GC both use root-level; the per-schema walk
then targets {schema}/ directly. Otherwise those objects stay readable but
outside the scanned section.
Tests updated to pass schema_name; the filepath test now asserts the hazard is
gone by construction; the _hash-substring test keeps its positive assertion.
GC suite 39/39; hash/object/npy/chaining 96 passed.
…ibute Storage-model classification is heading knowledge, not GC-private. Added, parallel to Attribute.is_blob / Heading.blobs: - Attribute.is_hash_object — external hash-addressed storage (<hash@>, or external <blob@>/<attach@>; inline blob/attach are False, matching the framework's is_store test). - Attribute.is_schema_object — schema-addressed storage, by codec TYPE (isinstance SchemaCodec), so custom subclasses are included (#1469). - Heading.hash_objects / Heading.schema_objects — attribute-name lists, parallel to Heading.blobs. gc.py drops its private _uses_hash_storage / _uses_schema_storage and consumes table.heading.hash_objects / schema_objects instead — one source of truth that other modules can reuse. Behavior is identical (the predicates moved verbatim). Their unit tests move to TestAttributeIsHashObject / TestAttributeIsSchemaObject, now building real heading.Attribute instances instead of MagicMocks. GC suite 40/40; gc + full unit 359 passed.
Uniform naming with list_schema_paths — both return {relative_path: size} for
one schema's section. No behavior change; all call sites and the two mocked
unit tests updated. GC suite 40/40.
Garbage collection is store-specific, so bind it to a store once instead of threading store_name/config through every function. - GarbageCollector(store, *, config=None) resolves the store EAGERLY at construction and exposes all ops as methods (hash_references, schema_references, list_hash_paths, list_schema_paths, delete_schema_path, scan, collect) with no store/config params. DB metadata is read via each schema's own connection; storage work uses the bound store. - Removed all module-level gc functions and thin wrappers; callers instantiate a GarbageCollector. _is_covered stays module-private. - Dropped unused combined totals from scan/collect output. - Removed the redundant per-loop schema.database local in scan. GC 41/41; gc+hash+object+unit 418 passed.
dimitri-yatsenko
added a commit
to datajoint/datajoint-docs
that referenced
this pull request
Jul 6, 2026
…ombined totals Reflects the GC refactor in datajoint/datajoint-python#1479: garbage collection is a store-bound GarbageCollector class, not module-level functions. - All examples use dj.gc.GarbageCollector(store=...).scan(...) / .collect(...). - The "Multiple Schemas" section is inverted: per-schema scoping makes any SUBSET of a store's schemas safe to collect — the old "always pass every schema or lose data" warning was the opposite of the shipped behavior and is replaced with a per-schema-attribution note. - Named-store examples bind the store at construction (no store_name= param). - Combined totals (orphaned/orphaned_bytes/referenced/stored) removed from the statistics reference and examples; readers sum the per-section fields.
Bind the schema set at construction instead of passing it into every method; and since collect(dry_run=True) is exactly the read-only scan, fold scan in. - GarbageCollector(*schemas, store=None, config=None): schemas required and fixed; config DEFAULTS to schemas[0].connection._config. Store resolved eagerly. - references/collect operate on self.schemas (no schema args). list_hash_paths /list_schema_paths still take a schema_name. - Removed scan(); collect(dry_run=True) default returns the full per-section report plus deletion outcome; dry_run=False also deletes. Size maps from the scan are reused for the byte tally (no second listing). - References gathered across all schemas at once (paths embed the schema, so combined coverage equals per-schema coverage). GC 39/39; hash+object+unit 377 passed.
dimitri-yatsenko
added a commit
to datajoint/datajoint-docs
that referenced
this pull request
Jul 6, 2026
…n) replaces scan Follows datajoint/datajoint-python#1479: the collector takes its schemas at construction (dj.gc.GarbageCollector(schema1, schema2, store=...)), and the separate scan() is gone — collect(dry_run=True) is the default read-only report (full orphan paths + reclaimable bytes), collect(dry_run=False) deletes.
…_hash_paths) Mirror the schema-addressed naming on the hash-addressed side of the collect() report, so the two sections read in parallel: hash_referenced -> hash_paths_referenced hash_stored -> hash_paths_stored hash_orphaned -> hash_paths_orphaned hash_orphaned_bytes -> hash_paths_orphaned_bytes orphaned_hashes -> orphaned_hash_paths hash_deleted -> hash_paths_deleted Keys and the corresponding internal variables renamed; no behavior change. Tests and docs (datajoint-docs) updated. GC suite 39/39.
dimitri-yatsenko
added a commit
to datajoint/datajoint-docs
that referenced
this pull request
Jul 6, 2026
…h_paths) Mirror the schema-addressed naming on the hash side of the collect() report (datajoint/datajoint-python#1479): hash_referenced->hash_paths_referenced, hash_stored->hash_paths_stored, hash_orphaned->hash_paths_orphaned, hash_orphaned_bytes->hash_paths_orphaned_bytes, orphaned_hashes->orphaned_hash_paths.
… symmetry Completes the hash/schema stat-name parallelism: the two orphan-path lists are now orphaned_hash_paths and orphaned_schema_paths (was the unqualified orphaned_paths). Keys + internal variable renamed; no behavior change. GC 39/39.
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 #1469 — garbage collection deleting external-store files that belong to existing rows. Implements the plan from #1469 / #1478.
Two root causes, both fixed
1. Custom-codec blindness (the reported bug).
gc.scanrecognized schema-addressed columns by hardcoded codec name (object/npy), so a customSchemaCodecsubclass (the reporter's NetCDF codec) was never scanned — its live files were reported orphaned andcollect()deleted them. Recognition is now by type (isinstance(codec, SchemaCodec)), and reference extraction moves to a codec-owned hook:Custom
SchemaCodecsubclasses inherit correct behavior for free; fully custom codecs can override.scancallsattr.codec.referenced_paths(value)per column instead of switching on name.2. Path-format mismatch (latent — also hit built-in
<object@>/<npy@>). A row's stored metadata references an object file ({schema}/{table}/{pk}/{field}_{token}), butlist_schema_pathsenumerated the enclosing directory — so the referenced and stored path sets never matched and every live schema-addressed object was flagged orphaned (existing tests only assertedreferenced >= 1, neverorphaned == 0, so it was hidden).list_schema_pathsnow enumerates files (matching referenced paths, with per-token granularity), anddelete_schema_pathremoves the single orphaned file and prunes empty parent dirs.Not changed
Delete-then-GC semantics are unchanged — external files still survive row delete by design; GC reclaims. No delete-time eager cleanup (unsafe for hash dedup).
Tests
SchemaCodecsubclass (_uses_schema_storage).collect(), the surviving row's file remains and only the deleted row's file is reclaimed — asserted by exact path (robust to the shared test store). Both new tests fail on pre-fix code.test_gc.py(32) + object/npy/hash/codec/adapter suites (237) pass locally on MySQL & PostgreSQL.Discovery layer of the trustworthy-GC work (#1478); #1445 (two-phase quarantine/grace/purge) builds on the now-trustworthy scan.
Fixes #1486 (prefix keys re-documented as namespace declarations; GC honors
filepath_prefixas a protected subtree — commit 068b365).Fixes #1487 (path settings not incorporated by writers/GC — see that issue for upgrade impact; ships in 2.3.1).