From fb6c280e56079d6befdb5ff982d42aa46aecfbd9 Mon Sep 17 00:00:00 2001 From: d-laub Date: Wed, 23 Sep 2026 22:00:26 +0000 Subject: [PATCH] fix(svar2): scope the link fingerprint to payload files (#419) `_svar2_store_fingerprint` walked the whole `.svar2` store with `rglob("*")`, so any file added beside the payload changed the recorded `{n_files, store_bytes}` and every dataset linked to that store began failing verification. The docstring already promised the narrow contract; the implementation did not honour it. Scope the fingerprint to the payload: each contig's `dense`, `fields`, `indel` and `var_key` subtrees plus `max_del.npy`, counting only `.bin`/`.npy`. Writing an annotation layer (`mutcat/`, say) alongside the payload no longer invalidates the link. Narrowing the scope alone would break datasets that verify cleanly today -- a link recorded under the old broad scope against a store that already had sibling layers would newly mismatch. So a fingerprint that matches the legacy broad walk is still accepted, as a second exact match. Only a value matching neither is a mismatch. On mismatch the error now says which scope it used, what the payload actually measures, and whether a sibling layer can account for the difference -- including the case where the recorded value is larger than the payload and no layer remains, which is what both a pre-scoping record and real truncation look like. The message says so rather than guessing. All new symbols are private; no public API change. Co-Authored-By: Claude Opus 5 (1M context) --- python/genvarloader/_dataset/_svar2_link.py | 156 +++++++++++++- tests/unit/dataset/test_svar2_link.py | 224 ++++++++++++++++++++ 2 files changed, 372 insertions(+), 8 deletions(-) diff --git a/python/genvarloader/_dataset/_svar2_link.py b/python/genvarloader/_dataset/_svar2_link.py index 276361a7..a9ef87c3 100644 --- a/python/genvarloader/_dataset/_svar2_link.py +++ b/python/genvarloader/_dataset/_svar2_link.py @@ -28,16 +28,87 @@ class Svar2Link(BaseModel): fingerprint: Svar2Fingerprint +#: Per-contig subtrees whose bytes a dataset actually reads. Anything else in +#: a contig directory belongs to some other pipeline and is none of this +#: fingerprint's business -- see `_svar2_payload_files`. +_PAYLOAD_SUBTREES: tuple[str, ...] = ("dense", "fields", "indel", "var_key") + +#: Per-contig payload files that sit directly in the contig directory. +_PAYLOAD_FILES: tuple[str, ...] = ("max_del.npy",) + +_PAYLOAD_SUFFIXES = frozenset({".bin", ".npy"}) + + +def _svar2_payload_files(svar2_path: Path) -> list[Path]: + """The ``.bin``/``.npy`` files under the payload subtrees, sorted. + + Deliberately NOT a walk of the whole store. A ``.svar2`` store is written + by one pipeline and read by many, so it accumulates sibling layers -- + per-contig annotations, scratch indices -- that no dataset opens. Counting + those makes an unrelated pipeline's write break every reader of the store + (issue #419), and it does so in the direction that matters least: the + variant data is byte-for-byte unchanged while every consumer goes down. + + Scoping to the read payload keeps the documented contract exactly -- the + fingerprint still changes iff the data the dataset reads changes -- while + additive siblings become invisible, which is the correct behavior. + """ + files: list[Path] = [] + for contig in sorted(p for p in svar2_path.iterdir() if p.is_dir()): + for name in _PAYLOAD_FILES: + f = contig / name + if f.is_file() and f.suffix in _PAYLOAD_SUFFIXES: + files.append(f) + for sub in _PAYLOAD_SUBTREES: + d = contig / sub + if not d.is_dir(): + continue + files.extend( + p for p in d.rglob("*") if p.is_file() and p.suffix in _PAYLOAD_SUFFIXES + ) + return sorted(files) + + +def _svar2_ignored_files(svar2_path: Path) -> list[Path]: + """``.bin``/``.npy`` files in the store that the payload scope excludes. + + Used only to explain a mismatch. A large ignored set is the signature of + a sibling annotation layer, and saying so turns issue #419's manual scope + bisection into a line of the error message. + """ + payload = set(_svar2_payload_files(svar2_path)) + return sorted( + p + for p in svar2_path.rglob("*") + if p.is_file() and p.suffix in _PAYLOAD_SUFFIXES and p not in payload + ) + + +def _svar2_legacy_store_fingerprint(svar2_path: Path) -> tuple[int, int]: + """The pre-scoping fingerprint: every ``.bin``/``.npy`` in the store. + + Kept solely to honor links recorded before the scope was narrowed. Those + records counted sibling layers, so re-scoping would invalidate every one + of them -- a check that starts rejecting stores it accepted yesterday is + worse than the bug being fixed. + """ + files = [ + p + for p in svar2_path.rglob("*") + if p.is_file() and p.suffix in _PAYLOAD_SUFFIXES + ] + return len(files), sum(p.stat().st_size for p in files) + + def _svar2_store_fingerprint(svar2_path: Path) -> tuple[int, int]: """Deterministic (file count, total bytes) over the .svar2 store's data files. - Walks the store for ``.bin``/``.npy`` files (dense + var_key + long-allele - payloads across all contigs). Changes iff the store's data files change -- - that is this fingerprint's only contract. + Covers the payload subtrees only (dense + fields + var_key + long-allele + across all contigs, plus each contig's ``max_del.npy``). Changes iff the + store's data files change -- that is this fingerprint's only contract, and + scoping the walk is what makes the implementation match it. """ - files = sorted( - p for p in svar2_path.rglob("*") if p.is_file() and p.suffix in {".bin", ".npy"} - ) + files = _svar2_payload_files(svar2_path) return len(files), sum(p.stat().st_size for p in files) @@ -88,9 +159,19 @@ def _verify_svar2_fingerprint(svar2_path: Path, link: Svar2Link | None) -> None: if link is None: return + exp = link.fingerprint + n_files_observed, bytes_observed = _svar2_store_fingerprint(svar2_path) + if (n_files_observed, bytes_observed) == (exp.n_files, exp.store_bytes): + return + + # A link recorded before the fingerprint was scoped counted every + # .bin/.npy in the store, sibling layers included. Accept that shape too: + # narrowing the scope fixes issue #419 for stores that GAIN a layer, and + # it must not break the datasets that verify cleanly today. + if _svar2_legacy_store_fingerprint(svar2_path) == (exp.n_files, exp.store_bytes): + return - exp = link.fingerprint mismatches: list[str] = [] if n_files_observed != exp.n_files: mismatches.append( @@ -102,8 +183,67 @@ def _verify_svar2_fingerprint(svar2_path: Path, link: Svar2Link | None) -> None: ) if mismatches: raise ValueError( - f"svar2 fingerprint mismatch at {svar2_path}: " + "; ".join(mismatches) + f"svar2 fingerprint mismatch at {svar2_path}: " + + "; ".join(mismatches) + + _mismatch_diagnosis(svar2_path, exp) + ) + + +def _mismatch_diagnosis(svar2_path: Path, exp: Svar2Fingerprint) -> str: + """Context appended to a mismatch, so the message is actionable on its own. + + The person who has to diagnose this is whoever's job broke, not whoever + changed the store, and they get one traceback to work from. Byte counts + alone forced a manual scope bisection (issue #419), so state the scope + that was used, what it covered, and -- the part that actually identifies + a sibling-layer situation -- what it ignored. + """ + try: + payload = _svar2_payload_files(svar2_path) + ignored = _svar2_ignored_files(svar2_path) + except OSError as exc: # unreadable store: say so rather than masking it + return f"\n (could not scan the store to explain this: {exc})" + + lines = [ + "", + f" scope: payload only -- each contig's {list(_PAYLOAD_FILES)} plus " + f"{list(_PAYLOAD_SUBTREES)}", + f" payload files counted: {len(payload)} " + f"({sum(p.stat().st_size for p in payload)} bytes)", + ] + if ignored: + example = ignored[0].relative_to(svar2_path) + lines.append( + f" non-payload files ignored: {len(ignored)} " + f"({sum(p.stat().st_size for p in ignored)} bytes), e.g. {example}" + ) + lines.append( + " a sibling layer like that does NOT affect this fingerprint; if " + "the recorded value was written before the fingerprint was scoped " + "to the payload, it counted those files and must be re-recorded." + ) + elif exp.n_files > len(payload) and exp.store_bytes > sum( + q.stat().st_size for q in payload + ): + # A record written under the old unscoped walk counted a SUPERSET of + # the payload, so it is larger on both axes. A store that has since + # lost the sibling layer has nothing left to point at, which is how + # this looks from the consumer's side -- and calling that corruption + # would send someone hunting for damage that is not there. + lines.append( + " the recorded value is larger than the payload on both counts " + "and no sibling layer remains to account for it. That is what a " + "record written before the fingerprint was scoped looks like once " + "the layer is gone; it is also what real truncation looks like. " + "Compare against a replica that still carries the layer before " + "deciding, then either restore the store or re-record the link." + ) + else: + lines.append( + " no non-payload files present, so this mismatch is in the data " + "the dataset actually reads -- treat the store as modified." ) + return "\n".join(lines) def make_svar2_link(gvl_path: Path, svar2_path: Path) -> Svar2Link: diff --git a/tests/unit/dataset/test_svar2_link.py b/tests/unit/dataset/test_svar2_link.py index 4462e3fa..7fb40bd5 100644 --- a/tests/unit/dataset/test_svar2_link.py +++ b/tests/unit/dataset/test_svar2_link.py @@ -62,3 +62,227 @@ def test_fingerprint_detects_mutated_store(svar2_store_2s: Path, tmp_path: Path) with pytest.raises(ValueError): _verify_svar2_fingerprint(store, link) + + +def _add_annotation_layer(store: Path, name: str = "mutcat") -> list[Path]: + """Write an additive annotation layer into every contig of `store`. + + Reproduces the shape of the layer in issue #419: a separate pipeline writes + per-contig annotations beside the payload, touching nothing the dataset + reads. Returns the files it created. + """ + written: list[Path] = [] + for contig in sorted(p for p in store.iterdir() if p.is_dir()): + layer = contig / name + layer.mkdir() + for fname in ("categories.npy", "offsets.npy", "codes.bin", "meta.bin"): + f = layer / fname + f.write_bytes(b"\x01" * 32) + written.append(f) + return written + + +def test_additive_annotation_layer_does_not_break_the_fingerprint( + svar2_store_2s: Path, tmp_path: Path +): + """Issue #419. A layer the dataset never reads must be invisible here. + + The fingerprint's stated contract is "changes iff the store's data files + change". A sibling layer is not a data file, and counting it takes a shared + store offline for every consumer while the variant data is byte-for-byte + unchanged. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + + link = make_svar2_link(gvl_path, store) + _verify_svar2_fingerprint(store, link) # baseline: clean + + added = _add_annotation_layer(store) + assert added, "the layer fixture must actually write files" + + # The store on disk now has strictly more .npy/.bin files than when the + # link was recorded, and none of them are payload. + _verify_svar2_fingerprint(store, link) + + +def test_removing_an_annotation_layer_also_leaves_the_fingerprint_alone( + svar2_store_2s: Path, tmp_path: Path +): + """The same bug in the other direction, which is how aster hit it. + + There the link was recorded while the layer was present and the store + later reached a consumer without it, so the recorded value referenced + bytes that no longer existed. A payload-scoped fingerprint is symmetric: + neither adding nor removing a sibling layer can move it. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + + added = _add_annotation_layer(store) + link = make_svar2_link(gvl_path, store) # recorded WITH the layer + _verify_svar2_fingerprint(store, link) + + for f in added: + f.unlink() + for contig in sorted(p for p in store.iterdir() if p.is_dir()): + (contig / "mutcat").rmdir() + + _verify_svar2_fingerprint(store, link) + + +def test_a_new_payload_file_is_still_detected(svar2_store_2s: Path, tmp_path: Path): + """Scoping must not become a hole: payload additions still count. + + Without this, "ignore what we don't read" could quietly widen into + "ignore additions", and a store that gained a contig would verify clean. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + link = make_svar2_link(gvl_path, store) + + contig = next(p for p in sorted(store.iterdir()) if p.is_dir()) + (contig / "var_key" / "snp" / "extra.bin").write_bytes(b"\x00" * 16) + + with pytest.raises(ValueError): + _verify_svar2_fingerprint(store, link) + + +def test_a_real_payload_mismatch_says_so_and_rules_out_a_sibling_layer( + svar2_store_2s: Path, tmp_path: Path +): + """Issue #419 asked the error to name appeared/disappeared files. + + It cannot name the specific file: the record is two integers, so there is + no baseline to diff against, and inventing one would mean writing a + per-file manifest into every dataset. What the message CAN do without + storing anything more is state the scope it used and separate the two + cases a reader has to tell apart -- and that is the part that cost a + manual bisection. Here nothing but payload exists, so the verdict is + unambiguous. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + link = make_svar2_link(gvl_path, store) + + target = sorted(store.rglob("var_key/snp/offsets.npy"))[0] + with open(target, "ab") as f: + f.write(b"\x00") + + with pytest.raises(ValueError) as exc: + _verify_svar2_fingerprint(store, link) + + msg = str(exc.value) + assert "scope: payload only" in msg + assert "var_key" in msg, "the message must say what the scope covers" + assert "treat the store as modified" in msg + assert "re-recorded" not in msg, ( + "with no sibling layer present the message must NOT suggest the " + "recorded value is merely stale -- that would excuse real corruption" + ) + + +def test_a_record_larger_than_the_payload_is_not_called_corruption( + svar2_store_2s: Path, tmp_path: Path +): + """The direction aster hit, seen from a store that no longer has the layer. + + Once the sibling layer is gone there is nothing left to point at, so the + store alone cannot prove which happened. The message must therefore say + both readings are open rather than assert corruption -- a record written + under the old unscoped walk is strictly larger than the payload on both + axes, and so is a truncated store. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + + payload = make_svar2_link(gvl_path, store).fingerprint + stale = Svar2Link( + relative_path="store.svar2", + absolute_path=str(store), + fingerprint=Svar2Fingerprint( + n_files=payload.n_files + 4, + store_bytes=payload.store_bytes + 128, + ), + ) + + with pytest.raises(ValueError) as exc: + _verify_svar2_fingerprint(store, stale) + + msg = str(exc.value) + assert "before the fingerprint was scoped" in msg + assert "Compare against a replica" in msg + assert "treat the store as modified" not in msg, ( + "must not assert corruption when a stale broad-scope record explains " + "the mismatch equally well" + ) + + +def test_a_link_recorded_before_scoping_still_verifies( + svar2_store_2s: Path, tmp_path: Path +): + """Narrowing the scope must not invalidate records that pass today. + + Datasets in the wild were linked while the walk covered the whole store, + so their recorded value includes any sibling layer present at write time. + Those stores are healthy and open fine; if scoping started rejecting them + the fix would break more than the bug did. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + + _add_annotation_layer(store) + broad = [ + q for q in store.rglob("*") if q.is_file() and q.suffix in {".bin", ".npy"} + ] + legacy = Svar2Link( + relative_path="store.svar2", + absolute_path=str(store), + fingerprint=Svar2Fingerprint( + n_files=len(broad), + store_bytes=sum(q.stat().st_size for q in broad), + ), + ) + + _verify_svar2_fingerprint(store, legacy) # must not raise + + +def test_legacy_acceptance_does_not_excuse_a_modified_payload( + svar2_store_2s: Path, tmp_path: Path +): + """The legacy path is a second exact match, never a loosening. + + If it degraded into "close enough", a store whose variant data changed + would sail through on the strength of an old record -- the one outcome + this fingerprint exists to prevent. + """ + store = shutil.copytree(svar2_store_2s, tmp_path / "store.svar2") + gvl_path = tmp_path / "ds.gvl" + gvl_path.mkdir() + + _add_annotation_layer(store) + broad = [ + q for q in store.rglob("*") if q.is_file() and q.suffix in {".bin", ".npy"} + ] + legacy = Svar2Link( + relative_path="store.svar2", + absolute_path=str(store), + fingerprint=Svar2Fingerprint( + n_files=len(broad), + store_bytes=sum(q.stat().st_size for q in broad), + ), + ) + _verify_svar2_fingerprint(store, legacy) + + payload_file = sorted(store.rglob("var_key/snp/offsets.npy"))[0] + with open(payload_file, "ab") as f: + f.write(b"\x00") + + with pytest.raises(ValueError): + _verify_svar2_fingerprint(store, legacy)