From 5037a3dae898476fc59d41d74624ab92d036f2bd Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Thu, 2 Jul 2026 16:44:54 -0500 Subject: [PATCH 01/20] fix(#1469): codec-driven GC discovery + file-level schema-addressed orphan matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 /). 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. --- src/datajoint/codecs.py | 40 +++++++++++++++ src/datajoint/gc.py | 94 ++++++++++++++++++++++-------------- tests/integration/test_gc.py | 92 ++++++++++++++++++++++++++++++++--- 3 files changed, 185 insertions(+), 41 deletions(-) diff --git a/src/datajoint/codecs.py b/src/datajoint/codecs.py index 2719e9509..c68e33413 100644 --- a/src/datajoint/codecs.py +++ b/src/datajoint/codecs.py @@ -38,6 +38,7 @@ class MyTable(dj.Manual): from __future__ import annotations +import json import logging from abc import ABC, abstractmethod from typing import Any @@ -219,6 +220,45 @@ def validate(self, value: Any) -> None: """ pass + def referenced_paths(self, stored: Any) -> list[tuple[str, str | None]]: + """ + Return the external store paths this stored value references. + + Garbage collection and delete-time cleanup call this on the *stored* + (encoded) representation of a column value — the JSON/dict already in + the database — so discovery stays metadata-only (no download or decode). + + The default recognizes DataJoint's standard external-storage metadata: + a dict (or JSON string) carrying a ``path`` and optional ``store``. This + covers hash-addressed (````/````/````) and + schema-addressed (````/```` and any :class:`SchemaCodec` + subclass) storage with no extra work, and returns an empty list for + values that are not externally stored (e.g. in-table ````). + + Override only if your codec stores external references in a non-standard + shape. Returning the paths here is what lets garbage collection see a + custom codec's files as *referenced* rather than orphaned (see #1469). + + Parameters + ---------- + stored : any + The stored (encoded) value as read from the database column. + + Returns + ------- + list[tuple[str, str | None]] + ``(path, store_name)`` for each external artifact referenced. + """ + value = stored + if isinstance(value, str): + try: + value = json.loads(value) + except (json.JSONDecodeError, TypeError, ValueError): + return [] + if isinstance(value, dict) and "path" in value: + return [(value["path"], value.get("store"))] + return [] + def __repr__(self) -> str: return f"<{self.__class__.__name__}(name={self.name!r})>" diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 8c87efd84..80b862e93 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -108,8 +108,12 @@ def _uses_schema_storage(attr) -> bool: if not attr.codec: return False - codec_name = getattr(attr.codec, "name", "") - return codec_name in ("object", "npy") + # Recognize schema-addressed storage by type, not by a hardcoded codec name, + # so custom SchemaCodec subclasses (e.g. a user's NetCDF codec) are seen by + # GC and their live files are not misclassified as orphans (#1469). + from .builtin_codecs.schema import SchemaCodec + + return isinstance(attr.codec, SchemaCodec) def _extract_hash_refs(value: Any) -> list[tuple[str, str | None]]: @@ -231,12 +235,13 @@ def scan_hash_references( # Read raw JSON metadata via cursor — bypasses decode_attribute # so we get the stored dict (PostgreSQL/JSONB) or JSON string - # (MySQL), not the decoded codec output. _extract_hash_refs - # handles both shapes. + # (MySQL), not the decoded codec output. The codec's own + # referenced_paths() extracts the referenced paths and handles + # both shapes (codec-driven discovery, #1469). try: cursor = table.proj(attr_name).cursor(as_dict=True) for row in cursor: - for path, ref_store in _extract_hash_refs(row[attr_name]): + for path, ref_store in attr.codec.referenced_paths(row[attr_name]): # Filter by store if specified if store_name is None or ref_store == store_name: referenced.add(path) @@ -296,12 +301,13 @@ def scan_schema_references( # Read raw JSON metadata via cursor — bypasses decode_attribute # so we get the stored dict (PostgreSQL/JSONB) or JSON string - # (MySQL), not the decoded codec output. _extract_schema_refs - # handles both shapes. + # (MySQL), not the decoded codec output. The codec's own + # referenced_paths() extracts the referenced paths and handles + # both shapes (codec-driven discovery, #1469). try: cursor = table.proj(attr_name).cursor(as_dict=True) for row in cursor: - for path, ref_store in _extract_schema_refs(row[attr_name]): + for path, ref_store in attr.codec.referenced_paths(row[attr_name]): # Filter by store if specified if store_name is None or ref_store == store_name: referenced.add(path) @@ -379,10 +385,14 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, int]: """ - List all schema-addressed items in storage. + List all schema-addressed object files in storage. - Scans for directories matching the schema-addressed storage pattern: - ``{schema}/{table}/{pk}/{field}/`` + Enumerates the individual object **files** written by schema-addressed + codecs, whose paths follow ``{schema}/{table}/{pk}/{field}_{token}[.ext]``. + Returning file paths (rather than the enclosing directory) is what lets + them match the file paths recorded in each row's stored metadata — the two + must be comparable for orphan detection, and per-token granularity lets + superseded versions be reclaimed while the current token is retained. Parameters ---------- @@ -394,13 +404,13 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i Returns ------- dict[str, int] - Dict mapping storage path to size in bytes. + Dict mapping each object file's relative path to its size in bytes. """ backend = get_store_backend(store_name, config=config) stored: dict[str, int] = {} try: - # Walk the storage looking for schema-addressed paths + # Walk the storage collecting schema-addressed object files full_prefix = backend._full_path("") for root, dirs, files in backend.fs.walk(full_prefix): @@ -408,26 +418,22 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i if "_hash" in root: continue - # Look for schema-addressed pattern (has files, not in _hash) - # Schema-addressed paths: {schema}/{table}/{pk}/{field}/ - relative_path = root.replace(full_prefix, "").lstrip("/") + for filename in files: + # Skip manifest sidecars (mirrors list_stored_hashes) + if filename.endswith(".manifest.json"): + continue - # Skip empty paths and root-level directories - if not relative_path or relative_path.count("/") < 2: - continue + file_path = f"{root}/{filename}" + relative_path = file_path.replace(full_prefix, "").lstrip("/") + + # Schema-addressed files live at least at {schema}/{table}/.../{file} + if relative_path.count("/") < 2: + continue - # Calculate total size of this directory - total_size = 0 - for file in files: try: - file_path = f"{root}/{file}" - total_size += backend.fs.size(file_path) + stored[relative_path] = backend.fs.size(file_path) except Exception: - pass - - # Only count directories with files (actual objects) - if total_size > 0 or files: - stored[relative_path] = total_size + stored[relative_path] = 0 except FileNotFoundError: pass @@ -439,12 +445,17 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i def delete_schema_path(path: str, store_name: str | None = None, config=None) -> bool: """ - Delete a schema-addressed directory from storage. + Delete a schema-addressed object file from storage. + + ``path`` is an individual object file (as enumerated by + :func:`list_schema_paths`), not a directory — so only the orphaned token + file is removed, leaving other versions/fields under the same primary-key + directory intact. Empty parent directories are pruned best-effort. Parameters ---------- path : str - Storage path (relative to store root). + Object file path (relative to store root). store_name : str, optional Store name (None = default store). config : Config, optional @@ -460,12 +471,25 @@ def delete_schema_path(path: str, store_name: str | None = None, config=None) -> try: full_path = backend._full_path(path) if backend.fs.exists(full_path): - # Remove entire directory tree - backend.fs.rm(full_path, recursive=True) - logger.debug(f"Deleted schema path: {path}") + # Remove just this object file (not the whole PK directory) + backend.fs.rm(full_path) + logger.debug(f"Deleted schema object: {path}") + + # Best-effort: prune now-empty parent directories up to the store root. + root = backend._full_path("").rstrip("/") + parent = full_path.rsplit("/", 1)[0] + while parent and parent != root and parent.startswith(root): + try: + if backend.fs.ls(parent): + break # not empty + backend.fs.rmdir(parent) + except Exception: + break + parent = parent.rsplit("/", 1)[0] + return True except Exception as e: - logger.warning(f"Error deleting schema path {path}: {e}") + logger.warning(f"Error deleting schema object {path}: {e}") return False diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index c9ea741bd..e0d22f86a 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -9,9 +9,21 @@ import datajoint as dj from datajoint import gc +from datajoint.builtin_codecs.object import ObjectCodec +from datajoint.codecs import get_codec from datajoint.errors import DataJointError +class GcCustomObjectCodec(ObjectCodec): + """A custom schema-addressed codec — a ``SchemaCodec`` subclass with a + non-built-in name, mirroring the user's NetCDF codec in #1469. Registered + at import via ``Codec.__init_subclass__``. GC must recognize it by type + (not by hardcoded name) and treat its files as referenced, not orphaned. + """ + + name = "gc_custom_object" + + # Tables used by TestScanWithLiveData. Defined at module scope so dj.Schema's # context resolution can find them by class name; bound to a schema inside # each fixture (see schema(...) calls below). @@ -41,6 +53,14 @@ class GcObjectTest(dj.Manual): """ +class GcCustomCodecTest(dj.Manual): + definition = """ + rid : int + --- + payload : + """ + + class TestUsesHashStorage: """Tests for _uses_hash_storage helper function.""" @@ -133,24 +153,30 @@ def test_returns_false_for_no_adapter(self): def test_returns_true_for_object_type(self): """Test that True is returned for type.""" attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "object" + attr.codec = get_codec("object") assert gc._uses_schema_storage(attr) is True def test_returns_true_for_npy_type(self): """Test that True is returned for type.""" attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "npy" + attr.codec = get_codec("npy") + + assert gc._uses_schema_storage(attr) is True + + def test_returns_true_for_custom_schema_subclass(self): + """Recognition is by type, not name: a custom SchemaCodec subclass + (here, a subclass of ObjectCodec) must be seen as schema-addressed so + GC does not misclassify its live files as orphans (#1469).""" + attr = MagicMock() + attr.codec = get_codec("gc_custom_object") assert gc._uses_schema_storage(attr) is True def test_returns_false_for_other_types(self): """Test that False is returned for non-schema-addressed types.""" attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "blob" + attr.codec = get_codec("blob") assert gc._uses_schema_storage(attr) is False @@ -465,3 +491,57 @@ def test_scan_finds_active_object_reference(self, schema_object): stats = gc.scan(schema_object, store_name="local") assert stats["schema_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" + + @pytest.fixture + def schema_custom(self, connection_test, prefix, mock_stores): + schema_name = f"{prefix}_test_gc_e2e_custom" + schema = dj.Schema( + schema_name, + context={"GcCustomCodecTest": GcCustomCodecTest}, + connection=connection_test, + ) + schema(GcCustomCodecTest) + yield schema + schema.drop() + + def test_custom_codec_reference_not_orphaned(self, schema_custom): + """#1469: a live custom SchemaCodec value must be recognized as + referenced and its file path must NOT be flagged as an orphan. Before + the fix, _uses_schema_storage keyed on the hardcoded names object/npy, + so this codec was never scanned and its live file was reported orphaned. + + Asserts on the specific live path (not global counts) so it is robust to + other tests sharing the same ``local`` store. + """ + GcCustomCodecTest.insert1({"rid": 1, "payload": b"live-payload"}) + + refs = gc.scan_schema_references(schema_custom, store_name="local") + assert refs, "custom codec's live reference must be discovered (#1469)" + live_path = next(iter(refs)) + + stats = gc.scan(schema_custom, store_name="local") + assert live_path not in stats["orphaned_paths"], f"live custom-codec file wrongly flagged orphan: {live_path}" + + def test_custom_codec_survives_collect(self, schema_custom): + """#1469 end-to-end data-loss guard: collect() must delete only the + deleted row's file and keep the surviving row's file (checked by exact + path, robust to a shared store).""" + GcCustomCodecTest.insert1({"rid": 1, "payload": b"row-one"}) + GcCustomCodecTest.insert1({"rid": 2, "payload": b"row-two"}) + + refs_all = gc.scan_schema_references(schema_custom, store_name="local") + assert len(refs_all) == 2, f"both live rows should be referenced; got {refs_all}" + + # Delete one row — its file becomes a genuine orphan (delete-then-GC). + (GcCustomCodecTest & {"rid": 1}).delete(prompt=False) + + refs_live = gc.scan_schema_references(schema_custom, store_name="local") + assert len(refs_live) == 1, f"one live row should remain referenced; got {refs_live}" + live_path = next(iter(refs_live)) + deleted_paths = refs_all - refs_live + + gc.collect(schema_custom, store_name="local", dry_run=False) + + stored_after = set(gc.list_schema_paths("local", config=schema_custom.connection._config)) + assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)" + assert not (deleted_paths & stored_after), f"deleted row's orphan not reclaimed: {deleted_paths & stored_after}" From 6e89a7d7894e80b6abe2ae8142d19442f07bcd29 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 08:50:52 -0500 Subject: [PATCH 02/20] fix(gc): coverage-based orphan matching for directory-valued objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file-level orphan matching introduced earlier in this PR broke DIRECTORY-valued objects (e.g. Zarr stores via ): 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. --- src/datajoint/gc.py | 54 ++++++++++++++++++++------ tests/integration/test_gc.py | 73 ++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 11 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 80b862e93..a98b2dafd 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -387,12 +387,14 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i """ List all schema-addressed object files in storage. - Enumerates the individual object **files** written by schema-addressed - codecs, whose paths follow ``{schema}/{table}/{pk}/{field}_{token}[.ext]``. - Returning file paths (rather than the enclosing directory) is what lets - them match the file paths recorded in each row's stored metadata — the two - must be comparable for orphan detection, and per-token granularity lets - superseded versions be reclaimed while the current token is retained. + Enumerates the individual **files** written by schema-addressed codecs. + A single-file object is one file at ``{schema}/{table}/{pk}/{field}_{token}[.ext]``; + a directory-valued object (e.g. a Zarr store) is many files under that + path prefix, plus a ``{path}.manifest.json`` sidecar beside it — all are + listed. Orphan detection matches these files against each row's referenced + object path via :func:`_is_covered` (exact, ancestor-prefix, or + manifest-sidecar), so superseded per-token versions are reclaimable while + every file belonging to a live object — including its manifest — is kept. Parameters ---------- @@ -419,10 +421,11 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i continue for filename in files: - # Skip manifest sidecars (mirrors list_stored_hashes) - if filename.endswith(".manifest.json"): - continue - + # Manifest sidecars ({object}.manifest.json, written for + # folder-valued objects) are deliberately INCLUDED: they are + # owned by their object and must be reclaimed with it when the + # object is orphaned. _is_covered() keeps a live object's + # manifest out of the orphan set. file_path = f"{root}/{filename}" relative_path = file_path.replace(full_prefix, "").lstrip("/") @@ -494,6 +497,32 @@ def delete_schema_path(path: str, store_name: str | None = None, config=None) -> return False +def _is_covered(path: str, referenced: set[str]) -> bool: + """ + Return True if a stored file is accounted for by a referenced object path. + + A stored file is covered when: + + - it IS a referenced path (single-file object, e.g. ``field_token.npy``); + - it lies UNDER a referenced path (directory-valued object, e.g. a Zarr + store ``field_token/`` whose stored form is many chunk files); or + - it is the ``.manifest.json`` sidecar of a referenced object (written + alongside folder-valued objects). + + Uses O(path-depth) ancestor-prefix lookups against the referenced set. + """ + if path.endswith(".manifest.json"): + path = path[: -len(".manifest.json")] + if path in referenced: + return True + idx = path.rfind("/") + while idx > 0: + if path[:idx] in referenced: + return True + idx = path.rfind("/", 0, idx) + return False + + def scan( *schemas: "Schema", store_name: str | None = None, @@ -545,7 +574,10 @@ def scan( # --- Schema-addressed storage --- schema_paths_referenced = scan_schema_references(*schemas, store_name=store_name, verbose=verbose) schema_paths_stored = list_schema_paths(store_name, config=_config) - orphaned_paths = set(schema_paths_stored.keys()) - schema_paths_referenced + # Coverage, not exact set difference: a referenced path may be a + # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is many + # files under that prefix, plus a `.manifest.json` sidecar beside it. + orphaned_paths = {p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)} schema_paths_orphaned_bytes = sum(schema_paths_stored.get(p, 0) for p in orphaned_paths) return { diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index e0d22f86a..9d496da02 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -545,3 +545,76 @@ def test_custom_codec_survives_collect(self, schema_custom): stored_after = set(gc.list_schema_paths("local", config=schema_custom.connection._config)) assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)" assert not (deleted_paths & stored_after), f"deleted row's orphan not reclaimed: {deleted_paths & stored_after}" + + +class TestDirectoryObjects: + """Directory-valued objects (e.g. Zarr stores) — the referenced metadata + path is a directory PREFIX whose stored form is many chunk files plus a + `{path}.manifest.json` sidecar. Orphan matching must be coverage-based + (exact / ancestor-prefix / manifest), not exact set difference: with exact + matching, every chunk of a LIVE store is misclassified as an orphan and + collect() deletes live data.""" + + @pytest.fixture + def schema_dirobj(self, connection_test, prefix, mock_stores): + schema = dj.Schema( + f"{prefix}_test_gc_dirobj", + context={"GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + schema(GcObjectTest) + yield schema + schema.drop() + + @staticmethod + def _make_store_dir(tmp_path, name, n_chunks=3): + d = tmp_path / name + d.mkdir() + (d / ".zarray").write_bytes(b"{}") + for i in range(n_chunks): + (d / f"0.{i}").write_bytes(f"chunk{i}".encode()) + return d + + def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): + """A live folder object's chunk files and manifest must be covered by + its referenced prefix — none may appear in orphaned_paths.""" + GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "live.zarr"))}) + + refs = gc.scan_schema_references(schema_dirobj, store_name="local") + assert len(refs) == 1 + ref = next(iter(refs)) + + stored = gc.list_schema_paths("local", config=schema_dirobj.connection._config) + chunk_files = [p for p in stored if p.startswith(ref + "/")] + assert len(chunk_files) >= 4, f"expected the folder's files under {ref}; got {sorted(stored)}" + assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)" + + stats = gc.scan(schema_dirobj, store_name="local") + flagged = [p for p in stats["orphaned_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json"] + assert not flagged, f"live directory object misclassified as orphaned: {flagged}" + + def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path): + """After deleting one row, collect() must reclaim that folder object's + chunks AND manifest while leaving the surviving row's folder intact.""" + GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "gone.zarr"))}) + GcObjectTest.insert1({"rid": 2, "results": str(self._make_store_dir(tmp_path, "kept.zarr"))}) + + refs_all = gc.scan_schema_references(schema_dirobj, store_name="local") + assert len(refs_all) == 2 + + (GcObjectTest & {"rid": 1}).delete(prompt=False) + refs_live = gc.scan_schema_references(schema_dirobj, store_name="local") + assert len(refs_live) == 1 + live_ref = next(iter(refs_live)) + dead_ref = next(iter(refs_all - refs_live)) + + gc.collect(schema_dirobj, store_name="local", dry_run=False) + + stored_after = set(gc.list_schema_paths("local", config=schema_dirobj.connection._config)) + live_files = {p for p in stored_after if p.startswith(live_ref + "/")} + assert len(live_files) >= 4, f"live folder object lost files: {sorted(stored_after)}" + assert f"{live_ref}.manifest.json" in stored_after, "live object's manifest must survive collect()" + + dead_files = {p for p in stored_after if p == dead_ref or p.startswith(dead_ref + "/")} + assert not dead_files, f"orphaned folder object not reclaimed: {sorted(dead_files)}" + assert f"{dead_ref}.manifest.json" not in stored_after, "orphaned object's manifest must be reclaimed" From 4a898c4221221c1500adc3afe3283c4f94ae9c76 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 10:57:41 -0500 Subject: [PATCH 03/20] refactor(gc): remove dead _extract_hash_refs/_extract_schema_refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/gc.py | 72 ------------------------------------ tests/integration/test_gc.py | 61 ++++++++++++++---------------- 2 files changed, 27 insertions(+), 106 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index a98b2dafd..9545fd7db 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -36,7 +36,6 @@ from __future__ import annotations -import json import logging from typing import TYPE_CHECKING, Any @@ -116,77 +115,6 @@ def _uses_schema_storage(attr) -> bool: return isinstance(attr.codec, SchemaCodec) -def _extract_hash_refs(value: Any) -> list[tuple[str, str | None]]: - """ - Extract path references from hash-addressed storage metadata. - - Hash-addressed storage stores metadata as JSON with ``path`` and ``hash`` keys. - The path is used for file operations; the hash is for integrity verification. - - Parameters - ---------- - value : Any - The stored value (JSON string or dict). - - Returns - ------- - list[tuple[str, str | None]] - List of (path, store_name) tuples. - """ - refs = [] - - if value is None: - return refs - - # Parse JSON if string - if isinstance(value, str): - try: - value = json.loads(value) - except (json.JSONDecodeError, TypeError): - return refs - - # Extract path from dict (path is required for new data, hash for legacy) - if isinstance(value, dict) and "path" in value: - refs.append((value["path"], value.get("store"))) - - return refs - - -def _extract_schema_refs(value: Any) -> list[tuple[str, str | None]]: - """ - Extract schema-addressed path references from a stored value. - - Schema-addressed storage stores metadata as JSON with a ``path`` key. - - Parameters - ---------- - value : Any - The stored value (JSON string or dict). - - Returns - ------- - list[tuple[str, str | None]] - List of (path, store_name) tuples. - """ - refs = [] - - if value is None: - return refs - - # Parse JSON if string - if isinstance(value, str): - try: - value = json.loads(value) - except (json.JSONDecodeError, TypeError): - return refs - - # Extract path from dict - if isinstance(value, dict) and "path" in value: - refs.append((value["path"], value.get("store"))) - - return refs - - def scan_hash_references( *schemas: "Schema", store_name: str | None = None, diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 9d496da02..72dca0d03 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -108,36 +108,30 @@ def test_returns_false_for_blob_internal(self): assert gc._uses_hash_storage(attr) is False -class TestExtractHashRefs: - """Tests for _extract_hash_refs helper function.""" +class TestHashReferencedPaths: + """Reference extraction for hash-addressed metadata via the codec API + (Codec.referenced_paths — the production path used by gc.scan).""" def test_returns_empty_for_none(self): - """Test that empty list is returned for None value.""" - assert gc._extract_hash_refs(None) == [] + assert get_codec("hash").referenced_paths(None) == [] def test_parses_json_string(self): - """Test parsing JSON string with path.""" value = '{"path": "_hash/schema/abc123", "hash": "abc123", "store": "mystore"}' - refs = gc._extract_hash_refs(value) + refs = get_codec("hash").referenced_paths(value) - assert len(refs) == 1 - assert refs[0] == ("_hash/schema/abc123", "mystore") + assert refs == [("_hash/schema/abc123", "mystore")] def test_parses_dict_directly(self): - """Test parsing dict with path.""" value = {"path": "_hash/schema/def456", "hash": "def456", "store": None} - refs = gc._extract_hash_refs(value) + refs = get_codec("hash").referenced_paths(value) - assert len(refs) == 1 - assert refs[0] == ("_hash/schema/def456", None) + assert refs == [("_hash/schema/def456", None)] def test_returns_empty_for_invalid_json(self): - """Test that empty list is returned for invalid JSON.""" - assert gc._extract_hash_refs("not json") == [] + assert get_codec("hash").referenced_paths("not json") == [] def test_returns_empty_for_dict_without_path(self): - """Test that empty list is returned for dict without path key.""" - assert gc._extract_hash_refs({"hash": "abc123"}) == [] + assert get_codec("hash").referenced_paths({"hash": "abc123"}) == [] class TestUsesSchemaStorage: @@ -181,32 +175,31 @@ def test_returns_false_for_other_types(self): assert gc._uses_schema_storage(attr) is False -class TestExtractSchemaRefs: - """Tests for _extract_schema_refs helper function.""" +class TestSchemaReferencedPaths: + """Reference extraction for schema-addressed metadata via the codec API + (inherited by every SchemaCodec subclass, including custom codecs).""" def test_returns_empty_for_none(self): - """Test that empty list is returned for None value.""" - assert gc._extract_schema_refs(None) == [] + assert get_codec("object").referenced_paths(None) == [] def test_parses_json_string(self): - """Test parsing JSON string with path.""" value = '{"path": "schema/table/pk/field", "store": "mystore"}' - refs = gc._extract_schema_refs(value) + refs = get_codec("object").referenced_paths(value) - assert len(refs) == 1 - assert refs[0] == ("schema/table/pk/field", "mystore") + assert refs == [("schema/table/pk/field", "mystore")] def test_parses_dict_directly(self): - """Test parsing dict with path.""" - value = {"path": "test/path", "store": None} - refs = gc._extract_schema_refs(value) + refs = get_codec("object").referenced_paths({"path": "test/path", "store": None}) - assert len(refs) == 1 - assert refs[0] == ("test/path", None) + assert refs == [("test/path", None)] + + def test_custom_subclass_inherits_extraction(self): + refs = get_codec("gc_custom_object").referenced_paths({"path": "a/b/c", "store": "local"}) + + assert refs == [("a/b/c", "local")] def test_returns_empty_for_dict_without_path(self): - """Test that empty list is returned for dict without path key.""" - assert gc._extract_schema_refs({"other": "data"}) == [] + assert get_codec("object").referenced_paths({"other": "data"}) == [] class TestScan: @@ -457,7 +450,7 @@ def test_scan_finds_active_blob_reference(self, schema_blob): """scan() must report hash_referenced >= 1 for a populated column. Decoded value type returned by BlobCodec.decode is numpy.ndarray, which - does not satisfy `_extract_hash_refs`'s dict/JSON-string check — this + does not satisfy the raw-metadata dict/JSON-string check in referenced_paths — this test fails before the cursor-based fix in scan_hash_references. """ GcBlobTest.insert1({"rid": 1, "payload": np.arange(64, dtype="uint8")}) @@ -470,7 +463,7 @@ def test_scan_finds_active_npy_reference(self, schema_npy): """scan() must report schema_paths_referenced >= 1 for a populated column. Decoded value type returned by NpyCodec.decode is NpyRef (lazy handle), - which does not satisfy `_extract_schema_refs`'s dict check — this test + which does not satisfy the raw-metadata dict check in referenced_paths — this test fails before the cursor-based fix in scan_schema_references. """ GcNpyTest.insert1({"rid": 1, "waveform": np.arange(64, dtype="float32")}) @@ -483,7 +476,7 @@ def test_scan_finds_active_object_reference(self, schema_object): """scan() must report schema_paths_referenced >= 1 for a populated column. Decoded value type returned by ObjectCodec.decode is ObjectRef (lazy - handle), which does not satisfy `_extract_schema_refs`'s dict check — + handle), which does not satisfy the raw-metadata dict check in referenced_paths — this test fails before the cursor-based fix in scan_schema_references. """ GcObjectTest.insert1({"rid": 1, "results": b"hello-gc-test"}) From 49bdbb0541603a7eae654d1a651e9ef83918ef9c Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 11:10:48 -0500 Subject: [PATCH 04/20] fix(gc): hash-subtree skip by first path segment; share prefix constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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. --- src/datajoint/gc.py | 12 +++++--- src/datajoint/hash_registry.py | 13 +++++++-- tests/integration/test_gc.py | 51 ++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 9545fd7db..2f2f6f216 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -39,7 +39,7 @@ import logging from typing import TYPE_CHECKING, Any -from .hash_registry import delete_path, get_store_backend +from .hash_registry import HASH_STORAGE_PREFIX, delete_path, get_store_backend from .errors import DataJointError if TYPE_CHECKING: @@ -274,7 +274,7 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, stored: dict[str, int] = {} # Hash-addressed storage: _hash/{schema}/{subfolders...}/{hash} - hash_prefix = "_hash/" + hash_prefix = f"{HASH_STORAGE_PREFIX}/" # Base32 pattern: 26 lowercase alphanumeric chars base32_pattern = re.compile(r"^[a-z2-7]{26}$") @@ -344,8 +344,12 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i full_prefix = backend._full_path("") for root, dirs, files in backend.fs.walk(full_prefix): - # Skip _hash directory (hash-addressed storage) - if "_hash" in root: + # Skip the hash-addressed storage subtree. Match on the FIRST + # store-relative path segment only — a substring test would also + # exclude user tables/schemas whose names merely contain "_hash" + # (e.g. a table `probe_hash`), silently leaking their orphans. + rel_root = root.replace(full_prefix, "").lstrip("/") + if rel_root == HASH_STORAGE_PREFIX or rel_root.startswith(HASH_STORAGE_PREFIX + "/"): continue for filename in files: diff --git a/src/datajoint/hash_registry.py b/src/datajoint/hash_registry.py index d33c916ba..25e775bdd 100644 --- a/src/datajoint/hash_registry.py +++ b/src/datajoint/hash_registry.py @@ -48,6 +48,15 @@ logger = logging.getLogger(__name__.split(".")[0]) +# Fixed layout prefix for hash-addressed storage within a store. This is a +# storage-layout CONVENTION, not a configurable setting: the `hash_prefix` +# store key accepted by settings currently serves only as a reserved-namespace +# declaration for validation (see builtin_codecs/filepath.py) and +# is NOT consumed here. GC imports this constant so the scanner can never +# drift from the writer. +HASH_STORAGE_PREFIX = "_hash" + + def compute_hash(data: bytes) -> str: """ Compute Base32-encoded MD5 hash of content. @@ -130,9 +139,9 @@ def build_hash_path( if subfolding: folds = _subfold(content_hash, subfolding) fold_path = "/".join(folds) - return f"_hash/{schema_name}/{fold_path}/{content_hash}" + return f"{HASH_STORAGE_PREFIX}/{schema_name}/{fold_path}/{content_hash}" else: - return f"_hash/{schema_name}/{content_hash}" + return f"{HASH_STORAGE_PREFIX}/{schema_name}/{content_hash}" def get_store_backend(store_name: str | None = None, config: Config | None = None) -> StorageBackend: diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 72dca0d03..ec8e96671 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -611,3 +611,54 @@ def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path dead_files = {p for p in stored_after if p == dead_ref or p.startswith(dead_ref + "/")} assert not dead_files, f"orphaned folder object not reclaimed: {sorted(dead_files)}" assert f"{dead_ref}.manifest.json" not in stored_after, "orphaned object's manifest must be reclaimed" + + +class GcProbeHash(dj.Manual): + """Table whose name (`gc_probe_hash`) contains the substring `_hash` — + regression guard for the schema-path walk's hash-subtree skip.""" + + definition = """ + rid : int + --- + results : + """ + + +class TestHashSubstringTableName: + """The hash-addressed subtree skip must match the first store-relative + path segment exactly (`_hash/...`), not any path containing the substring + `_hash` — otherwise tables like `gc_probe_hash` are silently excluded from + the schema-addressed listing and their orphans are never reclaimed.""" + + @pytest.fixture + def schema_probe(self, connection_test, prefix, mock_stores): + schema = dj.Schema( + f"{prefix}_test_gc_hashname", + context={"GcProbeHash": GcProbeHash}, + connection=connection_test, + ) + schema(GcProbeHash) + yield schema + schema.drop() + + def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): + GcProbeHash.insert1({"rid": 1, "results": b"keep"}) + GcProbeHash.insert1({"rid": 2, "results": b"drop"}) + + refs = gc.scan_schema_references(schema_probe, store_name="local") + assert len(refs) == 2 + stored = set(gc.list_schema_paths("local", config=schema_probe.connection._config)) + missing = refs - stored + assert not missing, ( + f"files of a table whose name contains '_hash' must be listed; missing {missing} " + "(substring-based skip would hide them from GC forever)" + ) + + (GcProbeHash & {"rid": 2}).delete(prompt=False) + refs_live = gc.scan_schema_references(schema_probe, store_name="local") + dead_ref = next(iter(refs - refs_live)) + + gc.collect(schema_probe, store_name="local", dry_run=False) + stored_after = set(gc.list_schema_paths("local", config=schema_probe.connection._config)) + assert dead_ref not in stored_after, "orphan under a '_hash'-substring table name must be reclaimed" + assert next(iter(refs_live)) in stored_after, "live file must survive" From 068b365e5212ea5672a2f2f27130b4b78562f9a4 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 11:18:10 -0500 Subject: [PATCH 05/20] fix(gc): honor filepath_prefix as a protected namespace; re-document prefix keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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. --- src/datajoint/gc.py | 17 +++++++++++++ src/datajoint/hash_registry.py | 12 ++++++---- src/datajoint/settings.py | 8 +++++++ tests/integration/test_gc.py | 44 ++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 2f2f6f216..54930c163 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -339,6 +339,19 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i backend = get_store_backend(store_name, config=config) stored: dict[str, int] = {} + # A store may declare a `filepath_prefix` — the namespace reserved for + # user-managed content. Files under it are NOT DataJoint-owned + # objects and must never be candidates for garbage collection. (Without a + # declared filepath_prefix, cohabiting filepath files are indistinguishable + # from orphans — see the fail-safe planned with the two-phase GC work.) + if config is None: + from .settings import config as _global_config + + config = _global_config + _spec = config.get_store_spec(store_name) + _fp = _spec.get("filepath_prefix") + filepath_prefix = (_fp.strip("/") + "/") if _fp else None + try: # Walk the storage collecting schema-addressed object files full_prefix = backend._full_path("") @@ -352,6 +365,10 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i if rel_root == HASH_STORAGE_PREFIX or rel_root.startswith(HASH_STORAGE_PREFIX + "/"): continue + # Skip the declared filepath namespace (user-managed files). + if filepath_prefix and (rel_root + "/").startswith(filepath_prefix): + continue + for filename in files: # Manifest sidecars ({object}.manifest.json, written for # folder-valued objects) are deliberately INCLUDED: they are diff --git a/src/datajoint/hash_registry.py b/src/datajoint/hash_registry.py index 25e775bdd..6ea627fb5 100644 --- a/src/datajoint/hash_registry.py +++ b/src/datajoint/hash_registry.py @@ -49,11 +49,13 @@ # Fixed layout prefix for hash-addressed storage within a store. This is a -# storage-layout CONVENTION, not a configurable setting: the `hash_prefix` -# store key accepted by settings currently serves only as a reserved-namespace -# declaration for validation (see builtin_codecs/filepath.py) and -# is NOT consumed here. GC imports this constant so the scanner can never -# drift from the writer. +# storage-layout CONVENTION, not a relocation setting. The `hash_prefix` / +# `schema_prefix` / `filepath_prefix` store keys accepted by settings are +# NAMESPACE DECLARATIONS for store cohabitation: validation keeps +# user files out of the reserved namespaces (builtin_codecs/filepath.py), and +# GC keeps its hands off the declared filepath namespace (gc.list_schema_paths). +# They do not relocate DataJoint-managed storage. GC imports this constant so +# the scanner can never drift from the writer. HASH_STORAGE_PREFIX = "_hash" diff --git a/src/datajoint/settings.py b/src/datajoint/settings.py index 6ae23478b..f0489932d 100644 --- a/src/datajoint/settings.py +++ b/src/datajoint/settings.py @@ -571,6 +571,14 @@ def _validate_prefix_separation( """ Validate that storage section prefixes don't overlap. + These keys are **namespace declarations** for store cohabitation, not + relocation settings: DataJoint's hash-addressed layout is fixed at + ``_hash/...`` and schema-addressed objects live at + ``{schema}/{table}/...`` regardless of these values. Declaring them + (a) lets ```` validation keep user-managed files out of the + DataJoint-owned namespaces, and (b) tells garbage collection which + subtree (``filepath_prefix``) it must never touch. + Parameters ---------- store_name : str diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index ec8e96671..5faba3406 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -662,3 +662,47 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): stored_after = set(gc.list_schema_paths("local", config=schema_probe.connection._config)) assert dead_ref not in stored_after, "orphan under a '_hash'-substring table name must be reclaimed" assert next(iter(refs_live)) in stored_after, "live file must survive" + + +class TestFilepathPrefixProtection: + """A store's declared `filepath_prefix` is the user-managed + namespace: GC must never list (and therefore never collect) files under + it. Without a declaration, cohabiting user files remain indistinguishable + from orphans (documented hazard; fail-safe planned with two-phase GC).""" + + @pytest.fixture + def schema_fp(self, connection_test, prefix, mock_stores): + schema = dj.Schema( + f"{prefix}_test_gc_fpprefix", + context={"GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + schema(GcObjectTest) + yield schema + schema.drop() + + def test_filepath_prefix_subtree_never_collected(self, schema_fp): + from pathlib import Path + + cfg = schema_fp.connection._config + location = Path(cfg.get_store_spec("local")["location"]) + user_file = location / "userfiles" / "deep" / "important.bin" + user_file.parent.mkdir(parents=True, exist_ok=True) + user_file.write_bytes(b"user-managed") + + GcObjectTest.insert1({"rid": 1, "results": b"managed-object"}) + + # Without a declared prefix, the stray user file IS treated as an + # orphan candidate (the documented cohabitation hazard). + stored_undeclared = gc.list_schema_paths("local", config=cfg) + assert "userfiles/deep/important.bin" in stored_undeclared + + cfg.stores["local"]["filepath_prefix"] = "userfiles" + try: + stored = gc.list_schema_paths("local", config=cfg) + assert "userfiles/deep/important.bin" not in stored, "declared filepath namespace must be excluded" + + gc.collect(schema_fp, store_name="local", dry_run=False) + assert user_file.exists(), "collect() must never touch the declared filepath namespace" + finally: + cfg.stores["local"].pop("filepath_prefix", None) From 6e139a3b99daf9e50f102a8bc9ceb8e2c0177bcf Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 11:24:09 -0500 Subject: [PATCH 06/20] fix(filepath): keep the fixed _hash layout reserved when hash_prefix is overridden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/builtin_codecs/filepath.py | 8 ++++- src/datajoint/settings.py | 10 ++++-- tests/unit/test_codecs.py | 40 ++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/datajoint/builtin_codecs/filepath.py b/src/datajoint/builtin_codecs/filepath.py index 034d5b53a..a58592668 100644 --- a/src/datajoint/builtin_codecs/filepath.py +++ b/src/datajoint/builtin_codecs/filepath.py @@ -113,7 +113,13 @@ def encode(self, value: Any, *, key: dict | None = None, store_name: str | None # Validate path doesn't use reserved sections (hash and schema) path_normalized = path.lstrip("/") - reserved_prefixes = [] + + # The fixed hash-addressed layout is ALWAYS reserved, independent of + # any declaration — actual hash objects live under HASH_STORAGE_PREFIX + # regardless of the (declarative) hash_prefix store key. + from ..hash_registry import HASH_STORAGE_PREFIX + + reserved_prefixes = [("hash storage (fixed layout)", HASH_STORAGE_PREFIX)] hash_prefix = spec.get("hash_prefix") if hash_prefix: diff --git a/src/datajoint/settings.py b/src/datajoint/settings.py index f0489932d..198a4ce22 100644 --- a/src/datajoint/settings.py +++ b/src/datajoint/settings.py @@ -575,9 +575,13 @@ def _validate_prefix_separation( relocation settings: DataJoint's hash-addressed layout is fixed at ``_hash/...`` and schema-addressed objects live at ``{schema}/{table}/...`` regardless of these values. Declaring them - (a) lets ```` validation keep user-managed files out of the - DataJoint-owned namespaces, and (b) tells garbage collection which - subtree (``filepath_prefix``) it must never touch. + (a) lets ```` validation reject user paths under the + declared namespaces — ``hash_prefix`` defaults to ``_hash`` (matching + the fixed layout, which stays reserved even if the key is overridden), + while ``schema_prefix``'s default ``_schema`` is vestigial: + schema-addressed objects actually live in root-level ``{schema}/`` + directories — and (b) tells garbage collection which subtree + (``filepath_prefix``) it must never touch. Parameters ---------- diff --git a/tests/unit/test_codecs.py b/tests/unit/test_codecs.py index 56445419d..2de9b3465 100644 --- a/tests/unit/test_codecs.py +++ b/tests/unit/test_codecs.py @@ -484,6 +484,46 @@ def test_filepath_rejects_hash_section(self): dj.config.stores.clear() dj.config.stores.update(original_stores) + def test_filepath_rejects_hash_layout_even_when_prefix_overridden(self): + """Overriding hash_prefix must NOT un-reserve the FIXED _hash/ layout: + the store key is declarative (it does not relocate hash storage), so + the true namespace stays protected regardless of the override.""" + from unittest.mock import MagicMock, patch + + import datajoint as dj + + filepath_codec = get_codec("filepath") + + original_stores = dj.config.stores.copy() + try: + dj.config.stores["test_store"] = { + "protocol": "file", + "location": "/tmp/test", + "hash_prefix": "content", # override; hash objects still land in _hash/ + } + + with patch("datajoint.hash_registry.get_store_backend") as mock_get_backend: + mock_backend = MagicMock() + mock_backend.exists.return_value = True + mock_get_backend.return_value = mock_backend + + # the fixed layout remains reserved + with pytest.raises( + ValueError, + match=r" cannot use reserved section '_hash'", + ): + filepath_codec.encode("_hash/schema/file.dat", store_name="test_store") + + # the declared override is reserved too + with pytest.raises( + ValueError, + match=r" cannot use reserved section 'content'", + ): + filepath_codec.encode("content/file.dat", store_name="test_store") + finally: + dj.config.stores.clear() + dj.config.stores.update(original_stores) + def test_filepath_rejects_schema_section(self): """Test that filepath rejects paths starting with default schema prefix.""" from unittest.mock import MagicMock, patch From c74a83095a0d1ac7dd9ee81c621c979586d40230 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 12:07:12 -0500 Subject: [PATCH 07/20] fix(storage): honor hash_prefix/schema_prefix settings in writers and GC 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. --- src/datajoint/builtin_codecs/filepath.py | 8 +--- src/datajoint/builtin_codecs/schema.py | 4 +- src/datajoint/gc.py | 32 +++++++++++---- src/datajoint/hash_registry.py | 46 ++++++++++++++-------- src/datajoint/settings.py | 23 +++++------ src/datajoint/staged_insert.py | 1 + src/datajoint/storage.py | 15 ++++++- tests/integration/test_gc.py | 50 ++++++++++++++++++++++++ tests/integration/test_hash_storage.py | 35 +++++++++++------ tests/integration/test_object.py | 3 +- tests/unit/test_codecs.py | 22 +++++------ 11 files changed, 171 insertions(+), 68 deletions(-) diff --git a/src/datajoint/builtin_codecs/filepath.py b/src/datajoint/builtin_codecs/filepath.py index a58592668..034d5b53a 100644 --- a/src/datajoint/builtin_codecs/filepath.py +++ b/src/datajoint/builtin_codecs/filepath.py @@ -113,13 +113,7 @@ def encode(self, value: Any, *, key: dict | None = None, store_name: str | None # Validate path doesn't use reserved sections (hash and schema) path_normalized = path.lstrip("/") - - # The fixed hash-addressed layout is ALWAYS reserved, independent of - # any declaration — actual hash objects live under HASH_STORAGE_PREFIX - # regardless of the (declarative) hash_prefix store key. - from ..hash_registry import HASH_STORAGE_PREFIX - - reserved_prefixes = [("hash storage (fixed layout)", HASH_STORAGE_PREFIX)] + reserved_prefixes = [] hash_prefix = spec.get("hash_prefix") if hash_prefix: diff --git a/src/datajoint/builtin_codecs/schema.py b/src/datajoint/builtin_codecs/schema.py index c8cc0759d..d8bba0150 100644 --- a/src/datajoint/builtin_codecs/schema.py +++ b/src/datajoint/builtin_codecs/schema.py @@ -114,7 +114,7 @@ def _build_path( Build schema-addressed storage path. Constructs a path that mirrors the database schema structure: - ``{schema}/{table}/{pk_values}/{field}{ext}`` + ``{schema_prefix}/{schema}/{table}/{pk_values}/{field}{ext}`` Supports partitioning if configured in the store. @@ -150,6 +150,7 @@ def _build_path( spec = config.get_store_spec(store_name) partition_pattern = spec.get("partition_pattern") token_length = spec.get("token_length", 8) + schema_prefix = spec.get("schema_prefix", "_schema") return build_object_path( schema=schema, @@ -159,6 +160,7 @@ def _build_path( ext=ext, partition_pattern=partition_pattern, token_length=token_length, + schema_prefix=schema_prefix, ) def _get_backend(self, store_name: str | None = None, config=None): diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 54930c163..59acdc19f 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -9,13 +9,15 @@ Hash-addressed storage Types: ````, ````, ```` - Path: ``_hash/{schema}/{hash}`` (with optional subfolding) + Path: ``{hash_prefix}/{schema}/{hash}`` (with optional subfolding; + ``hash_prefix`` defaults to ``_hash``) Deduplication: Per-schema (identical data within a schema shares storage) Deletion: Requires garbage collection Schema-addressed storage Types: ````, ```` - Path: ``{schema}/{table}/{pk}/{field}/`` + Path: ``{schema_prefix}/{schema}/{table}/{pk}/...`` (``schema_prefix`` + defaults to ``_schema``; pre-2.3.1 stores hold root-level ``{schema}/...`` paths) Deduplication: None (each entity has unique path) Deletion: Requires garbage collection @@ -39,7 +41,7 @@ import logging from typing import TYPE_CHECKING, Any -from .hash_registry import HASH_STORAGE_PREFIX, delete_path, get_store_backend +from .hash_registry import DEFAULT_HASH_PREFIX, delete_path, get_store_backend from .errors import DataJointError if TYPE_CHECKING: @@ -273,8 +275,18 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, backend = get_store_backend(store_name, config=config) stored: dict[str, int] = {} - # Hash-addressed storage: _hash/{schema}/{subfolders...}/{hash} - hash_prefix = f"{HASH_STORAGE_PREFIX}/" + if config is None: + from .settings import config as _global_config + + config = _global_config + # Hash-addressed storage: {hash_prefix}/{schema}/{subfolders...}/{hash}. + # The prefix comes from the store's settings — the same value the writer + # uses — so scanner and writer cannot drift. NOTE: only the CURRENTLY + # configured prefix is scanned; objects written under a previous prefix + # value remain readable (their metadata stores full paths) but are not + # candidates for reclamation until the setting is restored. + _spec = config.get_store_spec(store_name) + hash_prefix = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/") + "/" # Base32 pattern: 26 lowercase alphanumeric chars base32_pattern = re.compile(r"^[a-z2-7]{26}$") @@ -351,9 +363,15 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i _spec = config.get_store_spec(store_name) _fp = _spec.get("filepath_prefix") filepath_prefix = (_fp.strip("/") + "/") if _fp else None + _hp = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/") + hash_section = _hp + "/" if _hp else None try: - # Walk the storage collecting schema-addressed object files + # Walk the storage collecting schema-addressed object files. The walk + # covers the WHOLE store (minus the hash and filepath sections) rather + # than just the schema_prefix section: stores written before + # schema_prefix was honored hold objects at root-level {schema}/... + # paths, and those must remain listable and reclaimable. full_prefix = backend._full_path("") for root, dirs, files in backend.fs.walk(full_prefix): @@ -362,7 +380,7 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i # exclude user tables/schemas whose names merely contain "_hash" # (e.g. a table `probe_hash`), silently leaking their orphans. rel_root = root.replace(full_prefix, "").lstrip("/") - if rel_root == HASH_STORAGE_PREFIX or rel_root.startswith(HASH_STORAGE_PREFIX + "/"): + if hash_section and (rel_root + "/").startswith(hash_section): continue # Skip the declared filepath namespace (user-managed files). diff --git a/src/datajoint/hash_registry.py b/src/datajoint/hash_registry.py index 6ea627fb5..ccbbcd26a 100644 --- a/src/datajoint/hash_registry.py +++ b/src/datajoint/hash_registry.py @@ -5,11 +5,11 @@ codec. Content is identified by a Base32-encoded MD5 hash and stored with per-schema isolation:: - _hash/{schema}/{hash} + {hash_prefix}/{schema}/{hash} (hash_prefix defaults to _hash) With optional subfolding (configured per-store):: - _hash/{schema}/{fold1}/{fold2}/{hash} + {hash_prefix}/{schema}/{fold1}/{fold2}/{hash} Subfolding creates directory hierarchies to improve performance on filesystems that struggle with large directories (ext3, FAT32, NFS). Modern filesystems @@ -48,15 +48,12 @@ logger = logging.getLogger(__name__.split(".")[0]) -# Fixed layout prefix for hash-addressed storage within a store. This is a -# storage-layout CONVENTION, not a relocation setting. The `hash_prefix` / -# `schema_prefix` / `filepath_prefix` store keys accepted by settings are -# NAMESPACE DECLARATIONS for store cohabitation: validation keeps -# user files out of the reserved namespaces (builtin_codecs/filepath.py), and -# GC keeps its hands off the declared filepath namespace (gc.list_schema_paths). -# They do not relocate DataJoint-managed storage. GC imports this constant so -# the scanner can never drift from the writer. -HASH_STORAGE_PREFIX = "_hash" +# Default section prefix for hash-addressed storage. The authoritative value +# is the per-store `hash_prefix` setting (default "_hash", see +# settings.get_store_spec and docs/how-to/configure-storage) — writers, GC, +# and validation all consume the same setting so the layout can +# be relocated per store without the components drifting apart. +DEFAULT_HASH_PREFIX = "_hash" def compute_hash(data: bytes) -> str: @@ -108,17 +105,18 @@ def build_hash_path( content_hash: str, schema_name: str, subfolding: tuple[int, ...] | None = None, + hash_prefix: str = DEFAULT_HASH_PREFIX, ) -> str: """ Build the storage path for hash-addressed storage. Path structure without subfolding:: - _hash/{schema}/{hash} + {hash_prefix}/{schema}/{hash} (hash_prefix defaults to _hash) Path structure with subfolding (e.g., (2, 2)):: - _hash/{schema}/{fold1}/{fold2}/{hash} + {hash_prefix}/{schema}/{fold1}/{fold2}/{hash} Parameters ---------- @@ -128,6 +126,9 @@ def build_hash_path( Database/schema name for isolation. subfolding : tuple[int, ...], optional Subfolding pattern from store config. None means flat (no subfolding). + hash_prefix : str, optional + Section prefix from the store's ``hash_prefix`` setting + (default ``"_hash"``). Returns ------- @@ -138,12 +139,14 @@ def build_hash_path( if not (len(content_hash) == 26 and content_hash.isalnum() and content_hash.islower()): raise DataJointError(f"Invalid content hash (expected 26-char lowercase base32): {content_hash}") + prefix = hash_prefix.strip("/") + section = f"{prefix}/" if prefix else "" if subfolding: folds = _subfold(content_hash, subfolding) fold_path = "/".join(folds) - return f"{HASH_STORAGE_PREFIX}/{schema_name}/{fold_path}/{content_hash}" + return f"{section}{schema_name}/{fold_path}/{content_hash}" else: - return f"{HASH_STORAGE_PREFIX}/{schema_name}/{content_hash}" + return f"{section}{schema_name}/{content_hash}" def get_store_backend(store_name: str | None = None, config: Config | None = None) -> StorageBackend: @@ -228,8 +231,17 @@ def put_hash( Metadata dict with keys: hash, path, schema, store, size. """ content_hash = compute_hash(data) - subfolding = get_store_subfolding(store_name, config=config) - path = build_hash_path(content_hash, schema_name, subfolding) + if config is None: + from .settings import config # type: ignore[assignment] + assert config is not None + spec = config.get_store_spec(store_name) + subfolding = tuple(spec["subfolding"]) if spec.get("subfolding") else None + path = build_hash_path( + content_hash, + schema_name, + subfolding, + hash_prefix=spec.get("hash_prefix", DEFAULT_HASH_PREFIX), + ) backend = get_store_backend(store_name, config=config) diff --git a/src/datajoint/settings.py b/src/datajoint/settings.py index 198a4ce22..40167697e 100644 --- a/src/datajoint/settings.py +++ b/src/datajoint/settings.py @@ -571,17 +571,18 @@ def _validate_prefix_separation( """ Validate that storage section prefixes don't overlap. - These keys are **namespace declarations** for store cohabitation, not - relocation settings: DataJoint's hash-addressed layout is fixed at - ``_hash/...`` and schema-addressed objects live at - ``{schema}/{table}/...`` regardless of these values. Declaring them - (a) lets ```` validation reject user paths under the - declared namespaces — ``hash_prefix`` defaults to ``_hash`` (matching - the fixed layout, which stays reserved even if the key is overridden), - while ``schema_prefix``'s default ``_schema`` is vestigial: - schema-addressed objects actually live in root-level ``{schema}/`` - directories — and (b) tells garbage collection which subtree - (``filepath_prefix``) it must never touch. + These keys **control the store layout** (see + how-to/configure-storage in the documentation): ``hash_prefix`` + (default ``_hash``) is where hash-addressed objects are written and + scanned; ``schema_prefix`` (default ``_schema``) is where + schema-addressed objects are written; ``filepath_prefix`` (default + unrestricted) is the namespace ```` paths must stay in. + Writers, garbage collection, and ```` validation all + consume the same values, so the sections may be relocated per store + without the components drifting apart. Changing a prefix on a store + that already holds data is not recommended: existing objects remain + readable (their metadata stores full paths) but reclamation scans + only the currently configured sections. Parameters ---------- diff --git a/src/datajoint/staged_insert.py b/src/datajoint/staged_insert.py index ffbe8a8f2..b2d601539 100644 --- a/src/datajoint/staged_insert.py +++ b/src/datajoint/staged_insert.py @@ -120,6 +120,7 @@ def _resolve_field(self, field: str, ext: str) -> tuple[str, "StorageBackend"]: ext=ext if ext else None, partition_pattern=partition_pattern, token_length=token_length, + schema_prefix=spec.get("schema_prefix", "_schema"), ) self._staged_objects[field] = { diff --git a/src/datajoint/storage.py b/src/datajoint/storage.py index 6a8260163..d1290e760 100644 --- a/src/datajoint/storage.py +++ b/src/datajoint/storage.py @@ -188,6 +188,12 @@ def encode_pk_value(value: Any) -> str: return s +# Default section prefix for schema-addressed storage. The authoritative value +# is the per-store `schema_prefix` setting (default "_schema", see +# settings.get_store_spec and docs/how-to/configure-storage). +DEFAULT_SCHEMA_PREFIX = "_schema" + + def build_object_path( schema: str, table: str, @@ -196,6 +202,7 @@ def build_object_path( ext: str | None, partition_pattern: str | None = None, token_length: int = 8, + schema_prefix: str = DEFAULT_SCHEMA_PREFIX, ) -> tuple[str, str]: """ Build the storage path for an object attribute. @@ -216,6 +223,9 @@ def build_object_path( Partition pattern with ``{attr}`` placeholders. token_length : int, optional Length of random token suffix. Default 8. + schema_prefix : str, optional + Section prefix from the store's ``schema_prefix`` setting + (default ``"_schema"``). Returns ------- @@ -256,8 +266,11 @@ def build_object_path( pk_parts.append(f"{attr}={encode_pk_value(value)}") # Construct full path - # Pattern: {partition_attrs}/{schema}/{table}/{remaining_pk}/{filename} + # Pattern: {schema_prefix}/{partition_attrs}/{schema}/{table}/{remaining_pk}/{filename} parts = [] + prefix = schema_prefix.strip("/") + if prefix: + parts.append(prefix) if partition_parts: parts.extend(partition_parts) parts.append(schema) diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 5faba3406..789b48028 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -706,3 +706,53 @@ def test_filepath_prefix_subtree_never_collected(self, schema_fp): assert user_file.exists(), "collect() must never touch the declared filepath namespace" finally: cfg.stores["local"].pop("filepath_prefix", None) + + +class TestPrefixSettingsHonored: + """The per-store hash_prefix/schema_prefix settings control the layout + (docs: how-to/configure-storage): writers place objects under the + configured sections and GC scans the same sections — relocation without + drift between components.""" + + @pytest.fixture + def schema_prefixed(self, connection_test, prefix, mock_stores): + cfg = connection_test._config + cfg.stores["local"]["hash_prefix"] = "content" + cfg.stores["local"]["schema_prefix"] = "objects" + schema = dj.Schema( + f"{prefix}_test_gc_prefixes", + context={"GcBlobTest": GcBlobTest, "GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + schema(GcBlobTest) + schema(GcObjectTest) + yield schema + schema.drop() + cfg.stores["local"].pop("hash_prefix", None) + cfg.stores["local"].pop("schema_prefix", None) + + def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): + cfg = schema_prefixed.connection._config + GcBlobTest.insert1({"rid": 1, "payload": np.arange(16, dtype="uint8")}) + GcObjectTest.insert1({"rid": 1, "results": b"payload-one"}) + GcObjectTest.insert1({"rid": 2, "results": b"payload-two"}) + + # Writers honored the configured sections (metadata records the real paths) + hash_refs = gc.scan_hash_references(schema_prefixed, store_name="local") + assert hash_refs and all(r.startswith("content/") for r in hash_refs), hash_refs + obj_refs = gc.scan_schema_references(schema_prefixed, store_name="local") + assert len(obj_refs) == 2 and all(r.startswith("objects/") for r in obj_refs), obj_refs + + # GC scans the same sections: everything referenced, nothing falsely orphaned + stats = gc.scan(schema_prefixed, store_name="local") + assert stats["hash_referenced"] >= 1 + assert not (hash_refs & set(stats["orphaned_hashes"])) + assert not (obj_refs & set(stats["orphaned_paths"])) + + # And reclamation works within the configured sections + (GcObjectTest & {"rid": 2}).delete(prompt=False) + gc.collect(schema_prefixed, store_name="local", dry_run=False) + stored_after = set(gc.list_schema_paths("local", config=cfg)) + live = gc.scan_schema_references(schema_prefixed, store_name="local") + assert live <= stored_after, "live object under custom section must survive" + assert not ((obj_refs - live) & stored_after), "orphan under custom section must be reclaimed" diff --git a/tests/integration/test_hash_storage.py b/tests/integration/test_hash_storage.py index bc1c61a4d..edb26581e 100644 --- a/tests/integration/test_hash_storage.py +++ b/tests/integration/test_hash_storage.py @@ -98,17 +98,24 @@ def test_real_hash_path(self): class TestPutHash: """Tests for put_hash function.""" - @patch("datajoint.hash_registry.get_store_subfolding") @patch("datajoint.hash_registry.get_store_backend") - def test_stores_new_content(self, mock_get_backend, mock_get_subfolding): - """Test storing new content.""" + def test_stores_new_content(self, mock_get_backend): + """Test storing new content (spec-driven: put_hash reads the store + spec for subfolding and hash_prefix).""" + import datajoint as dj + mock_backend = MagicMock() mock_backend.exists.return_value = False mock_get_backend.return_value = mock_backend - mock_get_subfolding.return_value = None - data = b"new content" - result = put_hash(data, schema_name="test_schema", store_name="test_store") + original_stores = dj.config.stores.copy() + dj.config.stores["test_store"] = {"protocol": "file", "location": "/tmp/test_hash_store"} + try: + data = b"new content" + result = put_hash(data, schema_name="test_schema", store_name="test_store") + finally: + dj.config.stores.clear() + dj.config.stores.update(original_stores) # Verify return value includes hash and path assert "hash" in result @@ -122,17 +129,23 @@ def test_stores_new_content(self, mock_get_backend, mock_get_subfolding): # Verify backend was called mock_backend.put_buffer.assert_called_once() - @patch("datajoint.hash_registry.get_store_subfolding") @patch("datajoint.hash_registry.get_store_backend") - def test_deduplicates_existing_content(self, mock_get_backend, mock_get_subfolding): + def test_deduplicates_existing_content(self, mock_get_backend): """Test that existing content is not re-uploaded.""" + import datajoint as dj + mock_backend = MagicMock() mock_backend.exists.return_value = True # Content already exists mock_get_backend.return_value = mock_backend - mock_get_subfolding.return_value = None - data = b"existing content" - result = put_hash(data, schema_name="test_schema", store_name="test_store") + original_stores = dj.config.stores.copy() + dj.config.stores["test_store"] = {"protocol": "file", "location": "/tmp/test_hash_store"} + try: + data = b"existing content" + result = put_hash(data, schema_name="test_schema", store_name="test_store") + finally: + dj.config.stores.clear() + dj.config.stores.update(original_stores) # Verify return value is still correct assert result["hash"] == compute_hash(data) diff --git a/tests/integration/test_object.py b/tests/integration/test_object.py index c03540b58..fbb605838 100644 --- a/tests/integration/test_object.py +++ b/tests/integration/test_object.py @@ -123,7 +123,8 @@ def test_build_object_path_with_partition(self): partition_pattern="{subject_id}", ) # subject_id should be at the beginning due to partition - assert path.startswith("subject_id=1") + # section prefix first, then partition attrs (schema_prefix default "_schema") + assert path.startswith("_schema/subject_id=1") class TestObjectRef: diff --git a/tests/unit/test_codecs.py b/tests/unit/test_codecs.py index 2de9b3465..764ed2d31 100644 --- a/tests/unit/test_codecs.py +++ b/tests/unit/test_codecs.py @@ -484,10 +484,10 @@ def test_filepath_rejects_hash_section(self): dj.config.stores.clear() dj.config.stores.update(original_stores) - def test_filepath_rejects_hash_layout_even_when_prefix_overridden(self): - """Overriding hash_prefix must NOT un-reserve the FIXED _hash/ layout: - the store key is declarative (it does not relocate hash storage), so - the true namespace stays protected regardless of the override.""" + def test_filepath_reservation_follows_hash_prefix_override(self): + """hash_prefix controls the layout: overriding it moves the reserved + section along with the writer (both consume the same setting), so the + override is reserved and the former default is released.""" from unittest.mock import MagicMock, patch import datajoint as dj @@ -507,19 +507,17 @@ def test_filepath_rejects_hash_layout_even_when_prefix_overridden(self): mock_backend.exists.return_value = True mock_get_backend.return_value = mock_backend - # the fixed layout remains reserved - with pytest.raises( - ValueError, - match=r" cannot use reserved section '_hash'", - ): - filepath_codec.encode("_hash/schema/file.dat", store_name="test_store") - - # the declared override is reserved too + # the overridden section is reserved with pytest.raises( ValueError, match=r" cannot use reserved section 'content'", ): filepath_codec.encode("content/file.dat", store_name="test_store") + + # the former default is released — hash objects now live under + # the override, so `_hash/` is ordinary user namespace here + result = filepath_codec.encode("_hash/schema/file.dat", store_name="test_store") + assert result["path"] == "_hash/schema/file.dat" finally: dj.config.stores.clear() dj.config.stores.update(original_stores) From f494a6801a794cec858259d186b566cd019e25cc Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 12:54:24 -0500 Subject: [PATCH 08/20] =?UTF-8?q?refactor(storage):=20drop=20builder=20pre?= =?UTF-8?q?fix=20fallbacks=20=E2=80=94=20settings=20is=20the=20single=20so?= =?UTF-8?q?urce?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/builtin_codecs/schema.py | 2 +- src/datajoint/gc.py | 6 +++--- src/datajoint/hash_registry.py | 20 +++++++------------- src/datajoint/staged_insert.py | 2 +- src/datajoint/storage.py | 16 ++++++---------- tests/integration/test_hash_storage.py | 12 ++++++------ tests/integration/test_object.py | 4 ++++ 7 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/datajoint/builtin_codecs/schema.py b/src/datajoint/builtin_codecs/schema.py index d8bba0150..11c6c5750 100644 --- a/src/datajoint/builtin_codecs/schema.py +++ b/src/datajoint/builtin_codecs/schema.py @@ -150,7 +150,7 @@ def _build_path( spec = config.get_store_spec(store_name) partition_pattern = spec.get("partition_pattern") token_length = spec.get("token_length", 8) - schema_prefix = spec.get("schema_prefix", "_schema") + schema_prefix = spec["schema_prefix"] # always present: settings applies the default return build_object_path( schema=schema, diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 59acdc19f..793e871f9 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -41,7 +41,7 @@ import logging from typing import TYPE_CHECKING, Any -from .hash_registry import DEFAULT_HASH_PREFIX, delete_path, get_store_backend +from .hash_registry import delete_path, get_store_backend from .errors import DataJointError if TYPE_CHECKING: @@ -286,7 +286,7 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, # value remain readable (their metadata stores full paths) but are not # candidates for reclamation until the setting is restored. _spec = config.get_store_spec(store_name) - hash_prefix = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/") + "/" + hash_prefix = _spec["hash_prefix"].strip("/") + "/" # settings applies the "_hash" default # Base32 pattern: 26 lowercase alphanumeric chars base32_pattern = re.compile(r"^[a-z2-7]{26}$") @@ -363,7 +363,7 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i _spec = config.get_store_spec(store_name) _fp = _spec.get("filepath_prefix") filepath_prefix = (_fp.strip("/") + "/") if _fp else None - _hp = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/") + _hp = _spec["hash_prefix"].strip("/") # settings applies the "_hash" default hash_section = _hp + "/" if _hp else None try: diff --git a/src/datajoint/hash_registry.py b/src/datajoint/hash_registry.py index ccbbcd26a..34af10583 100644 --- a/src/datajoint/hash_registry.py +++ b/src/datajoint/hash_registry.py @@ -48,14 +48,6 @@ logger = logging.getLogger(__name__.split(".")[0]) -# Default section prefix for hash-addressed storage. The authoritative value -# is the per-store `hash_prefix` setting (default "_hash", see -# settings.get_store_spec and docs/how-to/configure-storage) — writers, GC, -# and validation all consume the same setting so the layout can -# be relocated per store without the components drifting apart. -DEFAULT_HASH_PREFIX = "_hash" - - def compute_hash(data: bytes) -> str: """ Compute Base32-encoded MD5 hash of content. @@ -105,7 +97,8 @@ def build_hash_path( content_hash: str, schema_name: str, subfolding: tuple[int, ...] | None = None, - hash_prefix: str = DEFAULT_HASH_PREFIX, + *, + hash_prefix: str, ) -> str: """ Build the storage path for hash-addressed storage. @@ -126,9 +119,10 @@ def build_hash_path( Database/schema name for isolation. subfolding : tuple[int, ...], optional Subfolding pattern from store config. None means flat (no subfolding). - hash_prefix : str, optional - Section prefix from the store's ``hash_prefix`` setting - (default ``"_hash"``). + hash_prefix : str + Section prefix from the store's ``hash_prefix`` setting. There is no + fallback here by design: settings (``get_store_spec``) is the single + source of the ``"_hash"`` default, applied to every store spec. Returns ------- @@ -240,7 +234,7 @@ def put_hash( content_hash, schema_name, subfolding, - hash_prefix=spec.get("hash_prefix", DEFAULT_HASH_PREFIX), + hash_prefix=spec["hash_prefix"], # always present: settings applies the default ) backend = get_store_backend(store_name, config=config) diff --git a/src/datajoint/staged_insert.py b/src/datajoint/staged_insert.py index b2d601539..b3d82ac6c 100644 --- a/src/datajoint/staged_insert.py +++ b/src/datajoint/staged_insert.py @@ -120,7 +120,7 @@ def _resolve_field(self, field: str, ext: str) -> tuple[str, "StorageBackend"]: ext=ext if ext else None, partition_pattern=partition_pattern, token_length=token_length, - schema_prefix=spec.get("schema_prefix", "_schema"), + schema_prefix=spec["schema_prefix"], # always present: settings applies the default ) self._staged_objects[field] = { diff --git a/src/datajoint/storage.py b/src/datajoint/storage.py index d1290e760..39a21ce46 100644 --- a/src/datajoint/storage.py +++ b/src/datajoint/storage.py @@ -188,12 +188,6 @@ def encode_pk_value(value: Any) -> str: return s -# Default section prefix for schema-addressed storage. The authoritative value -# is the per-store `schema_prefix` setting (default "_schema", see -# settings.get_store_spec and docs/how-to/configure-storage). -DEFAULT_SCHEMA_PREFIX = "_schema" - - def build_object_path( schema: str, table: str, @@ -202,7 +196,8 @@ def build_object_path( ext: str | None, partition_pattern: str | None = None, token_length: int = 8, - schema_prefix: str = DEFAULT_SCHEMA_PREFIX, + *, + schema_prefix: str, ) -> tuple[str, str]: """ Build the storage path for an object attribute. @@ -223,9 +218,10 @@ def build_object_path( Partition pattern with ``{attr}`` placeholders. token_length : int, optional Length of random token suffix. Default 8. - schema_prefix : str, optional - Section prefix from the store's ``schema_prefix`` setting - (default ``"_schema"``). + schema_prefix : str + Section prefix from the store's ``schema_prefix`` setting. No fallback + by design: settings (``get_store_spec``) is the single source of the + ``"_schema"`` default, applied to every store spec. Returns ------- diff --git a/tests/integration/test_hash_storage.py b/tests/integration/test_hash_storage.py index edb26581e..42283493b 100644 --- a/tests/integration/test_hash_storage.py +++ b/tests/integration/test_hash_storage.py @@ -58,33 +58,33 @@ class TestBuildHashPath: def test_builds_flat_path(self): """Test that path is built as _hash/{schema}/{hash}.""" test_hash = "abcdefghijklmnopqrstuvwxyz"[:26] # 26 char base32 - result = build_hash_path(test_hash, "my_schema") + result = build_hash_path(test_hash, "my_schema", hash_prefix="_hash") assert result == f"_hash/my_schema/{test_hash}" def test_builds_subfolded_path(self): """Test path with subfolding.""" test_hash = "abcdefghijklmnopqrstuvwxyz"[:26] - result = build_hash_path(test_hash, "my_schema", subfolding=(2, 2)) + result = build_hash_path(test_hash, "my_schema", subfolding=(2, 2), hash_prefix="_hash") assert result == f"_hash/my_schema/ab/cd/{test_hash}" def test_rejects_invalid_hash(self): """Test that invalid hash raises error.""" with pytest.raises(DataJointError, match="Invalid content hash"): - build_hash_path("not-a-hash", "my_schema") + build_hash_path("not-a-hash", "my_schema", hash_prefix="_hash") with pytest.raises(DataJointError, match="Invalid content hash"): - build_hash_path("a" * 64, "my_schema") # Too long + build_hash_path("a" * 64, "my_schema", hash_prefix="_hash") # Too long with pytest.raises(DataJointError, match="Invalid content hash"): - build_hash_path("ABCDEFGHIJKLMNOPQRSTUVWXYZ"[:26], "my_schema") # Uppercase + build_hash_path("ABCDEFGHIJKLMNOPQRSTUVWXYZ"[:26], "my_schema", hash_prefix="_hash") # Uppercase def test_real_hash_path(self): """Test path building with a real computed hash.""" data = b"test content" content_hash = compute_hash(data) - path = build_hash_path(content_hash, "test_schema") + path = build_hash_path(content_hash, "test_schema", hash_prefix="_hash") # Verify structure: _hash/{schema}/{hash} parts = path.split("/") diff --git a/tests/integration/test_object.py b/tests/integration/test_object.py index fbb605838..ec40f40b4 100644 --- a/tests/integration/test_object.py +++ b/tests/integration/test_object.py @@ -80,6 +80,7 @@ def test_build_object_path_basic(self): field="data_file", primary_key={"id": 123}, ext=".dat", + schema_prefix="_schema", ) assert "myschema" in path assert "MyTable" in path @@ -96,6 +97,7 @@ def test_build_object_path_no_extension(self): field="data_folder", primary_key={"id": 456}, ext=None, + schema_prefix="_schema", ) assert not path.endswith(".") assert "data_folder_" in path @@ -108,6 +110,7 @@ def test_build_object_path_multiple_pk(self): field="raw_data", primary_key={"subject_id": 1, "session_id": 2}, ext=".zarr", + schema_prefix="_schema", ) assert "subject_id=1" in path assert "session_id=2" in path @@ -121,6 +124,7 @@ def test_build_object_path_with_partition(self): primary_key={"subject_id": 1, "session_id": 2}, ext=".dat", partition_pattern="{subject_id}", + schema_prefix="_schema", ) # subject_id should be at the beginning due to partition # section prefix first, then partition attrs (schema_prefix default "_schema") From 9253600ea3e8201a7eb81c17202e51790e221136 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 13:38:38 -0500 Subject: [PATCH 09/20] fix(gc): never flag a live hash object as a schema orphan (prefix-change safety) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/gc.py | 12 ++++++++- tests/integration/test_gc.py | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 793e871f9..87e00e1ae 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -544,7 +544,17 @@ def scan( # Coverage, not exact set difference: a referenced path may be a # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is many # files under that prefix, plus a `.manifest.json` sidecar beside it. - orphaned_paths = {p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)} + # + # A stored file is live if covered by EITHER reference set. The schema walk + # covers the whole store minus the currently configured hash and filepath + # sections, so if hash objects live OUTSIDE the current hash section — e.g. + # after hash_prefix was changed on a populated store — they surface here. + # They are live (their full paths are in hash_referenced, recorded in row + # metadata), so excluding hash_referenced protects them from being deleted + # as bogus schema orphans. Without this, a hash_prefix change would make + # collect() destroy live hash-addressed data. + live = schema_paths_referenced | hash_referenced + orphaned_paths = {p for p in schema_paths_stored if not _is_covered(p, live)} schema_paths_orphaned_bytes = sum(schema_paths_stored.get(p, 0) for p in orphaned_paths) return { diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 789b48028..ad224ee51 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -756,3 +756,54 @@ def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): live = gc.scan_schema_references(schema_prefixed, store_name="local") assert live <= stored_after, "live object under custom section must survive" assert not ((obj_refs - live) & stored_after), "orphan under custom section must be reclaimed" + + +class TestHashObjectsOutsideCurrentSection: + """A live hash object is protected from GC even if it lives OUTSIDE the + currently configured hash section — e.g. after `hash_prefix` is changed on + a populated store. Reads always work (metadata records the full path); GC + must not misclassify such objects as schema-addressed orphans and delete + live data. Regression for the prefix-change corner.""" + + @pytest.fixture + def schema_pfx(self, connection_test, prefix, mock_stores): + schema = dj.Schema( + f"{prefix}_test_gc_hashmove", + context={"GcBlobTest": GcBlobTest, "GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + schema(GcBlobTest) + schema(GcObjectTest) + yield schema + schema.drop() + + def test_live_hash_object_survives_prefix_change(self, schema_pfx): + cfg = schema_pfx.connection._config + # Written under the default _hash/ section. + GcBlobTest.insert1({"rid": 1, "payload": np.arange(8, dtype="uint8")}) + GcObjectTest.insert1({"rid": 1, "results": b"schema-obj"}) + hash_ref = next(iter(gc.scan_hash_references(schema_pfx, store_name="local"))) + assert hash_ref.startswith("_hash/") + + # Relocate the hash section: new writes would go to content/, but the + # existing object stays at _hash/ (metadata keeps its full path). + cfg.stores["local"]["hash_prefix"] = "content" + try: + # Read still works via the metadata path. + assert (GcBlobTest & {"rid": 1}).fetch1("payload") is not None + + stats = gc.scan(schema_pfx, store_name="local") + assert hash_ref not in set( + stats["orphaned_paths"] + ), "live hash object outside the current hash section must not be a schema orphan" + assert hash_ref not in set(stats["orphaned_hashes"]) + + # End-to-end: collect must not destroy the live hash object. + gc.collect(schema_pfx, store_name="local", dry_run=False) + from datajoint.hash_registry import get_store_backend + + backend = get_store_backend("local", config=cfg) + assert backend.exists(hash_ref), "collect() deleted a live hash object after prefix change" + assert (GcBlobTest & {"rid": 1}).fetch1("payload") is not None + finally: + cfg.stores["local"].pop("hash_prefix", None) From 93eb6957160a589c2c29c79ee2889f31a483dee1 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 13:51:57 -0500 Subject: [PATCH 10/20] docs(gc): correct docstrings for settings-driven path construction and checking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/gc.py | 88 +++++++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 87e00e1ae..57c7c9f3b 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -141,7 +141,8 @@ def scan_hash_references( Returns ------- set[str] - Set of storage paths that are referenced. + Full relative store paths referenced by live rows, exactly as recorded + in each row's metadata (independent of the store's current prefixes). """ referenced: set[str] = set() @@ -193,7 +194,9 @@ def scan_schema_references( Scan schemas for schema-addressed storage references. Examines all tables in the given schemas and extracts paths from columns - that use schema-addressed storage (````, ````). + that use schema-addressed storage — any ``SchemaCodec`` (built-in + ````, ```` and custom subclasses, recognized by type per + #1469), not a fixed codec-name list. Parameters ---------- @@ -207,7 +210,9 @@ def scan_schema_references( Returns ------- set[str] - Set of storage paths that are referenced. + Full relative store paths referenced by live rows, exactly as recorded + in each row's metadata. For a directory-valued object the recorded + path is the directory prefix, not its individual files. """ referenced: set[str] = set() @@ -254,9 +259,12 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, """ List all hash-addressed items in storage. - Scans the ``_hash/`` directory in the specified store and returns - all storage paths found. These correspond to ````, ````, - and ```` types. + Scans the store's configured hash-addressed section — the ``hash_prefix`` + setting, default ``_hash/`` — and returns the stored objects found. These + correspond to ````, ````, and ```` types. Only the + CURRENTLY configured prefix is walked: objects written under a former + prefix are not listed here (they remain readable via their metadata paths, + and :func:`scan` still protects them from deletion — see its note). Parameters ---------- @@ -268,7 +276,8 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, Returns ------- dict[str, int] - Dict mapping storage path to size in bytes. + Dict mapping each object's full relative store path + (``{hash_prefix}/{schema}/[{subfolders}/]{hash}``) to its size in bytes. """ import re @@ -308,14 +317,14 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, file_path = f"{root}/{filename}" size = backend.fs.size(file_path) # Build relative path for comparison with stored metadata - # Path format: _hash/{schema}/{subfolders...}/{hash} + # Path format: {hash_prefix}/{schema}/{subfolders...}/{hash} relative_path = file_path.replace(backend._full_path(""), "").lstrip("/") stored[relative_path] = size except Exception: pass except FileNotFoundError: - # No _hash/ directory exists yet + # The hash section does not exist yet pass except Exception as e: logger.warning(f"Error listing stored hashes: {e}") @@ -328,13 +337,23 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i List all schema-addressed object files in storage. Enumerates the individual **files** written by schema-addressed codecs. - A single-file object is one file at ``{schema}/{table}/{pk}/{field}_{token}[.ext]``; - a directory-valued object (e.g. a Zarr store) is many files under that - path prefix, plus a ``{path}.manifest.json`` sidecar beside it — all are - listed. Orphan detection matches these files against each row's referenced - object path via :func:`_is_covered` (exact, ancestor-prefix, or - manifest-sidecar), so superseded per-token versions are reclaimable while - every file belonging to a live object — including its manifest — is kept. + A single-file object is one file at + ``{schema_prefix}/{schema}/{table}/{pk}/{field}_{token}[.ext]`` + (``schema_prefix`` defaults to ``_schema``); a directory-valued object + (e.g. a Zarr store) is many files under that path prefix, plus a + ``{path}.manifest.json`` sidecar beside it — all are listed. Orphan + detection matches these files against each row's referenced object path + via :func:`_is_covered` (exact, ancestor-prefix, or manifest-sidecar), so + superseded per-token versions are reclaimable while every file belonging + to a live object — including its manifest — is kept. + + Scope: the walk covers the WHOLE store except the store's configured hash + and filepath sections. This is deliberate — objects written by DataJoint + 2.3.0 and earlier live at root-level ``{schema}/...`` (no ``schema_prefix`` + section), and hash objects can sit outside the *current* hash section if + ``hash_prefix`` was changed on a populated store; both must stay listable. + :func:`scan` distinguishes true orphans from live hash objects that surface + here by checking coverage against BOTH the schema and hash reference sets. Parameters ---------- @@ -396,7 +415,9 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i file_path = f"{root}/{filename}" relative_path = file_path.replace(full_prefix, "").lstrip("/") - # Schema-addressed files live at least at {schema}/{table}/.../{file} + # Floor guard: a real object is at least {schema}/{table}/.../{file} + # (root-level legacy layout) or {schema_prefix}/{schema}/... — + # both have >= 2 slashes. Drops stray shallow files. if relative_path.count("/") < 2: continue @@ -498,8 +519,19 @@ def scan( """ Scan for orphaned storage items without deleting. - Scans both hash-addressed storage (for ````, ````, ````) - and schema-addressed storage (for ````, ````). + Scans both hash-addressed storage (````, ````, ````) + and schema-addressed storage (any ``SchemaCodec``: ````, ```` + and custom subclasses). Section locations follow the store's ``hash_prefix`` + / ``schema_prefix`` settings. + + A stored file is considered live if it is covered by EITHER reference set + (schema or hash). This is what keeps a live hash object safe when it sits + outside the current hash section — e.g. after ``hash_prefix`` was changed + on a populated store: it surfaces in the schema-addressed walk but is + covered by the hash references (its metadata path), so it is never + misclassified as an orphan. The trade-off (documented, not a bug): objects + under a *former* prefix are not reclamation candidates until the setting is + restored. Parameters ---------- @@ -519,12 +551,13 @@ def scan( - hash_stored: Number of hash items in storage - hash_orphaned: Number of unreferenced hash items - hash_orphaned_bytes: Total size of orphaned hashes - - orphaned_hashes: List of orphaned content hashes + - orphaned_hashes: List of orphaned hash items as full relative store + paths (NOT bare hashes — passed as-is to the deleter) - schema_paths_referenced: Number of schema items referenced in database - - schema_paths_stored: Number of schema items in storage - - schema_paths_orphaned: Number of unreferenced schema items - - schema_paths_orphaned_bytes: Total size of orphaned schema items - - orphaned_paths: List of orphaned schema paths + - schema_paths_stored: Number of schema files in storage + - schema_paths_orphaned: Number of unreferenced schema files + - schema_paths_orphaned_bytes: Total size of orphaned schema files + - orphaned_paths: List of orphaned schema files as full relative store paths """ if not schemas: raise DataJointError("At least one schema must be provided") @@ -587,8 +620,11 @@ def collect( """ Remove orphaned storage items. - Scans the given schemas for storage references, then removes any - items that are not referenced. + Scans the given schemas (via :func:`scan`) for storage references, then + removes items covered by neither the schema nor hash reference set. Orphan + identity therefore comes entirely from :func:`scan`: live objects outside + the current section (after a prefix change) and the store's declared + ``filepath_prefix`` namespace are never deleted. Parameters ---------- From 81d35167b78dec42d9ff6779dea068ef3fe61015 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 14:01:42 -0500 Subject: [PATCH 11/20] refactor(gc): remove format_stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/datajoint/gc.py | 68 ------------------------------------ tests/integration/test_gc.py | 68 ------------------------------------ 2 files changed, 136 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 57c7c9f3b..4cf9d3004 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -709,71 +709,3 @@ def collect( "hash_orphaned": stats["hash_orphaned"], "schema_paths_orphaned": stats["schema_paths_orphaned"], } - - -def format_stats(stats: dict[str, Any]) -> str: - """ - Format GC statistics as a human-readable string. - - Parameters - ---------- - stats : dict[str, Any] - Statistics dict from scan() or collect(). - - Returns - ------- - str - Formatted string. - """ - lines = ["Object Storage Statistics:"] - - # Show hash-addressed storage stats if present - if "hash_referenced" in stats: - lines.append("") - lines.append("Hash-Addressed Storage (, , ):") - lines.append(f" Referenced: {stats['hash_referenced']}") - lines.append(f" Stored: {stats['hash_stored']}") - lines.append(f" Orphaned: {stats['hash_orphaned']}") - if "hash_orphaned_bytes" in stats: - size_mb = stats["hash_orphaned_bytes"] / (1024 * 1024) - lines.append(f" Orphaned size: {size_mb:.2f} MB") - - # Show schema-addressed storage stats if present - if "schema_paths_referenced" in stats: - lines.append("") - lines.append("Schema-Addressed Storage (, ):") - lines.append(f" Referenced: {stats['schema_paths_referenced']}") - lines.append(f" Stored: {stats['schema_paths_stored']}") - lines.append(f" Orphaned: {stats['schema_paths_orphaned']}") - if "schema_paths_orphaned_bytes" in stats: - size_mb = stats["schema_paths_orphaned_bytes"] / (1024 * 1024) - lines.append(f" Orphaned size: {size_mb:.2f} MB") - - # Show totals - lines.append("") - lines.append("Totals:") - lines.append(f" Referenced in database: {stats['referenced']}") - lines.append(f" Stored in backend: {stats['stored']}") - lines.append(f" Orphaned (unreferenced): {stats['orphaned']}") - - if "orphaned_bytes" in stats: - size_mb = stats["orphaned_bytes"] / (1024 * 1024) - lines.append(f" Orphaned size: {size_mb:.2f} MB") - - # Show deletion results if this is from collect() - if "deleted" in stats: - lines.append("") - if stats.get("dry_run", True): - lines.append(" [DRY RUN - no changes made]") - else: - lines.append(f" Deleted: {stats['deleted']}") - if "hash_deleted" in stats: - lines.append(f" Hash items: {stats['hash_deleted']}") - if "schema_paths_deleted" in stats: - lines.append(f" Schema paths: {stats['schema_paths_deleted']}") - freed_mb = stats["bytes_freed"] / (1024 * 1024) - lines.append(f" Bytes freed: {freed_mb:.2f} MB") - if stats.get("errors", 0) > 0: - lines.append(f" Errors: {stats['errors']}") - - return "\n".join(lines) diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index ad224ee51..d85e665dd 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -331,74 +331,6 @@ def test_deletes_orphaned_schemas(self, mock_scan, mock_list_schemas, mock_delet mock_delete.assert_called_once_with("schema/table/pk/field", "test_store", config=mock_schema.connection._config) -class TestFormatStats: - """Tests for format_stats function.""" - - def test_formats_scan_stats(self): - """Test formatting scan statistics.""" - stats = { - "referenced": 10, - "stored": 15, - "orphaned": 5, - "orphaned_bytes": 1024 * 1024, # 1 MB - "hash_referenced": 6, - "hash_stored": 8, - "hash_orphaned": 2, - "hash_orphaned_bytes": 512 * 1024, - "schema_paths_referenced": 4, - "schema_paths_stored": 7, - "schema_paths_orphaned": 3, - "schema_paths_orphaned_bytes": 512 * 1024, - } - - result = gc.format_stats(stats) - - assert "Referenced in database: 10" in result - assert "Stored in backend: 15" in result - assert "Orphaned (unreferenced): 5" in result - assert "1.00 MB" in result - # Check for detailed sections - assert "Hash-Addressed Storage" in result - assert "Schema-Addressed Storage" in result - - def test_formats_collect_stats_dry_run(self): - """Test formatting collect statistics with dry_run.""" - stats = { - "referenced": 10, - "stored": 15, - "orphaned": 5, - "deleted": 0, - "bytes_freed": 0, - "dry_run": True, - } - - result = gc.format_stats(stats) - - assert "DRY RUN" in result - - def test_formats_collect_stats_actual(self): - """Test formatting collect statistics after actual deletion.""" - stats = { - "referenced": 10, - "stored": 15, - "orphaned": 5, - "deleted": 3, - "hash_deleted": 2, - "schema_paths_deleted": 1, - "bytes_freed": 2 * 1024 * 1024, # 2 MB - "errors": 2, - "dry_run": False, - } - - result = gc.format_stats(stats) - - assert "Deleted: 3" in result - assert "Hash items: 2" in result - assert "Schema paths: 1" in result - assert "2.00 MB" in result - assert "Errors: 2" in result - - class TestScanWithLiveData: """End-to-end tests for gc.scan() against real schemas with external storage. From b0dad04961928ae5e33fe3fc6655a52d1743efc5 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 14:16:47 -0500 Subject: [PATCH 12/20] =?UTF-8?q?feat(gc):=20per-schema=20scoping=20?= =?UTF-8?q?=E2=80=94=20scan/collect=20confine=20orphan=20detection=20to=20?= =?UTF-8?q?each=20schema?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/gc.py | 218 +++++++++++++++++++++-------------- tests/integration/test_gc.py | 85 ++++++++++++++ 2 files changed, 219 insertions(+), 84 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 4cf9d3004..da3f1cb52 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -255,9 +255,9 @@ def scan_schema_references( return referenced -def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, int]: +def list_stored_hashes(store_name: str | None = None, config=None, schema_name: str | None = None) -> dict[str, int]: """ - List all hash-addressed items in storage. + List hash-addressed items in storage. Scans the store's configured hash-addressed section — the ``hash_prefix`` setting, default ``_hash/`` — and returns the stored objects found. These @@ -266,12 +266,21 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, prefix are not listed here (they remain readable via their metadata paths, and :func:`scan` still protects them from deletion — see its note). + When ``schema_name`` is given, the walk is confined to that schema's + subtree (``{hash_prefix}/{schema}/``). Because every hash path embeds the + schema and deduplication is per-schema, this is complete for that schema — + which is what lets :func:`scan` operate on one schema without seeing (or + endangering) objects belonging to other schemas sharing the store. + Parameters ---------- store_name : str, optional Store to scan (None = default store). config : Config, optional Config instance. If None, falls back to global settings.config. + schema_name : str, optional + Restrict the walk to this schema's subtree. None = the whole hash + section (all schemas). Returns ------- @@ -296,11 +305,13 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, # candidates for reclamation until the setting is restored. _spec = config.get_store_spec(store_name) hash_prefix = _spec["hash_prefix"].strip("/") + "/" # settings applies the "_hash" default + # Confine the walk to one schema's subtree when requested. + section = f"{hash_prefix}{schema_name}/" if schema_name else hash_prefix # Base32 pattern: 26 lowercase alphanumeric chars base32_pattern = re.compile(r"^[a-z2-7]{26}$") try: - full_prefix = backend._full_path(hash_prefix) + full_prefix = backend._full_path(section) for root, dirs, files in backend.fs.walk(full_prefix): for filename in files: @@ -332,9 +343,9 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str, return stored -def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, int]: +def list_schema_paths(store_name: str | None = None, config=None, schema_name: str | None = None) -> dict[str, int]: """ - List all schema-addressed object files in storage. + List schema-addressed object files in storage. Enumerates the individual **files** written by schema-addressed codecs. A single-file object is one file at @@ -347,13 +358,20 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i superseded per-token versions are reclaimable while every file belonging to a live object — including its manifest — is kept. - Scope: the walk covers the WHOLE store except the store's configured hash - and filepath sections. This is deliberate — objects written by DataJoint - 2.3.0 and earlier live at root-level ``{schema}/...`` (no ``schema_prefix`` - section), and hash objects can sit outside the *current* hash section if - ``hash_prefix`` was changed on a populated store; both must stay listable. - :func:`scan` distinguishes true orphans from live hash objects that surface - here by checking coverage against BOTH the schema and hash reference sets. + Scope: with ``schema_name`` given, the walk is confined to that schema's + two possible locations — the ``{schema_prefix}/{schema}/`` section and the + legacy root-level ``{schema}/`` layout (DataJoint 2.3.0 and earlier). Both + begin with the schema name, so every schema-addressed object lives under + one of them and the per-schema walk is complete. With ``schema_name=None`` + the walk covers the WHOLE store (minus the configured hash and filepath + sections) — every schema at once. + + In both modes the hash section is skipped, but a hash object can still + surface here when it sits *outside* the current hash section (e.g. after + ``hash_prefix`` was changed on a populated store, or when ``hash_prefix`` + is empty so hash and schema share the root). :func:`scan` distinguishes + such live objects from true orphans by checking coverage against BOTH the + schema and hash reference sets. Parameters ---------- @@ -361,6 +379,8 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i Store to scan (None = default store). config : Config, optional Config instance. If None, falls back to global settings.config. + schema_name : str, optional + Restrict the walk to this schema's subtree(s). None = whole store. Returns ------- @@ -384,52 +404,58 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i filepath_prefix = (_fp.strip("/") + "/") if _fp else None _hp = _spec["hash_prefix"].strip("/") # settings applies the "_hash" default hash_section = _hp + "/" if _hp else None + _sp = _spec["schema_prefix"].strip("/") # settings applies the "_schema" default + + full_root = backend._full_path("") + + # Determine which subtree(s) to walk. Per-schema scoping confines orphan + # detection to the requested schema's own folders — objects belonging to + # other schemas sharing the store are never seen, hence never at risk. + if schema_name: + rel_roots = [f"{_sp}/{schema_name}"] if _sp else [] + rel_roots.append(schema_name) # legacy root-level layout (no schema_prefix) + else: + rel_roots = [""] # whole store + + for rel in rel_roots: + try: + for root, dirs, files in backend.fs.walk(backend._full_path(rel)): + # Skip the hash-addressed storage subtree. Match on the FIRST + # store-relative path segment only — a substring test would also + # exclude user tables/schemas whose names merely contain "_hash" + # (e.g. a table `probe_hash`), silently leaking their orphans. + rel_root = root.replace(full_root, "").lstrip("/") + if hash_section and (rel_root + "/").startswith(hash_section): + continue - try: - # Walk the storage collecting schema-addressed object files. The walk - # covers the WHOLE store (minus the hash and filepath sections) rather - # than just the schema_prefix section: stores written before - # schema_prefix was honored hold objects at root-level {schema}/... - # paths, and those must remain listable and reclaimable. - full_prefix = backend._full_path("") - - for root, dirs, files in backend.fs.walk(full_prefix): - # Skip the hash-addressed storage subtree. Match on the FIRST - # store-relative path segment only — a substring test would also - # exclude user tables/schemas whose names merely contain "_hash" - # (e.g. a table `probe_hash`), silently leaking their orphans. - rel_root = root.replace(full_prefix, "").lstrip("/") - if hash_section and (rel_root + "/").startswith(hash_section): - continue - - # Skip the declared filepath namespace (user-managed files). - if filepath_prefix and (rel_root + "/").startswith(filepath_prefix): - continue - - for filename in files: - # Manifest sidecars ({object}.manifest.json, written for - # folder-valued objects) are deliberately INCLUDED: they are - # owned by their object and must be reclaimed with it when the - # object is orphaned. _is_covered() keeps a live object's - # manifest out of the orphan set. - file_path = f"{root}/{filename}" - relative_path = file_path.replace(full_prefix, "").lstrip("/") - - # Floor guard: a real object is at least {schema}/{table}/.../{file} - # (root-level legacy layout) or {schema_prefix}/{schema}/... — - # both have >= 2 slashes. Drops stray shallow files. - if relative_path.count("/") < 2: + # Skip the declared filepath namespace (user-managed files). + if filepath_prefix and (rel_root + "/").startswith(filepath_prefix): continue - try: - stored[relative_path] = backend.fs.size(file_path) - except Exception: - stored[relative_path] = 0 + for filename in files: + # Manifest sidecars ({object}.manifest.json, written for + # folder-valued objects) are deliberately INCLUDED: they are + # owned by their object and must be reclaimed with it when the + # object is orphaned. _is_covered() keeps a live object's + # manifest out of the orphan set. + file_path = f"{root}/{filename}" + relative_path = file_path.replace(full_root, "").lstrip("/") + + # Floor guard: a real object is at least {schema}/{table}/.../{file} + # (root-level legacy layout) or {schema_prefix}/{schema}/... — + # both have >= 2 slashes. Drops stray shallow files. + if relative_path.count("/") < 2: + continue - except FileNotFoundError: - pass - except Exception as e: - logger.warning(f"Error listing stored schemas: {e}") + try: + stored[relative_path] = backend.fs.size(file_path) + except Exception: + stored[relative_path] = 0 + + except FileNotFoundError: + continue # this subtree does not exist (e.g. no legacy layout) + except Exception as e: + logger.warning(f"Error listing stored schemas: {e}") return stored @@ -524,19 +550,28 @@ def scan( and custom subclasses). Section locations follow the store's ``hash_prefix`` / ``schema_prefix`` settings. + Per-schema, and safe for subsets. Every managed object embeds its schema + in the path (both sections, plus the legacy root-level layout), and hash + deduplication is per-schema — so each schema is scanned independently + against ITS OWN subtree. Orphan detection is therefore confined to the + schemas you pass: objects of other schemas sharing the store are never + listed and never at risk. (This removes the former requirement to pass + *every* schema on a store at once.) + A stored file is considered live if it is covered by EITHER reference set - (schema or hash). This is what keeps a live hash object safe when it sits - outside the current hash section — e.g. after ``hash_prefix`` was changed - on a populated store: it surfaces in the schema-addressed walk but is - covered by the hash references (its metadata path), so it is never - misclassified as an orphan. The trade-off (documented, not a bug): objects - under a *former* prefix are not reclamation candidates until the setting is - restored. + (schema or hash) for its schema. This keeps a live hash object safe when it + sits outside the current hash section — e.g. after ``hash_prefix`` was + changed on a populated store, or when ``hash_prefix`` is empty so hash and + schema share the root: it is covered by the hash references (its metadata + path), so it is never misclassified as an orphan. The trade-off (documented, + not a bug): objects under a *former* prefix are not reclamation candidates + until the setting is restored. Parameters ---------- *schemas : Schema - Schema instances to scan. + Schema instances to scan. Each is scanned against its own subtree; + any subset of the schemas on a store may be passed safely. store_name : str, optional Store to check (None = default store). verbose : bool, optional @@ -565,29 +600,44 @@ def scan( # Extract config from the first schema's connection _config = schemas[0].connection._config if schemas else None - # --- Hash-addressed storage --- - hash_referenced = scan_hash_references(*schemas, store_name=store_name, verbose=verbose) - hash_stored = list_stored_hashes(store_name, config=_config) - orphaned_hashes = set(hash_stored.keys()) - hash_referenced - hash_orphaned_bytes = sum(hash_stored.get(h, 0) for h in orphaned_hashes) + hash_referenced: set[str] = set() + hash_stored: dict[str, int] = {} + orphaned_hashes: set[str] = set() + schema_paths_referenced: set[str] = set() + schema_paths_stored: dict[str, int] = {} + orphaned_paths: set[str] = set() + + # Scan each schema against its own subtree only. Because both sections + # embed the schema name and hash dedup is per-schema, a schema's orphans + # are fully determined by its own references vs. its own stored files — + # no other schema on the store can influence (or be endangered by) the + # result. Stats below aggregate across the passed schemas. + for schema in schemas: + db = schema.database + + # --- Hash-addressed storage (this schema's subtree) --- + h_ref = scan_hash_references(schema, store_name=store_name, verbose=verbose) + h_stored = list_stored_hashes(store_name, config=_config, schema_name=db) + orphaned_hashes |= set(h_stored.keys()) - h_ref + + # --- Schema-addressed storage (this schema's subtree) --- + s_ref = scan_schema_references(schema, store_name=store_name, verbose=verbose) + s_stored = list_schema_paths(store_name, config=_config, schema_name=db) + # Coverage, not exact set difference: a referenced path may be a + # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is + # many files under that prefix, plus a `.manifest.json` sidecar. + # A stored file is live if covered by EITHER of THIS schema's reference + # sets — the hash arm protects a live hash object that surfaces in the + # schema walk (empty hash_prefix, or a former prefix under this schema). + live = s_ref | h_ref + orphaned_paths |= {p for p in s_stored if not _is_covered(p, live)} + + hash_referenced |= h_ref + hash_stored.update(h_stored) + schema_paths_referenced |= s_ref + schema_paths_stored.update(s_stored) - # --- Schema-addressed storage --- - schema_paths_referenced = scan_schema_references(*schemas, store_name=store_name, verbose=verbose) - schema_paths_stored = list_schema_paths(store_name, config=_config) - # Coverage, not exact set difference: a referenced path may be a - # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is many - # files under that prefix, plus a `.manifest.json` sidecar beside it. - # - # A stored file is live if covered by EITHER reference set. The schema walk - # covers the whole store minus the currently configured hash and filepath - # sections, so if hash objects live OUTSIDE the current hash section — e.g. - # after hash_prefix was changed on a populated store — they surface here. - # They are live (their full paths are in hash_referenced, recorded in row - # metadata), so excluding hash_referenced protects them from being deleted - # as bogus schema orphans. Without this, a hash_prefix change would make - # collect() destroy live hash-addressed data. - live = schema_paths_referenced | hash_referenced - orphaned_paths = {p for p in schema_paths_stored if not _is_covered(p, live)} + hash_orphaned_bytes = sum(hash_stored.get(h, 0) for h in orphaned_hashes) schema_paths_orphaned_bytes = sum(schema_paths_stored.get(p, 0) for p in orphaned_paths) return { diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index d85e665dd..6f37beeac 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -739,3 +739,88 @@ def test_live_hash_object_survives_prefix_change(self, schema_pfx): assert (GcBlobTest & {"rid": 1}).fetch1("payload") is not None finally: cfg.stores["local"].pop("hash_prefix", None) + + +class TestPerSchemaScoping: + """GC operates per schema: because every object path embeds the schema and + hash dedup is per-schema, scanning/collecting one schema is confined to + that schema's subfolder and can never see or delete another schema's + objects — even when both share one store. Removes the old 'pass every + schema or lose data' hazard.""" + + @pytest.fixture + def two_schemas(self, connection_test, prefix, mock_stores): + # Unique names per test: the shared `local` store persists objects + # across tests, but per-schema scoping means leftovers under other + # schema names are invisible here — so unique names give clean counts. + import time + + uid = str(int(time.time() * 1000))[-9:] + a = dj.Schema( + f"{prefix}_gcsc_a{uid}", + context={"GcBlobTest": GcBlobTest, "GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + b = dj.Schema( + f"{prefix}_gcsc_b{uid}", + connection=connection_test, + ) + a(GcBlobTest) + a(GcObjectTest) + # Distinct classes bound to schema b (same definitions, second schema). + b_blob = type("GcBlobTest", (dj.Manual,), {"definition": GcBlobTest.definition}) + b_obj = type("GcObjectTest", (dj.Manual,), {"definition": GcObjectTest.definition}) + b(b_blob) + b(b_obj) + yield a, b, b_blob, b_obj + b.drop() + a.drop() + + def test_list_scoped_to_schema(self, two_schemas): + a, b, b_blob, b_obj = two_schemas + cfg = a.connection._config + GcBlobTest.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) + GcObjectTest.insert1({"rid": 1, "results": b"a-obj"}) + b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) + b_obj.insert1({"rid": 1, "results": b"b-obj"}) + + a_hashes = set(gc.list_stored_hashes("local", config=cfg, schema_name=a.database)) + a_paths = set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) + assert a_hashes and all(f"/{a.database}/" in h for h in a_hashes), a_hashes + assert a_paths and all(p.split("/")[0] == a.database or f"/{a.database}/" in p for p in a_paths), a_paths + # nothing from schema b leaked into schema a's scoped listings + assert not any(b.database in p for p in a_hashes | a_paths) + + def test_scan_one_schema_ignores_the_other(self, two_schemas): + a, b, b_blob, b_obj = two_schemas + GcBlobTest.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) + b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) + b_obj.insert1({"rid": 1, "results": b"b-obj"}) + + stats = gc.scan(a, store_name="local") + # b's live objects are outside a's subtree → not counted, not orphaned + assert stats["orphaned"] == 0 + assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hashes"]) + + def test_collect_one_schema_never_touches_the_other(self, two_schemas): + a, b, b_blob, b_obj = two_schemas + cfg = a.connection._config + # a has an orphan (insert then delete); b has only live data + GcObjectTest.insert1({"rid": 1, "results": b"a-doomed"}) + b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) + b_obj.insert1({"rid": 1, "results": b"b-live"}) + + a_orphan = next(iter(gc.scan_schema_references(a, store_name="local"))) + (GcObjectTest & {"rid": 1}).delete(prompt=False) + + b_hashes_before = set(gc.list_stored_hashes("local", config=cfg, schema_name=b.database)) + b_paths_before = set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) + assert b_hashes_before and b_paths_before + + gc.collect(a, store_name="local", dry_run=False) + + # a's orphan reclaimed; b entirely intact + assert a_orphan not in set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) + assert set(gc.list_stored_hashes("local", config=cfg, schema_name=b.database)) == b_hashes_before + assert set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) == b_paths_before + assert len(b_obj()) == 1 # b's row (and its object) untouched From e5bdb9b33dd0e33e2e7823e41fc0a2071194ed0e Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 14:32:44 -0500 Subject: [PATCH 13/20] refactor(gc): require schema_name; walk only the schema's own sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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. --- src/datajoint/gc.py | 213 +++++++++++++++-------------------- tests/integration/test_gc.py | 62 +++++----- 2 files changed, 120 insertions(+), 155 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index da3f1cb52..fc64e7fab 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -31,6 +31,12 @@ # Remove orphaned items (dry_run=False to actually delete) stats = dj.gc.collect(schema1, schema2, store_name='mystore', dry_run=True) +Both sections embed the schema name in every path, so garbage collection is +**per-schema**: each schema is scanned against its own subtree only. Orphan +detection is therefore confined to the schemas you pass — any subset of the +schemas sharing a store may be scanned/collected safely, and GC never touches +another schema's objects or user-managed content elsewhere in the store. + See Also -------- datajoint.builtin_codecs : Codec implementations for object storage types. @@ -255,22 +261,20 @@ def scan_schema_references( return referenced -def list_stored_hashes(store_name: str | None = None, config=None, schema_name: str | None = None) -> dict[str, int]: +def list_stored_hashes(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: """ - List hash-addressed items in storage. + List a schema's hash-addressed items in storage. - Scans the store's configured hash-addressed section — the ``hash_prefix`` - setting, default ``_hash/`` — and returns the stored objects found. These - correspond to ````, ````, and ```` types. Only the - CURRENTLY configured prefix is walked: objects written under a former - prefix are not listed here (they remain readable via their metadata paths, - and :func:`scan` still protects them from deletion — see its note). + Walks exactly one schema's subtree of the configured hash-addressed + section — ``{hash_prefix}/{schema_name}/`` (``hash_prefix`` default + ``_hash``) — and returns the stored objects (````, ````, + ````). Every hash path embeds the schema and deduplication is + per-schema, so this listing is complete for that schema and independent of + every other schema on the store. - When ``schema_name`` is given, the walk is confined to that schema's - subtree (``{hash_prefix}/{schema}/``). Because every hash path embeds the - schema and deduplication is per-schema, this is complete for that schema — - which is what lets :func:`scan` operate on one schema without seeing (or - endangering) objects belonging to other schemas sharing the store. + Only the CURRENTLY configured prefix is walked: objects written under a + former prefix are not listed here (they remain readable via their metadata + paths). ``schema_name`` is required — GC is always per-schema. Parameters ---------- @@ -278,9 +282,8 @@ def list_stored_hashes(store_name: str | None = None, config=None, schema_name: Store to scan (None = default store). config : Config, optional Config instance. If None, falls back to global settings.config. - schema_name : str, optional - Restrict the walk to this schema's subtree. None = the whole hash - section (all schemas). + schema_name : str + Schema whose hash subtree to list (required, keyword-only). Returns ------- @@ -299,14 +302,10 @@ def list_stored_hashes(store_name: str | None = None, config=None, schema_name: config = _global_config # Hash-addressed storage: {hash_prefix}/{schema}/{subfolders...}/{hash}. # The prefix comes from the store's settings — the same value the writer - # uses — so scanner and writer cannot drift. NOTE: only the CURRENTLY - # configured prefix is scanned; objects written under a previous prefix - # value remain readable (their metadata stores full paths) but are not - # candidates for reclamation until the setting is restored. + # uses — so scanner and writer cannot drift. _spec = config.get_store_spec(store_name) hash_prefix = _spec["hash_prefix"].strip("/") + "/" # settings applies the "_hash" default - # Confine the walk to one schema's subtree when requested. - section = f"{hash_prefix}{schema_name}/" if schema_name else hash_prefix + section = f"{hash_prefix}{schema_name}/" # Base32 pattern: 26 lowercase alphanumeric chars base32_pattern = re.compile(r"^[a-z2-7]{26}$") @@ -343,35 +342,35 @@ def list_stored_hashes(store_name: str | None = None, config=None, schema_name: return stored -def list_schema_paths(store_name: str | None = None, config=None, schema_name: str | None = None) -> dict[str, int]: +def list_schema_paths(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: """ - List schema-addressed object files in storage. - - Enumerates the individual **files** written by schema-addressed codecs. - A single-file object is one file at - ``{schema_prefix}/{schema}/{table}/{pk}/{field}_{token}[.ext]`` - (``schema_prefix`` defaults to ``_schema``); a directory-valued object - (e.g. a Zarr store) is many files under that path prefix, plus a - ``{path}.manifest.json`` sidecar beside it — all are listed. Orphan - detection matches these files against each row's referenced object path - via :func:`_is_covered` (exact, ancestor-prefix, or manifest-sidecar), so - superseded per-token versions are reclaimable while every file belonging + List a schema's schema-addressed object files in storage. + + Walks exactly one schema's section — ``{schema_prefix}/{schema_name}/`` + (``schema_prefix`` default ``_schema``) — and enumerates the individual + **files** written by schema-addressed codecs. A single-file object is one + file at ``{schema_prefix}/{schema}/{table}/{pk}/{field}_{token}[.ext]``; a + directory-valued object (e.g. a Zarr store) is many files under that path + prefix, plus a ``{path}.manifest.json`` sidecar beside it — all are listed. + Orphan detection matches these files against each row's referenced object + path via :func:`_is_covered` (exact, ancestor-prefix, or manifest-sidecar), + so superseded per-token versions are reclaimable while every file belonging to a live object — including its manifest — is kept. - Scope: with ``schema_name`` given, the walk is confined to that schema's - two possible locations — the ``{schema_prefix}/{schema}/`` section and the - legacy root-level ``{schema}/`` layout (DataJoint 2.3.0 and earlier). Both - begin with the schema name, so every schema-addressed object lives under - one of them and the per-schema walk is complete. With ``schema_name=None`` - the walk covers the WHOLE store (minus the configured hash and filepath - sections) — every schema at once. - - In both modes the hash section is skipped, but a hash object can still - surface here when it sits *outside* the current hash section (e.g. after - ``hash_prefix`` was changed on a populated store, or when ``hash_prefix`` - is empty so hash and schema share the root). :func:`scan` distinguishes - such live objects from true orphans by checking coverage against BOTH the - schema and hash reference sets. + The walk is confined to the schema's own section, so it never enters the + hash section or the filepath section (mutually-exclusive top-level + prefixes, enforced by store validation) — GC's blast radius is exactly + ``{schema_prefix}/{schema}/``, and user-managed ```` content and + other schemas' objects are structurally out of reach. + + ``schema_name`` is required — GC is always per-schema. + + Legacy layout: DataJoint 2.3.0 and earlier wrote schema-addressed objects + at root level ``{schema}/...`` (``schema_prefix`` was ignored — see + datajoint/datajoint-python#1487). Such a store should set + ``schema_prefix=""`` so both writes and this walk use the root-level + layout consistently; otherwise those objects remain readable (metadata + records full paths) but are outside the scanned section. Parameters ---------- @@ -379,8 +378,8 @@ def list_schema_paths(store_name: str | None = None, config=None, schema_name: s Store to scan (None = default store). config : Config, optional Config instance. If None, falls back to global settings.config. - schema_name : str, optional - Restrict the walk to this schema's subtree(s). None = whole store. + schema_name : str + Schema whose section to list (required, keyword-only). Returns ------- @@ -390,72 +389,32 @@ def list_schema_paths(store_name: str | None = None, config=None, schema_name: s backend = get_store_backend(store_name, config=config) stored: dict[str, int] = {} - # A store may declare a `filepath_prefix` — the namespace reserved for - # user-managed content. Files under it are NOT DataJoint-owned - # objects and must never be candidates for garbage collection. (Without a - # declared filepath_prefix, cohabiting filepath files are indistinguishable - # from orphans — see the fail-safe planned with the two-phase GC work.) if config is None: from .settings import config as _global_config config = _global_config _spec = config.get_store_spec(store_name) - _fp = _spec.get("filepath_prefix") - filepath_prefix = (_fp.strip("/") + "/") if _fp else None - _hp = _spec["hash_prefix"].strip("/") # settings applies the "_hash" default - hash_section = _hp + "/" if _hp else None _sp = _spec["schema_prefix"].strip("/") # settings applies the "_schema" default - + rel = f"{_sp}/{schema_name}" if _sp else schema_name full_root = backend._full_path("") - # Determine which subtree(s) to walk. Per-schema scoping confines orphan - # detection to the requested schema's own folders — objects belonging to - # other schemas sharing the store are never seen, hence never at risk. - if schema_name: - rel_roots = [f"{_sp}/{schema_name}"] if _sp else [] - rel_roots.append(schema_name) # legacy root-level layout (no schema_prefix) - else: - rel_roots = [""] # whole store - - for rel in rel_roots: - try: - for root, dirs, files in backend.fs.walk(backend._full_path(rel)): - # Skip the hash-addressed storage subtree. Match on the FIRST - # store-relative path segment only — a substring test would also - # exclude user tables/schemas whose names merely contain "_hash" - # (e.g. a table `probe_hash`), silently leaking their orphans. - rel_root = root.replace(full_root, "").lstrip("/") - if hash_section and (rel_root + "/").startswith(hash_section): - continue - - # Skip the declared filepath namespace (user-managed files). - if filepath_prefix and (rel_root + "/").startswith(filepath_prefix): - continue - - for filename in files: - # Manifest sidecars ({object}.manifest.json, written for - # folder-valued objects) are deliberately INCLUDED: they are - # owned by their object and must be reclaimed with it when the - # object is orphaned. _is_covered() keeps a live object's - # manifest out of the orphan set. - file_path = f"{root}/{filename}" - relative_path = file_path.replace(full_root, "").lstrip("/") - - # Floor guard: a real object is at least {schema}/{table}/.../{file} - # (root-level legacy layout) or {schema_prefix}/{schema}/... — - # both have >= 2 slashes. Drops stray shallow files. - if relative_path.count("/") < 2: - continue - - try: - stored[relative_path] = backend.fs.size(file_path) - except Exception: - stored[relative_path] = 0 - - except FileNotFoundError: - continue # this subtree does not exist (e.g. no legacy layout) - except Exception as e: - logger.warning(f"Error listing stored schemas: {e}") + try: + for root, dirs, files in backend.fs.walk(backend._full_path(rel)): + for filename in files: + # Manifest sidecars ({object}.manifest.json) are INCLUDED: they + # are owned by their object and must be reclaimed with it when + # the object is orphaned. _is_covered() keeps a live object's + # manifest out of the orphan set. + file_path = f"{root}/{filename}" + relative_path = file_path.replace(full_root, "").lstrip("/") + try: + stored[relative_path] = backend.fs.size(file_path) + except Exception: + stored[relative_path] = 0 + except FileNotFoundError: + pass # this schema's section does not exist yet + except Exception as e: + logger.warning(f"Error listing stored schemas: {e}") return stored @@ -558,14 +517,13 @@ def scan( listed and never at risk. (This removes the former requirement to pass *every* schema on a store at once.) - A stored file is considered live if it is covered by EITHER reference set - (schema or hash) for its schema. This keeps a live hash object safe when it - sits outside the current hash section — e.g. after ``hash_prefix`` was - changed on a populated store, or when ``hash_prefix`` is empty so hash and - schema share the root: it is covered by the hash references (its metadata - path), so it is never misclassified as an orphan. The trade-off (documented, - not a bug): objects under a *former* prefix are not reclamation candidates - until the setting is restored. + Each schema's two sections are walked in isolation + (``{hash_prefix}/{schema}/`` and ``{schema_prefix}/{schema}/``), so GC's + blast radius is exactly those folders: user ```` content and + other schemas' objects are structurally out of reach. Objects under a + *former* prefix (after a prefix change on a populated store) stay readable + via their metadata paths but are not reclamation candidates until the + setting is restored. Parameters ---------- @@ -620,17 +578,16 @@ def scan( h_stored = list_stored_hashes(store_name, config=_config, schema_name=db) orphaned_hashes |= set(h_stored.keys()) - h_ref - # --- Schema-addressed storage (this schema's subtree) --- + # --- Schema-addressed storage (this schema's section) --- s_ref = scan_schema_references(schema, store_name=store_name, verbose=verbose) s_stored = list_schema_paths(store_name, config=_config, schema_name=db) # Coverage, not exact set difference: a referenced path may be a # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is - # many files under that prefix, plus a `.manifest.json` sidecar. - # A stored file is live if covered by EITHER of THIS schema's reference - # sets — the hash arm protects a live hash object that surfaces in the - # schema walk (empty hash_prefix, or a former prefix under this schema). - live = s_ref | h_ref - orphaned_paths |= {p for p in s_stored if not _is_covered(p, live)} + # many files under that prefix, plus a `.manifest.json` sidecar. The + # schema walk is confined to this schema's schema_prefix section, so it + # can only contain this schema's schema-addressed objects — coverage + # against s_ref alone is complete (no hash objects can appear here). + orphaned_paths |= {p for p in s_stored if not _is_covered(p, s_ref)} hash_referenced |= h_ref hash_stored.update(h_stored) @@ -715,7 +672,11 @@ def collect( if not dry_run: # Delete orphaned hashes if stats["hash_orphaned"] > 0: - hash_stored = list_stored_hashes(store_name, config=_config) + # Size map for bytes_freed — built per schema (listers are + # per-schema) and merged across the passed schemas. + hash_stored: dict[str, int] = {} + for schema in schemas: + hash_stored.update(list_stored_hashes(store_name, config=_config, schema_name=schema.database)) for path in stats["orphaned_hashes"]: try: @@ -731,7 +692,9 @@ def collect( # Delete orphaned schema paths if stats["schema_paths_orphaned"] > 0: - schema_paths_stored = list_schema_paths(store_name, config=_config) + schema_paths_stored: dict[str, int] = {} + for schema in schemas: + schema_paths_stored.update(list_schema_paths(store_name, config=_config, schema_name=schema.database)) for path in stats["orphaned_paths"]: try: diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 6f37beeac..767fbf037 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -467,7 +467,9 @@ def test_custom_codec_survives_collect(self, schema_custom): gc.collect(schema_custom, store_name="local", dry_run=False) - stored_after = set(gc.list_schema_paths("local", config=schema_custom.connection._config)) + stored_after = set( + gc.list_schema_paths("local", config=schema_custom.connection._config, schema_name=schema_custom.database) + ) assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)" assert not (deleted_paths & stored_after), f"deleted row's orphan not reclaimed: {deleted_paths & stored_after}" @@ -509,7 +511,7 @@ def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): assert len(refs) == 1 ref = next(iter(refs)) - stored = gc.list_schema_paths("local", config=schema_dirobj.connection._config) + stored = gc.list_schema_paths("local", config=schema_dirobj.connection._config, schema_name=schema_dirobj.database) chunk_files = [p for p in stored if p.startswith(ref + "/")] assert len(chunk_files) >= 4, f"expected the folder's files under {ref}; got {sorted(stored)}" assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)" @@ -535,7 +537,9 @@ def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path gc.collect(schema_dirobj, store_name="local", dry_run=False) - stored_after = set(gc.list_schema_paths("local", config=schema_dirobj.connection._config)) + stored_after = set( + gc.list_schema_paths("local", config=schema_dirobj.connection._config, schema_name=schema_dirobj.database) + ) live_files = {p for p in stored_after if p.startswith(live_ref + "/")} assert len(live_files) >= 4, f"live folder object lost files: {sorted(stored_after)}" assert f"{live_ref}.manifest.json" in stored_after, "live object's manifest must survive collect()" @@ -557,10 +561,11 @@ class GcProbeHash(dj.Manual): class TestHashSubstringTableName: - """The hash-addressed subtree skip must match the first store-relative - path segment exactly (`_hash/...`), not any path containing the substring - `_hash` — otherwise tables like `gc_probe_hash` are silently excluded from - the schema-addressed listing and their orphans are never reclaimed.""" + """A table whose name contains the substring `_hash` (e.g. `gc_probe_hash`) + is listed and reclaimed normally. Per-schema scoping walks only the + schema's own section (`{schema_prefix}/{schema}/`), so such a table's + objects are ordinary schema-addressed files — there is no store-wide + hash-subtree skip that a substring could trip over.""" @pytest.fixture def schema_probe(self, connection_test, prefix, mock_stores): @@ -579,7 +584,7 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): refs = gc.scan_schema_references(schema_probe, store_name="local") assert len(refs) == 2 - stored = set(gc.list_schema_paths("local", config=schema_probe.connection._config)) + stored = set(gc.list_schema_paths("local", config=schema_probe.connection._config, schema_name=schema_probe.database)) missing = refs - stored assert not missing, ( f"files of a table whose name contains '_hash' must be listed; missing {missing} " @@ -591,21 +596,24 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): dead_ref = next(iter(refs - refs_live)) gc.collect(schema_probe, store_name="local", dry_run=False) - stored_after = set(gc.list_schema_paths("local", config=schema_probe.connection._config)) + stored_after = set( + gc.list_schema_paths("local", config=schema_probe.connection._config, schema_name=schema_probe.database) + ) assert dead_ref not in stored_after, "orphan under a '_hash'-substring table name must be reclaimed" assert next(iter(refs_live)) in stored_after, "live file must survive" -class TestFilepathPrefixProtection: - """A store's declared `filepath_prefix` is the user-managed - namespace: GC must never list (and therefore never collect) files under - it. Without a declaration, cohabiting user files remain indistinguishable - from orphans (documented hazard; fail-safe planned with two-phase GC).""" +class TestUserFilesOutsideSchemaSectionUntouched: + """Per-schema scoping walks only `{schema_prefix}/{schema}/`, so any file + outside that section — including user-managed content anywhere + else in the store — is structurally never a GC candidate. The old + cohabitation hazard (user files misread as orphans) is gone by + construction, with or without a declared filepath_prefix.""" @pytest.fixture def schema_fp(self, connection_test, prefix, mock_stores): schema = dj.Schema( - f"{prefix}_test_gc_fpprefix", + f"{prefix}_test_gc_userfiles", context={"GcObjectTest": GcObjectTest}, connection=connection_test, ) @@ -613,7 +621,7 @@ def schema_fp(self, connection_test, prefix, mock_stores): yield schema schema.drop() - def test_filepath_prefix_subtree_never_collected(self, schema_fp): + def test_user_file_outside_section_never_listed_or_collected(self, schema_fp): from pathlib import Path cfg = schema_fp.connection._config @@ -624,20 +632,14 @@ def test_filepath_prefix_subtree_never_collected(self, schema_fp): GcObjectTest.insert1({"rid": 1, "results": b"managed-object"}) - # Without a declared prefix, the stray user file IS treated as an - # orphan candidate (the documented cohabitation hazard). - stored_undeclared = gc.list_schema_paths("local", config=cfg) - assert "userfiles/deep/important.bin" in stored_undeclared + # The user file lives outside {schema_prefix}/{schema}/, so the scoped + # walk never lists it — no filepath_prefix declaration needed. + stored = gc.list_schema_paths("local", config=cfg, schema_name=schema_fp.database) + assert "userfiles/deep/important.bin" not in stored + assert stored, "the schema's own managed object must still be listed" - cfg.stores["local"]["filepath_prefix"] = "userfiles" - try: - stored = gc.list_schema_paths("local", config=cfg) - assert "userfiles/deep/important.bin" not in stored, "declared filepath namespace must be excluded" - - gc.collect(schema_fp, store_name="local", dry_run=False) - assert user_file.exists(), "collect() must never touch the declared filepath namespace" - finally: - cfg.stores["local"].pop("filepath_prefix", None) + gc.collect(schema_fp, store_name="local", dry_run=False) + assert user_file.exists(), "collect() must never touch files outside the schema section" class TestPrefixSettingsHonored: @@ -684,7 +686,7 @@ def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): # And reclamation works within the configured sections (GcObjectTest & {"rid": 2}).delete(prompt=False) gc.collect(schema_prefixed, store_name="local", dry_run=False) - stored_after = set(gc.list_schema_paths("local", config=cfg)) + stored_after = set(gc.list_schema_paths("local", config=cfg, schema_name=schema_prefixed.database)) live = gc.scan_schema_references(schema_prefixed, store_name="local") assert live <= stored_after, "live object under custom section must survive" assert not ((obj_refs - live) & stored_after), "orphan under custom section must be reclaimed" From 06c926934c2967a336055a915f2be15ffbef1a0c Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 15:06:52 -0500 Subject: [PATCH 14/20] refactor(heading): storage-model classification lives on Heading/Attribute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (, or external /; 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. --- src/datajoint/gc.py | 83 +++----------------------- src/datajoint/heading.py | 47 +++++++++++++++ tests/integration/test_gc.py | 111 ++++++++++++----------------------- 3 files changed, 94 insertions(+), 147 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index fc64e7fab..50111f4ac 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -56,73 +56,6 @@ logger = logging.getLogger(__name__.split(".")[0]) -def _uses_hash_storage(attr) -> bool: - """ - Check if an attribute uses hash-addressed storage. - - Hash-addressed types use content deduplication via MD5/Base32 hashing: - - - ```` - raw hash storage - - ```` - chains to ```` - - ```` - chains to ```` - - Parameters - ---------- - attr : Attribute - Attribute from table heading. - - Returns - ------- - bool - True if the attribute uses hash-addressed storage. - """ - if not attr.codec: - return False - - codec_name = getattr(attr.codec, "name", "") - store = getattr(attr, "store", None) - - # always uses hash-addressed storage (external only) - if codec_name == "hash": - return True - - # and use hash-addressed storage when external - if codec_name in ("blob", "attach") and store is not None: - return True - - return False - - -def _uses_schema_storage(attr) -> bool: - """ - Check if an attribute uses schema-addressed storage. - - Schema-addressed types store data at paths derived from the schema structure: - - - ```` - arbitrary objects (pickled or native formats) - - ```` - NumPy arrays with lazy loading - - Parameters - ---------- - attr : Attribute - Attribute from table heading. - - Returns - ------- - bool - True if the attribute uses schema-addressed storage. - """ - if not attr.codec: - return False - - # Recognize schema-addressed storage by type, not by a hardcoded codec name, - # so custom SchemaCodec subclasses (e.g. a user's NetCDF codec) are seen by - # GC and their live files are not misclassified as orphans (#1469). - from .builtin_codecs.schema import SchemaCodec - - return isinstance(attr.codec, SchemaCodec) - - def scan_hash_references( *schemas: "Schema", store_name: str | None = None, @@ -162,10 +95,10 @@ def scan_hash_references( # Get table class table = schema.get_table(table_name) - # Check each attribute for hash-addressed storage - for attr_name, attr in table.heading.attributes.items(): - if not _uses_hash_storage(attr): - continue + # Attributes stored in external hash-addressed storage + # (classification lives on the heading — Heading.hash_objects). + for attr_name in table.heading.hash_objects: + attr = table.heading.attributes[attr_name] if verbose: logger.info(f" Scanning {table_name}.{attr_name}") @@ -232,10 +165,10 @@ def scan_schema_references( # Get table class table = schema.get_table(table_name) - # Check each attribute for schema-addressed storage - for attr_name, attr in table.heading.attributes.items(): - if not _uses_schema_storage(attr): - continue + # Attributes stored in schema-addressed storage + # (classification lives on the heading — Heading.schema_objects). + for attr_name in table.heading.schema_objects: + attr = table.heading.attributes[attr_name] if verbose: logger.info(f" Scanning {table_name}.{attr_name}") diff --git a/src/datajoint/heading.py b/src/datajoint/heading.py index 8bd91ad3e..722c412f4 100644 --- a/src/datajoint/heading.py +++ b/src/datajoint/heading.py @@ -168,6 +168,41 @@ def original_name(self) -> str: assert self.attribute_expression.startswith(("`", '"')) return self.attribute_expression.strip('`"') + @property + def is_hash_object(self) -> bool: + """ + True if the value is stored in external hash-addressed storage. + + Covers ```` (external-only) and ````/```` *with* + a store. Inline ````/```` (no store) return False — their + bytes live in the column, so there is nothing in object storage to + collect. The ``store is not None`` test mirrors the framework's own + ``is_store`` determination (see ``declare``/``Heading`` compilation). + """ + if not self.codec: + return False + name = getattr(self.codec, "name", "") + if name == "hash": + return True # is external-only (get_dtype raises without @) + # / are dual-mode: external only when a store is set. + return name in ("blob", "attach") and self.store is not None + + @property + def is_schema_object(self) -> bool: + """ + True if the value is stored in schema-addressed storage. + + Covers ````, ````, and any custom ``SchemaCodec`` + subclass — recognized by codec *type*, not name, so third-party codecs + are included (see #1469). Schema-addressed storage is external-only, so + no store guard is needed. + """ + if not self.codec: + return False + from .builtin_codecs.schema import SchemaCodec # local import avoids import cycle + + return isinstance(self.codec, SchemaCodec) + class Heading: """ @@ -277,6 +312,18 @@ def non_blobs(self) -> list[str]: """Attributes that are not blobs or JSON.""" return [k for k, v in self.attributes.items() if not (v.is_blob or v.json)] + @property + def hash_objects(self) -> list[str]: + """Names of attributes stored in external hash-addressed storage (````, + external ````/````). See :attr:`Attribute.is_hash_object`.""" + return [k for k, v in self.attributes.items() if v.is_hash_object] + + @property + def schema_objects(self) -> list[str]: + """Names of attributes stored in schema-addressed storage (````, + ````, custom ``SchemaCodec``). See :attr:`Attribute.is_schema_object`.""" + return [k for k, v in self.attributes.items() if v.is_schema_object] + @property def new_attributes(self) -> list[str]: """Attributes with computed expressions (projections).""" diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 767fbf037..62cd737d2 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -61,51 +61,37 @@ class GcCustomCodecTest(dj.Manual): """ -class TestUsesHashStorage: - """Tests for _uses_hash_storage helper function.""" +def _make_attr(**overrides): + """Build a real heading.Attribute with defaults, overriding named fields.""" + from datajoint.heading import Attribute, default_attribute_properties - def test_returns_false_for_no_adapter(self): - """Test that False is returned when attribute has no codec.""" - attr = MagicMock() - attr.codec = None + return Attribute(**{**default_attribute_properties, **overrides}) - assert gc._uses_hash_storage(attr) is False - def test_returns_true_for_hash_type(self): - """Test that True is returned for type.""" - attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "hash" - attr.store = "mystore" +class TestAttributeIsHashObject: + """Attribute.is_hash_object classifies external hash-addressed storage + (moved from gc._uses_hash_storage — the classification now lives on the + heading so GC and other modules share one source of truth).""" - assert gc._uses_hash_storage(attr) is True + def test_false_for_no_codec(self): + assert _make_attr(codec=None).is_hash_object is False - def test_returns_true_for_blob_external(self): - """Test that True is returned for type (external).""" - attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "blob" - attr.store = "mystore" + def test_true_for_hash_type(self): + # is external-only; True regardless of store attribute value + assert _make_attr(codec=get_codec("hash"), store="mystore").is_hash_object is True - assert gc._uses_hash_storage(attr) is True + def test_true_for_blob_external(self): + assert _make_attr(codec=get_codec("blob"), store="mystore").is_hash_object is True - def test_returns_true_for_attach_external(self): - """Test that True is returned for type (external).""" - attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "attach" - attr.store = "mystore" + def test_true_for_attach_external(self): + assert _make_attr(codec=get_codec("attach"), store="mystore").is_hash_object is True - assert gc._uses_hash_storage(attr) is True + def test_false_for_blob_internal(self): + # inline (no store) lives in the column — not hash-addressed + assert _make_attr(codec=get_codec("blob"), store=None).is_hash_object is False - def test_returns_false_for_blob_internal(self): - """Test that False is returned for internal storage.""" - attr = MagicMock() - attr.codec = MagicMock() - attr.codec.name = "blob" - attr.store = None - - assert gc._uses_hash_storage(attr) is False + def test_false_for_schema_codec(self): + assert _make_attr(codec=get_codec("object"), store="mystore").is_hash_object is False class TestHashReferencedPaths: @@ -134,45 +120,25 @@ def test_returns_empty_for_dict_without_path(self): assert get_codec("hash").referenced_paths({"hash": "abc123"}) == [] -class TestUsesSchemaStorage: - """Tests for _uses_schema_storage helper function.""" - - def test_returns_false_for_no_adapter(self): - """Test that False is returned when attribute has no codec.""" - attr = MagicMock() - attr.codec = None - - assert gc._uses_schema_storage(attr) is False - - def test_returns_true_for_object_type(self): - """Test that True is returned for type.""" - attr = MagicMock() - attr.codec = get_codec("object") - - assert gc._uses_schema_storage(attr) is True - - def test_returns_true_for_npy_type(self): - """Test that True is returned for type.""" - attr = MagicMock() - attr.codec = get_codec("npy") +class TestAttributeIsSchemaObject: + """Attribute.is_schema_object classifies schema-addressed storage by codec + TYPE (so custom SchemaCodec subclasses are included, #1469).""" - assert gc._uses_schema_storage(attr) is True + def test_false_for_no_codec(self): + assert _make_attr(codec=None).is_schema_object is False - def test_returns_true_for_custom_schema_subclass(self): - """Recognition is by type, not name: a custom SchemaCodec subclass - (here, a subclass of ObjectCodec) must be seen as schema-addressed so - GC does not misclassify its live files as orphans (#1469).""" - attr = MagicMock() - attr.codec = get_codec("gc_custom_object") + def test_true_for_object_type(self): + assert _make_attr(codec=get_codec("object")).is_schema_object is True - assert gc._uses_schema_storage(attr) is True + def test_true_for_npy_type(self): + assert _make_attr(codec=get_codec("npy")).is_schema_object is True - def test_returns_false_for_other_types(self): - """Test that False is returned for non-schema-addressed types.""" - attr = MagicMock() - attr.codec = get_codec("blob") + def test_true_for_custom_schema_subclass(self): + # by type, not name — a custom ObjectCodec subclass counts (#1469) + assert _make_attr(codec=get_codec("gc_custom_object")).is_schema_object is True - assert gc._uses_schema_storage(attr) is False + def test_false_for_hash_codec(self): + assert _make_attr(codec=get_codec("blob"), store="mystore").is_schema_object is False class TestSchemaReferencedPaths: @@ -432,8 +398,9 @@ def schema_custom(self, connection_test, prefix, mock_stores): def test_custom_codec_reference_not_orphaned(self, schema_custom): """#1469: a live custom SchemaCodec value must be recognized as referenced and its file path must NOT be flagged as an orphan. Before - the fix, _uses_schema_storage keyed on the hardcoded names object/npy, - so this codec was never scanned and its live file was reported orphaned. + the fix, schema-addressed classification keyed on the hardcoded names + object/npy, so this codec was never scanned and its live file was + reported orphaned. Now Attribute.is_schema_object dispatches on type. Asserts on the specific live path (not global counts) so it is robust to other tests sharing the same ``local`` store. From b463702eaf9d035733899eeb81dfec5e0f90d099 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 15:30:34 -0500 Subject: [PATCH 15/20] refactor(gc): rename list_stored_hashes -> list_hash_paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/datajoint/gc.py | 6 +++--- tests/integration/test_gc.py | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 50111f4ac..c065a893e 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -194,7 +194,7 @@ def scan_schema_references( return referenced -def list_stored_hashes(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: +def list_hash_paths(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: """ List a schema's hash-addressed items in storage. @@ -508,7 +508,7 @@ def scan( # --- Hash-addressed storage (this schema's subtree) --- h_ref = scan_hash_references(schema, store_name=store_name, verbose=verbose) - h_stored = list_stored_hashes(store_name, config=_config, schema_name=db) + h_stored = list_hash_paths(store_name, config=_config, schema_name=db) orphaned_hashes |= set(h_stored.keys()) - h_ref # --- Schema-addressed storage (this schema's section) --- @@ -609,7 +609,7 @@ def collect( # per-schema) and merged across the passed schemas. hash_stored: dict[str, int] = {} for schema in schemas: - hash_stored.update(list_stored_hashes(store_name, config=_config, schema_name=schema.database)) + hash_stored.update(list_hash_paths(store_name, config=_config, schema_name=schema.database)) for path in stats["orphaned_hashes"]: try: diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 62cd737d2..c824806bf 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -179,7 +179,7 @@ def test_requires_at_least_one_schema(self): @patch("datajoint.gc.scan_schema_references") @patch("datajoint.gc.list_schema_paths") @patch("datajoint.gc.scan_hash_references") - @patch("datajoint.gc.list_stored_hashes") + @patch("datajoint.gc.list_hash_paths") def test_returns_stats(self, mock_list_hashes, mock_scan_hash, mock_list_schemas, mock_scan_schema): """Test that scan returns proper statistics.""" # Mock hash-addressed storage (now uses paths) @@ -243,7 +243,7 @@ def test_dry_run_does_not_delete(self, mock_scan): assert stats["dry_run"] is True @patch("datajoint.gc.delete_path") - @patch("datajoint.gc.list_stored_hashes") + @patch("datajoint.gc.list_hash_paths") @patch("datajoint.gc.scan") def test_deletes_orphaned_hashes(self, mock_scan, mock_list_stored, mock_delete): """Test that orphaned hashes are deleted when dry_run=False.""" @@ -753,7 +753,7 @@ def test_list_scoped_to_schema(self, two_schemas): b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-obj"}) - a_hashes = set(gc.list_stored_hashes("local", config=cfg, schema_name=a.database)) + a_hashes = set(gc.list_hash_paths("local", config=cfg, schema_name=a.database)) a_paths = set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) assert a_hashes and all(f"/{a.database}/" in h for h in a_hashes), a_hashes assert a_paths and all(p.split("/")[0] == a.database or f"/{a.database}/" in p for p in a_paths), a_paths @@ -782,7 +782,7 @@ def test_collect_one_schema_never_touches_the_other(self, two_schemas): a_orphan = next(iter(gc.scan_schema_references(a, store_name="local"))) (GcObjectTest & {"rid": 1}).delete(prompt=False) - b_hashes_before = set(gc.list_stored_hashes("local", config=cfg, schema_name=b.database)) + b_hashes_before = set(gc.list_hash_paths("local", config=cfg, schema_name=b.database)) b_paths_before = set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) assert b_hashes_before and b_paths_before @@ -790,6 +790,6 @@ def test_collect_one_schema_never_touches_the_other(self, two_schemas): # a's orphan reclaimed; b entirely intact assert a_orphan not in set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) - assert set(gc.list_stored_hashes("local", config=cfg, schema_name=b.database)) == b_hashes_before + assert set(gc.list_hash_paths("local", config=cfg, schema_name=b.database)) == b_hashes_before assert set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) == b_paths_before assert len(b_obj()) == 1 # b's row (and its object) untouched From 162ccb7b09f75af036f08a2760614d5da0994505 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 17:05:15 -0500 Subject: [PATCH 16/20] refactor(gc): store-bound GarbageCollector class as the sole API 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. --- src/datajoint/__init__.py | 2 +- src/datajoint/builtin_codecs/hash.py | 2 +- src/datajoint/builtin_codecs/npy.py | 2 +- src/datajoint/builtin_codecs/object.py | 2 +- src/datajoint/gc.py | 913 +++++++++---------------- src/datajoint/hash_registry.py | 2 +- tests/integration/test_gc.py | 243 +++---- 7 files changed, 471 insertions(+), 695 deletions(-) diff --git a/src/datajoint/__init__.py b/src/datajoint/__init__.py index 2e242ce84..9728d2781 100644 --- a/src/datajoint/__init__.py +++ b/src/datajoint/__init__.py @@ -284,7 +284,7 @@ def FreeTable(conn_or_name, full_table_name: str | None = None) -> _FreeTable: "diagram": (".diagram", None), # Return the module itself # cli imports click "cli": (".cli", "cli"), - # gc — exposed lazily so `dj.gc.scan(...)` works as documented in gc.py + # gc — exposed lazily so `dj.gc.GarbageCollector(...)` works as documented in gc.py # and in the user docs (how-to/garbage-collection.md). "gc": (".gc", None), # Return the module itself } diff --git a/src/datajoint/builtin_codecs/hash.py b/src/datajoint/builtin_codecs/hash.py index bb3a3852f..4873c073f 100644 --- a/src/datajoint/builtin_codecs/hash.py +++ b/src/datajoint/builtin_codecs/hash.py @@ -21,7 +21,7 @@ class HashCodec(Codec): The database column stores JSON metadata: ``{hash, store, size}``. Duplicate content is automatically deduplicated across all tables. - Deletion: Requires garbage collection via ``dj.gc.collect()``. + Deletion: Requires garbage collection via ``dj.gc.GarbageCollector``. External only - requires @ modifier. diff --git a/src/datajoint/builtin_codecs/npy.py b/src/datajoint/builtin_codecs/npy.py index 54853437b..3094dfb90 100644 --- a/src/datajoint/builtin_codecs/npy.py +++ b/src/datajoint/builtin_codecs/npy.py @@ -272,7 +272,7 @@ class Recording(dj.Manual): - Path: ``{schema}/{table}/{pk}/{attribute}.npy`` - Database column: JSON with ``{path, store, dtype, shape}`` - Deletion: Requires garbage collection via ``dj.gc.collect()``. + Deletion: Requires garbage collection via ``dj.gc.GarbageCollector``. See Also -------- diff --git a/src/datajoint/builtin_codecs/object.py b/src/datajoint/builtin_codecs/object.py index 1c0d8c673..08ef0e188 100644 --- a/src/datajoint/builtin_codecs/object.py +++ b/src/datajoint/builtin_codecs/object.py @@ -53,7 +53,7 @@ def make(self, key): {store_root}/{schema}/{table}/{pk}/{field}/ - Deletion: Requires garbage collection via ``dj.gc.collect()``. + Deletion: Requires garbage collection via ``dj.gc.GarbageCollector``. Comparison with hash-addressed:: diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index c065a893e..e4591bde5 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -1,9 +1,8 @@ """ Garbage collection for object storage. -This module provides utilities to identify and remove orphaned items -from object storage. Storage items become orphaned when all database rows -referencing them are deleted. +This module identifies and removes orphaned items from object storage. Storage +items become orphaned when all database rows referencing them are deleted. DataJoint uses two object storage patterns: @@ -21,15 +20,15 @@ Deduplication: None (each entity has unique path) Deletion: Requires garbage collection -Usage:: +Garbage collection is **store-specific** and handles one store at a time. The +:class:`GarbageCollector` is bound to a store (and the config that defines it) +at construction; its store and config are not threaded through each call:: import datajoint as dj - # Scan schemas and find orphaned items - stats = dj.gc.scan(schema1, schema2, store_name='mystore') - - # Remove orphaned items (dry_run=False to actually delete) - stats = dj.gc.collect(schema1, schema2, store_name='mystore', dry_run=True) + collector = dj.gc.GarbageCollector(store="mystore") + stats = collector.scan(schema1, schema2) # read-only + stats = collector.collect(schema1, schema2, dry_run=False) Both sections embed the schema name in every path, so garbage collection is **per-schema**: each schema is scanned against its own subtree only. Orphan @@ -45,362 +44,19 @@ from __future__ import annotations import logging +import re from typing import TYPE_CHECKING, Any -from .hash_registry import delete_path, get_store_backend from .errors import DataJointError +from .hash_registry import delete_path, get_store_backend if TYPE_CHECKING: from .schemas import _Schema as Schema logger = logging.getLogger(__name__.split(".")[0]) - -def scan_hash_references( - *schemas: "Schema", - store_name: str | None = None, - verbose: bool = False, -) -> set[str]: - """ - Scan schemas for hash-addressed storage references. - - Examines all tables in the given schemas and extracts storage paths - from columns that use hash-addressed storage (````, ````, - ````). - - Parameters - ---------- - *schemas : Schema - Schema instances to scan. - store_name : str, optional - Only include references to this store (None = all stores). - verbose : bool, optional - Print progress information. - - Returns - ------- - set[str] - Full relative store paths referenced by live rows, exactly as recorded - in each row's metadata (independent of the store's current prefixes). - """ - referenced: set[str] = set() - - for schema in schemas: - if verbose: - logger.info(f"Scanning schema: {schema.database}") - - # Get all tables in schema - for table_name in schema.list_tables(): - try: - # Get table class - table = schema.get_table(table_name) - - # Attributes stored in external hash-addressed storage - # (classification lives on the heading — Heading.hash_objects). - for attr_name in table.heading.hash_objects: - attr = table.heading.attributes[attr_name] - - if verbose: - logger.info(f" Scanning {table_name}.{attr_name}") - - # Read raw JSON metadata via cursor — bypasses decode_attribute - # so we get the stored dict (PostgreSQL/JSONB) or JSON string - # (MySQL), not the decoded codec output. The codec's own - # referenced_paths() extracts the referenced paths and handles - # both shapes (codec-driven discovery, #1469). - try: - cursor = table.proj(attr_name).cursor(as_dict=True) - for row in cursor: - for path, ref_store in attr.codec.referenced_paths(row[attr_name]): - # Filter by store if specified - if store_name is None or ref_store == store_name: - referenced.add(path) - except Exception as e: - logger.warning(f"Error scanning {table_name}.{attr_name}: {e}") - - except Exception as e: - logger.warning(f"Error accessing table {table_name}: {e}") - - return referenced - - -def scan_schema_references( - *schemas: "Schema", - store_name: str | None = None, - verbose: bool = False, -) -> set[str]: - """ - Scan schemas for schema-addressed storage references. - - Examines all tables in the given schemas and extracts paths from columns - that use schema-addressed storage — any ``SchemaCodec`` (built-in - ````, ```` and custom subclasses, recognized by type per - #1469), not a fixed codec-name list. - - Parameters - ---------- - *schemas : Schema - Schema instances to scan. - store_name : str, optional - Only include references to this store (None = all stores). - verbose : bool, optional - Print progress information. - - Returns - ------- - set[str] - Full relative store paths referenced by live rows, exactly as recorded - in each row's metadata. For a directory-valued object the recorded - path is the directory prefix, not its individual files. - """ - referenced: set[str] = set() - - for schema in schemas: - if verbose: - logger.info(f"Scanning schema for schema-addressed storage: {schema.database}") - - # Get all tables in schema - for table_name in schema.list_tables(): - try: - # Get table class - table = schema.get_table(table_name) - - # Attributes stored in schema-addressed storage - # (classification lives on the heading — Heading.schema_objects). - for attr_name in table.heading.schema_objects: - attr = table.heading.attributes[attr_name] - - if verbose: - logger.info(f" Scanning {table_name}.{attr_name}") - - # Read raw JSON metadata via cursor — bypasses decode_attribute - # so we get the stored dict (PostgreSQL/JSONB) or JSON string - # (MySQL), not the decoded codec output. The codec's own - # referenced_paths() extracts the referenced paths and handles - # both shapes (codec-driven discovery, #1469). - try: - cursor = table.proj(attr_name).cursor(as_dict=True) - for row in cursor: - for path, ref_store in attr.codec.referenced_paths(row[attr_name]): - # Filter by store if specified - if store_name is None or ref_store == store_name: - referenced.add(path) - except Exception as e: - logger.warning(f"Error scanning {table_name}.{attr_name}: {e}") - - except Exception as e: - logger.warning(f"Error accessing table {table_name}: {e}") - - return referenced - - -def list_hash_paths(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: - """ - List a schema's hash-addressed items in storage. - - Walks exactly one schema's subtree of the configured hash-addressed - section — ``{hash_prefix}/{schema_name}/`` (``hash_prefix`` default - ``_hash``) — and returns the stored objects (````, ````, - ````). Every hash path embeds the schema and deduplication is - per-schema, so this listing is complete for that schema and independent of - every other schema on the store. - - Only the CURRENTLY configured prefix is walked: objects written under a - former prefix are not listed here (they remain readable via their metadata - paths). ``schema_name`` is required — GC is always per-schema. - - Parameters - ---------- - store_name : str, optional - Store to scan (None = default store). - config : Config, optional - Config instance. If None, falls back to global settings.config. - schema_name : str - Schema whose hash subtree to list (required, keyword-only). - - Returns - ------- - dict[str, int] - Dict mapping each object's full relative store path - (``{hash_prefix}/{schema}/[{subfolders}/]{hash}``) to its size in bytes. - """ - import re - - backend = get_store_backend(store_name, config=config) - stored: dict[str, int] = {} - - if config is None: - from .settings import config as _global_config - - config = _global_config - # Hash-addressed storage: {hash_prefix}/{schema}/{subfolders...}/{hash}. - # The prefix comes from the store's settings — the same value the writer - # uses — so scanner and writer cannot drift. - _spec = config.get_store_spec(store_name) - hash_prefix = _spec["hash_prefix"].strip("/") + "/" # settings applies the "_hash" default - section = f"{hash_prefix}{schema_name}/" - # Base32 pattern: 26 lowercase alphanumeric chars - base32_pattern = re.compile(r"^[a-z2-7]{26}$") - - try: - full_prefix = backend._full_path(section) - - for root, dirs, files in backend.fs.walk(full_prefix): - for filename in files: - # Skip manifest files - if filename.endswith(".manifest.json"): - continue - - # The filename is the base32 hash - content_hash = filename - - # Validate it looks like a base32 hash - if base32_pattern.match(content_hash): - try: - file_path = f"{root}/{filename}" - size = backend.fs.size(file_path) - # Build relative path for comparison with stored metadata - # Path format: {hash_prefix}/{schema}/{subfolders...}/{hash} - relative_path = file_path.replace(backend._full_path(""), "").lstrip("/") - stored[relative_path] = size - except Exception: - pass - - except FileNotFoundError: - # The hash section does not exist yet - pass - except Exception as e: - logger.warning(f"Error listing stored hashes: {e}") - - return stored - - -def list_schema_paths(store_name: str | None = None, config=None, *, schema_name: str) -> dict[str, int]: - """ - List a schema's schema-addressed object files in storage. - - Walks exactly one schema's section — ``{schema_prefix}/{schema_name}/`` - (``schema_prefix`` default ``_schema``) — and enumerates the individual - **files** written by schema-addressed codecs. A single-file object is one - file at ``{schema_prefix}/{schema}/{table}/{pk}/{field}_{token}[.ext]``; a - directory-valued object (e.g. a Zarr store) is many files under that path - prefix, plus a ``{path}.manifest.json`` sidecar beside it — all are listed. - Orphan detection matches these files against each row's referenced object - path via :func:`_is_covered` (exact, ancestor-prefix, or manifest-sidecar), - so superseded per-token versions are reclaimable while every file belonging - to a live object — including its manifest — is kept. - - The walk is confined to the schema's own section, so it never enters the - hash section or the filepath section (mutually-exclusive top-level - prefixes, enforced by store validation) — GC's blast radius is exactly - ``{schema_prefix}/{schema}/``, and user-managed ```` content and - other schemas' objects are structurally out of reach. - - ``schema_name`` is required — GC is always per-schema. - - Legacy layout: DataJoint 2.3.0 and earlier wrote schema-addressed objects - at root level ``{schema}/...`` (``schema_prefix`` was ignored — see - datajoint/datajoint-python#1487). Such a store should set - ``schema_prefix=""`` so both writes and this walk use the root-level - layout consistently; otherwise those objects remain readable (metadata - records full paths) but are outside the scanned section. - - Parameters - ---------- - store_name : str, optional - Store to scan (None = default store). - config : Config, optional - Config instance. If None, falls back to global settings.config. - schema_name : str - Schema whose section to list (required, keyword-only). - - Returns - ------- - dict[str, int] - Dict mapping each object file's relative path to its size in bytes. - """ - backend = get_store_backend(store_name, config=config) - stored: dict[str, int] = {} - - if config is None: - from .settings import config as _global_config - - config = _global_config - _spec = config.get_store_spec(store_name) - _sp = _spec["schema_prefix"].strip("/") # settings applies the "_schema" default - rel = f"{_sp}/{schema_name}" if _sp else schema_name - full_root = backend._full_path("") - - try: - for root, dirs, files in backend.fs.walk(backend._full_path(rel)): - for filename in files: - # Manifest sidecars ({object}.manifest.json) are INCLUDED: they - # are owned by their object and must be reclaimed with it when - # the object is orphaned. _is_covered() keeps a live object's - # manifest out of the orphan set. - file_path = f"{root}/{filename}" - relative_path = file_path.replace(full_root, "").lstrip("/") - try: - stored[relative_path] = backend.fs.size(file_path) - except Exception: - stored[relative_path] = 0 - except FileNotFoundError: - pass # this schema's section does not exist yet - except Exception as e: - logger.warning(f"Error listing stored schemas: {e}") - - return stored - - -def delete_schema_path(path: str, store_name: str | None = None, config=None) -> bool: - """ - Delete a schema-addressed object file from storage. - - ``path`` is an individual object file (as enumerated by - :func:`list_schema_paths`), not a directory — so only the orphaned token - file is removed, leaving other versions/fields under the same primary-key - directory intact. Empty parent directories are pruned best-effort. - - Parameters - ---------- - path : str - Object file path (relative to store root). - store_name : str, optional - Store name (None = default store). - config : Config, optional - Config instance. If None, falls back to global settings.config. - - Returns - ------- - bool - True if deleted, False if not found. - """ - backend = get_store_backend(store_name, config=config) - - try: - full_path = backend._full_path(path) - if backend.fs.exists(full_path): - # Remove just this object file (not the whole PK directory) - backend.fs.rm(full_path) - logger.debug(f"Deleted schema object: {path}") - - # Best-effort: prune now-empty parent directories up to the store root. - root = backend._full_path("").rstrip("/") - parent = full_path.rsplit("/", 1)[0] - while parent and parent != root and parent.startswith(root): - try: - if backend.fs.ls(parent): - break # not empty - backend.fs.rmdir(parent) - except Exception: - break - parent = parent.rsplit("/", 1)[0] - - return True - except Exception as e: - logger.warning(f"Error deleting schema object {path}: {e}") - - return False +# Base32 content-hash filename: 26 lowercase alphanumeric chars. +_BASE32_HASH = re.compile(r"^[a-z2-7]{26}$") def _is_covered(path: str, referenced: set[str]) -> bool: @@ -429,229 +85,342 @@ def _is_covered(path: str, referenced: set[str]) -> bool: return False -def scan( - *schemas: "Schema", - store_name: str | None = None, - verbose: bool = False, -) -> dict[str, Any]: +class GarbageCollector: """ - Scan for orphaned storage items without deleting. - - Scans both hash-addressed storage (````, ````, ````) - and schema-addressed storage (any ``SchemaCodec``: ````, ```` - and custom subclasses). Section locations follow the store's ``hash_prefix`` - / ``schema_prefix`` settings. - - Per-schema, and safe for subsets. Every managed object embeds its schema - in the path (both sections, plus the legacy root-level layout), and hash - deduplication is per-schema — so each schema is scanned independently - against ITS OWN subtree. Orphan detection is therefore confined to the - schemas you pass: objects of other schemas sharing the store are never - listed and never at risk. (This removes the former requirement to pass - *every* schema on a store at once.) - - Each schema's two sections are walked in isolation - (``{hash_prefix}/{schema}/`` and ``{schema_prefix}/{schema}/``), so GC's - blast radius is exactly those folders: user ```` content and - other schemas' objects are structurally out of reach. Objects under a - *former* prefix (after a prefix change on a populated store) stay readable - via their metadata paths but are not reclamation candidates until the - setting is restored. + Store-specific garbage collector — handles exactly one store. - Parameters - ---------- - *schemas : Schema - Schema instances to scan. Each is scanned against its own subtree; - any subset of the schemas on a store may be passed safely. - store_name : str, optional - Store to check (None = default store). - verbose : bool, optional - Print progress information. - - Returns - ------- - dict[str, Any] - Dict with scan statistics: - - - hash_referenced: Number of hash items referenced in database - - hash_stored: Number of hash items in storage - - hash_orphaned: Number of unreferenced hash items - - hash_orphaned_bytes: Total size of orphaned hashes - - orphaned_hashes: List of orphaned hash items as full relative store - paths (NOT bare hashes — passed as-is to the deleter) - - schema_paths_referenced: Number of schema items referenced in database - - schema_paths_stored: Number of schema files in storage - - schema_paths_orphaned: Number of unreferenced schema files - - schema_paths_orphaned_bytes: Total size of orphaned schema files - - orphaned_paths: List of orphaned schema files as full relative store paths - """ - if not schemas: - raise DataJointError("At least one schema must be provided") - - # Extract config from the first schema's connection - _config = schemas[0].connection._config if schemas else None - - hash_referenced: set[str] = set() - hash_stored: dict[str, int] = {} - orphaned_hashes: set[str] = set() - schema_paths_referenced: set[str] = set() - schema_paths_stored: dict[str, int] = {} - orphaned_paths: set[str] = set() - - # Scan each schema against its own subtree only. Because both sections - # embed the schema name and hash dedup is per-schema, a schema's orphans - # are fully determined by its own references vs. its own stored files — - # no other schema on the store can influence (or be endangered by) the - # result. Stats below aggregate across the passed schemas. - for schema in schemas: - db = schema.database - - # --- Hash-addressed storage (this schema's subtree) --- - h_ref = scan_hash_references(schema, store_name=store_name, verbose=verbose) - h_stored = list_hash_paths(store_name, config=_config, schema_name=db) - orphaned_hashes |= set(h_stored.keys()) - h_ref - - # --- Schema-addressed storage (this schema's section) --- - s_ref = scan_schema_references(schema, store_name=store_name, verbose=verbose) - s_stored = list_schema_paths(store_name, config=_config, schema_name=db) - # Coverage, not exact set difference: a referenced path may be a - # DIRECTORY-valued object (e.g. a Zarr store), whose stored form is - # many files under that prefix, plus a `.manifest.json` sidecar. The - # schema walk is confined to this schema's schema_prefix section, so it - # can only contain this schema's schema-addressed objects — coverage - # against s_ref alone is complete (no hash objects can appear here). - orphaned_paths |= {p for p in s_stored if not _is_covered(p, s_ref)} - - hash_referenced |= h_ref - hash_stored.update(h_stored) - schema_paths_referenced |= s_ref - schema_paths_stored.update(s_stored) - - hash_orphaned_bytes = sum(hash_stored.get(h, 0) for h in orphaned_hashes) - schema_paths_orphaned_bytes = sum(schema_paths_stored.get(p, 0) for p in orphaned_paths) - - return { - # Hash-addressed storage stats - "hash_referenced": len(hash_referenced), - "hash_stored": len(hash_stored), - "hash_orphaned": len(orphaned_hashes), - "hash_orphaned_bytes": hash_orphaned_bytes, - "orphaned_hashes": sorted(orphaned_hashes), - # Schema-addressed storage stats - "schema_paths_referenced": len(schema_paths_referenced), - "schema_paths_stored": len(schema_paths_stored), - "schema_paths_orphaned": len(orphaned_paths), - "schema_paths_orphaned_bytes": schema_paths_orphaned_bytes, - "orphaned_paths": sorted(orphaned_paths), - # Combined totals - "referenced": len(hash_referenced) + len(schema_paths_referenced), - "stored": len(hash_stored) + len(schema_paths_stored), - "orphaned": len(orphaned_hashes) + len(orphaned_paths), - "orphaned_bytes": hash_orphaned_bytes + schema_paths_orphaned_bytes, - } - - -def collect( - *schemas: "Schema", - store_name: str | None = None, - dry_run: bool = True, - verbose: bool = False, -) -> dict[str, Any]: - """ - Remove orphaned storage items. - - Scans the given schemas (via :func:`scan`) for storage references, then - removes items covered by neither the schema nor hash reference set. Orphan - identity therefore comes entirely from :func:`scan`: live objects outside - the current section (after a prefix change) and the store's declared - ``filepath_prefix`` namespace are never deleted. + Bound to a store name and the config that defines it at construction, so + neither is threaded through individual operations. Storage-side work (the + stored-file listings, deletion) uses this store/config; database metadata + is read through each scanned schema's own connection. Parameters ---------- - *schemas : Schema - Schema instances to scan. - store_name : str, optional - Store to clean (None = default store). - dry_run : bool, optional - If True, report what would be deleted without deleting. Default True. - verbose : bool, optional - Print progress information. - - Returns - ------- - dict[str, Any] - Dict with collection statistics: - - - referenced: Total items referenced in database - - stored: Total items in storage - - orphaned: Total unreferenced items - - hash_deleted: Number of hash items deleted - - schema_paths_deleted: Number of schema items deleted - - deleted: Total items deleted (0 if dry_run) - - bytes_freed: Bytes freed (0 if dry_run) - - errors: Number of deletion errors + store : str, optional + Store name (None = the default store). Resolved at construction — an + unknown/misconfigured store raises immediately. + config : Config, optional + Config that defines the store. If None, the global ``settings.config`` + is used. """ - # First scan to find orphaned items - stats = scan(*schemas, store_name=store_name, verbose=verbose) - - # Extract config from the first schema's connection - _config = schemas[0].connection._config if schemas else None - - hash_deleted = 0 - schema_paths_deleted = 0 - bytes_freed = 0 - errors = 0 - - if not dry_run: - # Delete orphaned hashes - if stats["hash_orphaned"] > 0: - # Size map for bytes_freed — built per schema (listers are - # per-schema) and merged across the passed schemas. - hash_stored: dict[str, int] = {} - for schema in schemas: - hash_stored.update(list_hash_paths(store_name, config=_config, schema_name=schema.database)) - - for path in stats["orphaned_hashes"]: - try: - size = hash_stored.get(path, 0) - if delete_path(path, store_name, config=_config): - hash_deleted += 1 - bytes_freed += size - if verbose: - logger.info(f"Deleted: {path} ({size} bytes)") - except Exception as e: - errors += 1 - logger.warning(f"Failed to delete {path}: {e}") - # Delete orphaned schema paths - if stats["schema_paths_orphaned"] > 0: - schema_paths_stored: dict[str, int] = {} - for schema in schemas: - schema_paths_stored.update(list_schema_paths(store_name, config=_config, schema_name=schema.database)) - - for path in stats["orphaned_paths"]: + def __init__(self, store: str | None = None, *, config=None) -> None: + if config is None: + from .settings import config as _global_config + + config = _global_config + self.store = store + self.config = config + # Resolve the store eagerly: validates it exists and pins the backend + # and section prefixes for this collector's lifetime (one store only). + self.backend = get_store_backend(store, config=config) + spec = config.get_store_spec(store) + self._hash_prefix = spec["hash_prefix"].strip("/") # settings applies the "_hash" default + self._schema_prefix = spec["schema_prefix"].strip("/") # ... "_schema" default + + # ------------------------------------------------------------------ # + # References — extracted from database metadata (per-schema connection) + # ------------------------------------------------------------------ # + def hash_references(self, *schemas: "Schema", verbose: bool = False) -> set[str]: + """ + Full relative store paths of hash-addressed objects referenced by live rows. + + Reads columns classified by ``Heading.hash_objects`` (````, and + external ````/````). Paths are taken verbatim from each + row's metadata (independent of the store's current prefixes) and + filtered to this collector's store. + """ + return self._references("hash_objects", schemas, verbose) + + def schema_references(self, *schemas: "Schema", verbose: bool = False) -> set[str]: + """ + Full relative store paths of schema-addressed objects referenced by live rows. + + Reads columns classified by ``Heading.schema_objects`` — any + ``SchemaCodec`` (````, ````, custom subclasses, + recognized by type per #1469). For a directory-valued object the + recorded path is the directory prefix, not its individual files. + """ + return self._references("schema_objects", schemas, verbose) + + def _references(self, heading_attr: str, schemas, verbose: bool) -> set[str]: + referenced: set[str] = set() + for schema in schemas: + if verbose: + logger.info(f"Scanning schema {schema.database} for {heading_attr}") + for table_name in schema.list_tables(): try: - size = schema_paths_stored.get(path, 0) - if delete_schema_path(path, store_name, config=_config): - schema_paths_deleted += 1 - bytes_freed += size + table = schema.get_table(table_name) + # Classification lives on the heading — one source of truth. + for attr_name in getattr(table.heading, heading_attr): + attr = table.heading.attributes[attr_name] if verbose: - logger.info(f"Deleted schema path: {path} ({size} bytes)") + logger.info(f" Scanning {table_name}.{attr_name}") + # Read raw JSON metadata via cursor — bypasses + # decode_attribute, so we get the stored dict + # (PostgreSQL/JSONB) or JSON string (MySQL). The codec's + # referenced_paths() extracts paths and handles both + # shapes (codec-driven discovery, #1469). + try: + for row in table.proj(attr_name).cursor(as_dict=True): + for path, ref_store in attr.codec.referenced_paths(row[attr_name]): + if self.store is None or ref_store == self.store: + referenced.add(path) + except Exception as e: + logger.warning(f"Error scanning {table_name}.{attr_name}: {e}") except Exception as e: - errors += 1 - logger.warning(f"Failed to delete schema path {path}: {e}") - - return { - "referenced": stats["referenced"], - "stored": stats["stored"], - "orphaned": stats["orphaned"], - "hash_deleted": hash_deleted, - "schema_paths_deleted": schema_paths_deleted, - "deleted": hash_deleted + schema_paths_deleted, - "bytes_freed": bytes_freed, - "errors": errors, - "dry_run": dry_run, - # Include detailed stats - "hash_orphaned": stats["hash_orphaned"], - "schema_paths_orphaned": stats["schema_paths_orphaned"], - } + logger.warning(f"Error accessing table {table_name}: {e}") + return referenced + + # ------------------------------------------------------------------ # + # Stored listings — walk this store, one schema's subtree + # ------------------------------------------------------------------ # + def list_hash_paths(self, schema_name: str) -> dict[str, int]: + """ + List one schema's hash-addressed items: ``{hash_prefix}/{schema}/...``. + + Every hash path embeds the schema and deduplication is per-schema, so + this listing is complete for that schema and independent of every other + schema on the store. Only the currently configured prefix is walked; + objects under a former prefix are not listed (they remain readable via + their metadata paths). + + Returns a dict mapping each object's full relative store path to size. + """ + hp = self._hash_prefix + section = f"{hp}/{schema_name}/" if hp else f"{schema_name}/" + full_root = self.backend._full_path("") + stored: dict[str, int] = {} + try: + for root, _dirs, files in self.backend.fs.walk(self.backend._full_path(section)): + for filename in files: + if filename.endswith(".manifest.json"): + continue # folder-object sidecar, not a hash file + if not _BASE32_HASH.match(filename): + continue + file_path = f"{root}/{filename}" + try: + relative_path = file_path.replace(full_root, "").lstrip("/") + stored[relative_path] = self.backend.fs.size(file_path) + except Exception: + pass + except FileNotFoundError: + pass # the hash section does not exist yet + except Exception as e: + logger.warning(f"Error listing stored hashes: {e}") + return stored + + def list_schema_paths(self, schema_name: str) -> dict[str, int]: + """ + List one schema's schema-addressed object files: ``{schema_prefix}/{schema}/...``. + + Enumerates individual files: a single-file object is one file at + ``{schema_prefix}/{schema}/{table}/{pk}/{field}_{token}[.ext]``; a + directory-valued object (e.g. a Zarr store) is many files under that + prefix, plus a ``{path}.manifest.json`` sidecar — all listed. Orphan + detection matches them against references via :func:`_is_covered`. + + The walk is confined to the schema's own section, so it never enters the + hash or filepath sections (mutually-exclusive top-level prefixes, + enforced by store validation) — GC's blast radius is exactly + ``{schema_prefix}/{schema}/``; user ```` content and other + schemas' objects are structurally out of reach. + + Legacy layout: DataJoint 2.3.0 and earlier wrote schema-addressed + objects at root level ``{schema}/...`` (``schema_prefix`` ignored, see + datajoint/datajoint-python#1487). Such a store should set + ``schema_prefix=""`` so writes and this walk use the root-level layout + consistently; otherwise those objects remain readable but outside the + scanned section. + + Returns a dict mapping each object file's relative path to size. + """ + sp = self._schema_prefix + rel = f"{sp}/{schema_name}" if sp else schema_name + full_root = self.backend._full_path("") + stored: dict[str, int] = {} + try: + for root, _dirs, files in self.backend.fs.walk(self.backend._full_path(rel)): + for filename in files: + # Manifest sidecars are INCLUDED: owned by their object and + # reclaimed with it. _is_covered() keeps a live object's + # manifest out of the orphan set. + file_path = f"{root}/{filename}" + relative_path = file_path.replace(full_root, "").lstrip("/") + try: + stored[relative_path] = self.backend.fs.size(file_path) + except Exception: + stored[relative_path] = 0 + except FileNotFoundError: + pass # this schema's section does not exist yet + except Exception as e: + logger.warning(f"Error listing stored schemas: {e}") + return stored + + # ------------------------------------------------------------------ # + # Deletion + # ------------------------------------------------------------------ # + def delete_schema_path(self, path: str) -> bool: + """ + Delete a single schema-addressed object file (not a directory). + + ``path`` is an individual file (as enumerated by + :meth:`list_schema_paths`), so only the orphaned token file is removed, + leaving other versions/fields under the same primary-key directory + intact. Empty parent directories are pruned best-effort. Returns True if + deleted, False if not found. + """ + try: + full_path = self.backend._full_path(path) + if self.backend.fs.exists(full_path): + self.backend.fs.rm(full_path) + logger.debug(f"Deleted schema object: {path}") + # Best-effort: prune now-empty parent directories to store root. + root = self.backend._full_path("").rstrip("/") + parent = full_path.rsplit("/", 1)[0] + while parent and parent != root and parent.startswith(root): + try: + if self.backend.fs.ls(parent): + break # not empty + self.backend.fs.rmdir(parent) + except Exception: + break + parent = parent.rsplit("/", 1)[0] + return True + except Exception as e: + logger.warning(f"Error deleting schema object {path}: {e}") + return False + + # ------------------------------------------------------------------ # + # Orchestration + # ------------------------------------------------------------------ # + def scan(self, *schemas: "Schema", verbose: bool = False) -> dict[str, Any]: + """ + Scan for orphaned storage items without deleting. + + Scans both hash-addressed and schema-addressed storage. Each schema is + scanned against ITS OWN subtree, so orphan detection is confined to the + schemas passed — objects of other schemas sharing the store are never + listed and never at risk; any subset may be passed safely. Objects + under a *former* prefix stay readable via their metadata paths but are + not reclamation candidates until the setting is restored. + + Returns a dict with per-section statistics: + + - hash_referenced / hash_stored / hash_orphaned / hash_orphaned_bytes + - orphaned_hashes: orphaned hash items as full relative store paths + (NOT bare hashes — passed as-is to the deleter) + - schema_paths_referenced / schema_paths_stored / schema_paths_orphaned + / schema_paths_orphaned_bytes + - orphaned_paths: orphaned schema files as full relative store paths + """ + if not schemas: + raise DataJointError("At least one schema must be provided") + + hash_referenced: set[str] = set() + hash_stored: dict[str, int] = {} + orphaned_hashes: set[str] = set() + schema_paths_referenced: set[str] = set() + schema_paths_stored: dict[str, int] = {} + orphaned_paths: set[str] = set() + + for schema in schemas: + # --- Hash-addressed storage (this schema's subtree) --- + h_ref = self.hash_references(schema, verbose=verbose) + h_stored = self.list_hash_paths(schema.database) + orphaned_hashes |= set(h_stored.keys()) - h_ref + + # --- Schema-addressed storage (this schema's section) --- + s_ref = self.schema_references(schema, verbose=verbose) + s_stored = self.list_schema_paths(schema.database) + # Coverage, not exact set difference: a referenced path may be a + # directory-valued object (many files + a manifest). The walk is + # confined to this schema's section, so coverage against s_ref alone + # is complete (no hash objects can appear here). + orphaned_paths |= {p for p in s_stored if not _is_covered(p, s_ref)} + + hash_referenced |= h_ref + hash_stored.update(h_stored) + schema_paths_referenced |= s_ref + schema_paths_stored.update(s_stored) + + return { + # Hash-addressed storage stats + "hash_referenced": len(hash_referenced), + "hash_stored": len(hash_stored), + "hash_orphaned": len(orphaned_hashes), + "hash_orphaned_bytes": sum(hash_stored.get(h, 0) for h in orphaned_hashes), + "orphaned_hashes": sorted(orphaned_hashes), + # Schema-addressed storage stats + "schema_paths_referenced": len(schema_paths_referenced), + "schema_paths_stored": len(schema_paths_stored), + "schema_paths_orphaned": len(orphaned_paths), + "schema_paths_orphaned_bytes": sum(schema_paths_stored.get(p, 0) for p in orphaned_paths), + "orphaned_paths": sorted(orphaned_paths), + } + + def collect(self, *schemas: "Schema", dry_run: bool = True, verbose: bool = False) -> dict[str, Any]: + """ + Remove orphaned storage items. + + Scans the given schemas (via :meth:`scan`), then removes the orphans it + identifies. Orphan identity comes entirely from :meth:`scan`, so live + objects outside the current section (after a prefix change), other + schemas' objects, and the ``filepath_prefix`` namespace are never + deleted. + + Returns a dict with: hash_deleted, schema_paths_deleted, deleted, + bytes_freed (0 if dry_run), errors, dry_run, and the per-section orphan + counts (hash_orphaned, schema_paths_orphaned). + """ + stats = self.scan(*schemas, verbose=verbose) + + hash_deleted = 0 + schema_paths_deleted = 0 + bytes_freed = 0 + errors = 0 + + if not dry_run: + if stats["hash_orphaned"] > 0: + # Size map for bytes_freed, merged across the passed schemas. + hash_stored: dict[str, int] = {} + for schema in schemas: + hash_stored.update(self.list_hash_paths(schema.database)) + for path in stats["orphaned_hashes"]: + try: + size = hash_stored.get(path, 0) + if delete_path(path, self.store, config=self.config): + hash_deleted += 1 + bytes_freed += size + if verbose: + logger.info(f"Deleted: {path} ({size} bytes)") + except Exception as e: + errors += 1 + logger.warning(f"Failed to delete {path}: {e}") + + if stats["schema_paths_orphaned"] > 0: + schema_paths_stored: dict[str, int] = {} + for schema in schemas: + schema_paths_stored.update(self.list_schema_paths(schema.database)) + for path in stats["orphaned_paths"]: + try: + size = schema_paths_stored.get(path, 0) + if self.delete_schema_path(path): + schema_paths_deleted += 1 + bytes_freed += size + if verbose: + logger.info(f"Deleted schema path: {path} ({size} bytes)") + except Exception as e: + errors += 1 + logger.warning(f"Failed to delete schema path {path}: {e}") + + return { + "hash_deleted": hash_deleted, + "schema_paths_deleted": schema_paths_deleted, + "deleted": hash_deleted + schema_paths_deleted, + "bytes_freed": bytes_freed, + "errors": errors, + "dry_run": dry_run, + "hash_orphaned": stats["hash_orphaned"], + "schema_paths_orphaned": stats["schema_paths_orphaned"], + } diff --git a/src/datajoint/hash_registry.py b/src/datajoint/hash_registry.py index 34af10583..fb6f22818 100644 --- a/src/datajoint/hash_registry.py +++ b/src/datajoint/hash_registry.py @@ -25,7 +25,7 @@ Hash-addressed storage is used by ````, ````, and ```` types. Deduplication occurs within each schema. Deletion requires garbage collection -via ``dj.gc.collect()``. +via ``dj.gc.GarbageCollector``. See Also -------- diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index c824806bf..faca9cd1b 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -168,117 +168,116 @@ def test_returns_empty_for_dict_without_path(self): assert get_codec("object").referenced_paths({"other": "data"}) == [] +def _mock_collector(store="test_store"): + """A GarbageCollector wired to mocks (no real store), for logic-only tests. + The backend is resolved eagerly at construction, so patch it out.""" + cfg = MagicMock() + cfg.get_store_spec.return_value = {"hash_prefix": "_hash", "schema_prefix": "_schema"} + with patch("datajoint.gc.get_store_backend", return_value=MagicMock()): + return gc.GarbageCollector(store, config=cfg) + + +def _gc(schema, store="local"): + """One store-bound collector per live-data test (config from the schema).""" + return gc.GarbageCollector(store, config=schema.connection._config) + + +class TestGarbageCollectorConstruction: + """The collector is store-bound and resolves its store eagerly.""" + + def test_resolves_store_at_construction(self): + cfg = MagicMock() + cfg.get_store_spec.return_value = {"hash_prefix": "_hash", "schema_prefix": "_schema"} + with patch("datajoint.gc.get_store_backend", return_value="BACKEND") as mock_backend: + collector = gc.GarbageCollector("mystore", config=cfg) + mock_backend.assert_called_once_with("mystore", config=cfg) # eager + assert collector.store == "mystore" + assert collector.backend == "BACKEND" + assert collector._hash_prefix == "_hash" and collector._schema_prefix == "_schema" + + class TestScan: - """Tests for scan function.""" + """Tests for GarbageCollector.scan.""" def test_requires_at_least_one_schema(self): - """Test that at least one schema is required.""" + """At least one schema is required (checked before the store is even resolved).""" with pytest.raises(DataJointError, match="At least one schema must be provided"): - gc.scan() - - @patch("datajoint.gc.scan_schema_references") - @patch("datajoint.gc.list_schema_paths") - @patch("datajoint.gc.scan_hash_references") - @patch("datajoint.gc.list_hash_paths") - def test_returns_stats(self, mock_list_hashes, mock_scan_hash, mock_list_schemas, mock_scan_schema): - """Test that scan returns proper statistics.""" - # Mock hash-addressed storage (now uses paths) - mock_scan_hash.return_value = {"_hash/schema/path1", "_hash/schema/path2"} - mock_list_hashes.return_value = { - "_hash/schema/path1": 100, - "_hash/schema/path3": 200, # orphaned - } + _mock_collector().scan() - # Mock schema-addressed storage - mock_scan_schema.return_value = {"schema/table/pk1/field"} - mock_list_schemas.return_value = { - "schema/table/pk1/field": 500, - "schema/table/pk2/field": 300, # orphaned - } + @patch.object(gc.GarbageCollector, "schema_references") + @patch.object(gc.GarbageCollector, "list_schema_paths") + @patch.object(gc.GarbageCollector, "hash_references") + @patch.object(gc.GarbageCollector, "list_hash_paths") + def test_returns_stats(self, mock_list_hashes, mock_hash_ref, mock_list_schemas, mock_schema_ref): + """scan aggregates per-section stats (no combined totals).""" + mock_hash_ref.return_value = {"_hash/schema/path1", "_hash/schema/path2"} + mock_list_hashes.return_value = {"_hash/schema/path1": 100, "_hash/schema/path3": 200} # path3 orphaned + mock_schema_ref.return_value = {"schema/table/pk1/field"} + mock_list_schemas.return_value = {"schema/table/pk1/field": 500, "schema/table/pk2/field": 300} # pk2 orphaned - mock_schema = MagicMock() - stats = gc.scan(mock_schema, store_name="test_store") + stats = _mock_collector().scan(MagicMock()) - # Hash stats assert stats["hash_referenced"] == 2 assert stats["hash_stored"] == 2 assert stats["hash_orphaned"] == 1 assert "_hash/schema/path3" in stats["orphaned_hashes"] + assert stats["hash_orphaned_bytes"] == 200 - # Schema stats assert stats["schema_paths_referenced"] == 1 assert stats["schema_paths_stored"] == 2 assert stats["schema_paths_orphaned"] == 1 assert "schema/table/pk2/field" in stats["orphaned_paths"] + assert stats["schema_paths_orphaned_bytes"] == 300 - # Combined totals - assert stats["referenced"] == 3 - assert stats["stored"] == 4 - assert stats["orphaned"] == 2 - assert stats["orphaned_bytes"] == 500 # 200 hash + 300 schema + # combined totals were removed + for k in ("referenced", "stored", "orphaned", "orphaned_bytes"): + assert k not in stats class TestCollect: - """Tests for collect function.""" + """Tests for GarbageCollector.collect.""" - @patch("datajoint.gc.scan") + @patch.object(gc.GarbageCollector, "scan") def test_dry_run_does_not_delete(self, mock_scan): - """Test that dry_run=True doesn't delete anything.""" mock_scan.return_value = { - "referenced": 1, - "stored": 2, - "orphaned": 1, - "orphaned_bytes": 100, "orphaned_hashes": ["_hash/schema/orphan_path"], "orphaned_paths": [], "hash_orphaned": 1, "schema_paths_orphaned": 0, } - - mock_schema = MagicMock() - stats = gc.collect(mock_schema, store_name="test_store", dry_run=True) - + stats = _mock_collector().collect(MagicMock(), dry_run=True) assert stats["deleted"] == 0 assert stats["bytes_freed"] == 0 assert stats["dry_run"] is True @patch("datajoint.gc.delete_path") - @patch("datajoint.gc.list_hash_paths") - @patch("datajoint.gc.scan") - def test_deletes_orphaned_hashes(self, mock_scan, mock_list_stored, mock_delete): - """Test that orphaned hashes are deleted when dry_run=False.""" + @patch.object(gc.GarbageCollector, "list_hash_paths") + @patch.object(gc.GarbageCollector, "scan") + def test_deletes_orphaned_hashes(self, mock_scan, mock_list_hashes, mock_delete): mock_scan.return_value = { - "referenced": 1, - "stored": 2, - "orphaned": 1, - "orphaned_bytes": 100, "orphaned_hashes": ["_hash/schema/orphan_path"], "orphaned_paths": [], "hash_orphaned": 1, "schema_paths_orphaned": 0, } - mock_list_stored.return_value = {"_hash/schema/orphan_path": 100} + mock_list_hashes.return_value = {"_hash/schema/orphan_path": 100} mock_delete.return_value = True - mock_schema = MagicMock() - stats = gc.collect(mock_schema, store_name="test_store", dry_run=False) + collector = _mock_collector() + stats = collector.collect(MagicMock(), dry_run=False) assert stats["deleted"] == 1 assert stats["hash_deleted"] == 1 assert stats["bytes_freed"] == 100 assert stats["dry_run"] is False - mock_delete.assert_called_once_with("_hash/schema/orphan_path", "test_store", config=mock_schema.connection._config) + # deleted via the collector's own store/config — not threaded params + mock_delete.assert_called_once_with("_hash/schema/orphan_path", collector.store, config=collector.config) - @patch("datajoint.gc.delete_schema_path") - @patch("datajoint.gc.list_schema_paths") - @patch("datajoint.gc.scan") + @patch.object(gc.GarbageCollector, "delete_schema_path") + @patch.object(gc.GarbageCollector, "list_schema_paths") + @patch.object(gc.GarbageCollector, "scan") def test_deletes_orphaned_schemas(self, mock_scan, mock_list_schemas, mock_delete): - """Test that orphaned schema paths are deleted when dry_run=False.""" mock_scan.return_value = { - "referenced": 1, - "stored": 2, - "orphaned": 1, - "orphaned_bytes": 500, "orphaned_hashes": [], "orphaned_paths": ["schema/table/pk/field"], "hash_orphaned": 0, @@ -287,14 +286,13 @@ def test_deletes_orphaned_schemas(self, mock_scan, mock_list_schemas, mock_delet mock_list_schemas.return_value = {"schema/table/pk/field": 500} mock_delete.return_value = True - mock_schema = MagicMock() - stats = gc.collect(mock_schema, store_name="test_store", dry_run=False) + stats = _mock_collector().collect(MagicMock(), dry_run=False) assert stats["deleted"] == 1 assert stats["schema_paths_deleted"] == 1 assert stats["bytes_freed"] == 500 assert stats["dry_run"] is False - mock_delete.assert_called_once_with("schema/table/pk/field", "test_store", config=mock_schema.connection._config) + mock_delete.assert_called_once_with("schema/table/pk/field") class TestScanWithLiveData: @@ -353,7 +351,8 @@ def test_scan_finds_active_blob_reference(self, schema_blob): """ GcBlobTest.insert1({"rid": 1, "payload": np.arange(64, dtype="uint8")}) - stats = gc.scan(schema_blob, store_name="local") + collector = _gc(schema_blob) + stats = collector.scan(schema_blob) assert stats["hash_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -366,7 +365,8 @@ def test_scan_finds_active_npy_reference(self, schema_npy): """ GcNpyTest.insert1({"rid": 1, "waveform": np.arange(64, dtype="float32")}) - stats = gc.scan(schema_npy, store_name="local") + collector = _gc(schema_npy) + stats = collector.scan(schema_npy) assert stats["schema_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -379,7 +379,8 @@ def test_scan_finds_active_object_reference(self, schema_object): """ GcObjectTest.insert1({"rid": 1, "results": b"hello-gc-test"}) - stats = gc.scan(schema_object, store_name="local") + collector = _gc(schema_object) + stats = collector.scan(schema_object) assert stats["schema_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -407,11 +408,12 @@ def test_custom_codec_reference_not_orphaned(self, schema_custom): """ GcCustomCodecTest.insert1({"rid": 1, "payload": b"live-payload"}) - refs = gc.scan_schema_references(schema_custom, store_name="local") + collector = _gc(schema_custom) + refs = collector.schema_references(schema_custom) assert refs, "custom codec's live reference must be discovered (#1469)" live_path = next(iter(refs)) - stats = gc.scan(schema_custom, store_name="local") + stats = collector.scan(schema_custom) assert live_path not in stats["orphaned_paths"], f"live custom-codec file wrongly flagged orphan: {live_path}" def test_custom_codec_survives_collect(self, schema_custom): @@ -421,22 +423,21 @@ def test_custom_codec_survives_collect(self, schema_custom): GcCustomCodecTest.insert1({"rid": 1, "payload": b"row-one"}) GcCustomCodecTest.insert1({"rid": 2, "payload": b"row-two"}) - refs_all = gc.scan_schema_references(schema_custom, store_name="local") + collector = _gc(schema_custom) + refs_all = collector.schema_references(schema_custom) assert len(refs_all) == 2, f"both live rows should be referenced; got {refs_all}" # Delete one row — its file becomes a genuine orphan (delete-then-GC). (GcCustomCodecTest & {"rid": 1}).delete(prompt=False) - refs_live = gc.scan_schema_references(schema_custom, store_name="local") + refs_live = collector.schema_references(schema_custom) assert len(refs_live) == 1, f"one live row should remain referenced; got {refs_live}" live_path = next(iter(refs_live)) deleted_paths = refs_all - refs_live - gc.collect(schema_custom, store_name="local", dry_run=False) + collector.collect(schema_custom, dry_run=False) - stored_after = set( - gc.list_schema_paths("local", config=schema_custom.connection._config, schema_name=schema_custom.database) - ) + stored_after = set(collector.list_schema_paths(schema_custom.database)) assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)" assert not (deleted_paths & stored_after), f"deleted row's orphan not reclaimed: {deleted_paths & stored_after}" @@ -474,16 +475,17 @@ def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): its referenced prefix — none may appear in orphaned_paths.""" GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "live.zarr"))}) - refs = gc.scan_schema_references(schema_dirobj, store_name="local") + collector = _gc(schema_dirobj) + refs = collector.schema_references(schema_dirobj) assert len(refs) == 1 ref = next(iter(refs)) - stored = gc.list_schema_paths("local", config=schema_dirobj.connection._config, schema_name=schema_dirobj.database) + stored = collector.list_schema_paths(schema_dirobj.database) chunk_files = [p for p in stored if p.startswith(ref + "/")] assert len(chunk_files) >= 4, f"expected the folder's files under {ref}; got {sorted(stored)}" assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)" - stats = gc.scan(schema_dirobj, store_name="local") + stats = collector.scan(schema_dirobj) flagged = [p for p in stats["orphaned_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json"] assert not flagged, f"live directory object misclassified as orphaned: {flagged}" @@ -493,20 +495,19 @@ def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "gone.zarr"))}) GcObjectTest.insert1({"rid": 2, "results": str(self._make_store_dir(tmp_path, "kept.zarr"))}) - refs_all = gc.scan_schema_references(schema_dirobj, store_name="local") + collector = _gc(schema_dirobj) + refs_all = collector.schema_references(schema_dirobj) assert len(refs_all) == 2 (GcObjectTest & {"rid": 1}).delete(prompt=False) - refs_live = gc.scan_schema_references(schema_dirobj, store_name="local") + refs_live = collector.schema_references(schema_dirobj) assert len(refs_live) == 1 live_ref = next(iter(refs_live)) dead_ref = next(iter(refs_all - refs_live)) - gc.collect(schema_dirobj, store_name="local", dry_run=False) + collector.collect(schema_dirobj, dry_run=False) - stored_after = set( - gc.list_schema_paths("local", config=schema_dirobj.connection._config, schema_name=schema_dirobj.database) - ) + stored_after = set(collector.list_schema_paths(schema_dirobj.database)) live_files = {p for p in stored_after if p.startswith(live_ref + "/")} assert len(live_files) >= 4, f"live folder object lost files: {sorted(stored_after)}" assert f"{live_ref}.manifest.json" in stored_after, "live object's manifest must survive collect()" @@ -549,9 +550,10 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): GcProbeHash.insert1({"rid": 1, "results": b"keep"}) GcProbeHash.insert1({"rid": 2, "results": b"drop"}) - refs = gc.scan_schema_references(schema_probe, store_name="local") + collector = _gc(schema_probe) + refs = collector.schema_references(schema_probe) assert len(refs) == 2 - stored = set(gc.list_schema_paths("local", config=schema_probe.connection._config, schema_name=schema_probe.database)) + stored = set(collector.list_schema_paths(schema_probe.database)) missing = refs - stored assert not missing, ( f"files of a table whose name contains '_hash' must be listed; missing {missing} " @@ -559,13 +561,11 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): ) (GcProbeHash & {"rid": 2}).delete(prompt=False) - refs_live = gc.scan_schema_references(schema_probe, store_name="local") + refs_live = collector.schema_references(schema_probe) dead_ref = next(iter(refs - refs_live)) - gc.collect(schema_probe, store_name="local", dry_run=False) - stored_after = set( - gc.list_schema_paths("local", config=schema_probe.connection._config, schema_name=schema_probe.database) - ) + collector.collect(schema_probe, dry_run=False) + stored_after = set(collector.list_schema_paths(schema_probe.database)) assert dead_ref not in stored_after, "orphan under a '_hash'-substring table name must be reclaimed" assert next(iter(refs_live)) in stored_after, "live file must survive" @@ -601,11 +601,12 @@ def test_user_file_outside_section_never_listed_or_collected(self, schema_fp): # The user file lives outside {schema_prefix}/{schema}/, so the scoped # walk never lists it — no filepath_prefix declaration needed. - stored = gc.list_schema_paths("local", config=cfg, schema_name=schema_fp.database) + collector = _gc(schema_fp) + stored = collector.list_schema_paths(schema_fp.database) assert "userfiles/deep/important.bin" not in stored assert stored, "the schema's own managed object must still be listed" - gc.collect(schema_fp, store_name="local", dry_run=False) + collector.collect(schema_fp, dry_run=False) assert user_file.exists(), "collect() must never touch files outside the schema section" @@ -633,28 +634,28 @@ def schema_prefixed(self, connection_test, prefix, mock_stores): cfg.stores["local"].pop("schema_prefix", None) def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): - cfg = schema_prefixed.connection._config GcBlobTest.insert1({"rid": 1, "payload": np.arange(16, dtype="uint8")}) GcObjectTest.insert1({"rid": 1, "results": b"payload-one"}) GcObjectTest.insert1({"rid": 2, "results": b"payload-two"}) # Writers honored the configured sections (metadata records the real paths) - hash_refs = gc.scan_hash_references(schema_prefixed, store_name="local") + collector = _gc(schema_prefixed) + hash_refs = collector.hash_references(schema_prefixed) assert hash_refs and all(r.startswith("content/") for r in hash_refs), hash_refs - obj_refs = gc.scan_schema_references(schema_prefixed, store_name="local") + obj_refs = collector.schema_references(schema_prefixed) assert len(obj_refs) == 2 and all(r.startswith("objects/") for r in obj_refs), obj_refs # GC scans the same sections: everything referenced, nothing falsely orphaned - stats = gc.scan(schema_prefixed, store_name="local") + stats = collector.scan(schema_prefixed) assert stats["hash_referenced"] >= 1 assert not (hash_refs & set(stats["orphaned_hashes"])) assert not (obj_refs & set(stats["orphaned_paths"])) # And reclamation works within the configured sections (GcObjectTest & {"rid": 2}).delete(prompt=False) - gc.collect(schema_prefixed, store_name="local", dry_run=False) - stored_after = set(gc.list_schema_paths("local", config=cfg, schema_name=schema_prefixed.database)) - live = gc.scan_schema_references(schema_prefixed, store_name="local") + collector.collect(schema_prefixed, dry_run=False) + stored_after = set(collector.list_schema_paths(schema_prefixed.database)) + live = collector.schema_references(schema_prefixed) assert live <= stored_after, "live object under custom section must survive" assert not ((obj_refs - live) & stored_after), "orphan under custom section must be reclaimed" @@ -683,24 +684,29 @@ def test_live_hash_object_survives_prefix_change(self, schema_pfx): # Written under the default _hash/ section. GcBlobTest.insert1({"rid": 1, "payload": np.arange(8, dtype="uint8")}) GcObjectTest.insert1({"rid": 1, "results": b"schema-obj"}) - hash_ref = next(iter(gc.scan_hash_references(schema_pfx, store_name="local"))) - assert hash_ref.startswith("_hash/") # Relocate the hash section: new writes would go to content/, but the # existing object stays at _hash/ (metadata keeps its full path). cfg.stores["local"]["hash_prefix"] = "content" try: + # Construct AFTER the flip so the collector pins the current + # (content) prefix. The reference is the metadata path, recorded + # when the object was written under _hash — unaffected by the flip. + collector = _gc(schema_pfx) + hash_ref = next(iter(collector.hash_references(schema_pfx))) + assert hash_ref.startswith("_hash/") + # Read still works via the metadata path. assert (GcBlobTest & {"rid": 1}).fetch1("payload") is not None - stats = gc.scan(schema_pfx, store_name="local") + stats = collector.scan(schema_pfx) assert hash_ref not in set( stats["orphaned_paths"] ), "live hash object outside the current hash section must not be a schema orphan" assert hash_ref not in set(stats["orphaned_hashes"]) # End-to-end: collect must not destroy the live hash object. - gc.collect(schema_pfx, store_name="local", dry_run=False) + collector.collect(schema_pfx, dry_run=False) from datajoint.hash_registry import get_store_backend backend = get_store_backend("local", config=cfg) @@ -747,14 +753,14 @@ def two_schemas(self, connection_test, prefix, mock_stores): def test_list_scoped_to_schema(self, two_schemas): a, b, b_blob, b_obj = two_schemas - cfg = a.connection._config + collector = _gc(a) GcBlobTest.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) GcObjectTest.insert1({"rid": 1, "results": b"a-obj"}) b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-obj"}) - a_hashes = set(gc.list_hash_paths("local", config=cfg, schema_name=a.database)) - a_paths = set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) + a_hashes = set(collector.list_hash_paths(a.database)) + a_paths = set(collector.list_schema_paths(a.database)) assert a_hashes and all(f"/{a.database}/" in h for h in a_hashes), a_hashes assert a_paths and all(p.split("/")[0] == a.database or f"/{a.database}/" in p for p in a_paths), a_paths # nothing from schema b leaked into schema a's scoped listings @@ -762,34 +768,35 @@ def test_list_scoped_to_schema(self, two_schemas): def test_scan_one_schema_ignores_the_other(self, two_schemas): a, b, b_blob, b_obj = two_schemas + collector = _gc(a) GcBlobTest.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-obj"}) - stats = gc.scan(a, store_name="local") + stats = collector.scan(a) # b's live objects are outside a's subtree → not counted, not orphaned - assert stats["orphaned"] == 0 + assert stats["hash_orphaned"] == 0 and stats["schema_paths_orphaned"] == 0 assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hashes"]) def test_collect_one_schema_never_touches_the_other(self, two_schemas): a, b, b_blob, b_obj = two_schemas - cfg = a.connection._config + collector = _gc(a) # a has an orphan (insert then delete); b has only live data GcObjectTest.insert1({"rid": 1, "results": b"a-doomed"}) b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-live"}) - a_orphan = next(iter(gc.scan_schema_references(a, store_name="local"))) + a_orphan = next(iter(collector.schema_references(a))) (GcObjectTest & {"rid": 1}).delete(prompt=False) - b_hashes_before = set(gc.list_hash_paths("local", config=cfg, schema_name=b.database)) - b_paths_before = set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) + b_hashes_before = set(collector.list_hash_paths(b.database)) + b_paths_before = set(collector.list_schema_paths(b.database)) assert b_hashes_before and b_paths_before - gc.collect(a, store_name="local", dry_run=False) + collector.collect(a, dry_run=False) # a's orphan reclaimed; b entirely intact - assert a_orphan not in set(gc.list_schema_paths("local", config=cfg, schema_name=a.database)) - assert set(gc.list_hash_paths("local", config=cfg, schema_name=b.database)) == b_hashes_before - assert set(gc.list_schema_paths("local", config=cfg, schema_name=b.database)) == b_paths_before + assert a_orphan not in set(collector.list_schema_paths(a.database)) + assert set(collector.list_hash_paths(b.database)) == b_hashes_before + assert set(collector.list_schema_paths(b.database)) == b_paths_before assert len(b_obj()) == 1 # b's row (and its object) untouched From e6cd1a923b5cea5606b5e168bfe7f71ba3008c71 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 17:07:24 -0500 Subject: [PATCH 17/20] docs(test): refer to GarbageCollector.scan in test docstring --- tests/integration/test_gc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index faca9cd1b..fbbb0a62e 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -296,7 +296,7 @@ def test_deletes_orphaned_schemas(self, mock_scan, mock_list_schemas, mock_delet class TestScanWithLiveData: - """End-to-end tests for gc.scan() against real schemas with external storage. + """End-to-end tests for GarbageCollector.scan() against real schemas with external storage. Exercises the full production path: scan_*_references → table.proj(attr).cursor() → raw JSON metadata. From 6f20aefc0fce0ac21df99db0bc6cd598939b9214 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 17:20:46 -0500 Subject: [PATCH 18/20] refactor(gc): schemas in the constructor; merge scan into collect 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. --- src/datajoint/gc.py | 231 +++++++++++++++-------------------- tests/integration/test_gc.py | 198 ++++++++++++------------------ 2 files changed, 180 insertions(+), 249 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index e4591bde5..a7d19e963 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -21,20 +21,20 @@ Deletion: Requires garbage collection Garbage collection is **store-specific** and handles one store at a time. The -:class:`GarbageCollector` is bound to a store (and the config that defines it) -at construction; its store and config are not threaded through each call:: +:class:`GarbageCollector` is bound to its schemas and store at construction; +neither is threaded through each call (the config defaults to the schemas'):: import datajoint as dj - collector = dj.gc.GarbageCollector(store="mystore") - stats = collector.scan(schema1, schema2) # read-only - stats = collector.collect(schema1, schema2, dry_run=False) + collector = dj.gc.GarbageCollector(schema1, schema2, store="mystore") + stats = collector.collect() # read-only report (dry_run=True default) + stats = collector.collect(dry_run=False) # actually delete the orphans Both sections embed the schema name in every path, so garbage collection is **per-schema**: each schema is scanned against its own subtree only. Orphan -detection is therefore confined to the schemas you pass — any subset of the -schemas sharing a store may be scanned/collected safely, and GC never touches -another schema's objects or user-managed content elsewhere in the store. +detection is therefore confined to the collector's schemas — any subset of the +schemas sharing a store may be collected safely, and GC never touches another +schema's objects or user-managed content elsewhere in the store. See Also -------- @@ -87,65 +87,71 @@ def _is_covered(path: str, referenced: set[str]) -> bool: class GarbageCollector: """ - Store-specific garbage collector — handles exactly one store. + Store-specific garbage collector — one store, a fixed set of schemas. - Bound to a store name and the config that defines it at construction, so - neither is threaded through individual operations. Storage-side work (the - stored-file listings, deletion) uses this store/config; database metadata - is read through each scanned schema's own connection. + The schemas to collect and the store are bound at construction, so neither + is threaded through individual operations. The store is resolved eagerly + (an unknown/misconfigured store raises immediately). Storage-side work (the + stored-file listings, deletion) uses this store; database metadata is read + through each schema's own connection. Parameters ---------- + *schemas : Schema + The schemas to collect. At least one is required. Every managed path + embeds its schema, so collection is per-schema and confined to these + schemas: objects of any other schema sharing the store are never listed + and never at risk — pass any subset safely. store : str, optional - Store name (None = the default store). Resolved at construction — an - unknown/misconfigured store raises immediately. + Store name (None = the default store), keyword-only. config : Config, optional - Config that defines the store. If None, the global ``settings.config`` - is used. + Config that defines the store. Defaults to the first schema's + connection config (``schemas[0].connection._config``). """ - def __init__(self, store: str | None = None, *, config=None) -> None: - if config is None: - from .settings import config as _global_config - - config = _global_config + def __init__(self, *schemas: "Schema", store: str | None = None, config=None) -> None: + if not schemas: + raise DataJointError("At least one schema must be provided") + self.schemas = schemas self.store = store - self.config = config + self.config = config if config is not None else schemas[0].connection._config # Resolve the store eagerly: validates it exists and pins the backend # and section prefixes for this collector's lifetime (one store only). - self.backend = get_store_backend(store, config=config) - spec = config.get_store_spec(store) + self.backend = get_store_backend(store, config=self.config) + spec = self.config.get_store_spec(store) self._hash_prefix = spec["hash_prefix"].strip("/") # settings applies the "_hash" default self._schema_prefix = spec["schema_prefix"].strip("/") # ... "_schema" default # ------------------------------------------------------------------ # # References — extracted from database metadata (per-schema connection) # ------------------------------------------------------------------ # - def hash_references(self, *schemas: "Schema", verbose: bool = False) -> set[str]: + def hash_references(self, verbose: bool = False) -> set[str]: """ - Full relative store paths of hash-addressed objects referenced by live rows. + Full relative store paths of hash-addressed objects referenced by live + rows across this collector's schemas. Reads columns classified by ``Heading.hash_objects`` (````, and external ````/````). Paths are taken verbatim from each row's metadata (independent of the store's current prefixes) and filtered to this collector's store. """ - return self._references("hash_objects", schemas, verbose) + return self._references("hash_objects", verbose) - def schema_references(self, *schemas: "Schema", verbose: bool = False) -> set[str]: + def schema_references(self, verbose: bool = False) -> set[str]: """ - Full relative store paths of schema-addressed objects referenced by live rows. + Full relative store paths of schema-addressed objects referenced by live + rows across this collector's schemas. Reads columns classified by ``Heading.schema_objects`` — any ``SchemaCodec`` (````, ````, custom subclasses, recognized by type per #1469). For a directory-valued object the recorded path is the directory prefix, not its individual files. """ - return self._references("schema_objects", schemas, verbose) + return self._references("schema_objects", verbose) - def _references(self, heading_attr: str, schemas, verbose: bool) -> set[str]: + def _references(self, heading_attr: str, verbose: bool) -> set[str]: referenced: set[str] = set() - for schema in schemas: + for schema in self.schemas: if verbose: logger.info(f"Scanning schema {schema.database} for {heading_attr}") for table_name in schema.list_tables(): @@ -294,18 +300,23 @@ def delete_schema_path(self, path: str) -> bool: # ------------------------------------------------------------------ # # Orchestration # ------------------------------------------------------------------ # - def scan(self, *schemas: "Schema", verbose: bool = False) -> dict[str, Any]: + def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any]: """ - Scan for orphaned storage items without deleting. + Report — and, unless ``dry_run``, remove — orphaned storage. - Scans both hash-addressed and schema-addressed storage. Each schema is - scanned against ITS OWN subtree, so orphan detection is confined to the - schemas passed — objects of other schemas sharing the store are never - listed and never at risk; any subset may be passed safely. Objects - under a *former* prefix stay readable via their metadata paths but are - not reclamation candidates until the setting is restored. + ``dry_run=True`` (the default) is a **read-only scan**: it reports what + would be removed and deletes nothing. ``dry_run=False`` additionally + deletes the orphans. The report is the same either way, so the safe + default doubles as the inspection call. - Returns a dict with per-section statistics: + Every managed path embeds its schema, so a schema's stored files can + only match that schema's references — orphan detection is confined to + this collector's schemas; objects of any other schema on the store are + never listed, deleted, or at risk. Objects under a *former* prefix stay + readable via their metadata paths but are not reclamation candidates + until the setting is restored. + + Returns a dict with per-section stats and the deletion outcome: - hash_referenced / hash_stored / hash_orphaned / hash_orphaned_bytes - orphaned_hashes: orphaned hash items as full relative store paths @@ -313,36 +324,55 @@ def scan(self, *schemas: "Schema", verbose: bool = False) -> dict[str, Any]: - schema_paths_referenced / schema_paths_stored / schema_paths_orphaned / schema_paths_orphaned_bytes - orphaned_paths: orphaned schema files as full relative store paths + - hash_deleted / schema_paths_deleted / deleted / bytes_freed (0 when + dry_run) / errors / dry_run """ - if not schemas: - raise DataJointError("At least one schema must be provided") + # References can be gathered across all schemas at once: because paths + # embed the schema, a file under schema X is only ever covered by an X + # reference, so combined coverage equals per-schema coverage. + hash_referenced = self.hash_references(verbose=verbose) + schema_paths_referenced = self.schema_references(verbose=verbose) - hash_referenced: set[str] = set() hash_stored: dict[str, int] = {} - orphaned_hashes: set[str] = set() - schema_paths_referenced: set[str] = set() schema_paths_stored: dict[str, int] = {} - orphaned_paths: set[str] = set() - - for schema in schemas: - # --- Hash-addressed storage (this schema's subtree) --- - h_ref = self.hash_references(schema, verbose=verbose) - h_stored = self.list_hash_paths(schema.database) - orphaned_hashes |= set(h_stored.keys()) - h_ref - - # --- Schema-addressed storage (this schema's section) --- - s_ref = self.schema_references(schema, verbose=verbose) - s_stored = self.list_schema_paths(schema.database) - # Coverage, not exact set difference: a referenced path may be a - # directory-valued object (many files + a manifest). The walk is - # confined to this schema's section, so coverage against s_ref alone - # is complete (no hash objects can appear here). - orphaned_paths |= {p for p in s_stored if not _is_covered(p, s_ref)} - - hash_referenced |= h_ref - hash_stored.update(h_stored) - schema_paths_referenced |= s_ref - schema_paths_stored.update(s_stored) + for schema in self.schemas: + hash_stored.update(self.list_hash_paths(schema.database)) + schema_paths_stored.update(self.list_schema_paths(schema.database)) + + orphaned_hashes = sorted(set(hash_stored.keys()) - hash_referenced) + # Coverage, not exact set difference: a referenced path may be a + # directory-valued object (many files + a manifest sidecar). + orphaned_paths = sorted(p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)) + + hash_deleted = 0 + schema_paths_deleted = 0 + bytes_freed = 0 + errors = 0 + + if not dry_run: + # The size maps from the scan above are reused for the byte tally — + # no second listing pass. + for path in orphaned_hashes: + try: + if delete_path(path, self.store, config=self.config): + hash_deleted += 1 + bytes_freed += hash_stored.get(path, 0) + if verbose: + logger.info(f"Deleted: {path} ({hash_stored.get(path, 0)} bytes)") + except Exception as e: + errors += 1 + logger.warning(f"Failed to delete {path}: {e}") + + for path in orphaned_paths: + try: + if self.delete_schema_path(path): + schema_paths_deleted += 1 + bytes_freed += schema_paths_stored.get(path, 0) + if verbose: + logger.info(f"Deleted schema path: {path} ({schema_paths_stored.get(path, 0)} bytes)") + except Exception as e: + errors += 1 + logger.warning(f"Failed to delete schema path {path}: {e}") return { # Hash-addressed storage stats @@ -350,77 +380,18 @@ def scan(self, *schemas: "Schema", verbose: bool = False) -> dict[str, Any]: "hash_stored": len(hash_stored), "hash_orphaned": len(orphaned_hashes), "hash_orphaned_bytes": sum(hash_stored.get(h, 0) for h in orphaned_hashes), - "orphaned_hashes": sorted(orphaned_hashes), + "orphaned_hashes": orphaned_hashes, # Schema-addressed storage stats "schema_paths_referenced": len(schema_paths_referenced), "schema_paths_stored": len(schema_paths_stored), "schema_paths_orphaned": len(orphaned_paths), "schema_paths_orphaned_bytes": sum(schema_paths_stored.get(p, 0) for p in orphaned_paths), - "orphaned_paths": sorted(orphaned_paths), - } - - def collect(self, *schemas: "Schema", dry_run: bool = True, verbose: bool = False) -> dict[str, Any]: - """ - Remove orphaned storage items. - - Scans the given schemas (via :meth:`scan`), then removes the orphans it - identifies. Orphan identity comes entirely from :meth:`scan`, so live - objects outside the current section (after a prefix change), other - schemas' objects, and the ``filepath_prefix`` namespace are never - deleted. - - Returns a dict with: hash_deleted, schema_paths_deleted, deleted, - bytes_freed (0 if dry_run), errors, dry_run, and the per-section orphan - counts (hash_orphaned, schema_paths_orphaned). - """ - stats = self.scan(*schemas, verbose=verbose) - - hash_deleted = 0 - schema_paths_deleted = 0 - bytes_freed = 0 - errors = 0 - - if not dry_run: - if stats["hash_orphaned"] > 0: - # Size map for bytes_freed, merged across the passed schemas. - hash_stored: dict[str, int] = {} - for schema in schemas: - hash_stored.update(self.list_hash_paths(schema.database)) - for path in stats["orphaned_hashes"]: - try: - size = hash_stored.get(path, 0) - if delete_path(path, self.store, config=self.config): - hash_deleted += 1 - bytes_freed += size - if verbose: - logger.info(f"Deleted: {path} ({size} bytes)") - except Exception as e: - errors += 1 - logger.warning(f"Failed to delete {path}: {e}") - - if stats["schema_paths_orphaned"] > 0: - schema_paths_stored: dict[str, int] = {} - for schema in schemas: - schema_paths_stored.update(self.list_schema_paths(schema.database)) - for path in stats["orphaned_paths"]: - try: - size = schema_paths_stored.get(path, 0) - if self.delete_schema_path(path): - schema_paths_deleted += 1 - bytes_freed += size - if verbose: - logger.info(f"Deleted schema path: {path} ({size} bytes)") - except Exception as e: - errors += 1 - logger.warning(f"Failed to delete schema path {path}: {e}") - - return { + "orphaned_paths": orphaned_paths, + # Deletion outcome (all zero when dry_run) "hash_deleted": hash_deleted, "schema_paths_deleted": schema_paths_deleted, "deleted": hash_deleted + schema_paths_deleted, "bytes_freed": bytes_freed, "errors": errors, "dry_run": dry_run, - "hash_orphaned": stats["hash_orphaned"], - "schema_paths_orphaned": stats["schema_paths_orphaned"], } diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index fbbb0a62e..25f4a0dea 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -2,6 +2,7 @@ Tests for garbage collection (gc.py). """ +from contextlib import ExitStack from unittest.mock import MagicMock, patch import numpy as np @@ -174,12 +175,12 @@ def _mock_collector(store="test_store"): cfg = MagicMock() cfg.get_store_spec.return_value = {"hash_prefix": "_hash", "schema_prefix": "_schema"} with patch("datajoint.gc.get_store_backend", return_value=MagicMock()): - return gc.GarbageCollector(store, config=cfg) + return gc.GarbageCollector(MagicMock(), store=store, config=cfg) def _gc(schema, store="local"): """One store-bound collector per live-data test (config from the schema).""" - return gc.GarbageCollector(store, config=schema.connection._config) + return gc.GarbageCollector(schema, store=store) class TestGarbageCollectorConstruction: @@ -189,110 +190,69 @@ def test_resolves_store_at_construction(self): cfg = MagicMock() cfg.get_store_spec.return_value = {"hash_prefix": "_hash", "schema_prefix": "_schema"} with patch("datajoint.gc.get_store_backend", return_value="BACKEND") as mock_backend: - collector = gc.GarbageCollector("mystore", config=cfg) + collector = gc.GarbageCollector(MagicMock(), store="mystore", config=cfg) mock_backend.assert_called_once_with("mystore", config=cfg) # eager assert collector.store == "mystore" assert collector.backend == "BACKEND" assert collector._hash_prefix == "_hash" and collector._schema_prefix == "_schema" -class TestScan: - """Tests for GarbageCollector.scan.""" +class TestCollect: + """Tests for GarbageCollector.collect (dry_run=True is the read-only scan).""" + + # Fixtures patched onto the four data-gathering methods so the orphan set is + # fixed: hashes path1,path2 referenced but path1,path3 stored → path3 + # orphaned; schema pk1 referenced but pk1,pk2 stored → pk2 orphaned. + def _patch_data(self): + return [ + patch.object(gc.GarbageCollector, "hash_references", return_value={"_hash/s/path1", "_hash/s/path2"}), + patch.object(gc.GarbageCollector, "schema_references", return_value={"s/t/pk1/f"}), + patch.object(gc.GarbageCollector, "list_hash_paths", return_value={"_hash/s/path1": 100, "_hash/s/path3": 200}), + patch.object(gc.GarbageCollector, "list_schema_paths", return_value={"s/t/pk1/f": 500, "s/t/pk2/f": 300}), + ] def test_requires_at_least_one_schema(self): - """At least one schema is required (checked before the store is even resolved).""" + """At least one schema is required, checked before the store is resolved.""" with pytest.raises(DataJointError, match="At least one schema must be provided"): - _mock_collector().scan() - - @patch.object(gc.GarbageCollector, "schema_references") - @patch.object(gc.GarbageCollector, "list_schema_paths") - @patch.object(gc.GarbageCollector, "hash_references") - @patch.object(gc.GarbageCollector, "list_hash_paths") - def test_returns_stats(self, mock_list_hashes, mock_hash_ref, mock_list_schemas, mock_schema_ref): - """scan aggregates per-section stats (no combined totals).""" - mock_hash_ref.return_value = {"_hash/schema/path1", "_hash/schema/path2"} - mock_list_hashes.return_value = {"_hash/schema/path1": 100, "_hash/schema/path3": 200} # path3 orphaned - mock_schema_ref.return_value = {"schema/table/pk1/field"} - mock_list_schemas.return_value = {"schema/table/pk1/field": 500, "schema/table/pk2/field": 300} # pk2 orphaned - - stats = _mock_collector().scan(MagicMock()) - - assert stats["hash_referenced"] == 2 - assert stats["hash_stored"] == 2 - assert stats["hash_orphaned"] == 1 - assert "_hash/schema/path3" in stats["orphaned_hashes"] - assert stats["hash_orphaned_bytes"] == 200 + gc.GarbageCollector(store="test_store") - assert stats["schema_paths_referenced"] == 1 - assert stats["schema_paths_stored"] == 2 - assert stats["schema_paths_orphaned"] == 1 - assert "schema/table/pk2/field" in stats["orphaned_paths"] - assert stats["schema_paths_orphaned_bytes"] == 300 + def test_dry_run_reports_stats_and_deletes_nothing(self): + """dry_run=True (default) returns the full orphan report without deleting.""" + with ExitStack() as es: + for p in self._patch_data(): + es.enter_context(p) + mock_delete = es.enter_context(patch("datajoint.gc.delete_path")) + stats = _mock_collector().collect() # dry_run defaults True - # combined totals were removed + assert stats["dry_run"] is True + assert stats["deleted"] == 0 and stats["bytes_freed"] == 0 + mock_delete.assert_not_called() + # full report is present + assert stats["hash_orphaned"] == 1 and stats["orphaned_hashes"] == ["_hash/s/path3"] + assert stats["hash_orphaned_bytes"] == 200 + assert stats["schema_paths_orphaned"] == 1 and stats["orphaned_paths"] == ["s/t/pk2/f"] + assert stats["schema_paths_orphaned_bytes"] == 300 + # no combined totals for k in ("referenced", "stored", "orphaned", "orphaned_bytes"): assert k not in stats - -class TestCollect: - """Tests for GarbageCollector.collect.""" - - @patch.object(gc.GarbageCollector, "scan") - def test_dry_run_does_not_delete(self, mock_scan): - mock_scan.return_value = { - "orphaned_hashes": ["_hash/schema/orphan_path"], - "orphaned_paths": [], - "hash_orphaned": 1, - "schema_paths_orphaned": 0, - } - stats = _mock_collector().collect(MagicMock(), dry_run=True) - assert stats["deleted"] == 0 - assert stats["bytes_freed"] == 0 - assert stats["dry_run"] is True - - @patch("datajoint.gc.delete_path") - @patch.object(gc.GarbageCollector, "list_hash_paths") - @patch.object(gc.GarbageCollector, "scan") - def test_deletes_orphaned_hashes(self, mock_scan, mock_list_hashes, mock_delete): - mock_scan.return_value = { - "orphaned_hashes": ["_hash/schema/orphan_path"], - "orphaned_paths": [], - "hash_orphaned": 1, - "schema_paths_orphaned": 0, - } - mock_list_hashes.return_value = {"_hash/schema/orphan_path": 100} - mock_delete.return_value = True - - collector = _mock_collector() - stats = collector.collect(MagicMock(), dry_run=False) - - assert stats["deleted"] == 1 - assert stats["hash_deleted"] == 1 - assert stats["bytes_freed"] == 100 - assert stats["dry_run"] is False - # deleted via the collector's own store/config — not threaded params - mock_delete.assert_called_once_with("_hash/schema/orphan_path", collector.store, config=collector.config) - - @patch.object(gc.GarbageCollector, "delete_schema_path") - @patch.object(gc.GarbageCollector, "list_schema_paths") - @patch.object(gc.GarbageCollector, "scan") - def test_deletes_orphaned_schemas(self, mock_scan, mock_list_schemas, mock_delete): - mock_scan.return_value = { - "orphaned_hashes": [], - "orphaned_paths": ["schema/table/pk/field"], - "hash_orphaned": 0, - "schema_paths_orphaned": 1, - } - mock_list_schemas.return_value = {"schema/table/pk/field": 500} - mock_delete.return_value = True - - stats = _mock_collector().collect(MagicMock(), dry_run=False) - - assert stats["deleted"] == 1 - assert stats["schema_paths_deleted"] == 1 - assert stats["bytes_freed"] == 500 + def test_delete_removes_orphans_via_bound_store(self): + """dry_run=False deletes each section's orphans, sized from the same scan.""" + with ExitStack() as es: + for p in self._patch_data(): + es.enter_context(p) + mock_delete_hash = es.enter_context(patch("datajoint.gc.delete_path", return_value=True)) + mock_delete_schema = es.enter_context(patch.object(gc.GarbageCollector, "delete_schema_path", return_value=True)) + collector = _mock_collector() + stats = collector.collect(dry_run=False) + + assert stats["hash_deleted"] == 1 and stats["schema_paths_deleted"] == 1 + assert stats["deleted"] == 2 + assert stats["bytes_freed"] == 500 # 200 (hash path3) + 300 (schema pk2) assert stats["dry_run"] is False - mock_delete.assert_called_once_with("schema/table/pk/field") + # hash deleted via the collector's own store/config (not threaded params) + mock_delete_hash.assert_called_once_with("_hash/s/path3", collector.store, config=collector.config) + mock_delete_schema.assert_called_once_with("s/t/pk2/f") class TestScanWithLiveData: @@ -352,7 +312,7 @@ def test_scan_finds_active_blob_reference(self, schema_blob): GcBlobTest.insert1({"rid": 1, "payload": np.arange(64, dtype="uint8")}) collector = _gc(schema_blob) - stats = collector.scan(schema_blob) + stats = collector.collect() assert stats["hash_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -366,7 +326,7 @@ def test_scan_finds_active_npy_reference(self, schema_npy): GcNpyTest.insert1({"rid": 1, "waveform": np.arange(64, dtype="float32")}) collector = _gc(schema_npy) - stats = collector.scan(schema_npy) + stats = collector.collect() assert stats["schema_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -380,7 +340,7 @@ def test_scan_finds_active_object_reference(self, schema_object): GcObjectTest.insert1({"rid": 1, "results": b"hello-gc-test"}) collector = _gc(schema_object) - stats = collector.scan(schema_object) + stats = collector.collect() assert stats["schema_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" @@ -409,11 +369,11 @@ def test_custom_codec_reference_not_orphaned(self, schema_custom): GcCustomCodecTest.insert1({"rid": 1, "payload": b"live-payload"}) collector = _gc(schema_custom) - refs = collector.schema_references(schema_custom) + refs = collector.schema_references() assert refs, "custom codec's live reference must be discovered (#1469)" live_path = next(iter(refs)) - stats = collector.scan(schema_custom) + stats = collector.collect() assert live_path not in stats["orphaned_paths"], f"live custom-codec file wrongly flagged orphan: {live_path}" def test_custom_codec_survives_collect(self, schema_custom): @@ -424,18 +384,18 @@ def test_custom_codec_survives_collect(self, schema_custom): GcCustomCodecTest.insert1({"rid": 2, "payload": b"row-two"}) collector = _gc(schema_custom) - refs_all = collector.schema_references(schema_custom) + refs_all = collector.schema_references() assert len(refs_all) == 2, f"both live rows should be referenced; got {refs_all}" # Delete one row — its file becomes a genuine orphan (delete-then-GC). (GcCustomCodecTest & {"rid": 1}).delete(prompt=False) - refs_live = collector.schema_references(schema_custom) + refs_live = collector.schema_references() assert len(refs_live) == 1, f"one live row should remain referenced; got {refs_live}" live_path = next(iter(refs_live)) deleted_paths = refs_all - refs_live - collector.collect(schema_custom, dry_run=False) + collector.collect(dry_run=False) stored_after = set(collector.list_schema_paths(schema_custom.database)) assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)" @@ -476,7 +436,7 @@ def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "live.zarr"))}) collector = _gc(schema_dirobj) - refs = collector.schema_references(schema_dirobj) + refs = collector.schema_references() assert len(refs) == 1 ref = next(iter(refs)) @@ -485,7 +445,7 @@ def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): assert len(chunk_files) >= 4, f"expected the folder's files under {ref}; got {sorted(stored)}" assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)" - stats = collector.scan(schema_dirobj) + stats = collector.collect() flagged = [p for p in stats["orphaned_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json"] assert not flagged, f"live directory object misclassified as orphaned: {flagged}" @@ -496,16 +456,16 @@ def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path GcObjectTest.insert1({"rid": 2, "results": str(self._make_store_dir(tmp_path, "kept.zarr"))}) collector = _gc(schema_dirobj) - refs_all = collector.schema_references(schema_dirobj) + refs_all = collector.schema_references() assert len(refs_all) == 2 (GcObjectTest & {"rid": 1}).delete(prompt=False) - refs_live = collector.schema_references(schema_dirobj) + refs_live = collector.schema_references() assert len(refs_live) == 1 live_ref = next(iter(refs_live)) dead_ref = next(iter(refs_all - refs_live)) - collector.collect(schema_dirobj, dry_run=False) + collector.collect(dry_run=False) stored_after = set(collector.list_schema_paths(schema_dirobj.database)) live_files = {p for p in stored_after if p.startswith(live_ref + "/")} @@ -551,7 +511,7 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): GcProbeHash.insert1({"rid": 2, "results": b"drop"}) collector = _gc(schema_probe) - refs = collector.schema_references(schema_probe) + refs = collector.schema_references() assert len(refs) == 2 stored = set(collector.list_schema_paths(schema_probe.database)) missing = refs - stored @@ -561,10 +521,10 @@ def test_hash_substring_table_listed_and_reclaimed(self, schema_probe): ) (GcProbeHash & {"rid": 2}).delete(prompt=False) - refs_live = collector.schema_references(schema_probe) + refs_live = collector.schema_references() dead_ref = next(iter(refs - refs_live)) - collector.collect(schema_probe, dry_run=False) + collector.collect(dry_run=False) stored_after = set(collector.list_schema_paths(schema_probe.database)) assert dead_ref not in stored_after, "orphan under a '_hash'-substring table name must be reclaimed" assert next(iter(refs_live)) in stored_after, "live file must survive" @@ -606,7 +566,7 @@ def test_user_file_outside_section_never_listed_or_collected(self, schema_fp): assert "userfiles/deep/important.bin" not in stored assert stored, "the schema's own managed object must still be listed" - collector.collect(schema_fp, dry_run=False) + collector.collect(dry_run=False) assert user_file.exists(), "collect() must never touch files outside the schema section" @@ -640,22 +600,22 @@ def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): # Writers honored the configured sections (metadata records the real paths) collector = _gc(schema_prefixed) - hash_refs = collector.hash_references(schema_prefixed) + hash_refs = collector.hash_references() assert hash_refs and all(r.startswith("content/") for r in hash_refs), hash_refs - obj_refs = collector.schema_references(schema_prefixed) + obj_refs = collector.schema_references() assert len(obj_refs) == 2 and all(r.startswith("objects/") for r in obj_refs), obj_refs # GC scans the same sections: everything referenced, nothing falsely orphaned - stats = collector.scan(schema_prefixed) + stats = collector.collect() assert stats["hash_referenced"] >= 1 assert not (hash_refs & set(stats["orphaned_hashes"])) assert not (obj_refs & set(stats["orphaned_paths"])) # And reclamation works within the configured sections (GcObjectTest & {"rid": 2}).delete(prompt=False) - collector.collect(schema_prefixed, dry_run=False) + collector.collect(dry_run=False) stored_after = set(collector.list_schema_paths(schema_prefixed.database)) - live = collector.schema_references(schema_prefixed) + live = collector.schema_references() assert live <= stored_after, "live object under custom section must survive" assert not ((obj_refs - live) & stored_after), "orphan under custom section must be reclaimed" @@ -693,20 +653,20 @@ def test_live_hash_object_survives_prefix_change(self, schema_pfx): # (content) prefix. The reference is the metadata path, recorded # when the object was written under _hash — unaffected by the flip. collector = _gc(schema_pfx) - hash_ref = next(iter(collector.hash_references(schema_pfx))) + hash_ref = next(iter(collector.hash_references())) assert hash_ref.startswith("_hash/") # Read still works via the metadata path. assert (GcBlobTest & {"rid": 1}).fetch1("payload") is not None - stats = collector.scan(schema_pfx) + stats = collector.collect() assert hash_ref not in set( stats["orphaned_paths"] ), "live hash object outside the current hash section must not be a schema orphan" assert hash_ref not in set(stats["orphaned_hashes"]) # End-to-end: collect must not destroy the live hash object. - collector.collect(schema_pfx, dry_run=False) + collector.collect(dry_run=False) from datajoint.hash_registry import get_store_backend backend = get_store_backend("local", config=cfg) @@ -773,7 +733,7 @@ def test_scan_one_schema_ignores_the_other(self, two_schemas): b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-obj"}) - stats = collector.scan(a) + stats = collector.collect() # b's live objects are outside a's subtree → not counted, not orphaned assert stats["hash_orphaned"] == 0 and stats["schema_paths_orphaned"] == 0 assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hashes"]) @@ -786,14 +746,14 @@ def test_collect_one_schema_never_touches_the_other(self, two_schemas): b_blob.insert1({"rid": 1, "payload": np.arange(4, dtype="uint8")}) b_obj.insert1({"rid": 1, "results": b"b-live"}) - a_orphan = next(iter(collector.schema_references(a))) + a_orphan = next(iter(collector.schema_references())) (GcObjectTest & {"rid": 1}).delete(prompt=False) b_hashes_before = set(collector.list_hash_paths(b.database)) b_paths_before = set(collector.list_schema_paths(b.database)) assert b_hashes_before and b_paths_before - collector.collect(a, dry_run=False) + collector.collect(dry_run=False) # a's orphan reclaimed; b entirely intact assert a_orphan not in set(collector.list_schema_paths(a.database)) From f6bcccb8af1e3331b7b456dc7d20bb6273953f52 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 17:32:24 -0500 Subject: [PATCH 19/20] refactor(gc): consistent hash-side stat names (hash_paths_*, orphaned_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. --- src/datajoint/gc.py | 38 ++++++++++++++++++------------------ tests/integration/test_gc.py | 20 +++++++++---------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index a7d19e963..50e210c38 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -318,33 +318,33 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] Returns a dict with per-section stats and the deletion outcome: - - hash_referenced / hash_stored / hash_orphaned / hash_orphaned_bytes - - orphaned_hashes: orphaned hash items as full relative store paths + - hash_paths_referenced / hash_paths_stored / hash_paths_orphaned / hash_paths_orphaned_bytes + - orphaned_hash_paths: orphaned hash items as full relative store paths (NOT bare hashes — passed as-is to the deleter) - schema_paths_referenced / schema_paths_stored / schema_paths_orphaned / schema_paths_orphaned_bytes - orphaned_paths: orphaned schema files as full relative store paths - - hash_deleted / schema_paths_deleted / deleted / bytes_freed (0 when + - hash_paths_deleted / schema_paths_deleted / deleted / bytes_freed (0 when dry_run) / errors / dry_run """ # References can be gathered across all schemas at once: because paths # embed the schema, a file under schema X is only ever covered by an X # reference, so combined coverage equals per-schema coverage. - hash_referenced = self.hash_references(verbose=verbose) + hash_paths_referenced = self.hash_references(verbose=verbose) schema_paths_referenced = self.schema_references(verbose=verbose) - hash_stored: dict[str, int] = {} + hash_paths_stored: dict[str, int] = {} schema_paths_stored: dict[str, int] = {} for schema in self.schemas: - hash_stored.update(self.list_hash_paths(schema.database)) + hash_paths_stored.update(self.list_hash_paths(schema.database)) schema_paths_stored.update(self.list_schema_paths(schema.database)) - orphaned_hashes = sorted(set(hash_stored.keys()) - hash_referenced) + orphaned_hash_paths = sorted(set(hash_paths_stored.keys()) - hash_paths_referenced) # Coverage, not exact set difference: a referenced path may be a # directory-valued object (many files + a manifest sidecar). orphaned_paths = sorted(p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)) - hash_deleted = 0 + hash_paths_deleted = 0 schema_paths_deleted = 0 bytes_freed = 0 errors = 0 @@ -352,13 +352,13 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] if not dry_run: # The size maps from the scan above are reused for the byte tally — # no second listing pass. - for path in orphaned_hashes: + for path in orphaned_hash_paths: try: if delete_path(path, self.store, config=self.config): - hash_deleted += 1 - bytes_freed += hash_stored.get(path, 0) + hash_paths_deleted += 1 + bytes_freed += hash_paths_stored.get(path, 0) if verbose: - logger.info(f"Deleted: {path} ({hash_stored.get(path, 0)} bytes)") + logger.info(f"Deleted: {path} ({hash_paths_stored.get(path, 0)} bytes)") except Exception as e: errors += 1 logger.warning(f"Failed to delete {path}: {e}") @@ -376,11 +376,11 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] return { # Hash-addressed storage stats - "hash_referenced": len(hash_referenced), - "hash_stored": len(hash_stored), - "hash_orphaned": len(orphaned_hashes), - "hash_orphaned_bytes": sum(hash_stored.get(h, 0) for h in orphaned_hashes), - "orphaned_hashes": orphaned_hashes, + "hash_paths_referenced": len(hash_paths_referenced), + "hash_paths_stored": len(hash_paths_stored), + "hash_paths_orphaned": len(orphaned_hash_paths), + "hash_paths_orphaned_bytes": sum(hash_paths_stored.get(h, 0) for h in orphaned_hash_paths), + "orphaned_hash_paths": orphaned_hash_paths, # Schema-addressed storage stats "schema_paths_referenced": len(schema_paths_referenced), "schema_paths_stored": len(schema_paths_stored), @@ -388,9 +388,9 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] "schema_paths_orphaned_bytes": sum(schema_paths_stored.get(p, 0) for p in orphaned_paths), "orphaned_paths": orphaned_paths, # Deletion outcome (all zero when dry_run) - "hash_deleted": hash_deleted, + "hash_paths_deleted": hash_paths_deleted, "schema_paths_deleted": schema_paths_deleted, - "deleted": hash_deleted + schema_paths_deleted, + "deleted": hash_paths_deleted + schema_paths_deleted, "bytes_freed": bytes_freed, "errors": errors, "dry_run": dry_run, diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 25f4a0dea..761489a34 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -228,8 +228,8 @@ def test_dry_run_reports_stats_and_deletes_nothing(self): assert stats["deleted"] == 0 and stats["bytes_freed"] == 0 mock_delete.assert_not_called() # full report is present - assert stats["hash_orphaned"] == 1 and stats["orphaned_hashes"] == ["_hash/s/path3"] - assert stats["hash_orphaned_bytes"] == 200 + assert stats["hash_paths_orphaned"] == 1 and stats["orphaned_hash_paths"] == ["_hash/s/path3"] + assert stats["hash_paths_orphaned_bytes"] == 200 assert stats["schema_paths_orphaned"] == 1 and stats["orphaned_paths"] == ["s/t/pk2/f"] assert stats["schema_paths_orphaned_bytes"] == 300 # no combined totals @@ -246,7 +246,7 @@ def test_delete_removes_orphans_via_bound_store(self): collector = _mock_collector() stats = collector.collect(dry_run=False) - assert stats["hash_deleted"] == 1 and stats["schema_paths_deleted"] == 1 + assert stats["hash_paths_deleted"] == 1 and stats["schema_paths_deleted"] == 1 assert stats["deleted"] == 2 assert stats["bytes_freed"] == 500 # 200 (hash path3) + 300 (schema pk2) assert stats["dry_run"] is False @@ -303,7 +303,7 @@ def schema_object(self, connection_test, prefix, mock_stores): schema.drop() def test_scan_finds_active_blob_reference(self, schema_blob): - """scan() must report hash_referenced >= 1 for a populated column. + """scan() must report hash_paths_referenced >= 1 for a populated column. Decoded value type returned by BlobCodec.decode is numpy.ndarray, which does not satisfy the raw-metadata dict/JSON-string check in referenced_paths — this @@ -314,7 +314,7 @@ def test_scan_finds_active_blob_reference(self, schema_blob): collector = _gc(schema_blob) stats = collector.collect() - assert stats["hash_referenced"] >= 1, f"scan should find the active reference; got {stats}" + assert stats["hash_paths_referenced"] >= 1, f"scan should find the active reference; got {stats}" def test_scan_finds_active_npy_reference(self, schema_npy): """scan() must report schema_paths_referenced >= 1 for a populated column. @@ -607,8 +607,8 @@ def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): # GC scans the same sections: everything referenced, nothing falsely orphaned stats = collector.collect() - assert stats["hash_referenced"] >= 1 - assert not (hash_refs & set(stats["orphaned_hashes"])) + assert stats["hash_paths_referenced"] >= 1 + assert not (hash_refs & set(stats["orphaned_hash_paths"])) assert not (obj_refs & set(stats["orphaned_paths"])) # And reclamation works within the configured sections @@ -663,7 +663,7 @@ def test_live_hash_object_survives_prefix_change(self, schema_pfx): assert hash_ref not in set( stats["orphaned_paths"] ), "live hash object outside the current hash section must not be a schema orphan" - assert hash_ref not in set(stats["orphaned_hashes"]) + assert hash_ref not in set(stats["orphaned_hash_paths"]) # End-to-end: collect must not destroy the live hash object. collector.collect(dry_run=False) @@ -735,8 +735,8 @@ def test_scan_one_schema_ignores_the_other(self, two_schemas): stats = collector.collect() # b's live objects are outside a's subtree → not counted, not orphaned - assert stats["hash_orphaned"] == 0 and stats["schema_paths_orphaned"] == 0 - assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hashes"]) + assert stats["hash_paths_orphaned"] == 0 and stats["schema_paths_orphaned"] == 0 + assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hash_paths"]) def test_collect_one_schema_never_touches_the_other(self, two_schemas): a, b, b_blob, b_obj = two_schemas From 8218f64a7eb90dbf2e853149dec98e8a0301eb0c Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 6 Jul 2026 17:39:05 -0500 Subject: [PATCH 20/20] refactor(gc): rename orphaned_paths -> orphaned_schema_paths for full 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. --- src/datajoint/gc.py | 12 ++++++------ tests/integration/test_gc.py | 16 +++++++++------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/datajoint/gc.py b/src/datajoint/gc.py index 50e210c38..30ce884a7 100644 --- a/src/datajoint/gc.py +++ b/src/datajoint/gc.py @@ -323,7 +323,7 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] (NOT bare hashes — passed as-is to the deleter) - schema_paths_referenced / schema_paths_stored / schema_paths_orphaned / schema_paths_orphaned_bytes - - orphaned_paths: orphaned schema files as full relative store paths + - orphaned_schema_paths: orphaned schema files as full relative store paths - hash_paths_deleted / schema_paths_deleted / deleted / bytes_freed (0 when dry_run) / errors / dry_run """ @@ -342,7 +342,7 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] orphaned_hash_paths = sorted(set(hash_paths_stored.keys()) - hash_paths_referenced) # Coverage, not exact set difference: a referenced path may be a # directory-valued object (many files + a manifest sidecar). - orphaned_paths = sorted(p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)) + orphaned_schema_paths = sorted(p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)) hash_paths_deleted = 0 schema_paths_deleted = 0 @@ -363,7 +363,7 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] errors += 1 logger.warning(f"Failed to delete {path}: {e}") - for path in orphaned_paths: + for path in orphaned_schema_paths: try: if self.delete_schema_path(path): schema_paths_deleted += 1 @@ -384,9 +384,9 @@ def collect(self, dry_run: bool = True, verbose: bool = False) -> dict[str, Any] # Schema-addressed storage stats "schema_paths_referenced": len(schema_paths_referenced), "schema_paths_stored": len(schema_paths_stored), - "schema_paths_orphaned": len(orphaned_paths), - "schema_paths_orphaned_bytes": sum(schema_paths_stored.get(p, 0) for p in orphaned_paths), - "orphaned_paths": orphaned_paths, + "schema_paths_orphaned": len(orphaned_schema_paths), + "schema_paths_orphaned_bytes": sum(schema_paths_stored.get(p, 0) for p in orphaned_schema_paths), + "orphaned_schema_paths": orphaned_schema_paths, # Deletion outcome (all zero when dry_run) "hash_paths_deleted": hash_paths_deleted, "schema_paths_deleted": schema_paths_deleted, diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 761489a34..42150d832 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -230,7 +230,7 @@ def test_dry_run_reports_stats_and_deletes_nothing(self): # full report is present assert stats["hash_paths_orphaned"] == 1 and stats["orphaned_hash_paths"] == ["_hash/s/path3"] assert stats["hash_paths_orphaned_bytes"] == 200 - assert stats["schema_paths_orphaned"] == 1 and stats["orphaned_paths"] == ["s/t/pk2/f"] + assert stats["schema_paths_orphaned"] == 1 and stats["orphaned_schema_paths"] == ["s/t/pk2/f"] assert stats["schema_paths_orphaned_bytes"] == 300 # no combined totals for k in ("referenced", "stored", "orphaned", "orphaned_bytes"): @@ -374,7 +374,7 @@ def test_custom_codec_reference_not_orphaned(self, schema_custom): live_path = next(iter(refs)) stats = collector.collect() - assert live_path not in stats["orphaned_paths"], f"live custom-codec file wrongly flagged orphan: {live_path}" + assert live_path not in stats["orphaned_schema_paths"], f"live custom-codec file wrongly flagged orphan: {live_path}" def test_custom_codec_survives_collect(self, schema_custom): """#1469 end-to-end data-loss guard: collect() must delete only the @@ -432,7 +432,7 @@ def _make_store_dir(tmp_path, name, n_chunks=3): def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): """A live folder object's chunk files and manifest must be covered by - its referenced prefix — none may appear in orphaned_paths.""" + its referenced prefix — none may appear in orphaned_schema_paths.""" GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "live.zarr"))}) collector = _gc(schema_dirobj) @@ -446,7 +446,9 @@ def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path): assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)" stats = collector.collect() - flagged = [p for p in stats["orphaned_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json"] + flagged = [ + p for p in stats["orphaned_schema_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json" + ] assert not flagged, f"live directory object misclassified as orphaned: {flagged}" def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path): @@ -609,7 +611,7 @@ def test_custom_sections_written_scanned_and_collected(self, schema_prefixed): stats = collector.collect() assert stats["hash_paths_referenced"] >= 1 assert not (hash_refs & set(stats["orphaned_hash_paths"])) - assert not (obj_refs & set(stats["orphaned_paths"])) + assert not (obj_refs & set(stats["orphaned_schema_paths"])) # And reclamation works within the configured sections (GcObjectTest & {"rid": 2}).delete(prompt=False) @@ -661,7 +663,7 @@ def test_live_hash_object_survives_prefix_change(self, schema_pfx): stats = collector.collect() assert hash_ref not in set( - stats["orphaned_paths"] + stats["orphaned_schema_paths"] ), "live hash object outside the current hash section must not be a schema orphan" assert hash_ref not in set(stats["orphaned_hash_paths"]) @@ -736,7 +738,7 @@ def test_scan_one_schema_ignores_the_other(self, two_schemas): stats = collector.collect() # b's live objects are outside a's subtree → not counted, not orphaned assert stats["hash_paths_orphaned"] == 0 and stats["schema_paths_orphaned"] == 0 - assert not any(b.database in p for p in stats["orphaned_paths"] + stats["orphaned_hash_paths"]) + assert not any(b.database in p for p in stats["orphaned_schema_paths"] + stats["orphaned_hash_paths"]) def test_collect_one_schema_never_touches_the_other(self, two_schemas): a, b, b_blob, b_obj = two_schemas