From e14ab5ee6a8c6e25fbfc5a3382fb6b460f592c35 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sat, 1 Aug 2026 23:06:40 +0200 Subject: [PATCH 01/16] Add a check mode that validates data without downloading it fwl-io could serve what is present and refuse when it is not, or download what is missing. Neither answers the question a diagnostic asks: is this tree complete and intact? `proteus doctor` wants the whole picture in one pass, and it must not repair the thing it is inspecting. `check_for(model)` returns a report of every dataset the model requires, one entry per file, in one of four states. A file is ok when it is present and matches the registry, missing when it is not there, and mismatch when it is there and its contents differ. Those last two are kept apart because the remedies differ: an absent file may never have been fetched, while a corrupt one was fetched and then damaged. The fourth state is present, meaning the file is on disk and nothing was available to check it against. An archive dataset's registry pins the checksum of the archive, not of what came out of it, so once the archive is dropped there is nothing to hash its members against. A report that called those verified would claim more than it checked, so it says presence only and `hashed` is false for that dataset. A manifest that fails to load is carried in the report rather than dropped, and it alone is enough to make the report not ok. Its datasets were never inspected, so treating it as harmless would let a tree with an unreadable provider read exactly like a healthy one. `fwl-io check ` prints the report and exits 1 on any fault. Nothing is downloaded and no dataset directory or file is written; resolving the data root creates that root when absent, as it does for every entry point, and that is the only mark a check leaves. Closes #20. --- src/fwl_io/__init__.py | 6 + src/fwl_io/check.py | 262 +++++++++++++++++++++++++++++++++++++++++ src/fwl_io/cli.py | 20 ++++ tests/test_check.py | 261 ++++++++++++++++++++++++++++++++++++++++ tests/test_cli.py | 49 ++++++++ 5 files changed, 598 insertions(+) create mode 100644 src/fwl_io/check.py create mode 100644 tests/test_check.py diff --git a/src/fwl_io/__init__.py b/src/fwl_io/__init__.py index aa46276..ac8b6d3 100644 --- a/src/fwl_io/__init__.py +++ b/src/fwl_io/__init__.py @@ -9,6 +9,7 @@ from importlib.metadata import PackageNotFoundError, version +from fwl_io.check import CheckReport, DatasetCheck, FileCheck, check_dataset, check_for from fwl_io.fetch import DownloadError, Fetcher, OfflineDataError, create_fetcher from fwl_io.manifest import ( Dataset, @@ -25,13 +26,18 @@ __version__ = '0.0.0' __all__ = [ + 'CheckReport', 'Dataset', + 'DatasetCheck', 'DownloadError', + 'FileCheck', 'Fetcher', 'ManifestSchemaError', 'MissingDataRootError', 'OfflineDataError', '__version__', + 'check_dataset', + 'check_for', 'create_fetcher', 'discover_manifests', 'fetch_for', diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py new file mode 100644 index 0000000..fb5040b --- /dev/null +++ b/src/fwl_io/check.py @@ -0,0 +1,262 @@ +"""Report whether a data tree matches its manifest, without downloading. + +This sits between the two behaviours the fetcher already has. Offline mode +serves what is present and raises as soon as something is not; online mode +downloads whatever is missing. Neither can answer "is this tree complete and +intact", which is what a diagnostic such as ``proteus doctor`` needs: it wants +the whole picture in one pass, and it must not repair the thing it is +inspecting. + +A check therefore never reaches the network, never downloads, and never +creates or repairs a dataset. It reads the manifest, hashes what is on disk, +and returns a report. The one mark it leaves is the data root itself, which +resolving a path creates when it is absent, exactly as every other entry point +does; no dataset directory and no file is written. + +Two things the report is careful about, because a diagnostic that overstates +what it verified is worse than no diagnostic at all. A manifest that fails to +load is carried in the report rather than dropped, so a tree cannot read as +complete when whole datasets were never looked at. And an archive dataset's +members are reported as present rather than as verified: the archive-only +checksum policy records member names, not per-file digests, so there is +nothing to hash them against once the archive itself is gone. +""" + +from __future__ import annotations + +import json +import logging +from dataclasses import dataclass +from pathlib import Path + +from fwl_io.fetch import _STAMP_FILENAME, Fetcher, _hash_matches, create_fetcher + +log = logging.getLogger('fwl.' + __name__) + +# A file is in exactly one of these states. ``present`` is deliberately +# distinct from ``ok``: it says the file is on disk and that nothing was +# available to check its contents against, which is a weaker statement than +# ``ok`` and must not be reported as the same thing. +OK = 'ok' +MISSING = 'missing' +MISMATCH = 'mismatch' +PRESENT = 'present' + +#: States that mean the tree is not usable as the manifest describes it. +FAULT_STATES = (MISSING, MISMATCH) + + +@dataclass(frozen=True) +class FileCheck: + """The state of one file the manifest declares.""" + + name: str + path: Path + state: str + + @property + def faulty(self) -> bool: + """True when this file is absent or does not match its checksum.""" + return self.state in FAULT_STATES + + +@dataclass(frozen=True) +class DatasetCheck: + """The state of every file in one dataset.""" + + key: str + subdir: str + directory: Path + files: tuple[FileCheck, ...] + + @property + def missing(self) -> tuple[FileCheck, ...]: + """Files the manifest declares that are not on disk.""" + return tuple(f for f in self.files if f.state == MISSING) + + @property + def mismatched(self) -> tuple[FileCheck, ...]: + """Files on disk whose contents differ from the registry.""" + return tuple(f for f in self.files if f.state == MISMATCH) + + @property + def complete(self) -> bool: + """True when nothing is missing and nothing fails its checksum.""" + return not any(f.faulty for f in self.files) + + @property + def hashed(self) -> bool: + """True when every file present was checked against a known digest. + + False for an archive dataset, whose members are recorded by name only, + so a truncated member is indistinguishable from an intact one here. + """ + return not any(f.state == PRESENT for f in self.files) + + def summary(self) -> str: + """One line naming the counts, for a report a person reads.""" + total = len(self.files) + parts = [f'{total} file(s)'] + if self.missing: + parts.append(f'{len(self.missing)} missing') + if self.mismatched: + parts.append(f'{len(self.mismatched)} corrupt') + if not self.hashed: + parts.append('presence only') + state = 'ok' if self.complete else 'FAILED' + return f'{self.key}: {state}, ' + ', '.join(parts) + + +@dataclass(frozen=True) +class CheckReport: + """Every dataset checked, and every manifest that could not be read.""" + + datasets: dict[str, DatasetCheck] + manifest_errors: dict[str, str] + + @property + def ok(self) -> bool: + """True only when every dataset is complete and every manifest loaded. + + A manifest that failed to load counts against the report. Its datasets + were never inspected, so treating it as harmless would let a tree with + an unreadable provider report exactly like a healthy one. + """ + return not self.manifest_errors and all(d.complete for d in self.datasets.values()) + + @property + def faults(self) -> tuple[DatasetCheck, ...]: + """Datasets with something missing or corrupt, worst named first.""" + broken = [d for d in self.datasets.values() if not d.complete] + return tuple(sorted(broken, key=lambda d: (-len(d.missing) - len(d.mismatched), d.key))) + + def summary(self) -> str: + """A short human-readable report, one line per dataset plus a verdict.""" + lines = [d.summary() for d in sorted(self.datasets.values(), key=lambda d: d.key)] + for provider, error in sorted(self.manifest_errors.items()): + lines.append(f'{provider}: MANIFEST UNREADABLE, {error}') + if not lines: + return 'no datasets checked' + lines.append('all data present and verified' if self.ok else 'data check FAILED') + return '\n'.join(lines) + + +def _archive_members(fetcher: Fetcher) -> list[str] | None: + """Return the member names an archive dataset's stamp recorded. + + ``None`` when there is no usable stamp, which means the extracted tree is + not there to be checked rather than that it is empty. + """ + stamp = fetcher.target_dir / _STAMP_FILENAME + try: + record = json.loads(stamp.read_text()) + except (OSError, ValueError): + return None + if record.get('extract') != fetcher.extract: + # The stamp describes a different fetch of this deposit, so its member + # list does not describe the tree this dataset expects. + return None + members = record.get('members') + if not isinstance(members, list) or not members: + return None + return [str(m) for m in members] + + +def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: + """Report the state of one dataset's files without touching the network. + + Parameters + ---------- + fetcher : Fetcher + Configured for the dataset to inspect. Nothing is fetched. + key : str, optional + Name for the dataset in the report; defaults to its subdirectory. + + Returns + ------- + DatasetCheck + One entry per file the manifest declares. For an archive dataset the + entries are the extracted members recorded in the provenance stamp, + reported as present rather than verified, since the registry pins the + archive's checksum and not the checksums of what came out of it. + """ + key = key or fetcher.subdir + + if fetcher.extract is not None: + members = _archive_members(fetcher) + if members is None: + # No usable stamp means no extracted tree to speak of. The dataset + # is reported as one missing item under the archive's own name, + # rather than as zero items, which would read as complete. + archive_name = next(iter(fetcher.registry)) + files = (FileCheck(archive_name, fetcher.target_dir / archive_name, MISSING),) + return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, files) + checks = [] + for name in sorted(members): + path = fetcher.target_dir / name + checks.append(FileCheck(name, path, PRESENT if path.is_file() else MISSING)) + return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) + + checks = [] + for name, known_hash in sorted(fetcher.registry.items()): + path = fetcher.target_dir / name + if not path.is_file(): + state = MISSING + elif _hash_matches(path, known_hash): + state = OK + else: + state = MISMATCH + checks.append(FileCheck(name, path, state)) + return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) + + +def check_for(model: str, data_root: str | Path | None = None) -> CheckReport: + """Report the state of every dataset a given model requires. + + Nothing is downloaded and no dataset directory or file is written, so this + is safe to run against a tree another process is reading. Resolving the + data root creates that root when it is absent, which is the only mark a + check leaves. + + A dataset whose registry or fetcher cannot be built is reported as a + manifest error rather than skipped, for the same reason an unreadable + manifest is: the alternative is a report that looks clean because it + checked less than it appears to. + + Parameters + ---------- + model : str + Model name matched (case-insensitively) against ``required_by``. + data_root : str | Path | None + Override for the data root; defaults to the resolved FWL_DATA tree. + + Returns + ------- + CheckReport + Keyed by dataset, alongside every manifest that could not be read. + """ + from fwl_io.manifest import _discover + + model = model.lower() + datasets: dict[str, DatasetCheck] = {} + providers, errors = _discover() + manifest_errors = dict(errors) + for provider_datasets in providers.values(): + for ds in provider_datasets: + if model not in tuple(r.lower() for r in ds.required_by): + continue + try: + fetcher = create_fetcher( + subdir=ds.subdir, + zenodo=ds.zenodo, + dataverse=ds.dataverse, + registry=ds.registry(), + data_root=data_root, + extract=ds.extract, + ) + except Exception as exc: # noqa: BLE001 -- reported, never raised + manifest_errors[ds.key] = str(exc) + log.warning('cannot check dataset %r: %s', ds.key, exc) + continue + datasets[ds.key] = check_dataset(fetcher, key=ds.key) + return CheckReport(datasets=datasets, manifest_errors=manifest_errors) diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index b4d6d8c..7eb706b 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -50,6 +50,19 @@ def _cmd_fetch(args: argparse.Namespace) -> int: return 0 +def _cmd_check(args: argparse.Namespace) -> int: + from fwl_io.check import check_for + + report = check_for(args.model, data_root=args.data_root) + if not report.datasets and not report.manifest_errors: + print(f'no datasets declare required_by = {args.model!r}', file=sys.stderr) + return 1 + # The summary goes to stdout whatever the verdict: a caller running this to + # find out what is wrong needs the detail, not just the exit code. + print(report.summary()) + return 0 if report.ok else 1 + + def _cmd_mirror(args: argparse.Namespace) -> int: from fwl_io.mirror import mirror_to_dataverse @@ -97,6 +110,13 @@ def main(argv: list[str] | None = None) -> int: p_fetch.add_argument('--data-root', default=None, help='override the FWL_DATA root') p_fetch.set_defaults(func=_cmd_fetch) + p_check = sub.add_parser( + 'check', help='report whether a model has its data, without downloading' + ) + p_check.add_argument('model', help='model name matched against required_by') + p_check.add_argument('--data-root', default=None, help='override the FWL_DATA root') + p_check.set_defaults(func=_cmd_check) + p_mirror = sub.add_parser('mirror', help='mirror a Zenodo deposit to a Dataverse collection') p_mirror.add_argument('zenodo_doi', help='Zenodo version DOI to mirror') p_mirror.add_argument('--collection', required=True, help='target Dataverse collection alias') diff --git a/tests/test_check.py b/tests/test_check.py new file mode 100644 index 0000000..0f21f0f --- /dev/null +++ b/tests/test_check.py @@ -0,0 +1,261 @@ +"""Tests for :mod:`fwl_io.check`, the validate-only mode. + +The contract exercised here is that a check reports the state of a data tree +and changes nothing: no download, no repair, no write. It also has to keep +apart three outcomes a caller acts on differently, an absent file, a corrupt +one, and one that is present but has nothing to be verified against, and it +must never let an unread manifest read as a clean tree. + +No test here touches the network. Every fetcher is built against a URL on a +closed port, so a check that tried to download would fail loudly rather than +quietly pass. +""" + +from __future__ import annotations + +import hashlib +import json +import socket + +import pytest + +from fwl_io.check import ( + MISMATCH, + MISSING, + OK, + PRESENT, + CheckReport, + DatasetCheck, + check_dataset, +) +from fwl_io.fetch import create_fetcher + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + +SUBDIR = 'interior_lookup_tables/demo' +RECID = '15729114' +ZENODO = f'10.5281/zenodo.{RECID}' +STAMP = '.fwl-io.json' + +# Two files with deliberately different contents and lengths, so a check that +# compared the wrong file against the wrong digest cannot pass by coincidence. +CONTENTS = {'alpha.dat': b'0.1 0.2 0.3\n', 'beta.dat': b'9.87\n'} + + +def _closed_url(): + """A URL on a port bound then released, so any request fails at once.""" + with socket.socket() as sock: + sock.bind(('127.0.0.1', 0)) + port = sock.getsockname()[1] + return f'http://127.0.0.1:{port}/' + + +def _registry(names=None): + names = CONTENTS if names is None else {n: CONTENTS[n] for n in names} + return {n: f'sha256:{hashlib.sha256(b).hexdigest()}' for n, b in names.items()} + + +def _plain_fetcher(data_root, registry=None): + return create_fetcher( + subdir=SUBDIR, + registry=registry or _registry(), + base_urls=[_closed_url()], + data_root=data_root, + ) + + +def _archive_fetcher(data_root): + return create_fetcher( + subdir=SUBDIR, + registry={'bundle.tar.gz': 'sha256:' + 'a' * 64}, + zenodo=ZENODO, + base_urls=[_closed_url()], + data_root=data_root, + extract='tar', + ) + + +def _populate(fetcher, names=None, corrupt=()): + """Write the dataset's files under the fetcher's target directory.""" + fetcher.target_dir.mkdir(parents=True, exist_ok=True) + for name in CONTENTS if names is None else names: + body = b'not the recorded contents\n' if name in corrupt else CONTENTS[name] + (fetcher.target_dir / name).write_bytes(body) + + +def _write_stamp(fetcher, members): + fetcher.target_dir.mkdir(parents=True, exist_ok=True) + (fetcher.target_dir / STAMP).write_text( + json.dumps({'schema': 1, 'extract': 'tar', 'record_id': RECID, 'members': members}) + ) + + +def test_a_complete_tree_is_reported_complete_and_verified(tmp_path): + """Every declared file present with matching contents reads as ok.""" + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher) + + result = check_dataset(fetcher, key='demo') + + assert result.complete + assert result.hashed, 'a plain dataset has digests, so it must not report presence only' + assert [f.name for f in result.files] == ['alpha.dat', 'beta.dat'] + assert {f.state for f in result.files} == {OK} + assert result.missing == () and result.mismatched == () + + +def test_an_empty_tree_reports_every_file_missing(tmp_path): + """Nothing on disk names every declared file rather than reporting nothing. + + The edge case that matters: a dataset with no files present must not come + back with an empty file list, which would satisfy ``complete`` and read as + a healthy tree. + """ + fetcher = _plain_fetcher(tmp_path) + + result = check_dataset(fetcher) + + assert not result.complete + assert len(result.files) == 2, 'an absent tree still reports one entry per declared file' + assert {f.name for f in result.missing} == {'alpha.dat', 'beta.dat'} + assert result.mismatched == (), 'an absent file is missing, never a checksum failure' + + +def test_corrupt_and_absent_are_reported_apart(tmp_path): + """A corrupt file and an absent one are distinct states, not one fault. + + They are distinguished because the remedies differ: an absent file may + never have been fetched, while a corrupt one was fetched and then damaged + or truncated, which points at the tree rather than at the download. + """ + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher, names=['alpha.dat'], corrupt=['alpha.dat']) + + result = check_dataset(fetcher) + states = {f.name: f.state for f in result.files} + + assert states == {'alpha.dat': MISMATCH, 'beta.dat': MISSING} + assert [f.name for f in result.mismatched] == ['alpha.dat'] + assert [f.name for f in result.missing] == ['beta.dat'] + assert not result.complete + + +def test_a_check_changes_nothing_on_disk(tmp_path): + """The tree is byte-identical afterwards, including a corrupt file. + + A diagnostic that repaired what it inspected would report a tree that no + longer exists, and would do it while another process may be reading. + """ + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher, corrupt=['beta.dat']) + before = {p.name: p.read_bytes() for p in sorted(fetcher.target_dir.iterdir())} + + check_dataset(fetcher) + + after = {p.name: p.read_bytes() for p in sorted(fetcher.target_dir.iterdir())} + assert after == before + assert after['beta.dat'] != CONTENTS['beta.dat'], ( + 'the corrupt file must still be corrupt, or this asserts nothing' + ) + + +def test_archive_members_are_present_not_verified(tmp_path): + """An extracted tree is reported by presence, and says so. + + The registry pins the archive's checksum, not the checksums of what came + out of it, so there is nothing to hash the members against. Reporting them + as verified would overstate what was checked. + """ + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, ['inner/one.dat', 'inner/two.dat']) + for member in ('inner/one.dat', 'inner/two.dat'): + path = fetcher.target_dir / member + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'x') + + result = check_dataset(fetcher) + + assert result.complete + assert not result.hashed, 'an archive dataset cannot claim its members were verified' + assert {f.state for f in result.files} == {PRESENT} + assert 'presence only' in result.summary() + + +def test_a_deleted_archive_member_is_reported_missing(tmp_path): + """A member removed after extraction is named, not silently tolerated.""" + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, ['inner/one.dat', 'inner/two.dat']) + kept = fetcher.target_dir / 'inner/one.dat' + kept.parent.mkdir(parents=True, exist_ok=True) + kept.write_bytes(b'x') + + result = check_dataset(fetcher) + + assert not result.complete + assert [f.name for f in result.missing] == ['inner/two.dat'] + assert [f.name for f in result.files if f.state == PRESENT] == ['inner/one.dat'] + + +@pytest.mark.parametrize( + 'stamp_body', + ['', 'not json at all', json.dumps({'schema': 1}), json.dumps({'extract': 'zip'})], + ids=['empty', 'unparseable', 'no-members', 'wrong-kind'], +) +def test_an_archive_without_a_usable_stamp_is_not_complete(tmp_path, stamp_body): + """No usable stamp means no tree, reported as one missing item. + + The trap this guards is reporting zero files, which would make + ``complete`` true and hand back a clean bill of health for a dataset that + was never extracted. + """ + fetcher = _archive_fetcher(tmp_path) + fetcher.target_dir.mkdir(parents=True, exist_ok=True) + (fetcher.target_dir / STAMP).write_text(stamp_body) + + result = check_dataset(fetcher) + + assert result.files, 'an unusable stamp must not produce an empty, complete report' + assert not result.complete + assert [f.name for f in result.missing] == ['bundle.tar.gz'] + + +def test_an_unreadable_manifest_fails_the_report(tmp_path): + """A provider that failed to load keeps the report from reading clean. + + Its datasets were never inspected, so a report that ignored it would be + indistinguishable from one where everything was checked and passed. The + dataset here is deliberately complete, so only the manifest error can be + what fails it. + """ + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher) + healthy = check_dataset(fetcher, key='demo') + assert healthy.complete, 'the dataset must be sound, or this proves nothing' + + clean = CheckReport(datasets={'demo': healthy}, manifest_errors={}) + broken = CheckReport(datasets={'demo': healthy}, manifest_errors={'other': 'no such file'}) + + assert clean.ok + assert not broken.ok + assert 'MANIFEST UNREADABLE' in broken.summary() + assert broken.faults == (), 'the datasets are sound; the fault is the unread manifest' + + +def test_the_summary_names_the_worst_dataset_first(tmp_path): + """Datasets with more faults are listed ahead of those with fewer.""" + one_fault = DatasetCheck('one', SUBDIR, tmp_path, (_file('a', MISSING),)) + two_faults = DatasetCheck('two', SUBDIR, tmp_path, (_file('a', MISSING), _file('b', MISMATCH))) + report = CheckReport(datasets={'one': one_fault, 'two': two_faults}, manifest_errors={}) + + assert not report.ok + assert [d.key for d in report.faults] == ['two', 'one'] + assert 'data check FAILED' in report.summary() + assert '1 missing, 1 corrupt' in two_faults.summary() + + +def _file(name, state): + from pathlib import Path + + from fwl_io.check import FileCheck + + return FileCheck(name, Path(name), state) diff --git a/tests/test_cli.py b/tests/test_cli.py index 14e5d77..56e6699 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -99,3 +99,52 @@ def load(self): err = capsys.readouterr().err assert code == 1 assert 'g.demo' in err and 'Traceback' not in err + + +@pytest.mark.unit +def test_check_unknown_module_exits_nonzero(capsys, monkeypatch): + """Asking about a model no manifest declares is an error, not a clean tree.""" + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: []) + code = main(['check', 'nomodule']) + assert code == 1 + err = capsys.readouterr() + assert 'no datasets' in err.err + assert 'all data present' not in err.out, 'nothing was checked, so nothing may be declared ok' + + +@pytest.mark.unit +def test_check_reports_missing_data_and_exits_nonzero(tmp_path, capsys, monkeypatch): + """Absent data exits 1 and names the dataset, without downloading it.""" + import hashlib + + manifest = tmp_path / 'manifest.toml' + # The dataset location comes from the table key, so this one lands under + # "g/demo"; a manifest does not name its own subdirectory. + manifest.write_text('[g.demo]\nzenodo = "10.5281/zenodo.1234567"\nrequired_by = ["demo"]\n') + registry = tmp_path / 'g.demo.registry.txt' + digest = hashlib.sha256(b'contents\n').hexdigest() + registry.write_text(f'alpha.dat sha256:{digest}\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + data_root = tmp_path / 'data' + code = main(['check', 'demo', '--data-root', str(data_root)]) + out = capsys.readouterr().out + + assert code == 1 + assert 'FAILED' in out + assert 'g.demo' in out + # The dataset has to be reported as data that is absent, not as a manifest + # this could not read. Both exit 1 and both name the dataset, so without + # this the test would pass just as well against a misplaced registry file + # and would be proving nothing about the check itself. + assert '1 missing' in out + assert 'MANIFEST UNREADABLE' not in out + # Resolving a path creates the data root, as it does for every entry point. + # What a check must not do is populate it: no dataset directory, no file. + assert list(data_root.iterdir()) == [], 'a check must not create the tree it inspects' From 299947a7c1289253ead2665a1ac9632506305622 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sat, 1 Aug 2026 23:07:21 +0200 Subject: [PATCH 02/16] Document the check mode Adds the subcommand to the CLI reference with its four file states and what each one means, and says on the design page why a read-only mode exists beside the offline and online ones: neither of those can report the state of a whole tree, because one stops at the first fault and the other repairs it. --- docs/Explanations/design.md | 2 ++ docs/Reference/cli.md | 14 +++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/Explanations/design.md b/docs/Explanations/design.md index 87b8d99..da309b5 100644 --- a/docs/Explanations/design.md +++ b/docs/Explanations/design.md @@ -38,6 +38,8 @@ The data root comes from an explicit path or the `FWL_DATA` environment variable *Why*: production runs happen on compute nodes without internet. Anything that only works online is not usable for the campaigns this ecosystem runs. +Beside those two there is a third, read-only mode: `fwl-io check` reports whether a tree is complete and matches its registries without downloading anything or writing to it. Offline mode stops at the first thing it cannot serve and online mode repairs what it finds, so neither can answer what a diagnostic asks, which is the state of the whole tree at once. A check reports what it could not verify as well as what it could, so a dataset whose contents nothing pins and a manifest that failed to load both count against the verdict rather than passing quietly. + ## Atomic placement, distinct failure classes Every write is staged on the destination filesystem and moved into place with an atomic rename. Network or checksum failures raise a download error listing every mirror attempt; local problems (read-only tree, full disk) raise their own OSError. diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index 2d2c824..b654c11 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -1,6 +1,6 @@ # CLI reference -The `fwl-io` command has four subcommands. Failures are reported as concise messages on stderr (never a traceback) and exit with status 1; success exits 0. `sync` and `fetch` aggregate per-dataset failures into a multi-line report, and a download failure lists every mirror attempt. +The `fwl-io` command has five subcommands. Failures are reported as concise messages on stderr (never a traceback) and exit with status 1; success exits 0. `sync` and `fetch` aggregate per-dataset failures into a multi-line report, and a download failure lists every mirror attempt. ## fwl-io sync @@ -26,6 +26,18 @@ fwl-io fetch [--data-root PATH] Fetches every dataset that lists `` in its `required_by`. All datasets are attempted; failures are aggregated into one report. `--data-root` overrides the `FWL_DATA` tree. +## fwl-io check + +```bash +fwl-io check [--data-root PATH] +``` + +Reports whether every dataset that lists `` in its `required_by` is present and matches its registry, without downloading anything. Each file is reported in one of four states: `ok` (present, checksum matches), `missing`, `mismatch` (present, contents differ), or `present`. The last means the file is there and nothing was available to verify it against, which is the case for the members of an archive dataset: the registry pins the checksum of the archive, not of the files extracted from it, so such a dataset is reported `presence only`. + +A manifest that fails to load is reported alongside the datasets and is on its own enough to fail the check, because its datasets were never inspected. The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing file, any checksum mismatch, any unreadable manifest, or a model no manifest declares. + +Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. The equivalent Python entry point is `fwl_io.check_for`. + ## fwl-io mirror ```bash From e45f4abefeb47f436be2d5059017839c7de6bfa2 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sat, 1 Aug 2026 23:22:42 +0200 Subject: [PATCH 03/16] Report what a check could not establish, not just what it found A diagnostic that overstates its own coverage is worse than none, and the first cut of the check mode did so in several places. Two kinds of failure were filed together and printed under one label, so a dataset whose registry has simply never been generated was reported as an unreadable manifest, telling the user to fix the wrong thing. They are now separate maps with separate labels, and the message for an unresolvable dataset names `fwl-io sync` as the remedy. An empty report claimed to be ok. Asking about a model nothing declares, and being told nothing is wrong, is indistinguishable from a clean tree to anyone reading a boolean. A report with no datasets in it is now never ok, which is what the command already did and the library did not. A file that could not be read raised out of the whole report, so one permission problem hid every other dataset. Unreadable is now a state of its own and counts as a fault: its contents are unknown, which is not the same as correct. The stamp reading that decides which members an extracted tree should have existed twice, in the fetcher and again in the checker, and the two had already drifted apart: the checker accepted a stamp left by a fetch of a different record, and joined member names onto the tree without checking they stayed inside it. Both now go through one method on the fetcher, which requires the stamp to describe this record and drops any name resolving outside the dataset directory. A stamp is an ordinary file and can be edited, so a name in it deserves the same suspicion as a name inside an archive. Also corrects the design page, which said a presence-only dataset counts against the verdict. It does not and should not: presence is all the archive-only checksum policy makes checkable, so the report says so plainly rather than failing every archive dataset forever. --- docs/Explanations/design.md | 4 +- docs/Reference/cli.md | 8 +- src/fwl_io/check.py | 187 ++++++++++++++++-------------- src/fwl_io/cli.py | 4 +- src/fwl_io/fetch.py | 70 +++++++++--- tests/test_check.py | 220 +++++++++++++++++++++++++++++++++++- 6 files changed, 390 insertions(+), 103 deletions(-) diff --git a/docs/Explanations/design.md b/docs/Explanations/design.md index da309b5..f09626b 100644 --- a/docs/Explanations/design.md +++ b/docs/Explanations/design.md @@ -38,7 +38,9 @@ The data root comes from an explicit path or the `FWL_DATA` environment variable *Why*: production runs happen on compute nodes without internet. Anything that only works online is not usable for the campaigns this ecosystem runs. -Beside those two there is a third, read-only mode: `fwl-io check` reports whether a tree is complete and matches its registries without downloading anything or writing to it. Offline mode stops at the first thing it cannot serve and online mode repairs what it finds, so neither can answer what a diagnostic asks, which is the state of the whole tree at once. A check reports what it could not verify as well as what it could, so a dataset whose contents nothing pins and a manifest that failed to load both count against the verdict rather than passing quietly. +Beside those two there is a third, read-only mode: `fwl-io check` reports whether a tree is complete and matches its registries without downloading anything or writing to it. Offline mode stops at the first thing it cannot serve and online mode repairs what it finds, so neither can answer what a diagnostic asks, which is the state of the whole tree at once. + +*Why the report says what it could not establish*: anything that stopped a file being looked at counts against the verdict, so a manifest that failed to load, a dataset whose registry has never been generated, a file that cannot be read, and a model no manifest declares are all faults rather than silence. A dataset whose members carry no digests to check against is reported as `presence only`; that is not a fault, because presence is all the archive-only checksum policy makes checkable, but it is said plainly rather than reported as verification. ## Atomic placement, distinct failure classes diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index b654c11..80f3ab0 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -32,11 +32,13 @@ Fetches every dataset that lists `` in its `required_by`. All datasets ar fwl-io check [--data-root PATH] ``` -Reports whether every dataset that lists `` in its `required_by` is present and matches its registry, without downloading anything. Each file is reported in one of four states: `ok` (present, checksum matches), `missing`, `mismatch` (present, contents differ), or `present`. The last means the file is there and nothing was available to verify it against, which is the case for the members of an archive dataset: the registry pins the checksum of the archive, not of the files extracted from it, so such a dataset is reported `presence only`. +Reports whether every dataset that lists `` in its `required_by` is present and matches its registry, without downloading anything. Each file is reported in one of five states: `ok` (present, checksum matches), `missing`, `mismatch` (present, contents differ), `unreadable` (present, could not be read to be checked), or `present`. The last means the file is there and nothing was available to verify it against, which is the case for the members of an archive dataset: the registry pins the checksum of the archive, not of the files extracted from it, so such a dataset is reported `presence only`. That is not counted as a fault, since presence is all that is checkable there, but it is never reported as verification. -A manifest that fails to load is reported alongside the datasets and is on its own enough to fail the check, because its datasets were never inspected. The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing file, any checksum mismatch, any unreadable manifest, or a model no manifest declares. +Two kinds of failure are reported apart from the datasets, because they call for different repairs. `MANIFEST UNREADABLE` means an installed package's manifest could not be read at all, so nothing it declares was inspected. `NOT CHECKED` means the manifest was fine but one dataset could not be resolved, most often because its registry has not been generated yet; run `fwl-io sync` for it. Either is on its own enough to fail the check. -Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. The equivalent Python entry point is `fwl_io.check_for`. +The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing, corrupt or unreadable file, any unreadable manifest, any unresolvable dataset, or a model no manifest declares. + +Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. Resolving the data root creates that root if it does not exist, as it does for every other subcommand. The equivalent Python entry point is `fwl_io.check_for`, whose `CheckReport.ok` is false when nothing was checked, so a model that matches no dataset can never read as a clean tree. ## fwl-io mirror diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py index fb5040b..2ced61c 100644 --- a/src/fwl_io/check.py +++ b/src/fwl_io/check.py @@ -9,27 +9,28 @@ A check therefore never reaches the network, never downloads, and never creates or repairs a dataset. It reads the manifest, hashes what is on disk, -and returns a report. The one mark it leaves is the data root itself, which -resolving a path creates when it is absent, exactly as every other entry point -does; no dataset directory and no file is written. - -Two things the report is careful about, because a diagnostic that overstates -what it verified is worse than no diagnostic at all. A manifest that fails to -load is carried in the report rather than dropped, so a tree cannot read as -complete when whole datasets were never looked at. And an archive dataset's -members are reported as present rather than as verified: the archive-only -checksum policy records member names, not per-file digests, so there is -nothing to hash them against once the archive itself is gone. +and returns a report. Resolving a path creates the data root when it is +absent, exactly as every other entry point does; no dataset directory and no +file is written. + +The report is careful about what it did not establish, because a diagnostic +that overstates its own coverage is worse than none. A manifest that fails to +load and a dataset whose registry cannot be read are both carried in the +report and both count against the verdict, since their files were never +looked at, and they are carried apart from each other because they call for +different repairs. An archive dataset's members are reported as present +rather than as verified: the archive-only checksum policy records member +names, not per-file digests, so once the archive is gone there is nothing to +hash them against. """ from __future__ import annotations -import json import logging -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path -from fwl_io.fetch import _STAMP_FILENAME, Fetcher, _hash_matches, create_fetcher +from fwl_io.fetch import Fetcher, create_fetcher log = logging.getLogger('fwl.' + __name__) @@ -40,10 +41,13 @@ OK = 'ok' MISSING = 'missing' MISMATCH = 'mismatch' +UNREADABLE = 'unreadable' PRESENT = 'present' -#: States that mean the tree is not usable as the manifest describes it. -FAULT_STATES = (MISSING, MISMATCH) +#: States that mean the tree is not usable as the manifest describes it. A file +#: that cannot be read counts: whether its contents are right is unknown, and a +#: check reports what it could not establish rather than assuming the best. +FAULT_STATES = (MISSING, MISMATCH, UNREADABLE) @dataclass(frozen=True) @@ -56,7 +60,7 @@ class FileCheck: @property def faulty(self) -> bool: - """True when this file is absent or does not match its checksum.""" + """True when this file is absent, corrupt, or could not be read.""" return self.state in FAULT_STATES @@ -69,20 +73,33 @@ class DatasetCheck: directory: Path files: tuple[FileCheck, ...] + def _in_state(self, state: str) -> tuple[FileCheck, ...]: + return tuple(f for f in self.files if f.state == state) + @property def missing(self) -> tuple[FileCheck, ...]: """Files the manifest declares that are not on disk.""" - return tuple(f for f in self.files if f.state == MISSING) + return self._in_state(MISSING) @property def mismatched(self) -> tuple[FileCheck, ...]: """Files on disk whose contents differ from the registry.""" - return tuple(f for f in self.files if f.state == MISMATCH) + return self._in_state(MISMATCH) + + @property + def unreadable(self) -> tuple[FileCheck, ...]: + """Files on disk that could not be read to be checked.""" + return self._in_state(UNREADABLE) + + @property + def faults(self) -> tuple[FileCheck, ...]: + """Every file that is not in a usable state.""" + return tuple(f for f in self.files if f.faulty) @property def complete(self) -> bool: - """True when nothing is missing and nothing fails its checksum.""" - return not any(f.faulty for f in self.files) + """True when nothing is missing, corrupt, or unreadable.""" + return not self.faults @property def hashed(self) -> bool: @@ -91,16 +108,18 @@ def hashed(self) -> bool: False for an archive dataset, whose members are recorded by name only, so a truncated member is indistinguishable from an intact one here. """ - return not any(f.state == PRESENT for f in self.files) + return not self._in_state(PRESENT) def summary(self) -> str: """One line naming the counts, for a report a person reads.""" - total = len(self.files) - parts = [f'{total} file(s)'] - if self.missing: - parts.append(f'{len(self.missing)} missing') - if self.mismatched: - parts.append(f'{len(self.mismatched)} corrupt') + parts = [f'{len(self.files)} file(s)'] + for label, group in ( + ('missing', self.missing), + ('corrupt', self.mismatched), + ('unreadable', self.unreadable), + ): + if group: + parts.append(f'{len(group)} {label}') if not self.hashed: parts.append('presence only') state = 'ok' if self.complete else 'FAILED' @@ -109,57 +128,66 @@ def summary(self) -> str: @dataclass(frozen=True) class CheckReport: - """Every dataset checked, and every manifest that could not be read.""" + """Every dataset checked, and everything that stopped one being checked. + + The two error maps are kept apart because they call for different repairs. + A manifest error means an installed package's manifest could not be read at + all, so nothing it declares was inspected. A dataset error means the + manifest was fine but that one dataset could not be resolved, most often + because its registry has never been generated. + """ - datasets: dict[str, DatasetCheck] - manifest_errors: dict[str, str] + datasets: dict[str, DatasetCheck] = field(default_factory=dict) + manifest_errors: dict[str, str] = field(default_factory=dict) + dataset_errors: dict[str, str] = field(default_factory=dict) @property def ok(self) -> bool: - """True only when every dataset is complete and every manifest loaded. + """True only when something was checked and all of it was sound. - A manifest that failed to load counts against the report. Its datasets - were never inspected, so treating it as harmless would let a tree with - an unreadable provider report exactly like a healthy one. + An empty report is not ok. A caller asking about a model and being told + nothing is wrong, when in truth nothing was looked at, is the failure + this whole module exists to avoid; the two are indistinguishable to + anyone reading a boolean. """ - return not self.manifest_errors and all(d.complete for d in self.datasets.values()) + if not self.datasets: + return False + if self.manifest_errors or self.dataset_errors: + return False + return all(d.complete for d in self.datasets.values()) @property def faults(self) -> tuple[DatasetCheck, ...]: - """Datasets with something missing or corrupt, worst named first.""" + """Datasets with something wrong, the worst affected named first.""" broken = [d for d in self.datasets.values() if not d.complete] - return tuple(sorted(broken, key=lambda d: (-len(d.missing) - len(d.mismatched), d.key))) + return tuple(sorted(broken, key=lambda d: (-len(d.faults), d.key))) def summary(self) -> str: """A short human-readable report, one line per dataset plus a verdict.""" lines = [d.summary() for d in sorted(self.datasets.values(), key=lambda d: d.key)] for provider, error in sorted(self.manifest_errors.items()): lines.append(f'{provider}: MANIFEST UNREADABLE, {error}') + for key, error in sorted(self.dataset_errors.items()): + lines.append(f'{key}: NOT CHECKED, {error}') if not lines: - return 'no datasets checked' + return 'nothing was checked' lines.append('all data present and verified' if self.ok else 'data check FAILED') return '\n'.join(lines) -def _archive_members(fetcher: Fetcher) -> list[str] | None: - """Return the member names an archive dataset's stamp recorded. - - ``None`` when there is no usable stamp, which means the extracted tree is - not there to be checked rather than that it is empty. - """ - stamp = fetcher.target_dir / _STAMP_FILENAME +def _file_state(fetcher: Fetcher, name: str) -> str: + """Classify one registry file: present and correct, absent, or otherwise.""" + path = fetcher.target_dir / name try: - record = json.loads(stamp.read_text()) - except (OSError, ValueError): - return None - if record.get('extract') != fetcher.extract: - # The stamp describes a different fetch of this deposit, so its member - # list does not describe the tree this dataset expects. - return None - members = record.get('members') - if not isinstance(members, list) or not members: - return None - return [str(m) for m in members] + if not path.is_file(): + return MISSING + return OK if fetcher.file_matches(name) else MISMATCH + except OSError as exc: + # A file the checker cannot read is not evidence of a good tree. This + # is reported rather than raised so one unreadable file cannot abort a + # report covering every other dataset. + log.warning('cannot read %s: %s', path, exc) + return UNREADABLE def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: @@ -183,7 +211,7 @@ def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: key = key or fetcher.subdir if fetcher.extract is not None: - members = _archive_members(fetcher) + members = fetcher.recorded_members() if members is None: # No usable stamp means no extracted tree to speak of. The dataset # is reported as one missing item under the archive's own name, @@ -197,16 +225,10 @@ def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: checks.append(FileCheck(name, path, PRESENT if path.is_file() else MISSING)) return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) - checks = [] - for name, known_hash in sorted(fetcher.registry.items()): - path = fetcher.target_dir / name - if not path.is_file(): - state = MISSING - elif _hash_matches(path, known_hash): - state = OK - else: - state = MISMATCH - checks.append(FileCheck(name, path, state)) + checks = [ + FileCheck(name, fetcher.target_dir / name, _file_state(fetcher, name)) + for name in sorted(fetcher.registry) + ] return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) @@ -214,14 +236,11 @@ def check_for(model: str, data_root: str | Path | None = None) -> CheckReport: """Report the state of every dataset a given model requires. Nothing is downloaded and no dataset directory or file is written, so this - is safe to run against a tree another process is reading. Resolving the - data root creates that root when it is absent, which is the only mark a - check leaves. + is safe to run against a tree another process is reading. - A dataset whose registry or fetcher cannot be built is reported as a - manifest error rather than skipped, for the same reason an unreadable - manifest is: the alternative is a report that looks clean because it - checked less than it appears to. + Nothing here raises for the state of the data or of a manifest. A caller + running a check wants the whole picture, including the parts that could not + be established, so every failure is carried in the report instead. Parameters ---------- @@ -233,14 +252,15 @@ def check_for(model: str, data_root: str | Path | None = None) -> CheckReport: Returns ------- CheckReport - Keyed by dataset, alongside every manifest that could not be read. + Keyed by dataset, alongside the manifests that could not be read and + the datasets that could not be resolved. """ from fwl_io.manifest import _discover model = model.lower() datasets: dict[str, DatasetCheck] = {} - providers, errors = _discover() - manifest_errors = dict(errors) + dataset_errors: dict[str, str] = {} + providers, manifest_errors = _discover() for provider_datasets in providers.values(): for ds in provider_datasets: if model not in tuple(r.lower() for r in ds.required_by): @@ -254,9 +274,12 @@ def check_for(model: str, data_root: str | Path | None = None) -> CheckReport: data_root=data_root, extract=ds.extract, ) + datasets[ds.key] = check_dataset(fetcher, key=ds.key) except Exception as exc: # noqa: BLE001 -- reported, never raised - manifest_errors[ds.key] = str(exc) + dataset_errors[ds.key] = str(exc) log.warning('cannot check dataset %r: %s', ds.key, exc) - continue - datasets[ds.key] = check_dataset(fetcher, key=ds.key) - return CheckReport(datasets=datasets, manifest_errors=manifest_errors) + return CheckReport( + datasets=datasets, + manifest_errors=dict(manifest_errors), + dataset_errors=dataset_errors, + ) diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index 7eb706b..d18f2de 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -1,4 +1,4 @@ -"""Command-line interface: ``fwl-io sync | list | fetch | mirror``. +"""Command-line interface: ``fwl-io sync | list | fetch | check | mirror``. Failures from the package's own error types exit with status 1 and a one-line message on stderr instead of a traceback. @@ -54,7 +54,7 @@ def _cmd_check(args: argparse.Namespace) -> int: from fwl_io.check import check_for report = check_for(args.model, data_root=args.data_root) - if not report.datasets and not report.manifest_errors: + if not (report.datasets or report.manifest_errors or report.dataset_errors): print(f'no datasets declare required_by = {args.model!r}', file=sys.stderr) return 1 # The summary goes to stdout whatever the verdict: a caller running this to diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index e644d0d..365217d 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -493,26 +493,70 @@ def _place_tree(self, src_dir: Path, target_dir: Path) -> None: target_dir.unlink() os.replace(src_dir, target_dir) - def _archive_tree_intact(self, stamp: Path) -> bool: - """True when every member the stamp recorded is still present on disk. + def file_matches(self, fname: str) -> bool: + """True when the local file for ``fname`` matches its registry digest. + + Reads the file; it does not fetch, and it does not check that the file + exists first, so a caller wanting to tell an absent file from a corrupt + one tests for presence itself. An unreadable file raises ``OSError`` + rather than reporting a mismatch, since a permission problem on the + tree is a different fault from wrong contents. + """ + if fname not in self.registry: + raise KeyError(f'{fname!r} is not in the registry for {self.subdir!r}') + return _hash_matches(self.target_dir / fname, self.registry[fname]) - This detects a member deleted after extraction, so the tree is - re-fetched rather than served incomplete. It does not re-hash contents: - the archive-only checksum policy records member names, not per-file - digests, so a truncated member is not detected here. + def recorded_members(self) -> list[str] | None: + """Return the extracted members this dataset's stamp records. + + ``None`` when there is no stamp describing this dataset's tree, which + says the tree is not there to be examined rather than that it is empty. + Callers must keep those apart: an empty list would read as a complete + tree of no files. + + A stamp qualifies only when it describes this archive kind and this + record id. One left by a different fetch of the same subdirectory, a + plain fetch or another version, does not describe this tree. + + Members that would resolve outside the dataset directory are dropped. + A stamp is a file on disk like any other and can be edited or replaced, + so a name in it is treated with the same suspicion as a name inside an + archive rather than joined onto the tree unchecked. """ + stamp = self.target_dir / _STAMP_FILENAME try: record = json.loads(stamp.read_text()) except (OSError, ValueError): - return False + return None if record.get('extract') != self.extract: - # The stamp describes a different fetch of this deposit, a plain - # one or a different archive kind, so its tree is not this dataset. - return False + return None + if record.get('record_id') != self.record_id: + return None members = record.get('members') if not isinstance(members, list) or not members: - # An absent or empty member list describes no tree at all, and must - # never read as a complete one. + return None + root = self.target_dir.resolve() + safe = [] + for member in members: + if not isinstance(member, str): + continue + resolved = (self.target_dir / member).resolve() + if resolved == root or not resolved.is_relative_to(root): + log.warning('stamp for %s names a member outside it: %r', self.subdir, member) + continue + safe.append(member) + return safe or None + + def _archive_tree_intact(self) -> bool: + """True when every member the stamp recorded is still present on disk. + + This detects a member deleted after extraction, so the tree is + re-fetched rather than served incomplete. It does not re-hash contents: + the archive-only checksum policy records member names, not per-file + digests, so a truncated member is not detected here. + """ + members = self.recorded_members() + if members is None: return False return all((self.target_dir / m).is_file() for m in members) @@ -528,7 +572,7 @@ def _fetch_archive(self, offline: bool | None = None) -> list[Path]: """ archive_name, known_hash = next(iter(self.registry.items())) stamp = self.target_dir / _STAMP_FILENAME - if self._stamp_is_current(stamp) and self._archive_tree_intact(stamp): + if self._stamp_is_current(stamp) and self._archive_tree_intact(): self._sources.setdefault(archive_name, 'local') return self._extracted_files() diff --git a/tests/test_check.py b/tests/test_check.py index 0f21f0f..2c53c98 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -24,9 +24,11 @@ MISSING, OK, PRESENT, + UNREADABLE, CheckReport, DatasetCheck, check_dataset, + check_for, ) from fwl_io.fetch import create_fetcher @@ -83,10 +85,10 @@ def _populate(fetcher, names=None, corrupt=()): (fetcher.target_dir / name).write_bytes(body) -def _write_stamp(fetcher, members): +def _write_stamp(fetcher, members, record_id=RECID): fetcher.target_dir.mkdir(parents=True, exist_ok=True) (fetcher.target_dir / STAMP).write_text( - json.dumps({'schema': 1, 'extract': 'tar', 'record_id': RECID, 'members': members}) + json.dumps({'schema': 1, 'extract': 'tar', 'record_id': record_id, 'members': members}) ) @@ -259,3 +261,217 @@ def _file(name, state): from fwl_io.check import FileCheck return FileCheck(name, Path(name), state) + + +def test_an_unreadable_file_is_a_fault_not_a_pass(tmp_path): + """A file that cannot be read is reported, not assumed to be fine. + + Its contents are unknown, which is not the same as correct, and the + diagnostic must not resolve that doubt in the tree's favour. + """ + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher) + unreadable = fetcher.target_dir / 'alpha.dat' + unreadable.chmod(0o000) + try: + result = check_dataset(fetcher) + finally: + unreadable.chmod(0o644) + + states = {f.name: f.state for f in result.files} + assert states['alpha.dat'] == UNREADABLE + assert states['beta.dat'] == OK, 'one unreadable file must not spoil the others' + assert not result.complete + assert [f.name for f in result.unreadable] == ['alpha.dat'] + + +# --------------------------------------------------------------------------- +# check_for, the entry point a diagnostic calls +# --------------------------------------------------------------------------- + + +def _install_manifest(monkeypatch, manifest_path, name='demoprovider'): + class _EP: + def __init__(self): + self.name = name + + def load(self): + return lambda: manifest_path + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + +def _demo_manifest(tmp_path, *, with_registry=True, required_by='"demo"'): + manifest = tmp_path / 'manifest.toml' + manifest.write_text( + f'[g.demo]\nzenodo = "10.5281/zenodo.1234567"\nrequired_by = [{required_by}]\n' + ) + if with_registry: + digests = _registry() + lines = ''.join(f'{n} {d}\n' for n, d in sorted(digests.items())) + (tmp_path / 'g.demo.registry.txt').write_text(lines) + return manifest + + +def test_check_for_reports_a_complete_tree_through_the_real_path(tmp_path, monkeypatch): + """A populated tree read through the manifest comes back ok. + + This goes through discovery, the registry file and the fetcher, unlike the + tests above which hand a fetcher straight to check_dataset. + """ + _install_manifest(monkeypatch, _demo_manifest(tmp_path)) + data_root = tmp_path / 'data' + target = data_root / 'g' / 'demo' / 'r1234567' + target.mkdir(parents=True) + for name, body in CONTENTS.items(): + (target / name).write_bytes(body) + + report = check_for('demo', data_root=data_root) + + assert report.ok + assert list(report.datasets) == ['g.demo'] + assert report.datasets['g.demo'].complete + assert report.manifest_errors == {} and report.dataset_errors == {} + + +def test_check_for_matches_the_model_case_insensitively(tmp_path, monkeypatch): + """``required_by`` matching ignores case, and a different model matches nothing.""" + _install_manifest(monkeypatch, _demo_manifest(tmp_path, required_by='"Demo"')) + data_root = tmp_path / 'data' + + matched = check_for('dEmO', data_root=data_root) + unmatched = check_for('othermodel', data_root=data_root) + + assert list(matched.datasets) == ['g.demo'] + assert unmatched.datasets == {} + assert not unmatched.ok, 'a model that matches nothing must never read as a clean tree' + + +def test_check_for_carries_a_manifest_failure_into_the_report(tmp_path, monkeypatch): + """An unreadable manifest fails the report through the real discovery path. + + The report is otherwise empty, so nothing else can be what fails it. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text('this is not valid toml [[[\n') + _install_manifest(monkeypatch, manifest) + + report = check_for('demo', data_root=tmp_path / 'data') + + assert not report.ok + assert list(report.manifest_errors) == ['demoprovider'] + assert report.dataset_errors == {} + assert 'MANIFEST UNREADABLE' in report.summary() + + +def test_check_for_separates_a_missing_registry_from_a_bad_manifest(tmp_path, monkeypatch): + """A dataset with no registry is reported as unchecked, not as a bad manifest. + + They tell the user to fix different things: one is a broken file, the other + is a registry that has simply never been generated. + """ + _install_manifest(monkeypatch, _demo_manifest(tmp_path, with_registry=False)) + + report = check_for('demo', data_root=tmp_path / 'data') + summary = report.summary() + + assert not report.ok + assert list(report.dataset_errors) == ['g.demo'] + assert report.manifest_errors == {} + assert 'NOT CHECKED' in summary + assert 'MANIFEST UNREADABLE' not in summary + assert 'fwl-io sync' in summary, 'the message has to name the remedy' + + +def test_check_for_reports_missing_files_without_creating_them(tmp_path, monkeypatch): + """Absent data is reported per file, and the check populates nothing.""" + _install_manifest(monkeypatch, _demo_manifest(tmp_path)) + data_root = tmp_path / 'data' + + report = check_for('demo', data_root=data_root) + + assert not report.ok + dataset = report.datasets['g.demo'] + assert {f.name for f in dataset.missing} == set(CONTENTS) + assert not (data_root / 'g').exists(), 'a check must not create the dataset directory' + + +def test_an_empty_report_is_not_ok(): + """Nothing checked is not the same as nothing wrong.""" + assert not CheckReport().ok + assert CheckReport().summary() == 'nothing was checked' + + +def test_a_stamp_from_another_record_does_not_describe_this_tree(tmp_path): + """A stamp naming a different deposit is not evidence about this one. + + The version directory is named for the record, so a stamp carrying another + record id was left by a different fetch and its member list says nothing + about what should be here. + """ + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, ['inner/one.dat'], record_id='99999999') + member = fetcher.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + + assert fetcher.recorded_members() is None + result = check_dataset(fetcher) + + assert not result.complete, 'a foreign stamp must not certify this tree' + assert [f.name for f in result.missing] == ['bundle.tar.gz'] + + +def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path): + """A member name pointing outside the dataset directory is dropped. + + A stamp is an ordinary file on disk and can be edited or replaced, so a + name inside it gets the same suspicion as a name inside an archive rather + than being joined onto the tree and reported on. + """ + fetcher = _archive_fetcher(tmp_path) + outside = tmp_path / 'outside.dat' + outside.write_bytes(b'not part of the dataset\n') + _write_stamp(fetcher, ['../../../outside.dat', 'inner/one.dat']) + member = fetcher.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + + members = fetcher.recorded_members() + + assert members == ['inner/one.dat'], 'the escaping name must not survive' + reported = {f.name for f in check_dataset(fetcher).files} + assert reported == {'inner/one.dat'} + assert outside.is_file(), 'the check reads only; it never touches what it refused' + + +def test_one_unresolvable_dataset_fails_a_report_of_sound_ones(tmp_path, monkeypatch): + """A dataset that could not be checked fails the report beside healthy ones. + + The discriminating case for the rule: with a complete dataset present, an + empty result cannot be what fails the report, so only the unchecked + dataset can be. Without this, a report holding nothing but errors already + fails for having checked nothing, and the rule would go untested. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text( + '[g.demo]\nzenodo = "10.5281/zenodo.1234567"\nrequired_by = ["demo"]\n\n' + '[g.other]\nzenodo = "10.5281/zenodo.7654321"\nrequired_by = ["demo"]\n' + ) + lines = ''.join(f'{n} {d}\n' for n, d in sorted(_registry().items())) + (tmp_path / 'g.demo.registry.txt').write_text(lines) + # g.other deliberately has no registry file, so it cannot be resolved. + _install_manifest(monkeypatch, manifest) + + data_root = tmp_path / 'data' + target = data_root / 'g' / 'demo' / 'r1234567' + target.mkdir(parents=True) + for name, body in CONTENTS.items(): + (target / name).write_bytes(body) + + report = check_for('demo', data_root=data_root) + + assert report.datasets['g.demo'].complete, 'the sound dataset must really be sound' + assert list(report.dataset_errors) == ['g.other'] + assert not report.ok, 'an unchecked dataset fails the report even when the rest is sound' + assert report.faults == (), 'no dataset is broken; the fault is the one never checked' From 3c5c0bdbb8b6185a582363a3ee33a16afafbdf90 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 06:27:35 +0200 Subject: [PATCH 04/16] Stop the check verdict claiming more than it checked A sound tree closed with "all data present and verified" even when a dataset line directly above it read "presence only". Nothing about those contents had been established, so the verdict now names them: "all data present, N dataset(s) by presence only". `CheckReport.verified` asks the same question programmatically. It is stricter than `ok`, which a presence-only dataset still satisfies, because presence is all the archive-only checksum policy makes checkable and failing every archive dataset forever is not the answer. Whether a dataset can be verified is now a property of the dataset rather than of what happened to survive on disk. Read off the file states, an archive whose members had all gone missing reported itself verifiable, since it had no present files left to say otherwise. An archive member the checker cannot reach is reported unreadable rather than aborting the dataset. The plain-file path already worked that way; the archive path let the error out, so one directory denying traversal turned every other member of that dataset into no information at all. The shared-cache stamp is read by the same method as the dataset's own, so both are held to one standard. The cache reader had grown its own qualifying rules and no containment check, which let an edited cache stamp name members outside the cached tree and still be trusted. Reading both through one method also means a stamp has to name this deposit, not only a record id parsed out of it. Alongside: the check reads and hashes every file the manifest declares, which the command's help text and the reference now say plainly, since a full pass over a multi-gigabyte tree is not what "without downloading" suggests; `fwl_io.check` gets a reference page like every other public module; and `file_matches` documents the KeyError it raises. --- docs/Reference/api/check.md | 3 + docs/Reference/api/index.md | 3 + docs/Reference/cli.md | 6 +- mkdocs.yml | 1 + src/fwl_io/check.py | 79 ++++++++++++++++----- src/fwl_io/cli.py | 3 +- src/fwl_io/fetch.py | 50 +++++++------ tests/test_check.py | 137 +++++++++++++++++++++++++++++++++--- tests/test_fetch.py | 37 ++++++++++ 9 files changed, 270 insertions(+), 49 deletions(-) create mode 100644 docs/Reference/api/check.md diff --git a/docs/Reference/api/check.md b/docs/Reference/api/check.md new file mode 100644 index 0000000..494e353 --- /dev/null +++ b/docs/Reference/api/check.md @@ -0,0 +1,3 @@ +# Check + +::: fwl_io.check diff --git a/docs/Reference/api/index.md b/docs/Reference/api/index.md index 5d63eec..aaf311d 100644 --- a/docs/Reference/api/index.md +++ b/docs/Reference/api/index.md @@ -7,6 +7,8 @@ from fwl_io import ( create_fetcher, Fetcher, # fetching load_manifest, discover_manifests, # manifests fetch_for, Dataset, + check_for, check_dataset, # validate-only checking + CheckReport, DatasetCheck, FileCheck, resolve_data_root, resolve_cache_root, DownloadError, OfflineDataError, MissingDataRootError, ManifestSchemaError, @@ -16,6 +18,7 @@ from fwl_io import ( Per-module reference pages: - [Fetching](fetch.md): `Fetcher`, `create_fetcher`, error types +- [Checking](check.md): `check_for`, `check_dataset`, `CheckReport`, `DatasetCheck`, `FileCheck` - [Manifests](manifest.md): `Dataset`, `load_manifest`, `discover_manifests`, `fetch_for`, `ManifestSchemaError` - [Registries](registry.md): registry file reading and writing - [Sync](sync.md): registry generation from the Zenodo API diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index 80f3ab0..28a8aa7 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -36,9 +36,11 @@ Reports whether every dataset that lists `` in its `required_by` is prese Two kinds of failure are reported apart from the datasets, because they call for different repairs. `MANIFEST UNREADABLE` means an installed package's manifest could not be read at all, so nothing it declares was inspected. `NOT CHECKED` means the manifest was fine but one dataset could not be resolved, most often because its registry has not been generated yet; run `fwl-io sync` for it. Either is on its own enough to fail the check. -The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing, corrupt or unreadable file, any unreadable manifest, any unresolvable dataset, or a model no manifest declares. +The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing, corrupt or unreadable file, any unreadable manifest, any unresolvable dataset, or a model no manifest declares. The closing line says `all data present and verified` only when every file was compared against a digest; a sound tree holding a presence-only dataset closes with `all data present, N dataset(s) by presence only` instead, and still exits 0. -Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. Resolving the data root creates that root if it does not exist, as it does for every other subcommand. The equivalent Python entry point is `fwl_io.check_for`, whose `CheckReport.ok` is false when nothing was checked, so a model that matches no dataset can never read as a clean tree. +Checking reads and hashes every file the manifest declares, so the cost is one full pass over the model's data. On a multi-gigabyte tree, or a shared cluster filesystem, expect it to take as long as reading that data once. + +Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. Resolving the data root creates that root if it does not exist, as it does for every other subcommand. The equivalent Python entry point is `fwl_io.check_for`, whose `CheckReport.ok` is false when nothing was checked, so a model that matches no dataset can never read as a clean tree. `CheckReport.verified` is the stricter question, false whenever any part of the tree was checked by presence alone. ## fwl-io mirror diff --git a/mkdocs.yml b/mkdocs.yml index 8ba2018..453ddaa 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -27,6 +27,7 @@ nav: - API reference: - Overview: Reference/api/index.md - Fetching: Reference/api/fetch.md + - Checking: Reference/api/check.md - Manifests: Reference/api/manifest.md - Registries: Reference/api/registry.md - Sync: Reference/api/sync.md diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py index 2ced61c..d16cb55 100644 --- a/src/fwl_io/check.py +++ b/src/fwl_io/check.py @@ -66,12 +66,20 @@ def faulty(self) -> bool: @dataclass(frozen=True) class DatasetCheck: - """The state of every file in one dataset.""" + """The state of every file in one dataset. + + ``verifiable`` says whether this dataset's registry carries a digest for + each file it declares. It is false for an archive dataset, whose members + are recorded by name only, and it is a property of the dataset rather than + of what happens to be on disk, so an archive dataset with no members left + cannot read as verifiable. + """ key: str subdir: str directory: Path files: tuple[FileCheck, ...] + verifiable: bool = True def _in_state(self, state: str) -> tuple[FileCheck, ...]: return tuple(f for f in self.files if f.state == state) @@ -101,15 +109,6 @@ def complete(self) -> bool: """True when nothing is missing, corrupt, or unreadable.""" return not self.faults - @property - def hashed(self) -> bool: - """True when every file present was checked against a known digest. - - False for an archive dataset, whose members are recorded by name only, - so a truncated member is indistinguishable from an intact one here. - """ - return not self._in_state(PRESENT) - def summary(self) -> str: """One line naming the counts, for a report a person reads.""" parts = [f'{len(self.files)} file(s)'] @@ -120,7 +119,7 @@ def summary(self) -> str: ): if group: parts.append(f'{len(group)} {label}') - if not self.hashed: + if not self.verifiable: parts.append('presence only') state = 'ok' if self.complete else 'FAILED' return f'{self.key}: {state}, ' + ', '.join(parts) @@ -156,6 +155,23 @@ def ok(self) -> bool: return False return all(d.complete for d in self.datasets.values()) + @property + def verified(self) -> bool: + """True when the tree is sound and every file in it was hashed. + + Stricter than ``ok``, which a presence-only dataset satisfies. Presence + is all the archive-only checksum policy makes checkable, so such a + dataset is not a fault; but a caller that needs to know the contents + were compared against a digest must ask this and not ``ok``. + """ + return self.ok and all(d.verifiable for d in self.datasets.values()) + + @property + def presence_only(self) -> tuple[DatasetCheck, ...]: + """Datasets whose files carry no digest to be checked against.""" + unhashed = [d for d in self.datasets.values() if not d.verifiable] + return tuple(sorted(unhashed, key=lambda d: d.key)) + @property def faults(self) -> tuple[DatasetCheck, ...]: """Datasets with something wrong, the worst affected named first.""" @@ -171,9 +187,20 @@ def summary(self) -> str: lines.append(f'{key}: NOT CHECKED, {error}') if not lines: return 'nothing was checked' - lines.append('all data present and verified' if self.ok else 'data check FAILED') + lines.append(self._verdict()) return '\n'.join(lines) + def _verdict(self) -> str: + """The closing line, which must not claim more than was established.""" + if not self.ok: + return 'data check FAILED' + if self.verified: + return 'all data present and verified' + # Sound, but part of it was checked by name alone. Saying "verified" + # here is the overstatement this module exists to avoid. + count = len(self.presence_only) + return f'all data present, {count} dataset(s) by presence only' + def _file_state(fetcher: Fetcher, name: str) -> str: """Classify one registry file: present and correct, absent, or otherwise.""" @@ -190,6 +217,20 @@ def _file_state(fetcher: Fetcher, name: str) -> str: return UNREADABLE +def _member_state(path: Path) -> str: + """Classify one extracted member, which has no digest to be checked against. + + Reporting rather than raising for the same reason ``_file_state`` does: a + member the checker cannot reach, most often because a directory above it + denies traversal, must cost that one entry and not the whole report. + """ + try: + return PRESENT if path.is_file() else MISSING + except OSError as exc: + log.warning('cannot read %s: %s', path, exc) + return UNREADABLE + + def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: """Report the state of one dataset's files without touching the network. @@ -218,12 +259,14 @@ def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: # rather than as zero items, which would read as complete. archive_name = next(iter(fetcher.registry)) files = (FileCheck(archive_name, fetcher.target_dir / archive_name, MISSING),) - return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, files) - checks = [] - for name in sorted(members): - path = fetcher.target_dir / name - checks.append(FileCheck(name, path, PRESENT if path.is_file() else MISSING)) - return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) + return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, files, verifiable=False) + checks = [ + FileCheck(name, fetcher.target_dir / name, _member_state(fetcher.target_dir / name)) + for name in sorted(members) + ] + return DatasetCheck( + key, fetcher.subdir, fetcher.target_dir, tuple(checks), verifiable=False + ) checks = [ FileCheck(name, fetcher.target_dir / name, _file_state(fetcher, name)) diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index d18f2de..95ec919 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -111,7 +111,8 @@ def main(argv: list[str] | None = None) -> int: p_fetch.set_defaults(func=_cmd_fetch) p_check = sub.add_parser( - 'check', help='report whether a model has its data, without downloading' + 'check', + help='report whether a model has its data, without downloading (hashes every file)', ) p_check.add_argument('model', help='model name matched against required_by') p_check.add_argument('--data-root', default=None, help='override the FWL_DATA root') diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index 365217d..56ea6ae 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -501,6 +501,14 @@ def file_matches(self, fname: str) -> bool: one tests for presence itself. An unreadable file raises ``OSError`` rather than reporting a mismatch, since a permission problem on the tree is a different fault from wrong contents. + + Raises + ------ + KeyError + ``fname`` is not declared in this dataset's registry, so there is + no digest to compare it against. + OSError + The file is present but could not be read. """ if fname not in self.registry: raise KeyError(f'{fname!r} is not in the registry for {self.subdir!r}') @@ -514,33 +522,44 @@ def recorded_members(self) -> list[str] | None: Callers must keep those apart: an empty list would read as a complete tree of no files. - A stamp qualifies only when it describes this archive kind and this - record id. One left by a different fetch of the same subdirectory, a - plain fetch or another version, does not describe this tree. + A stamp qualifies only when it describes this deposit and this archive + kind. One left by a different fetch of the same subdirectory, a plain + fetch or another version, does not describe this tree. Members that would resolve outside the dataset directory are dropped. A stamp is a file on disk like any other and can be edited or replaced, so a name in it is treated with the same suspicion as a name inside an archive rather than joined onto the tree unchecked. """ - stamp = self.target_dir / _STAMP_FILENAME + return self._stamp_members(self.target_dir) + + def _stamp_members(self, directory: Path) -> list[str] | None: + """Members recorded by the stamp in ``directory``, or ``None``. + + The single reader for both the dataset's own tree and a copy of it in + the shared cache. They are held to one standard on purpose: a cache + stamp is no more trustworthy than a local one, and two readers with + their own qualifying rules drift apart. + """ try: - record = json.loads(stamp.read_text()) + record = json.loads((directory / _STAMP_FILENAME).read_text()) except (OSError, ValueError): return None + if not isinstance(record, dict): + return None if record.get('extract') != self.extract: return None - if record.get('record_id') != self.record_id: + if record.get('record_id') != self.record_id or record.get('zenodo') != self.zenodo: return None members = record.get('members') if not isinstance(members, list) or not members: return None - root = self.target_dir.resolve() + root = directory.resolve() safe = [] for member in members: if not isinstance(member, str): continue - resolved = (self.target_dir / member).resolve() + resolved = (directory / member).resolve() if resolved == root or not resolved.is_relative_to(root): log.warning('stamp for %s names a member outside it: %r', self.subdir, member) continue @@ -634,23 +653,14 @@ def _copy_cached_tree(self) -> bool: archive, since the archive is dropped after extraction. The cached stamp has to describe this deposit and the same archive kind, and every member it names has to be present, which is the same standard - the local tree is held to. + the local tree is held to, read by the same method. """ cache_root = resolve_cache_root() if cache_root is None: return False cached_dir = cache_root / self.rel_dir - cached_stamp = cached_dir / _STAMP_FILENAME - try: - record = json.loads(cached_stamp.read_text()) - except (OSError, ValueError): - return False - if record.get('record_id') != self.record_id or record.get('zenodo') != self.zenodo: - return False - if record.get('extract') != self.extract: - return False - members = record.get('members') - if not isinstance(members, list) or not members: + members = self._stamp_members(cached_dir) + if members is None: return False if not all((cached_dir / m).is_file() for m in members): return False diff --git a/tests/test_check.py b/tests/test_check.py index 2c53c98..508e89c 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -85,10 +85,24 @@ def _populate(fetcher, names=None, corrupt=()): (fetcher.target_dir / name).write_bytes(body) -def _write_stamp(fetcher, members, record_id=RECID): - fetcher.target_dir.mkdir(parents=True, exist_ok=True) - (fetcher.target_dir / STAMP).write_text( - json.dumps({'schema': 1, 'extract': 'tar', 'record_id': record_id, 'members': members}) +def _write_stamp(fetcher, members, record_id=RECID, zenodo=ZENODO, directory=None): + """Write a stamp of the shape the fetcher itself writes after an extraction. + + Every field the reader qualifies on is present, so a test that changes one + of them is changing the one thing under examination. + """ + directory = fetcher.target_dir if directory is None else directory + directory.mkdir(parents=True, exist_ok=True) + (directory / STAMP).write_text( + json.dumps( + { + 'schema': 1, + 'extract': 'tar', + 'record_id': record_id, + 'zenodo': zenodo, + 'members': members, + } + ) ) @@ -100,7 +114,7 @@ def test_a_complete_tree_is_reported_complete_and_verified(tmp_path): result = check_dataset(fetcher, key='demo') assert result.complete - assert result.hashed, 'a plain dataset has digests, so it must not report presence only' + assert result.verifiable, 'a plain dataset has digests, so it must not report presence only' assert [f.name for f in result.files] == ['alpha.dat', 'beta.dat'] assert {f.state for f in result.files} == {OK} assert result.missing == () and result.mismatched == () @@ -178,7 +192,7 @@ def test_archive_members_are_present_not_verified(tmp_path): result = check_dataset(fetcher) assert result.complete - assert not result.hashed, 'an archive dataset cannot claim its members were verified' + assert not result.verifiable, 'an archive dataset cannot claim its members were verified' assert {f.state for f in result.files} == {PRESENT} assert 'presence only' in result.summary() @@ -198,6 +212,62 @@ def test_a_deleted_archive_member_is_reported_missing(tmp_path): assert [f.name for f in result.files if f.state == PRESENT] == ['inner/one.dat'] +def test_an_unreadable_archive_member_costs_one_entry_not_the_dataset(tmp_path): + """A member the checker cannot reach is one fault, not a lost report. + + The plain-file path already reports an unreadable file and carries on. The + archive path has to do the same, or a single directory denying traversal + turns every other member of that dataset into no information at all. + """ + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, ['locked/one.dat', 'inner/two.dat']) + for member in ('locked/one.dat', 'inner/two.dat'): + path = fetcher.target_dir / member + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'x') + locked = fetcher.target_dir / 'locked' + locked.chmod(0o000) + try: + result = check_dataset(fetcher) + finally: + locked.chmod(0o755) + + states = {f.name: f.state for f in result.files} + assert states['locked/one.dat'] == UNREADABLE + assert states['inner/two.dat'] == PRESENT, 'the reachable member must still be reported' + assert not result.complete + assert [f.name for f in result.unreadable] == ['locked/one.dat'] + + +def test_a_presence_only_report_does_not_claim_verification(tmp_path): + """A sound archive tree is reported present, and the verdict says only that. + + An archive dataset carries no per-file digests, so nothing about its + contents was established. A verdict reading "verified" over a line reading + "presence only" is the overstatement this module exists to avoid, and a + caller needing the stronger statement asks ``verified`` rather than ``ok``. + """ + archive = _archive_fetcher(tmp_path) + _write_stamp(archive, ['inner/one.dat']) + member = archive.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + plain = _plain_fetcher(tmp_path / 'other') + _populate(plain) + + by_presence = CheckReport(datasets={'arc': check_dataset(archive, key='arc')}) + by_digest = CheckReport(datasets={'plain': check_dataset(plain, key='plain')}) + + assert by_presence.ok, 'presence is all that is checkable there, so it is not a fault' + assert not by_presence.verified, 'nothing was hashed, so nothing was verified' + assert [d.key for d in by_presence.presence_only] == ['arc'] + assert 'verified' not in by_presence.summary() + assert '1 dataset(s) by presence only' in by_presence.summary() + + assert by_digest.verified, 'a hashed tree must still reach the stronger verdict' + assert 'all data present and verified' in by_digest.summary() + + @pytest.mark.parametrize( 'stamp_body', ['', 'not json at all', json.dumps({'schema': 1}), json.dumps({'extract': 'zip'})], @@ -219,6 +289,11 @@ def test_an_archive_without_a_usable_stamp_is_not_complete(tmp_path, stamp_body) assert result.files, 'an unusable stamp must not produce an empty, complete report' assert not result.complete assert [f.name for f in result.missing] == ['bundle.tar.gz'] + assert not result.verifiable, ( + 'an archive dataset carries no per-file digests whatever its stamp says, ' + 'so it can never report itself checkable against one' + ) + assert 'presence only' in result.summary() def test_an_unreadable_manifest_fails_the_report(tmp_path): @@ -422,20 +497,32 @@ def test_a_stamp_from_another_record_does_not_describe_this_tree(tmp_path): assert [f.name for f in result.missing] == ['bundle.tar.gz'] -def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path): +@pytest.mark.parametrize( + 'escaping', + ['../../../outside.dat', 'ABSOLUTE', 'link/outside.dat', '.', 'inner/../..'], + ids=['relative', 'absolute', 'through-a-symlink', 'the-directory-itself', 'trailing'], +) +def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path, escaping): """A member name pointing outside the dataset directory is dropped. A stamp is an ordinary file on disk and can be edited or replaced, so a name inside it gets the same suspicion as a name inside an archive rather than being joined onto the tree and reported on. + + The symlink case is why containment is decided on the resolved path rather + than on the spelling of the name: ``link/outside.dat`` has no ``..``, no + leading separator, and nothing else a lexical check could object to. """ fetcher = _archive_fetcher(tmp_path) outside = tmp_path / 'outside.dat' outside.write_bytes(b'not part of the dataset\n') - _write_stamp(fetcher, ['../../../outside.dat', 'inner/one.dat']) + if escaping == 'ABSOLUTE': + escaping = str(outside) + _write_stamp(fetcher, [escaping, 'inner/one.dat']) member = fetcher.target_dir / 'inner/one.dat' member.parent.mkdir(parents=True, exist_ok=True) member.write_bytes(b'x') + (fetcher.target_dir / 'link').symlink_to(tmp_path, target_is_directory=True) members = fetcher.recorded_members() @@ -445,6 +532,40 @@ def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path): assert outside.is_file(), 'the check reads only; it never touches what it refused' +def test_a_stamp_naming_only_escaping_members_describes_no_tree(tmp_path): + """Every name dropped leaves no tree, not a complete tree of nothing. + + The boundary of the rule above: an empty survivor list must read the same + as an absent stamp, or a stamp holding nothing but escaping names would + certify a dataset whose files were never looked at. + """ + fetcher = _archive_fetcher(tmp_path) + (tmp_path / 'outside.dat').write_bytes(b'not part of the dataset\n') + _write_stamp(fetcher, ['../../../outside.dat']) + + assert fetcher.recorded_members() is None + + result = check_dataset(fetcher) + assert not result.complete + assert [f.name for f in result.missing] == ['bundle.tar.gz'] + + +def test_a_stamp_from_another_deposit_does_not_describe_this_tree(tmp_path): + """A stamp naming a different DOI is rejected like one naming another record. + + The cache path and the local path qualify a stamp by the same fields, so + the deposit has to match and not only the record id parsed out of it. + """ + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, ['inner/one.dat'], zenodo='10.5281/zenodo.7654321') + member = fetcher.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + + assert fetcher.recorded_members() is None + assert not check_dataset(fetcher).complete + + def test_one_unresolvable_dataset_fails_a_report_of_sound_ones(tmp_path, monkeypatch): """A dataset that could not be checked fails the report beside healthy ones. diff --git a/tests/test_fetch.py b/tests/test_fetch.py index 1e2b389..739f6f4 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -697,6 +697,43 @@ def test_shared_cache_serves_an_archive_dataset_offline(http_server, tmp_path, m _archive_fetcher(base_url, registry, tmp_path / 'other', 'tar').fetch_all(offline=True) +def test_a_cache_stamp_naming_members_outside_the_cache_is_refused( + http_server, tmp_path, monkeypatch +): + """A shared-cache stamp gets the same suspicion as a local one. + + The cache is group-writable by design, so its stamp is no more trustworthy + than the dataset's own. A stamp whose members resolve outside the cached + directory describes no tree there, and the copy has to be refused rather + than accepted because some file somewhere answered to the name. + """ + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + cache_root = tmp_path / 'shared_cache' + _archive_fetcher(base_url, registry, cache_root, 'tar').fetch_all() + cached_stamp = cache_root / VERSIONED / '.fwl-io.json' + record = json.loads(cached_stamp.read_text()) + outside = tmp_path / 'planted.txt' + outside.write_bytes(b'not part of the cached tree\n') + + monkeypatch.setenv('FWL_DATA_CACHE', str(cache_root)) + tampered = dict(record, members=['../../../../planted.txt']) + cached_stamp.write_text(json.dumps(tampered)) + with pytest.raises(OfflineDataError): + _archive_fetcher('http://127.0.0.1:1/', registry, tmp_path / 'a', 'tar').fetch_all( + offline=True + ) + + # Discrimination: the same cache with its real member list does serve, so + # the refusal above is the escaping name and not a broken fixture. + cached_stamp.write_text(json.dumps(record)) + paths = _archive_fetcher('http://127.0.0.1:1/', registry, tmp_path / 'b', 'tar').fetch_all( + offline=True + ) + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + assert outside.is_file(), 'the refused name is read only, never touched' + + def test_archive_extracts_a_top_level_directory_member(http_server, tmp_path): """A tar whose members sit under a top-level directory keeps that structure. From be10da96a3775d4fc6bcf7d2066925b659a3b92c Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 06:54:57 +0200 Subject: [PATCH 05/16] Read every provenance stamp through one guarded parser A stamp holding valid JSON that is not an object aborted an archive fetch with an AttributeError from deep inside the fetcher: the reader parsed the file and then asked a list for a key. Truncating or hand-editing `.fwl-io.json` is how that happens, and the file being editable is the whole reason the member names in it are treated with suspicion, so the reader itself has to survive one. The two readers now share `_read_stamp`, which decides absent, unreadable, not JSON and not an object all in one place; a stamp like that is now rewritten on the next fetch and reported as no tree by a check, which is what both already claimed to do. The check path was already safe, so this only ever bit the fetch path. `DatasetCheck.verifiable` loses its default. Its wrong value is the one that lets a presence-only dataset read as verified, and a default that is right for one of the two dataset kinds is a quiet way to reach that, so every construction site states it. Scoping two claims that were wider than the truth: the cost note now says a full read pass is what plain datasets cost, since an archive dataset is only tested for presence and reads nothing; and `file_matches` raises OSError for an absent file as much as an unreadable one, which its Raises clause now says. `CheckReport.faults` says in its docstring that it covers datasets and nothing else, so an empty tuple is not an answer to whether anything is wrong. Four gaps in what the tests pin: a stamp that parses to a list, null, a number or a string; a member entry that is not a name at all; the soundness half of `verified`, which a hashed tree with a missing file has to fail; and the command exiting 0, which nothing covered in either the verified or the presence-only wording. --- docs/Reference/cli.md | 2 +- src/fwl_io/check.py | 16 +++++++--- src/fwl_io/cli.py | 2 +- src/fwl_io/fetch.py | 47 +++++++++++++++++++--------- tests/test_check.py | 71 ++++++++++++++++++++++++++++++++++++++++--- tests/test_cli.py | 59 +++++++++++++++++++++++++++++++++++ tests/test_fetch.py | 35 +++++++++++++++++++++ 7 files changed, 207 insertions(+), 25 deletions(-) diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index 28a8aa7..9039294 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -38,7 +38,7 @@ Two kinds of failure are reported apart from the datasets, because they call for The report goes to stdout whatever the verdict, so a caller running this to find out what is wrong gets the detail and not only the exit status. Exit is 1 on any missing, corrupt or unreadable file, any unreadable manifest, any unresolvable dataset, or a model no manifest declares. The closing line says `all data present and verified` only when every file was compared against a digest; a sound tree holding a presence-only dataset closes with `all data present, N dataset(s) by presence only` instead, and still exits 0. -Checking reads and hashes every file the manifest declares, so the cost is one full pass over the model's data. On a multi-gigabyte tree, or a shared cluster filesystem, expect it to take as long as reading that data once. +Checking reads and hashes every file a plain dataset declares, so for those the cost is one full pass over the data: on a multi-gigabyte tree, or a shared cluster filesystem, expect it to take as long as reading that data once. An archive dataset costs far less, since its members have no digests to check and are only tested for presence. Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. Resolving the data root creates that root if it does not exist, as it does for every other subcommand. The equivalent Python entry point is `fwl_io.check_for`, whose `CheckReport.ok` is false when nothing was checked, so a model that matches no dataset can never read as a clean tree. `CheckReport.verified` is the stricter question, false whenever any part of the tree was checked by presence alone. diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py index d16cb55..44ace5e 100644 --- a/src/fwl_io/check.py +++ b/src/fwl_io/check.py @@ -72,14 +72,16 @@ class DatasetCheck: each file it declares. It is false for an archive dataset, whose members are recorded by name only, and it is a property of the dataset rather than of what happens to be on disk, so an archive dataset with no members left - cannot read as verifiable. + cannot read as verifiable. It has no default: the wrong value is the one + that lets a presence-only dataset read as verified, so every caller states + it rather than inheriting it. """ key: str subdir: str directory: Path files: tuple[FileCheck, ...] - verifiable: bool = True + verifiable: bool def _in_state(self, state: str) -> tuple[FileCheck, ...]: return tuple(f for f in self.files if f.state == state) @@ -174,7 +176,13 @@ def presence_only(self) -> tuple[DatasetCheck, ...]: @property def faults(self) -> tuple[DatasetCheck, ...]: - """Datasets with something wrong, the worst affected named first.""" + """Datasets with something wrong, the worst affected named first. + + Datasets only. A report failed by an unreadable manifest or a dataset + that could not be resolved has nothing to put here, so this being + empty is not the same as nothing being wrong; ``ok`` is the question + that covers every reason. + """ broken = [d for d in self.datasets.values() if not d.complete] return tuple(sorted(broken, key=lambda d: (-len(d.faults), d.key))) @@ -272,7 +280,7 @@ def check_dataset(fetcher: Fetcher, key: str = '') -> DatasetCheck: FileCheck(name, fetcher.target_dir / name, _file_state(fetcher, name)) for name in sorted(fetcher.registry) ] - return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks)) + return DatasetCheck(key, fetcher.subdir, fetcher.target_dir, tuple(checks), verifiable=True) def check_for(model: str, data_root: str | Path | None = None) -> CheckReport: diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index 95ec919..a7a41c3 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -112,7 +112,7 @@ def main(argv: list[str] | None = None) -> int: p_check = sub.add_parser( 'check', - help='report whether a model has its data, without downloading (hashes every file)', + help='report whether a model has its data, without downloading (hashes what it can)', ) p_check.add_argument('model', help='model name matched against required_by') p_check.add_argument('--data-root', default=None, help='override the FWL_DATA root') diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index 56ea6ae..08c105d 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -508,7 +508,8 @@ def file_matches(self, fname: str) -> bool: ``fname`` is not declared in this dataset's registry, so there is no digest to compare it against. OSError - The file is present but could not be read. + The file could not be read, whether because it is absent or + because the tree denies access to it. """ if fname not in self.registry: raise KeyError(f'{fname!r} is not in the registry for {self.subdir!r}') @@ -533,6 +534,23 @@ def recorded_members(self) -> list[str] | None: """ return self._stamp_members(self.target_dir) + @staticmethod + def _read_stamp(directory: Path) -> dict | None: + """The stamp record in ``directory``, or ``None`` if there is no usable one. + + Every reader of a stamp goes through here, so none of them has to + rediscover that the file may be absent, unreadable, not JSON, or JSON + that is not an object. The last is the one worth naming: a stamp is an + ordinary file that can be edited or truncated, and a reader that parsed + ``[]`` and then asked it for a key would raise where it should have + decided the stamp says nothing. + """ + try: + record = json.loads((directory / _STAMP_FILENAME).read_text()) + except (OSError, ValueError): + return None + return record if isinstance(record, dict) else None + def _stamp_members(self, directory: Path) -> list[str] | None: """Members recorded by the stamp in ``directory``, or ``None``. @@ -541,11 +559,8 @@ def _stamp_members(self, directory: Path) -> list[str] | None: stamp is no more trustworthy than a local one, and two readers with their own qualifying rules drift apart. """ - try: - record = json.loads((directory / _STAMP_FILENAME).read_text()) - except (OSError, ValueError): - return None - if not isinstance(record, dict): + record = self._read_stamp(directory) + if record is None: return None if record.get('extract') != self.extract: return None @@ -590,8 +605,7 @@ def _fetch_archive(self, offline: bool | None = None) -> list[Path]: is staged and the tree is moved into place atomically. """ archive_name, known_hash = next(iter(self.registry.items())) - stamp = self.target_dir / _STAMP_FILENAME - if self._stamp_is_current(stamp) and self._archive_tree_intact(): + if self._stamp_is_current(self.target_dir) and self._archive_tree_intact(): self._sources.setdefault(archive_name, 'local') return self._extracted_files() @@ -676,15 +690,18 @@ def _copy_cached_tree(self) -> bool: log.info('copied dataset from shared cache %s', cache_root) return True - def _stamp_is_current(self, stamp: Path) -> bool: + def _stamp_is_current(self, directory: Path) -> bool: """True when a valid stamp for this exact record id already exists. - A missing, unreadable, non-JSON, or mismatched stamp is not current, - so it is rewritten (healed) rather than trusted forever. + A missing, unreadable, malformed, or mismatched stamp is not current, + so it is rewritten (healed) rather than trusted forever. It asks less + than :meth:`_stamp_members`, which also has to agree about the archive + kind and the member list; both read the file through + :meth:`_read_stamp`, so neither can be broken by a stamp the other + would have refused. """ - try: - existing = json.loads(stamp.read_text()) - except (OSError, ValueError): + existing = self._read_stamp(directory) + if existing is None: return False return existing.get('record_id') == self.record_id and existing.get('zenodo') == self.zenodo @@ -706,7 +723,7 @@ def _write_stamp(self) -> None: if self.version_dir is None: return stamp = self.target_dir / _STAMP_FILENAME - if self._stamp_is_current(stamp): + if self._stamp_is_current(self.target_dir): return record = { 'schema': _STAMP_SCHEMA, diff --git a/tests/test_check.py b/tests/test_check.py index 508e89c..24371cf 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -270,8 +270,26 @@ def test_a_presence_only_report_does_not_claim_verification(tmp_path): @pytest.mark.parametrize( 'stamp_body', - ['', 'not json at all', json.dumps({'schema': 1}), json.dumps({'extract': 'zip'})], - ids=['empty', 'unparseable', 'no-members', 'wrong-kind'], + [ + '', + 'not json at all', + json.dumps({'schema': 1}), + json.dumps({'extract': 'zip'}), + json.dumps([1, 2, 3]), + json.dumps(None), + json.dumps(42), + json.dumps('a string'), + ], + ids=[ + 'empty', + 'unparseable', + 'no-members', + 'wrong-kind', + 'a-json-list', + 'json-null', + 'a-json-number', + 'a-json-string', + ], ) def test_an_archive_without_a_usable_stamp_is_not_complete(tmp_path, stamp_body): """No usable stamp means no tree, reported as one missing item. @@ -279,6 +297,11 @@ def test_an_archive_without_a_usable_stamp_is_not_complete(tmp_path, stamp_body) The trap this guards is reporting zero files, which would make ``complete`` true and hand back a clean bill of health for a dataset that was never extracted. + + The four JSON bodies that parse to something other than an object are the + ones a reader is most likely to trip over: a stamp truncated or replaced by + hand still parses, and asking a list for a key raises where the answer + should be that the stamp describes nothing. """ fetcher = _archive_fetcher(tmp_path) fetcher.target_dir.mkdir(parents=True, exist_ok=True) @@ -320,8 +343,10 @@ def test_an_unreadable_manifest_fails_the_report(tmp_path): def test_the_summary_names_the_worst_dataset_first(tmp_path): """Datasets with more faults are listed ahead of those with fewer.""" - one_fault = DatasetCheck('one', SUBDIR, tmp_path, (_file('a', MISSING),)) - two_faults = DatasetCheck('two', SUBDIR, tmp_path, (_file('a', MISSING), _file('b', MISMATCH))) + one_fault = DatasetCheck('one', SUBDIR, tmp_path, (_file('a', MISSING),), verifiable=True) + two_faults = DatasetCheck( + 'two', SUBDIR, tmp_path, (_file('a', MISSING), _file('b', MISMATCH)), verifiable=True + ) report = CheckReport(datasets={'one': one_fault, 'two': two_faults}, manifest_errors={}) assert not report.ok @@ -532,6 +557,44 @@ def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path, escaping): assert outside.is_file(), 'the check reads only; it never touches what it refused' +def test_a_member_that_is_not_a_name_is_dropped(tmp_path): + """A member entry of the wrong type is skipped, not joined onto the tree. + + Alongside the escaping-name rule: the list in a stamp is as editable as the + names in it, and a number where a name belongs must not reach a path join. + """ + fetcher = _archive_fetcher(tmp_path) + _write_stamp(fetcher, [123, None, {'not': 'a name'}, 'inner/one.dat']) + member = fetcher.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + + assert fetcher.recorded_members() == ['inner/one.dat'] + result = check_dataset(fetcher) + assert result.complete, 'the one real member is present, so the tree is whole' + assert [f.name for f in result.files] == ['inner/one.dat'] + + +def test_a_faulty_but_hashed_tree_is_not_verified(tmp_path): + """Verification needs the tree to be sound as well as hashed. + + A plain dataset carries digests for every file it declares, so nothing in + it is presence-only; a missing file still has to keep it from reading as + verified, or the stronger question would be weaker than ``ok`` for exactly + the trees that fail. + """ + fetcher = _plain_fetcher(tmp_path) + _populate(fetcher, names=['alpha.dat']) + dataset = check_dataset(fetcher, key='demo') + report = CheckReport(datasets={'demo': dataset}) + + assert dataset.verifiable, 'a plain dataset carries digests, so it is checkable' + assert not report.ok + assert not report.verified, 'a tree with a missing file is not verified whatever it hashes' + assert report.presence_only == (), 'nothing here was checked by presence' + assert 'data check FAILED' in report.summary() + + def test_a_stamp_naming_only_escaping_members_describes_no_tree(tmp_path): """Every name dropped leaves no tree, not a complete tree of nothing. diff --git a/tests/test_cli.py b/tests/test_cli.py index 56e6699..04006ac 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -148,3 +148,62 @@ def load(self): # Resolving a path creates the data root, as it does for every entry point. # What a check must not do is populate it: no dataset directory, no file. assert list(data_root.iterdir()) == [], 'a check must not create the tree it inspects' + + +@pytest.mark.unit +def test_check_exits_zero_and_says_which_verdict_it_reached(tmp_path, capsys, monkeypatch): + """A sound tree exits 0, and the wording separates hashed from presence-only. + + Both trees here are sound, so the exit code cannot tell them apart, which is + the intended contract: presence is all an archive dataset makes checkable and + it is not a fault. What must differ is the claim. Without the second half a + change tying the exit code to verification instead of soundness would go + unnoticed, and every archive dataset would start failing. + """ + import hashlib + + manifest = tmp_path / 'manifest.toml' + manifest.write_text( + '[g.plain]\nzenodo = "10.5281/zenodo.1234567"\nrequired_by = ["demo"]\n\n' + '[g.arc]\nzenodo = "10.5281/zenodo.7654321"\nrequired_by = ["demo"]\n' + 'extract = "tar"\n' + ) + body = b'contents\n' + digest = hashlib.sha256(body).hexdigest() + (tmp_path / 'g.plain.registry.txt').write_text(f'alpha.dat sha256:{digest}\n') + (tmp_path / 'g.arc.registry.txt').write_text('bundle.tar sha256:' + 'a' * 64 + '\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + data_root = tmp_path / 'data' + plain_dir = data_root / 'g' / 'plain' / 'r1234567' + plain_dir.mkdir(parents=True) + (plain_dir / 'alpha.dat').write_bytes(body) + arc_dir = data_root / 'g' / 'arc' / 'r7654321' + arc_dir.mkdir(parents=True) + (arc_dir / 'inner.dat').write_bytes(b'x') + (arc_dir / '.fwl-io.json').write_text( + json.dumps( + { + 'extract': 'tar', + 'record_id': '7654321', + 'zenodo': '10.5281/zenodo.7654321', + 'members': ['inner.dat'], + } + ) + ) + + code = main(['check', 'demo', '--data-root', str(data_root)]) + out = capsys.readouterr().out + + assert code == 0, 'a sound tree exits 0 even where only presence was checkable' + assert 'FAILED' not in out + assert 'g.plain: ok' in out and 'g.arc: ok' in out + assert 'presence only' in out, 'the archive dataset has to say what it could not check' + assert 'all data present, 1 dataset(s) by presence only' in out + assert 'and verified' not in out, 'one presence-only dataset forfeits the stronger claim' diff --git a/tests/test_fetch.py b/tests/test_fetch.py index 739f6f4..f003aae 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -697,6 +697,41 @@ def test_shared_cache_serves_an_archive_dataset_offline(http_server, tmp_path, m _archive_fetcher(base_url, registry, tmp_path / 'other', 'tar').fetch_all(offline=True) +@pytest.mark.parametrize( + 'stamp_body', + ['[1, 2, 3]', 'null', '42', '"a string"'], + ids=['a-json-list', 'json-null', 'a-json-number', 'a-json-string'], +) +def test_a_stamp_that_is_not_an_object_is_healed_not_raised( + http_server, tmp_path, stamp_body, monkeypatch +): + """A stamp holding valid JSON that is not an object is rewritten, not fatal. + + Truncating or hand-editing the file is how it happens, and the result still + parses. Every reader has to treat it as a stamp that says nothing: the fetch + re-downloads and writes a good one, rather than failing the dataset with an + error about the shape of a provenance file. + """ + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + version_dir = tmp_path / VERSIONED + version_dir.mkdir(parents=True) + (version_dir / '.fwl-io.json').write_text(stamp_body) + + paths = _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + healed = json.loads((version_dir / '.fwl-io.json').read_text()) + assert healed['record_id'] == RECID, 'the unusable stamp is replaced by a real one' + assert healed['members'] == ['m0p1.txt', 'nested/m1p0.txt'] + + # Discrimination: offline, the same unusable stamp yields the honest + # "nothing here to serve" error rather than one about the stamp itself. + (version_dir / '.fwl-io.json').write_text(stamp_body) + with pytest.raises(OfflineDataError): + _archive_fetcher(base_url, registry, tmp_path / 'other', 'tar').fetch_all(offline=True) + + def test_a_cache_stamp_naming_members_outside_the_cache_is_refused( http_server, tmp_path, monkeypatch ): From 8a174258b0336bc461254cff5a374d729cefe267 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 07:23:29 +0200 Subject: [PATCH 06/16] Read the stamp schema instead of only writing it `_STAMP_SCHEMA` went into every stamp and nothing ever read it back, which leaves the guarded parser guarding the wrong thing. A stamp written by a future version is well-formed JSON in an object, so it passes every check there was, while its fields need not mean what they mean here; the reader would then trust them precisely when it should not. An unrecognised schema now reads as no stamp, so the tree is refetched and restamped rather than misread. That is also what makes the shape checks worth having: they cover a file that was damaged, and this covers one that was written on purpose to different rules. The states that fail a dataset and the words the report prints for them are now one mapping rather than two lists. Adding a state to the first would have failed a dataset while the line a person reads never said why. Three corrections to the tests. The offline half of the malformed-stamp case pointed at an empty data root, so it raised for want of any data at all and would have passed whatever the stamp reader did; it now puts the bad stamp back over the tree it has just populated, where the stamp is the only thing left that can decide the dataset is unservable. The command-line fixture wrote a stamp without the schema field that every real one carries. And the shared cache had no malformed-stamp case, although a group-writable cache on a cluster is where a half-written one is most likely to turn up. --- src/fwl_io/check.py | 19 ++++++++-------- src/fwl_io/fetch.py | 22 ++++++++++++++----- tests/test_check.py | 53 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_cli.py | 1 + tests/test_fetch.py | 19 +++++++++++++--- 5 files changed, 96 insertions(+), 18 deletions(-) diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py index 44ace5e..6c8dc6d 100644 --- a/src/fwl_io/check.py +++ b/src/fwl_io/check.py @@ -44,10 +44,14 @@ UNREADABLE = 'unreadable' PRESENT = 'present' -#: States that mean the tree is not usable as the manifest describes it. A file -#: that cannot be read counts: whether its contents are right is unknown, and a -#: check reports what it could not establish rather than assuming the best. -FAULT_STATES = (MISSING, MISMATCH, UNREADABLE) +#: States that mean the tree is not usable as the manifest describes it, each +#: with the word the report prints for it. A file that cannot be read counts: +#: whether its contents are right is unknown, and a check reports what it could +#: not establish rather than assuming the best. The summary counts these by +#: walking this mapping, so a state added here is named in the report rather +#: than failing a dataset for a reason the text never gives. +FAULT_LABELS = {MISSING: 'missing', MISMATCH: 'corrupt', UNREADABLE: 'unreadable'} +FAULT_STATES = tuple(FAULT_LABELS) @dataclass(frozen=True) @@ -114,11 +118,8 @@ def complete(self) -> bool: def summary(self) -> str: """One line naming the counts, for a report a person reads.""" parts = [f'{len(self.files)} file(s)'] - for label, group in ( - ('missing', self.missing), - ('corrupt', self.mismatched), - ('unreadable', self.unreadable), - ): + for state, label in FAULT_LABELS.items(): + group = self._in_state(state) if group: parts.append(f'{len(group)} {label}') if not self.verifiable: diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index 08c105d..7c84caa 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -539,17 +539,27 @@ def _read_stamp(directory: Path) -> dict | None: """The stamp record in ``directory``, or ``None`` if there is no usable one. Every reader of a stamp goes through here, so none of them has to - rediscover that the file may be absent, unreadable, not JSON, or JSON - that is not an object. The last is the one worth naming: a stamp is an - ordinary file that can be edited or truncated, and a reader that parsed - ``[]`` and then asked it for a key would raise where it should have - decided the stamp says nothing. + rediscover that the file may be absent, unreadable, not JSON, JSON that + is not an object, or an object written to a schema this version does + not know. The third is the one worth naming: a stamp is an ordinary + file that can be edited or truncated, and a reader that parsed ``[]`` + and then asked it for a key would raise where it should have decided + the stamp says nothing. + + The schema is what makes the rest of that safe over time. A stamp + written by a future version can be well-formed JSON in a shape whose + fields no longer mean what they did, and the fields this version reads + would then be trusted while meaning something else; an unrecognised + schema is treated as no stamp, so the tree is refetched and restamped + rather than misread. """ try: record = json.loads((directory / _STAMP_FILENAME).read_text()) except (OSError, ValueError): return None - return record if isinstance(record, dict) else None + if not isinstance(record, dict) or record.get('schema') != _STAMP_SCHEMA: + return None + return record def _stamp_members(self, directory: Path) -> list[str] | None: """Members recorded by the stamp in ``directory``, or ``None``. diff --git a/tests/test_check.py b/tests/test_check.py index 24371cf..b70de9a 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -557,6 +557,59 @@ def test_a_stamp_member_escaping_the_dataset_is_refused(tmp_path, escaping): assert outside.is_file(), 'the check reads only; it never touches what it refused' +def test_a_stamp_written_to_an_unknown_schema_is_not_read(tmp_path): + """A stamp whose schema this version does not know describes nothing. + + Its fields can be well-formed and still mean something else, so reading + them would be trusting a format nobody here has seen. The tree is reported + absent, which sends the dataset back through a fetch that restamps it. + """ + fetcher = _archive_fetcher(tmp_path) + fetcher.target_dir.mkdir(parents=True, exist_ok=True) + (fetcher.target_dir / STAMP).write_text( + json.dumps( + { + 'schema': 99, + 'extract': 'tar', + 'record_id': RECID, + 'zenodo': ZENODO, + 'members': ['inner/one.dat'], + } + ) + ) + member = fetcher.target_dir / 'inner/one.dat' + member.parent.mkdir(parents=True, exist_ok=True) + member.write_bytes(b'x') + + assert fetcher.recorded_members() is None + assert not check_dataset(fetcher).complete + + # Discrimination: the same stamp at the known schema is read, so it is the + # schema and not some other field that decided the answer above. + _write_stamp(fetcher, ['inner/one.dat']) + assert fetcher.recorded_members() == ['inner/one.dat'] + assert check_dataset(fetcher).complete + + +def test_every_fault_state_is_named_in_the_summary(tmp_path): + """Each way a file can be at fault is counted in the line a person reads. + + The states that fail a dataset and the words the report prints for them + come from one mapping, so a dataset can never fail for a reason the text + leaves out. + """ + from fwl_io.check import FAULT_LABELS, FAULT_STATES + + files = tuple(_file(f'f{i}.dat', state) for i, state in enumerate(FAULT_STATES)) + dataset = DatasetCheck('demo', SUBDIR, tmp_path, files, verifiable=True) + line = dataset.summary() + + assert not dataset.complete + assert len(dataset.faults) == len(FAULT_STATES), 'every state here has to be a fault' + for label in FAULT_LABELS.values(): + assert f'1 {label}' in line, f'the report never says {label!r}' + + def test_a_member_that_is_not_a_name_is_dropped(tmp_path): """A member entry of the wrong type is skipped, not joined onto the tree. diff --git a/tests/test_cli.py b/tests/test_cli.py index 04006ac..c0ad397 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -190,6 +190,7 @@ def load(self): (arc_dir / '.fwl-io.json').write_text( json.dumps( { + 'schema': 1, 'extract': 'tar', 'record_id': '7654321', 'zenodo': '10.5281/zenodo.7654321', diff --git a/tests/test_fetch.py b/tests/test_fetch.py index f003aae..26460ae 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -725,11 +725,16 @@ def test_a_stamp_that_is_not_an_object_is_healed_not_raised( assert healed['record_id'] == RECID, 'the unusable stamp is replaced by a real one' assert healed['members'] == ['m0p1.txt', 'nested/m1p0.txt'] - # Discrimination: offline, the same unusable stamp yields the honest - # "nothing here to serve" error rather than one about the stamp itself. + # Discrimination: put the unusable stamp back over the tree that is now + # fully populated, and go offline. The members are all on disk, so the only + # thing that can decide the dataset is unservable is the stamp, and the + # answer has to be the honest "nothing here to serve" rather than an error + # about the shape of a provenance file. Pointing this at an empty root + # instead would raise the same error whatever the stamp reader did. (version_dir / '.fwl-io.json').write_text(stamp_body) + assert (version_dir / 'm0p1.txt').is_file(), 'the tree must be intact, or this proves nothing' with pytest.raises(OfflineDataError): - _archive_fetcher(base_url, registry, tmp_path / 'other', 'tar').fetch_all(offline=True) + _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all(offline=True) def test_a_cache_stamp_naming_members_outside_the_cache_is_refused( @@ -759,6 +764,14 @@ def test_a_cache_stamp_naming_members_outside_the_cache_is_refused( offline=True ) + # A cache stamp that is malformed rather than tampered is refused the same + # way, by the reader both trees share, rather than raising out of the copy. + cached_stamp.write_text('[1, 2, 3]') + with pytest.raises(OfflineDataError): + _archive_fetcher('http://127.0.0.1:1/', registry, tmp_path / 'c', 'tar').fetch_all( + offline=True + ) + # Discrimination: the same cache with its real member list does serve, so # the refusal above is the escaping name and not a broken fixture. cached_stamp.write_text(json.dumps(record)) From 89ba18233e8dcd201f61d476ef88508ab95f0fab Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 10:15:04 +0200 Subject: [PATCH 07/16] Add a command that moves legacy data into the current layout A tree fetched before the current layout existed still holds directories like `stellar_evolution_tracks/Baraffe`. Unmigrated code reads them, so they are left alone and age out as their consumers migrate, which is right for the ecosystem and no help at all to someone who wants their own tree tidy today. `fwl-io relocate` is for them: it finds the datasets the installed manifests declare, works out where each one used to live, and moves the files across. Nothing moves on trust. Every file is hashed against the registry before anything is touched, and a dataset with a file missing or a file whose contents differ is reported and left exactly where it is. Moving first and discovering afterwards would turn a stale copy into a stale copy at the location the fetcher then believes, which is worse than leaving it somewhere a reader can still tell it is old. A move that fails part way puts back what it moved, since a dataset split across two layouts is the one state neither the reader nor the fetcher can interpret. Where each dataset used to live is the one thing a manifest cannot say, so it ships as a table in the package. The manifest supplies the record id and the checksums; the table supplies the old path. Keying it by dataset key means a directory that held several datasets comes apart correctly, since each moves only the files its own registry names, and a dataset created after the migration needs no entry at all. The only directories it removes are ones it has just emptied itself, and the walk upward stops at the data root. A copy still sitting at the old location beside a current one is named rather than deleted, and counted in the closing line: on a tree where everything has already been refetched that count is the only thing the run has to say, and it is the disk the user can reclaim. An unread manifest keeps the report from claiming to be complete. It may be the one declaring the dataset whose tree is still sitting there, and a run that looked at nothing otherwise reads exactly like a tree with nothing left to move. --- docs/Explanations/manifests.md | 4 +- docs/Reference/api/index.md | 2 + docs/Reference/api/relocate.md | 3 + docs/Reference/cli.md | 14 ++ mkdocs.yml | 1 + src/fwl_io/__init__.py | 5 + src/fwl_io/cli.py | 22 +- src/fwl_io/data/legacy_layout.toml | 28 +++ src/fwl_io/fetch.py | 16 +- src/fwl_io/relocate.py | 346 +++++++++++++++++++++++++++++ tests/test_fetch.py | 37 +++ tests/test_relocate.py | 302 +++++++++++++++++++++++++ 12 files changed, 776 insertions(+), 4 deletions(-) create mode 100644 docs/Reference/api/relocate.md create mode 100644 src/fwl_io/data/legacy_layout.toml create mode 100644 src/fwl_io/relocate.py create mode 100644 tests/test_relocate.py diff --git a/docs/Explanations/manifests.md b/docs/Explanations/manifests.md index 1cdc381..42f276b 100644 --- a/docs/Explanations/manifests.md +++ b/docs/Explanations/manifests.md @@ -67,7 +67,7 @@ For every requested file: ## The FWL_DATA layout -This section is the target layout specification: new datasets and migrating models use it; existing trees keep their legacy directory names until their consumers migrate, so both forms coexist during the transition. A flat copy left by a pre-versioning fetch is re-fetched rather than adopted; a command that relocates such trees in place is tracked in [#13](https://github.com/FormingWorlds/fwl-io/issues/13). +This section is the target layout specification: new datasets and migrating models use it; existing trees keep their legacy directory names until their consumers migrate, so both forms coexist during the transition. A flat copy left by a pre-versioning fetch is re-fetched rather than adopted; `fwl-io relocate` moves such a tree into its current location instead, once its files have been checked against the registry. The target tree is organized by physical domain, mirroring the package structure of the PROTEUS source tree (`src/proteus/`), with one deliberate exception: the two interior packages (`interior_struct`, `interior_energetics`) share a single `interior/` data domain, because the equation-of-state tables serve both. @@ -95,7 +95,7 @@ FWL_DATA/ The tree holds **immutable fetched reference data only**: anything generated at runtime (derived tables, interpolation caches, solver caches) belongs in run output or cache directories, never below `FWL_DATA`. This keeps a shared read-only cache trustworthy as a whole. -Models adopt this layout when they migrate to fwl-io; legacy directories from the previous layout remain readable by unmigrated code and age out when their last consumer migrates (a relocate command for cleaning local trees immediately is tracked in [#13](https://github.com/FormingWorlds/fwl-io/issues/13)). The mapping from the legacy locations: +Models adopt this layout when they migrate to fwl-io; legacy directories from the previous layout remain readable by unmigrated code and age out when their last consumer migrates. `fwl-io relocate` cleans a local tree up straight away instead, moving each dataset whose files check out against its registry. The mapping from the legacy locations, which the package carries in `legacy_layout.toml` and the command reads: | Legacy location (live today) | Target location | |---|---| diff --git a/docs/Reference/api/index.md b/docs/Reference/api/index.md index aaf311d..fe0f533 100644 --- a/docs/Reference/api/index.md +++ b/docs/Reference/api/index.md @@ -8,6 +8,7 @@ from fwl_io import ( load_manifest, discover_manifests, # manifests fetch_for, Dataset, check_for, check_dataset, # validate-only checking + relocate, plan_relocations, # moving a legacy tree into the current layout CheckReport, DatasetCheck, FileCheck, resolve_data_root, resolve_cache_root, DownloadError, OfflineDataError, MissingDataRootError, @@ -19,6 +20,7 @@ Per-module reference pages: - [Fetching](fetch.md): `Fetcher`, `create_fetcher`, error types - [Checking](check.md): `check_for`, `check_dataset`, `CheckReport`, `DatasetCheck`, `FileCheck` +- [Relocating](relocate.md): `relocate`, `plan_relocations`, `RelocationReport`, `Relocation` - [Manifests](manifest.md): `Dataset`, `load_manifest`, `discover_manifests`, `fetch_for`, `ManifestSchemaError` - [Registries](registry.md): registry file reading and writing - [Sync](sync.md): registry generation from the Zenodo API diff --git a/docs/Reference/api/relocate.md b/docs/Reference/api/relocate.md new file mode 100644 index 0000000..2ca7bf7 --- /dev/null +++ b/docs/Reference/api/relocate.md @@ -0,0 +1,3 @@ +# Relocate + +::: fwl_io.relocate diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index 9039294..099b6f2 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -42,6 +42,20 @@ Checking reads and hashes every file a plain dataset declares, so for those the Nothing is downloaded and no dataset directory or file is written, which makes this safe to run against a tree another process is reading. Resolving the data root creates that root if it does not exist, as it does for every other subcommand. The equivalent Python entry point is `fwl_io.check_for`, whose `CheckReport.ok` is false when nothing was checked, so a model that matches no dataset can never read as a clean tree. `CheckReport.verified` is the stricter question, false whenever any part of the tree was checked by presence alone. +## fwl-io relocate + +```bash +fwl-io relocate [--data-root PATH] [--dry-run] +``` + +Moves data left by the previous layout into the place it belongs now, for a tree fetched before the current layout existed. Unmigrated code still reads the old directories, so they are otherwise left alone and age out as their consumers migrate; this is for cleaning a tree up straight away instead. + +A dataset moves only when every file its registry declares is present in the old location and matches its recorded digest. Anything else is reported and left exactly where it is: an incomplete tree, a file whose contents differ, or a dataset whose registry has not been generated. Verifying first is the point, since moving a stale copy would put it where the fetcher then trusts it. Once a dataset's files have moved, the emptied directories are removed, and the walk upward stops at the data root. + +A dataset already at its current location is not a fault, and a copy still sitting at the old location beside it is named rather than deleted. Nothing here removes data: the only directories it removes are ones it has just emptied itself. + +Exit is 1 when a legacy tree was found and could not be moved, or when an installed manifest could not be read, since that manifest may be the one declaring the dataset a tree still holds. A tree that was already tidy exits 0. `--dry-run` reports the same plan without moving anything. The equivalent Python entry points are `fwl_io.relocate` and `fwl_io.plan_relocations`. + ## fwl-io mirror ```bash diff --git a/mkdocs.yml b/mkdocs.yml index 453ddaa..7d5c850 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -28,6 +28,7 @@ nav: - Overview: Reference/api/index.md - Fetching: Reference/api/fetch.md - Checking: Reference/api/check.md + - Relocating: Reference/api/relocate.md - Manifests: Reference/api/manifest.md - Registries: Reference/api/registry.md - Sync: Reference/api/sync.md diff --git a/src/fwl_io/__init__.py b/src/fwl_io/__init__.py index ac8b6d3..6926bb6 100644 --- a/src/fwl_io/__init__.py +++ b/src/fwl_io/__init__.py @@ -19,6 +19,7 @@ load_manifest, ) from fwl_io.paths import MissingDataRootError, resolve_cache_root, resolve_data_root +from fwl_io.relocate import Relocation, RelocationReport, plan_relocations, relocate try: __version__ = version('fwl-io') @@ -35,6 +36,8 @@ 'ManifestSchemaError', 'MissingDataRootError', 'OfflineDataError', + 'Relocation', + 'RelocationReport', '__version__', 'check_dataset', 'check_for', @@ -42,6 +45,8 @@ 'discover_manifests', 'fetch_for', 'load_manifest', + 'plan_relocations', + 'relocate', 'resolve_cache_root', 'resolve_data_root', ] diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index a7a41c3..ae5af93 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -1,4 +1,4 @@ -"""Command-line interface: ``fwl-io sync | list | fetch | check | mirror``. +"""Command-line interface: ``fwl-io sync | list | fetch | check | relocate | mirror``. Failures from the package's own error types exit with status 1 and a one-line message on stderr instead of a traceback. @@ -63,6 +63,17 @@ def _cmd_check(args: argparse.Namespace) -> int: return 0 if report.ok else 1 +def _cmd_relocate(args: argparse.Namespace) -> int: + from fwl_io.relocate import relocate + + report = relocate(data_root=args.data_root, dry_run=args.dry_run) + print(report.summary()) + # A tree that was already tidy is a success, so only a legacy tree that + # could not be moved fails the command. Nothing was changed in that case, + # which is what the exit code has to make actionable. + return 1 if report.faults else 0 + + def _cmd_mirror(args: argparse.Namespace) -> int: from fwl_io.mirror import mirror_to_dataverse @@ -118,6 +129,15 @@ def main(argv: list[str] | None = None) -> int: p_check.add_argument('--data-root', default=None, help='override the FWL_DATA root') p_check.set_defaults(func=_cmd_check) + p_relocate = sub.add_parser( + 'relocate', help='move data left by the previous layout into the current one' + ) + p_relocate.add_argument('--data-root', default=None, help='override the FWL_DATA root') + p_relocate.add_argument( + '--dry-run', action='store_true', help='report what would move without moving it' + ) + p_relocate.set_defaults(func=_cmd_relocate) + p_mirror = sub.add_parser('mirror', help='mirror a Zenodo deposit to a Dataverse collection') p_mirror.add_argument('zenodo_doi', help='Zenodo version DOI to mirror') p_mirror.add_argument('--collection', required=True, help='target Dataverse collection alias') diff --git a/src/fwl_io/data/legacy_layout.toml b/src/fwl_io/data/legacy_layout.toml new file mode 100644 index 0000000..b5ad5d4 --- /dev/null +++ b/src/fwl_io/data/legacy_layout.toml @@ -0,0 +1,28 @@ +# Where each dataset lived before the current FWL_DATA layout. +# +# One entry per dataset, keyed by the dotted manifest key, whose value is the +# directory the dataset occupied relative to the data root. The current +# location is not repeated here: it is the key with its dots turned into +# separators, plus the version directory derived from the manifest's Zenodo +# pin, which is where the manifest already says the dataset belongs. +# +# "star.tracks.baraffe_2015" = "stellar_evolution_tracks/Baraffe" +# +# `fwl-io relocate` reads this together with the installed manifests: the +# manifest supplies the record id and the file checksums, this file supplies +# the only thing the manifest cannot know, which is where the files used to +# be. A dataset absent from this file has no legacy location and is left +# alone, so a dataset added after the migration needs no entry. +# +# Several datasets may name the same legacy directory. Each moves only the +# files its own registry declares, so a directory that held more than one +# dataset comes apart correctly. +# +# This is a record of what happened, not configuration: entries are added when +# a model migrates its data paths, and are never edited afterwards, since a +# tree that was laid out that way is laid out that way forever. + +[legacy] +"star.tracks.baraffe_2015" = "stellar_evolution_tracks/Baraffe" +"observe.exoplanet_reference" = "planet_reference/Exoplanets" +"observe.mass_radius.zeng_2019" = "mass_radius/Zeng2019" diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index 7c84caa..1aa89bb 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -557,7 +557,21 @@ def _read_stamp(directory: Path) -> dict | None: record = json.loads((directory / _STAMP_FILENAME).read_text()) except (OSError, ValueError): return None - if not isinstance(record, dict) or record.get('schema') != _STAMP_SCHEMA: + if not isinstance(record, dict): + return None + if record.get('schema') != _STAMP_SCHEMA: + # Worth saying out loud, because the cost is visible and the cause + # is not: the dataset is refetched in full, and it will be again on + # every run that shares this tree with the version that wrote the + # stamp. Someone watching a cluster job redownload the same data + # nightly needs the reason named. + log.warning( + 'stamp in %s is schema %r, not %r, so it cannot be read and the ' + 'dataset will be fetched again', + directory, + record.get('schema'), + _STAMP_SCHEMA, + ) return None return record diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py new file mode 100644 index 0000000..77f5ef2 --- /dev/null +++ b/src/fwl_io/relocate.py @@ -0,0 +1,346 @@ +"""Move a dataset left by the previous layout into the place it belongs now. + +A tree fetched before the current layout existed holds directories such as +``stellar_evolution_tracks/Baraffe``. Unmigrated code still reads them, so +they are left alone and age out as their consumers migrate. That is the right +default and the wrong one for anybody who wants the tree tidy today, which is +what this does: it finds the datasets an installed manifest declares, works +out where each one used to live, and moves the files across. + +Nothing is moved on trust. Every file is hashed against the registry the +manifest ships before anything is touched, and a dataset with a file missing +or a file whose contents do not match is reported and left exactly where it +is. The alternative, moving first and discovering afterwards, turns a stale +copy into a stale copy in the place the fetcher will now believe. + +Nothing is downloaded either. A dataset whose legacy tree is incomplete stays +incomplete here; the fetcher is what fills it, and it will do so at the +current location once the move has happened. +""" + +from __future__ import annotations + +import logging +import os +import tomllib +from dataclasses import dataclass, field +from importlib.resources import files +from pathlib import Path +from typing import TYPE_CHECKING + +from fwl_io.fetch import _hash_matches +from fwl_io.paths import resolve_data_root + +if TYPE_CHECKING: + from fwl_io.manifest import Dataset + +log = logging.getLogger('fwl.' + __name__) + +_LAYOUT_RESOURCE = 'legacy_layout.toml' + +# What a dataset's legacy tree turned out to be. Only ``READY`` describes +# something to do; the rest say why nothing was done, and are kept apart +# because they call for different responses. ``INCOMPLETE`` and ``MISMATCH`` +# are faults in the tree, the other two are the ordinary cases of a dataset +# that has already moved or never had a legacy copy at all. +READY = 'ready' +ABSENT = 'absent' +ALREADY_CURRENT = 'already-current' +INCOMPLETE = 'incomplete' +MISMATCH = 'mismatch' +UNRESOLVABLE = 'unresolvable' +MOVED = 'moved' +FAILED = 'failed' + +#: States that mean a legacy tree is there but cannot be moved as it stands. +FAULT_STATES = (INCOMPLETE, MISMATCH, UNRESOLVABLE, FAILED) + + +@dataclass(frozen=True) +class Relocation: + """One dataset's legacy tree, and what can be done with it.""" + + key: str + state: str + legacy_dir: Path | None = None + target_dir: Path | None = None + files: tuple[str, ...] = () + detail: str = '' + legacy_present: bool = False + + @property + def faulty(self) -> bool: + """True when a legacy tree is present but was not usable.""" + return self.state in FAULT_STATES + + def summary(self) -> str: + """One line a person reads, naming the dataset and its outcome.""" + line = f'{self.key}: {self.state}' + if self.state in (READY, MOVED): + line += f', {len(self.files)} file(s) {self.legacy_dir} -> {self.target_dir}' + if self.detail: + line += f', {self.detail}' + return line + + +@dataclass(frozen=True) +class RelocationReport: + """Every dataset considered, whether or not anything happened to it. + + ``manifest_errors`` is carried beside them because a manifest that failed + to load declares datasets nobody here got to look at. Without it a report + covering nothing would read exactly like a tree with nothing left to move. + """ + + entries: tuple[Relocation, ...] = () + manifest_errors: dict[str, str] = field(default_factory=dict) + + def _in_state(self, *states: str) -> tuple[Relocation, ...]: + return tuple(e for e in self.entries if e.state in states) + + @property + def ok(self) -> bool: + """True when every legacy tree found was dealt with and none was skipped.""" + return not self.faults and not self.manifest_errors + + @property + def ready(self) -> tuple[Relocation, ...]: + """Datasets whose legacy tree checks out and is waiting to be moved.""" + return self._in_state(READY) + + @property + def moved(self) -> tuple[Relocation, ...]: + """Datasets whose files were moved into the current layout.""" + return self._in_state(MOVED) + + @property + def redundant(self) -> tuple[Relocation, ...]: + """Datasets whose old copy is still on disk beside the current one. + + Nothing here removes it, so it is worth counting: this is the disk the + user can reclaim by hand, and it is the whole reason to run the + command against a tree where every dataset has already been refetched. + """ + return tuple(e for e in self.entries if e.state == ALREADY_CURRENT and e.legacy_present) + + @property + def faults(self) -> tuple[Relocation, ...]: + """Legacy trees that are present and could not be moved.""" + return tuple(e for e in self.entries if e.faulty) + + def summary(self) -> str: + """A short report, one line per dataset plus a closing count.""" + lines = [e.summary() for e in sorted(self.entries, key=lambda e: e.key)] + for provider, error in sorted(self.manifest_errors.items()): + lines.append(f'{provider}: MANIFEST UNREADABLE, {error}') + if not lines: + return 'no dataset declares a legacy location' + done, waiting, bad = len(self.moved), len(self.ready), len(self.faults) + closing = f'{done} moved, {waiting} ready to move, {bad} left in place' + if self.redundant: + # Named because it is the disk a user can reclaim by hand, and + # because on a tree where everything has already been refetched it + # is the only thing the run has to tell them. + closing += ( + f'; {len(self.redundant)} dataset(s) still have an old copy on disk, ' + 'which was left alone' + ) + if self.manifest_errors: + # A manifest that did not load may be the one declaring the dataset + # this tree still holds, so the counts above are a floor and saying + # otherwise would be the overstatement the report exists to avoid. + closing += f'; {len(self.manifest_errors)} manifest(s) not read, so this may be partial' + lines.append(closing) + return '\n'.join(lines) + + +def _legacy_locations() -> dict[str, str]: + """Read the shipped table of where each dataset used to live.""" + text = files('fwl_io.data').joinpath(_LAYOUT_RESOURCE).read_text() + return dict(tomllib.loads(text).get('legacy', {})) + + +def _classify(legacy_dir: Path, target_dir: Path, registry: dict[str, str]) -> tuple[str, str]: + """Decide what the two trees on disk allow, without touching either.""" + if _all_match(target_dir, registry): + detail = f'already at {target_dir}' + if legacy_dir.is_dir(): + # Both copies are intact, so the legacy one is redundant rather + # than needed. Naming it is as far as this goes: deleting data the + # user has not asked to lose is not this command's business. + detail += f'; the copy at {legacy_dir} is now redundant and was left alone' + return ALREADY_CURRENT, detail + if not legacy_dir.is_dir(): + return ABSENT, '' + missing = [name for name in registry if not (legacy_dir / name).is_file()] + if missing: + return INCOMPLETE, f'{len(missing)} of {len(registry)} file(s) absent from {legacy_dir}' + try: + wrong = [ + name + for name, digest in registry.items() + if not _hash_matches(legacy_dir / name, digest) + ] + except OSError as exc: + return UNRESOLVABLE, f'cannot read {legacy_dir}: {exc}' + if wrong: + return MISMATCH, f'{len(wrong)} file(s) differ from the registry in {legacy_dir}' + return READY, '' + + +def _all_match(directory: Path, registry: dict[str, str]) -> bool: + """True when ``directory`` already holds every registry file, intact.""" + if not directory.is_dir(): + return False + try: + return all( + (directory / name).is_file() and _hash_matches(directory / name, digest) + for name, digest in registry.items() + ) + except OSError: + return False + + +def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: + """Report what a relocation would do, touching nothing. + + Parameters + ---------- + data_root : str | Path | None + Override for the data root; defaults to the resolved FWL_DATA tree. + + Returns + ------- + RelocationReport + One entry per dataset that declares a legacy location, whether or not + that location exists on this machine. + """ + from fwl_io.manifest import _discover + + root = resolve_data_root(data_root) + locations = _legacy_locations() + entries: list[Relocation] = [] + seen: set[str] = set() + providers, manifest_errors = _discover() + for provider_datasets in providers.values(): + for ds in provider_datasets: + legacy = locations.get(ds.key) + if legacy is None or ds.key in seen: + continue + seen.add(ds.key) + legacy_dir = root / legacy + try: + registry = ds.registry() + target_dir = root / _version_dir(ds) + except Exception as exc: # noqa: BLE001 -- reported, never raised + entries.append( + Relocation(ds.key, UNRESOLVABLE, legacy_dir=legacy_dir, detail=str(exc)) + ) + continue + state, detail = _classify(legacy_dir, target_dir, registry) + entries.append( + Relocation( + ds.key, + state, + legacy_dir=legacy_dir, + target_dir=target_dir, + files=tuple(sorted(registry)), + detail=detail, + legacy_present=legacy_dir.is_dir(), + ) + ) + return RelocationReport(tuple(entries), dict(manifest_errors)) + + +def _version_dir(ds: Dataset) -> str: + """The dataset's location below the data root, version directory included.""" + from fwl_io.doi import zenodo_record_id + + return f'{ds.subdir}/r{zenodo_record_id(ds.zenodo)}' + + +def _move_one(entry: Relocation, root: Path) -> Relocation: + """Move one verified legacy tree, leaving nothing half-moved behind.""" + assert entry.legacy_dir is not None and entry.target_dir is not None + done: list[str] = [] + try: + entry.target_dir.mkdir(parents=True, exist_ok=True) + for name in entry.files: + destination = entry.target_dir / name + destination.parent.mkdir(parents=True, exist_ok=True) + os.replace(entry.legacy_dir / name, destination) + done.append(name) + except OSError as exc: + # Put back what was moved, so a failure part way leaves the tree as it + # was rather than split across two layouts, which is the one state + # neither the reader nor the fetcher knows how to interpret. + for name in done: + try: + os.replace(entry.target_dir / name, entry.legacy_dir / name) + except OSError: + log.error('could not restore %s to %s', name, entry.legacy_dir) + return Relocation( + entry.key, + FAILED, + legacy_dir=entry.legacy_dir, + target_dir=entry.target_dir, + files=entry.files, + detail=str(exc), + ) + _prune(entry.legacy_dir, root) + log.info('relocated %s to %s', entry.key, entry.target_dir) + return Relocation( + entry.key, + MOVED, + legacy_dir=entry.legacy_dir, + target_dir=entry.target_dir, + files=entry.files, + ) + + +def _prune(directory: Path, root: Path) -> None: + """Remove the emptied legacy directory, and any parent it leaves empty. + + Only ever removes a directory with nothing in it, so no data can be lost + here, and the walk upward stops at the data root: the root itself is not a + leftover of the previous layout and other datasets live beside it. + """ + root = root.resolve() + while directory.resolve() != root and directory.resolve().is_relative_to(root): + if any(directory.iterdir()): + return + try: + directory.rmdir() + except OSError: + return + directory = directory.parent + + +def relocate(data_root: str | Path | None = None, dry_run: bool = False) -> RelocationReport: + """Move every legacy tree that checks out into the current layout. + + A dataset is moved only when every file its registry declares is present + in the legacy location and matches its recorded digest. Anything else is + reported and left untouched, including a dataset already at its current + location, which is the ordinary state once a fetch has happened there. + + Parameters + ---------- + data_root : str | Path | None + Override for the data root; defaults to the resolved FWL_DATA tree. + dry_run : bool + Report what would move without moving it. + + Returns + ------- + RelocationReport + The plan, with each moved dataset's entry rewritten to say so. + """ + plan = plan_relocations(data_root) + if dry_run: + return plan + root = resolve_data_root(data_root) + return RelocationReport( + tuple(_move_one(e, root) if e.state == READY else e for e in plan.entries), + dict(plan.manifest_errors), + ) diff --git a/tests/test_fetch.py b/tests/test_fetch.py index 26460ae..db13e02 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -1,5 +1,6 @@ import io import json +import logging import socket import tarfile import zipfile @@ -737,6 +738,42 @@ def test_a_stamp_that_is_not_an_object_is_healed_not_raised( _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all(offline=True) +def test_a_stamp_at_an_unknown_schema_costs_a_refetch_and_says_so(http_server, tmp_path, caplog): + """An unreadable schema means the whole dataset comes down again, loudly. + + This is the price of refusing to read a stamp written to rules this + version does not know, and it is the right price: the alternative is + reading fields whose meaning may have changed. It is worth naming in the + log because two versions sharing one data root will pay it on every run, + and a nightly job redownloading the same tree gives no other clue why. + """ + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + version_dir = tmp_path / VERSIONED + stamp_path = version_dir / '.fwl-io.json' + record = json.loads(stamp_path.read_text()) + assert record['schema'] == 1, 'the fixture must start from a stamp this version wrote' + + # A later version stamps the same intact tree to rules this one lacks. + stamp_path.write_text(json.dumps(dict(record, schema=2, added_later='meaning-changed'))) + with caplog.at_level(logging.WARNING, logger='fwl.fwl_io.fetch'): + paths = _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + healed = json.loads(stamp_path.read_text()) + assert healed['schema'] == 1, 'the tree is restamped to the schema this version writes' + assert 'added_later' not in healed, 'the unreadable stamp is replaced, not edited' + assert any('schema 2' in r.getMessage() for r in caplog.records), ( + 'the refetch has to name the schema that caused it' + ) + + # Discrimination: with the stamp left alone the same call serves locally, + # so the refetch above is the schema and not the fetch path in general. + offline = _archive_fetcher('http://127.0.0.1:1/', registry, tmp_path, 'tar') + assert sorted(p.name for p in offline.fetch_all(offline=True)) == ['m0p1.txt', 'm1p0.txt'] + + def test_a_cache_stamp_naming_members_outside_the_cache_is_refused( http_server, tmp_path, monkeypatch ): diff --git a/tests/test_relocate.py b/tests/test_relocate.py new file mode 100644 index 0000000..2786377 --- /dev/null +++ b/tests/test_relocate.py @@ -0,0 +1,302 @@ +"""Tests for :mod:`fwl_io.relocate`, moving a legacy tree into the current layout. + +The contract exercised here is that files move only when they are provably the +files the registry describes, that everything else is reported and left exactly +as it was, and that a command whose whole job is moving data on disk cannot +delete anything the user still has. + +No test here touches the network. Every fetcher a check might build is pointed +at a closed port, and the relocation path itself never downloads. +""" + +from __future__ import annotations + +import hashlib + +import pytest + +from fwl_io.relocate import ( + ABSENT, + ALREADY_CURRENT, + INCOMPLETE, + MISMATCH, + MOVED, + READY, + UNRESOLVABLE, + plan_relocations, + relocate, +) + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + +# A dataset that really did move, so the legacy path under test is the one the +# shipped table records rather than one invented for the test. +KEY = 'star.tracks.baraffe_2015' +RECID = '15729114' +ZENODO = f'10.5281/zenodo.{RECID}' +LEGACY = 'stellar_evolution_tracks/Baraffe' +TARGET = f'star/tracks/baraffe_2015/r{RECID}' + +# Two files of different lengths and contents, so a move that paired a name +# with the wrong digest could not pass by coincidence. +CONTENTS = {'BHAC15_tracks.dat': b'0.1 0.2 0.3\n', 'notes.txt': b'9.87\n'} + + +def _digests(names=None): + chosen = CONTENTS if names is None else {n: CONTENTS[n] for n in names} + return {n: f'sha256:{hashlib.sha256(b).hexdigest()}' for n, b in chosen.items()} + + +def _install_manifest(monkeypatch, tmp_path, *, with_registry=True): + """Install a manifest declaring the migrated dataset, and its registry.""" + manifest = tmp_path / 'manifest.toml' + manifest.write_text(f'[{KEY}]\nzenodo = "{ZENODO}"\nrequired_by = ["mors"]\n') + if with_registry: + lines = ''.join(f'{n} {d}\n' for n, d in sorted(_digests().items())) + (tmp_path / f'{KEY}.registry.txt').write_text(lines) + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + return manifest + + +def _populate(directory, names=None, corrupt=()): + directory.mkdir(parents=True, exist_ok=True) + for name in CONTENTS if names is None else names: + body = b'not the recorded contents\n' if name in corrupt else CONTENTS[name] + (directory / name).write_bytes(body) + + +def test_a_verified_legacy_tree_moves_and_leaves_nothing_behind(tmp_path, monkeypatch): + """Files that match the registry move, and the emptied directories go. + + Tidying the tree is the whole point of the command, so an empty + ``stellar_evolution_tracks`` left standing afterwards would mean the user + still has to finish the job by hand. + """ + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / LEGACY) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [MOVED] + for name, body in CONTENTS.items(): + assert (root / TARGET / name).read_bytes() == body, 'contents must survive the move' + assert not (root / LEGACY).exists() + assert not (root / 'stellar_evolution_tracks').exists(), 'the emptied parent goes too' + assert root.is_dir(), 'the data root is not a leftover of the old layout' + + +def test_a_file_that_differs_from_the_registry_stops_the_move(tmp_path, monkeypatch): + """A tree that is not what it claims is reported, and nothing is touched. + + Moving first and hashing afterwards would turn a stale copy into a stale + copy at the location the fetcher now trusts, which is worse than leaving + it where a reader can still tell it is old. + """ + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / LEGACY, corrupt=['notes.txt']) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [MISMATCH] + assert report.faults, 'a legacy tree that cannot be moved has to fail the run' + assert (root / LEGACY / 'notes.txt').read_bytes() == b'not the recorded contents\n' + assert (root / LEGACY / 'BHAC15_tracks.dat').is_file(), 'the sound file stays put as well' + assert not (root / TARGET).exists(), 'no half-move: the target is not created' + + +def test_a_legacy_tree_missing_a_file_is_reported_not_half_moved(tmp_path, monkeypatch): + """An incomplete tree is left whole rather than partly relocated. + + The edge case that matters: moving the files that are present would leave + the dataset split across two layouts, which is the one state neither the + reader nor the fetcher can interpret. + """ + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / LEGACY, names=['BHAC15_tracks.dat']) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [INCOMPLETE] + assert '1 of 2 file(s) absent' in report.entries[0].detail + assert (root / LEGACY / 'BHAC15_tracks.dat').is_file() + assert not (root / TARGET).exists() + + +def test_a_tree_already_at_its_current_location_is_left_alone(tmp_path, monkeypatch): + """A dataset that has already moved is not a fault, and the old copy survives. + + The redundant copy is named so the user can remove it, and not removed + here: deleting data nobody asked to lose is not this command's business. + """ + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / TARGET) + _populate(root / LEGACY) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [ALREADY_CURRENT] + assert not report.faults, 'an already-tidy tree is a success' + assert (root / LEGACY / 'notes.txt').is_file(), 'the redundant copy is reported, not deleted' + assert 'redundant' in report.entries[0].detail + # The closing line is all some readers see, and on a tree where everything + # has already been refetched the old copies are the only thing to say. + assert len(report.redundant) == 1 + assert 'still have an old copy on disk' in report.summary() + + +def test_a_current_tree_with_no_old_copy_beside_it_reports_nothing_to_reclaim( + tmp_path, monkeypatch +): + """Without the old directory there is no disk to reclaim, and none is claimed. + + The discriminating half of the case above: both report ``already-current``, + so only the count separates a tree that still carries a duplicate from one + that is genuinely finished. + """ + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / TARGET) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [ALREADY_CURRENT] + assert report.redundant == () + assert 'still have an old copy on disk' not in report.summary() + assert 'redundant' not in report.entries[0].detail + + +def test_nothing_on_disk_is_reported_absent_rather_than_missing(tmp_path, monkeypatch): + """A machine that never had the legacy layout has nothing to relocate.""" + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [ABSENT] + assert not report.faults, 'never having had the old layout is not a fault' + assert report.moved == () and report.ready == () + assert list(root.iterdir()) == [], 'a relocation must not create the tree it inspects' + + +def test_a_dry_run_reports_the_move_without_making_it(tmp_path, monkeypatch): + """The plan names what would move, and the tree is untouched afterwards.""" + _install_manifest(monkeypatch, tmp_path) + root = tmp_path / 'data' + _populate(root / LEGACY) + before = {p.name: p.read_bytes() for p in sorted((root / LEGACY).iterdir())} + + report = relocate(data_root=root, dry_run=True) + + assert [e.state for e in report.entries] == [READY] + assert report.ready and not report.moved + after = {p.name: p.read_bytes() for p in sorted((root / LEGACY).iterdir())} + assert after == before + assert not (root / TARGET).exists() + + # Discrimination: the same call without dry_run does move it, so the + # assertions above pin the flag and not some other refusal. + assert relocate(data_root=root).moved + assert (root / TARGET / 'notes.txt').is_file() + + +def test_a_dataset_whose_registry_is_missing_is_reported_not_moved(tmp_path, monkeypatch): + """Without a registry there is nothing to verify against, so nothing moves. + + The files may well be the right ones, but a relocation that assumed so + would be moving on the strength of a directory name. + """ + _install_manifest(monkeypatch, tmp_path, with_registry=False) + root = tmp_path / 'data' + _populate(root / LEGACY) + + report = relocate(data_root=root) + + assert [e.state for e in report.entries] == [UNRESOLVABLE] + assert report.faults + assert (root / LEGACY / 'notes.txt').is_file(), 'an unverifiable tree stays where it is' + assert not (root / TARGET).exists() + + +def test_a_manifest_that_did_not_load_keeps_the_report_from_reading_complete(tmp_path, monkeypatch): + """An unread manifest may be the one declaring the tree still sitting there. + + Nothing moved and nothing was found, which on its own is exactly what a + finished tree looks like. The manifest that failed has to be carried, or + the command reports success over data it never considered. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text('this is not valid toml [[[\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + root = tmp_path / 'data' + _populate(root / LEGACY) + + report = relocate(data_root=root) + + assert report.entries == (), 'no dataset was declared, so none could be considered' + assert list(report.manifest_errors) == ['demoprovider'] + assert not report.ok, 'a report that looked at nothing must not read as a tidy tree' + assert 'MANIFEST UNREADABLE' in report.summary() + assert 'may be partial' in report.summary() + assert (root / LEGACY / 'notes.txt').is_file(), 'the tree it could not judge is untouched' + + +def test_a_dataset_with_no_legacy_location_is_not_considered(tmp_path, monkeypatch): + """A dataset created after the migration has no old location to leave. + + Its absence from the report is the point: the command speaks only about + datasets that predate the layout, so a clean install has nothing to say. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text('[atmos_chem.networks.demo]\nzenodo = "10.5281/zenodo.1234567"\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + report = plan_relocations(data_root=tmp_path / 'data') + + assert report.entries == () + assert report.summary() == 'no dataset declares a legacy location' + + +def test_the_shipped_table_names_only_datasets_and_relative_locations(): + """Every entry is a dotted key and a location inside the data root. + + An absolute path or one climbing out of the root would be joined onto the + tree and then walked, so the table is held to the same suspicion as any + other file the package reads. + """ + from pathlib import Path + + from fwl_io.relocate import _legacy_locations + + table = _legacy_locations() + + assert table, 'the table has to declare the datasets that have already moved' + for key, location in table.items(): + assert '.' in key, f'{key!r} is not a dotted dataset key' + assert not Path(location).is_absolute(), f'{location!r} is absolute' + assert '..' not in Path(location).parts, f'{location!r} climbs out of the data root' + assert location.strip('/') == location, f'{location!r} is not a clean relative path' From bee36a73e3e6b48f00cfe03661471565f0fee756 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 16:45:30 +0200 Subject: [PATCH 08/16] Trim two comments to the length they need --- src/fwl_io/check.py | 9 +++------ tests/test_fetch.py | 9 +++------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/fwl_io/check.py b/src/fwl_io/check.py index 6c8dc6d..c2b4c32 100644 --- a/src/fwl_io/check.py +++ b/src/fwl_io/check.py @@ -44,12 +44,9 @@ UNREADABLE = 'unreadable' PRESENT = 'present' -#: States that mean the tree is not usable as the manifest describes it, each -#: with the word the report prints for it. A file that cannot be read counts: -#: whether its contents are right is unknown, and a check reports what it could -#: not establish rather than assuming the best. The summary counts these by -#: walking this mapping, so a state added here is named in the report rather -#: than failing a dataset for a reason the text never gives. +#: States that make the tree unusable, each with the word the report prints. +#: Unreadable counts: its contents are unknown, not correct. The summary walks +#: this mapping, so a state added here is named rather than silently dropped. FAULT_LABELS = {MISSING: 'missing', MISMATCH: 'corrupt', UNREADABLE: 'unreadable'} FAULT_STATES = tuple(FAULT_LABELS) diff --git a/tests/test_fetch.py b/tests/test_fetch.py index db13e02..44985ea 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -726,12 +726,9 @@ def test_a_stamp_that_is_not_an_object_is_healed_not_raised( assert healed['record_id'] == RECID, 'the unusable stamp is replaced by a real one' assert healed['members'] == ['m0p1.txt', 'nested/m1p0.txt'] - # Discrimination: put the unusable stamp back over the tree that is now - # fully populated, and go offline. The members are all on disk, so the only - # thing that can decide the dataset is unservable is the stamp, and the - # answer has to be the honest "nothing here to serve" rather than an error - # about the shape of a provenance file. Pointing this at an empty root - # instead would raise the same error whatever the stamp reader did. + # Discrimination: the members are all on disk now, so only the stamp can + # make the dataset unservable. Pointing this at an empty root instead would + # raise the same error whatever the stamp reader did. (version_dir / '.fwl-io.json').write_text(stamp_body) assert (version_dir / 'm0p1.txt').is_file(), 'the tree must be intact, or this proves nothing' with pytest.raises(OfflineDataError): From e7b88e56ffd0656100a36cc180f64055402dac88 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 18:40:54 +0200 Subject: [PATCH 09/16] Serialise an archive rebuild the way a file download already is Fetching one file takes a per-target lock so a burst of processes that all miss it does not hit the mirrors at once. Rebuilding an archive dataset took no lock at all, although it is the more dangerous of the two: it replaces the whole version directory, so two processes doing it together move a tree out from under each other while a third is reading it. These are the shared filesystems this package is built for, and the concurrency note in the module header only ever described the per-file path. The lock is keyed on the archive, so unrelated datasets still fetch in parallel, and an intact tree is still served on the fast path without waiting for anything: the common case pays nothing. A process that did wait re-checks the tree before rebuilding, so it serves what the winner built instead of doing the same work again, which is the difference between suppressing a herd and staggering one. Where the lock cannot be taken at all, an archive fetch degrades to an unguarded one exactly as a file fetch does. It never carried correctness, only politeness towards the mirrors, and a mount with no lock manager has to keep working. --- src/fwl_io/fetch.py | 18 ++++++++++ tests/test_fetch.py | 86 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/src/fwl_io/fetch.py b/src/fwl_io/fetch.py index 1aa89bb..b4379e6 100644 --- a/src/fwl_io/fetch.py +++ b/src/fwl_io/fetch.py @@ -627,12 +627,30 @@ def _fetch_archive(self, offline: bool | None = None) -> list[Path]: the per-file path it does not re-hash contents (the archive-only checksum policy records member names, not per-file digests). Extraction is staged and the tree is moved into place atomically. + + Serialised per dataset like the per-file path, and for a sharper + reason: rebuilding replaces the whole version directory, so two + processes doing it at once would move a tree in from under each other + while a third reads it. Unrelated datasets still fetch in parallel. """ archive_name, known_hash = next(iter(self.registry.items())) if self._stamp_is_current(self.target_dir) and self._archive_tree_intact(): self._sources.setdefault(archive_name, 'local') return self._extracted_files() + with self._fetch_lock(archive_name, self.target_dir): + return self._rebuild_archive(archive_name, known_hash, offline) + + def _rebuild_archive( + self, archive_name: str, known_hash: str, offline: bool | None + ) -> list[Path]: + """Populate the version directory, with the dataset's lock already held.""" + # Re-check under the lock: another process may have finished the whole + # rebuild while this one waited for it. + if self._stamp_is_current(self.target_dir) and self._archive_tree_intact(): + self._sources.setdefault(archive_name, 'local') + return self._extracted_files() + cached_archive = self._cached_archive(archive_name, known_hash) if cached_archive is None and self._copy_cached_tree(): self._sources[archive_name] = f'cache:{resolve_cache_root()}' diff --git a/tests/test_fetch.py b/tests/test_fetch.py index 44985ea..757545a 100644 --- a/tests/test_fetch.py +++ b/tests/test_fetch.py @@ -735,6 +735,92 @@ def test_a_stamp_that_is_not_an_object_is_healed_not_raised( _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all(offline=True) +def test_an_archive_rebuild_takes_the_lock_and_an_intact_tree_does_not( + http_server, tmp_path, monkeypatch +): + """Rebuilding is serialised per dataset; serving an intact tree is not. + + A rebuild replaces the whole version directory, so two processes doing it + at once would move a tree out from under each other. The second half is + what keeps that from costing anything: the common case, where the data is + already there, must not queue behind a lock. + """ + from filelock import FileLock + + taken = [] + acquire = FileLock.acquire + + def spy(self, *args, **kwargs): + taken.append(self.lock_file) + return acquire(self, *args, **kwargs) + + monkeypatch.setattr(FileLock, 'acquire', spy) + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + + _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + assert len(taken) == 1, 'the rebuild has to be serialised' + + taken.clear() + paths = _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + assert taken == [], 'an intact tree is served without waiting for anything' + + +def test_a_rebuild_rechecks_the_tree_under_the_lock(http_server, tmp_path, monkeypatch): + """Whoever waited for the lock serves what the winner built, not a second copy. + + Without the re-check, every process queued behind a rebuild would redo it + in turn, which is the thundering herd the lock exists to stop rather than + merely stagger. + """ + import shutil + from contextlib import contextmanager + + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + built = tmp_path / VERSIONED + + # A mirror that cannot answer, so anything but the re-check fails loudly. + fetcher = _archive_fetcher('http://127.0.0.1:1/', registry, tmp_path / 'other', 'tar') + + @contextmanager + def another_process_finishes_first(fname, target): + shutil.copytree(built, fetcher.target_dir) + yield + + monkeypatch.setattr(fetcher, '_fetch_lock', another_process_finishes_first) + paths = fetcher.fetch_all(offline=True) + + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + assert fetcher.provenance()[0]['source'] == 'local', 'it served the tree, it did not rebuild' + + +def test_an_archive_fetch_proceeds_when_the_lock_manager_is_unavailable( + http_server, tmp_path, monkeypatch +): + """A filesystem without a working lock manager still fetches an archive. + + The lock never carries correctness, only politeness towards the mirrors, + so an ENOLCK mount degrades to an unguarded fetch exactly as the per-file + path does rather than failing the dataset. + """ + from filelock import FileLock + + def enolck(self, *args, **kwargs): + raise OSError(37, 'No locks available') + + monkeypatch.setattr(FileLock, 'acquire', enolck) + base_url, root = http_server + registry = _serve_archive(root, 'tracks.tar', ARCHIVE_MEMBERS, 'tar') + + paths = _archive_fetcher(base_url, registry, tmp_path, 'tar').fetch_all() + + assert sorted(p.name for p in paths) == ['m0p1.txt', 'm1p0.txt'] + assert (tmp_path / VERSIONED / '.fwl-io.json').is_file(), 'the stamp is still written' + + def test_a_stamp_at_an_unknown_schema_costs_a_refetch_and_says_so(http_server, tmp_path, caplog): """An unreadable schema means the whole dataset comes down again, loudly. From 2c0d78e0bfbe483d2a6d6be280f6110370fae7f3 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 19:30:33 +0200 Subject: [PATCH 10/16] Keep relocate inside the data root and finish what it empties Four faults in a command whose whole job is moving files, so each one mattered more than its size suggests. A legacy location naming an absolute path, or climbing out with `..`, was joined onto the root and followed, so files were moved into the tree from outside it. The shipped table was trusted further than a provenance stamp is, which is backwards: the table is a file too, and it becomes the path files are moved out of. Entries that are not inside the root are now dropped when the table is read, the planner refuses a directory that resolves outside, and the move refuses again on its own account, so that guarantee belongs to the code doing the moving rather than to whoever called it. A symlinked legacy directory escapes the same way and only shows it when the path is resolved, which is why the check is on the resolved path. A registry name may nest. Moving `sub/nested.dat` emptied `sub/` and left it standing, which kept the whole legacy directory alive, and a later run then reported that husk as an old copy still holding data when it held nothing. Emptied subdirectories now go before the walk upward. A rollback that could not put a file back logged and carried on, leaving the dataset in both places at once: the exact state the rollback exists to prevent, reported as an ordinary failure. That is now a state of its own, because a rerun fixes a failure and does not fix this, and the run stops there instead of moving more data past a tree somebody has to look at. The command exited 0 when a manifest could not be read, although the summary on the same run said the pass may be partial and `ok` was already false. Anything reading the exit code was told a run that looked at nothing was clean. Also renames the entry point to `relocate_all`, since a module and a function of the same name shadow each other on the package, and every other module here already avoids that: `fetch_for`, `sync_manifest`, `mirror_to_dataverse`. --- docs/Reference/api/index.md | 4 +- docs/Reference/cli.md | 4 +- src/fwl_io/__init__.py | 4 +- src/fwl_io/cli.py | 11 ++- src/fwl_io/relocate.py | 106 ++++++++++++++++++++--- tests/test_cli.py | 59 +++++++++++++ tests/test_relocate.py | 168 +++++++++++++++++++++++++++++++++--- 7 files changed, 320 insertions(+), 36 deletions(-) diff --git a/docs/Reference/api/index.md b/docs/Reference/api/index.md index fe0f533..be978b1 100644 --- a/docs/Reference/api/index.md +++ b/docs/Reference/api/index.md @@ -8,7 +8,7 @@ from fwl_io import ( load_manifest, discover_manifests, # manifests fetch_for, Dataset, check_for, check_dataset, # validate-only checking - relocate, plan_relocations, # moving a legacy tree into the current layout + relocate_all, plan_relocations, # moving a legacy tree into the current layout CheckReport, DatasetCheck, FileCheck, resolve_data_root, resolve_cache_root, DownloadError, OfflineDataError, MissingDataRootError, @@ -20,7 +20,7 @@ Per-module reference pages: - [Fetching](fetch.md): `Fetcher`, `create_fetcher`, error types - [Checking](check.md): `check_for`, `check_dataset`, `CheckReport`, `DatasetCheck`, `FileCheck` -- [Relocating](relocate.md): `relocate`, `plan_relocations`, `RelocationReport`, `Relocation` +- [Relocating](relocate.md): `relocate_all`, `plan_relocations`, `RelocationReport`, `Relocation` - [Manifests](manifest.md): `Dataset`, `load_manifest`, `discover_manifests`, `fetch_for`, `ManifestSchemaError` - [Registries](registry.md): registry file reading and writing - [Sync](sync.md): registry generation from the Zenodo API diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index 099b6f2..e782741 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -1,6 +1,6 @@ # CLI reference -The `fwl-io` command has five subcommands. Failures are reported as concise messages on stderr (never a traceback) and exit with status 1; success exits 0. `sync` and `fetch` aggregate per-dataset failures into a multi-line report, and a download failure lists every mirror attempt. +The `fwl-io` command has six subcommands. Failures are reported as concise messages on stderr (never a traceback) and exit with status 1; success exits 0. `sync` and `fetch` aggregate per-dataset failures into a multi-line report, and a download failure lists every mirror attempt. ## fwl-io sync @@ -54,7 +54,7 @@ A dataset moves only when every file its registry declares is present in the old A dataset already at its current location is not a fault, and a copy still sitting at the old location beside it is named rather than deleted. Nothing here removes data: the only directories it removes are ones it has just emptied itself. -Exit is 1 when a legacy tree was found and could not be moved, or when an installed manifest could not be read, since that manifest may be the one declaring the dataset a tree still holds. A tree that was already tidy exits 0. `--dry-run` reports the same plan without moving anything. The equivalent Python entry points are `fwl_io.relocate` and `fwl_io.plan_relocations`. +Exit is 1 when a legacy tree was found and could not be moved, or when an installed manifest could not be read, since that manifest may be the one declaring the dataset a tree still holds. A tree that was already tidy exits 0. `--dry-run` reports the same plan without moving anything. The equivalent Python entry points are `fwl_io.relocate_all` and `fwl_io.plan_relocations`. ## fwl-io mirror diff --git a/src/fwl_io/__init__.py b/src/fwl_io/__init__.py index 6926bb6..50e3a3e 100644 --- a/src/fwl_io/__init__.py +++ b/src/fwl_io/__init__.py @@ -19,7 +19,7 @@ load_manifest, ) from fwl_io.paths import MissingDataRootError, resolve_cache_root, resolve_data_root -from fwl_io.relocate import Relocation, RelocationReport, plan_relocations, relocate +from fwl_io.relocate import Relocation, RelocationReport, plan_relocations, relocate_all try: __version__ = version('fwl-io') @@ -46,7 +46,7 @@ 'fetch_for', 'load_manifest', 'plan_relocations', - 'relocate', + 'relocate_all', 'resolve_cache_root', 'resolve_data_root', ] diff --git a/src/fwl_io/cli.py b/src/fwl_io/cli.py index ae5af93..5c4cd46 100644 --- a/src/fwl_io/cli.py +++ b/src/fwl_io/cli.py @@ -64,14 +64,13 @@ def _cmd_check(args: argparse.Namespace) -> int: def _cmd_relocate(args: argparse.Namespace) -> int: - from fwl_io.relocate import relocate + from fwl_io.relocate import relocate_all - report = relocate(data_root=args.data_root, dry_run=args.dry_run) + report = relocate_all(data_root=args.data_root, dry_run=args.dry_run) print(report.summary()) - # A tree that was already tidy is a success, so only a legacy tree that - # could not be moved fails the command. Nothing was changed in that case, - # which is what the exit code has to make actionable. - return 1 if report.faults else 0 + # Matches the summary: an unread manifest may be the one declaring the + # tree still sitting there, so it cannot exit as a clean run either. + return 0 if report.ok else 1 def _cmd_mirror(args: argparse.Namespace) -> int: diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py index 77f5ef2..f666afe 100644 --- a/src/fwl_io/relocate.py +++ b/src/fwl_io/relocate.py @@ -51,9 +51,10 @@ UNRESOLVABLE = 'unresolvable' MOVED = 'moved' FAILED = 'failed' +SPLIT = 'split' #: States that mean a legacy tree is there but cannot be moved as it stands. -FAULT_STATES = (INCOMPLETE, MISMATCH, UNRESOLVABLE, FAILED) +FAULT_STATES = (INCOMPLETE, MISMATCH, UNRESOLVABLE, FAILED, SPLIT) @dataclass(frozen=True) @@ -155,9 +156,31 @@ def summary(self) -> str: def _legacy_locations() -> dict[str, str]: - """Read the shipped table of where each dataset used to live.""" + """Read the shipped table of where each dataset used to live. + + An entry naming an absolute path or climbing out of the data root is + dropped. The table ships with the package, but it is still a file being + turned into a path that files get moved out of, so it earns the same + suspicion as a name inside a provenance stamp. + """ text = files('fwl_io.data').joinpath(_LAYOUT_RESOURCE).read_text() - return dict(tomllib.loads(text).get('legacy', {})) + table = dict(tomllib.loads(text).get('legacy', {})) + safe = {} + for key, location in table.items(): + parts = Path(location).parts + if not isinstance(location, str) or Path(location).is_absolute() or '..' in parts: + log.warning('legacy location for %s is not inside the data root: %r', key, location) + continue + safe[key] = location + return safe + + +def _inside(path: Path, root: Path) -> bool: + """True when ``path`` resolves within ``root``, symlinks followed.""" + try: + return path.resolve().is_relative_to(root.resolve()) + except OSError: + return False def _classify(legacy_dir: Path, target_dir: Path, registry: dict[str, str]) -> tuple[str, str]: @@ -229,6 +252,18 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: continue seen.add(ds.key) legacy_dir = root / legacy + if legacy_dir.exists() and not _inside(legacy_dir, root): + # A symlink is the way this happens in a real tree: the joined + # path is clean, and only resolving it shows it leaves the root. + entries.append( + Relocation( + ds.key, + UNRESOLVABLE, + legacy_dir=legacy_dir, + detail=f'{legacy_dir} resolves outside the data root {root}', + ) + ) + continue try: registry = ds.registry() target_dir = root / _version_dir(ds) @@ -262,6 +297,17 @@ def _version_dir(ds: Dataset) -> str: def _move_one(entry: Relocation, root: Path) -> Relocation: """Move one verified legacy tree, leaving nothing half-moved behind.""" assert entry.legacy_dir is not None and entry.target_dir is not None + if not _inside(entry.legacy_dir, root) or not _inside(entry.target_dir, root): + # Checked here as well as when the plan is built, so the guarantee that + # this only ever moves files inside the data root belongs to the code + # that does the moving rather than to whoever called it. + return Relocation( + entry.key, + UNRESOLVABLE, + legacy_dir=entry.legacy_dir, + target_dir=entry.target_dir, + detail=f'refusing to move files outside the data root {root}', + ) done: list[str] = [] try: entry.target_dir.mkdir(parents=True, exist_ok=True) @@ -272,13 +318,29 @@ def _move_one(entry: Relocation, root: Path) -> Relocation: done.append(name) except OSError as exc: # Put back what was moved, so a failure part way leaves the tree as it - # was rather than split across two layouts, which is the one state - # neither the reader nor the fetcher knows how to interpret. + # was rather than split across two layouts. + unrestored = [] for name in done: try: os.replace(entry.target_dir / name, entry.legacy_dir / name) except OSError: - log.error('could not restore %s to %s', name, entry.legacy_dir) + unrestored.append(name) + if unrestored: + # The state the rollback exists to prevent, reached anyway. It is + # reported as its own thing because the remedy is a person looking + # at two directories, not a rerun. + log.error('could not restore %s to %s', ', '.join(unrestored), entry.legacy_dir) + return Relocation( + entry.key, + SPLIT, + legacy_dir=entry.legacy_dir, + target_dir=entry.target_dir, + files=entry.files, + detail=( + f'{exc}; {len(unrestored)} file(s) could not be put back, so this ' + f'dataset is now split between {entry.legacy_dir} and {entry.target_dir}' + ), + ) return Relocation( entry.key, FAILED, @@ -299,13 +361,22 @@ def _move_one(entry: Relocation, root: Path) -> Relocation: def _prune(directory: Path, root: Path) -> None: - """Remove the emptied legacy directory, and any parent it leaves empty. + """Remove the emptied legacy tree, and any parent it leaves empty. Only ever removes a directory with nothing in it, so no data can be lost here, and the walk upward stops at the data root: the root itself is not a - leftover of the previous layout and other datasets live beside it. + leftover of the previous layout and other datasets live beside it. A + registry name may nest, so the emptied subdirectories inside go first, or + the husk they leave keeps the whole legacy directory standing. """ root = root.resolve() + if directory.is_dir(): + for path in sorted(directory.rglob('*'), key=lambda p: len(p.parts), reverse=True): + if path.is_dir() and not any(path.iterdir()): + try: + path.rmdir() + except OSError: + return while directory.resolve() != root and directory.resolve().is_relative_to(root): if any(directory.iterdir()): return @@ -316,7 +387,7 @@ def _prune(directory: Path, root: Path) -> None: directory = directory.parent -def relocate(data_root: str | Path | None = None, dry_run: bool = False) -> RelocationReport: +def relocate_all(data_root: str | Path | None = None, dry_run: bool = False) -> RelocationReport: """Move every legacy tree that checks out into the current layout. A dataset is moved only when every file its registry declares is present @@ -340,7 +411,16 @@ def relocate(data_root: str | Path | None = None, dry_run: bool = False) -> Relo if dry_run: return plan root = resolve_data_root(data_root) - return RelocationReport( - tuple(_move_one(e, root) if e.state == READY else e for e in plan.entries), - dict(plan.manifest_errors), - ) + done, halted = [], False + for entry in plan.entries: + if entry.state != READY or halted: + done.append(entry) + continue + moved = _move_one(entry, root) + done.append(moved) + if moved.state in (FAILED, SPLIT): + # Stop rather than move more data past a tree that is already in a + # state somebody has to look at. + log.error('stopping after %s could not be relocated', moved.key) + halted = True + return RelocationReport(tuple(done), dict(plan.manifest_errors)) diff --git a/tests/test_cli.py b/tests/test_cli.py index c0ad397..a6a7cd5 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -208,3 +208,62 @@ def load(self): assert 'presence only' in out, 'the archive dataset has to say what it could not check' assert 'all data present, 1 dataset(s) by presence only' in out assert 'and verified' not in out, 'one presence-only dataset forfeits the stronger claim' + + +@pytest.mark.unit +def test_relocate_exits_nonzero_when_a_manifest_could_not_be_read(tmp_path, capsys, monkeypatch): + """A run that could not read a manifest is not a clean run. + + Nothing moved and nothing was found, which on its own is what a finished + tree looks like. The manifest that failed may be the one declaring the + dataset whose old directory is still sitting there, so automation reading + only the exit code must not be told this pass was complete. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text('this is not valid toml [[[\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + data_root = tmp_path / 'data' + + code = main(['relocate', '--data-root', str(data_root)]) + out = capsys.readouterr().out + + assert code == 1, 'an unread manifest cannot exit as success' + assert 'MANIFEST UNREADABLE' in out + assert 'may be partial' in out + + +@pytest.mark.unit +def test_relocate_exits_zero_on_a_tree_with_nothing_to_move(tmp_path, capsys, monkeypatch): + """A machine that never had the old layout is a success, not a fault. + + The discriminating half of the case above: both runs move nothing and + report no faults, so only the manifest error separates them, and the exit + code has to follow that rather than the move count. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text('[star.tracks.baraffe_2015]\nzenodo = "10.5281/zenodo.15729114"\n') + (tmp_path / 'star.tracks.baraffe_2015.registry.txt').write_text( + 'a.dat sha256:' + 'a' * 64 + '\n' + ) + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + code = main(['relocate', '--data-root', str(tmp_path / 'data')]) + out = capsys.readouterr().out + + assert code == 0 + assert 'MANIFEST UNREADABLE' not in out + assert 'absent' in out diff --git a/tests/test_relocate.py b/tests/test_relocate.py index 2786377..ed362dc 100644 --- a/tests/test_relocate.py +++ b/tests/test_relocate.py @@ -23,8 +23,9 @@ MOVED, READY, UNRESOLVABLE, + Relocation, plan_relocations, - relocate, + relocate_all, ) pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] @@ -83,7 +84,7 @@ def test_a_verified_legacy_tree_moves_and_leaves_nothing_behind(tmp_path, monkey root = tmp_path / 'data' _populate(root / LEGACY) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [MOVED] for name, body in CONTENTS.items(): @@ -104,7 +105,7 @@ def test_a_file_that_differs_from_the_registry_stops_the_move(tmp_path, monkeypa root = tmp_path / 'data' _populate(root / LEGACY, corrupt=['notes.txt']) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [MISMATCH] assert report.faults, 'a legacy tree that cannot be moved has to fail the run' @@ -124,7 +125,7 @@ def test_a_legacy_tree_missing_a_file_is_reported_not_half_moved(tmp_path, monke root = tmp_path / 'data' _populate(root / LEGACY, names=['BHAC15_tracks.dat']) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [INCOMPLETE] assert '1 of 2 file(s) absent' in report.entries[0].detail @@ -143,7 +144,7 @@ def test_a_tree_already_at_its_current_location_is_left_alone(tmp_path, monkeypa _populate(root / TARGET) _populate(root / LEGACY) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [ALREADY_CURRENT] assert not report.faults, 'an already-tidy tree is a success' @@ -168,7 +169,7 @@ def test_a_current_tree_with_no_old_copy_beside_it_reports_nothing_to_reclaim( root = tmp_path / 'data' _populate(root / TARGET) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [ALREADY_CURRENT] assert report.redundant == () @@ -181,7 +182,7 @@ def test_nothing_on_disk_is_reported_absent_rather_than_missing(tmp_path, monkey _install_manifest(monkeypatch, tmp_path) root = tmp_path / 'data' - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [ABSENT] assert not report.faults, 'never having had the old layout is not a fault' @@ -196,7 +197,7 @@ def test_a_dry_run_reports_the_move_without_making_it(tmp_path, monkeypatch): _populate(root / LEGACY) before = {p.name: p.read_bytes() for p in sorted((root / LEGACY).iterdir())} - report = relocate(data_root=root, dry_run=True) + report = relocate_all(data_root=root, dry_run=True) assert [e.state for e in report.entries] == [READY] assert report.ready and not report.moved @@ -206,7 +207,7 @@ def test_a_dry_run_reports_the_move_without_making_it(tmp_path, monkeypatch): # Discrimination: the same call without dry_run does move it, so the # assertions above pin the flag and not some other refusal. - assert relocate(data_root=root).moved + assert relocate_all(data_root=root).moved assert (root / TARGET / 'notes.txt').is_file() @@ -220,7 +221,7 @@ def test_a_dataset_whose_registry_is_missing_is_reported_not_moved(tmp_path, mon root = tmp_path / 'data' _populate(root / LEGACY) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert [e.state for e in report.entries] == [UNRESOLVABLE] assert report.faults @@ -248,7 +249,7 @@ def load(self): root = tmp_path / 'data' _populate(root / LEGACY) - report = relocate(data_root=root) + report = relocate_all(data_root=root) assert report.entries == (), 'no dataset was declared, so none could be considered' assert list(report.manifest_errors) == ['demoprovider'] @@ -258,6 +259,82 @@ def load(self): assert (root / LEGACY / 'notes.txt').is_file(), 'the tree it could not judge is untouched' +def test_a_nested_member_leaves_no_husk_behind(tmp_path, monkeypatch): + """A registry name may nest, and the emptied subdirectory goes too. + + Removing only the directories above the legacy one leaves an empty `sub/` + inside it, which keeps the whole legacy tree standing and then reads on a + later run as an old copy still holding data, when it holds nothing. + """ + from fwl_io.relocate import MOVED, _move_one + + root = tmp_path / 'data' + legacy = root / LEGACY + (legacy / 'sub').mkdir(parents=True) + (legacy / 'sub' / 'nested.dat').write_bytes(CONTENTS['notes.txt']) + target = root / TARGET + entry = Relocation(KEY, READY, legacy_dir=legacy, target_dir=target, files=('sub/nested.dat',)) + + result = _move_one(entry, root) + + assert result.state == MOVED + assert (target / 'sub' / 'nested.dat').read_bytes() == CONTENTS['notes.txt'] + assert not legacy.exists(), 'the emptied subdirectory must not keep the tree alive' + assert not (root / 'stellar_evolution_tracks').exists() + + +@pytest.mark.parametrize('escape', ['relative', 'absolute'], ids=['dot-dot', 'absolute']) +def test_files_outside_the_data_root_are_never_moved(tmp_path, escape): + """A legacy path leaving the root is refused rather than followed. + + The table ships with the package, but it still becomes a path that files + are moved out of, so it gets the same suspicion as a name inside a stamp. + A symlinked legacy directory escapes the same way and only shows it when + the path is resolved. + """ + from fwl_io.relocate import _move_one + + root = tmp_path / 'data' + root.mkdir() + outside = tmp_path / 'outside_dataset' + outside.mkdir() + (outside / 'a.dat').write_bytes(CONTENTS['notes.txt']) + legacy = root / '../outside_dataset' if escape == 'relative' else outside + entry = Relocation(KEY, READY, legacy_dir=legacy, target_dir=root / TARGET, files=('a.dat',)) + + result = _move_one(entry, root) + + assert result.state == UNRESOLVABLE + assert 'outside the data root' in result.detail + assert (outside / 'a.dat').is_file(), 'the file outside the root is untouched' + assert not (root / TARGET).exists() + + +def test_the_shipped_table_is_filtered_at_the_point_it_is_read(monkeypatch): + """An entry naming a path outside the root is dropped, not merely asserted about. + + A test over the shipped file proves what ships today; this proves the code + refuses a bad entry, which is what protects a tree if the file ever changes. + """ + import fwl_io.relocate as module + + table = '[legacy]\n"a.b" = "../escape"\n"c.d" = "/etc"\n"e.f" = "good/place"\n' + + class _Resource: + def read_text(self): + return table + + class _Package: + def joinpath(self, name): + return _Resource() + + monkeypatch.setattr(module, 'files', lambda package: _Package()) + + kept = module._legacy_locations() + + assert kept == {'e.f': 'good/place'}, 'only the contained entry survives' + + def test_a_dataset_with_no_legacy_location_is_not_considered(tmp_path, monkeypatch): """A dataset created after the migration has no old location to leave. @@ -300,3 +377,72 @@ def test_the_shipped_table_names_only_datasets_and_relative_locations(): assert not Path(location).is_absolute(), f'{location!r} is absolute' assert '..' not in Path(location).parts, f'{location!r} climbs out of the data root' assert location.strip('/') == location, f'{location!r} is not a clean relative path' + + +def test_a_rollback_that_cannot_restore_is_reported_as_a_split_tree(tmp_path, monkeypatch): + """When the files cannot be put back, say so rather than call it a failure. + + A plain failure means the tree is as it was and a rerun is the remedy. This + one means the dataset is in two places at once, which no rerun fixes and a + person has to look at, so it gets a state of its own. + """ + import fwl_io.relocate as module + from fwl_io.relocate import SPLIT, _move_one + + root = tmp_path / 'data' + legacy = root / LEGACY + _populate(legacy) + real_replace = module.os.replace + calls = [] + + def failing_replace(src, dst): + calls.append((str(src), str(dst))) + if len(calls) == 1: + return real_replace(src, dst) + raise OSError(28, 'No space left on device') + + monkeypatch.setattr(module.os, 'replace', failing_replace) + entry = Relocation( + KEY, + READY, + legacy_dir=legacy, + target_dir=root / TARGET, + files=tuple(sorted(CONTENTS)), + ) + + result = _move_one(entry, root) + + assert result.state == SPLIT, 'a tree in two places is not the same as an untouched one' + assert 'split between' in result.detail + assert result.faulty + assert len(calls) == 3, 'one move succeeded, one failed, one rollback was attempted' + + +def test_a_failed_move_stops_the_run_rather_than_moving_more_data(tmp_path, monkeypatch): + """After a dataset fails to move, the ones behind it are left alone. + + Continuing would move more data past a tree somebody already has to look + at, and the entries that never ran are reported still ready rather than + quietly dropped. + """ + import fwl_io.relocate as module + from fwl_io.relocate import FAILED, RelocationReport + + planned = RelocationReport( + ( + Relocation('a.first', READY, legacy_dir=tmp_path / 'l1', target_dir=tmp_path / 't1'), + Relocation('b.second', READY, legacy_dir=tmp_path / 'l2', target_dir=tmp_path / 't2'), + ) + ) + monkeypatch.setattr(module, 'plan_relocations', lambda data_root=None: planned) + monkeypatch.setattr( + module, + '_move_one', + lambda entry, root: Relocation(entry.key, FAILED, detail='disk full'), + ) + + report = module.relocate_all(data_root=tmp_path) + + assert [e.state for e in report.entries] == [FAILED, READY] + assert [e.key for e in report.ready] == ['b.second'], 'the untried one is still ready' + assert len(report.faults) == 1 From b63592e6721eeca0102442232f0790e1780b90e3 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 19:32:45 +0200 Subject: [PATCH 11/16] Pin what pruning promises, and say which datasets relocate knows The claim that the walk upward stops at the data root was resting on a test that could not reach it: after a move the root holds the newly placed tree, so it is never empty, never a candidate for removal, and the guard never runs. The assertion that the root survived passed because the root had contents, not because anything refused to remove it. Pruning an emptied chain into an otherwise empty root reaches the guard, and a sibling dataset under a shared parent pins the other half. A directory that held two datasets now has a test as well. Each moves only the files its own registry names and the shared parent goes once both are done, which the layout table has been claiming since it was written. The design page said the package carries the mapping in `legacy_layout.toml` and the command reads it, which reads as though every row is live. Three of them are. The rest are the historical mapping and their trees are left alone until a model declares the dataset, so the page now says which is which rather than implying a tree will move when it will not. --- docs/Explanations/manifests.md | 2 +- tests/test_relocate.py | 81 ++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/docs/Explanations/manifests.md b/docs/Explanations/manifests.md index 42f276b..9fb0224 100644 --- a/docs/Explanations/manifests.md +++ b/docs/Explanations/manifests.md @@ -95,7 +95,7 @@ FWL_DATA/ The tree holds **immutable fetched reference data only**: anything generated at runtime (derived tables, interpolation caches, solver caches) belongs in run output or cache directories, never below `FWL_DATA`. This keeps a shared read-only cache trustworthy as a whole. -Models adopt this layout when they migrate to fwl-io; legacy directories from the previous layout remain readable by unmigrated code and age out when their last consumer migrates. `fwl-io relocate` cleans a local tree up straight away instead, moving each dataset whose files check out against its registry. The mapping from the legacy locations, which the package carries in `legacy_layout.toml` and the command reads: +Models adopt this layout when they migrate to fwl-io; legacy directories from the previous layout remain readable by unmigrated code and age out when their last consumer migrates. `fwl-io relocate` cleans a local tree up straight away instead, moving each dataset whose files check out against its registry. It acts on the datasets listed in the package's `legacy_layout.toml`, which grows as each model migrates and today names three of the families below; the rest are the historical mapping, and a tree holding one of them is left alone until its dataset is declared. The mapping from the legacy locations: | Legacy location (live today) | Target location | |---|---| diff --git a/tests/test_relocate.py b/tests/test_relocate.py index ed362dc..78da67c 100644 --- a/tests/test_relocate.py +++ b/tests/test_relocate.py @@ -446,3 +446,84 @@ def test_a_failed_move_stops_the_run_rather_than_moving_more_data(tmp_path, monk assert [e.state for e in report.entries] == [FAILED, READY] assert [e.key for e in report.ready] == ['b.second'], 'the untried one is still ready' assert len(report.faults) == 1 + + +def test_pruning_never_removes_the_data_root(tmp_path): + """The walk upward stops at the root even when the root is left empty. + + Discriminating on purpose: after a real move the root holds the new tree, + so it is never a candidate for removal and the guard is never reached. Here + it is the only thing standing between an emptied chain and the root itself. + """ + from fwl_io.relocate import _prune + + root = tmp_path / 'data' + nested = root / 'stellar_evolution_tracks' / 'Baraffe' + nested.mkdir(parents=True) + + _prune(nested, root) + + assert not (root / 'stellar_evolution_tracks').exists(), 'the emptied chain goes' + assert root.is_dir(), 'the root is not a leftover of the previous layout' + assert list(root.iterdir()) == [], 'and it really was left empty, or this proves nothing' + + +def test_pruning_stops_at_a_parent_that_still_holds_something(tmp_path): + """A shared parent survives while another dataset still lives under it.""" + from fwl_io.relocate import _prune + + root = tmp_path / 'data' + legacy = root / 'stellar_evolution_tracks' / 'Baraffe' + legacy.mkdir(parents=True) + sibling = root / 'stellar_evolution_tracks' / 'Spada' + sibling.mkdir() + (sibling / 'keep.dat').write_bytes(b'x') + + _prune(legacy, root) + + assert not legacy.exists() + assert (sibling / 'keep.dat').read_bytes() == b'x', 'the neighbour is untouched' + assert (root / 'stellar_evolution_tracks').is_dir(), 'a parent still in use stays' + + +def test_two_datasets_sharing_one_legacy_directory_come_apart(tmp_path, monkeypatch): + """Each dataset moves only the files its own registry names. + + A directory that held more than one dataset is the case where a prune + keyed on the directory rather than on the files would take a neighbour's + data with it, so the shared parent may go only once both have moved. + """ + import fwl_io.relocate as module + + shared = 'stellar_evolution_tracks' + first, second = 'star.tracks.baraffe_2015', 'star.tracks.spada_2013' + monkeypatch.setattr(module, '_legacy_locations', lambda: {first: shared, second: shared}) + + manifest = tmp_path / 'manifest.toml' + manifest.write_text( + f'[{first}]\nzenodo = "10.5281/zenodo.15729114"\n\n' + f'[{second}]\nzenodo = "10.5281/zenodo.7654321"\n' + ) + (tmp_path / f'{first}.registry.txt').write_text( + f'BHAC15_tracks.dat {_digests(["BHAC15_tracks.dat"])["BHAC15_tracks.dat"]}\n' + ) + (tmp_path / f'{second}.registry.txt').write_text( + f'notes.txt {_digests(["notes.txt"])["notes.txt"]}\n' + ) + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + root = tmp_path / 'data' + _populate(root / shared) + + report = module.relocate_all(data_root=root) + + assert sorted(e.state for e in report.entries) == [MOVED, MOVED] + assert (root / TARGET / 'BHAC15_tracks.dat').is_file() + assert (root / 'star/tracks/spada_2013/r7654321/notes.txt').is_file() + assert not (root / shared).exists(), 'the shared directory goes once both have moved' From 4bc60b52eaaa79292d8d58a5ad5a4da891522190 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Sun, 2 Aug 2026 21:49:55 +0200 Subject: [PATCH 12/16] Check the files relocate moves, not only the directories The containment added last round guarded the two directories and nothing else, which left the case it was written for wide open. A registry name may nest, and `rename` follows a symlink in the middle of a path, so a member named `sub/nested.dat` whose `sub` points somewhere else resolves outside the tree: the directory holding it passes every check, the file that moves in comes from outside, and the original is gone. Every source and destination path is now checked, in the planner and again in the move. Pruning stopped at the first entry it could not remove and reported the move a success, so one stray symlink in an old tree left every emptied directory standing while the command said it had tidied up. A symlink answers `is_dir` for whatever it points at and `rmdir` refuses it, which is the combination that triggered it. Those are skipped now, a refusal moves to the next entry instead of ending the walk, and the walk order is fixed by name as well as depth so it does not vary by filesystem. The check meant to reject a table value that is not a path ran after the value had already been turned into one, so a number or a list raised `TypeError` out of the command rather than being dropped. It runs first now. Also corrects a test that could not fail: it pointed its symlink at a directory with a file in it, so the emptiness test skipped the symlink before the removal that was the whole point was ever attempted. --- src/fwl_io/relocate.py | 79 ++++++++++++++++++++++++++++------------ tests/test_relocate.py | 82 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 23 deletions(-) diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py index f666afe..5883027 100644 --- a/src/fwl_io/relocate.py +++ b/src/fwl_io/relocate.py @@ -167,8 +167,10 @@ def _legacy_locations() -> dict[str, str]: table = dict(tomllib.loads(text).get('legacy', {})) safe = {} for key, location in table.items(): - parts = Path(location).parts - if not isinstance(location, str) or Path(location).is_absolute() or '..' in parts: + if not isinstance(location, str): + log.warning('legacy location for %s is not a path: %r', key, location) + continue + if Path(location).is_absolute() or '..' in Path(location).parts: log.warning('legacy location for %s is not inside the data root: %r', key, location) continue safe[key] = location @@ -183,6 +185,25 @@ def _inside(path: Path, root: Path) -> bool: return False +def _escaping( + legacy_dir: Path, target_dir: Path, names: tuple[str, ...], root: Path +) -> Path | None: + """The first path here that leaves ``root``, or ``None`` if all stay inside. + + Every file is checked, not just the two directories: a registry name may + nest, and a symlinked component inside it resolves somewhere else entirely + while the directory holding it looks perfectly ordinary. + """ + for path in (legacy_dir, target_dir): + if not _inside(path, root): + return path + for name in names: + for path in (legacy_dir / name, target_dir / name): + if not _inside(path, root): + return path + return None + + def _classify(legacy_dir: Path, target_dir: Path, registry: dict[str, str]) -> tuple[str, str]: """Decide what the two trees on disk allow, without touching either.""" if _all_match(target_dir, registry): @@ -252,18 +273,6 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: continue seen.add(ds.key) legacy_dir = root / legacy - if legacy_dir.exists() and not _inside(legacy_dir, root): - # A symlink is the way this happens in a real tree: the joined - # path is clean, and only resolving it shows it leaves the root. - entries.append( - Relocation( - ds.key, - UNRESOLVABLE, - legacy_dir=legacy_dir, - detail=f'{legacy_dir} resolves outside the data root {root}', - ) - ) - continue try: registry = ds.registry() target_dir = root / _version_dir(ds) @@ -272,6 +281,20 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: Relocation(ds.key, UNRESOLVABLE, legacy_dir=legacy_dir, detail=str(exc)) ) continue + outside = _escaping(legacy_dir, target_dir, tuple(registry), root) + if outside is not None: + # A symlink is how this happens in a real tree: every joined + # path looks clean and only resolving one shows it leaves. + entries.append( + Relocation( + ds.key, + UNRESOLVABLE, + legacy_dir=legacy_dir, + target_dir=target_dir, + detail=f'{outside} resolves outside the data root {root}', + ) + ) + continue state, detail = _classify(legacy_dir, target_dir, registry) entries.append( Relocation( @@ -297,7 +320,8 @@ def _version_dir(ds: Dataset) -> str: def _move_one(entry: Relocation, root: Path) -> Relocation: """Move one verified legacy tree, leaving nothing half-moved behind.""" assert entry.legacy_dir is not None and entry.target_dir is not None - if not _inside(entry.legacy_dir, root) or not _inside(entry.target_dir, root): + outside = _escaping(entry.legacy_dir, entry.target_dir, entry.files, root) + if outside is not None: # Checked here as well as when the plan is built, so the guarantee that # this only ever moves files inside the data root belongs to the code # that does the moving rather than to whoever called it. @@ -306,7 +330,7 @@ def _move_one(entry: Relocation, root: Path) -> Relocation: UNRESOLVABLE, legacy_dir=entry.legacy_dir, target_dir=entry.target_dir, - detail=f'refusing to move files outside the data root {root}', + detail=f'{outside} resolves outside the data root {root}', ) done: list[str] = [] try: @@ -371,16 +395,25 @@ def _prune(directory: Path, root: Path) -> None: """ root = root.resolve() if directory.is_dir(): - for path in sorted(directory.rglob('*'), key=lambda p: len(p.parts), reverse=True): - if path.is_dir() and not any(path.iterdir()): - try: + # Deepest first so a child is gone before its parent is tried, and the + # name breaks the tie so the walk is the same on every filesystem. + for path in sorted( + directory.rglob('*'), key=lambda p: (len(p.parts), str(p)), reverse=True + ): + # A symlink answers is_dir() for whatever it points at, and rmdir + # refuses it, so following one here would both leave the tree + # standing and reach outside it. + if path.is_symlink() or not path.is_dir(): + continue + try: + if not any(path.iterdir()): path.rmdir() - except OSError: - return + except OSError: + continue while directory.resolve() != root and directory.resolve().is_relative_to(root): - if any(directory.iterdir()): - return try: + if any(directory.iterdir()): + return directory.rmdir() except OSError: return diff --git a/tests/test_relocate.py b/tests/test_relocate.py index 78da67c..30887ac 100644 --- a/tests/test_relocate.py +++ b/tests/test_relocate.py @@ -527,3 +527,85 @@ def load(self): assert (root / TARGET / 'BHAC15_tracks.dat').is_file() assert (root / 'star/tracks/spada_2013/r7654321/notes.txt').is_file() assert not (root / shared).exists(), 'the shared directory goes once both have moved' + + +def test_a_nested_name_reaching_outside_the_root_is_refused(tmp_path): + """A symlinked component inside a member name escapes, and is caught. + + The directory holding it looks perfectly ordinary and passes every check + on the directories alone, which is why the files are checked too: `rename` + follows symlinks in the middle of a path, so the file that moves in is one + from outside the tree and the original is gone. + """ + from fwl_io.relocate import _move_one + + root = tmp_path / 'data' + legacy = root / LEGACY + legacy.mkdir(parents=True) + outside = tmp_path / 'elsewhere' + outside.mkdir() + (outside / 'nested.dat').write_bytes(CONTENTS['notes.txt']) + (legacy / 'sub').symlink_to(outside, target_is_directory=True) + entry = Relocation( + KEY, READY, legacy_dir=legacy, target_dir=root / TARGET, files=('sub/nested.dat',) + ) + + result = _move_one(entry, root) + + assert result.state == UNRESOLVABLE + assert 'resolves outside the data root' in result.detail + assert (outside / 'nested.dat').is_file(), 'the file outside the tree is untouched' + assert not (root / TARGET / 'sub' / 'nested.dat').exists() + + +def test_a_symlink_in_the_legacy_tree_does_not_abort_the_prune(tmp_path): + """One entry that cannot be removed must not stop the rest being removed. + + A symlink answers ``is_dir`` for whatever it points at and ``rmdir`` + refuses it, so treating that refusal as the end of the walk would leave + every emptied directory standing while the command reported success. + """ + from fwl_io.relocate import _prune + + root = tmp_path / 'data' + legacy = root / LEGACY + (legacy / 'aaa_empty_one').mkdir(parents=True) + (legacy / 'aaa_empty_two').mkdir() + # Empty on purpose: a symlink to a non-empty directory is skipped by the + # emptiness test before the removal is ever tried, so it would prove nothing. + elsewhere = tmp_path / 'elsewhere' + elsewhere.mkdir() + # Named to sort first, so the walk meets the symlink before the two empty + # directories and an abort there would leave both of them standing. + (legacy / 'zzz_link').symlink_to(elsewhere, target_is_directory=True) + + _prune(legacy, root) + + assert not (legacy / 'aaa_empty_one').exists(), 'the walk carried on past the symlink' + assert not (legacy / 'aaa_empty_two').exists() + assert (legacy / 'zzz_link').is_symlink(), 'the symlink itself is not ours to remove' + assert elsewhere.is_dir(), 'and what it points at is not ours to remove either' + + +def test_a_table_value_that_is_not_a_path_is_dropped_not_raised(monkeypatch): + """A non-string entry is refused before anything tries to build a path from it. + + Ordering matters here: constructing the path first raises ``TypeError`` out + of the command, so the check that is meant to reject the value has to come + before the value is used. + """ + import fwl_io.relocate as module + + table = '[legacy]\n"a.b" = 42\n"c.d" = ["x"]\n"e.f" = "good/place"\n' + + class _Resource: + def read_text(self): + return table + + class _Package: + def joinpath(self, name): + return _Resource() + + monkeypatch.setattr(module, 'files', lambda package: _Package()) + + assert module._legacy_locations() == {'e.f': 'good/place'} From efa91ab618af8d384d68f193dd7e9c7e1cf4b6a4 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Mon, 3 Aug 2026 08:46:51 +0200 Subject: [PATCH 13/16] Refuse the relocations that cannot be verified Three inputs reached the tree comparison and were answered from a check that could not mean what it said. A dataset with an empty registry passed every check vacuously, since each one asks whether the tree holds what the registry lists. An untouched legacy directory was reported as already moved, or moved with zero files, and the run exited 0 with the data still sitting where it was. An archive dataset was compared against a registry that pins the packed archive rather than the files a legacy tree actually holds, so an intact tree was reported incomplete for a file it never had. Both are now refused by name, with the reason, and nothing is touched. A layout table that will not parse raised out of the command instead of being reported. Every other unreadable input here is carried in the report, so this one is too. --- src/fwl_io/relocate.py | 62 ++++++++++++++++++++++++- tests/test_relocate.py | 103 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 2 deletions(-) diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py index 5883027..0a209c1 100644 --- a/src/fwl_io/relocate.py +++ b/src/fwl_io/relocate.py @@ -16,6 +16,10 @@ Nothing is downloaded either. A dataset whose legacy tree is incomplete stays incomplete here; the fetcher is what fills it, and it will do so at the current location once the move has happened. + +A dataset packaged as an archive is reported rather than moved. Its registry +pins the packed archive, and a legacy tree holds the extracted members, so +there is nothing to hash the tree against. """ from __future__ import annotations @@ -163,8 +167,16 @@ def _legacy_locations() -> dict[str, str]: turned into a path that files get moved out of, so it earns the same suspicion as a name inside a provenance stamp. """ - text = files('fwl_io.data').joinpath(_LAYOUT_RESOURCE).read_text() - table = dict(tomllib.loads(text).get('legacy', {})) + try: + text = files('fwl_io.data').joinpath(_LAYOUT_RESOURCE).read_text() + table = tomllib.loads(text).get('legacy', {}) + if not isinstance(table, dict): + raise TypeError(f'[legacy] is {type(table).__name__}, not a table') + except (OSError, ValueError, TypeError) as exc: + # A relocation nobody can plan is still a report, not a traceback, the + # same as a manifest that will not load. + log.error('cannot read %s, so no legacy location is known: %s', _LAYOUT_RESOURCE, exc) + return {} safe = {} for key, location in table.items(): if not isinstance(location, str): @@ -204,6 +216,40 @@ def _escaping( return None +def _unmovable(ds: Dataset, registry: dict[str, str]) -> str | None: + """Why this dataset cannot be relocated at all, or ``None`` if it can. + + Both cases would otherwise reach :func:`_classify` and be answered from a + comparison that cannot mean what it says. + + Parameters + ---------- + ds : Dataset + The dataset as its manifest declares it. + registry : dict[str, str] + Registry filenames mapped to their expected digests. + + Returns + ------- + str | None + A sentence naming the obstacle, or ``None`` when there is none. + """ + if not registry: + # Every check here is "does the tree hold what the registry lists", and + # over an empty registry that is vacuously true: an untouched legacy + # directory would be reported as already moved. + return 'empty registry: run "fwl-io sync" for this dataset first' + if ds.extract is not None: + # The registry pins the packed archive, which a legacy tree never held: + # it holds the extracted members. Comparing against it would call an + # intact tree incomplete, and moving on that basis would be worse. + return ( + f'{ds.extract} archive dataset: its registry pins the archive rather ' + 'than the extracted files, so a legacy tree cannot be verified against it' + ) + return None + + def _classify(legacy_dir: Path, target_dir: Path, registry: dict[str, str]) -> tuple[str, str]: """Decide what the two trees on disk allow, without touching either.""" if _all_match(target_dir, registry): @@ -281,6 +327,18 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: Relocation(ds.key, UNRESOLVABLE, legacy_dir=legacy_dir, detail=str(exc)) ) continue + unmovable = _unmovable(ds, registry) + if unmovable is not None: + entries.append( + Relocation( + ds.key, + UNRESOLVABLE, + legacy_dir=legacy_dir, + target_dir=target_dir, + detail=unmovable, + ) + ) + continue outside = _escaping(legacy_dir, target_dir, tuple(registry), root) if outside is not None: # A symlink is how this happens in a real tree: every joined diff --git a/tests/test_relocate.py b/tests/test_relocate.py index 30887ac..f184e62 100644 --- a/tests/test_relocate.py +++ b/tests/test_relocate.py @@ -609,3 +609,106 @@ def joinpath(self, name): monkeypatch.setattr(module, 'files', lambda package: _Package()) assert module._legacy_locations() == {'e.f': 'good/place'} + + +def test_an_empty_registry_does_not_report_an_untouched_tree_as_moved(tmp_path, monkeypatch): + """A dataset whose registry lists nothing is refused, not declared complete. + + Every comparison this module makes asks whether the tree holds what the + registry lists, and over an empty registry each one is vacuously true. The + legacy tree must survive the run for the report to have meant anything. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text(f'[{KEY}]\nzenodo = "{ZENODO}"\nrequired_by = ["mors"]\n') + (tmp_path / f'{KEY}.registry.txt').write_text('# no files\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + legacy_dir = tmp_path / LEGACY + _populate(legacy_dir) + # The precondition the assertions below rest on: real files are present, so + # a pass cannot come from an empty tree. + assert (legacy_dir / 'BHAC15_tracks.dat').is_file() + + report = relocate_all(data_root=tmp_path) + + (entry,) = [e for e in report.entries if e.key == KEY] + assert entry.state == UNRESOLVABLE, f'empty registry reported as {entry.state}' + assert entry.state not in (MOVED, ALREADY_CURRENT, READY) + assert 'empty registry' in entry.detail + assert not report.ok + for name, body in CONTENTS.items(): + assert (legacy_dir / name).read_bytes() == body + assert not (tmp_path / TARGET).exists() + + +def test_an_archive_dataset_is_refused_rather_than_called_incomplete(tmp_path, monkeypatch): + """An archive dataset's registry pins the archive, which a legacy tree never held. + + Hashing the tree against it would report an intact tree as incomplete, so + the dataset is refused by name instead, and nothing is moved either way. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text(f'[{KEY}]\nzenodo = "{ZENODO}"\nrequired_by = ["mors"]\nextract = "tar"\n') + archive_digest = f'sha256:{hashlib.sha256(b"packed").hexdigest()}' + (tmp_path / f'{KEY}.registry.txt').write_text(f'bundle.tar.gz {archive_digest}\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + legacy_dir = tmp_path / LEGACY + _populate(legacy_dir) + # The tree holds the extracted members, exactly as a real legacy tree does, + # and never the archive the registry names. + assert (legacy_dir / 'BHAC15_tracks.dat').is_file() + assert not (legacy_dir / 'bundle.tar.gz').exists() + + report = relocate_all(data_root=tmp_path) + + (entry,) = [e for e in report.entries if e.key == KEY] + assert entry.state == UNRESOLVABLE, f'archive dataset reported as {entry.state}' + assert entry.state != INCOMPLETE + assert 'archive' in entry.detail + for name, body in CONTENTS.items(): + assert (legacy_dir / name).read_bytes() == body + assert not (tmp_path / TARGET).exists() + + +@pytest.mark.parametrize( + ('table', 'why'), + [ + ('[legacy]\n"a.b" = \n', 'unparseable TOML'), + ('legacy = "not-a-table"\n', 'legacy is a string'), + ('legacy = [1, 2]\n', 'legacy is an array'), + ], +) +def test_an_unreadable_layout_table_reports_nothing_rather_than_raising(monkeypatch, table, why): + """A table that will not load leaves no legacy locations, and no traceback. + + Every other unreadable input this command meets is carried in the report, + so this one does not get to be the exception that aborts the run. + """ + import fwl_io.relocate as module + + class _Resource: + def read_text(self): + return table + + class _Package: + def joinpath(self, name): + return _Resource() + + monkeypatch.setattr(module, 'files', lambda package: _Package()) + + assert module._legacy_locations() == {}, why From 7b6678de3d4acd547b98ac96447ac4f4e502d4a8 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Mon, 3 Aug 2026 08:46:51 +0200 Subject: [PATCH 14/16] Install pytest-timeout so the test timeouts apply Two test files carry a 30 s timeout marker, but the package was never a dependency, so the marker did nothing in a fresh developer or CI environment and a hang would have run unbounded. --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 2553a6b..819b885 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,6 +44,7 @@ Issues = "https://github.com/FormingWorlds/fwl-io/issues" develop = [ "pytest>=8.0", "pytest-cov", + "pytest-timeout", "ruff", ] docs = [ From 6559ef9c225507487ccacc2dc41cd6927272bd2c Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Mon, 3 Aug 2026 11:06:39 +0200 Subject: [PATCH 15/16] Report a legacy table that will not load, and refuse only trees that exist Two faults in how the previous commit refused what it cannot verify. An unreadable layout table produced no locations and an empty report, which reads exactly like a tidy tree with nothing left to move: ok was true and the command exited 0. The reason is now carried back with the locations and named in the report, the same way an unreadable manifest already is. A broken install raises ImportError from the resource lookup, which was not among the exceptions being caught, so that is handled too. The refusals also fired before anything asked whether a legacy tree exists, so an archive dataset on a machine that never had the old layout was a permanent fault rather than simply absent. They now apply only when there is a directory to refuse. --- src/fwl_io/relocate.py | 33 +++++++++++------ tests/test_relocate.py | 83 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 100 insertions(+), 16 deletions(-) diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py index 0a209c1..22c5c1c 100644 --- a/src/fwl_io/relocate.py +++ b/src/fwl_io/relocate.py @@ -99,6 +99,7 @@ class RelocationReport: entries: tuple[Relocation, ...] = () manifest_errors: dict[str, str] = field(default_factory=dict) + layout_error: str | None = None def _in_state(self, *states: str) -> tuple[Relocation, ...]: return tuple(e for e in self.entries if e.state in states) @@ -106,7 +107,7 @@ def _in_state(self, *states: str) -> tuple[Relocation, ...]: @property def ok(self) -> bool: """True when every legacy tree found was dealt with and none was skipped.""" - return not self.faults and not self.manifest_errors + return not self.faults and not self.manifest_errors and self.layout_error is None @property def ready(self) -> tuple[Relocation, ...]: @@ -138,6 +139,10 @@ def summary(self) -> str: lines = [e.summary() for e in sorted(self.entries, key=lambda e: e.key)] for provider, error in sorted(self.manifest_errors.items()): lines.append(f'{provider}: MANIFEST UNREADABLE, {error}') + if self.layout_error is not None: + # Without this the run reports nothing to do, which is what a tidy + # tree also reports, and the two are not the same answer. + lines.append(f'LEGACY LAYOUT UNREADABLE, {self.layout_error}') if not lines: return 'no dataset declares a legacy location' done, waiting, bad = len(self.moved), len(self.ready), len(self.faults) @@ -159,24 +164,30 @@ def summary(self) -> str: return '\n'.join(lines) -def _legacy_locations() -> dict[str, str]: +def _legacy_locations() -> tuple[dict[str, str], str | None]: """Read the shipped table of where each dataset used to live. An entry naming an absolute path or climbing out of the data root is dropped. The table ships with the package, but it is still a file being turned into a path that files get moved out of, so it earns the same suspicion as a name inside a provenance stamp. + + Returns + ------- + tuple[dict[str, str], str | None] + Dataset keys mapped to their legacy directory, and why the table could + not be read, which is ``None`` when it was read. """ try: text = files('fwl_io.data').joinpath(_LAYOUT_RESOURCE).read_text() table = tomllib.loads(text).get('legacy', {}) if not isinstance(table, dict): raise TypeError(f'[legacy] is {type(table).__name__}, not a table') - except (OSError, ValueError, TypeError) as exc: - # A relocation nobody can plan is still a report, not a traceback, the - # same as a manifest that will not load. + except (OSError, ValueError, TypeError, ImportError) as exc: + # Reported rather than raised, like a manifest that will not load, and + # carried back so the run cannot read as a tree with nothing to do. log.error('cannot read %s, so no legacy location is known: %s', _LAYOUT_RESOURCE, exc) - return {} + return {}, f'{_LAYOUT_RESOURCE}: {exc}' safe = {} for key, location in table.items(): if not isinstance(location, str): @@ -186,7 +197,7 @@ def _legacy_locations() -> dict[str, str]: log.warning('legacy location for %s is not inside the data root: %r', key, location) continue safe[key] = location - return safe + return safe, None def _inside(path: Path, root: Path) -> bool: @@ -308,7 +319,7 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: from fwl_io.manifest import _discover root = resolve_data_root(data_root) - locations = _legacy_locations() + locations, layout_error = _legacy_locations() entries: list[Relocation] = [] seen: set[str] = set() providers, manifest_errors = _discover() @@ -327,7 +338,7 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: Relocation(ds.key, UNRESOLVABLE, legacy_dir=legacy_dir, detail=str(exc)) ) continue - unmovable = _unmovable(ds, registry) + unmovable = _unmovable(ds, registry) if legacy_dir.is_dir() else None if unmovable is not None: entries.append( Relocation( @@ -365,7 +376,7 @@ def plan_relocations(data_root: str | Path | None = None) -> RelocationReport: legacy_present=legacy_dir.is_dir(), ) ) - return RelocationReport(tuple(entries), dict(manifest_errors)) + return RelocationReport(tuple(entries), dict(manifest_errors), layout_error) def _version_dir(ds: Dataset) -> str: @@ -514,4 +525,4 @@ def relocate_all(data_root: str | Path | None = None, dry_run: bool = False) -> # state somebody has to look at. log.error('stopping after %s could not be relocated', moved.key) halted = True - return RelocationReport(tuple(done), dict(plan.manifest_errors)) + return RelocationReport(tuple(done), dict(plan.manifest_errors), plan.layout_error) diff --git a/tests/test_relocate.py b/tests/test_relocate.py index f184e62..761c61c 100644 --- a/tests/test_relocate.py +++ b/tests/test_relocate.py @@ -16,6 +16,7 @@ import pytest from fwl_io.relocate import ( + _LAYOUT_RESOURCE, ABSENT, ALREADY_CURRENT, INCOMPLETE, @@ -330,8 +331,9 @@ def joinpath(self, name): monkeypatch.setattr(module, 'files', lambda package: _Package()) - kept = module._legacy_locations() + kept, error = module._legacy_locations() + assert error is None, 'a readable table reports no error' assert kept == {'e.f': 'good/place'}, 'only the contained entry survives' @@ -369,7 +371,9 @@ def test_the_shipped_table_names_only_datasets_and_relative_locations(): from fwl_io.relocate import _legacy_locations - table = _legacy_locations() + table, error = _legacy_locations() + + assert error is None, 'the shipped table has to be readable' assert table, 'the table has to declare the datasets that have already moved' for key, location in table.items(): @@ -497,7 +501,9 @@ def test_two_datasets_sharing_one_legacy_directory_come_apart(tmp_path, monkeypa shared = 'stellar_evolution_tracks' first, second = 'star.tracks.baraffe_2015', 'star.tracks.spada_2013' - monkeypatch.setattr(module, '_legacy_locations', lambda: {first: shared, second: shared}) + monkeypatch.setattr( + module, '_legacy_locations', lambda: ({first: shared, second: shared}, None) + ) manifest = tmp_path / 'manifest.toml' manifest.write_text( @@ -608,7 +614,7 @@ def joinpath(self, name): monkeypatch.setattr(module, 'files', lambda package: _Package()) - assert module._legacy_locations() == {'e.f': 'good/place'} + assert module._legacy_locations() == ({'e.f': 'good/place'}, None) def test_an_empty_registry_does_not_report_an_untouched_tree_as_moved(tmp_path, monkeypatch): @@ -711,4 +717,71 @@ def joinpath(self, name): monkeypatch.setattr(module, 'files', lambda package: _Package()) - assert module._legacy_locations() == {}, why + locations, error = module._legacy_locations() + + assert locations == {}, why + assert error is not None, f'{why} was swallowed instead of being carried back' + assert _LAYOUT_RESOURCE in error + + +def test_a_layout_table_that_did_not_load_is_not_reported_as_nothing_to_do(tmp_path, monkeypatch): + """An unreadable table is carried into the report, not just logged. + + Returning no locations makes the run indistinguishable from a tidy tree + with nothing left to move, which is the overstatement every other unreadable + input here is refused for. + """ + import fwl_io.relocate as module + + class _Resource: + def read_text(self): + return 'legacy = "not-a-table"\n' + + class _Package: + def joinpath(self, name): + return _Resource() + + monkeypatch.setattr(module, 'files', lambda package: _Package()) + + report = module.plan_relocations(data_root=tmp_path) + + assert report.entries == (), 'nothing could be planned without the table' + assert not report.ok, 'a run that read no table cannot report success' + assert report.layout_error is not None + summary = report.summary() + assert 'LEGACY LAYOUT UNREADABLE' in summary + assert summary != 'no dataset declares a legacy location', ( + 'the failure reads exactly like a tree with nothing to do' + ) + + +def test_an_archive_dataset_with_no_legacy_tree_is_absent_not_a_fault(tmp_path, monkeypatch): + """A refusal describes a tree that is there, never one that never existed. + + The archive and empty-registry refusals answer "this tree cannot be + verified". With no legacy directory at all there is no tree to refuse, and + a machine that never had the old layout must not fail the command forever. + """ + manifest = tmp_path / 'manifest.toml' + manifest.write_text(f'[{KEY}]\nzenodo = "{ZENODO}"\nrequired_by = ["mors"]\nextract = "tar"\n') + archive_digest = f'sha256:{hashlib.sha256(b"packed").hexdigest()}' + (tmp_path / f'{KEY}.registry.txt').write_text(f'bundle.tar.gz {archive_digest}\n') + + class _EP: + name = 'demoprovider' + + def load(self): + return lambda: manifest + + monkeypatch.setattr('fwl_io.manifest.entry_points', lambda group: [_EP()]) + + # The precondition: no legacy tree anywhere, which is every machine that + # installed after the layout changed. + assert not (tmp_path / LEGACY).exists() + + report = relocate_all(data_root=tmp_path) + + (entry,) = [e for e in report.entries if e.key == KEY] + assert entry.state == ABSENT, f'no legacy tree reported as {entry.state}' + assert entry.state != UNRESOLVABLE + assert report.ok, 'a machine that never had the old layout must not fail' From b2e9bb20bf7ce9d60f7acd26ea8c3768333b0c65 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Mon, 3 Aug 2026 11:08:38 +0200 Subject: [PATCH 16/16] Say in the CLI reference what relocate refuses and when it exits 1 The relocate section described neither of the two datasets it will not move, and its exit-code sentence left out the case where the table of old locations cannot be read. The archive refusal also now says what to do about it, since there is nothing the command itself can do for such a tree. --- docs/Reference/cli.md | 4 +++- src/fwl_io/relocate.py | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/Reference/cli.md b/docs/Reference/cli.md index e782741..e5ed135 100644 --- a/docs/Reference/cli.md +++ b/docs/Reference/cli.md @@ -52,9 +52,11 @@ Moves data left by the previous layout into the place it belongs now, for a tree A dataset moves only when every file its registry declares is present in the old location and matches its recorded digest. Anything else is reported and left exactly where it is: an incomplete tree, a file whose contents differ, or a dataset whose registry has not been generated. Verifying first is the point, since moving a stale copy would put it where the fetcher then trusts it. Once a dataset's files have moved, the emptied directories are removed, and the walk upward stops at the data root. +Two kinds of dataset cannot be verified at all and are refused rather than moved, each named with its reason. An archive dataset's registry pins the packed archive, while an old tree holds the files extracted from it, so there is nothing to hash the tree against; move such a tree by hand, or delete it and let the fetcher rebuild it at the current location. A dataset whose registry is empty offers no files to compare, so every check over it would pass for want of anything to fail; run `fwl-io sync` for it. Both are reported only when an old directory is actually there, so a machine that never had the previous layout is unaffected. + A dataset already at its current location is not a fault, and a copy still sitting at the old location beside it is named rather than deleted. Nothing here removes data: the only directories it removes are ones it has just emptied itself. -Exit is 1 when a legacy tree was found and could not be moved, or when an installed manifest could not be read, since that manifest may be the one declaring the dataset a tree still holds. A tree that was already tidy exits 0. `--dry-run` reports the same plan without moving anything. The equivalent Python entry points are `fwl_io.relocate_all` and `fwl_io.plan_relocations`. +Exit is 1 when a legacy tree was found and could not be moved, when an installed manifest could not be read, since that manifest may be the one declaring the dataset a tree still holds, or when the shipped table of old locations could not be read, since without it no dataset has an old location to look at and a run that reported nothing would read like a tidy tree. A tree that was already tidy exits 0. `--dry-run` reports the same plan without moving anything. The equivalent Python entry points are `fwl_io.relocate_all` and `fwl_io.plan_relocations`. ## fwl-io mirror diff --git a/src/fwl_io/relocate.py b/src/fwl_io/relocate.py index 22c5c1c..5c8e243 100644 --- a/src/fwl_io/relocate.py +++ b/src/fwl_io/relocate.py @@ -256,7 +256,8 @@ def _unmovable(ds: Dataset, registry: dict[str, str]) -> str | None: # intact tree incomplete, and moving on that basis would be worse. return ( f'{ds.extract} archive dataset: its registry pins the archive rather ' - 'than the extracted files, so a legacy tree cannot be verified against it' + 'than the extracted files, so this tree cannot be verified against it; ' + 'move it by hand, or delete it and let the fetcher rebuild it' ) return None