Skip to content

Commit 6e89a7d

Browse files
fix(gc): coverage-based orphan matching for directory-valued objects
The file-level orphan matching introduced earlier in this PR broke DIRECTORY-valued objects (e.g. Zarr stores via <object@>): a row's metadata references the directory PREFIX, while list_schema_paths enumerates the chunk FILES under it — exact set difference therefore flagged every chunk of a LIVE store as an orphan (empirically confirmed), and collect() would delete live data. Folder objects also write a {path}.manifest.json sidecar, which the listing skipped — orphaned folders would leak their manifests forever. Orphan detection is now coverage-based via _is_covered(): a stored file is referenced when it IS a referenced path (single-file object), lies UNDER a referenced path (directory object), or is the .manifest.json sidecar of a referenced object. Manifests are now listed (so orphaned ones are reclaimed); lookup is O(path-depth) ancestor-prefix set membership per file. Note the pre-existing code also mishandled directory objects (the PK-level directory holding the manifest was orphan-flagged, so collect() would rm -r the live object) — neither version was safe; this makes the new file-level model correct for both single-file and directory objects. Tests: a live folder object's chunks+manifest are never flagged (fails before this commit); after deleting one of two rows, collect() reclaims exactly the orphaned folder's chunks+manifest and preserves the survivor's. Full gc suite 34/34.
1 parent 5037a3d commit 6e89a7d

2 files changed

Lines changed: 116 additions & 11 deletions

File tree

src/datajoint/gc.py

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -387,12 +387,14 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i
387387
"""
388388
List all schema-addressed object files in storage.
389389
390-
Enumerates the individual object **files** written by schema-addressed
391-
codecs, whose paths follow ``{schema}/{table}/{pk}/{field}_{token}[.ext]``.
392-
Returning file paths (rather than the enclosing directory) is what lets
393-
them match the file paths recorded in each row's stored metadata — the two
394-
must be comparable for orphan detection, and per-token granularity lets
395-
superseded versions be reclaimed while the current token is retained.
390+
Enumerates the individual **files** written by schema-addressed codecs.
391+
A single-file object is one file at ``{schema}/{table}/{pk}/{field}_{token}[.ext]``;
392+
a directory-valued object (e.g. a Zarr store) is many files under that
393+
path prefix, plus a ``{path}.manifest.json`` sidecar beside it — all are
394+
listed. Orphan detection matches these files against each row's referenced
395+
object path via :func:`_is_covered` (exact, ancestor-prefix, or
396+
manifest-sidecar), so superseded per-token versions are reclaimable while
397+
every file belonging to a live object — including its manifest — is kept.
396398
397399
Parameters
398400
----------
@@ -419,10 +421,11 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i
419421
continue
420422

421423
for filename in files:
422-
# Skip manifest sidecars (mirrors list_stored_hashes)
423-
if filename.endswith(".manifest.json"):
424-
continue
425-
424+
# Manifest sidecars ({object}.manifest.json, written for
425+
# folder-valued objects) are deliberately INCLUDED: they are
426+
# owned by their object and must be reclaimed with it when the
427+
# object is orphaned. _is_covered() keeps a live object's
428+
# manifest out of the orphan set.
426429
file_path = f"{root}/{filename}"
427430
relative_path = file_path.replace(full_prefix, "").lstrip("/")
428431

@@ -494,6 +497,32 @@ def delete_schema_path(path: str, store_name: str | None = None, config=None) ->
494497
return False
495498

496499

500+
def _is_covered(path: str, referenced: set[str]) -> bool:
501+
"""
502+
Return True if a stored file is accounted for by a referenced object path.
503+
504+
A stored file is covered when:
505+
506+
- it IS a referenced path (single-file object, e.g. ``field_token.npy``);
507+
- it lies UNDER a referenced path (directory-valued object, e.g. a Zarr
508+
store ``field_token/`` whose stored form is many chunk files); or
509+
- it is the ``.manifest.json`` sidecar of a referenced object (written
510+
alongside folder-valued objects).
511+
512+
Uses O(path-depth) ancestor-prefix lookups against the referenced set.
513+
"""
514+
if path.endswith(".manifest.json"):
515+
path = path[: -len(".manifest.json")]
516+
if path in referenced:
517+
return True
518+
idx = path.rfind("/")
519+
while idx > 0:
520+
if path[:idx] in referenced:
521+
return True
522+
idx = path.rfind("/", 0, idx)
523+
return False
524+
525+
497526
def scan(
498527
*schemas: "Schema",
499528
store_name: str | None = None,
@@ -545,7 +574,10 @@ def scan(
545574
# --- Schema-addressed storage ---
546575
schema_paths_referenced = scan_schema_references(*schemas, store_name=store_name, verbose=verbose)
547576
schema_paths_stored = list_schema_paths(store_name, config=_config)
548-
orphaned_paths = set(schema_paths_stored.keys()) - schema_paths_referenced
577+
# Coverage, not exact set difference: a referenced path may be a
578+
# DIRECTORY-valued object (e.g. a Zarr store), whose stored form is many
579+
# files under that prefix, plus a `.manifest.json` sidecar beside it.
580+
orphaned_paths = {p for p in schema_paths_stored if not _is_covered(p, schema_paths_referenced)}
549581
schema_paths_orphaned_bytes = sum(schema_paths_stored.get(p, 0) for p in orphaned_paths)
550582

551583
return {

tests/integration/test_gc.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,3 +545,76 @@ def test_custom_codec_survives_collect(self, schema_custom):
545545
stored_after = set(gc.list_schema_paths("local", config=schema_custom.connection._config))
546546
assert live_path in stored_after, f"collect() deleted the live custom-codec file {live_path} (#1469)"
547547
assert not (deleted_paths & stored_after), f"deleted row's orphan not reclaimed: {deleted_paths & stored_after}"
548+
549+
550+
class TestDirectoryObjects:
551+
"""Directory-valued objects (e.g. Zarr stores) — the referenced metadata
552+
path is a directory PREFIX whose stored form is many chunk files plus a
553+
`{path}.manifest.json` sidecar. Orphan matching must be coverage-based
554+
(exact / ancestor-prefix / manifest), not exact set difference: with exact
555+
matching, every chunk of a LIVE store is misclassified as an orphan and
556+
collect() deletes live data."""
557+
558+
@pytest.fixture
559+
def schema_dirobj(self, connection_test, prefix, mock_stores):
560+
schema = dj.Schema(
561+
f"{prefix}_test_gc_dirobj",
562+
context={"GcObjectTest": GcObjectTest},
563+
connection=connection_test,
564+
)
565+
schema(GcObjectTest)
566+
yield schema
567+
schema.drop()
568+
569+
@staticmethod
570+
def _make_store_dir(tmp_path, name, n_chunks=3):
571+
d = tmp_path / name
572+
d.mkdir()
573+
(d / ".zarray").write_bytes(b"{}")
574+
for i in range(n_chunks):
575+
(d / f"0.{i}").write_bytes(f"chunk{i}".encode())
576+
return d
577+
578+
def test_live_directory_object_not_orphaned(self, schema_dirobj, tmp_path):
579+
"""A live folder object's chunk files and manifest must be covered by
580+
its referenced prefix — none may appear in orphaned_paths."""
581+
GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "live.zarr"))})
582+
583+
refs = gc.scan_schema_references(schema_dirobj, store_name="local")
584+
assert len(refs) == 1
585+
ref = next(iter(refs))
586+
587+
stored = gc.list_schema_paths("local", config=schema_dirobj.connection._config)
588+
chunk_files = [p for p in stored if p.startswith(ref + "/")]
589+
assert len(chunk_files) >= 4, f"expected the folder's files under {ref}; got {sorted(stored)}"
590+
assert f"{ref}.manifest.json" in stored, "manifest sidecar must be listed (it is reclaimable state)"
591+
592+
stats = gc.scan(schema_dirobj, store_name="local")
593+
flagged = [p for p in stats["orphaned_paths"] if p == ref or p.startswith(ref + "/") or p == f"{ref}.manifest.json"]
594+
assert not flagged, f"live directory object misclassified as orphaned: {flagged}"
595+
596+
def test_orphaned_directory_object_fully_reclaimed(self, schema_dirobj, tmp_path):
597+
"""After deleting one row, collect() must reclaim that folder object's
598+
chunks AND manifest while leaving the surviving row's folder intact."""
599+
GcObjectTest.insert1({"rid": 1, "results": str(self._make_store_dir(tmp_path, "gone.zarr"))})
600+
GcObjectTest.insert1({"rid": 2, "results": str(self._make_store_dir(tmp_path, "kept.zarr"))})
601+
602+
refs_all = gc.scan_schema_references(schema_dirobj, store_name="local")
603+
assert len(refs_all) == 2
604+
605+
(GcObjectTest & {"rid": 1}).delete(prompt=False)
606+
refs_live = gc.scan_schema_references(schema_dirobj, store_name="local")
607+
assert len(refs_live) == 1
608+
live_ref = next(iter(refs_live))
609+
dead_ref = next(iter(refs_all - refs_live))
610+
611+
gc.collect(schema_dirobj, store_name="local", dry_run=False)
612+
613+
stored_after = set(gc.list_schema_paths("local", config=schema_dirobj.connection._config))
614+
live_files = {p for p in stored_after if p.startswith(live_ref + "/")}
615+
assert len(live_files) >= 4, f"live folder object lost files: {sorted(stored_after)}"
616+
assert f"{live_ref}.manifest.json" in stored_after, "live object's manifest must survive collect()"
617+
618+
dead_files = {p for p in stored_after if p == dead_ref or p.startswith(dead_ref + "/")}
619+
assert not dead_files, f"orphaned folder object not reclaimed: {sorted(dead_files)}"
620+
assert f"{dead_ref}.manifest.json" not in stored_after, "orphaned object's manifest must be reclaimed"

0 commit comments

Comments
 (0)