From 64c37f0504b395410bcf3557d07a1e5809a75d2d Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:37:44 -0600 Subject: [PATCH 01/14] Fuzz the edges: 13 confirmed bugs, pinned as strict xfails test_fuzz.py fuzzes what a filename can contain. This fuzzes what a card can contain: a BRAW clip whose atom headers lie, a tree that maps two sources onto one destination, a junction pointing at its own parent, a retry policy someone hand-edited into a preset. Each xfail(strict=True) is a reproduced bug naming its own offending line, so fixing one turns the test XPASS and pytest asks for the marker back. The unmarked tests are controls that pin behaviour which already holds, several of which are the sibling of a broken path: write_mhl survives names that crash write_csv, and preserving the tree keeps destinations injective where --flat does not. The worst of it, in order: - --flat maps every source onto dest_root/name with no collision check, so two takes named the same in different card folders leave one file on disk and two VERIFIED rows in the report. FULL verification passes both: each is verified before the next clobbers it. - engine.run calls probe() with no try/except and probe()'s BRAW branch has none either, so one malformed clip aborts the whole job. The clips after it are never copied and no report is written. - braw._read_timing indexes and unpacks at offsets taken from an atom's declared size without checking the buffer reaches that far. - braw._descend recurses per nested container with no depth limit, so a 16 KB file exhausts the stack. - ascmhl.py never grew the _xml_safe filter reports/mhl.py has, and read_manifest_hashes swallows the resulting ParseError, so a file can leave the chain of custody with nothing to show for it. The random-atom-tree sweep passes and is kept as a control: a random tree almost never grows the vide handler _read_timing requires, so the truncated-trak sweep pins that shape and fuzzes only the two bodies read at fixed offsets. It rediscovers the crash and minimises it to two variants. --- tests/test_fuzz_edges.py | 623 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 623 insertions(+) create mode 100644 tests/test_fuzz_edges.py diff --git a/tests/test_fuzz_edges.py b/tests/test_fuzz_edges.py new file mode 100644 index 0000000..d6f5aff --- /dev/null +++ b/tests/test_fuzz_edges.py @@ -0,0 +1,623 @@ +"""Property-based tests for the edges the happy path never reaches. + +`test_fuzz.py` fuzzes what a *filename* can contain. This module fuzzes what a +*card* can contain: a BRAW clip whose atom headers lie, a tree whose shape maps +two sources onto one destination, a directory junction that points at its own +parent, a retry policy someone hand-edited into a preset. + +The contract under test is narrow and worth stating plainly, because every +failure below is a violation of one of these three: + + 1. No input may make an offload lose a file without saying so. A report that + says VERIFIED is an attestation; producing one for a file that is not at + the destination is worse than crashing. + 2. No single unreadable or malformed file may abort a job. A card with one + bad clip must still offload the other clips and still write a report. + 3. A parser fed hostile bytes may return nothing, but it may not raise + something the caller has no reason to catch. + +Tests marked `xfail(strict=True)` are confirmed live bugs, not aspirations. +Each names the offending line. If you fix one, the marker turns the test XPASS +and pytest will tell you to delete the marker. + +Run longer sweeps with: pytest --fuzz tests/test_fuzz_edges.py +""" + +from __future__ import annotations + +import datetime as _dt +import errno +import json +import struct +import subprocess +import sys +import threading +from pathlib import Path +from typing import TYPE_CHECKING, Any, cast +from xml.etree import ElementTree as ET + +import pytest +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st + +from offloader import ascmhl, braw, engine, naming, retry +from offloader.engine import OffloadOptions +from offloader.hashers import hash_file +from offloader.history import History +from offloader.models import ( + Destination, + FileEntry, + FileStatus, + Job, + MediaInfo, + VerificationMode, +) +from offloader.reports import write_csv, write_mhl + +if TYPE_CHECKING: + from typing import BinaryIO + +WINDOWS = sys.platform == "win32" + +#: What a parser is allowed to raise. Anything else reaches a caller that has +#: no reason to be catching it. In this codebase, `engine.run`, which catches +#: nothing around `probe()` and so dies mid-job. +PARSER_MAY_RAISE = (OSError,) + + +# ------------------------------------------------------------------ BRAW atoms + + +def atom(tag: bytes, payload: bytes = b"", *, declared: int | None = None) -> bytes: + """One ISO-BMFF atom. `declared` lies about the size on purpose.""" + size = len(payload) + 8 if declared is None else declared + return struct.pack(">I", size) + tag + payload + + +#: The container types `braw._descend` will recurse into. +CONTAINERS = [b"moov", b"trak", b"mdia", b"minf", b"stbl", b"udta", b"meta"] +LEAVES = [b"hdlr", b"mdhd", b"stts", b"keys", b"ilst", b"free", b"ftyp", b"data"] + +#: A `hdlr` that makes `_read_timing` accept the enclosing trak as the video +#: track: version+flags(4), pre_defined(4), then the handler type itself. +VIDEO_HANDLER = atom(b"hdlr", b"\x00" * 8 + b"vide") + + +def braw_file(moov_payload: bytes) -> bytes: + return atom(b"ftyp", b"braw") + atom(b"moov", moov_payload) + + +def video_trak(mdhd_payload: bytes, stts_payload: bytes) -> bytes: + """A trak shaped exactly like the one `_read_timing` goes looking for.""" + mdia = (VIDEO_HANDLER + + atom(b"mdhd", mdhd_payload) + + atom(b"minf", atom(b"stbl", atom(b"stts", stts_payload)))) + return atom(b"trak", atom(b"mdia", mdia)) + + +#: A well-formed mdhd body: version+flags(4), created(4), modified(4), +#: timescale(4), duration(4). +GOOD_MDHD = b"\x00" * 12 + struct.pack(">II", 1000, 5000) + + +def atom_trees(depth: int = 3): + """Random nested atom trees, the shape a corrupt card actually produces.""" + leaf = st.builds( + lambda tag, body: atom(tag, body), + st.sampled_from(LEAVES), + st.binary(max_size=48), + ) + if depth <= 0: + return leaf + return st.one_of( + leaf, + st.builds( + lambda tag, kids: atom(tag, b"".join(kids)), + st.sampled_from(CONTAINERS), + st.lists(atom_trees(depth - 1), max_size=3), + ), + ) + + +def write_braw(directory: Path, payload: bytes, name: str = "clip.braw") -> Path: + target = directory / name + target.write_bytes(payload) + return target + + +# ---------------------------------------------------- BRAW: never crash a job + + +@settings(deadline=None, suppress_health_check=[HealthCheck.too_slow]) +@given(payload=st.binary(max_size=4096)) +def test_braw_read_info_survives_arbitrary_bytes(payload: bytes, tmp_path_factory): + """The cheapest fuzz there is: a .braw that is not BRAW at all.""" + target = write_braw(tmp_path_factory.mktemp("braw"), payload) + try: + braw.read_info(target) + except PARSER_MAY_RAISE: + pass + except Exception as exc: # noqa: BLE001 + pytest.fail(f"read_info raised {type(exc).__name__}: {exc}") + + +@settings(deadline=None, suppress_health_check=[HealthCheck.too_slow]) +@given(trees=st.lists(atom_trees(), min_size=1, max_size=4)) +def test_braw_read_info_survives_random_atom_trees(trees, tmp_path_factory): + """Random well-formed-looking atom trees. This one holds: `_walk` and + `_descend` bound-check honestly, so a tree that never resolves to a video + track is parsed and discarded without complaint.""" + target = write_braw(tmp_path_factory.mktemp("braw"), braw_file(b"".join(trees))) + try: + braw.read_info(target) + except PARSER_MAY_RAISE: + pass + except Exception as exc: # noqa: BLE001 + pytest.fail(f"read_info raised {type(exc).__name__}: {exc}") + + +@pytest.mark.xfail(strict=True, reason=( + "braw.py:333/335/338/344/349 index and unpack at offsets derived from an " + "atom's *declared* size without checking the buffer actually extends that " + "far, so a short mdhd or a lying stts entry_count raises struct.error / " + "IndexError straight out of read_info")) +@settings(deadline=None, suppress_health_check=[HealthCheck.too_slow]) +@given(mdhd=st.binary(max_size=40), stts=st.binary(max_size=40), + trailing=st.lists(atom_trees(2), max_size=2)) +def test_braw_read_info_survives_truncated_video_traks(mdhd, stts, trailing, + tmp_path_factory): + """The sweep above never finds the crash because a random tree almost never + grows the `vide` handler that `_read_timing` requires. Pin that shape and + fuzz only the two atom bodies it then reads at fixed offsets, which is + exactly where the bound checks are missing.""" + payload = video_trak(mdhd, stts) + b"".join(trailing) + target = write_braw(tmp_path_factory.mktemp("braw"), braw_file(payload)) + try: + braw.read_info(target) + except PARSER_MAY_RAISE: + pass + except Exception as exc: # noqa: BLE001 + pytest.fail(f"read_info raised {type(exc).__name__}: {exc}") + + +@pytest.mark.parametrize("label, mdhd, stts", [ + ("mdhd carries no body at all", + b"", b"\x00" * 4 + struct.pack(">I", 1)), + ("mdhd stops before the v0 timescale", + b"\x00" * 4, b"\x00" * 4 + struct.pack(">I", 1)), + ("mdhd claims v1 but is only v0-sized", + b"\x01" + b"\x00" * 19, b"\x00" * 4 + struct.pack(">I", 1)), + ("stts entry_count promises entries that are not there", + GOOD_MDHD, b"\x00" * 4 + struct.pack(">I", 4096)), +]) +@pytest.mark.xfail(strict=True, reason=( + "braw._read_timing trusts the declared atom size over the real buffer " + "length; see braw.py:333-350")) +def test_braw_truncated_atoms_do_not_raise(label, mdhd, stts, tmp_path): + """Minimised regressions for the structured sweep above.""" + target = write_braw(tmp_path, braw_file(video_trak(mdhd, stts))) + braw.read_info(target) + + +@pytest.mark.xfail(strict=True, reason=( + "braw._descend (braw.py:225) recurses once per nested container with no " + "depth limit, so ~1000 nested atoms exhaust the interpreter stack")) +def test_braw_deep_nesting_does_not_exhaust_the_stack(tmp_path): + """8 bytes per level, so a 16 KB file is enough to blow the stack.""" + nest = atom(b"stbl") + for _ in range(2000): + nest = atom(b"minf", nest) + target = write_braw(tmp_path, braw_file(atom(b"trak", atom(b"mdia", nest)))) + braw.read_info(target) + + +class _RecordingHandle: + """A file handle that remembers the largest read it was ever asked for. + + Used instead of actually planting a 1 TB atom size, which would prove the + same point by exhausting the machine's memory. + """ + + def __init__(self, data: bytes) -> None: + self._data = data + self._pos = 0 + self.largest_read = 0 + + def read(self, size: int = -1) -> bytes: + if size is None or size < 0: + size = len(self._data) - self._pos + self.largest_read = max(self.largest_read, size) + chunk = self._data[self._pos:self._pos + size] + self._pos += len(chunk) + return chunk + + def seek(self, pos: int, whence: int = 0) -> int: + self._pos = pos if whence == 0 else self._pos + pos + return self._pos + + def tell(self) -> int: + return self._pos + + +@pytest.mark.xfail(strict=True, reason=( + "braw._find_moov (braw.py:198) does `handle.read(size - header_length)` " + "with a size taken straight from the file (up to 2**64-1 via the " + "extended-size path), without the `offset + size > file_size` check that " + "check_container (braw.py:177) does perform")) +@given(claimed=st.integers(min_value=2 ** 32, max_value=2 ** 48)) +@settings(deadline=None) +def test_find_moov_never_reads_past_the_end_of_the_file(claimed: int): + """A moov header may claim any size it likes. The parser must not believe + it: the read it issues has to be bounded by the file that is actually + there.""" + data = atom(b"ftyp", b"braw") + atom(b"moov", b"", declared=claimed) + handle = _RecordingHandle(data) + braw._find_moov(cast("BinaryIO", handle), len(data)) + assert handle.largest_read <= len(data), ( + f"asked the OS for {handle.largest_read:,} bytes " + f"from a {len(data):,}-byte file") + + +# ------------------------------------------- one bad file must not kill a job + + +def _offload(source: Path, dest: Path, **overrides) -> Job: + options: dict[str, Any] = dict( + destinations=[dest], thumbnail_count=0, extra_probe=False, + verification=VerificationMode.FULL) + options.update(overrides) + return engine.run(source, OffloadOptions(**options)) + + +@pytest.mark.xfail(strict=True, reason=( + "engine.run calls probe_mod.probe(source) with no try/except, and " + "probe.probe's BRAW branch (probe.py:133-134) has none either, so any " + "braw.py crash aborts the whole job: the clips after the bad one are " + "never copied and no report is written")) +def test_one_malformed_clip_does_not_abort_the_offload(tmp_path): + """The scenario this tool exists for: a card with one bad clip. The bad + clip may fail. The other two must still land, and the job must still + return a Job that says so.""" + source, dest = tmp_path / "card", tmp_path / "dst" + source.mkdir() + (source / "A001_C001.mov").write_bytes(b"A" * 5000) + write_braw(source, braw_file(video_trak(b"", b"\x00" * 4 + struct.pack(">I", 1))), + "A001_C002.braw") + (source / "A001_C003.mov").write_bytes(b"C" * 5000) + + job = _offload(source, dest, extra_probe=True) + + copied = {p.name for p in dest.rglob("*") if p.is_file()} + assert {"A001_C001.mov", "A001_C003.mov"} <= copied, ( + f"a malformed clip stopped the good ones landing; got {sorted(copied)}") + assert len(job.files) == 3 + + +# --------------------------------------------------- destination injectivity + + +def test_destination_paths_are_injective_when_structure_is_preserved(tmp_path): + """The control case: with the tree preserved, distinct sources cannot + collide. This one passes, and pins the behaviour the flat case breaks.""" + source, dest = tmp_path / "card", tmp_path / "dst" + (source / "A").mkdir(parents=True) + (source / "B").mkdir(parents=True) + (source / "A" / "clip.mov").write_bytes(b"A" * 1000) + (source / "B" / "clip.mov").write_bytes(b"B" * 2000) + + _offload(source, dest) + + landed = sorted(p.stat().st_size for p in dest.rglob("*") if p.is_file()) + assert landed == [1000, 2000] + + +@pytest.mark.xfail(strict=True, reason=( + "engine._destination_for (engine.py:232) maps every source onto " + "`dest_root / source.name` when preserve_structure is False, and nothing " + "checks two sources for the same target. engine.py:85's `seen` dict " + "dedupes destination *roots* only. Reachable as `offloader --flat` and as " + "the GUI's 'Recreate the source folder structure' checkbox")) +@settings(max_examples=30, deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture, + HealthCheck.too_slow]) +@given(folders=st.lists(st.text(alphabet="ABCDEFG", min_size=1, max_size=3), + min_size=2, max_size=5, unique=True), + basename=st.sampled_from(["clip.mov", "A001_C001.mov", "take.braw"])) +def test_flat_mode_never_silently_drops_a_file(folders, basename, tmp_path_factory): + """Every byte that leaves the card must arrive, or the job must say it + didn't. Reporting VERIFIED for a file that is not at the destination is the + one outcome a verified-copy tool may never produce.""" + root = tmp_path_factory.mktemp("flat") + source, dest = root / "card", root / "dst" + for index, folder in enumerate(folders): + (source / folder).mkdir(parents=True) + (source / folder / basename).write_bytes(bytes([65 + index]) * (1000 + index)) + + job = _offload(source, dest, preserve_structure=False) + + source_bytes = sum(p.stat().st_size for p in source.rglob("*") if p.is_file()) + dest_bytes = sum(p.stat().st_size for p in dest.rglob("*") if p.is_file()) + verified = [d for entry in job.files for d in entry.destinations + if d.status is FileStatus.VERIFIED] + + if dest_bytes < source_bytes: + pytest.fail( + f"{len(folders)} sources ({source_bytes} bytes) collapsed to " + f"{dest_bytes} bytes at the destination, and {len(verified)} of " + f"{len(job.files)} destinations still report VERIFIED") + + +@pytest.mark.xfail(strict=True, reason="same collision as the sweep above") +def test_flat_mode_collision_is_reported_not_silent(tmp_path): + """Minimised regression: two takes, one surviving file, a clean report.""" + source, dest = tmp_path / "card", tmp_path / "dst" + (source / "A").mkdir(parents=True) + (source / "B").mkdir(parents=True) + (source / "A" / "clip.mov").write_bytes(b"A" * 1000) + (source / "B" / "clip.mov").write_bytes(b"B" * 2000) + + job = _offload(source, dest, preserve_structure=False) + + landed = [p for p in dest.rglob("*") if p.is_file()] + failures = [d for entry in job.files for d in entry.destinations + if d.status is not FileStatus.VERIFIED] + assert len(landed) == 2 or failures, ( + f"{len(job.files)} files became {len(landed)} on disk, " + f"and not one destination reported a problem") + + +# ------------------------------------------------------------ scanning cycles + + +@pytest.mark.skipif(not WINDOWS, reason="directory junctions are Windows-only") +@pytest.mark.xfail(strict=True, reason=( + "engine.scan (engine.py:215) is a bare os.walk with no cycle guard, and " + "Path.is_symlink() is False for a junction, so the usual symlink check " + "would not help either. It terminates only because Windows refuses paths " + "past MAX_PATH; with long paths enabled it would not")) +def test_scan_does_not_follow_a_directory_junction_into_a_cycle(tmp_path): + card = tmp_path / "card" + (card / "sub").mkdir(parents=True) + (card / "sub" / "clip.mov").write_bytes(b"x" * 10) + + made = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(card / "sub" / "loop"), str(card)], + capture_output=True, text=True) + if made.returncode != 0: + pytest.skip(f"could not create a junction: {made.stderr.strip()}") + + found: list[int] = [] + worker = threading.Thread(target=lambda: found.append(len(engine.scan(card))), + daemon=True) + worker.start() + worker.join(timeout=30) + assert not worker.is_alive(), "scan() did not terminate on a junction cycle" + assert found == [1], ( + f"one real file was scanned {found[0]} times through the junction") + + +# ----------------------------------------------------------- report totality + + +def _job_named(name: str, root: Path) -> Job: + when = _dt.datetime(2026, 8, 4, 12, 0).timestamp() + entry = FileEntry( + source=root / name, source_root=root, size=10, created=when, + modified=when, checksum="abc123", media=MediaInfo(), + destinations=[Destination(root=root, path=root / name, + status=FileStatus.VERIFIED, checksum="abc123", + created=when, modified=when)], + ) + return Job(name="A001", source_root=root, destination_roots=[root], + verification=VerificationMode.SOURCE_ONLY, + hash_label="XXHash3-64", + started=_dt.datetime(2026, 8, 4, 5, 0), + finished=_dt.datetime(2026, 8, 4, 6, 0), files=[entry]) + + +#: Names a real filesystem can hand back. The lone surrogate is not exotic: it +#: is what os.listdir returns via surrogateescape for any POSIX filename that +#: is not valid UTF-8, and NTFS accepts unpaired surrogates outright. +UNENCODABLE_NAMES = ["clip\ud800.mov", "clip\udcff.mov"] +ILLEGAL_XML_NAMES = ["clip\x00.mov", "clip\x0b.mov", "clip\x1f.mov"] + + +@pytest.mark.parametrize("name", ILLEGAL_XML_NAMES + UNENCODABLE_NAMES) +def test_write_mhl_is_total(name, tmp_path): + """reports/mhl.py filters to the XML 1.0 character set through `_xml_safe` + (reports/mhl.py:31-46) before ElementTree sees anything. It holds up: this + is the behaviour the other two writers are missing.""" + out = write_mhl(_job_named(name, tmp_path), tmp_path / "JobReport.mhl") + assert ET.parse(out).getroot().tag == "hashlist" + + +@pytest.mark.parametrize("name", UNENCODABLE_NAMES) +@pytest.mark.xfail(strict=True, reason=( + "csv_report.py:47 opens with the default errors='strict' and does no " + "filtering, so writer.writerow (csv_report.py:96) raises " + "UnicodeEncodeError. write_mhl survives the same name because it has " + "_xml_safe; write_csv has no equivalent")) +def test_write_csv_is_total(name, tmp_path): + """A report writer runs *after* the bytes are safely copied. Crashing there + turns a finished offload into one with no paperwork.""" + write_csv(_job_named(name, tmp_path), tmp_path / "JobReport.csv") + + +# ------------------------------------------------------- ASC MHL round trip + + +@pytest.mark.parametrize("name", ["clip.mov", "a b.mov", "café.mov", "клип.mov"]) +def test_ascmhl_round_trips_ordinary_names(name, tmp_path): + """The chain of custody is only worth anything if what was written can be + read back. This is the control case.""" + manifest = ascmhl.write_manifest(_job_named(name, tmp_path), tmp_path) + assert ascmhl.read_manifest_hashes(manifest), "wrote a manifest, read back nothing" + + +@pytest.mark.parametrize("name", ILLEGAL_XML_NAMES + UNENCODABLE_NAMES) +@pytest.mark.xfail(strict=True, reason=( + "ascmhl.py has no _xml_safe equivalent: the text at ascmhl.py:355 " + "is written raw. Control characters produce a manifest that will not " + "reparse, and read_manifest_hashes swallows the ParseError " + "(ascmhl.py:176) and returns {}, so the file vanishes from the chain of " + "custody silently. A lone surrogate fails earlier, in the UTF-8 encode at " + "ascmhl.py:212")) +def test_ascmhl_round_trips_hostile_names(name, tmp_path): + """Whatever the name, `write_manifest` must either refuse it outright or + produce a manifest `read_manifest_hashes` can fully recover. Writing a + manifest that silently reads back empty is the one thing it may not do.""" + manifest = ascmhl.write_manifest(_job_named(name, tmp_path), tmp_path) + assert ascmhl.read_manifest_hashes(manifest), ( + "manifest was written but reads back empty: the file is now missing " + "from the chain of custody with nothing to indicate it") + + +# -------------------------------------------------------------- retry policy + + +@pytest.mark.xfail(strict=True, reason=( + "RetryPolicy is a frozen dataclass with no __post_init__ validation, and " + "presets.py builds one straight from presets.json, so a negative delay " + "reaches retry.py:114's `if pause: sleep(pause)`, and time.sleep raises " + "ValueError on a negative argument")) +@given(attempts=st.integers(min_value=-5, max_value=12), + delay=st.floats(min_value=-10, max_value=10, allow_nan=False, + allow_infinity=False), + backoff=st.floats(min_value=-4, max_value=4, allow_nan=False, + allow_infinity=False)) +@settings(deadline=None) +def test_retry_never_asks_to_sleep_for_a_negative_time(attempts, delay, backoff): + """`RetryPolicy` is a frozen dataclass with no validation, and presets.py + builds one straight from presets.json. A hand-edited negative delay must + not reach time.sleep, which raises ValueError on a negative argument.""" + policy = retry.RetryPolicy(attempts=attempts, delay=delay, backoff=backoff) + slept: list[float] = [] + calls = {"n": 0} + + def flaky(): + calls["n"] += 1 + if calls["n"] < 3: + raise OSError(errno.EIO, "transient") + return "ok" + + try: + retry.call(flaky, policy, sleep=slept.append) + except OSError: + pass + + assert all(pause >= 0 for pause in slept), ( + f"policy(delay={delay}, backoff={backoff}) asked for sleeps {slept}; " + "time.sleep raises ValueError on a negative argument") + + +@pytest.mark.xfail(strict=True, reason=( + "hashers.hash_file's chunk_size is unvalidated: read(0) returns b'' " + "immediately, so iter(..., b'') stops before the first byte and a " + "non-empty file hashes as empty. Not reachable from the CLI today, which " + "is the only reason this is not a live data-integrity bug")) +def test_hash_file_rejects_a_zero_chunk_size(tmp_path): + """A checksum that silently describes a different file than the one on + disk is the worst failure mode this codebase has.""" + target = tmp_path / "clip.mov" + target.write_bytes(b"X" * 100_000) + empty = tmp_path / "empty.bin" + empty.write_bytes(b"") + + with pytest.raises(ValueError): + digest = hash_file(target, "xxh3-64", chunk_size=0) + assert digest != hash_file(empty, "xxh3-64"), ( + "a 100 KB file hashed to the empty-file digest") + + +# ------------------------------------------------- things that already hold + + +@given(st.lists(st.text(max_size=12), max_size=6)) +@settings(deadline=None) +def test_exclude_patterns_never_raise(patterns): + """Exclude globs come from `--exclude` and from presets.json, so they are + user input. fnmatch tolerates unclosed brackets; confirm that stays true.""" + engine.is_excluded(Path("clip.mov"), patterns) + + +def test_empty_source_tree_produces_an_empty_job(tmp_path): + """A card that mounted but scanned empty must not divide by a zero total.""" + source, dest = tmp_path / "card", tmp_path / "dst" + source.mkdir() + events: list[object] = [] + job = engine.run(source, OffloadOptions(destinations=[dest], thumbnail_count=0, + extra_probe=False), progress=events.append) + assert job.files == [] + + +def test_zero_byte_file_verifies(tmp_path): + """Cameras do leave 0-byte files behind after a card pull.""" + source, dest = tmp_path / "card", tmp_path / "dst" + source.mkdir() + (source / "empty.mov").write_bytes(b"") + job = _offload(source, dest) + assert [d.status for entry in job.files for d in entry.destinations] == \ + [FileStatus.VERIFIED] + assert (dest / "empty.mov").stat().st_size == 0 + + +# ----------------------------------------------------------- name exhaustion + + +@pytest.mark.parametrize("template, taken", [ + ("{index}", [f"{i:03d}" for i in range(1, 1000)]), + ("{card}", ["A001"] + [f"A001-{i}" for i in range(2, 1000)]), +]) +@pytest.mark.xfail(strict=True, reason=( + "naming.build's two exhaustion fallbacks return an unchecked name: " + "naming.py:82 returns render(..., index=999) and naming.py:91 returns " + "`base`, neither tested against `used`. Two jobs then share one report " + "folder, which is the exact outcome the dedup exists to prevent")) +def test_build_never_returns_a_taken_name_even_when_exhausted(template, taken): + """`test_fuzz.py` already asserts this property, but with `max_size=8` it + can never reach the 999-name ceiling where the guarantee lapses.""" + name = naming.build(template, Path("A001"), taken=taken) + assert name.casefold() not in {t.casefold() for t in taken}, ( + f"build() handed back {name!r}, which is already taken") + + +# ---------------------------------------------------------- corrupt history + + +CORRUPT_HISTORY = [ + pytest.param("42", id="a bare number"), + pytest.param('["oops"]', id="a list of strings"), + pytest.param('[{"file_count": [1, 2]}]', id="file_count is a list"), + pytest.param('[{"destinations": 7}]', id="destinations is a number"), +] + + +@pytest.mark.parametrize("body", CORRUPT_HISTORY) +@pytest.mark.xfail(strict=True, reason=( + "History.__init__ maps HistoryEntry.from_dict over read_json's result with " + "no try/except, and from_dict itself does bare int()/list() conversions, " + "so a corrupt history.json raises out of the constructor")) +def test_corrupt_history_does_not_stop_an_offload(body, tmp_path): + """history.py's own reason for existing is that a damaged history must + never stop someone offloading a card. An exception from the constructor + stops exactly that.""" + path = tmp_path / "history.json" + path.write_text(body, encoding="utf-8") + History(path) + + +@pytest.mark.xfail(strict=True, reason=( + "config.read_json catches OSError and json.JSONDecodeError, but " + "path.read_text raises UnicodeDecodeError (a ValueError) for invalid " + "UTF-8, which is not caught")) +def test_history_survives_invalid_utf8(tmp_path): + path = tmp_path / "history.json" + path.write_bytes(b'[{"job_name": "\xff\xfe"}]') + History(path) + + +def test_valid_history_still_round_trips(tmp_path): + """The control case for the two above: a well-formed file must load.""" + path = tmp_path / "history.json" + path.write_text(json.dumps([]), encoding="utf-8") + assert History(path).entries == [] From ed6cdc0873f90c4a006be6e222091f2c45ce2880 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:11:11 -0600 Subject: [PATCH 02/14] Edge cases between the bytes and the paperwork: 11 more bugs test_fuzz_edges.py goes after the copy engine and the binary parsers. This goes after everything downstream: what ffprobe hands back, what a number formats to, what a digest compares equal to, what a name renders to. Each failure is a value that is legal where it is produced and fatal, or silently wrong, where it is consumed, because the two ends disagree about the contract: - ffprobe writes "N/A" in a numeric field and _build calls int() on it. This is the realistic version of "one bad file kills the job" that the BRAW tests found the hard way: no crafted file needed, just a clip whose audio stream ffprobe cannot fully describe. _build sits outside the try that catches SubprocessError and JSONDecodeError, so the ValueError escapes probe() and then engine.run(). - _parse_rate accepts "inf/1" because float() does, and every fps formatter then calls round() on it, which does not. Both ends are tested here so whichever one moves, the other is pinned. - Another tool writes its hex digests uppercase and verify.py compares with ==, so every byte-identical file reports MISMATCH. The fix has to be per-algorithm rather than a blanket .lower(), because C4 digests are base58 and genuinely case-sensitive. - render substitutes token values into the string it is still substituting into, so a card folder literally named {index} has its name overwritten by the sequence number. - format_size tests `< 1000` before rounding and before considering sign, so it prints a 1000.0 mantissa at every decade boundary and never promotes a negative size out of bytes. - fingerprint hashes the joined file lines and nothing else, so every empty source shares da39a3ee, the SHA-1 of nothing. Two of the CLI findings turned out to be non-bugs on inspection and are kept as passing controls rather than dropped: argparse's choices= catches an unknown --verify or --profile before the enum constructor runs, so there is no traceback, and _parse_rate is total over rationals and junk. Knowing which neighbouring paths are already safe is what makes the xfails above worth trusting. --- tests/test_edge_cases.py | 325 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 325 insertions(+) create mode 100644 tests/test_edge_cases.py diff --git a/tests/test_edge_cases.py b/tests/test_edge_cases.py new file mode 100644 index 0000000..cbfedd6 --- /dev/null +++ b/tests/test_edge_cases.py @@ -0,0 +1,325 @@ +"""Edge cases in the layers between the bytes and the paperwork. + +`test_fuzz_edges.py` goes after the copy engine and the binary parsers. This +module goes after everything downstream of them: what ffprobe hands back, what +a number formats to, what a digest compares equal to, what a name renders to. + +The failures here share a shape. Each is a value that is perfectly legal at the +point it is produced and fatal (or silently wrong) at the point it is consumed, +because the two ends disagree about the contract: + + * ffprobe writes "N/A" in a numeric field. `_build` calls `int()` on it. + * `_parse_rate` accepts "inf/1" because `float()` does. Every fps formatter + then calls `round()` on it, which does not. + * Another tool writes its hex digests uppercase. `verify` compares with `==`. + * `render` substitutes token values into a string it then keeps substituting + into. + +Tests marked `xfail(strict=True)` are reproduced, not hypothesised. Fixing one +turns it XPASS and pytest will ask for the marker back. + +Run longer sweeps with: pytest --fuzz tests/test_edge_cases.py +""" + +from __future__ import annotations + +import datetime as _dt +import json +import math +import re +import subprocess +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hypothesis import given, settings +from hypothesis import strategies as st + +from offloader import config, history, naming, probe, util, verify +from offloader.hashers import hash_file +from offloader.models import ( + Destination, + FileEntry, + FileStatus, + Job, + MediaInfo, + VerificationMode, +) +from offloader.reports import write_mhl + +# ------------------------------------------------------- ffprobe says "N/A" + + +#: ffprobe emits the literal string "N/A" in numeric fields all the time: for +#: codecs that do not declare a sample rate, for data and attachment streams, +#: and for anything it only partially decoded. It is ordinary output, not +#: corruption. +NA_STREAMS = [ + pytest.param({"codec_type": "audio", "channels": "2", "sample_rate": "N/A"}, + id="sample_rate is N/A"), + pytest.param({"codec_type": "audio", "channels": "N/A"}, + id="channels is N/A"), + pytest.param({"codec_type": "audio", "channels": 2, "bit_rate": "N/A"}, + id="bit_rate is N/A"), +] + + +def test_probe_build_handles_healthy_output(): + """Control: the shape ffprobe produces for an ordinary clip.""" + info = probe._build({ + "streams": [{"codec_type": "video", "codec_name": "h264", "width": 1920, + "height": 1080, "r_frame_rate": "24/1", "duration": "10"}], + "format": {"format_name": "mov", "duration": "10"}, + }) + assert info.fps == 24.0 + + +@pytest.mark.parametrize("stream", NA_STREAMS) +@pytest.mark.xfail(strict=True, reason=( + "probe._build does bare int()/float() on ffprobe's audio fields " + "(probe.py:208, 211, 212) with no try/except, unlike the duration field " + "at probe.py:166-169 which is guarded. _build is called at probe.py:152, " + "*outside* the try that catches SubprocessError/JSONDecodeError, so the " + "ValueError escapes probe() and then engine.run(), which does not guard " + "the probe call either")) +def test_probe_build_survives_na_fields(stream): + """This is the realistic version of 'one bad file kills the job'. It needs + no crafted BRAW, only a clip whose audio stream ffprobe cannot fully + describe.""" + probe._build({"streams": [stream], "format": {}}) + + +@pytest.mark.xfail(strict=True, reason=( + "probe._build assumes every entry of `streams` is a dict (probe.py:158)")) +def test_probe_build_survives_a_non_dict_stream(): + probe._build({"streams": ["garbage"], "format": {}}) + + +@pytest.mark.xfail(strict=True, reason=( + "same unguarded _build as above, reached the way it is reached in " + "production: through probe.probe() with ffprobe returning real output")) +def test_probe_survives_an_ffprobe_that_reports_na(tmp_path, monkeypatch): + """End to end through the public entry point, with ffprobe stubbed to + return output it genuinely produces.""" + clip = tmp_path / "clip.mov" + clip.write_bytes(b"\x00" * 64) + payload = json.dumps({ + "streams": [{"codec_type": "audio", "channels": "2", "sample_rate": "N/A"}], + "format": {"format_name": "mov"}, + }) + monkeypatch.setattr( + probe.subprocess, "run", + lambda *a, **k: SimpleNamespace(returncode=0, stdout=payload, stderr="")) + + assert isinstance(probe.probe(clip), MediaInfo) + + +# -------------------------------------------------- non-finite frame rates + + +@pytest.mark.parametrize("rate", ["inf/1", "-inf/1", "nan/1", "Infinity/1"]) +@pytest.mark.xfail(strict=True, reason=( + "probe._parse_rate (probe.py:66-79) guards division by zero but not the " + "conversion itself: float('inf') and float('nan') do not raise, so a " + "non-finite rate flows into MediaInfo.fps and reaches every formatter")) +def test_parse_rate_never_returns_a_non_finite_number(rate): + """A frame rate is a physical quantity. The parser is the right place to + refuse a value that is not one, because nothing downstream checks.""" + parsed = probe._parse_rate(rate) + assert parsed is None or math.isfinite(parsed) + + +@pytest.mark.parametrize("fps", [float("inf"), float("nan")]) +@pytest.mark.xfail(strict=True, reason=( + "util.format_fps and util.format_timecode both call round()/int() on fps " + "before any guard can help: round(nan) raises ValueError and round(inf) " + "raises OverflowError. The `or 1` fallback in format_timecode's " + "`int(round(fps)) or 1` never runs, because round() raises first")) +def test_fps_formatters_accept_whatever_parse_rate_produces(fps): + """The two ends of the same contract. `_parse_rate` can produce these, so + the formatters have to survive them, or one of the two has to change.""" + util.format_fps(fps) + util.format_timecode(240, fps) + + +# --------------------------------------------------------- size formatting + + +SIZE_SHAPE = re.compile(r"-?\d+(?:\.\d+)? (bytes|KB|MB|GB|TB)") + + +@given(size=st.integers(min_value=-10 ** 15, max_value=-1)) +@settings(deadline=None) +@pytest.mark.xfail(strict=True, reason=( + "util.format_size's first branch is `num_bytes < 1000`, which is true for " + "every negative number regardless of magnitude, so a negative size is " + "printed verbatim in bytes and never promotes to KB/MB/GB/TB")) +def test_format_size_promotes_negative_values_too(size: int): + """`test_fuzz.py` pins this property for non-negative sizes only. A byte + count that can go negative anywhere (history.py uses -1 as its stat-failure + sentinel) should still render in a sane unit.""" + text = util.format_size(size) + assert SIZE_SHAPE.fullmatch(text), text + if abs(size) >= 1000: + assert not text.endswith(" bytes"), ( + f"{size} rendered as {text!r} instead of promoting to a larger unit") + + +@pytest.mark.parametrize("size, unit", [ + (999_999, "KB"), (999_999_999, "MB"), (999_999_999_999, "GB"), +]) +@pytest.mark.xfail(strict=True, reason=( + "util.format_size compares against 1000 before rounding to one or two " + "decimals, so a value just under a decade boundary rounds up to a " + "mantissa of 1000.0 in the smaller unit instead of promoting")) +def test_format_size_mantissa_never_reaches_1000(size: int, unit: str): + """'1000.0 MB' is not wrong by much, but it is a unit the function's own + branch structure says it will never print.""" + text = util.format_size(size) + mantissa, printed = text.rsplit(" ", 1) + assert float(mantissa) < 1000, ( + f"{size} rendered as {text!r}; expected promotion out of {unit}") + assert printed != unit, f"{size} should have promoted past {unit}" + + +@pytest.mark.parametrize("value", [float("nan"), float("inf")]) +@pytest.mark.xfail(strict=True, reason=( + "every `< 1000` comparison is False for nan and inf, so format_size falls " + "through all of its branches and returns the last one, producing " + "'nan TB' / 'inf TB'")) +def test_format_size_does_not_invent_a_unit_for_non_finite_input(value): + text = util.format_size(value) + assert SIZE_SHAPE.fullmatch(text), text + + +# --------------------------------------------------- digest case sensitivity + + +def _single_file_job(root: Path, digest: str, clip: Path) -> Job: + when = _dt.datetime(2026, 8, 4, 12, 0).timestamp() + entry = FileEntry( + source=clip, source_root=root, size=clip.stat().st_size, created=when, + modified=when, checksum=digest, media=MediaInfo(), + destinations=[Destination(root=root, path=clip, + status=FileStatus.VERIFIED, checksum=digest, + created=when, modified=when)], + ) + return Job(name="A001", source_root=root, destination_roots=[root], + verification=VerificationMode.SOURCE_ONLY, + hash_label="XXHash3-64", + started=_dt.datetime(2026, 8, 4, 5, 0), + finished=_dt.datetime(2026, 8, 4, 6, 0), files=[entry]) + + +def _manifest_for(tmp_path: Path) -> tuple[Path, str]: + clip = tmp_path / "clip.mov" + clip.write_bytes(b"X" * 4096) + digest = hash_file(clip, "xxh3-64") + manifest = write_mhl(_single_file_job(tmp_path, digest, clip), + tmp_path / "A001.mhl") + return manifest, digest + + +def test_verify_passes_a_manifest_we_wrote_ourselves(tmp_path): + """Control: our own lowercase-hex manifest verifies clean.""" + manifest, _ = _manifest_for(tmp_path) + assert verify.verify_manifest(manifest).counts() == {"ok": 1} + + +@pytest.mark.xfail(strict=True, reason=( + "verify.py:201 compares digests with a plain case-sensitive `==`, so a " + "manifest written by a tool that emits uppercase hex reports every file " + "as MISMATCH. Note the fix is per-algorithm, not a blanket .lower(): C4 " + "digests are base58 and genuinely case-sensitive (hashers.py:53-59)")) +def test_verify_accepts_an_uppercase_hex_manifest(tmp_path): + """MHL is an interchange format. Telling an editor their footage is + corrupt because another tool shifted the case of a hex digit is the most + expensive kind of false alarm this tool can raise.""" + manifest, digest = _manifest_for(tmp_path) + shouty = tmp_path / "A001_upper.mhl" + shouty.write_text( + manifest.read_text(encoding="utf-8").replace(digest, digest.upper()), + encoding="utf-8") + + report = verify.verify_manifest(shouty) + assert report.counts() == {"ok": 1}, ( + f"byte-identical file reported as {report.summary()}") + + +# ---------------------------------------------------------- name rendering + + +@pytest.mark.xfail(strict=True, reason=( + "naming.render applies str.replace once per token in sequence, over the " + "string it is accumulating, so a token value that itself contains token " + "text is substituted again by a later pass")) +def test_render_does_not_substitute_into_a_value_it_already_placed(): + """A folder literally named `{index}` is a legal directory name. Rendering + `{card}` from it puts the text `{index}` into the result, and the later + `{index}` pass then overwrites the card name with the sequence number.""" + values = naming.context(Path("{index}"), index=7) + assert values["card"] == "{index}" + assert naming.render("{card}_{index}", values) == "{index}_007" + + +# -------------------------------------------------------------- fingerprint + + +@pytest.mark.xfail(strict=True, reason=( + "history.fingerprint hashes the joined per-file lines and nothing else, " + "so an empty file list hashes the empty string: da39a3ee..., the SHA-1 of " + "nothing, for every source root there has ever been")) +def test_empty_sources_do_not_share_one_fingerprint(): + """The fingerprint decides whether a card has been offloaded before. Two + different empty volumes must not look like the same prior job.""" + assert history.fingerprint([], Path("/cardA")) != \ + history.fingerprint([], Path("/cardB")) + + +# ------------------------------------------------------------ config paths + + +@pytest.mark.parametrize("name", ["D:/evil.json", "/etc/passwd"]) +@pytest.mark.xfail(strict=True, reason=( + "config.config_file joins with pathlib's `/`, which discards the left " + "operand entirely when the right operand is absolute, so an absolute " + "`name` silently resolves outside the config directory")) +def test_config_file_stays_inside_the_config_directory(name: str): + """Only ever called with literals today, which is the single reason this + is latent rather than live.""" + resolved = config.config_file(name).resolve() + assert config.config_dir().resolve() in resolved.parents + + +# ----------------------------------------------- things that already hold + + +@pytest.mark.parametrize("flag", ["--verify", "--profile"]) +def test_cli_rejects_an_unknown_enum_value_without_a_traceback(flag: str): + """models.py's enums define no `_missing_`, so constructing one from an + unknown string raises a bare ValueError. argparse's `choices=` catches + these first, so the user gets a usage error rather than a stack trace. + That guard is the only thing standing between the two.""" + result = subprocess.run( + [sys.executable, "-m", "offloader", "offload", "nonexistent-source", + "nonexistent-dest", flag, "definitely-not-a-real-value"], + capture_output=True, text=True) + assert result.returncode == 2 + assert "invalid choice" in result.stderr + assert "Traceback" not in result.stderr + + +@given(numerator=st.integers(min_value=-10 ** 6, max_value=10 ** 6), + denominator=st.integers(min_value=-10 ** 6, max_value=10 ** 6)) +@settings(deadline=None) +def test_parse_rate_never_raises_on_a_rational(numerator: int, denominator: int): + """Whatever integers ffprobe puts either side of the slash, including a + zero denominator, the parser returns a value or None.""" + probe._parse_rate(f"{numerator}/{denominator}") + + +@given(text=st.text(max_size=24)) +@settings(deadline=None) +def test_parse_rate_never_raises_on_junk(text: str): + probe._parse_rate(text) From 821b39b88e95c2de2bba3e8342657f65e5f74512 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:41:12 -0600 Subject: [PATCH 03/14] Fix all 24 bugs the fuzz suites found The two suites go from 49 strict xfails to none: every test that was pinning a reproduction is now pinning a regression. 482 pass, and the 3000-example soak passes too. Two of these were worth the whole exercise. Flattening could destroy a file and certify it. Every source mapped to destination/name with nothing checking two sources for one target, so two clips of the same name in different card folders left one file on disk and both rows reading VERIFIED. Each was verified as it landed, before the next overwrote it, so full verification passed them as well. The collision is now caught against the scan, before a byte moves, and the job refuses naming both paths. Comparison goes through os.path.normcase, which also catches the case-only collision I could not build a portable test for: two names distinct on the case-sensitive volume they came from, one file on the volume they are going to. One malformed clip aborted the whole offload. engine.run called probe() unguarded and probe() did not guard its own parsers. The realistic trigger was not a crafted file at all, it was ffprobe writing "N/A" in a numeric field, which it does routinely. Metadata is now contained at both call sites: the bytes are copied and verified before any of it runs, so an unreadable container costs its own metadata and a line in the report rather than the rest of the card. The rest, grouped by what they had in common: - Trusting a length that came out of the file. braw's _read_timing, its ilst loop, _descend's recursion and _find_moov's read are all now bounded by what is actually present. - Text that cannot survive the trip. The XML 1.0 filter that reports/mhl.py always had now lives in util and covers the CSV and ASC MHL writers too. ASC MHL filters where the filename first enters, so the structure hashes and the text they describe are computed from the same string. - Comparing unlike with unlike. Digests now compare per algorithm: case-folded for hex, exact for C4, whose base58 alphabet uses case to carry information. - Trusting a file we wrote. A damaged history.json no longer raises out of History(), and read_json treats invalid UTF-8 as unreadable rather than letting UnicodeDecodeError through, since that is a ValueError and not an OSError. - Arithmetic that ignored its own edges. format_size rounds before picking a unit and handles sign; _parse_rate rejects a rate that is not finite and positive; naming.build's searches are bounded by the number of taken names instead of a fixed 999. Two behaviour changes worth knowing about. RetryPolicy now clamps rather than rejects, because presets.json is hand-editable and a nonsensical delay should not raise mid-transfer. And history fingerprints are seeded with the source's name, so entries recorded before this change will not match: duplicate-card detection degrades to not warning, which is the safe direction, and re-records correctly on the next offload. --- CHANGELOG.md | 80 +++++++++++++++++ src/offloader/ascmhl.py | 29 ++++++ src/offloader/braw.py | 100 ++++++++++++++++----- src/offloader/config.py | 18 +++- src/offloader/engine.py | 114 ++++++++++++++++++++---- src/offloader/hashers.py | 19 +++- src/offloader/history.py | 76 +++++++++++++--- src/offloader/naming.py | 36 +++++--- src/offloader/probe.py | 93 +++++++++++++++----- src/offloader/reports/csv_report.py | 21 ++++- src/offloader/reports/mhl.py | 30 ++----- src/offloader/retry.py | 23 ++++- src/offloader/util.py | 122 ++++++++++++++++++++++---- src/offloader/verify.py | 7 +- tests/test_edge_cases.py | 69 ++------------- tests/test_fuzz_edges.py | 131 ++++++++++------------------ 16 files changed, 693 insertions(+), 275 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46aab63..d08606e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,86 @@ project uses [semantic versioning][semver]. ### Fixed +Found by fuzzing the edges: `tests/test_fuzz_edges.py` for what a card can +contain, `tests/test_edge_cases.py` for the layers between the bytes and the +paperwork. Each fix has the reproduction that found it. + +- **Flattening could silently destroy a file and certify it.** With + `--flat` (and the desktop app's "Recreate the source folder structure" + unticked), every source mapped to `destination / name`, and nothing checked + two sources for one target. Two clips of the same name in different card + folders left one file on disk and *both* rows reading VERIFIED, because each + was verified as it landed, before the next overwrote it. Full verification + passed them too. The collision is now detected against the scan, before a + byte moves, and the job refuses with both paths named. Paths compare through + `os.path.normcase`, so on Windows this also catches two names differing only + in case: distinct on the case-sensitive volume they came from, one file on + the volume they are going to. +- **One malformed clip aborted the whole offload.** `engine.run` called + `probe()` with no guard, and `probe()` did not guard its own parsers, so a + BRAW whose atom headers lied, or an ordinary clip whose audio stream ffprobe + described as `"N/A"`, raised out of the middle of a job. The clips after it + were never copied and no report was written. Metadata is now contained: the + bytes are already copied and verified by that point, so an unreadable + container costs its own metadata and a line in the report, not the rest of + the card. +- **The BRAW parser trusted the file's own size fields.** `_read_timing` + indexed and unpacked at offsets derived from an atom's *declared* size + without checking the buffer reached that far; `_descend` recursed once per + nested container with no depth limit, so a 16 KB file exhausted the stack; + and `_find_moov` issued a read for whatever size the header claimed, up to + 2^64. Offsets are now bounded by each atom's real end, depth is capped, and + the read is clamped to the bytes actually present. +- **A filename could stop the paperwork.** `write_csv` wrote to a strict UTF-8 + stream with no filtering, so a name carrying a lone surrogate (what + `os.listdir` returns for any POSIX name that is not valid UTF-8, and what + NTFS accepts outright) raised mid-row after the copy had succeeded. ASC MHL + had the same gap in a worse place: control characters produced a manifest + that would not reparse, and `read_manifest_hashes` answers a parse failure + with an empty dict, so the file left the chain of custody with nothing to + show for it. The XML 1.0 character filter that `reports/mhl.py` always had + now lives in `util` and covers all three writers. +- **Verify called good footage corrupt.** Digests were compared with a plain + case-sensitive `==`, so a manifest from a tool that emits uppercase hex + reported every byte-identical file as a mismatch. Comparison is now per + algorithm: case-folded for hex, exact for C4, whose base58 alphabet uses + case to carry information. +- **A directory junction sent the scanner round in circles.** `scan` was a bare + `os.walk` with no cycle guard, and `Path.is_symlink()` is False for a + junction, so the usual check would not have helped. It terminated only + because Windows refuses paths past MAX_PATH, having by then returned the same + file dozens of times. Directories are now visited at most once each. +- **A corrupt history blocked offloading**, which is precisely what it exists + not to do. `History()` mapped `from_dict` over the file with no guard, and + `from_dict` did bare `int()`/`list()` conversions, so a hand-edited or + half-written `history.json` raised out of the constructor. A record that will + not parse is now dropped. `read_json` also treats a file that is not valid + UTF-8 as unreadable rather than letting `UnicodeDecodeError`, which is a + ValueError and not an OSError, escape. +- **Job names could collide after 999.** `naming.build`'s two exhaustion + fallbacks returned a name without checking it was free, so two jobs would + share one report folder: the exact outcome the deduplication exists to + prevent. Both searches are now bounded by the number of taken names, and the + suffix search always produces one more candidate than there are names to + avoid. +- **Byte counts printed a mantissa of 1000** at every decade boundary + (`999_999` rendered as `1000.0 KB`), and negative counts never promoted out + of bytes at all, because the unit test came before the rounding and ignored + the sign. `nan` rendered as `nan TB`. +- **A frame rate of `inf` crashed the report.** `_parse_rate` accepted `"inf"` + and `"nan"` because `float()` does, and every fps formatter then called + `round()` on the result, which does not. The parser now rejects a rate that + is not finite and positive, and the formatters degrade instead of raising. +- **Smaller ones, same sweep.** `hash_file` accepted a `chunk_size` of 0 and + returned the empty-file digest for a file that was not empty. `RetryPolicy` + validated nothing, so a hand-edited negative delay reached `time.sleep`, + which raises on one. `naming.render` substituted into values it had already + placed, so a card folder genuinely named `{index}` had its name overwritten + by the sequence number. `history.fingerprint` hashed the file listing and + nothing else, so every empty source shared one digest. `config_file` joined + with pathlib's `/`, which discards the left operand when the right is + absolute. + Found by adding CI on Linux and macOS — the suite had only ever run on Windows. - **A preset with explicit nulls loaded unusable.** `dict.get(key, default)` diff --git a/src/offloader/ascmhl.py b/src/offloader/ascmhl.py index 61e3934..fce12d1 100644 --- a/src/offloader/ascmhl.py +++ b/src/offloader/ascmhl.py @@ -24,6 +24,7 @@ from . import PRODUCT_NAME, __version__ from .hashers import ALGORITHMS, c4_of_bytes, get_algorithm, new_hasher from .models import FileStatus, Job +from .util import xml_safe ASCMHL_DIRNAME = "ascmhl" CHAIN_FILENAME = "ascmhl_chain.xml" @@ -201,10 +202,33 @@ def _previous_hashes(root: Path) -> dict[str, dict[str, str]]: # ------------------------------------------------------------------ writing +def _sanitise_tree(element: ET.Element) -> None: + """Filter every text node and attribute to what XML can actually carry. + + `` text is a filename off a filesystem. A control character in one + produces a manifest that will not parse, and `read_manifest_hashes` + answers a parse failure with an empty dict, so the file would drop out of + the chain of custody with nothing to show for it. A lone surrogate is + worse still: it fails in the UTF-8 encode below, before the manifest + exists at all. + + Done over the finished tree rather than at each assignment, so a call site + added later cannot forget. + """ + for node in element.iter(): + if node.text is not None: + node.text = xml_safe(node.text) + if node.tail is not None: + node.tail = xml_safe(node.tail) + for key, value in list(node.attrib.items()): + node.attrib[key] = xml_safe(value) + + def _indent_and_write(element: ET.Element, path: Path) -> None: # ElementTree writes the declaration with single quotes; the reference # implementation and the spec's examples use double. Match them, so a # manifest can be diffed against another tool's output directly. + _sanitise_tree(element) ET.indent(element, space=" ") declaration = '\n' body = ET.tostring(element, encoding="unicode") @@ -260,6 +284,11 @@ def write_manifest(job: Job, root: Path, *, destination_index: int = 0, relative = Path(destination.path).relative_to(root) except ValueError: continue + # Filter here, where a filesystem name first enters the manifest, so + # the structure hashes below and the text they describe are + # computed from the same string. A lone surrogate would otherwise + # reach _structure_entry's UTF-8 encode and abort the manifest. + relative = Path(xml_safe(relative.as_posix())) key = relative.as_posix() if destination.status is FileStatus.FAILED: diff --git a/src/offloader/braw.py b/src/offloader/braw.py index 6f11910..1e0c86d 100644 --- a/src/offloader/braw.py +++ b/src/offloader/braw.py @@ -23,6 +23,7 @@ from __future__ import annotations +import math import struct from dataclasses import dataclass, field from enum import Enum @@ -195,10 +196,28 @@ def _find_moov(handle: BinaryIO, file_size: int) -> bytes | None: for offset, size, atom_type, header_length in _top_level_atoms(handle, file_size): if atom_type == b"moov": handle.seek(offset + header_length) - return handle.read(size - header_length) + # `size` came out of the file and can claim up to 2**64-1 through + # the extended-size header. Ask only for what is actually there, + # the way check_container already does, or a corrupt clip turns + # into a request the allocator cannot serve. + available = max(0, file_size - (offset + header_length)) + return handle.read(min(size - header_length, available)) return None +def _uint(buf: bytes, offset: int, width: int, limit: int) -> int | None: + """A big-endian unsigned int, or None if it does not fit. + + Every fixed offset in this module is measured from an atom header whose + size the *file* declared. `limit` is that atom's real end, so a truncated + atom yields None here instead of a short slice and a struct.error three + frames up. + """ + if offset < 0 or offset + width > min(limit, len(buf)): + return None + return int.from_bytes(buf[offset:offset + width], "big") + + def _walk(buf: bytes, start: int, end: int): offset = start while offset + 8 <= end: @@ -216,13 +235,21 @@ def _walk(buf: bytes, start: int, end: int): offset += size -def _descend(buf: bytes, start: int, end: int, wanted: bytes): +#: Real BRAW nests moov/trak/mdia/minf/stbl about six deep. Anything past this +#: is a malformed or hostile file, and each level costs an interpreter frame. +_MAX_ATOM_DEPTH = 64 + + +def _descend(buf: bytes, start: int, end: int, wanted: bytes, depth: int = 0): """Depth-first search for an atom type inside a parsed buffer.""" + if depth >= _MAX_ATOM_DEPTH: + return None for offset, size, atom_type, header_length in _walk(buf, start, end): if atom_type == wanted: return offset, size, header_length if atom_type in _CONTAINERS: - found = _descend(buf, offset + header_length, offset + size, wanted) + found = _descend(buf, offset + header_length, offset + size, + wanted, depth + 1) if found: return found return None @@ -286,12 +313,20 @@ def _read_metadata_pairs(moov: bytes) -> dict[str, Any]: cursor = ilst_offset + ilst_header limit = ilst_offset + ilst_size while cursor + 16 <= limit: - item_size, index = struct.unpack(">II", moov[cursor:cursor + 8]) + item_size = _uint(moov, cursor, 4, limit) + index = _uint(moov, cursor + 4, 4, limit) + if item_size is None or index is None: + break if item_size < 16 or cursor + item_size > limit: break - data_size = struct.unpack(">I", moov[cursor + 8:cursor + 12])[0] - type_indicator = struct.unpack( - ">I", moov[cursor + 16:cursor + 20])[0] & 0x00FFFFFF + data_size = _uint(moov, cursor + 8, 4, limit) + # The loop condition only guarantees 16 bytes, but the type indicator + # sits at 16..20: an item declaring exactly 16 bytes at the end of the + # atom leaves nothing to read. + type_indicator = _uint(moov, cursor + 16, 4, limit) + if data_size is None or type_indicator is None: + break + type_indicator &= 0x00FFFFFF payload = moov[cursor + 24:cursor + 8 + data_size] if 1 <= index <= len(names): decoded = _decode_value(type_indicator, payload) @@ -329,25 +364,33 @@ def _read_timing(moov: bytes) -> tuple[float | None, float | None, int | None]: if not stts or not mdhd: continue - mdhd_offset = mdhd[0] - version = moov[mdhd_offset + 8] + mdhd_offset, mdhd_size, _ = mdhd + mdhd_end = mdhd_offset + mdhd_size + version = _uint(moov, mdhd_offset + 8, 1, mdhd_end) + if version is None: + continue if version == 1: - timescale = struct.unpack(">I", moov[mdhd_offset + 28:mdhd_offset + 32])[0] - duration = struct.unpack(">Q", moov[mdhd_offset + 32:mdhd_offset + 40])[0] + timescale = _uint(moov, mdhd_offset + 28, 4, mdhd_end) + duration = _uint(moov, mdhd_offset + 32, 8, mdhd_end) else: - timescale = struct.unpack(">I", moov[mdhd_offset + 20:mdhd_offset + 24])[0] - duration = struct.unpack(">I", moov[mdhd_offset + 24:mdhd_offset + 28])[0] - if not timescale: + timescale = _uint(moov, mdhd_offset + 20, 4, mdhd_end) + duration = _uint(moov, mdhd_offset + 24, 4, mdhd_end) + if not timescale or duration is None: continue - stts_offset = stts[0] - entry_count = struct.unpack( - ">I", moov[stts_offset + 12:stts_offset + 16])[0] + stts_offset, stts_size, _ = stts + stts_end = stts_offset + stts_size + entry_count = _uint(moov, stts_offset + 12, 4, stts_end) + if entry_count is None: + continue frames, delta = 0, None cursor = stts_offset + 16 for _ in range(min(entry_count, 4096)): - sample_count, sample_delta = struct.unpack( - ">II", moov[cursor:cursor + 8]) + # The count is the file's claim; the atom's own end is the fact. + sample_count = _uint(moov, cursor, 4, stts_end) + sample_delta = _uint(moov, cursor + 4, 4, stts_end) + if sample_count is None or sample_delta is None: + break frames += sample_count delta = delta or sample_delta cursor += 8 @@ -360,6 +403,17 @@ def _read_timing(moov: bytes) -> tuple[float | None, float | None, int | None]: return (None, None, None) +def _as_pixels(value: Any) -> int | None: + """A sensor dimension, or None if the file's bytes did not decode to one.""" + try: + if not math.isfinite(value): + return None + pixels = int(value) + except (TypeError, ValueError, OverflowError): + return None + return pixels if 0 < pixels <= 100_000 else None + + def read_info(path: Path) -> BrawInfo | None: """Parse a BRAW file's metadata. Returns None if it is not readable BRAW.""" path = Path(path) @@ -378,8 +432,12 @@ def read_info(path: Path) -> BrawInfo | None: info = BrawInfo(fps=fps, duration_sec=duration, frame_count=frames, raw=values) crop = values.get("crop_size") or values.get("sensor_area_captured") - if isinstance(crop, tuple): - info.width, info.height = int(crop[0]), int(crop[1]) + if isinstance(crop, tuple) and len(crop) >= 2: + # These arrive as a raw IEEE754 pair out of the file, so they can be + # NaN or infinity, neither of which survives int(). + width, height = _as_pixels(crop[0]), _as_pixels(crop[1]) + if width and height: + info.width, info.height = width, height info.camera_type = values.get("camera_type") info.camera_id = values.get("camera_id") diff --git a/src/offloader/config.py b/src/offloader/config.py index 8f292a6..90b9c7a 100644 --- a/src/offloader/config.py +++ b/src/offloader/config.py @@ -30,18 +30,30 @@ def config_dir() -> Path: def config_file(name: str) -> Path: - return config_dir() / name + """A path for `name` inside the config directory. + + `name` is coerced into the directory rather than joined onto it. pathlib's + `/` discards the left operand entirely when the right one is absolute, so + a plain join would put `config_file("D:/x.json")` at the root of D:. + """ + parts = [part for part in Path(name).parts + if part not in ("", ".", "..") and not Path(part).anchor] + if not parts: + raise ValueError(f"not a usable config file name: {name!r}") + return config_dir().joinpath(*parts) def read_json(path: Path, default: Any) -> Any: """Load JSON, falling back to `default` on anything unreadable. Config corruption must never stop someone offloading a card, so a bad file - is treated as an empty one. + is treated as an empty one. That includes a file that is not valid UTF-8: + `read_text` raises UnicodeDecodeError, which is a ValueError rather than an + OSError, and json.JSONDecodeError is a ValueError too. """ try: return json.loads(path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): + except (OSError, ValueError): return default diff --git a/src/offloader/engine.py b/src/offloader/engine.py index 168afc8..68e61a5 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -28,6 +28,7 @@ FileEntry, FileStatus, Job, + MediaInfo, Profile, VerificationMode, ) @@ -105,6 +106,45 @@ def assert_safe_destinations(source_root: Path, destinations: Sequence[Path]) -> seen[resolved] = Path(destination) +def assert_no_destination_collisions( + sources: Sequence[Path], source_root: Path, + destination_roots: Sequence[Path], preserve_structure: bool, +) -> None: + """Refuse a layout that maps two source files onto one destination path. + + Flattening a tree is a lossy operation whenever two cards, or two folders + on one card, hold a clip of the same name. Copying both leaves one file on + disk, and because each is verified as it lands, before the next one + overwrites it, both are reported VERIFIED. A report that attests to a file + which is not at the destination is worse than no report, so this is caught + up front, before a single byte moves. + + Paths are compared with `os.path.normcase`, so on Windows this also + catches two names differing only in case: distinct on the case-sensitive + volume they came from, one file on the volume they are going to. + """ + claimed: dict[str, Path] = {} + collisions: list[tuple[Path, Path, Path]] = [] + + for source in sources: + for root in destination_roots: + target = _destination_for(source, source_root, root, preserve_structure) + key = os.path.normcase(str(target)) + if key in claimed: + collisions.append((claimed[key], source, target)) + else: + claimed[key] = source + + if collisions: + first, second, target = collisions[0] + extra = (f" (and {len(collisions) - 1} more)" if len(collisions) > 1 else "") + raise UnsafeDestination( + f"{first} and {second} would both be copied to {target}{extra}; " + "one would silently overwrite the other. Keep the source folder " + "structure, or offload the colliding folders separately." + ) + + class JobControl: """Cooperative pause/resume/cancel for a running offload. @@ -209,12 +249,36 @@ def is_excluded(path: Path, patterns: Iterable[str]) -> bool: def scan(root: Path, excludes: Iterable[str] = DEFAULT_EXCLUDES) -> list[Path]: - """Every file under `root`, sorted, with junk filtered out.""" + """Every file under `root`, sorted, with junk filtered out. + + Directories are visited at most once each. A Windows junction pointing at + its own parent otherwise walks forever, and `Path.is_symlink()` is False + for a junction, so the usual symlink guard does not see it. Termination is + left to chance without this: today it only stops because Windows refuses + paths past MAX_PATH, and it stops having already returned the same file + dozens of times. + """ patterns = tuple(excludes) found: list[Path] = [] + visited: set[str] = set() + + def already_seen(directory: Path) -> bool: + try: + real = os.path.normcase(os.path.realpath(directory)) + except OSError: # pragma: no cover - unreadable entry + return True + if real in visited: + return True + visited.add(real) + return False + + already_seen(Path(root)) for dirpath, dirnames, filenames in os.walk(root): here = Path(dirpath) - dirnames[:] = sorted(d for d in dirnames if not is_excluded(here / d, patterns)) + dirnames[:] = sorted( + d for d in dirnames + if not is_excluded(here / d, patterns) and not already_seen(here / d) + ) for filename in sorted(filenames): candidate = here / filename if not is_excluded(candidate, patterns): @@ -423,6 +487,8 @@ def run(source_root: Path, options: OffloadOptions, assert_safe_destinations(source_root, dest_roots) files = scan(source_root, options.excludes) + assert_no_destination_collisions(files, source_root, dest_roots, + options.preserve_structure) counters = _Counters(job_bytes_total=sum(p.stat().st_size for p in files)) host = sysinfo.collect() @@ -671,15 +737,24 @@ def note_retry(attempt: int, exc: BaseException, pause: float, emit(ProgressEvent(index, len(files), source.name, "probe", 0, stat.st_size, counters.job_bytes_done, counters.job_bytes_total)) - entry.media = probe_mod.probe(source) - - if braw_mod.is_braw(source): - # A clip whose recording was interrupted has no moov atom. It - # copies and verifies perfectly and will not play, so the time - # to notice is now, while the card is still in hand. - check = braw_mod.check_container(source) - if check.is_fatal: - job.warnings.append(f"{source.name}: {check.detail}") + # Everything from here down is metadata: nice to have, and not + # worth a transfer. The bytes are already copied and verified by + # this point, so a clip whose container will not parse costs its + # own metadata and a line in the report, not the rest of the card. + try: + entry.media = probe_mod.probe(source) + + if braw_mod.is_braw(source): + # A clip whose recording was interrupted has no moov atom. + # It copies and verifies perfectly and will not play, so + # the time to notice is now, while the card is in hand. + check = braw_mod.check_container(source) + if check.is_fatal: + job.warnings.append(f"{source.name}: {check.detail}") + except Exception as exc: # noqa: BLE001 - see above + entry.media = MediaInfo() + job.warnings.append( + f"{source.name}: metadata could not be read ({exc})") if options.thumbnail_count > 0 and entry.media.is_video: emit(ProgressEvent(index, len(files), source.name, "thumbs", @@ -783,11 +858,18 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], ) if options.extra_probe: - entry.media = probe_mod.probe(source) - if options.thumbnail_count > 0 and entry.media.is_video: - entry.thumbnails = thumbs.extract( - source, entry.media, thumb_dir, options.thumbnail_count - ) + # Same reasoning as in run(): a rescan exists to regenerate + # paperwork, and one unparseable clip must not stop it. + try: + entry.media = probe_mod.probe(source) + if options.thumbnail_count > 0 and entry.media.is_video: + entry.thumbnails = thumbs.extract( + source, entry.media, thumb_dir, options.thumbnail_count + ) + except Exception as exc: # noqa: BLE001 + entry.media = MediaInfo() + job.warnings.append( + f"{source.name}: metadata could not be read ({exc})") job.files.append(entry) job.finished = _dt.datetime.now() diff --git a/src/offloader/hashers.py b/src/offloader/hashers.py index c9a5290..f9bcf57 100644 --- a/src/offloader/hashers.py +++ b/src/offloader/hashers.py @@ -28,10 +28,21 @@ class Algorithm: factory: Callable[[], Hasher] | None #: MHL/ASC-MHL element name, or None if the format has no slot for it. mhl_tag: str | None = None + #: Whether two digests differing only in case are different digests. True + #: for C4, whose base58 alphabet uses case to carry information. False for + #: every hex format, where another tool's uppercase output describes the + #: very same bytes. + case_sensitive: bool = False def new(self) -> Hasher | None: return self.factory() if self.factory else None + def digests_match(self, left: str, right: str) -> bool: + """Compare two digests of this algorithm.""" + if self.case_sensitive: + return left == right + return left.casefold() == right.casefold() + #: Base58 alphabet used by C4 (SMPTE ST 2114) — no 0, O, I or l. _C4_ALPHABET = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz" @@ -81,7 +92,7 @@ def hexdigest(self) -> str: # noqa: D102 "md5": Algorithm("md5", "MD5", hashlib.md5, "md5"), "sha1": Algorithm("sha1", "SHA-1", hashlib.sha1, "sha1"), "sha256": Algorithm("sha256", "SHA-256", hashlib.sha256, "sha256"), - "c4": Algorithm("c4", "C4", C4Hasher, "c4"), + "c4": Algorithm("c4", "C4", C4Hasher, "c4", case_sensitive=True), "none": Algorithm("none", "None", None, None), } @@ -108,6 +119,12 @@ def hash_file(path: Path, key: str, chunk_size: int = 8 << 20) -> str: re-check checksums without copying.""" from .longpath import open_binary + if chunk_size <= 0: + # read(0) returns b'' immediately, so the sentinel iteration below + # would stop before the first byte and hand back the digest of an + # empty file for a file that is not empty. + raise ValueError(f"chunk_size must be positive, got {chunk_size}") + hasher = new_hasher(key) with open_binary(path, "rb") as handle: for chunk in iter(lambda: handle.read(chunk_size), b""): diff --git a/src/offloader/history.py b/src/offloader/history.py index 696181b..342d56c 100644 --- a/src/offloader/history.py +++ b/src/offloader/history.py @@ -31,6 +31,11 @@ def fingerprint(paths: Sequence[Path], root: Path) -> str: """ digest = hashlib.sha1() root = Path(root) + # Seed with the source's own name. Without it the listing is the only + # input, so every empty card hashes to the SHA-1 of nothing and two + # unrelated blank volumes look like the same prior offload. + digest.update(root.name.encode("utf-8", "replace")) + digest.update(b"\n") entries = [] for path in paths: try: @@ -74,19 +79,49 @@ def describe(self) -> str: f"({self.status})") def to_dict(self) -> dict: - return self.__dict__.copy() + payload = self.__dict__.copy() + # __dict__ is a shallow copy, so without this the caller's dict shares + # the entry's own destinations list. + payload["destinations"] = list(self.destinations) + return payload @classmethod def from_dict(cls, data: dict) -> HistoryEntry: + """Build an entry from whatever is in the file. + + Every field is coerced rather than trusted. history.json is a + hand-editable file that a version skew or a half-finished write can + leave in any shape at all, and the module's whole premise is that a + damaged history never stops someone offloading a card. + """ + if not isinstance(data, dict): + data = {} + + def text(key: str) -> str: + value = data.get(key, "") + return value if isinstance(value, str) else str(value) + + def count(key: str) -> int: + try: + return int(data.get(key, 0)) + except (TypeError, ValueError): + return 0 + + # A JSON string here would otherwise be exploded character by + # character by list(), which is silent corruption rather than a crash. + destinations = data.get("destinations", []) + if isinstance(destinations, str) or not isinstance(destinations, Iterable): + destinations = [] + return cls( - fingerprint=data.get("fingerprint", ""), - job_name=data.get("job_name", ""), - source=data.get("source", ""), - destinations=list(data.get("destinations", [])), - file_count=int(data.get("file_count", 0)), - total_bytes=int(data.get("total_bytes", 0)), - status=data.get("status", ""), - finished=data.get("finished", ""), + fingerprint=text("fingerprint"), + job_name=text("job_name"), + source=text("source"), + destinations=[str(d) for d in destinations], + file_count=count("file_count"), + total_bytes=count("total_bytes"), + status=text("status"), + finished=text("finished"), ) @@ -95,9 +130,26 @@ class History: def __init__(self, path: Path | None = None) -> None: self.path = path or config_file(HISTORY_FILE) - self.entries: list[HistoryEntry] = [ - HistoryEntry.from_dict(item) for item in read_json(self.path, []) - ] + self.entries: list[HistoryEntry] = self._load() + + def _load(self) -> list[HistoryEntry]: + """Every entry the file can still yield, and nothing that raises. + + A damaged history is worth less than an offload, so a record that will + not parse is dropped rather than allowed to stop construction. The + top-level value has to be a list: a bare number is not iterable, and a + bare object iterates its keys as strings. + """ + payload = read_json(self.path, []) + if not isinstance(payload, list): + return [] + entries = [] + for item in payload: + try: + entries.append(HistoryEntry.from_dict(item)) + except Exception: # noqa: BLE001 - see the docstring + continue + return entries def save(self) -> None: write_json(self.path, [entry.to_dict() for entry in self.entries[:MAX_ENTRIES]]) diff --git a/src/offloader/naming.py b/src/offloader/naming.py index 0d670c9..b6c8ef7 100644 --- a/src/offloader/naming.py +++ b/src/offloader/naming.py @@ -29,6 +29,9 @@ _ILLEGAL = re.compile(r'[<>:"/\\|?*\x00-\x1f]') +#: One pass over the template, so a substituted value is never re-scanned. +_TOKEN = re.compile(r"\{(\w+)\}") + def sanitize(name: str) -> str: """Strip characters Windows and exFAT reject, and trailing dots/spaces.""" @@ -58,12 +61,17 @@ def context(source: Path, volume_label: str | None = None, def render(template: str, values: dict[str, str]) -> str: - """Substitute tokens. Unknown tokens are left alone rather than raising — - a typo in a preset should not stop an offload.""" - result = template or DEFAULT_TEMPLATE - for token, value in values.items(): - result = result.replace("{" + token + "}", value) - return sanitize(result) + """Substitute tokens. Unknown tokens are left alone rather than raising: + a typo in a preset should not stop an offload. + + Substitution is a single pass, so a value that itself contains token text + is never substituted into again. A card folder genuinely named `{index}` + keeps its name instead of being overwritten by the sequence number. + """ + def replace(match: re.Match[str]) -> str: + return values.get(match.group(1), match.group(0)) + + return sanitize(_TOKEN.sub(replace, template or DEFAULT_TEMPLATE)) def build(template: str, source: Path, *, volume_label: str | None = None, @@ -72,20 +80,28 @@ def build(template: str, source: Path, *, volume_label: str | None = None, Templates without `{index}` get a `-2`, `-3` suffix instead, so a name collision can never silently overwrite an earlier job's reports. + + Both searches are bounded by the size of `taken` rather than by a fixed + ceiling. The suffix search is the backstop, and it always succeeds: it + generates one more distinct candidate than there are taken names, so at + least one of them has to be free. """ used = {name.casefold() for name in taken} + if "{index}" in (template or ""): - for index in range(1, 1000): + for index in range(1, len(used) + 2): candidate = render(template, context(source, volume_label, when, index)) if candidate.casefold() not in used: return candidate - return render(template, context(source, volume_label, when, 999)) + # A template whose `{index}` does not survive sanitising renders the + # same name every time. Fall through to the suffix search rather than + # handing back a name we know is taken. base = render(template, context(source, volume_label, when)) if base.casefold() not in used: return base - for suffix in range(2, 1000): + for suffix in range(2, len(used) + 3): candidate = f"{base}-{suffix}" if candidate.casefold() not in used: return candidate - return base + raise AssertionError("unreachable: more candidates than taken names") diff --git a/src/offloader/probe.py b/src/offloader/probe.py index 9b4ab64..3c24a90 100644 --- a/src/offloader/probe.py +++ b/src/offloader/probe.py @@ -7,6 +7,7 @@ from __future__ import annotations import json +import math import shutil import subprocess from pathlib import Path @@ -64,22 +65,58 @@ def ffprobe_path() -> str | None: def _parse_rate(value: str | None) -> float | None: - """ffprobe rates arrive as "24/1" or "24000/1001".""" + """ffprobe rates arrive as "24/1" or "24000/1001". + + A frame rate that is not a finite positive number is not a frame rate. + `float()` accepts "inf" and "nan" without complaint, and everything + downstream calls `round()` on the result, which does not. + """ if not value or value in ("0/0", "0"): return None if "/" in value: num, _, den = value.partition("/") try: den_f = float(den) - return float(num) / den_f if den_f else None - except ValueError: + rate = float(num) / den_f if den_f else None + except (TypeError, ValueError): + return None + else: + try: + rate = float(value) + except (TypeError, ValueError): return None + if rate is None or not math.isfinite(rate) or rate <= 0: + return None + return rate + + +def _as_int(value: object) -> int | None: + """ffprobe writes the literal string "N/A" in numeric fields routinely: + for codecs that declare no sample rate, for data streams, and for anything + it only partly decoded. That is ordinary output, not corruption.""" try: - return float(value) - except ValueError: + return int(value) # type: ignore[arg-type] + except (TypeError, ValueError): return None +def _as_float(value: object) -> float | None: + try: + parsed = float(value) # type: ignore[arg-type] + except (TypeError, ValueError): + return None + return parsed if math.isfinite(parsed) else None + + +def _tags(obj: object) -> dict: + """A stream's or format's `tags`, defended against it not being a map.""" + if isinstance(obj, dict): + tags = obj.get("tags") + if isinstance(tags, dict): + return tags + return {} + + def _from_braw(path: Path) -> MediaInfo: """BRAW metadata, read straight out of the container. @@ -126,9 +163,20 @@ def _from_braw(path: Path) -> MediaInfo: def probe(path: Path, timeout: float = 30.0) -> MediaInfo: - """Read metadata for one file. Returns an empty MediaInfo for non-media - files, unreadable files, or when ffprobe is missing — a failed probe must - never fail the offload.""" + """Read metadata for one file. + + Returns an empty MediaInfo for non-media files, unreadable files, and when + ffprobe is missing. A failed probe must never fail the offload, so this + catches everything rather than only the failures we predicted: metadata is + a convenience, and no clip is worth abandoning a transfer over. + """ + try: + return _probe(path, timeout) + except Exception: # noqa: BLE001 - see the docstring + return MediaInfo() + + +def _probe(path: Path, timeout: float) -> MediaInfo: path = Path(path) if path.suffix.lower() == ".braw": return _from_braw(path) @@ -153,8 +201,11 @@ def probe(path: Path, timeout: float = 30.0) -> MediaInfo: def _build(data: dict) -> MediaInfo: - fmt = data.get("format", {}) - streams = data.get("streams", []) + fmt = data.get("format") + fmt = fmt if isinstance(fmt, dict) else {} + # ffprobe is the only thing that should be writing this document, but it is + # still parsed JSON from a subprocess: entries need not be maps. + streams = [s for s in data.get("streams", []) if isinstance(s, dict)] video = next((s for s in streams if s.get("codec_type") == "video"), None) audios = [s for s in streams if s.get("codec_type") == "audio"] @@ -163,10 +214,7 @@ def _build(data: dict) -> MediaInfo: info.container = _CONTAINERS.get(format_name, format_name.upper() or None) duration = fmt.get("duration") or (video or {}).get("duration") - try: - info.duration_sec = float(duration) if duration else None - except (TypeError, ValueError): - info.duration_sec = None + info.duration_sec = _as_float(duration) if duration else None if video is not None: info.width = video.get("width") @@ -186,11 +234,11 @@ def _build(data: dict) -> MediaInfo: # Timecode lives on the format, the video stream, or a dedicated # data stream depending on the camera. tc = ( - fmt.get("tags", {}).get("timecode") - or video.get("tags", {}).get("timecode") + _tags(fmt).get("timecode") + or _tags(video).get("timecode") or next( - (s.get("tags", {}).get("timecode") for s in streams - if s.get("tags", {}).get("timecode")), + (_tags(s).get("timecode") for s in streams + if _tags(s).get("timecode")), None, ) ) @@ -201,15 +249,14 @@ def _build(data: dict) -> MediaInfo: for stream in audios: codec = stream.get("codec_name", "") - bit_rate = stream.get("bit_rate") - sample_rate = stream.get("sample_rate") + bit_rate = _as_float(stream.get("bit_rate")) info.audio_tracks.append( AudioTrack( - channels=int(stream.get("channels") or 0), + channels=_as_int(stream.get("channels")) or 0, layout=stream.get("channel_layout") or "", codec=_AUDIO_CODECS.get(codec, codec.upper() or "Unknown"), - bit_rate_kbps=float(bit_rate) / 1000.0 if bit_rate else None, - sample_rate_hz=int(sample_rate) if sample_rate else None, + bit_rate_kbps=bit_rate / 1000.0 if bit_rate else None, + sample_rate_hz=_as_int(stream.get("sample_rate")), ) ) return info diff --git a/src/offloader/reports/csv_report.py b/src/offloader/reports/csv_report.py index 3ebb994..8fbd7b8 100644 --- a/src/offloader/reports/csv_report.py +++ b/src/offloader/reports/csv_report.py @@ -6,7 +6,24 @@ from pathlib import Path from ..models import Job -from ..util import format_file_datetime, format_size +from ..util import format_file_datetime, format_size, xml_safe + + +class _SafeWriter: + """A csv.writer that cannot be killed by a filename. + + The underlying stream is UTF-8 with the default strict error handling, so + a name carrying a lone surrogate raises UnicodeEncodeError mid-row and + takes the whole report with it. Filtering here covers every column at + once, and matches what the MHL writer has always done. + """ + + def __init__(self, writer) -> None: + self._writer = writer + + def writerow(self, values) -> None: + self._writer.writerow( + [xml_safe(v) if isinstance(v, str) else v for v in values]) COLUMNS = [ "File Name", @@ -45,7 +62,7 @@ def write_csv(job: Job, path: Path, *, delimiter: str = ",", **_options) -> Path path.parent.mkdir(parents=True, exist_ok=True) with open(path, "w", newline="", encoding="utf-8") as handle: - writer = csv.writer(handle, delimiter=delimiter) + writer = _SafeWriter(csv.writer(handle, delimiter=delimiter)) writer.writerow([f"# {job.name}"]) writer.writerow([ f"# Source: {job.source_root}", diff --git a/src/offloader/reports/mhl.py b/src/offloader/reports/mhl.py index 434a3ee..078f746 100644 --- a/src/offloader/reports/mhl.py +++ b/src/offloader/reports/mhl.py @@ -16,34 +16,18 @@ from .. import PRODUCT_NAME, __version__ from ..models import Job +from ..util import xml_safe _MHL_VERSION = "1.1" -#: XML 1.0 forbids most C0 control characters outright — they cannot even be +#: XML 1.0 forbids most C0 control characters outright: they cannot even be #: written as character references. ElementTree emits them raw, producing a #: document no parser will read. One stray control byte in one filename would -#: therefore strand the verification of the *entire* delivery, so it is -#: replaced with U+FFFD rather than passed through. -#: U+FFFD REPLACEMENT CHARACTER. -_REPLACEMENT = chr(0xFFFD) - - -def _is_xml_char(code: int) -> bool: - """The XML 1.0 Char production.""" - return ( - code in (0x09, 0x0A, 0x0D) - or 0x20 <= code <= 0xD7FF - or 0xE000 <= code <= 0xFFFD - or 0x10000 <= code <= 0x10FFFF - ) - - -def _xml_safe(text: object) -> str: - """Text ElementTree can serialise and a parser can read back.""" - return "".join( - character if _is_xml_char(ord(character)) else _REPLACEMENT - for character in str(text) - ) +#: therefore strand the verification of the *entire* delivery. +#: +#: The filter lives in util now, so the ASC MHL writer and the CSV writer get +#: the same guarantee instead of each having to rediscover it. +_xml_safe = xml_safe def _iso(value: float | _dt.datetime) -> str: diff --git a/src/offloader/retry.py b/src/offloader/retry.py index 97224fe..cb88a37 100644 --- a/src/offloader/retry.py +++ b/src/offloader/retry.py @@ -58,14 +58,29 @@ } +#: A ceiling for a backoff that overflowed. Five minutes is already far past +#: the point where a card is worth waiting on. +_MAX_WAIT = 300.0 + + @dataclass(frozen=True) class RetryPolicy: - """How hard to try again. `attempts` counts the first try.""" + """How hard to try again. `attempts` counts the first try. + + Values are clamped rather than rejected. These come out of presets.json, + which is a hand-editable file: a nonsensical delay should not raise out of + the middle of a transfer, and `time.sleep` raises on a negative argument. + """ attempts: int = 3 delay: float = 2.0 backoff: float = 1.5 + def __post_init__(self) -> None: + object.__setattr__(self, "attempts", max(1, int(self.attempts))) + object.__setattr__(self, "delay", max(0.0, float(self.delay))) + object.__setattr__(self, "backoff", max(0.0, float(self.backoff))) + @property def enabled(self) -> bool: return self.attempts > 1 @@ -74,7 +89,11 @@ def wait_before(self, attempt: int) -> float: """Seconds to wait before `attempt` (1-based; attempt 1 never waits).""" if attempt <= 1: return 0.0 - return self.delay * (self.backoff ** (attempt - 2)) + try: + return max(0.0, self.delay * (self.backoff ** (attempt - 2))) + except OverflowError: + return _MAX_WAIT + NO_RETRY = RetryPolicy(attempts=1) diff --git a/src/offloader/util.py b/src/offloader/util.py index 6f55581..d0467b2 100644 --- a/src/offloader/util.py +++ b/src/offloader/util.py @@ -8,6 +8,28 @@ from __future__ import annotations import datetime as _dt +import math + +#: (unit, bytes per unit, decimals). The decimal counts are the reference +#: report's, not ours: 1 for KB/MB, 2 for GB/TB. +_SIZE_UNITS: tuple[tuple[str, int, int], ...] = ( + ("bytes", 1, 0), + ("KB", 10 ** 3, 1), + ("MB", 10 ** 6, 1), + ("GB", 10 ** 9, 2), + ("TB", 10 ** 12, 2), +) + + +def _is_finite(value: object) -> bool: + """`math.isfinite` without the surprises: an int too large to convert to + float is still a finite number, and a non-number is not.""" + if isinstance(value, int) and not isinstance(value, bool): + return True + try: + return math.isfinite(value) # type: ignore[arg-type] + except (TypeError, OverflowError): + return False def format_size(num_bytes: int) -> str: @@ -16,23 +38,42 @@ def format_size(num_bytes: int) -> str: The reference uses 1 decimal for MB values ("258.8 MB", "8.6 MB") and 2 decimals for GB values ("27.8 GB", "483.11 GB"). Reproduce that. + The unit is chosen against the *rounded* mantissa rather than the raw one, + so a count just below a decade boundary promotes instead of printing a + mantissa of 1000 in the smaller unit. Sign is handled separately, so a + negative count promotes the same way a positive one does. + >>> format_size(258758961) '258.8 MB' >>> format_size(512) '512 bytes' + >>> format_size(999_999) + '1.0 MB' + >>> format_size(-1024) + '-1.0 KB' """ - if num_bytes < 1000: - return f"{num_bytes} bytes" - kb = num_bytes / 1000.0 - if kb < 1000: - return f"{kb:.1f} KB" - mb = kb / 1000.0 - if mb < 1000: - return f"{mb:.1f} MB" - gb = mb / 1000.0 - if gb < 1000: - return f"{gb:.2f} GB" - return f"{gb / 1000.0:.2f} TB" + if not _is_finite(num_bytes): + # A size we failed to measure is not a size. "nan TB" in a report is + # worse than admitting to nothing. + return "0 bytes" + + sign = "-" if num_bytes < 0 else "" + magnitude = abs(num_bytes) + + for unit, scale, decimals in _SIZE_UNITS: + # The mantissa rounds up to 1000 just below the boundary, so promote a + # half-step early. All integer arithmetic, so an enormous count picks + # its unit without ever overflowing a float. + promote_at = 1000 * scale - scale // (2 * 10 ** decimals) + if magnitude >= promote_at and unit != _SIZE_UNITS[-1][0]: + continue + if decimals == 0: + return f"{sign}{magnitude} {unit}" + try: + return f"{sign}{magnitude / scale:.{decimals}f} {unit}" + except OverflowError: # pragma: no cover - absurd counts + return f"{sign}{magnitude // scale} {unit}" + raise AssertionError("unreachable: the last unit always matches") def format_duration(seconds: float) -> str: @@ -61,10 +102,22 @@ def format_job_datetime(ts: float | _dt.datetime) -> str: return f"{dt:%B} {dt.day:02d}, {dt.year}-{dt:%H_%M_%S}" +def whole_frame_rate(fps: float) -> int: + """Frames per second as a positive whole number, for timecode arithmetic. + + Anything unusable (nan, inf, zero, negative) becomes 1. A probe that could + not read the rate must not be able to crash a report, and a negative rate + must not flip the sign of the timecode fields. + """ + if not _is_finite(fps) or fps <= 0: + return 1 + return max(1, int(round(fps))) + + def format_timecode(frames: int, fps: float, drop_frame: bool = False) -> str: """Frame count to "HH:MM:SS:FF NDF"/"DF" timecode.""" - rate = int(round(fps)) or 1 - hours, rem = divmod(frames, rate * 3600) + rate = whole_frame_rate(fps) + hours, rem = divmod(int(frames), rate * 3600) minutes, rem = divmod(rem, rate * 60) secs, fr = divmod(rem, rate) tag = "DF" if drop_frame else "NDF" @@ -72,12 +125,51 @@ def format_timecode(frames: int, fps: float, drop_frame: bool = False) -> str: def format_fps(fps: float) -> str: - """"24 FPS" for integral rates, "23.98 FPS" otherwise.""" + """"24 FPS" for integral rates, "23.98 FPS" otherwise. + + A rate that is not a finite number renders as nothing at all, which the + report writers already treat as "omit this field". + """ + if not _is_finite(fps): + return "" if abs(fps - round(fps)) < 0.001: return f"{int(round(fps))} FPS" return f"{fps:.2f} FPS" +#: U+FFFD REPLACEMENT CHARACTER. +_REPLACEMENT = chr(0xFFFD) + + +def _is_xml_char(code: int) -> bool: + """The XML 1.0 Char production.""" + return ( + code in (0x09, 0x0A, 0x0D) + or 0x20 <= code <= 0xD7FF + or 0xE000 <= code <= 0xFFFD + or 0x10000 <= code <= 0x10FFFF + ) + + +def xml_safe(text: object) -> str: + """Text that serialises and reads back, for manifests and reports alike. + + Filenames come off a filesystem, not out of a validator. A POSIX name that + is not valid UTF-8 arrives as lone surrogates via surrogateescape, and NTFS + accepts unpaired surrogates outright; neither can be encoded as UTF-8 at + all. Control characters encode fine but fall outside the XML 1.0 Char + production, so a manifest containing one will not parse again. + + One bad filename must not strand a whole delivery's paperwork, so the + offending characters are replaced with U+FFFD rather than passed through + or allowed to raise. + """ + return "".join( + character if _is_xml_char(ord(character)) else _REPLACEMENT + for character in str(text) + ) + + def channel_layout_name(channels: int, layout: str | None = None) -> str: """Human name for an audio track: "Stereo", "Mono", "5.1".""" if layout and layout not in ("unknown",): diff --git a/src/offloader/verify.py b/src/offloader/verify.py index 3975139..c9cf1f8 100644 --- a/src/offloader/verify.py +++ b/src/offloader/verify.py @@ -21,7 +21,7 @@ from .ascmhl import ASCMHL_DIRNAME from .ascmhl import NAMESPACE as ASCMHL_NAMESPACE -from .hashers import ALGORITHMS, hash_file +from .hashers import ALGORITHMS, get_algorithm, hash_file from .integrity import evict_from_cache @@ -198,7 +198,10 @@ def verify_manifest( detail=str(exc))) continue - matched = actual == expected + # Case-folded for hex, exact for C4. An MHL is an interchange format, + # and plenty of tools write their hex uppercase: comparing those with + # `==` calls every byte-identical file a mismatch. + matched = get_algorithm(algorithm_key).digests_match(actual, expected) report.verdicts.append(FileVerdict( path, EntryResult.OK if matched else EntryResult.MISMATCH, diff --git a/tests/test_edge_cases.py b/tests/test_edge_cases.py index cbfedd6..de01cad 100644 --- a/tests/test_edge_cases.py +++ b/tests/test_edge_cases.py @@ -4,19 +4,18 @@ module goes after everything downstream of them: what ffprobe hands back, what a number formats to, what a digest compares equal to, what a name renders to. -The failures here share a shape. Each is a value that is perfectly legal at the -point it is produced and fatal (or silently wrong) at the point it is consumed, -because the two ends disagree about the contract: +The bugs these were written against shared a shape. Each was a value that was +perfectly legal at the point it was produced and fatal, or silently wrong, at +the point it was consumed, because the two ends disagreed about the contract: - * ffprobe writes "N/A" in a numeric field. `_build` calls `int()` on it. + * ffprobe writes "N/A" in a numeric field. `_build` called `int()` on it. * `_parse_rate` accepts "inf/1" because `float()` does. Every fps formatter - then calls `round()` on it, which does not. - * Another tool writes its hex digests uppercase. `verify` compares with `==`. - * `render` substitutes token values into a string it then keeps substituting - into. + then called `round()` on it, which does not. + * Another tool writes its hex digests uppercase. `verify` compared with `==`. + * `render` substituted token values into a string it kept substituting into. -Tests marked `xfail(strict=True)` are reproduced, not hypothesised. Fixing one -turns it XPASS and pytest will ask for the marker back. +All four are fixed, and both ends of each pair are pinned here: whichever side +a later change moves, the other still has to hold. Run longer sweeps with: pytest --fuzz tests/test_edge_cases.py """ @@ -76,13 +75,6 @@ def test_probe_build_handles_healthy_output(): @pytest.mark.parametrize("stream", NA_STREAMS) -@pytest.mark.xfail(strict=True, reason=( - "probe._build does bare int()/float() on ffprobe's audio fields " - "(probe.py:208, 211, 212) with no try/except, unlike the duration field " - "at probe.py:166-169 which is guarded. _build is called at probe.py:152, " - "*outside* the try that catches SubprocessError/JSONDecodeError, so the " - "ValueError escapes probe() and then engine.run(), which does not guard " - "the probe call either")) def test_probe_build_survives_na_fields(stream): """This is the realistic version of 'one bad file kills the job'. It needs no crafted BRAW, only a clip whose audio stream ffprobe cannot fully @@ -90,15 +82,10 @@ def test_probe_build_survives_na_fields(stream): probe._build({"streams": [stream], "format": {}}) -@pytest.mark.xfail(strict=True, reason=( - "probe._build assumes every entry of `streams` is a dict (probe.py:158)")) def test_probe_build_survives_a_non_dict_stream(): probe._build({"streams": ["garbage"], "format": {}}) -@pytest.mark.xfail(strict=True, reason=( - "same unguarded _build as above, reached the way it is reached in " - "production: through probe.probe() with ffprobe returning real output")) def test_probe_survives_an_ffprobe_that_reports_na(tmp_path, monkeypatch): """End to end through the public entry point, with ffprobe stubbed to return output it genuinely produces.""" @@ -119,10 +106,6 @@ def test_probe_survives_an_ffprobe_that_reports_na(tmp_path, monkeypatch): @pytest.mark.parametrize("rate", ["inf/1", "-inf/1", "nan/1", "Infinity/1"]) -@pytest.mark.xfail(strict=True, reason=( - "probe._parse_rate (probe.py:66-79) guards division by zero but not the " - "conversion itself: float('inf') and float('nan') do not raise, so a " - "non-finite rate flows into MediaInfo.fps and reaches every formatter")) def test_parse_rate_never_returns_a_non_finite_number(rate): """A frame rate is a physical quantity. The parser is the right place to refuse a value that is not one, because nothing downstream checks.""" @@ -131,11 +114,6 @@ def test_parse_rate_never_returns_a_non_finite_number(rate): @pytest.mark.parametrize("fps", [float("inf"), float("nan")]) -@pytest.mark.xfail(strict=True, reason=( - "util.format_fps and util.format_timecode both call round()/int() on fps " - "before any guard can help: round(nan) raises ValueError and round(inf) " - "raises OverflowError. The `or 1` fallback in format_timecode's " - "`int(round(fps)) or 1` never runs, because round() raises first")) def test_fps_formatters_accept_whatever_parse_rate_produces(fps): """The two ends of the same contract. `_parse_rate` can produce these, so the formatters have to survive them, or one of the two has to change.""" @@ -151,10 +129,6 @@ def test_fps_formatters_accept_whatever_parse_rate_produces(fps): @given(size=st.integers(min_value=-10 ** 15, max_value=-1)) @settings(deadline=None) -@pytest.mark.xfail(strict=True, reason=( - "util.format_size's first branch is `num_bytes < 1000`, which is true for " - "every negative number regardless of magnitude, so a negative size is " - "printed verbatim in bytes and never promotes to KB/MB/GB/TB")) def test_format_size_promotes_negative_values_too(size: int): """`test_fuzz.py` pins this property for non-negative sizes only. A byte count that can go negative anywhere (history.py uses -1 as its stat-failure @@ -169,10 +143,6 @@ def test_format_size_promotes_negative_values_too(size: int): @pytest.mark.parametrize("size, unit", [ (999_999, "KB"), (999_999_999, "MB"), (999_999_999_999, "GB"), ]) -@pytest.mark.xfail(strict=True, reason=( - "util.format_size compares against 1000 before rounding to one or two " - "decimals, so a value just under a decade boundary rounds up to a " - "mantissa of 1000.0 in the smaller unit instead of promoting")) def test_format_size_mantissa_never_reaches_1000(size: int, unit: str): """'1000.0 MB' is not wrong by much, but it is a unit the function's own branch structure says it will never print.""" @@ -184,10 +154,6 @@ def test_format_size_mantissa_never_reaches_1000(size: int, unit: str): @pytest.mark.parametrize("value", [float("nan"), float("inf")]) -@pytest.mark.xfail(strict=True, reason=( - "every `< 1000` comparison is False for nan and inf, so format_size falls " - "through all of its branches and returns the last one, producing " - "'nan TB' / 'inf TB'")) def test_format_size_does_not_invent_a_unit_for_non_finite_input(value): text = util.format_size(value) assert SIZE_SHAPE.fullmatch(text), text @@ -227,11 +193,6 @@ def test_verify_passes_a_manifest_we_wrote_ourselves(tmp_path): assert verify.verify_manifest(manifest).counts() == {"ok": 1} -@pytest.mark.xfail(strict=True, reason=( - "verify.py:201 compares digests with a plain case-sensitive `==`, so a " - "manifest written by a tool that emits uppercase hex reports every file " - "as MISMATCH. Note the fix is per-algorithm, not a blanket .lower(): C4 " - "digests are base58 and genuinely case-sensitive (hashers.py:53-59)")) def test_verify_accepts_an_uppercase_hex_manifest(tmp_path): """MHL is an interchange format. Telling an editor their footage is corrupt because another tool shifted the case of a hex digit is the most @@ -250,10 +211,6 @@ def test_verify_accepts_an_uppercase_hex_manifest(tmp_path): # ---------------------------------------------------------- name rendering -@pytest.mark.xfail(strict=True, reason=( - "naming.render applies str.replace once per token in sequence, over the " - "string it is accumulating, so a token value that itself contains token " - "text is substituted again by a later pass")) def test_render_does_not_substitute_into_a_value_it_already_placed(): """A folder literally named `{index}` is a legal directory name. Rendering `{card}` from it puts the text `{index}` into the result, and the later @@ -266,10 +223,6 @@ def test_render_does_not_substitute_into_a_value_it_already_placed(): # -------------------------------------------------------------- fingerprint -@pytest.mark.xfail(strict=True, reason=( - "history.fingerprint hashes the joined per-file lines and nothing else, " - "so an empty file list hashes the empty string: da39a3ee..., the SHA-1 of " - "nothing, for every source root there has ever been")) def test_empty_sources_do_not_share_one_fingerprint(): """The fingerprint decides whether a card has been offloaded before. Two different empty volumes must not look like the same prior job.""" @@ -281,10 +234,6 @@ def test_empty_sources_do_not_share_one_fingerprint(): @pytest.mark.parametrize("name", ["D:/evil.json", "/etc/passwd"]) -@pytest.mark.xfail(strict=True, reason=( - "config.config_file joins with pathlib's `/`, which discards the left " - "operand entirely when the right operand is absolute, so an absolute " - "`name` silently resolves outside the config directory")) def test_config_file_stays_inside_the_config_directory(name: str): """Only ever called with literals today, which is the single reason this is latent rather than live.""" diff --git a/tests/test_fuzz_edges.py b/tests/test_fuzz_edges.py index d6f5aff..e882421 100644 --- a/tests/test_fuzz_edges.py +++ b/tests/test_fuzz_edges.py @@ -16,9 +16,9 @@ 3. A parser fed hostile bytes may return nothing, but it may not raise something the caller has no reason to catch. -Tests marked `xfail(strict=True)` are confirmed live bugs, not aspirations. -Each names the offending line. If you fix one, the marker turns the test XPASS -and pytest will tell you to delete the marker. +Every test here began as a reproduced bug. They are regressions now, and the +comments name the mechanism rather than the fix, because the mechanism is what +a future change is liable to reintroduce. Run longer sweeps with: pytest --fuzz tests/test_fuzz_edges.py """ @@ -69,8 +69,15 @@ def atom(tag: bytes, payload: bytes = b"", *, declared: int | None = None) -> bytes: - """One ISO-BMFF atom. `declared` lies about the size on purpose.""" + """One ISO-BMFF atom. `declared` lies about the size on purpose. + + A size past 2**32 does not fit the 32-bit field, so it goes in the 64-bit + extended form the format defines for exactly that: a size of 1, then the + real size as a big-endian u64. That is the path a hostile file would use. + """ size = len(payload) + 8 if declared is None else declared + if size > 0xFFFFFFFF: + return struct.pack(">I", 1) + tag + struct.pack(">Q", size) + payload return struct.pack(">I", size) + tag + payload @@ -156,11 +163,6 @@ def test_braw_read_info_survives_random_atom_trees(trees, tmp_path_factory): pytest.fail(f"read_info raised {type(exc).__name__}: {exc}") -@pytest.mark.xfail(strict=True, reason=( - "braw.py:333/335/338/344/349 index and unpack at offsets derived from an " - "atom's *declared* size without checking the buffer actually extends that " - "far, so a short mdhd or a lying stts entry_count raises struct.error / " - "IndexError straight out of read_info")) @settings(deadline=None, suppress_health_check=[HealthCheck.too_slow]) @given(mdhd=st.binary(max_size=40), stts=st.binary(max_size=40), trailing=st.lists(atom_trees(2), max_size=2)) @@ -190,18 +192,12 @@ def test_braw_read_info_survives_truncated_video_traks(mdhd, stts, trailing, ("stts entry_count promises entries that are not there", GOOD_MDHD, b"\x00" * 4 + struct.pack(">I", 4096)), ]) -@pytest.mark.xfail(strict=True, reason=( - "braw._read_timing trusts the declared atom size over the real buffer " - "length; see braw.py:333-350")) def test_braw_truncated_atoms_do_not_raise(label, mdhd, stts, tmp_path): """Minimised regressions for the structured sweep above.""" target = write_braw(tmp_path, braw_file(video_trak(mdhd, stts))) braw.read_info(target) -@pytest.mark.xfail(strict=True, reason=( - "braw._descend (braw.py:225) recurses once per nested container with no " - "depth limit, so ~1000 nested atoms exhaust the interpreter stack")) def test_braw_deep_nesting_does_not_exhaust_the_stack(tmp_path): """8 bytes per level, so a 16 KB file is enough to blow the stack.""" nest = atom(b"stbl") @@ -239,11 +235,6 @@ def tell(self) -> int: return self._pos -@pytest.mark.xfail(strict=True, reason=( - "braw._find_moov (braw.py:198) does `handle.read(size - header_length)` " - "with a size taken straight from the file (up to 2**64-1 via the " - "extended-size path), without the `offset + size > file_size` check that " - "check_container (braw.py:177) does perform")) @given(claimed=st.integers(min_value=2 ** 32, max_value=2 ** 48)) @settings(deadline=None) def test_find_moov_never_reads_past_the_end_of_the_file(claimed: int): @@ -269,11 +260,6 @@ def _offload(source: Path, dest: Path, **overrides) -> Job: return engine.run(source, OffloadOptions(**options)) -@pytest.mark.xfail(strict=True, reason=( - "engine.run calls probe_mod.probe(source) with no try/except, and " - "probe.probe's BRAW branch (probe.py:133-134) has none either, so any " - "braw.py crash aborts the whole job: the clips after the bad one are " - "never copied and no report is written")) def test_one_malformed_clip_does_not_abort_the_offload(tmp_path): """The scenario this tool exists for: a card with one bad clip. The bad clip may fail. The other two must still land, and the job must still @@ -311,12 +297,6 @@ def test_destination_paths_are_injective_when_structure_is_preserved(tmp_path): assert landed == [1000, 2000] -@pytest.mark.xfail(strict=True, reason=( - "engine._destination_for (engine.py:232) maps every source onto " - "`dest_root / source.name` when preserve_structure is False, and nothing " - "checks two sources for the same target. engine.py:85's `seen` dict " - "dedupes destination *roots* only. Reachable as `offloader --flat` and as " - "the GUI's 'Recreate the source folder structure' checkbox")) @settings(max_examples=30, deadline=None, suppress_health_check=[HealthCheck.function_scoped_fixture, HealthCheck.too_slow]) @@ -324,16 +304,24 @@ def test_destination_paths_are_injective_when_structure_is_preserved(tmp_path): min_size=2, max_size=5, unique=True), basename=st.sampled_from(["clip.mov", "A001_C001.mov", "take.braw"])) def test_flat_mode_never_silently_drops_a_file(folders, basename, tmp_path_factory): - """Every byte that leaves the card must arrive, or the job must say it - didn't. Reporting VERIFIED for a file that is not at the destination is the - one outcome a verified-copy tool may never produce.""" + """Every byte that leaves the card must arrive, or the job must say so. + + Reporting VERIFIED for a file that is not at the destination is the one + outcome a verified-copy tool may never produce. Refusing the layout counts + as saying so: the collision is detected before any byte moves, so the card + is untouched and the operator gets to choose a layout that fits. + """ root = tmp_path_factory.mktemp("flat") source, dest = root / "card", root / "dst" for index, folder in enumerate(folders): (source / folder).mkdir(parents=True) (source / folder / basename).write_bytes(bytes([65 + index]) * (1000 + index)) - job = _offload(source, dest, preserve_structure=False) + try: + job = _offload(source, dest, preserve_structure=False) + except engine.UnsafeDestination: + assert not list(dest.rglob("*")), "refused the job but still wrote" + return source_bytes = sum(p.stat().st_size for p in source.rglob("*") if p.is_file()) dest_bytes = sum(p.stat().st_size for p in dest.rglob("*") if p.is_file()) @@ -347,34 +335,42 @@ def test_flat_mode_never_silently_drops_a_file(folders, basename, tmp_path_facto f"{len(job.files)} destinations still report VERIFIED") -@pytest.mark.xfail(strict=True, reason="same collision as the sweep above") -def test_flat_mode_collision_is_reported_not_silent(tmp_path): - """Minimised regression: two takes, one surviving file, a clean report.""" +def test_flat_mode_collision_is_refused_before_anything_moves(tmp_path): + """Minimised regression: two takes that would become one file. + + The check runs on the scan, so the error names both sources and nothing + has been written by the time it is raised. + """ source, dest = tmp_path / "card", tmp_path / "dst" (source / "A").mkdir(parents=True) (source / "B").mkdir(parents=True) (source / "A" / "clip.mov").write_bytes(b"A" * 1000) (source / "B" / "clip.mov").write_bytes(b"B" * 2000) - job = _offload(source, dest, preserve_structure=False) + with pytest.raises(engine.UnsafeDestination, match="would both be copied"): + _offload(source, dest, preserve_structure=False) + + assert not dest.exists() or not list(dest.rglob("*")) + + +def test_flat_mode_still_works_when_names_do_not_collide(tmp_path): + """The guard must not cost the ordinary flattening case.""" + source, dest = tmp_path / "card", tmp_path / "dst" + (source / "A").mkdir(parents=True) + (source / "B").mkdir(parents=True) + (source / "A" / "one.mov").write_bytes(b"A" * 1000) + (source / "B" / "two.mov").write_bytes(b"B" * 2000) + + _offload(source, dest, preserve_structure=False) - landed = [p for p in dest.rglob("*") if p.is_file()] - failures = [d for entry in job.files for d in entry.destinations - if d.status is not FileStatus.VERIFIED] - assert len(landed) == 2 or failures, ( - f"{len(job.files)} files became {len(landed)} on disk, " - f"and not one destination reported a problem") + assert sorted(p.name for p in dest.rglob("*") if p.is_file()) == \ + ["one.mov", "two.mov"] # ------------------------------------------------------------ scanning cycles @pytest.mark.skipif(not WINDOWS, reason="directory junctions are Windows-only") -@pytest.mark.xfail(strict=True, reason=( - "engine.scan (engine.py:215) is a bare os.walk with no cycle guard, and " - "Path.is_symlink() is False for a junction, so the usual symlink check " - "would not help either. It terminates only because Windows refuses paths " - "past MAX_PATH; with long paths enabled it would not")) def test_scan_does_not_follow_a_directory_junction_into_a_cycle(tmp_path): card = tmp_path / "card" (card / "sub").mkdir(parents=True) @@ -432,11 +428,6 @@ def test_write_mhl_is_total(name, tmp_path): @pytest.mark.parametrize("name", UNENCODABLE_NAMES) -@pytest.mark.xfail(strict=True, reason=( - "csv_report.py:47 opens with the default errors='strict' and does no " - "filtering, so writer.writerow (csv_report.py:96) raises " - "UnicodeEncodeError. write_mhl survives the same name because it has " - "_xml_safe; write_csv has no equivalent")) def test_write_csv_is_total(name, tmp_path): """A report writer runs *after* the bytes are safely copied. Crashing there turns a finished offload into one with no paperwork.""" @@ -455,13 +446,6 @@ def test_ascmhl_round_trips_ordinary_names(name, tmp_path): @pytest.mark.parametrize("name", ILLEGAL_XML_NAMES + UNENCODABLE_NAMES) -@pytest.mark.xfail(strict=True, reason=( - "ascmhl.py has no _xml_safe equivalent: the text at ascmhl.py:355 " - "is written raw. Control characters produce a manifest that will not " - "reparse, and read_manifest_hashes swallows the ParseError " - "(ascmhl.py:176) and returns {}, so the file vanishes from the chain of " - "custody silently. A lone surrogate fails earlier, in the UTF-8 encode at " - "ascmhl.py:212")) def test_ascmhl_round_trips_hostile_names(name, tmp_path): """Whatever the name, `write_manifest` must either refuse it outright or produce a manifest `read_manifest_hashes` can fully recover. Writing a @@ -475,11 +459,6 @@ def test_ascmhl_round_trips_hostile_names(name, tmp_path): # -------------------------------------------------------------- retry policy -@pytest.mark.xfail(strict=True, reason=( - "RetryPolicy is a frozen dataclass with no __post_init__ validation, and " - "presets.py builds one straight from presets.json, so a negative delay " - "reaches retry.py:114's `if pause: sleep(pause)`, and time.sleep raises " - "ValueError on a negative argument")) @given(attempts=st.integers(min_value=-5, max_value=12), delay=st.floats(min_value=-10, max_value=10, allow_nan=False, allow_infinity=False), @@ -510,11 +489,6 @@ def flaky(): "time.sleep raises ValueError on a negative argument") -@pytest.mark.xfail(strict=True, reason=( - "hashers.hash_file's chunk_size is unvalidated: read(0) returns b'' " - "immediately, so iter(..., b'') stops before the first byte and a " - "non-empty file hashes as empty. Not reachable from the CLI today, which " - "is the only reason this is not a live data-integrity bug")) def test_hash_file_rejects_a_zero_chunk_size(tmp_path): """A checksum that silently describes a different file than the one on disk is the worst failure mode this codebase has.""" @@ -568,11 +542,6 @@ def test_zero_byte_file_verifies(tmp_path): ("{index}", [f"{i:03d}" for i in range(1, 1000)]), ("{card}", ["A001"] + [f"A001-{i}" for i in range(2, 1000)]), ]) -@pytest.mark.xfail(strict=True, reason=( - "naming.build's two exhaustion fallbacks return an unchecked name: " - "naming.py:82 returns render(..., index=999) and naming.py:91 returns " - "`base`, neither tested against `used`. Two jobs then share one report " - "folder, which is the exact outcome the dedup exists to prevent")) def test_build_never_returns_a_taken_name_even_when_exhausted(template, taken): """`test_fuzz.py` already asserts this property, but with `max_size=8` it can never reach the 999-name ceiling where the guarantee lapses.""" @@ -593,10 +562,6 @@ def test_build_never_returns_a_taken_name_even_when_exhausted(template, taken): @pytest.mark.parametrize("body", CORRUPT_HISTORY) -@pytest.mark.xfail(strict=True, reason=( - "History.__init__ maps HistoryEntry.from_dict over read_json's result with " - "no try/except, and from_dict itself does bare int()/list() conversions, " - "so a corrupt history.json raises out of the constructor")) def test_corrupt_history_does_not_stop_an_offload(body, tmp_path): """history.py's own reason for existing is that a damaged history must never stop someone offloading a card. An exception from the constructor @@ -606,10 +571,6 @@ def test_corrupt_history_does_not_stop_an_offload(body, tmp_path): History(path) -@pytest.mark.xfail(strict=True, reason=( - "config.read_json catches OSError and json.JSONDecodeError, but " - "path.read_text raises UnicodeDecodeError (a ValueError) for invalid " - "UTF-8, which is not caught")) def test_history_survives_invalid_utf8(tmp_path): path = tmp_path / "history.json" path.write_bytes(b'[{"job_name": "\xff\xfe"}]') From af8784f752a1387e3a771d965ab228e6043af30a Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:04:53 -0600 Subject: [PATCH 04/14] Sweep the docs for the fuzzing fixes Behaviour that changed, documented where someone would look for it rather than only in the changelog. docs/data-safety.md carries the weight, since that is the file the PR template points at for anything touching the copy path. Five new rows in the guarded table, a section on why flattening is now refused rather than allowed to lose a file, and one honest addition to "what is still not protected": the collision check compares through os.path.normcase, so copying on Linux or macOS from a case-sensitive source onto a case-insensitive destination is the one arrangement it does not catch. Detecting that means asking the destination filesystem what it considers distinct, which is not implemented. docs/braw.md gains a section on reading a file that is lying to you: a declared size is a claim and the buffer is the fact. docs/ascmhl.md records the round-trip bug alongside the ones the reference diff caught, and explains why digests compare per algorithm rather than as strings. docs/performance.md no longer implies junctions are simply unhandled; they are still not reproduced at the destination, but one pointing at its own parent can no longer send the scan round in circles. README gets the corrected counts (482 tests, 83% coverage; it claimed 413 at 82%), the three property modules described by what each one fuzzes, and the --flat entry noting the refusal. CONTRIBUTING and the PR template had their own stale counts. CI's soak job only ever ran test_fuzz.py, so the two new property modules would not have been soaked at all. It now runs all three, which is 93 tests in about two and a half minutes. --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/ci.yml | 5 +++- CONTRIBUTING.md | 4 +-- README.md | 51 ++++++++++++++++++++++---------- docs/ascmhl.md | 25 ++++++++++++++++ docs/braw.md | 25 ++++++++++++++++ docs/data-safety.md | 30 ++++++++++++++++++- docs/performance.md | 7 +++-- 8 files changed, 126 insertions(+), 23 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9d9cfd1..16fcae3 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -12,7 +12,7 @@ the first attempt rather than burning three." ## Checklist -- [ ] `pytest` passes (400 tests) +- [ ] `pytest` passes (482 tests) - [ ] `ruff check src tests` passes - [ ] Tests added for the behaviour changed diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e548b1..1e6fbc0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,7 +104,10 @@ jobs: python -m pip install --upgrade pip python -m pip install -e ".[dev]" - name: Run the property tests at soak depth - run: pytest tests/test_fuzz.py --fuzz -q + # All three property modules: test_fuzz.py fuzzes what a filename can + # contain, test_fuzz_edges.py what a card can contain, and + # test_edge_cases.py the layers between the bytes and the paperwork. + run: pytest tests/test_fuzz.py tests/test_fuzz_edges.py tests/test_edge_cases.py --fuzz -q build: name: build the distribution diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c08f130..dda162f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,8 +37,8 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # ~400 tests, about 33s -pytest --fuzz # property tests at 3000 examples each, about 2 min +pytest # ~480 tests, about 20s +pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` diff --git a/README.md b/README.md index c8f1fc5..0b30b2b 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ offloader verify D:\video\080426\A001 | `--logo PATH` | image for the PDF header | | `--footer TEXT` | footer line for the PDF | | `--exclude GLOB` | extra filename pattern to skip; repeatable | -| `--flat` | do not recreate the source folder structure | +| `--flat` | do not recreate the source folder structure; refused if two files would land on one path | | `--skip-existing` | skip files already present at matching size | | `--retries N` | attempts per failing read on a transient error (default 3, 1 disables) | | `--retry-wait SECONDS` | pause before the first retry, backing off after (default 2) | @@ -295,7 +295,9 @@ The short version: `offloader verify` catches a single flipped bit in a file whose size never changed. Run it before erasing a card, and again on the archive months later to -catch bit rot. +catch bit rot. It reads manifests other tools wrote, and compares digests per +algorithm: hex case-insensitively, C4 exactly. Another tool's choice of casing +is not corruption, and saying it is would be a costly thing to get wrong. ## Blackmagic RAW @@ -357,13 +359,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 413 tests, ~33s -pytest --fuzz # same suite, 3000 examples per property (~2 min) +pytest # 482 tests, ~20s +pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -413 tests at 82% line coverage. They cover formatting against the reference's +482 tests at 83% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, @@ -381,11 +383,20 @@ real camera file when one is present. ### Property-based testing -`tests/test_fuzz.py` uses [Hypothesis][hyp] to assert invariants over generated -input rather than over a handful of fixtures. Filenames come off camera cards, -which in practice means any Unicode at all — accented takes, CJK slates, emoji -from a naming macro, and the occasional control character from a corrupt -directory entry. +Three modules use [Hypothesis][hyp] to assert invariants over generated input +rather than over a handful of fixtures, each aimed at a different layer: + +- **`tests/test_fuzz.py`: what a filename can contain.** Names come off camera + cards, which in practice means any Unicode at all: accented takes, CJK + slates, emoji from a naming macro, and the occasional control character from + a corrupt directory entry. +- **`tests/test_fuzz_edges.py`: what a card can contain.** A BRAW clip whose + atom headers lie, a tree that maps two sources onto one destination, a + directory junction pointing at its own parent, a retry policy someone + hand-edited into a preset. +- **`tests/test_edge_cases.py`: the layers between the bytes and the + paperwork.** What ffprobe hands back, what a number formats to, what a digest + compares equal to, what a name renders to. The properties worth knowing about: @@ -396,14 +407,22 @@ The properties worth knowing about: - Chunked hashing equals whole-buffer hashing for every algorithm at arbitrary chunk boundaries — the engine's boundaries fall wherever a read lands. - `sanitize()` always returns a legal filename, and `build()` never collides - with a name already taken. + with a name already taken, including once the numeric space is exhausted. - Presets survive a JSON round trip, and load from arbitrary garbage without raising — a hand-edited or version-skewed config must not brick the app. - -This found a real bug: XML 1.0 cannot represent most C0 control characters even -as character references, so a control byte in one filename produced an MHL that -no parser would read — stranding verification of the entire delivery, not just -that file. Names are now sanitised into the XML character range. +- No input makes an offload lose a file without saying so, no single malformed + file aborts a job, and a parser fed hostile bytes may return nothing but may + not raise something its caller has no reason to catch. + +These have earned their keep. The first found that XML 1.0 cannot represent +most C0 control characters even as character references, so a control byte in +one filename produced an MHL no parser would read, stranding verification of +the entire delivery rather than one file. The later two found 24 more, the +worst of which certified a file it had just overwritten: flattening a tree +mapped two clips onto one destination path, and each was verified as it landed, +before the next replaced it. Both were reported `Verified`. That check now runs +against the scan, before a byte moves. Every one of those 24 has a test here +that fails against the code as it was. [hyp]: https://hypothesis.readthedocs.io/ diff --git a/docs/ascmhl.md b/docs/ascmhl.md index c997cda..5093c77 100644 --- a/docs/ascmhl.md +++ b/docs/ascmhl.md @@ -70,6 +70,14 @@ Only the newest generation of a history is checked. Every generation covers the same files, so verifying all of them would hash the media once per generation for no additional evidence. +Digests are compared per algorithm rather than as plain strings. Hex formats +compare case-insensitively, because a manifest is an interchange format and +plenty of tools emit their hex uppercase: the same bytes described in a +different case are not a mismatch, and reporting one is the most expensive +false alarm this tool can raise. C4 compares exactly, since its base58 alphabet +uses case to carry information and folding it would make genuinely different +identifiers compare equal. + ## Validation Implementing a standard from prose is how you end up with something that only @@ -103,6 +111,23 @@ the manifest entirely rather than recorded with `action="failed"`. Excluding them from directory hashes is correct; excluding them from the document destroys the evidence the format exists to carry. +A second one came from fuzzing the round trip rather than diffing it. A +`` is a filename off a filesystem, not validated text: a control +character in one produces a manifest no parser will read, and +`read_manifest_hashes` answers a parse failure by returning nothing, so that +file would leave the chain of custody with nothing to indicate it ever left. A +lone surrogate (what `os.listdir` hands back for any POSIX name that is not +valid UTF-8, and what NTFS accepts outright) failed earlier still, in the +UTF-8 encode, so no manifest was written at all. + +Names are now filtered to the XML 1.0 character set, replacing anything outside +it with U+FFFD. The filter runs where a filename first enters the manifest, so +the structure hashes and the `` text they describe are computed from the +same string; sanitising only at serialisation time would leave the two +disagreeing. It is the same filter the plain-MHL writer has always used, which +is the point: it now lives in `util` and covers the ASC MHL and CSV writers +too, rather than each having to rediscover it. + ## Limits - **Directory hashes are written but not verified.** `offloader verify` checks diff --git a/docs/braw.md b/docs/braw.md index 0a80cbe..89aef04 100644 --- a/docs/braw.md +++ b/docs/braw.md @@ -102,6 +102,31 @@ the original was already broken. The same walk catches an atom whose declared length runs past the end of the file, which is what a truncated transfer from some other tool looks like. +## Reading a file that is lying to you + +Every offset in this parser is derived from a length the *file* supplied, and a +corrupt clip supplies whatever it likes. The rule throughout is that a declared +size is a claim and the buffer is the fact: + +- Fixed-offset reads inside an atom are bounded by that atom's real end, so a + short `mdhd` or an `stts` promising entries it does not carry yields nothing + rather than raising out of the parser. +- Descent into container atoms is depth-limited. Real BRAW nests about six + deep; without a cap, a 16 KB file of nothing but nested headers exhausts the + interpreter stack. +- The `moov` read is clamped to the bytes actually present. The extended-size + header can claim up to 2^64, and asking the allocator for that is its own + kind of failure. +- Sensor dimensions arrive as a raw IEEE 754 pair, so they can decode to NaN or + infinity. Neither survives `int()`, and both are rejected. + +None of this makes a broken clip readable. It makes a broken clip cost its own +metadata instead of the rest of the card: metadata is read *after* the bytes +are copied and verified, and a container that will not parse now leaves a +warning on the job rather than ending it. `tests/test_fuzz_edges.py` fuzzes +this by pinning the atom shape `_read_timing` looks for and generating the two +bodies it then reads at fixed offsets. + ## Testing Fixtures are synthesised — `tests/test_braw.py` builds a structurally faithful diff --git a/docs/data-safety.md b/docs/data-safety.md index 0dd67e3..a5e70db 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -150,12 +150,32 @@ exists precisely so it can be checked independently. | --- | --- | | Empty (0-byte) source file | Warned. It hashes and verifies perfectly, and is still a lost take. | | Destination without room | Checked before queueing; prompts. | -| Re-offloading a card already pulled | Fingerprinted by name and size, warned with the earlier job's name and date. Only successful offloads count. | +| Re-offloading a card already pulled | Fingerprinted by source name plus the listing's names and sizes, warned with the earlier job's name and date. Only successful offloads count. | | Card pulled mid-copy | `OSError` fails that file; the partial is deleted; other files are unaffected. | | Quit during an offload | Confirmed, then cancelled cleanly, partials removed. | | Cancel | Deletes the in-flight partial rather than leaving plausible-looking media. | | Report writer crashing | Caught; a bad PDF never invalidates a good copy. | | Corrupt config file | Treated as empty. Config must never stand between someone and their card. | +| Two source files mapping to one destination path | Refused before a byte moves, naming both sources. See below. | +| Metadata that will not parse | The file still copies and verifies; only its metadata is lost, and the job carries a warning. | +| A directory junction pointing at its own parent | Each directory is visited at most once, so the scan cannot loop. | +| A manifest whose digests are uppercase | Compared case-insensitively for hex formats, exactly for C4. Another tool's casing is not corruption. | +| A filename XML or UTF-8 cannot carry | Filtered to U+FFFD in manifests and CSV, so one bad name cannot strand a delivery's paperwork. | + +### Flattening is refused when it would lose a file + +`--flat` maps every source onto `destination/name`. Two clips of the same name +in different folders therefore want the same destination path, and copying both +leaves one file where there were two. This is the worst shape a bug can take +here: each copy is verified as it lands, *before* the next overwrites it, so +both would be reported `Verified` and the report would attest to a file that is +no longer there. + +The collision is detected against the scan, before any byte moves, and the +offload refuses with both source paths named. Paths are compared through +`os.path.normcase`, so on Windows this also catches two names differing only in +case: distinct on the case-sensitive volume they came from, one file on the +volume they are going to. ## What is still not protected @@ -170,6 +190,14 @@ known. safety one, and should not be used on a tree whose integrity is in question. - **Concurrent instances.** One app instance serialises its queue. Two instances pointed at the same destination are not coordinated. +- **Case-only filename collisions on a case-insensitive destination, off + Windows.** The collision check compares with `os.path.normcase`, which folds + case on Windows and is the identity elsewhere. Copying on Linux or macOS from + a case-sensitive source onto a case-insensitive destination (an exFAT card, + an SMB share) is therefore the one arrangement where `Clip.mov` and + `clip.MOV` are still two files at the source and one at the destination. + Detecting it means asking the destination filesystem what it considers + distinct, which is not implemented. - **A source that changes while it is being read.** The checksum is of the bytes that were read, so a file modified mid-copy, or swapped between a retry's close and reopen, produces a destination that faithfully matches diff --git a/docs/performance.md b/docs/performance.md index 7d77a55..c543c1f 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -103,8 +103,11 @@ Two of the three have since been adopted; both are covered in retried at the failing chunk rather than the whole file, so one marginal sector does not cost a re-read of the clip. - **Metadata fidelity.** ACLs, alternate data streams, junctions. Still not - handled. Not relevant to camera originals, but relevant if anyone points this - at a general file tree. + *reproduced* at the destination, which is not relevant to camera originals + but is if anyone points this at a general file tree. A junction in the source + is at least no longer a hazard: the scan visits each directory once, so one + pointing back at its own parent cannot send it round in circles. That guard + cannot lean on `Path.is_symlink()`, which is False for a junction. ## Reproducing From 94c135caf386588842ba1e50773c7a1c75a7b2ac Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:08:55 -0600 Subject: [PATCH 05/14] Second pass on the docs: fix a contradiction the sweep left behind The first sweep updated docs/data-safety.md to say the duplicate-card fingerprint covers the source name plus the listing, and left README saying the listing alone. Two files, two answers, same function. Three gaps alongside it. The fingerprint change makes pre-upgrade entries stop matching, which belonged in the honest-limits list rather than only in the changelog. RetryPolicy's clamping is now noted in the paragraph that says 'or per preset', since a hand-edited preset is exactly how a negative delay reaches time.sleep. And CONTRIBUTING's 'testing without a camera card' table now names the new modules, which cover four of its own categories: a lying BRAW header, a junction cycle, a destination collision, and ffprobe returning N/A. --- CONTRIBUTING.md | 6 +++++- README.md | 9 +++++---- docs/data-safety.md | 12 ++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dda162f..3ea8943 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # ~480 tests, about 20s +pytest # 482 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing @@ -63,6 +63,10 @@ suite fakes all of it: | Corruption | flip a byte and re-verify; size stays identical, checksum does not | | An interrupted recording | write a BRAW with `include_moov=False` | | A deep Windows path | build until `len(str(path)) > 260` — do not hard-code, the temp root's length varies | +| A lying BRAW header | `tests/test_fuzz_edges.py::atom` declares whatever size you pass, including the 64-bit extended form | +| A directory-junction cycle | `tests/test_fuzz_edges.py` shells out to `mklink /J` and skips when it cannot | +| Two files that want one destination | `tests/test_fuzz_edges.py` flattens a tree with a repeated basename | +| ffprobe returning `"N/A"` | `tests/test_edge_cases.py` stubs `subprocess.run`; it is ordinary output, not corruption | If you have real hardware, there are tests that use it and skip cleanly when it is absent (`test_braw.py::test_synthetic_fixture_matches_a_real_camera_file`). diff --git a/README.md b/README.md index 0b30b2b..d55ee40 100644 --- a/README.md +++ b/README.md @@ -255,10 +255,11 @@ destination file rather than leaving something that looks complete. Two guards run before anything is queued: -- **Duplicate offload protection.** The source's file listing — names and sizes, - never contents — is fingerprinted and checked against past offloads. Re-pulling - a card you already have gets a warning naming the earlier job and when it ran. - Only successful offloads count; a cancelled attempt is a reason to run again. +- **Duplicate offload protection.** The source's name and its file listing (names + and sizes, never contents) are fingerprinted and checked against past offloads. + Re-pulling a card you already have gets a warning naming the earlier job and + when it ran. Only successful offloads count; a cancelled attempt is a reason to + run again. - **Space and containment checks.** A destination inside the source is refused outright; one without room prompts before queueing. diff --git a/docs/data-safety.md b/docs/data-safety.md index a5e70db..433a8db 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -198,6 +198,12 @@ known. `clip.MOV` are still two files at the source and one at the destination. Detecting it means asking the destination filesystem what it considers distinct, which is not implemented. +- **Duplicate detection for cards offloaded before this version.** The + fingerprint now includes the source's name as well as its listing, so entries + recorded earlier no longer match and their cards will not raise the + "already offloaded" warning. It fails in the safe direction, since a missing + warning is a wasted hour rather than lost footage, and each card re-records + correctly the next time it is pulled. - **A source that changes while it is being read.** The checksum is of the bytes that were read, so a file modified mid-copy, or swapped between a retry's close and reopen, produces a destination that faithfully matches @@ -212,6 +218,12 @@ that fails for a transient-looking reason is retried — three attempts by default, backing off from two seconds, matching robocopy's `/R` and `/W`. Tune with `--retries` and `--retry-wait`, or per preset. +Those values are clamped where the policy is built, not where it is used: +at least one attempt, and no negative delay or backoff. `presets.json` is a +file people edit by hand, and the failure it prevents is a specific one, since +`time.sleep` raises on a negative argument. A nonsensical retry setting should +cost you the retry, not the transfer. + The discrimination matters more than the retrying. Retrying a missing file, a permission denial or a full disk wastes time and buries the real fault in a delay, so only errors with a plausible transient cause qualify: `EIO`, `EBUSY`, From 94be98d637800f77acc0d9e824a9c9f442fc28b4 Mon Sep 17 00:00:00 2001 From: Owen Kent Date: Thu, 8 Oct 2026 18:23:38 -0400 Subject: [PATCH 06/14] Do not let a directory symlink hide the directory it points at The scan's cycle guard recorded every directory's resolved path, including directory symlinks that os.walk never enters. With a -> z listed before z, the guard marked z as seen and dropped it, the walk skipped a as a link, and z's files were never copied while the job still reported VERIFIED. Only directories the walk actually enters are recorded now, so junctions keep the guard. The regression pins the alias-first listing order. --- CHANGELOG.md | 6 ++++ src/offloader/engine.py | 13 +++++++- tests/test_fuzz_edges.py | 66 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d08606e..83eef23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,12 @@ paperwork. Each fix has the reproduction that found it. junction, so the usual check would not have helped. It terminated only because Windows refuses paths past MAX_PATH, having by then returned the same file dozens of times. Directories are now visited at most once each. +- **That cycle guard could hide a real directory behind a symlink to it.** It + recorded every directory's resolved path, including directory symlinks that + `os.walk` never enters. With `a -> z` listed before `z`, the guard marked `z` + as seen, dropped the real `z`, and the walk skipped `a` as a link, so `z`'s + files were never copied and the job still reported VERIFIED. Only directories + the walk actually enters are recorded now; junctions keep the guard. - **A corrupt history blocked offloading**, which is precisely what it exists not to do. `History()` mapped `from_dict` over the file with no guard, and `from_dict` did bare `int()`/`list()` conversions, so a hand-edited or diff --git a/src/offloader/engine.py b/src/offloader/engine.py index 68e61a5..b5dc7e0 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -257,12 +257,22 @@ def scan(root: Path, excludes: Iterable[str] = DEFAULT_EXCLUDES) -> list[Path]: left to chance without this: today it only stops because Windows refuses paths past MAX_PATH, and it stops having already returned the same file dozens of times. + + Only directories the walk will actually enter are recorded. `os.walk` + does not follow a directory symlink, so recording one would mark its + target as seen without anything having scanned it: an alias `a -> z` + listed before `z` would then hide `z` itself, and neither path's files + would be found. A junction is not a symlink to `os.walk`, which does enter + it, so a junction still goes through the guard. """ patterns = tuple(excludes) found: list[Path] = [] visited: set[str] = set() def already_seen(directory: Path) -> bool: + if os.path.islink(directory): + # Not followed by os.walk, so not a visit. See the docstring. + return False try: real = os.path.normcase(os.path.realpath(directory)) except OSError: # pragma: no cover - unreadable entry @@ -272,7 +282,8 @@ def already_seen(directory: Path) -> bool: visited.add(real) return False - already_seen(Path(root)) + # The root is always entered, even when it is itself a symlink. + visited.add(os.path.normcase(os.path.realpath(root))) for dirpath, dirnames, filenames in os.walk(root): here = Path(dirpath) dirnames[:] = sorted( diff --git a/tests/test_fuzz_edges.py b/tests/test_fuzz_edges.py index e882421..cc7ae44 100644 --- a/tests/test_fuzz_edges.py +++ b/tests/test_fuzz_edges.py @@ -28,6 +28,7 @@ import datetime as _dt import errno import json +import os import struct import subprocess import sys @@ -392,6 +393,71 @@ def test_scan_does_not_follow_a_directory_junction_into_a_cycle(tmp_path): f"one real file was scanned {found[0]} times through the junction") +class _ListedInOrder: + """`os.scandir` with the entries handed back in a fixed order, so a test + can pin the one listing order a filesystem might or might not produce.""" + + def __init__(self, entries: list[os.DirEntry], first: str): + self._entries = iter(sorted(entries, key=lambda e: (e.name != first, e.name))) + + def __iter__(self): + return self + + def __next__(self) -> os.DirEntry: + return next(self._entries) + + def __enter__(self): + return self + + def __exit__(self, *_exc) -> None: + return None + + def close(self) -> None: + return None + + +def test_a_directory_symlink_listed_first_does_not_hide_its_target(tmp_path, monkeypatch): + """`os.walk` does not follow a directory symlink, but the cycle guard used + to record the symlink's *target* as visited anyway. With `a -> z` listed + before `z`, the guard then dropped the real `z` as already seen and the + walk skipped `a` as a link, so `z/clip.bin` was scanned by neither path. + The job came back VERIFIED with no warning and the clip was never copied. + """ + source, dest = tmp_path / "card", tmp_path / "dst" + (source / "z").mkdir(parents=True) + (source / "z" / "clip.bin").write_bytes(b"z" * 64) + (source / "keep.txt").write_bytes(b"k" * 8) + try: + (source / "a").symlink_to(source / "z", target_is_directory=True) + except OSError as exc: + pytest.skip(f"could not create a directory symlink: {exc}") + + real_scandir = os.scandir + + def alias_first(path=".", *args): + with real_scandir(path, *args) as listing: + return _ListedInOrder(list(listing), first="a") + + monkeypatch.setattr(os, "scandir", alias_first) + with os.scandir(source) as listing: + assert [e.name for e in listing][0] == "a", "listing order was not pinned" + + job = _offload(source, dest) + + copied = sorted(p.relative_to(dest).as_posix() for p in dest.rglob("*") + if p.is_file() and p.suffix in {".bin", ".txt"}) + assert copied == ["keep.txt", "z/clip.bin"], f"copied only {copied}" + assert (dest / "z" / "clip.bin").read_bytes() == b"z" * 64 + assert sorted(e.source.relative_to(source).as_posix() for e in job.files) == \ + ["keep.txt", "z/clip.bin"] + assert [d.status for e in job.files for d in e.destinations] == \ + [FileStatus.VERIFIED, FileStatus.VERIFIED] + + report = write_csv(job, tmp_path / "JobReport.csv").read_text(encoding="utf-8") + assert "clip.bin" in report, "the copied clip is missing from the report" + assert "keep.txt" in report + + # ----------------------------------------------------------- report totality From 501772c6d152884efd15d751c7e43d3239871105 Mon Sep 17 00:00:00 2001 From: Owen Kent Date: Thu, 8 Oct 2026 20:16:59 -0400 Subject: [PATCH 07/14] Compare ASC MHL directory hashes per algorithm, as file hashes already are --- CHANGELOG.md | 3 ++- CONTRIBUTING.md | 2 +- README.md | 4 ++-- docs/ascmhl.md | 14 +++++++------- src/offloader/verify.py | 7 +++++-- tests/test_ascmhl.py | 19 +++++++++++++++++++ 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66eab56..bf44d10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -225,7 +225,8 @@ paperwork. Each fix has the reproduction that found it. case-sensitive `==`, so a manifest from a tool that emits uppercase hex reported every byte-identical file as a mismatch. Comparison is now per algorithm: case-folded for hex, exact for C4, whose base58 alphabet uses - case to carry information. + case to carry information. ASC MHL directory hashes are compared the same + way. - **A directory junction sent the scanner round in circles.** `scan` was a bare `os.walk` with no cycle guard, and `Path.is_symlink()` is False for a junction, so the usual check would not have helped. It terminated only diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2353aa5..cca7d3d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # 549 tests, about 20s +pytest # 550 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index e00c04c..969336e 100644 --- a/README.md +++ b/README.md @@ -394,13 +394,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 549 tests, ~20s +pytest # 550 tests, ~20s pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -549 tests at 85% line coverage. They cover formatting against the reference's +550 tests at 85% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, diff --git a/docs/ascmhl.md b/docs/ascmhl.md index 989318a..ffcdc41 100644 --- a/docs/ascmhl.md +++ b/docs/ascmhl.md @@ -102,13 +102,13 @@ Only the newest generation of a history is checked. Every generation covers the same files, so verifying all of them would hash the media once per generation for no additional evidence. -Digests are compared per algorithm rather than as plain strings. Hex formats -compare case-insensitively, because a manifest is an interchange format and -plenty of tools emit their hex uppercase: the same bytes described in a -different case are not a mismatch, and reporting one is the most expensive -false alarm this tool can raise. C4 compares exactly, since its base58 alphabet -uses case to carry information and folding it would make genuinely different -identifiers compare equal. +Digests, file and directory alike, are compared per algorithm rather than as +plain strings. Hex formats compare case-insensitively, because a manifest is an +interchange format and plenty of tools emit their hex uppercase: the same bytes +described in a different case are not a mismatch, and reporting one is the most +expensive false alarm this tool can raise. C4 compares exactly, since its base58 +alphabet uses case to carry information and folding it would make genuinely +different identifiers compare equal. ## Validation diff --git a/src/offloader/verify.py b/src/offloader/verify.py index 3e4281d..1717b82 100644 --- a/src/offloader/verify.py +++ b/src/offloader/verify.py @@ -495,6 +495,9 @@ def _directory_verdicts(base: Path, recorded: dict[str, tuple[str, str]], [(Path(relative), digest) for relative, digest in on_disk.items()], algorithm_key) + # The same per-algorithm comparison the file hashes get: a directory hash + # another tool wrote in uppercase hex describes the same tree. + same = get_algorithm(algorithm_key).digests_match verdicts: list[DirectoryVerdict] = [] for relative in sorted(recorded): expected_content, expected_structure = recorded[relative] @@ -517,9 +520,9 @@ def _directory_verdicts(base: Path, recorded: dict[str, tuple[str, str]], continue actual_content, actual_structure = found - if actual_content != expected_content: + if not same(actual_content, expected_content): result = DirectoryResult.CHANGED - elif actual_structure != expected_structure: + elif not same(actual_structure, expected_structure): result = DirectoryResult.RENAMED else: result = DirectoryResult.OK diff --git a/tests/test_ascmhl.py b/tests/test_ascmhl.py index 11267ea..96cfa76 100644 --- a/tests/test_ascmhl.py +++ b/tests/test_ascmhl.py @@ -364,6 +364,25 @@ def test_verify_rechecks_the_recorded_directory_hashes(history): assert "directory hashes" in report.summary() +def test_uppercase_directory_hashes_still_verify(history): + """Directory hashes are compared the way file hashes are: hex without + regard to case. Another tool's uppercase is not a changed tree.""" + _job, destination, manifest = history + root = ET.parse(manifest).getroot() + shouted = 0 + for side in root.iter(): + if side.tag.rsplit("}", 1)[-1] in ("content", "structure"): + for digest in side: + digest.text = digest.text.upper() + shouted += 1 + assert shouted + ET.ElementTree(root).write(manifest, encoding="utf-8", xml_declaration=True) + + report = verify.verify_manifest(verify.find_manifests(destination)[0]) + assert report.directories + assert all(v.result is verify.DirectoryResult.OK for v in report.directories) + + def test_a_rename_is_a_structure_mismatch_not_a_content_one(history): """The whole reason the structure hash exists: every file is individually fine, and the tree is still not what was recorded.""" From 35901873f73d4cefe38bc73a0796484535a09f5b Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:28:51 -0600 Subject: [PATCH 08/14] Update test counts --- CONTRIBUTING.md | 2 +- README.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cca7d3d..17a5c8d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # 550 tests, about 20s +pytest # 553 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index 969336e..6100e2d 100644 --- a/README.md +++ b/README.md @@ -394,13 +394,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 550 tests, ~20s +pytest # 553 tests, ~20s pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -550 tests at 85% line coverage. They cover formatting against the reference's +553 tests at 85% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, From 43738ddc6b109bfdb573237458f346e26942c72c Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:29:12 -0600 Subject: [PATCH 09/14] Keep every scanned file in the manifest and never let a failure vanish into Verified A file that failed to copy was left out of the MHL, so a later verify said safe to erase. A skip-existing rerun overwrote the manifest with one that omitted the skipped files, so a corrupt same-size copy passed. A directory the scan could not list was dropped without a word. A source name ending in a dot or space opened its namesake on Windows and both were certified. A junction listed before its target claimed the real directory. Reports could land on a copied folder of the same name and overwrite certified files. Report-writer failures were swallowed. A second card with the same relative paths silently replaced verified files. With --report-dir inside a second destination, the only manifest described the first. An incomplete job exited 0. Verify's unlisted-file scan switched itself off for small manifests. A file that vanished after the scan aborted the run. Thumbnails for same-named clips overwrote each other. ASC MHL compared previous digests case-sensitively. Manifests now list every scanned file per destination, failed ones with the source hash and unhashed ones as such, so verify cannot pass them. Manifests are never overwritten; a reused report folder gets a numbered name. Unlistable directories, skipped links, vanished files and report failures are recorded on the job and fail the verdict. All source and destination access goes through literal long paths. Promotion never replaces an existing file whose content differs unless --replace-existing is given. Every destination gets its own manifest. The CLI exit codes are documented. --- CHANGELOG.md | 54 +++ CONTRIBUTING.md | 2 +- README.md | 29 +- docs/ascmhl.md | 9 + docs/data-safety.md | 99 +++++- src/offloader/ascmhl.py | 55 +-- src/offloader/cli.py | 109 +++--- src/offloader/engine.py | 328 +++++++++++++++--- src/offloader/gui/main_window.py | 13 + src/offloader/gui/worker.py | 51 +-- src/offloader/integrity.py | 6 +- src/offloader/longpath.py | 95 ++++- src/offloader/models.py | 147 +++++++- src/offloader/reports/mhl.py | 72 +++- src/offloader/reports/paperwork.py | 120 +++++++ src/offloader/thumbs.py | 19 +- src/offloader/verify.py | 50 ++- tests/test_reports.py | 9 +- tests/test_verdict_integrity.py | 533 +++++++++++++++++++++++++++++ 19 files changed, 1562 insertions(+), 238 deletions(-) create mode 100644 src/offloader/reports/paperwork.py create mode 100644 tests/test_verdict_integrity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e36076..cf7ca11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,60 @@ project uses [semantic versioning][semver]. ### Fixed +- **`verify` passed a destination whose offload had failed a file.** The + manifest listed only the files that copied, so a clip that failed was simply + absent and `verify` said "VERIFIED - safe to erase the source". MHL and ASC + MHL now list every scanned file for each destination: a failed copy with the + source checksum (verify finds it missing or different), and anything with no + checksum at all (an unreadable source, a file a cancel never reached, a folder + that could not be listed) as an unhashed entry, which verify fails. +- **A `--skip-existing` rerun overwrote the manifest that covered the skipped + files.** A same-size corrupt copy the size check skipped then verified clean. + A manifest is never overwritten now: a rerun's paperwork goes to + `_Reports-2` when `_Reports` already exists, a reused + `--report-dir` gets `JobReport-2.mhl`, and `verify` checks every manifest + under the folder. +- **A source folder that could not be listed was silently left out.** The job + read Verified without the files under it. Listing errors are now recorded, + the job fails naming the folder, the CLI exits `1`, and the manifests carry an + unhashed entry so `verify` fails as well. +- **Names Windows rewrites were read as a different file.** `clip.mov.` opened + `clip.mov`, so the wrong bytes were copied and certified, and `NUL` read as an + empty device. Such names now take the extended-length prefix everywhere (scan, + copy, stat, rename, eviction, manifest paths, verify) and copy as themselves. +- **A junction listed before its target claimed the real folder.** The real + folder was skipped and its files landed under the alias. Directory symlinks + and junctions are no longer entered at all, and each one skipped is named in + a warning. +- **Reports overwrote copied files when the source held `_Reports`.** + Re-offloading a previous destination replaced the shuttle's own manifest + after certifying it. Reports now go to the next free `_Reports-N` with + a warning. +- **Report failures disappeared into a Verified verdict.** The GUI swallowed + them and the CLI exited `0`. Failures are recorded on the job + (`report_failures`), the status reads `Verified, reports failed`, the GUI + lists them on completion and the CLI exits `5`. +- **A different card with the same folder layout replaced verified files.** A + finished copy no longer replaces an existing file whose content differs: that + file fails with the reason and the existing copy is kept. Identical content is + still replaced. `--replace-existing` (`OffloadOptions.replace_existing`) + restores the old behaviour. +- **With `--report-dir` inside the second destination, its only manifest + described the first.** Every destination now gets its own manifest inside it; + `--report-dir` moves the PDF, CSV and HTML and holds a manifest only for the + destination it sits in. +- **`offload` exited `0` for an Incomplete job.** An empty source or a + `--skip-existing` rerun now exits `4`; the exit codes are documented in the + README. +- **`verify` did not report unlisted files beside a one-file manifest.** The + scan now starts at the root the manifest belongs to. +- **A file deleted after the scan aborted the whole offload.** It now fails + alone with "source vanished after scan" and the job carries on. +- **Same-named clips in different folders shared thumbnail files.** One clip's + frames replaced the other's in the report; thumbnails are now named by a hash + of the relative path. +- **An uppercase hex digest in a previous ASC MHL generation was recorded as + `failed`.** The comparison is now case-insensitive for hex and exact for C4. - **Pause and cancel are honoured during rereads.** A cancel issued during the `--paranoid` source reread or the full-verification destination reread was ignored, and the file was promoted and reported Verified. Both rereads now diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 17a5c8d..63db694 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # 553 tests, about 20s +pytest # 575 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index 6100e2d..a70a5b9 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,7 @@ offloader verify D:\video\080426\A001 | `--profile P` | `media` (default: ffprobe, thumbnails, BRAW) or `data` (generic transfer, no media probing) | | `--generic` | shorthand for `--profile data` | | `--report FMT[,FMT]` | `pdf` (default), `csv`, `mhl`, `ascmhl`, `html` | -| `--report-dir PATH` | override the report location | +| `--report-dir PATH` | move the PDF, CSV and HTML; each destination still gets its own MHL inside it (the folder holds one only for the destination it sits in) | | `--thumbs N` | frames per clip, 0 to disable (default 4) | | `--name NAME` | job name; defaults to the source folder name, or the volume label for a card offloaded from its root | | `--logo PATH` | image for the PDF header | @@ -102,14 +102,26 @@ offloader verify D:\video\080426\A001 | `--exclude GLOB` | extra filename pattern to skip; repeatable | | `--flat` | do not recreate the source folder structure; refused if two files would land on one path | | `--skip-existing` | skip files already present at matching size | +| `--replace-existing` | let a copy replace an existing destination file whose content differs; by default that file is kept and the copy fails (offload only) | | `--paranoid` | read each source file twice and compare (offload only) | | `--retries N` | attempts per failing read on a transient error (default 3, 1 disables) | | `--retry-wait SECONDS` | pause before the first retry, backing off after (default 2) | | `--no-probe` | skip ffprobe metadata and thumbnails | | `--quiet` | suppress progress | -Exit status is `0` on success, `1` if any file failed verification, `2` on a -usage or I/O error, `3` if a destination was refused as unsafe. +Exit status for `offload`: + +| Code | Meaning | +| --- | --- | +| `0` | Verified (or Copied under `--verify none`), and every report was written | +| `1` | a file failed, or a source folder could not be listed | +| `2` | usage or I/O error; nothing was attempted | +| `3` | a destination was refused as unsafe; nothing was copied | +| `4` | nothing failed, but the job is not Verified: an empty source, files skipped by `--skip-existing` (checked by size only), or a cancel | +| `5` | the copies verified but a report or manifest could not be written | +| `130` | interrupted | + +`report` exits `5` when a report could not be written and `0` otherwise. ### `verify` @@ -119,8 +131,11 @@ offloader verify PATH [--allow-cache] [--quiet] `PATH` is an `.mhl` file or a folder to search for them. Re-hashes everything the manifest lists and exits non-zero if anything is off, so a format script can gate -on it. `--allow-cache` skips the page-cache eviction — faster, and may verify -memory rather than the device. +on it. An offload's manifest lists every file it scanned for that +destination, including any that failed (recorded with no checksum when none +could be taken), so a destination missing a clip fails here; every manifest +under the folder is checked, earlier runs' included. `--allow-cache` skips the +page-cache eviction: faster, and may verify memory rather than the device. For an ASC MHL history it also recomputes the directory content and structure hashes, which is the only check that catches a rename or a moved file — every @@ -394,13 +409,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 553 tests, ~20s +pytest # 575 tests, ~20s pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -553 tests at 85% line coverage. They cover formatting against the reference's +575 tests at 87% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, diff --git a/docs/ascmhl.md b/docs/ascmhl.md index ffcdc41..99cf869 100644 --- a/docs/ascmhl.md +++ b/docs/ascmhl.md @@ -39,6 +39,15 @@ A `failed` hash is recorded but never reused: the spec is explicit that only failed file is excluded from the directory hashes so a directory hash can never certify a tree known to contain a bad file. +A file whose copy to this destination failed is recorded `failed` too, with +the source hash, and one the offload could not hash at all (an unreadable +source, a file a cancel never reached, a folder the scan could not list) is +recorded as a bare `` with no hash. Either way `verify` fails it, so a +history cannot read as complete for a copy that is missing a file. The +comparison with the previous generation is per algorithm: hex digests +case-insensitively, C4 exactly, so another tool's uppercase hex is not +recorded as `failed`. + ## Directory hashes Each directory gets a `content` and a `structure` hash, and the root gets both diff --git a/docs/data-safety.md b/docs/data-safety.md index 7b5e468..c9c2feb 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -130,6 +130,12 @@ checksum finds that. It also reports files present on disk that the manifest does not list, and it evicts each file before reading so a freshly written tree is read off the device. +The unlisted-file scan starts at the root the manifest belongs to (the folder +holding `_Reports`), not at the common folder of the files it lists, which +for a one-file manifest was the file itself and found nothing. + +Unlisted files are a diagnostic, not a failure. What makes a missing clip fail is +that the manifest lists it; see "A manifest lists what did not arrive" below. Where the manifest is an ASC MHL history, the directory hashes are recomputed too, and they catch the one class of change no checksum can. Rename a clip and @@ -165,13 +171,64 @@ eviction, not the reading. An MHL that records absolute paths is useless the moment the drive gets a different letter or the tree moves. This was broken here: reports live in `_Reports/` while the media sits one level up, so `Path.relative_to` -failed and silently fell back to absolute paths. Now `os.path.relpath` is used, -so entries look like `../Clips/A001_C001.mov` and the manifest works wherever the -tree is mounted. - -A manifest is also written **per destination**. One that lives only with the -first copy cannot re-verify the second — and the second copy is the one that -exists precisely so it can be checked independently. +failed and silently fell back to absolute paths. Entries are now relative, like +`../Clips/A001_C001.mov`, so the manifest works wherever the tree is mounted. The +relative path is computed lexically: `os.path.relpath` goes through Windows' +path normalisation, which would record `clip.mov.` as `clip.mov`. + +A manifest is also written **per destination**, always inside that destination +(`/_Reports/JobReport.mhl`), so each copy can be verified on its own. +One that lives only with the first copy cannot re-verify the second, and the +second copy is the one that exists precisely so it can be checked independently. +`--report-dir` moves the PDF, CSV and HTML. It receives a manifest only for the +destination it sits inside, if any. It used to receive destination 1's manifest +even when it sat inside destination 2, which then skipped its own: `verify` on +destination 2 re-hashed destination 1's copy and passed while its own copy was +corrupt. + +### A manifest lists what did not arrive + +A manifest used to list only the files that copied. A clip that failed was +simply absent, so `verify` on that destination checked everything it was told +about and said `VERIFIED`, and a format script gated on it would have erased a +card holding a clip that was never copied. + +Every file the job scanned is now listed for each destination: + +- A copy that failed is listed with the *source* checksum when one was taken, + so `verify` reports it missing or mismatched. When none was (the source could + not be read), it is listed with no checksum, which `verify` reports as + `no hash` and fails. In ASC MHL the entry is labelled `failed`, or is a bare + path with no hash. +- Files a cancelled job never reached, and folders the scan could not list, are + listed with no checksum for the same reason. +- Files skipped by `--skip-existing` are **not** listed: nothing re-read them, + and listing them would certify bytes nobody checked. + +An unhashed entry has nothing to pass against, so it keeps failing. After a +failed run has been resumed, its manifest still lists the files it could not +read; once the rerun has verified them, retire that manifest by hand. Nothing +else can say those files were checked. + +### A manifest is never overwritten + +A rerun used to overwrite `JobReport.mhl`. With `--skip-existing` the new one +listed only what that run copied, so the only record of the skipped files was +gone, and a same-size corrupt copy that the size check skipped then verified +clean. + +Now the job's paperwork folder is chosen before the copy: `_Reports`, or +`_Reports-2` and up when that name already exists in any destination or is +part of what is being copied. When a manifest would still land on an existing +file (a reused `--report-dir`), it is written as `JobReport-2.mhl` and up, and +the summary says so. `verify` on a folder checks every classic manifest under it, +so the earlier run's coverage is still checked. + +Re-offloading a previous destination (a shuttle drive holding +`A001/A001_Reports/`) used to write the new reports over the copied folder, +replacing files the job had just certified, the shuttle's own manifest among +them. The folder choice above covers that case too, with a warning naming the +folder used. ## Other things that are guarded @@ -183,11 +240,16 @@ exists precisely so it can be checked independently. | Card pulled mid-copy | `OSError` fails that file; the partial is deleted; other files are unaffected. | | Quit during an offload | Confirmed, then cancelled cleanly, partials removed. | | Cancel | Deletes the in-flight partial rather than leaving plausible-looking media. Pause and cancel are also honoured during the `--paranoid` source reread and the full-verification destination reread, so a cancel there never promotes the file. | -| Report writer crashing | Caught; a bad PDF never invalidates a good copy. | +| Report writer crashing | Caught; a bad PDF never changes a file's status. Every failure is recorded on the job, which then reads `Verified, reports failed` rather than `Verified`; the GUI lists the failures when the job finishes and the CLI prints them and exits `5`. | +| A source folder that cannot be listed | The job fails, naming the folder, and the CLI exits `1`. Its manifests carry an unhashed entry for the folder, so `verify` fails too. It used to be dropped silently and the job read Verified. | +| A file deleted after the scan | That file fails with "source vanished after scan"; the rest of the job carries on and its paperwork is written. | +| A different card with the same folder layout | A finished copy never replaces an existing destination file whose content differs. The existing file is hashed and compared; identical content is replaced (that changes nothing), different content fails that file with the reason and keeps the existing copy. `--replace-existing` overrides this. | +| A source that already holds `_Reports` | Reports go to `_Reports-2` (or the next free number) with a warning; the copied folder is left as copied. | +| Same-named clips in different folders | Thumbnail files are named by a hash of each clip's relative path, so one clip's frames never replace another's in the report. | | Corrupt config file | Treated as empty. Config must never stand between someone and their card. | | Two source files mapping to one destination path | Refused before a byte moves, naming both sources. See below. | | Metadata that will not parse | The file still copies and verifies; only its metadata is lost, and the job carries a warning. | -| A directory junction pointing at its own parent | Each directory is visited at most once, so the scan cannot loop. | +| A directory symlink or junction in the source | Never entered, and each one skipped is named in a warning. A junction pointing at its own parent cannot loop, and an alias listed before its target (`A_alias -> Z_real`) can no longer claim the real folder: the real folder is always scanned under its real name. Cloud-storage placeholder folders are reparse points too but are real folders, so only links (name-surrogate reparse points) are skipped. | | A manifest whose digests are uppercase | Compared case-insensitively for hex formats, exactly for C4. Another tool's casing is not corruption. | | A filename XML or UTF-8 cannot carry | Filtered to U+FFFD in manifests and CSV, so one bad name cannot strand a delivery's paperwork. | @@ -219,6 +281,11 @@ known. source twice" below. - **`skip_existing` compares size, not checksum.** It is a speed option, not a safety one, and should not be used on a tree whose integrity is in question. + Skipped files are left out of the rerun's manifest and the job reads + `Incomplete` (CLI exit `4`). An earlier classic manifest that covered them is + kept and still checked; an ASC MHL history checks only its newest + generation, which omits them, so its directory hashes fail `verify` after such + a rerun (a false alarm, in the safe direction). - **Concurrent instances.** One app instance serialises its queue. Two instances pointed at the same destination are not coordinated. - **Case-only filename collisions on a case-insensitive destination, off @@ -335,6 +402,14 @@ the limit. The prefix disables path normalisation, so paths are made absolute and normalised first, and UNC paths take the `UNC\server\share` form rather than a doubled prefix. +Short paths take the prefix too when a name in them is one Windows would +rewrite: a trailing dot or space, or a reserved device name such as `NUL`, +`CON` or `COM1`. A card written by macOS or Linux can hold `clip.mov.` beside +`clip.mov`. Without the prefix, opening the first opens the second, so the +wrong bytes were read and certified, and `NUL` read as an empty device. The +scan, every source and destination open, stat and rename, cache eviction and +`verify` now address such names literally, so each copies as itself. + **Caveat on the evidence.** The development machine has `LongPathsEnabled = 1` in the registry, so Windows accepts long paths there natively and *the original failure could not be reproduced on it* — a 315-character destination worked @@ -355,6 +430,12 @@ The tool cannot enforce this, but it is what the tool is built to support: Do not erase the card until step 3 has passed on more than one copy. +A script can key off `offload`'s exit status: `0` Verified (or Copied under +`--verify none`) with every report written, `1` a file or source folder failed, +`2` could not run, `3` destination refused, `4` nothing failed but not +everything was verified (an empty source, a `--skip-existing` rerun, a cancel), +`5` the copies verified but a report or manifest could not be written. + ## Sources - [ARRI — Data Transfer and File Handling](https://www.arri.com/en/learn-help/learn-help-camera-system/pre-postproduction/data-transfer) diff --git a/src/offloader/ascmhl.py b/src/offloader/ascmhl.py index cb1b25f..e421cde 100644 --- a/src/offloader/ascmhl.py +++ b/src/offloader/ascmhl.py @@ -23,7 +23,7 @@ from . import PRODUCT_NAME, __version__ from .hashers import ALGORITHMS, c4_of_bytes, get_algorithm, new_hasher -from .models import FileStatus, Job +from .models import Job from .util import xml_safe ASCMHL_DIRNAME = "ascmhl" @@ -183,13 +183,15 @@ def _default_ignores(job: Job, root: Path, report_dir: Path | None) -> list[str] it just wrote as a change to the tree. Two entries rather than one because they can differ: `--report-dir` moves - the PDF and CSV, while thumbnails always land in `_Reports`. A report + the PDF and CSV, while thumbnails always land in the job's own folder in + the first copy (`_Reports`, or `-2` and up when that name was taken + or is part of the data being copied, in which case it is data). A report directory outside this copy needs no pattern, and one that *is* the copy cannot have one — ignoring `.` would ignore everything. """ root = Path(root) patterns = [ASCMHL_DIRNAME] - for candidate in (report_dir, root / f"{job.name}_Reports"): + for candidate in (report_dir, job.report_dir_for(root)): if candidate is None: continue try: @@ -329,17 +331,20 @@ def write_manifest(job: Job, root: Path, *, destination_index: int = 0, tag = algorithm.mhl_tag or "xxh64" previous = _previous_hashes(root) - entries: list[tuple[Path, str, int, float, str]] = [] # rel, digest, size, mtime, action - - for entry in job.files: - if destination_index >= len(entry.destinations): - continue - destination = entry.destinations[destination_index] - digest = destination.checksum or entry.checksum - if not digest: - continue + # rel, digest ("" when none was taken), size, mtime, action + entries: list[tuple[Path, str, int | None, float | None, str]] = [] + + compare = algorithm.digests_match + # Every file the job scanned for this copy is recorded, failures included: + # a history that omits a failed copy reads as complete, and `verify` would + # pass a destination missing a clip. A failed copy keeps the source digest + # but is labelled `failed`, which verify will not accept as a reference; + # one with no digest at all is written as a bare path, which verify + # reports as having no checksum. Both fail. + for row in job.manifest_rows(destination_index): + digest = row.checksum or "" try: - relative = Path(destination.path).relative_to(root) + relative = Path(row.path).relative_to(root) except ValueError: continue # Filter here, where a filesystem name first enters the manifest, so @@ -349,14 +354,18 @@ def write_manifest(job: Job, root: Path, *, destination_index: int = 0, relative = Path(xml_safe(relative.as_posix())) key = relative.as_posix() - if destination.status is FileStatus.FAILED: + if row.failed or not digest: action = ACTION_FAILED elif key in previous: - action = (ACTION_VERIFIED if previous[key].get(tag) == digest + # Case-folded for hex, exact for C4: another tool's uppercase hex + # describes the same bytes, and calling it a mismatch would write + # a false failure permanently into the chain of custody. + earlier = previous[key].get(tag) + action = (ACTION_VERIFIED if earlier is not None and compare(earlier, digest) else ACTION_FAILED) else: action = ACTION_ORIGINAL - entries.append((relative, digest, entry.size, entry.modified, action)) + entries.append((relative, digest, row.size, row.modified, action)) manifest = ET.Element("hashlist", {"version": VERSION, "xmlns": NAMESPACE}) @@ -440,10 +449,16 @@ def _emit(parent: ET.Element, node: _Node, prefix: Path, relative = (Path(name) if prefix == Path(".") else prefix / name).as_posix() digest, size, modified, action = lookup[relative] element = ET.SubElement(parent, "hash") - ET.SubElement(element, "path", { - "size": str(size), - "lastmodificationdate": _iso(modified), - }).text = relative + attributes = {} + if size is not None: + attributes["size"] = str(size) + if modified is not None: + attributes["lastmodificationdate"] = _iso(modified) + ET.SubElement(element, "path", attributes).text = relative + if not digest: + # Nothing was ever hashed: a path with no hash is the record that + # this file belongs here and was not certified. + continue ET.SubElement(element, tag, { "action": action, "hashdate": _iso(moment), diff --git a/src/offloader/cli.py b/src/offloader/cli.py index b3c89c3..2eb5e2a 100644 --- a/src/offloader/cli.py +++ b/src/offloader/cli.py @@ -9,20 +9,23 @@ from . import PRODUCT_NAME, __version__, engine, hashers, longpath, probe, retry, thumbs from .models import FileStatus, Job, Profile, VerificationMode -from .reports import WRITERS +from .reports import WRITERS, paperwork from .util import format_elapsed, format_size #: Filenames written inside "_Reports/", matching the reference layout. -REPORT_FILENAMES = { - "pdf": "JobReport.pdf", - "csv": "JobReport.csv", - "mhl": "JobReport.mhl", - "ascmhl": "ascmhl", - "html": "JobReport.html", -} +REPORT_FILENAMES = paperwork.REPORT_FILENAMES DEFAULT_REPORTS = "pdf" +#: Exit codes for `offload`. Documented in the README; a format script keys +#: off them, so a new meaning gets a new number rather than reusing one. +EXIT_OK = 0 # Verified, or Copied under --verify none +EXIT_FAILED = 1 # a file or a source folder failed +EXIT_ERROR = 2 # could not run (bad source, unreadable input) +EXIT_REFUSED = 3 # unsafe destination layout, nothing copied +EXIT_INCOMPLETE = 4 # nothing failed, but not everything was verified +EXIT_REPORTS_FAILED = 5 # copies good, but a report or manifest failed + class _Progress: """Single-line progress on stderr; silent when not a TTY.""" @@ -69,53 +72,15 @@ def _parse_reports(value: str) -> list[str]: return keys -def _write_reports(job: Job, formats: list[str], out_dir: Path, +def _write_reports(job: Job, formats: list[str], out_dir: Path | None, logo: Path | None, footer: str | None = None) -> list[Path]: - out_dir.mkdir(parents=True, exist_ok=True) - written: list[Path] = [] - for key in formats: - target = out_dir / REPORT_FILENAMES[key] - try: - written.append(WRITERS[key](job, target, logo=logo, footer=footer)) - except Exception as exc: # a bad report must not mask a good offload - print(f" ! {key} report failed: {exc}", file=sys.stderr) - - if "mhl" in formats: - written.extend(_write_extra_manifests(job, out_dir)) - if "ascmhl" in formats: - written.extend(_write_extra_histories(job)) - return written - + """Every requested report, plus one manifest inside each destination. -def _write_extra_manifests(job: Job, primary_dir: Path) -> list[Path]: - """One MHL per destination, written beside that copy. - - A manifest that lives only with the first copy cannot re-verify the second. - Each copy needs its own chain of custody. + `out_dir` is `--report-dir`, or None for the job's own folder. Failures + land on `job.report_failures`, which the summary and exit code report. """ - written: list[Path] = [] - for index, root in enumerate(job.destination_roots[1:], start=1): - target = root / f"{job.name}_Reports" / REPORT_FILENAMES["mhl"] - if target.parent == primary_dir: - continue - try: - written.append(WRITERS["mhl"](job, target, destination_index=index)) - except Exception as exc: - print(f" ! MHL for {root} failed: {exc}", file=sys.stderr) - return written - - -def _write_extra_histories(job: Job) -> list[Path]: - """An ASC MHL history belongs at the root of every copy, not just the first.""" - from .ascmhl import write_manifest - - written: list[Path] = [] - for index, root in enumerate(job.destination_roots[1:], start=1): - try: - written.append(write_manifest(job, root, destination_index=index)) - except Exception as exc: - print(f" ! ASC MHL for {root} failed: {exc}", file=sys.stderr) - return written + return paperwork.write_reports(job, formats, report_dir=out_dir, + logo=logo, footer=footer) def _summarize(job: Job, reports: list[Path]) -> None: @@ -134,6 +99,17 @@ def _summarize(job: Job, reports: list[Path]) -> None: print(f" -> {destination}") for report in reports: print(f" report: {report}") + if job.report_failures: + print(file=sys.stderr) + print(f" {len(job.report_failures)} REPORT(S) FAILED:", file=sys.stderr) + for failure in job.report_failures: + print(f" ! {failure}", file=sys.stderr) + if job.scan_errors: + print(file=sys.stderr) + print(f" {len(job.scan_errors)} folder(s) could not be read, so " + "their files were not copied:", file=sys.stderr) + for problem in job.scan_errors: + print(f" {problem.describe()}", file=sys.stderr) if job.warnings: print(file=sys.stderr) print(f" {len(job.warnings)} warning(s):", file=sys.stderr) @@ -212,6 +188,10 @@ def build_parser() -> argparse.ArgumentParser: help="do not recreate the source folder structure") offload.add_argument("--skip-existing", action="store_true", help="skip files already present with a matching size") + offload.add_argument("--replace-existing", action="store_true", + help="let a copy replace a file already at the " + "destination whose content differs (by default " + "that file is kept and the copy is failed)") offload.add_argument("--paranoid", action="store_true", help="read each source file twice and compare, to " "catch a read that returned wrong bytes without " @@ -261,13 +241,22 @@ def _options_from(args: argparse.Namespace, destinations: list[Path]) -> engine. retry=retry.RetryPolicy(attempts=max(1, args.retries), delay=max(0.0, args.retry_wait)), paranoid=getattr(args, "paranoid", False), + replace_existing=getattr(args, "replace_existing", False), ) -def _report_dir(args: argparse.Namespace, job: Job, fallback_root: Path) -> Path: - if args.report_dir: - return args.report_dir - return fallback_root / f"{job.name}_Reports" +def exit_code(job: Job) -> int: + """The `offload` exit code for a finished job. See the EXIT_* constants.""" + status = job.copy_status + if status == "Failed": + return EXIT_FAILED + if status not in ("Verified", "Copied"): + # Incomplete (nothing to copy, or files skipped on size alone rather + # than verified) or Cancelled. Not a failure, and not a pass either. + return EXIT_INCOMPLETE + if job.report_failures: + return EXIT_REPORTS_FAILED + return EXIT_OK def cmd_offload(args: argparse.Namespace) -> int: @@ -279,12 +268,12 @@ def cmd_offload(args: argparse.Namespace) -> int: reports = _write_reports( job, args.report if isinstance(args.report, list) else _parse_reports(args.report), - _report_dir(args, job, job.destination_roots[0]), + args.report_dir, args.logo, args.footer, ) _summarize(job, reports) - return 1 if any(f.status is FileStatus.FAILED for f in job.files) else 0 + return exit_code(job) def cmd_report(args: argparse.Namespace) -> int: @@ -296,12 +285,12 @@ def cmd_report(args: argparse.Namespace) -> int: reports = _write_reports( job, args.report if isinstance(args.report, list) else _parse_reports(args.report), - _report_dir(args, job, (args.destinations or [args.source])[0]), + args.report_dir, args.logo, args.footer, ) _summarize(job, reports) - return 0 + return EXIT_REPORTS_FAILED if job.report_failures else 0 def cmd_verify(args: argparse.Namespace) -> int: diff --git a/src/offloader/engine.py b/src/offloader/engine.py index 52d76ee..5181b77 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -11,7 +11,7 @@ import fnmatch import os import queue -import shutil +import stat as stat_mod import threading import time from collections.abc import Callable, Iterable, Sequence @@ -30,6 +30,7 @@ Job, MediaInfo, Profile, + ScanProblem, VerificationMode, ) @@ -233,6 +234,13 @@ class OffloadOptions: #: pass over the card, and is the only thing that catches a read which #: returned wrong bytes without the operating system noticing. paranoid: bool = False + #: Let a finished copy replace a file already at the destination whose + #: content differs. Off by default: a second card with the same folder + #: layout would otherwise silently replace the first card's verified + #: copies, and the first card may already have been formatted on the + #: strength of them. An existing file with identical content is replaced + #: either way, since that changes nothing. + replace_existing: bool = False def __post_init__(self) -> None: # The data profile is defined by the absence of media work, so enforce @@ -256,48 +264,104 @@ def is_excluded(path: Path, patterns: Iterable[str]) -> bool: return any(fnmatch.fnmatch(name, pattern) for pattern in patterns) -def scan(root: Path, excludes: Iterable[str] = DEFAULT_EXCLUDES) -> list[Path]: +#: `FILE_ATTRIBUTE_REPARSE_POINT`, and the reparse-tag bit Microsoft sets on +#: tags that redirect to another name (symlinks, junctions, mount points). +_REPARSE_POINT = 0x400 +_NAME_SURROGATE = 0x20000000 + + +def _is_directory_link(path: str) -> bool: + """A symlink or junction: a directory entry that is really another name. + + Judged from `lstat`, so the link itself is examined rather than its + target. On Windows that means the reparse-point attribute, because + `os.path.islink` is False for a junction. Only name-surrogate reparse + points count: a cloud-storage placeholder is also a reparse point, but it + is the real folder, and skipping it would drop real data. + """ + try: + info = os.lstat(path) + except OSError: + # Not ours to judge: the walk will try to list it and report the + # failure through `onerror` if it cannot. + return False + attributes = getattr(info, "st_file_attributes", 0) + if attributes & _REPARSE_POINT: + tag = getattr(info, "st_reparse_tag", 0) + return bool(tag & _NAME_SURROGATE) if tag else True + return stat_mod.S_ISLNK(info.st_mode) + + +def scan(root: Path, excludes: Iterable[str] = DEFAULT_EXCLUDES, *, + errors: list[ScanProblem] | None = None, + links: list[Path] | None = None) -> list[Path]: """Every file under `root`, sorted, with junk filtered out. - Directories are visited at most once each. A Windows junction pointing at - its own parent otherwise walks forever, and `Path.is_symlink()` is False - for a junction, so the usual symlink guard does not see it. Termination is - left to chance without this: today it only stops because Windows refuses - paths past MAX_PATH, and it stops having already returned the same file - dozens of times. - - Only directories the walk will actually enter are recorded. `os.walk` - does not follow a directory symlink, so recording one would mark its - target as seen without anything having scanned it: an alias `a -> z` - listed before `z` would then hide `z` itself, and neither path's files - would be found. A junction is not a symlink to `os.walk`, which does enter - it, so a junction still goes through the guard. + A directory that cannot be listed is appended to `errors` rather than + dropped. `os.walk` swallows listing errors by default, and the files under + such a directory would then be absent from the job, its byte total and its + manifests, with the job reading Verified. A caller that passes no list + gets the old silent behaviour, so `run()` always passes one. + + Directory symlinks and junctions are never entered, and each one skipped + is appended to `links` so the caller can say so. Following them is how a + junction pointing at its own parent walks forever, and how an alias + listed before its target (`A_alias -> Z_real`) used to claim the real + directory under the alias name. Not entering any of them means the real + directory is always scanned under its real name. The root is entered even + when it is itself a link: that is the folder the operator chose. + + The walk runs on the extended-length form of the root (see `longpath`), + so a directory whose name Win32 would trim, or one past MAX_PATH, is + listed as itself. The paths returned are in the form `root` was given in. """ patterns = tuple(excludes) found: list[Path] = [] visited: set[str] = set() - - def already_seen(directory: Path) -> bool: - if os.path.islink(directory): - # Not followed by os.walk, so not a visit. See the docstring. - return False + root = Path(root) + walk_root = longpath.literal(root) + + def plain(walked: str) -> Path: + # os.walk joins names onto the top it was given, so every path it + # reports starts with `walk_root`. + if walked.startswith(walk_root): + tail = walked[len(walk_root):].lstrip("\\/") + return root / tail if tail else root + return Path(longpath.shorten_for_display(walked)) + + def already_seen(walked: str) -> bool: try: - real = os.path.normcase(os.path.realpath(directory)) - except OSError: # pragma: no cover - unreadable entry - return True + real = os.path.normcase(os.path.realpath(walked)) + except OSError: + # Enter it: if it really cannot be read, onerror records that. + return False if real in visited: return True visited.add(real) return False - # The root is always entered, even when it is itself a symlink. - visited.add(os.path.normcase(os.path.realpath(root))) - for dirpath, dirnames, filenames in os.walk(root): - here = Path(dirpath) - dirnames[:] = sorted( - d for d in dirnames - if not is_excluded(here / d, patterns) and not already_seen(here / d) - ) + def onerror(exc: OSError) -> None: + if errors is None: + return + where = plain(str(exc.filename)) if exc.filename else root + errors.append(ScanProblem(where, exc.strerror or str(exc))) + + visited.add(os.path.normcase(os.path.realpath(walk_root))) + for dirpath, dirnames, filenames in os.walk(walk_root, onerror=onerror): + here = plain(dirpath) + keep: list[str] = [] + for name in sorted(dirnames): + if is_excluded(here / name, patterns): + continue + walked = os.path.join(dirpath, name) + if _is_directory_link(walked): + if links is not None: + links.append(here / name) + continue + if already_seen(walked): + continue + keep.append(name) + dirnames[:] = keep for filename in sorted(filenames): candidate = here / filename if not is_excluded(candidate, patterns): @@ -370,7 +434,10 @@ def _copy_fanout(source: Path, targets: Sequence[Path], algorithm: str, # target that *is* the source would destroy the original before a byte # of it was read. assert_safe_destinations should have caught this long # before now; refuse anyway rather than trust that it did. - if Path(target).resolve() == source.resolve(): + # Resolved in extended form, so a name Win32 would rewrite is compared + # as itself and not as whatever its trimmed namesake resolves to. + if (Path(longpath.extended(target)).resolve() + == Path(longpath.extended(source)).resolve()): raise UnsafeDestination( f"refusing to write {target}: it is the source file") longpath.makedirs(target.parent) @@ -629,10 +696,12 @@ def run(source_root: Path, options: OffloadOptions, raise ValueError("at least one destination is required") assert_safe_destinations(source_root, dest_roots) - files = scan(source_root, options.excludes) + scan_errors: list[ScanProblem] = [] + links: list[Path] = [] + files = scan(source_root, options.excludes, errors=scan_errors, links=links) assert_no_destination_collisions(files, source_root, dest_roots, options.preserve_structure) - counters = _Counters(job_bytes_total=sum(p.stat().st_size for p in files)) + counters = _Counters(job_bytes_total=sum(_size_or_zero(p) for p in files)) host = sysinfo.collect() job = Job( @@ -650,9 +719,27 @@ def run(source_root: Path, options: OffloadOptions, processors=host.processors, system_ram=host.system_ram, paranoid=options.paranoid, + scan_errors=scan_errors, + preserve_structure=options.preserve_structure, ) - thumb_dir = options.thumbnail_dir or (dest_roots[0] / f"{job.name}_Reports" / "thumbs") + for problem in scan_errors: + counters.errors.append(problem.describe()) + for link in links: + job.warnings.append( + f"{link} is a symbolic link or junction and was not followed, so " + "nothing under it was copied; offload its target directly if it " + "is needed") + + job.report_dir_name = choose_report_folder( + job.name, files, source_root, dest_roots, options.preserve_structure) + if job.report_dir_name != f"{job.name}_Reports": + job.warnings.append( + f"{job.name}_Reports already exists at a destination or is part of " + f"the source, so this job's reports are in {job.report_dir_name} " + "instead; nothing already there was overwritten") + + thumb_dir = options.thumbnail_dir or (job.report_dir_for(dest_roots[0]) / "thumbs") # A sidecar belongs to a *clip*. Under the data profile nothing is a clip, # so stem-matching a dataset would announce that `run_1440.xmp` belongs to @@ -685,7 +772,32 @@ def emit(event: ProgressEvent) -> None: job.cancelled = True break - stat = source.stat() + targets = [ + _destination_for(source, source_root, root, options.preserve_structure) + for root in dest_roots + ] + + try: + stat = longpath.stat(source) + except OSError as exc: + # Deleted, renamed or dropped by the card since the scan. One + # file's disappearance costs that file, recorded as such, not the + # rest of the card and the paperwork for what did copy. + reason = ("source vanished after scan" + if isinstance(exc, FileNotFoundError) + else f"source could not be read: {exc}") + entry = FileEntry(source=source, source_root=source_root, size=0, + created=0.0, modified=0.0, + companion_of=belongs_to.get(source), + companions=owns.get(source, [])) + counters.errors.append(f"{source}: {reason}") + for root, target in zip(dest_roots, targets, strict=True): + entry.destinations.append( + Destination(root=root, path=target, + status=FileStatus.FAILED, error=reason)) + job.files.append(entry) + continue + entry = FileEntry( source=source, source_root=source_root, @@ -696,17 +808,12 @@ def emit(event: ProgressEvent) -> None: companions=owns.get(source, []), ) - targets = [ - _destination_for(source, source_root, root, options.preserve_structure) - for root in dest_roots - ] - emit(ProgressEvent(index, len(files), source.name, "copy", 0, stat.st_size, counters.job_bytes_done, counters.job_bytes_total)) if options.skip_existing and all( - t.exists() and t.stat().st_size == stat.st_size for t in targets + _size_or_none(t) == stat.st_size for t in targets ): entry.checksum = None for root, target in zip(dest_roots, targets, strict=True): @@ -832,7 +939,7 @@ def copy_once(_src=source, _partials=partials, _idx=index, # Mirror source timestamps so the destination reads as an archival # copy, not a fresh file. try: - shutil.copystat(source, partial) + longpath.copystat(source, partial) except OSError: pass @@ -885,7 +992,7 @@ def copy_once(_src=source, _partials=partials, _idx=index, entry.destinations.append(destination) continue - size_ok = partial.exists() and partial.stat().st_size == stat.st_size + size_ok = _size_or_none(partial) == stat.st_size sum_ok = (src_sum == dst_sum) if src_sum else True if size_ok and sum_ok: destination.status = FileStatus.VERIFIED @@ -900,6 +1007,26 @@ def copy_once(_src=source, _partials=partials, _idx=index, entry.destinations.append(destination) continue + if not options.replace_existing and longpath.exists(target): + # Staging protects an existing file from a copy that fails. This + # protects it from one that succeeds: a different card with the + # same folder layout would otherwise replace the first card's + # verified copy, and that card may already be formatted. + try: + refusal = _existing_file_refusal( + target, partial, src_sum, options.algorithm, control) + except JobCancelled: + _discard(partials) + cancelled_in_verify = True + break + if refusal is not None: + destination.status = FileStatus.FAILED + destination.error = refusal + counters.errors.append(f"{target}: {refusal}") + _discard([partial]) + entry.destinations.append(destination) + continue + try: longpath.replace(partial, target) # atomic within a filesystem except OSError as exc: @@ -911,7 +1038,7 @@ def copy_once(_src=source, _partials=partials, _idx=index, continue try: - dst_stat = target.stat() + dst_stat = longpath.stat(target) destination.created = getattr(dst_stat, "st_birthtime", dst_stat.st_ctime) destination.modified = dst_stat.st_mtime except OSError: @@ -973,13 +1100,17 @@ def copy_once(_src=source, _partials=partials, _idx=index, entry.thumbnails = thumbs.extract( picture, entry.media, thumb_dir, options.thumbnail_count, - memo=thumb_memo, + memo=thumb_memo, key=entry.relative.as_posix(), ) job.files.append(entry) job.finished = _dt.datetime.now() if job.cancelled: + attempted = {entry.source for entry in job.files} + # Kept so the manifests can list them unhashed: a cancelled job's + # manifest must not read as covering the whole card. + job.not_attempted = [path for path in files if path not in attempted] not_attempted = len(files) - len(job.files) if not_attempted > 0: counters.errors.append( @@ -989,6 +1120,94 @@ def copy_once(_src=source, _partials=partials, _idx=index, return job +def _size_or_none(path: Path) -> int | None: + """The size of whatever is at `path`, or None when nothing readable is.""" + try: + return longpath.stat(path).st_size + except (OSError, ValueError): + return None + + +def _size_or_zero(path: Path) -> int: + """For the up-front byte total only. A file that cannot be sized here is + failed, with its reason, when the copy loop reaches it.""" + return _size_or_none(path) or 0 + + +def choose_report_folder(name: str, files: Sequence[Path], source_root: Path, + dest_roots: Sequence[Path], preserve_structure: bool) -> str: + """`_Reports`, or `_Reports-2` and up when that is taken. + + Taken means it already exists in any destination root, or something being + copied lands on it. Re-offloading a previous destination (a shuttle drive + holding `A001/A001_Reports/`) copies that folder, and writing new reports + over it would replace files this job had just certified, the shuttle's own + manifest among them. An existing folder from an earlier run is left alone + for the same reason: its manifest covers files a rerun may skip. + """ + claimed: set[str] = set() + for source in files: + if preserve_structure: + try: + first = source.relative_to(source_root).parts[0] + except (ValueError, IndexError): + first = source.name + else: + first = source.name + claimed.add(os.path.normcase(first)) + + base = f"{name}_Reports" + candidate, number = base, 1 + while (os.path.normcase(candidate) in claimed + or any(longpath.exists(Path(root) / candidate) for root in dest_roots)): + number += 1 + candidate = f"{base}-{number}" + return candidate + + +def _existing_file_refusal(target: Path, partial: Path, source_checksum: str, + algorithm: str, + control: JobControl | None) -> str | None: + """Why the finished copy at `partial` must not replace `target`, or None. + + Identical content may be replaced: that changes nothing on disk, and it is + what a re-offload of the same card does. Anything else, or anything that + cannot be read to find out, is refused. + """ + advice = "use --replace-existing to overwrite it" + try: + if longpath.stat(target).st_size != longpath.stat(partial).st_size: + return ("an existing file with different content is already there; " + + advice) + checker = get_algorithm(algorithm) + checkpoint = control.checkpoint if control else None + if source_checksum and checker.factory is not None: + existing = hash_file(target, algorithm, checkpoint=checkpoint) + same = checker.digests_match(existing, source_checksum) + else: + same = _same_bytes(target, partial, checkpoint) + except OSError as exc: + return (f"an existing file is already there and could not be read to " + f"compare ({exc}); {advice}") + if same: + return None + return "an existing file with different content is already there; " + advice + + +def _same_bytes(left: Path, right: Path, + checkpoint: Callable[[], None] | None) -> bool: + """Byte-for-byte comparison, for when there is no checksum to compare.""" + with longpath.open_binary(left, "rb") as one, longpath.open_binary(right, "rb") as two: + while True: + if checkpoint is not None: + checkpoint() + a, b = one.read(CHUNK_SIZE), two.read(CHUNK_SIZE) + if a != b: + return False + if not a: + return True + + def rescan(source_root: Path, destination_roots: Sequence[Path], options: OffloadOptions, progress: ProgressCallback | None = None) -> Job: @@ -998,7 +1217,9 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], a report can be regenerated (or a delivery audited) after the fact. """ source_root = Path(source_root) - files = scan(source_root, options.excludes) + scan_errors: list[ScanProblem] = [] + links: list[Path] = [] + files = scan(source_root, options.excludes, errors=scan_errors, links=links) algorithm = get_algorithm(options.algorithm) host = sysinfo.collect() @@ -1016,10 +1237,15 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], os_version=host.os_version, processors=host.processors, system_ram=host.system_ram, + scan_errors=scan_errors, + preserve_structure=options.preserve_structure, ) + job.warnings.extend( + f"{link} is a symbolic link or junction and was not followed" + for link in links) thumb_dir = options.thumbnail_dir or (source_root / f"{job.name}_Reports" / "thumbs") thumb_memo = thumbs.DecoderMemo() - total = sum(p.stat().st_size for p in files) + total = sum(_size_or_zero(p) for p in files) done = 0 # A sidecar belongs to a *clip*. Under the data profile nothing is a clip, @@ -1030,7 +1256,7 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], owns = _invert_companions(belongs_to) for index, source in enumerate(files): - stat = source.stat() + stat = longpath.stat(source) entry = FileEntry( source=source, source_root=source_root, @@ -1052,7 +1278,7 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], for root in job.destination_roots: target = _destination_for(source, source_root, root, options.preserve_structure) - exists = target.exists() + exists = longpath.exists(target) entry.destinations.append( Destination( root=root, @@ -1072,7 +1298,7 @@ def rescan(source_root: Path, destination_roots: Sequence[Path], if options.thumbnail_count > 0 and entry.media.is_video: entry.thumbnails = thumbs.extract( source, entry.media, thumb_dir, options.thumbnail_count, - memo=thumb_memo, + memo=thumb_memo, key=entry.relative.as_posix(), ) except Exception as exc: # noqa: BLE001 entry.media = MediaInfo() diff --git a/src/offloader/gui/main_window.py b/src/offloader/gui/main_window.py index 8eab0ff..98abfb8 100644 --- a/src/offloader/gui/main_window.py +++ b/src/offloader/gui/main_window.py @@ -330,6 +330,19 @@ def _on_job_finished(self, identifier: int) -> None: self.statusBar().showMessage(f"“{item.name}” failed", 10000) elif item.state is JobState.CANCELLED: self.statusBar().showMessage(f"“{item.name}” cancelled", 6000) + elif item.job is not None and item.job.report_failures: + # The copies are fine, but a missing manifest leaves a later + # verify with nothing to check against. Never a quiet success. + failures = item.job.report_failures + body = "\n".join(f"• {failure}" for failure in failures[:12]) + if len(failures) > 12: + body += f"\n… and {len(failures) - 12} more" + QMessageBox.warning( + self, "Offload finished, but reports failed", + f"“{item.name}” copied, but {len(failures)} report(s) could " + f"not be written:\n\n{body}") + self.statusBar().showMessage( + f"“{item.name}” {item.job.final_status.lower()}", 10000) elif item.job is not None and item.job.warnings: shown = item.job.warnings[:12] body = "\n".join(f"• {warning}" for warning in shown) diff --git a/src/offloader/gui/worker.py b/src/offloader/gui/worker.py index 770b3e4..f67adcd 100644 --- a/src/offloader/gui/worker.py +++ b/src/offloader/gui/worker.py @@ -18,20 +18,14 @@ from .. import engine, history, naming from ..models import Job from ..presets import Preset -from ..reports import WRITERS +from ..reports import paperwork from ..volumes import volume_label #: Trailing window over which throughput is measured. Long enough to smooth #: per-chunk jitter, short enough that a stall shows up within seconds. RATE_WINDOW_SEC = 5.0 -REPORT_FILENAMES = { - "pdf": "JobReport.pdf", - "csv": "JobReport.csv", - "mhl": "JobReport.mhl", - "ascmhl": "ascmhl", - "html": "JobReport.html", -} +REPORT_FILENAMES = paperwork.REPORT_FILENAMES class JobState(str, Enum): @@ -166,41 +160,12 @@ def run(self) -> None: # noqa: D102 - QThread entry point def _write_reports(self, job: Job, preset: Preset) -> list[Path]: if job.cancelled or not preset.reports or not job.destination_roots: return [] - out_dir = job.destination_roots[0] / f"{job.name}_Reports" - written: list[Path] = [] - for key in preset.reports: - writer = WRITERS.get(key) - if writer is None: - continue - try: - written.append(writer(job, out_dir / REPORT_FILENAMES[key], - logo=preset.logo, footer=preset.footer)) - except Exception: - # A report that fails to render must not invalidate a good copy. - continue - - if "ascmhl" in preset.reports: - from ..ascmhl import write_manifest - for index, root in enumerate(job.destination_roots[1:], start=1): - try: - written.append(write_manifest(job, root, - destination_index=index)) - except Exception: - continue - - if "mhl" in preset.reports: - # Each copy needs its own manifest, or the second destination has - # nothing to re-verify itself against. - for index, root in enumerate(job.destination_roots[1:], start=1): - target = root / f"{job.name}_Reports" / REPORT_FILENAMES["mhl"] - if target.parent == out_dir: - continue - try: - written.append(WRITERS["mhl"](job, target, - destination_index=index)) - except Exception: - continue - return written + # The shared writer: one manifest inside every destination, never + # over an existing one, and every failure recorded on the job so the + # queue row and the completion summary show it. A report that fails + # to render still never changes a file's status. + return paperwork.write_reports(job, list(preset.reports), + logo=preset.logo, footer=preset.footer) class QueueController(QObject): diff --git a/src/offloader/integrity.py b/src/offloader/integrity.py index 1323d95..fd6da45 100644 --- a/src/offloader/integrity.py +++ b/src/offloader/integrity.py @@ -22,6 +22,8 @@ import platform from pathlib import Path +from . import longpath + _WINDOWS = platform.system() == "Windows" # CreateFileW constants @@ -46,7 +48,9 @@ def _evict_windows(path: Path) -> bool: kernel32.CloseHandle.argtypes = [wintypes.HANDLE] handle = kernel32.CreateFileW( - str(path), _GENERIC_READ, _FILE_SHARE_READ | _FILE_SHARE_WRITE, + # Through longpath, or a name Win32 rewrites (a trailing dot, `NUL`) + # evicts some other file, or none. + longpath.extended(path), _GENERIC_READ, _FILE_SHARE_READ | _FILE_SHARE_WRITE, None, _OPEN_EXISTING, _FILE_FLAG_NO_BUFFERING, None, ) if not handle or handle == _INVALID_HANDLE or handle == 2 ** 64 - 1: diff --git a/src/offloader/longpath.py b/src/offloader/longpath.py index 43476de..67d834d 100644 --- a/src/offloader/longpath.py +++ b/src/offloader/longpath.py @@ -10,9 +10,17 @@ must already be absolute, backslash-separated and free of `.` and `..` — which is why it is applied through these helpers rather than sprinkled at call sites. -It is applied only when a path is close to the limit. Below that the ordinary -form is used, so the common case behaves exactly as it always did and the -extended form is not a new variable in every code path. +It is applied only when a path is close to the limit, or when a name in it is +one Win32 would rewrite. Otherwise the ordinary form is used, so the common +case behaves exactly as it always did and the extended form is not a new +variable in every code path. + +The second case is a correctness matter, not a length one. Win32 silently +trims a trailing dot or space from a name and maps reserved device names +(`NUL`, `CON`, `COM1`...) to devices. A card written by macOS or Linux can hold +`clip.mov.` beside `clip.mov`, and opening the first without the prefix opens +the second: the wrong bytes are read and certified. `NUL` reads as an empty +device. With the prefix every name reaches the filesystem exactly as listed. """ from __future__ import annotations @@ -32,22 +40,48 @@ _UNC_PREFIX = "\\\\?\\UNC\\" -def needs_extended(path: os.PathLike | str) -> bool: - return _WINDOWS and len(str(path)) >= THRESHOLD +#: Names Win32 maps to a device rather than a file, matched on the part before +#: the first dot (`NUL.txt` is the device too on older Windows). +_RESERVED = frozenset( + ["CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$"] + + [f"{base}{suffix}" for base in ("COM", "LPT") + for suffix in "0123456789\u00b9\u00b2\u00b3"] +) -def extended(path: os.PathLike | str) -> str: - """The form of `path` that Windows will accept at any length. +def _rewritten_by_win32(name: str) -> bool: + """Whether Win32 would open something other than `name` without the prefix.""" + if name in ("", ".", "..") or name.endswith(":"): + return False + if name.endswith((".", " ")): + return True + return name.split(".", 1)[0].rstrip(" ").upper() in _RESERVED - A no-op off Windows, on short paths, and on paths already prefixed. - """ + +def needs_literal(path: os.PathLike | str) -> bool: + """A name in `path` that Win32 would trim or turn into a device.""" + if not _WINDOWS: + return False text = str(path) - if not _WINDOWS or text.startswith(_PREFIX) or not needs_extended(text): - return text + if text.startswith(_PREFIX): + return False + return any(_rewritten_by_win32(part) for part in text.replace("/", "\\").split("\\")) + +def needs_extended(path: os.PathLike | str) -> bool: + return _WINDOWS and (len(str(path)) >= THRESHOLD or needs_literal(path)) + + +def _prefixed(text: str) -> str: # The prefix disables normalisation, so normalise first or `..` and `/` - # would be passed to the filesystem verbatim. - absolute = os.path.abspath(text) + # would be passed to the filesystem verbatim. Lexically, though: + # `abspath` goes through GetFullPathName, which applies exactly the Win32 + # rewriting (trailing dots and spaces, device names) the prefix is here to + # avoid. + if needs_literal(text): + absolute = os.path.normpath(os.path.join(os.getcwd(), text)) + else: + absolute = os.path.abspath(text) if absolute.startswith(_PREFIX): return absolute if absolute.startswith("\\\\"): # UNC: \\server\share -> \\?\UNC\server\share @@ -55,6 +89,31 @@ def extended(path: os.PathLike | str) -> str: return _PREFIX + absolute +def extended(path: os.PathLike | str) -> str: + """The form of `path` that Windows will accept at any length and with + any name. + + A no-op off Windows, on short ordinary paths, and on paths already + prefixed. + """ + text = str(path) + if not _WINDOWS or text.startswith(_PREFIX) or not needs_extended(text): + return text + return _prefixed(text) + + +def literal(path: os.PathLike | str) -> str: + """The prefixed form of `path` on Windows, whatever its length. + + For a directory walk: the names *below* the root are not known until they + are listed, and one of them may need the prefix to be listed in turn. + """ + text = str(path) + if not _WINDOWS or text.startswith(_PREFIX): + return text + return _prefixed(text) + + def open_binary(path: os.PathLike | str, mode: str = "rb") -> BinaryIO: """`open` that works at any path length. @@ -83,6 +142,16 @@ def stat(path: os.PathLike | str) -> os.stat_result: return os.stat(extended(path)) +def lstat(path: os.PathLike | str) -> os.stat_result: + return os.lstat(extended(path)) + + +def copystat(source: os.PathLike | str, target: os.PathLike | str) -> None: + import shutil + + shutil.copystat(extended(source), extended(target)) + + def exists(path: os.PathLike | str) -> bool: try: os.stat(extended(path)) diff --git a/src/offloader/models.py b/src/offloader/models.py index 0c9da07..30fc1d9 100644 --- a/src/offloader/models.py +++ b/src/offloader/models.py @@ -206,6 +206,32 @@ def is_video(self) -> bool: return self.media.is_video +@dataclass +class ScanProblem: + """A directory under the source that the scan could not list.""" + + path: Path + error: str + + def describe(self) -> str: + return f"could not list {self.path}: {self.error}" + + +@dataclass +class ManifestRow: + """One line of a manifest for one destination. See `Job.manifest_rows`.""" + + #: Where the file is, or should be, at that destination. + path: Path + size: int | None + modified: float | None + #: The checksum to verify against. None means there is nothing to verify + #: against, which a verifier must treat as a failure, never as a pass. + checksum: str | None + #: This destination did not receive a good copy. + failed: bool + + @dataclass class Job: """A complete offload, ready to render.""" @@ -234,6 +260,113 @@ class Job: #: erasing a card — empty files, verifications that may have been served #: from cache. warnings: list[str] = field(default_factory=list) + #: Directories under the source that could not be listed. Every file in + #: one is unknown, so the job cannot be Verified: it reads Failed, and the + #: manifests carry an unhashed entry for each so `verify` fails as well. + scan_errors: list[ScanProblem] = field(default_factory=list) + #: Source files the scan found but the job never reached, because it was + #: cancelled first. Recorded so the manifests can list them unhashed + #: instead of reading as complete. + not_attempted: list[Path] = field(default_factory=list) + #: Whether the source tree was recreated under each destination root. + #: Needed to say where a file that never arrived *would* have been. + preserve_structure: bool = True + #: The folder, inside each destination root, that holds this job's + #: paperwork. Empty means the conventional `_Reports`; the engine + #: picks `_Reports-2` and up when that one already exists or is + #: part of what is being copied. + report_dir_name: str = "" + #: Every report or manifest that could not be written, one line each. + #: Never changes a file's status, since the copies are what they are, but + #: it keeps the job from reading as a plain "Verified": a later `verify` + #: depends on the manifest that is missing. + report_failures: list[str] = field(default_factory=list) + + @property + def reports_folder(self) -> str: + """The paperwork folder's name inside a destination root.""" + return self.report_dir_name or f"{self.name}_Reports" + + def report_dir_for(self, root: Path) -> Path: + return Path(root) / self.reports_folder + + def manifest_rows(self, destination_index: int = 0) -> list[ManifestRow]: + """What a manifest for one destination has to say, file by file. + + Every scanned file the job touched is listed for that destination, + not only the ones that arrived: a manifest that leaves out a failed + copy reads as complete, and `verify` then passes a destination that + is missing a clip. A failed copy is listed with the *source* checksum + when one was taken (so verify finds it missing or different), and + unhashed otherwise (so verify reports it as having no checksum, which + fails). Files the job never reached and directories it could not list + are listed unhashed for the same reason. + + Skipped files are left out. They were not checksummed this run, so a + manifest entry would certify a file nobody read; an earlier manifest + that did read them is never overwritten. + """ + rows: list[ManifestRow] = [] + roots = self.destination_roots + root = roots[destination_index] if destination_index < len(roots) else None + + def expected_path(relative: Path, name: str) -> Path | None: + if root is None: + return None + return root / relative if self.preserve_structure else root / name + + for entry in self.files: + if destination_index < len(entry.destinations): + destination = entry.destinations[destination_index] + if destination.status is FileStatus.SKIPPED: + continue + if destination.status in (FileStatus.FAILED, FileStatus.CANCELLED): + # Never the destination's own checksum here: for a failed + # verification that is the hash of the bad bytes. + rows.append(ManifestRow(destination.path, entry.size, + entry.modified, entry.checksum, True)) + else: + rows.append(ManifestRow(destination.path, entry.size, + entry.modified, + destination.checksum or entry.checksum, + False)) + continue + if not entry.destinations or root is None: + # A job assembled without destination records (a library + # caller, a test) describes the source itself, as it always has. + if entry.checksum: + rows.append(ManifestRow(entry.source, entry.size, + entry.modified, entry.checksum, False)) + continue + # Records for other destinations but not this one: a cancel landed + # before it was reached, so the file is not there. + first = entry.destinations[0] + try: + relative = Path(first.path).relative_to(first.root) + except ValueError: + relative = entry.relative + rows.append(ManifestRow(root / relative, entry.size, entry.modified, + entry.checksum, True)) + + for source in self.not_attempted: + try: + relative = source.relative_to(self.source_root) + except ValueError: + relative = Path(source.name) + path = expected_path(relative, source.name) + if path is not None: + rows.append(ManifestRow(path, None, None, None, True)) + + for problem in self.scan_errors: + try: + relative = problem.path.relative_to(self.source_root) + except ValueError: + relative = Path(problem.path.name) + # Recorded under the destination layout even when flattening, as a + # marker: nothing from inside it reached any destination. + if root is not None: + rows.append(ManifestRow(root / relative, None, None, None, True)) + return rows @property def total_bytes(self) -> int: @@ -254,8 +387,9 @@ def elapsed_sec(self) -> float: return (self.finished - self.started).total_seconds() @property - def final_status(self) -> str: - if any(f.status is FileStatus.FAILED for f in self.files): + def copy_status(self) -> str: + """The verdict on the copies alone, before any paperwork.""" + if self.scan_errors or any(f.status is FileStatus.FAILED for f in self.files): return "Failed" if self.cancelled: return "Cancelled" @@ -265,6 +399,15 @@ def final_status(self) -> str: return "Verified" return "Incomplete" + @property + def final_status(self) -> str: + status = self.copy_status + if self.report_failures and status in ("Verified", "Copied"): + # The copies are good, but the manifest a later `verify` needs may + # be the thing that is missing. Not a plain success. + return f"{status}, reports failed" + return status + @property def verification_label(self) -> str: """"XXHash3-64 Checksum(source only)" — matches the reference header.""" diff --git a/src/offloader/reports/mhl.py b/src/offloader/reports/mhl.py index 078f746..c3106e6 100644 --- a/src/offloader/reports/mhl.py +++ b/src/offloader/reports/mhl.py @@ -43,14 +43,38 @@ def _relative(target: Path, base: Path) -> str: typically one level up. `Path.relative_to` cannot express that and would silently fall back to an absolute path, which pins the manifest to one machine and one drive letter and stops it travelling with the media. + + Computed lexically when both paths are absolute. `os.path.relpath` goes + through `abspath`, which on Windows trims a trailing dot or space from a + name: `clip.mov.` would be recorded as `clip.mov`, and verify would then + check the wrong file. """ try: - return Path(os.path.relpath(target, base)).as_posix() + return Path(_lexical_relpath(str(target), str(base))).as_posix() except ValueError: # Different drives on Windows: no relative path exists. return Path(target).as_posix() +def _lexical_relpath(target: str, base: str) -> str: + if not (os.path.isabs(target) and os.path.isabs(base)): + return os.path.relpath(target, base) + target, base = os.path.normpath(target), os.path.normpath(base) + target_drive, target_rest = os.path.splitdrive(target) + base_drive, base_rest = os.path.splitdrive(base) + if os.path.normcase(target_drive) != os.path.normcase(base_drive): + raise ValueError("paths are on different drives") + target_parts = [part for part in target_rest.split(os.sep) if part] + base_parts = [part for part in base_rest.split(os.sep) if part] + common = 0 + while (common < min(len(target_parts), len(base_parts)) + and os.path.normcase(target_parts[common]) + == os.path.normcase(base_parts[common])): + common += 1 + parts = [os.pardir] * (len(base_parts) - common) + target_parts[common:] + return os.path.join(*parts) if parts else os.curdir + + def write_mhl(job: Job, path: Path, *, destination_index: int = 0, **_options) -> Path: """Write an MHL describing one destination copy of the job. @@ -73,27 +97,45 @@ def write_mhl(job: Job, path: Path, *, destination_index: int = 0, **_options) - ET.SubElement(info, "startdate").text = _iso(job.started) ET.SubElement(info, "finishdate").text = _iso(job.finished or job.started) - for entry in job.files: - if destination_index < len(entry.destinations): - destination = entry.destinations[destination_index] - file_path, checksum = destination.path, destination.checksum or entry.checksum - else: - file_path, checksum = entry.source, entry.checksum - if not checksum: - continue - + # Every file the job scanned for this destination is listed, including the + # ones that failed: a manifest that omits a failed copy reads as complete, + # and `verify` would then pass a destination that is missing a clip. An + # entry with no checksum element is how this file says "nothing to check + # against", and `verify` fails it rather than counting it as a match. + for row in job.manifest_rows(destination_index): node = ET.SubElement(root, "hash") - ET.SubElement(node, "file").text = _xml_safe(_relative(file_path, base)) - ET.SubElement(node, "size").text = str(entry.size) - ET.SubElement(node, "lastmodificationdate").text = _iso(entry.modified) - ET.SubElement(node, tag).text = _xml_safe(checksum) - ET.SubElement(node, "hashdate").text = _iso(job.finished or job.started) + ET.SubElement(node, "file").text = _xml_safe(_relative(row.path, base)) + if row.size is not None: + ET.SubElement(node, "size").text = str(row.size) + if row.modified is not None: + ET.SubElement(node, "lastmodificationdate").text = _iso(row.modified) + if row.checksum: + ET.SubElement(node, tag).text = _xml_safe(row.checksum) + ET.SubElement(node, "hashdate").text = _iso(job.finished or job.started) ET.indent(root, space=" ") ET.ElementTree(root).write(path, encoding="UTF-8", xml_declaration=True) return path +def next_free_path(path: Path) -> Path: + """`path`, or `JobReport-2.mhl`, `-3` and so on: the first that is free. + + A manifest is the record of what one run checked. Overwriting it with a + later run's, which may cover fewer files (a `--skip-existing` rerun lists + only what it copied), would discard the only evidence for the rest. + """ + path = Path(path) + if not path.exists(): + return path + number = 2 + while True: + candidate = path.with_name(f"{path.stem}-{number}{path.suffix}") + if not candidate.exists(): + return candidate + number += 1 + + def _resolve_tag(hash_label: str) -> str: """Map a report-facing algorithm name to its MHL element name.""" from ..hashers import ALGORITHMS diff --git a/src/offloader/reports/paperwork.py b/src/offloader/reports/paperwork.py new file mode 100644 index 0000000..15a8722 --- /dev/null +++ b/src/offloader/reports/paperwork.py @@ -0,0 +1,120 @@ +"""Writing a finished job's reports and manifests, for the CLI and the GUI. + +One implementation, so the two front ends cannot drift apart on the rules that +decide whether a later `verify` can be trusted: + +* Every destination gets its own MHL, inside that destination, so each copy + can be verified on its own. `--report-dir` moves the rest of the paperwork, + and holds a manifest only for the destination it sits inside, if any. +* An existing manifest is never overwritten. A rerun's manifest is numbered + (`JobReport-2.mhl`), because the earlier one may be the only record of + files the rerun skipped. +* Every writer failure is recorded on the job (`Job.report_failures`), which + keeps its final status from reading as a plain success. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +from ..models import Job +from .mhl import next_free_path + +#: Filenames written inside "_Reports/", matching the reference layout. +REPORT_FILENAMES = { + "pdf": "JobReport.pdf", + "csv": "JobReport.csv", + "mhl": "JobReport.mhl", + "ascmhl": "ascmhl", + "html": "JobReport.html", +} + +#: Formats that describe one destination each rather than the job as a whole. +MANIFEST_FORMATS = ("mhl", "ascmhl") + + +def _containing_root(directory: Path, roots: list[Path]) -> int | None: + """Index of the destination root `directory` sits inside, the deepest if + roots nest, or None when it is outside them all.""" + try: + target = Path(directory).resolve() + except OSError: + return None + best: tuple[int, int] | None = None + for index, root in enumerate(roots): + try: + resolved = Path(root).resolve() + target.relative_to(resolved) + except (OSError, ValueError): + continue + depth = len(resolved.parts) + if best is None or depth > best[0]: + best = (depth, index) + return None if best is None else best[1] + + +def paperwork_dir(job: Job, report_dir: Path | None = None) -> Path: + """Where the PDF, CSV and HTML go.""" + if report_dir is not None: + return Path(report_dir) + roots = job.destination_roots or [job.source_root] + return job.report_dir_for(roots[0]) + + +def write_reports(job: Job, formats: list[str], *, report_dir: Path | None = None, + logo: Path | None = None, footer: str | None = None) -> list[Path]: + """Write every requested format and return what was written. + + Failures are appended to `job.report_failures` rather than raised: a bad + PDF must not mask a good offload, but it must not vanish either. + Manifests are written first, so a manifest failure or a renumbered + manifest is already on the job when the PDF and CSV describe it. + """ + # Looked up per call, so a test can stand a failing writer in. + from . import WRITERS + + roots = list(job.destination_roots or [job.source_root]) + paperwork = paperwork_dir(job, report_dir) + relocated_into = (_containing_root(paperwork, roots) + if report_dir is not None else None) + written: list[Path] = [] + + if "mhl" in formats: + for index, root in enumerate(roots): + directory = (paperwork if index == relocated_into + else job.report_dir_for(root)) + planned = directory / REPORT_FILENAMES["mhl"] + try: + target = next_free_path(planned) + written.append(WRITERS["mhl"](job, target, destination_index=index)) + except Exception as exc: # noqa: BLE001 - recorded, see docstring + job.report_failures.append(f"MHL for {root}: {exc}") + continue + if target != planned: + job.warnings.append( + f"{planned} already existed and was kept; this run's " + f"manifest is {target.name}") + + if "ascmhl" in formats: + for index, root in enumerate(roots): + try: + written.append(WRITERS["ascmhl"]( + job, paperwork / REPORT_FILENAMES["ascmhl"], + destination_index=index)) + except Exception as exc: # noqa: BLE001 + job.report_failures.append(f"ASC MHL for {root}: {exc}") + + for key in formats: + if key in MANIFEST_FORMATS: + continue + writer = WRITERS.get(key) + if writer is None: + continue + try: + os.makedirs(paperwork, exist_ok=True) + written.append(writer(job, paperwork / REPORT_FILENAMES[key], + logo=logo, footer=footer)) + except Exception as exc: # noqa: BLE001 + job.report_failures.append(f"{key} report: {exc}") + return written diff --git a/src/offloader/thumbs.py b/src/offloader/thumbs.py index 4f5bd11..e86f175 100644 --- a/src/offloader/thumbs.py +++ b/src/offloader/thumbs.py @@ -7,6 +7,7 @@ from __future__ import annotations +import hashlib import shutil import subprocess from pathlib import Path @@ -57,6 +58,14 @@ def _sample_offsets(duration: float, count: int) -> list[float]: return [start + span * i / (count - 1) for i in range(count)] +def thumbnail_stem(source: Path, key: str | None = None) -> str: + """`C001_1a2b3c4d`: the clip's stem plus a stable hash of `key`.""" + digest = hashlib.sha1( + (key if key is not None else str(source)).encode("utf-8", "surrogatepass") + ).hexdigest()[:8] + return f"{Path(source).stem}_{digest}" + + def extract( source: Path, media: MediaInfo, @@ -64,12 +73,18 @@ def extract( count: int = 4, timeout: float = 60.0, memo: DecoderMemo | None = None, + key: str | None = None, ) -> list[Path]: """Grab `count` thumbnails. Returns [] if ffmpeg is missing, the file has no video stream, or extraction fails — thumbnails are never load-bearing. With a `memo`, a suffix whose every extraction failed is skipped for the - rest of that memo's lifetime instead of re-spawning ffmpeg per clip.""" + rest of that memo's lifetime instead of re-spawning ffmpeg per clip. + + `key` names the clip, normally its path relative to the source root. + It goes into the filenames, so two clips with one stem in different + folders (A/C001.mov and B/C001.mov) cannot overwrite each other's + frames and put the wrong picture beside a verified clip.""" exe = ffmpeg_path() if exe is None or not media.is_video or not media.duration_sec: return [] @@ -77,7 +92,7 @@ def extract( return [] out_dir.mkdir(parents=True, exist_ok=True) - stem = source.stem + stem = thumbnail_stem(source, key) results: list[Path] = [] vf = ( diff --git a/src/offloader/verify.py b/src/offloader/verify.py index 1717b82..1e08139 100644 --- a/src/offloader/verify.py +++ b/src/offloader/verify.py @@ -25,6 +25,7 @@ from pathlib import Path from xml.etree import ElementTree as ET +from . import longpath from .ascmhl import ASCMHL_DIRNAME from .ascmhl import NAMESPACE as ASCMHL_NAMESPACE from .ascmhl import directory_hashes as ascmhl_directory_hashes @@ -314,7 +315,7 @@ def note(path: Path, digest: str | None) -> str | None: # reason. Deliberately not noted either way. report.verdicts.append(FileVerdict(path, EntryResult.NO_CHECKSUM)) continue - if not path.exists(): + if not longpath.exists(path): note(path, None) report.verdicts.append( FileVerdict(path, EntryResult.MISSING, expected=expected, @@ -322,7 +323,7 @@ def note(path: Path, digest: str | None) -> str | None: continue try: - actual_size = path.stat().st_size + actual_size = longpath.stat(path).st_size if bypass_cache: evict_from_cache(path) actual = hash_file(path, algorithm_key) @@ -360,6 +361,11 @@ def note(path: Path, digest: str | None) -> str | None: # the absence of any recorded pattern but the history's own folder, so # it stops applying the moment a manifest describes its own layout. legacy_paperwork = not [p for p in patterns if p != ASCMHL_DIRNAME] + # A classic manifest now scans from the root it describes, which also + # holds the tool's paperwork. A PDF is not a file the manifest forgot. + paperwork = ([] if ascmhl_root is not None + else [REPORT_DIRECTORY_GLOB, REPORT_DIRECTORY_GLOB + "-[0-9]*"]) + own_folder = manifest.parent.resolve() for candidate in scan_root.rglob("*"): if not candidate.is_file() or candidate.suffix.lower() == ".mhl": @@ -367,11 +373,15 @@ def note(path: Path, digest: str | None) -> str | None: # The history's own bookkeeping is not managed data. if ASCMHL_DIRNAME in candidate.parts: continue - if candidate.resolve() in listed: + resolved = candidate.resolve() + if resolved in listed: continue relative = candidate.relative_to(scan_root).as_posix() if _ignored(relative, patterns): continue + if paperwork and (resolved.parent == own_folder + or _ignored(relative, paperwork)): + continue if find_unlisted: report.unlisted.append(candidate) @@ -566,18 +576,34 @@ def _ascmhl_entries(manifest: Path, root: ET.Element): def _described_root(manifest: Path, listed: set[Path]) -> Path: - """The directory a manifest actually describes. - - Reports live in `_Reports/` while the media sits alongside, so - scanning the manifest's own folder would find nothing. Use the common - ancestor of the files it lists instead. + """The managed root a classic manifest belongs to. + + This tool files a manifest in `_Reports/` (or `-2` and up) inside + the destination, so the root is the folder holding that one; a manifest + anywhere else describes its own folder. That root is used whenever every + listed file is inside it. + + It used to be the common ancestor of the listed files, which is the file + itself for a one-entry manifest (and a file has nothing under it to scan) + and only a subfolder when every entry shares one, so files beside them + were never reported as missing from the manifest. Failing the root test + (an old manifest written to a separate report folder), the common + ancestor of the listed files' *folders* is the fallback. """ + folder = manifest.parent + root = (folder.parent + if fnmatch.fnmatch(folder.name, REPORT_DIRECTORY_GLOB) + or fnmatch.fnmatch(folder.name, REPORT_DIRECTORY_GLOB + "-[0-9]*") + else folder) if not listed: - return manifest.parent + return root try: - return Path(os.path.commonpath([str(p) for p in listed])) - except ValueError: - return manifest.parent + resolved_root = root.resolve() + if all(path.is_relative_to(resolved_root) for path in listed): + return root + return Path(os.path.commonpath([str(p.parent) for p in listed])) + except (OSError, ValueError): + return folder def verify_tree(root: Path, **options) -> list[VerifyReport]: diff --git a/tests/test_reports.py b/tests/test_reports.py index 73982ed..541c92c 100644 --- a/tests/test_reports.py +++ b/tests/test_reports.py @@ -215,11 +215,16 @@ def test_mhl_is_valid_and_relative(sample_job: Job, tmp_path: Path): assert first.findtext("xxh64") == "cd4990759f33f032" -def test_mhl_omits_files_without_a_checksum(sample_job: Job, tmp_path: Path): +def test_mhl_lists_a_file_without_a_checksum_unhashed(sample_job: Job, tmp_path: Path): + """It used to be omitted, which made the manifest read as complete. Listed + with no checksum element, `verify` reports it as unhashed and fails.""" sample_job.files[1].checksum = None sample_job.files[1].destinations[0].checksum = None root = ET.parse(write_mhl(sample_job, tmp_path / "j.mhl")).getroot() - assert len(root.findall("hash")) == 1 + hashes = root.findall("hash") + assert len(hashes) == 2 + assert hashes[1].findtext("xxh64") is None + assert hashes[1].findtext("size") is not None # ------------------------------------------------------------------ HTML diff --git a/tests/test_verdict_integrity.py b/tests/test_verdict_integrity.py new file mode 100644 index 0000000..2bf3513 --- /dev/null +++ b/tests/test_verdict_integrity.py @@ -0,0 +1,533 @@ +"""Regression tests for ways a "Verified" verdict, or a passing `verify`, could +be wrong about what is at the destination. + +Each test reproduces one finding from a private review of the fuzz-edges +branch, on synthetic data only, and fails against the code before its fix. +The bar is the one in AGENTS.md: someone may reformat a card because this tool +said Verified, so a failed, missing or unchecked file must never disappear +into a success verdict, and an existing good copy must never be replaced by +something else. +""" + +from __future__ import annotations + +import os +import platform +import subprocess +import xml.etree.ElementTree as ET +from pathlib import Path + +import pytest + +from offloader import ascmhl, cli, engine, longpath, verify +from offloader import reports as reports_mod +from offloader.models import FileStatus, MediaInfo, VerificationMode +from offloader.reports.mhl import write_mhl + +WINDOWS = platform.system() == "Windows" + + +def _options(dest: Path | list[Path], **overrides) -> engine.OffloadOptions: + defaults = dict( + destinations=dest if isinstance(dest, list) else [dest], + algorithm="xxh3-64", + verification=VerificationMode.FULL, + thumbnail_count=0, + extra_probe=False, + ) + defaults.update(overrides) + return engine.OffloadOptions(**defaults) + + +def _card(tmp_path: Path, files: dict[str, bytes], name: str = "A001") -> Path: + root = tmp_path / name + for relative, data in files.items(): + path = root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(data) + root.mkdir(parents=True, exist_ok=True) + return root + + +def _offload(*args: str) -> int: + """Exit codes are compared as literals on purpose: they are a documented + contract (README), and a format script keys off the numbers.""" + return cli.main(["offload", *args, "--no-probe", "--quiet", "--retries", "1"]) + + +def _flip_one_byte(path: Path) -> None: + data = bytearray(path.read_bytes()) + data[len(data) // 2] ^= 0xFF + path.write_bytes(bytes(data)) + + +def _fail_reads_of(monkeypatch, name: str) -> None: + """Make every read of the source file called `name` fail, as a card with + an unreadable clip does. Partials and destinations are unaffected.""" + real = longpath.open_binary + + def failing(path, mode="rb"): + if "r" in mode and str(path).endswith(name): + raise PermissionError(13, "Access is denied", str(path)) + return real(path, mode) + + monkeypatch.setattr(longpath, "open_binary", failing) + + +# ------------------------------------------------- P1: a failed file in the MHL + + +@pytest.mark.parametrize("report", ["mhl", "ascmhl"]) +def test_a_failed_copy_stays_in_the_manifest_so_verify_fails( + tmp_path, monkeypatch, capsys, report): + """The manifest used to list only the files that copied, so `verify` on a + destination missing a clip said "VERIFIED - safe to erase the source".""" + card = _card(tmp_path, {"Clips/A002_C001.mov": os.urandom(5000), + "Clips/BAD_C002.mov": os.urandom(6000)}) + dest = tmp_path / "dst" + with monkeypatch.context() as patch: + _fail_reads_of(patch, "BAD_C002.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", report) == 1 + capsys.readouterr() + + assert cli.main(["verify", str(dest), "--quiet"]) == 1 + out = capsys.readouterr().out + assert "safe to erase" not in out.replace("do not erase", "") + assert "BAD_C002.mov" in out + + +def test_a_cancelled_job_manifest_lists_what_it_never_reached(tmp_path): + card = _card(tmp_path, {f"C{n:03d}.mov": os.urandom(3000) for n in range(1, 4)}) + dest = tmp_path / "dst" + control = engine.JobControl() + + def cancel_after_first(event: engine.ProgressEvent) -> None: + if event.file_index >= 1: + control.cancel() + + job = engine.run(card, _options(dest), cancel_after_first, control) + assert job.cancelled + manifest = write_mhl(job, dest / "A001_Reports" / "JobReport.mhl") + report = verify.verify_manifest(manifest) + assert not report.passed, report.summary() + + +# ----------------------------------------- P1: skip-existing keeps the manifest + + +def test_a_skip_existing_rerun_never_overwrites_the_earlier_manifest( + tmp_path, capsys): + """A rerun used to overwrite JobReport.mhl with one listing only what it + copied, so a corrupt same-size copy it skipped verified clean.""" + card = _card(tmp_path, {"Clips/C001.mov": os.urandom(5000), + "Clips/C002.mov": os.urandom(5000)}) + dest = tmp_path / "dst" + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 0 + first = dest / "A001_Reports" / "JobReport.mhl" + original = first.read_bytes() + + _flip_one_byte(dest / "Clips" / "C001.mov") # same size, wrong bytes + (card / "Clips" / "C003.mov").write_bytes(os.urandom(4000)) + rerun = _offload("--source", str(card), "--dest", str(dest), "--report", "mhl", + "--skip-existing") + capsys.readouterr() + + assert first.read_bytes() == original + assert len(verify.find_manifests(dest)) == 2 + assert cli.main(["verify", str(dest), "--quiet"]) == 1 + out = capsys.readouterr().out + assert "MISMATCH" in out and "C001.mov" in out + assert rerun == 4 # skipped files were not verified: Incomplete + + +def test_a_reused_report_dir_gets_a_numbered_manifest(tmp_path, capsys): + card = _card(tmp_path, {"C001.mov": os.urandom(3000)}) + dest = tmp_path / "dst" + paperwork = dest / "paperwork" + for _ in range(2): + _offload("--source", str(card), "--dest", str(dest), "--report", "mhl", + "--report-dir", str(paperwork)) + assert (paperwork / "JobReport.mhl").is_file() + assert (paperwork / "JobReport-2.mhl").is_file() + assert "JobReport-2.mhl" in capsys.readouterr().err + + +# --------------------------------------------- P1: unlistable directories fail + + +def test_a_directory_that_cannot_be_listed_fails_the_job(tmp_path, monkeypatch): + card = _card(tmp_path, {"Clips/C001.mov": b"a" * 100, + "Locked/C002.mov": b"b" * 100}) + locked = card / "Locked" + real = os.scandir + + def refusing(path=".", *args): + if os.path.normcase(str(path)).endswith(os.path.normcase("Locked")): + raise PermissionError(13, "Access is denied", str(path)) + return real(path, *args) + + monkeypatch.setattr(os, "scandir", refusing) + job = engine.run(card, _options(tmp_path / "dst")) + monkeypatch.undo() + + assert job.final_status == "Failed" + assert [p.path for p in job.scan_errors] == [locked] + assert "Locked" in job.notes + + +def test_a_listing_that_fails_part_way_fails_the_job(tmp_path, monkeypatch): + card = _card(tmp_path, {"Clips/C001.mov": b"a" * 100, + "Bad/C002.mov": b"b" * 100}) + real = os.scandir + + class Breaks: + def __init__(self, path): + self._path = path + + def __iter__(self): + return self + + def __next__(self): + raise OSError(5, "I/O error", str(self._path)) + + def __enter__(self): + return self + + def __exit__(self, *_): + return None + + def close(self): + return None + + def flaky(path=".", *args): + if os.path.normcase(str(path)).endswith(os.path.normcase("Bad")): + return Breaks(path) + return real(path, *args) + + monkeypatch.setattr(os, "scandir", flaky) + job = engine.run(card, _options(tmp_path / "dst")) + monkeypatch.undo() + assert job.final_status == "Failed" + assert job.scan_errors and job.scan_errors[0].path.name == "Bad" + + +@pytest.mark.skipif(not WINDOWS, reason="uses an NTFS deny ACE") +def test_an_acl_locked_directory_fails_the_offload_and_verify(tmp_path, capsys): + card = _card(tmp_path, {"Clips/A001_C001.mov": os.urandom(5000), + "Locked/A001_C002.mov": os.urandom(7000)}) + locked = card / "Locked" + user = os.environ.get("USERNAME", "") + denied = subprocess.run(["icacls", str(locked), "/deny", f"{user}:(RD)"], + capture_output=True, text=True) + try: + if denied.returncode != 0: + pytest.skip(f"icacls failed: {denied.stdout} {denied.stderr}") + try: + os.listdir(locked) + pytest.skip("the deny ACE did not stop listing (elevated session?)") + except PermissionError: + pass + dest = tmp_path / "dst" + code = _offload("--source", str(card), "--dest", str(dest), "--report", "mhl") + err = capsys.readouterr().err + assert code == 1 + assert "Locked" in err + assert cli.main(["verify", str(dest), "--quiet"]) == 1 + finally: + subprocess.run(["icacls", str(locked), "/remove:d", user], + capture_output=True, text=True) + + +# ------------------------------------- P2: names Win32 rewrites are literal + + +@pytest.mark.skipif(not WINDOWS, reason="Win32 name rewriting is Windows-only") +def test_names_win32_would_rewrite_are_copied_as_themselves(tmp_path, capsys): + """`clip.mov.` used to open `clip.mov`, so the wrong bytes were read and + certified; `NUL` read as an empty device and left a partial behind.""" + card = _card(tmp_path, {"B/clip.mov": b"A" * 1000}) + odd = {"clip.mov.": b"Z" * 3000, "clip.mov ": b"S" * 2000, "NUL": b"N" * 500} + for name, data in odd.items(): + with open("\\\\?\\" + str(card / "B" / name), "wb") as handle: + handle.write(data) + dest = tmp_path / "dst" + try: + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 0 + listing = os.listdir("\\\\?\\" + str(dest / "B")) + assert sorted(listing) == sorted(["clip.mov", *odd]), listing + for name, data in {**odd, "clip.mov": b"A" * 1000}.items(): + with open("\\\\?\\" + str(dest / "B" / name), "rb") as handle: + assert handle.read() == data, name + assert cli.main(["verify", str(dest), "--quiet"]) == 0 + finally: + for root in (card, dest): + for name in odd: + try: + os.remove("\\\\?\\" + str(root / "B" / name)) + except OSError: + pass + + +# ------------------------------------------ P2: junctions are never followed + + +@pytest.mark.skipif(not WINDOWS, reason="directory junctions are Windows-only") +def test_a_junction_listed_before_its_target_does_not_claim_it(tmp_path): + card = _card(tmp_path, {"Z_real/C001.mov": os.urandom(4000)}) + made = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(card / "A_alias"), str(card / "Z_real")], + capture_output=True, text=True) + if made.returncode != 0: + pytest.skip(f"could not create a junction: {made.stderr.strip()}") + + dest = tmp_path / "dst" + job = engine.run(card, _options(dest)) + + assert job.final_status == "Verified" + assert (dest / "Z_real" / "C001.mov").read_bytes() == \ + (card / "Z_real" / "C001.mov").read_bytes() + assert not (dest / "A_alias").exists() + assert any("A_alias" in warning for warning in job.warnings) + + +# ------------------------- P2: a source holding _Reports is not overwritten + + +def test_reports_never_land_on_a_copied_reports_folder(tmp_path, capsys): + card = _card(tmp_path, { + "Clips/C001.mov": os.urandom(3000), + "A001_Reports/clip.txt": b"shuttle notes", + "A001_Reports/JobReport.mhl": b"the shuttle's manifest", + "A001_Reports/JobReport.csv": b"shuttle,csv\n", + }) + dest = tmp_path / "dst" + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl,csv") == 0 + for name in ("clip.txt", "JobReport.mhl", "JobReport.csv"): + assert (dest / "A001_Reports" / name).read_bytes() == \ + (card / "A001_Reports" / name).read_bytes(), name + assert (dest / "A001_Reports-2" / "JobReport.mhl").is_file() + assert "A001_Reports-2" in capsys.readouterr().err + assert cli.main(["verify", str(dest / "A001_Reports-2" / "JobReport.mhl"), + "--quiet"]) == 0 + + +# ---------------------------------------- P2: report failures are not success + + +def test_a_report_failure_exits_non_zero_and_is_on_the_job(tmp_path, monkeypatch, + capsys): + card = _card(tmp_path, {"C001.mov": os.urandom(3000)}) + + def broken(*_args, **_kwargs): + raise OSError(28, "No space left on device") + + monkeypatch.setitem(reports_mod.WRITERS, "mhl", broken) + code = _offload("--source", str(card), "--dest", str(tmp_path / "dst"), + "--report", "mhl,csv") + captured = capsys.readouterr() + assert code == 5 + assert "REPORT(S) FAILED" in captured.err + assert "reports failed" in captured.out + + job = engine.run(card, _options(tmp_path / "dst2")) + reports_mod.paperwork.write_reports(job, ["mhl"]) + assert job.report_failures + assert job.final_status == "Verified, reports failed" + + +def test_the_gui_runner_records_report_failures(tmp_path, monkeypatch): + os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + pytest.importorskip("PySide6", reason="GUI extra not installed") + from PySide6.QtWidgets import QApplication + + from offloader.gui import worker + from offloader.presets import Preset + + QApplication.instance() or QApplication([]) + card = _card(tmp_path, {"C001.mov": os.urandom(3000)}) + dest = tmp_path / "dst" + + def broken(*_args, **_kwargs): + raise PermissionError(13, "Permission denied") + + monkeypatch.setitem(reports_mod.WRITERS, "mhl", broken) + preset = Preset(name="t", destinations=[str(dest)], reports=["mhl"], + thumbnail_count=0) + item = worker.QueueItem(identifier=1, source=card, name="A001", preset=preset) + job = engine.run(card, _options(dest, job_name="A001")) + runner = worker._Runner(item) + runner._write_reports(job, preset) + item.job, item.state = job, worker.JobState.DONE + + assert "reports failed" in item.status_text + assert job.report_failures + + +# ------------------------------- P2: never replace a differing existing file + + +def test_a_different_card_never_replaces_an_existing_verified_file(tmp_path): + relative = "DCIM/100CANON/IMG_0001.CR3" + card_a = _card(tmp_path, {relative: b"A" * 4096}, name="cardA") + card_b = _card(tmp_path, {relative: b"B" * 4096}, name="cardB") + dest = tmp_path / "dst" + target = dest / relative + + assert engine.run(card_a, _options(dest)).final_status == "Verified" + second = engine.run(card_b, _options(dest)) + + assert second.final_status == "Failed" + error = second.files[0].destinations[0].error + assert "different content" in error and "--replace-existing" in error + assert target.read_bytes() == b"A" * 4096 + assert not list(dest.rglob("*.offloader-partial")) + + # The same card again is harmless and still verifies. + assert engine.run(card_a, _options(dest)).final_status == "Verified" + # Asked for explicitly, the replacement happens. + forced = engine.run(card_b, _options(dest, replace_existing=True)) + assert forced.final_status == "Verified" + assert target.read_bytes() == b"B" * 4096 + + +def test_the_cli_takes_replace_existing(tmp_path): + args = cli.build_parser().parse_args( + ["offload", "--source", "s", "--dest", "d", "--replace-existing"]) + assert cli._options_from(args, [Path("d")]).replace_existing is True + + +# ------------------------------- P2: every destination has its own manifest + + +def test_each_destination_verifies_on_its_own_with_report_dir_in_the_second( + tmp_path, capsys): + card = _card(tmp_path, {"C001.mov": os.urandom(5000)}) + d0, d1 = tmp_path / "d0", tmp_path / "d1" + assert _offload("--source", str(card), "--dest", str(d0), "--dest", str(d1), + "--report", "mhl,csv", + "--report-dir", str(d1 / "A001_Reports")) == 0 + assert (d0 / "A001_Reports" / "JobReport.mhl").is_file() + listed = [node.text for node in + ET.parse(d1 / "A001_Reports" / "JobReport.mhl").getroot().iter("file")] + assert listed == ["../C001.mov"], listed + + assert cli.main(["verify", str(d0), "--quiet"]) == 0 + assert cli.main(["verify", str(d1), "--quiet"]) == 0 + _flip_one_byte(d1 / "C001.mov") + capsys.readouterr() + assert cli.main(["verify", str(d1), "--quiet"]) == 1 + assert cli.main(["verify", str(d0), "--quiet"]) == 0 + + +# ------------------------------------------------------- P3: exit codes + + +def test_an_empty_source_exits_incomplete(tmp_path): + card = _card(tmp_path, {}) + assert _offload("--source", str(card), "--dest", str(tmp_path / "dst"), + "--report", "csv") == 4 + + +def test_an_all_skipped_rerun_exits_incomplete(tmp_path): + card = _card(tmp_path, {"C001.mov": os.urandom(3000)}) + dest = tmp_path / "dst" + assert _offload("--source", str(card), "--dest", str(dest), "--report", "csv") == 0 + assert _offload("--source", str(card), "--dest", str(dest), "--report", "csv", + "--skip-existing") == 4 + + +# ---------------------------------------- P3: verify scans the managed root + + +def test_verify_reports_files_beside_a_one_entry_manifest(tmp_path): + card = _card(tmp_path, {"Clips/C001.mov": os.urandom(2000)}) + dest = tmp_path / "dst" + job = engine.run(card, _options(dest)) + manifest = write_mhl(job, dest / "A001_Reports" / "JobReport.mhl") + (dest / "Clips" / "C002.mov").write_bytes(b"not in the manifest") + (dest / "Other").mkdir() + (dest / "Other" / "C003.mov").write_bytes(b"nor this") + + report = verify.verify_manifest(manifest) + names = sorted(p.name for p in report.unlisted) + assert names == ["C002.mov", "C003.mov"], names + assert report.passed # unlisted files are diagnostic, not failures + + +# ------------------------------------- P3: a file vanishing mid-job is one file + + +def test_a_file_that_vanishes_after_the_scan_fails_alone(tmp_path): + card = _card(tmp_path, {f"C{n:03d}.mov": os.urandom(3000) for n in range(1, 4)}) + doomed = card / "C002.mov" + + def delete_during_first(event: engine.ProgressEvent) -> None: + if event.file_index == 0 and doomed.exists(): + doomed.unlink() + + job = engine.run(card, _options(tmp_path / "dst"), delete_during_first) + + by_name = {entry.name: entry for entry in job.files} + assert by_name["C002.mov"].status is FileStatus.FAILED + assert "vanished" in by_name["C002.mov"].destinations[0].error + assert by_name["C003.mov"].status is FileStatus.VERIFIED + assert job.final_status == "Failed" + + +# --------------------------------------- P3: thumbnails per clip, not per stem + + +def test_same_named_clips_in_different_folders_keep_their_own_thumbnails( + tmp_path, monkeypatch): + from offloader import probe as probe_mod + from offloader import thumbs + + card = _card(tmp_path, {"A/C001.mov": b"first take" * 100, + "B/C001.mov": b"second take" * 100}) + video = MediaInfo(width=1920, height=1080, duration_sec=10.0) + monkeypatch.setattr(probe_mod, "probe", lambda _path: video) + monkeypatch.setattr(thumbs, "ffmpeg_path", lambda: "ffmpeg") + + def fake_ffmpeg(command, **_kwargs): + source, target = command[command.index("-i") + 1], command[-1] + Path(target).write_text(source, encoding="utf-8") + return subprocess.CompletedProcess(command, 0, "", "") + + monkeypatch.setattr(thumbs.subprocess, "run", fake_ffmpeg) + job = engine.run(card, _options(tmp_path / "dst", extra_probe=True, + thumbnail_count=2)) + + first, second = job.files + assert first.thumbnails and second.thumbnails + assert not set(first.thumbnails) & set(second.thumbnails) + for entry in job.files: + for picture in entry.thumbnails: + drawn_from = Path(picture.read_text(encoding="utf-8")) + assert drawn_from.parent.name == entry.source.parent.name + + +# ------------------------------- P3: ASC MHL previous digest case-insensitive + + +def test_an_uppercase_previous_digest_still_labels_the_file_verified(tmp_path): + card = _card(tmp_path, {"C001.mov": os.urandom(2000)}) + dest = tmp_path / "dst" + job = engine.run(card, _options(dest, algorithm="xxh64")) + first = ascmhl.write_manifest(job, dest) + namespace = f"{{{ascmhl.NAMESPACE}}}" + tree = ET.parse(first) + for element in tree.getroot().iter(f"{namespace}xxh64"): + if element.get("action"): + element.text = element.text.upper() + text = ET.tostring(tree.getroot(), encoding="unicode") + first.write_text(text, encoding="utf-8") + + second = ascmhl.write_manifest(job, dest) + actions = [element.get("action") + for element in ET.parse(second).getroot().iter(f"{namespace}xxh64") + if element.get("action")] + assert actions == ["verified"] From f4ef8533866f54fa8091917b22dcb25d5b0902d6 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:43:53 -0600 Subject: [PATCH 10/14] Record later destinations as cancelled when a cancel lands during the existing-file check --- CHANGELOG.md | 3 ++- src/offloader/engine.py | 48 +++++++++++++++++++++++++---------------- tests/test_control.py | 37 +++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f09b3c..fbb9108 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,7 +84,8 @@ project uses [semantic versioning][semver]. - **A cancel during a second destination's reread left the file reading as Verified.** Only the destinations already promoted were recorded, and a file's status is its weakest recorded copy, so the copy that was never made - could not lower it. The missing copies are now recorded as cancelled. + could not lower it. The missing copies are now recorded as cancelled, and + the same applies to a cancel while an existing destination file is compared. - **`offloader verify` stayed silent when it could not evict the page cache.** The offload warns that a read-back may have come from memory; verify discarded the same result. It now reports the same warning. diff --git a/src/offloader/engine.py b/src/offloader/engine.py index 7da33d3..9f113c6 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -977,23 +977,8 @@ def copy_once(_src=source, _partials=partials, _idx=index, "failing") destination.checksum = dst_sum or None except JobCancelled: - # Nothing unpromoted survives. Destinations already - # promoted were verified and stay; their partials - # are gone, so discarding the whole list only - # removes this one and the ones not yet reached. - _discard(partials) - # Record this copy and every later one as cancelled. - # The entry's status is its weakest copy, and a copy - # that was never made must be the weakest, or the - # file would read as Verified on the strength of the - # destinations that did finish. - for later_root, later_target in zip( - dest_roots[position:], targets[position:], - strict=True): - entry.destinations.append(Destination( - root=later_root, path=later_target, - status=FileStatus.CANCELLED, - error="cancelled before verification")) + _cancel_unpromoted(entry, partials, dest_roots, + targets, position) cancelled_in_verify = True break except OSError as exc: @@ -1028,7 +1013,11 @@ def copy_once(_src=source, _partials=partials, _idx=index, refusal = _existing_file_refusal( target, partial, src_sum, options.algorithm, control) except JobCancelled: - _discard(partials) + # The same as a cancel during the reread: this copy and + # every later one are recorded, or the file reads as + # Verified on the strength of the destinations before it. + _cancel_unpromoted(entry, partials, dest_roots, + targets, position) cancelled_in_verify = True break if refusal is not None: @@ -1176,6 +1165,29 @@ def choose_report_folder(name: str, files: Sequence[Path], source_root: Path, return candidate +def _cancel_unpromoted(entry: FileEntry, partials: Sequence[Path], + dest_roots: Sequence[Path], targets: Sequence[Path], + position: int) -> None: + """Record a cancel that landed before destination `position` was promoted. + + Nothing unpromoted survives. Destinations already promoted were verified + and stay; their partials are gone, so discarding the whole list only + removes this one and the ones not yet reached. + + This copy and every later one are recorded as cancelled. The entry's + status is its weakest copy, and a copy that was never made must be the + weakest, or the file would read as Verified on the strength of the + destinations that did finish. + """ + _discard(partials) + for later_root, later_target in zip(dest_roots[position:], targets[position:], + strict=True): + entry.destinations.append(Destination( + root=later_root, path=later_target, + status=FileStatus.CANCELLED, + error="cancelled before verification")) + + def _existing_file_refusal(target: Path, partial: Path, source_checksum: str, algorithm: str, control: JobControl | None) -> str | None: diff --git a/tests/test_control.py b/tests/test_control.py index e71e189..4c35c6c 100644 --- a/tests/test_control.py +++ b/tests/test_control.py @@ -248,3 +248,40 @@ def progress(event: engine.ProgressEvent) -> None: assert (first / "clip.mov").exists() assert not (second / "clip.mov").exists() assert not list(tmp_path.rglob("*.offloader-partial")) + + +def test_cancel_during_the_existing_file_check_records_the_missing_copy( + tmp_path: Path, monkeypatch, +): + """A cancel while a differing file already at destination two is being + compared must not leave the file reading as Verified on the strength of + destination one, and must not touch the file that was already there.""" + card = _one_file_card(tmp_path) + first, second = tmp_path / "d1", tmp_path / "d2" + second.mkdir() + existing = second / "clip.mov" + original = b"\x00" * 300_000 # same size, different content + existing.write_bytes(original) + control = engine.JobControl() + real_hash_file = engine.hash_file + + def hash_file(path, *args, **kwargs): + if Path(path) == existing: + control.cancel() + return real_hash_file(path, *args, **kwargs) + + monkeypatch.setattr(engine, "hash_file", hash_file) + + job = engine.run(card, _options(tmp_path, destinations=[first, second]), + control=control) + + assert job.cancelled + assert job.final_status != "Verified" + (entry,) = job.files + assert [d.status for d in entry.destinations] == [ + FileStatus.VERIFIED, FileStatus.CANCELLED] + assert entry.destinations[1].error == "cancelled before verification" + assert entry.status is FileStatus.CANCELLED + assert (first / "clip.mov").exists() + assert existing.read_bytes() == original + assert not list(tmp_path.rglob("*.offloader-partial")) From 02ba8962ecb7ea6fa7537d1a57c705a2661325eb Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:55:00 -0600 Subject: [PATCH 11/14] Update test counts --- CONTRIBUTING.md | 2 +- README.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 63db694..59b77fd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # 575 tests, about 20s +pytest # 579 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index a70a5b9..6b36a84 100644 --- a/README.md +++ b/README.md @@ -409,13 +409,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 575 tests, ~20s +pytest # 579 tests, ~20s pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -575 tests at 87% line coverage. They cover formatting against the reference's +579 tests at 87% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, From 9e6aa67d3bc16a16dee8c752d58015fa8f63af72 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:19:52 -0600 Subject: [PATCH 12/14] Let a newer manifest supersede an earlier run's unhashed entry in verify A failed run's manifest lists the files it could not read with no hash, and a successful rerun writes a new manifest beside it, so verify kept failing until the old manifest was deleted by hand. An unhashed entry is now superseded when a newer manifest for the same destination root lists the same path with a hash that matches, and verify prints it as superseded by that manifest. A hashed entry is never superseded, and newer is decided by the manifest's own recorded time, then its numbering, never by mtime. --- CHANGELOG.md | 8 ++ README.md | 9 +- docs/data-safety.md | 36 +++++- src/offloader/cli.py | 14 ++- src/offloader/verify.py | 187 +++++++++++++++++++++++++++++-- tests/test_verdict_integrity.py | 190 ++++++++++++++++++++++++++++++++ 6 files changed, 427 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbb9108..c748132 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,14 @@ project uses [semantic versioning][semver]. `_Reports-2` when `_Reports` already exists, a reused `--report-dir` gets `JobReport-2.mhl`, and `verify` checks every manifest under the folder. +- **A failed run's manifest kept failing `verify` after a successful rerun.** + The files it could not read stay unhashed in it, so `verify` failed until the + operator deleted it by hand. An unhashed entry is now superseded when a newer + manifest for the same destination root lists the same path with a hash that + matches; it is printed as `superseded by `. Only that direction: + a hashed entry is never superseded, so a copy corrupted since still fails. + Newer means the manifest's own recorded time, then its number + (`_Reports-2`, `JobReport-2.mhl`), never the file's modification time. - **A source folder that could not be listed was silently left out.** The job read Verified without the files under it. Listing errors are now recorded, the job fails naming the folder, the CLI exits `1`, and the manifests carry an diff --git a/README.md b/README.md index 6b36a84..622feb9 100644 --- a/README.md +++ b/README.md @@ -134,8 +134,13 @@ manifest lists and exits non-zero if anything is off, so a format script can gat on it. An offload's manifest lists every file it scanned for that destination, including any that failed (recorded with no checksum when none could be taken), so a destination missing a clip fails here; every manifest -under the folder is checked, earlier runs' included. `--allow-cache` skips the -page-cache eviction: faster, and may verify memory rather than the device. +under the folder is checked, earlier runs' included. An entry a failed run could +not hash is reported as superseded, and stops failing, once a newer manifest for +the same destination lists that file with a hash that still matches; a hashed +entry is never superseded. See +[`docs/data-safety.md`](docs/data-safety.md#a-manifest-lists-what-did-not-arrive). +`--allow-cache` skips the page-cache eviction: faster, and may verify memory +rather than the device. For an ASC MHL history it also recomputes the directory content and structure hashes, which is the only check that catches a rename or a moved file — every diff --git a/docs/data-safety.md b/docs/data-safety.md index cf5077c..cf20455 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -206,10 +206,35 @@ Every file the job scanned is now listed for each destination: - Files skipped by `--skip-existing` are **not** listed: nothing re-read them, and listing them would certify bytes nobody checked. -An unhashed entry has nothing to pass against, so it keeps failing. After a -failed run has been resumed, its manifest still lists the files it could not -read; once the rerun has verified them, retire that manifest by hand. Nothing -else can say those files were checked. +An unhashed entry has nothing to pass against, so on its own it keeps failing. +After a failed run has been resumed, its manifest still lists the files it could +not read. When `verify` checks a folder, such an entry is **superseded** if a +newer manifest for the same destination root lists the same path with a hash +and that file matches it now. The entry is printed as `superseded by +`, not dropped, and the older manifest passes only if nothing else in +it failed. + +Supersession runs one way only: + +- Only an entry with no hash can be superseded. A hashed entry is never + answered by a later manifest, so a copy corrupted since an earlier run still + fails on that run's manifest, whatever a later run recorded. +- Only a hashed entry that matched can supersede. A later run that could not + read the file lists it unhashed, and that entry fails on its own. +- "Newer" is the manifest's own recorded time (MHL 1.1 `finishdate`, ASC MHL + `creationdate`). When those are equal (two runs inside one second) or + missing, the numbering decides (`_Reports-2`, `JobReport-2.mhl`), and + only between manifests in the same numbered series. File modification times + are never used. When neither says which is newer, nothing is superseded. +- Same destination root means the root `verify` already derives for each + manifest: the folder above `_Reports*`, or the folder above `ascmhl/`. + A manifest for a subfolder (another job whose destination was + `dst/Clips`) does not answer one for `dst`, even about the same file. +- Directory hash failures are not touched, so the ASC MHL false alarm after a + `--skip-existing` rerun (below) still fails. + +Verifying a single `.mhl` file checks that file alone, so its unhashed entries +fail as before. ### A manifest is never overwritten @@ -223,7 +248,8 @@ Now the job's paperwork folder is chosen before the copy: `_Reports`, or part of what is being copied. When a manifest would still land on an existing file (a reused `--report-dir`), it is written as `JobReport-2.mhl` and up, and the summary says so. `verify` on a folder checks every classic manifest under it, -so the earlier run's coverage is still checked. +so the earlier run's coverage is still checked, and a newer manifest can +supersede an earlier one's unhashed entries as described above. Re-offloading a previous destination (a shuttle drive holding `A001/A001_Reports/`) used to write the new reports over the copied folder, diff --git a/src/offloader/cli.py b/src/offloader/cli.py index 0c56a75..75245ee 100644 --- a/src/offloader/cli.py +++ b/src/offloader/cli.py @@ -318,10 +318,11 @@ def progress(index: int, total: int, path: Path) -> None: sys.stderr.flush() worst = 0 + reports = [] for manifest in manifests: try: - report = verify_mod.verify_manifest( - manifest, progress=progress, bypass_cache=not args.allow_cache) + reports.append(verify_mod.verify_manifest( + manifest, progress=progress, bypass_cache=not args.allow_cache)) except Exception as exc: print(f"error: could not read {manifest}: {exc}", file=sys.stderr) worst = max(worst, 2) @@ -329,10 +330,19 @@ def progress(index: int, total: int, path: Path) -> None: if show: sys.stderr.write('\r' + " " * 90 + '\r') + # Every manifest is checked before any is reported, so a rerun's manifest + # can answer the entries an earlier failed run could not hash. Only that + # direction: see `verify.supersede`. + verify_mod.supersede(reports) + + for report in reports: + manifest = report.manifest print('\n' + str(manifest)) print(f" {report.summary()}") for verdict in report.failures: print(f" {verdict.describe()}") + for verdict in report.superseded: + print(f" {verdict.describe()}") for verdict in report.directory_failures: print(f" {verdict.describe()}") for warning in report.warnings: diff --git a/src/offloader/verify.py b/src/offloader/verify.py index d7fc197..df8f091 100644 --- a/src/offloader/verify.py +++ b/src/offloader/verify.py @@ -17,8 +17,10 @@ from __future__ import annotations +import datetime as _dt import fnmatch import os +import re from collections.abc import Callable, Iterator from dataclasses import dataclass, field from enum import Enum @@ -55,12 +57,19 @@ class FileVerdict: expected_size: int | None = None actual_size: int | None = None detail: str = "" + #: A later manifest for the same root hashed this file and it matched, + #: which answers an entry this manifest could not check. Only ever set on + #: a `NO_CHECKSUM` verdict; see `supersede`. + superseded_by: Path | None = None @property def ok(self) -> bool: return self.result is EntryResult.OK def describe(self) -> str: + if self.superseded_by is not None: + return (f"superseded {self.path} (no hash here; superseded by " + f"{self.superseded_by})") if self.result is EntryResult.OK: return f"ok {self.path}" if self.result is EntryResult.MISSING: @@ -138,6 +147,12 @@ class VerifyReport: #: Caveats on the evidence that do not change a verdict, such as a #: page-cache eviction that failed, so the bytes may have come from memory. warnings: list[str] = field(default_factory=list) + #: The destination root the manifest describes, which is what decides + #: whether another manifest is about the same copy. + root: Path | None = None + #: When the manifest says it was written (MHL 1.1 `finishdate`, ASC MHL + #: `creationdate`), or `None` when it does not say or cannot be read. + recorded_at: _dt.datetime | None = None @property def checked(self) -> int: @@ -145,7 +160,12 @@ def checked(self) -> int: @property def failures(self) -> list[FileVerdict]: - return [v for v in self.verdicts if not v.ok] + return [v for v in self.verdicts + if not v.ok and v.superseded_by is None] + + @property + def superseded(self) -> list[FileVerdict]: + return [v for v in self.verdicts if v.superseded_by is not None] @property def directory_failures(self) -> list[DirectoryVerdict]: @@ -159,12 +179,24 @@ def passed(self) -> bool: def counts(self) -> dict[str, int]: tally: dict[str, int] = {} for verdict in self.verdicts: - tally[verdict.result.value] = tally.get(verdict.result.value, 0) + 1 + name = ("superseded" if verdict.superseded_by is not None + else verdict.result.value) + tally[name] = tally.get(name, 0) + 1 return tally def summary(self) -> str: if not self.verdicts: return "manifest listed no files with checksums" + if self.passed and self.superseded: + # Never "all N files match": the superseded ones had nothing here + # to match, and the line has to say where their check came from. + line = (f"{self.checked - len(self.superseded)} files match the " + f"manifest; {len(self.superseded)} with no hash here " + "matched a later manifest") + if self.directories: + line += (f"; all {len(self.directories)} directory hashes " + "match") + return line if self.passed: if self.directories: return (f"all {self.checked} files and all " @@ -176,8 +208,10 @@ def summary(self) -> str: # file tally first would open "N checked: N ok", which reads as a # pass to anyone scanning — and this is the one verdict where the # file hashes agreeing is the point rather than the reassurance. + files = (f"all {self.checked} files match" if not self.superseded + else "every file matches here or in a later manifest") return (f"{len(self.directory_failures)} of {len(self.directories)} " - f"directory hashes differ; all {self.checked} files match, " + f"directory hashes differ; {files}, " "so the bytes are intact and the tree is not") parts = [f"{count} {name}" for name, count in sorted(self.counts().items())] line = f"{self.checked} checked: " + ", ".join(parts) @@ -275,7 +309,12 @@ def verify_manifest( # ASC MHL keeps its manifests in `ascmhl/` at the root of the managed data, # so the folder above is what the recorded paths are relative to. managed_root = manifest.parent.parent - ascmhl_root = _ascmhl_root(manifest) if check_directories else None + parsed = _ascmhl_root(manifest) + listed = {p.resolve() for p, _, _, _ in entries} + report.root = (managed_root if parsed is not None + else _described_root(manifest, listed)) + report.recorded_at = _recorded_at(manifest) + ascmhl_root = parsed if check_directories else None recorded: dict[str, tuple[str, str]] = {} if ascmhl_root is not None: recorded, directory_algorithm = _ascmhl_directory_hashes(ascmhl_root) @@ -364,7 +403,6 @@ def _evict(path: Path) -> None: # The directory pass needs the unexpected files hashed, so it scans even # when the caller did not ask for them to be reported. if find_unlisted or ascmhl_root is not None: - listed = {p.resolve() for p, _, _, _ in entries} patterns = _ignore_patterns(ascmhl_root) if ascmhl_root is not None else [] scan_root = (managed_root if ascmhl_root is not None else _described_root(manifest, listed)) @@ -619,7 +657,140 @@ def _described_root(manifest: Path, listed: set[Path]) -> Path: return folder +def _recorded_at(manifest: Path) -> _dt.datetime | None: + """When the manifest says it was written, from its own creator info. + + MHL 1.1 records `finishdate` (falling back to `startdate`); ASC MHL records + `creationdate`. A value that will not parse is treated as absent rather + than guessed at, and the file's mtime is never used: copying an archive + to another drive rewrites mtimes, which says nothing about which run came + later. + """ + try: + root = ET.parse(manifest).getroot() + except (OSError, ET.ParseError): + return None + prefix = (f"{{{ASCMHL_NAMESPACE}}}" + if root.tag == f"{{{ASCMHL_NAMESPACE}}}hashlist" else "") + info = root.find(f"{prefix}creatorinfo") + if info is None: + return None + names = ("creationdate",) if prefix else ("finishdate", "startdate") + for name in names: + text = (info.findtext(f"{prefix}{name}") or "").strip() + if not text: + continue + try: + return _dt.datetime.fromisoformat(text.replace("Z", "+00:00")) + except ValueError: + return None + return None + + +_NUMBERED = re.compile(r"^(?P.*)-(?P[0-9]+)$") + + +def _numbering(manifest: Path) -> tuple[tuple[str, ...], tuple[int, ...]]: + """(series, position) of a manifest in this tool's numbering. + + A classic manifest is `_Reports[-N]/JobReport[-N].mhl`; two of them + are in one series when everything but the numbers matches, and the later + numbers are the later run. An ASC MHL generation is numbered by its + `0001_` prefix within its own `ascmhl/` folder. Only a manifest in the + same series can be ordered by name; across series the answer is unknown. + """ + folder = manifest.parent + if folder.name == ASCMHL_DIRNAME: + match = re.match(r"^([0-9]+)_", manifest.name) + number = int(match.group(1)) if match else 0 + return ("ascmhl", os.path.normcase(str(folder))), (number,) + + folder_name, folder_number = folder.name, 1 + match = _NUMBERED.match(folder.name) + if match and fnmatch.fnmatch(match.group("base"), REPORT_DIRECTORY_GLOB): + folder_name, folder_number = match.group("base"), int(match.group("number")) + + stem, file_number = manifest.stem, 1 + match = _NUMBERED.match(manifest.stem) + if match: + stem, file_number = match.group("base"), int(match.group("number")) + + series = ("mhl", os.path.normcase(str(folder.parent)), + os.path.normcase(folder_name), os.path.normcase(stem)) + return series, (folder_number, file_number) + + +def _is_newer(candidate: VerifyReport, than: VerifyReport) -> bool: + """Whether `candidate` is known to have been written after `than`. + + The manifests' own timestamps decide when both have one and they differ. + Otherwise (equal to the second, which two quick runs manage, or missing) + the numbering decides, and only within one series. Anything else is not + known to be newer, which keeps the older entry failing. + """ + a, b = candidate.recorded_at, than.recorded_at + if a is not None and b is not None: + try: + if a != b: + return a > b + except TypeError: + # One carries a UTC offset and the other does not. + pass + series_a, position_a = _numbering(candidate.manifest) + series_b, position_b = _numbering(than.manifest) + return series_a == series_b and position_a > position_b + + +def _same_root(a: VerifyReport, b: VerifyReport) -> bool: + if a.root is None or b.root is None: + return False + try: + return (os.path.normcase(str(a.root.resolve())) + == os.path.normcase(str(b.root.resolve()))) + except OSError: + return False + + +def _entry_key(path: Path) -> str: + # Lexical on purpose, as `reports.mhl._relative` is: `abspath` trims a + # trailing dot on Windows and would make `clip.mov.` match `clip.mov`. + return os.path.normcase(os.path.normpath(str(path))) + + +def supersede(reports: list[VerifyReport]) -> None: + """Let a later manifest answer an earlier one's unhashed entries. + + A failed run writes the files it could not read with no hash, and a rerun + that copies them writes a new manifest beside the old one rather than over + it. Without this the old manifest's unhashed entries fail `verify` forever. + + One direction only. An entry is superseded when it has no hash in its own + manifest, and a newer manifest for the same destination root lists the + same path with a hash that matched just now. A hashed entry is never + superseded, so a copy corrupted since an earlier run cannot hide behind a + later manifest; and an unhashed entry never answers anything. Directory + hash failures are untouched, so a manifest that has them still fails. + """ + for older in reports: + pending = [v for v in older.verdicts + if v.result is EntryResult.NO_CHECKSUM and v.superseded_by is None] + if not pending: + continue + for newer in reports: + if newer is older or not _same_root(newer, older): + continue + if not _is_newer(newer, older): + continue + matched = {_entry_key(v.path) for v in newer.verdicts + if v.result is EntryResult.OK and v.expected} + for verdict in pending: + if verdict.superseded_by is None and _entry_key(verdict.path) in matched: + verdict.superseded_by = newer.manifest + + def verify_tree(root: Path, **options) -> list[VerifyReport]: - """Verify every MHL found under `root`.""" - return [verify_manifest(manifest, **options) - for manifest in find_manifests(root)] + """Verify every MHL found under `root`, with `supersede` applied.""" + reports = [verify_manifest(manifest, **options) + for manifest in find_manifests(root)] + supersede(reports) + return reports diff --git a/tests/test_verdict_integrity.py b/tests/test_verdict_integrity.py index 2bf3513..fb2cf1d 100644 --- a/tests/test_verdict_integrity.py +++ b/tests/test_verdict_integrity.py @@ -21,6 +21,7 @@ from offloader import ascmhl, cli, engine, longpath, verify from offloader import reports as reports_mod +from offloader.hashers import get_algorithm, hash_file from offloader.models import FileStatus, MediaInfo, VerificationMode from offloader.reports.mhl import write_mhl @@ -154,6 +155,195 @@ def test_a_reused_report_dir_gets_a_numbered_manifest(tmp_path, capsys): assert "JobReport-2.mhl" in capsys.readouterr().err +# ------------------------- P2: a later manifest answers an earlier unhashed one + + +def _report_for(reports: list[verify.VerifyReport], manifest: Path) -> verify.VerifyReport: + return next(r for r in reports if r.manifest.resolve() == manifest.resolve()) + + +@pytest.mark.parametrize("rerun_flags", [[], ["--skip-existing"]]) +def test_a_successful_rerun_supersedes_the_failed_runs_unhashed_entry( + tmp_path, monkeypatch, capsys, rerun_flags): + """The failed run's manifest kept failing `verify` until the operator + deleted it by hand, even after a rerun had copied and verified the file.""" + card = _card(tmp_path, {"Clips/C001.mov": os.urandom(5000), + "Clips/BAD_C002.mov": os.urandom(6000)}) + dest = tmp_path / "dst" + with monkeypatch.context() as patch: + _fail_reads_of(patch, "BAD_C002.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 1 + rerun = _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl", *rerun_flags) + assert rerun == (4 if rerun_flags else 0) + capsys.readouterr() + + first = dest / "A001_Reports" / "JobReport.mhl" + second = dest / "A001_Reports-2" / "JobReport.mhl" + assert cli.main(["verify", str(dest), "--quiet"]) == 0 + out = capsys.readouterr().out + assert "safe to erase the source" in out + assert "superseded" in out and "BAD_C002.mov" in out + assert f"superseded by {second}" in out + + report = _report_for(verify.verify_tree(dest), first) + assert report.passed + assert [v.path.name for v in report.superseded] == ["BAD_C002.mov"] + assert "with no hash here matched a later manifest" in report.summary() + + +def test_a_corrupted_copy_is_caught_even_when_a_later_manifest_lists_it_unhashed( + tmp_path, monkeypatch, capsys): + """Supersession runs one way only: a hashed entry is never answered by a + later manifest, so a copy corrupted since cannot hide behind a rerun that + could not read its source.""" + card = _card(tmp_path, {"Clips/C001.mov": os.urandom(5000), + "Clips/C002.mov": os.urandom(6000)}) + dest = tmp_path / "dst" + with monkeypatch.context() as patch: + _fail_reads_of(patch, "C002.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 1 + _flip_one_byte(dest / "Clips" / "C001.mov") # same size, wrong bytes + with monkeypatch.context() as patch: + _fail_reads_of(patch, "C001.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 1 + capsys.readouterr() + + assert cli.main(["verify", str(dest), "--quiet"]) == 1 + out = capsys.readouterr().out + assert "do not erase" in out + assert "MISMATCH" in out and "C001.mov" in out + # The other direction still works in the same tree, so this is the guard + # holding rather than supersession being off. + assert "superseded" in out and "C002.mov" in out + + reports = verify.verify_tree(dest) + first = _report_for(reports, dest / "A001_Reports" / "JobReport.mhl") + second = _report_for(reports, dest / "A001_Reports-2" / "JobReport.mhl") + assert [(v.path.name, v.result) for v in first.failures] == [ + ("C001.mov", verify.EntryResult.MISMATCH)] + assert [v.path.name for v in first.superseded] == ["C002.mov"] + # The rerun's own unhashed entry is not answered by the older hashed one. + assert [(v.path.name, v.result) for v in second.failures] == [ + ("C001.mov", verify.EntryResult.NO_CHECKSUM)] + assert not second.superseded + + +def test_a_manifest_for_a_different_destination_root_does_not_supersede( + tmp_path, monkeypatch, capsys): + card = _card(tmp_path, {"Clips/C001.mov": os.urandom(5000), + "Clips/C002.mov": os.urandom(6000)}) + dest = tmp_path / "dst" + with monkeypatch.context() as patch: + _fail_reads_of(patch, "C001.mov") + _fail_reads_of(patch, "C002.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 1 + # A separate job whose root is dst/Clips hashes the same file on disk. + other = _card(tmp_path, {"C001.mov": (card / "Clips" / "C001.mov").read_bytes()}, + name="B001") + assert _offload("--source", str(other), "--dest", str(dest / "Clips"), + "--report", "mhl") == 0 + # And a rerun of the first job, for the first root, copies only C002. + with monkeypatch.context() as patch: + _fail_reads_of(patch, "C001.mov") + assert _offload("--source", str(card), "--dest", str(dest), + "--report", "mhl") == 1 + capsys.readouterr() + + first = dest / "A001_Reports" / "JobReport.mhl" + elsewhere = dest / "Clips" / "B001_Reports" / "JobReport.mhl" + rerun = dest / "A001_Reports-2" / "JobReport.mhl" + reports = verify.verify_tree(dest) + assert _report_for(reports, elsewhere).passed + report = _report_for(reports, first) + assert [v.path.name for v in report.failures] == ["C001.mov"] + assert [(v.path.name, v.superseded_by) for v in report.superseded] == [ + ("C002.mov", rerun)] + + assert cli.main(["verify", str(dest), "--quiet"]) == 1 + out = capsys.readouterr().out + assert f"superseded by {elsewhere}" not in out + assert f"superseded by {rerun}" in out + + +def _classic_manifest(path: Path, finished: str, entries: dict[str, str | None]) -> Path: + """A hand-written MHL 1.1 file, so the timestamps are exactly as given.""" + tag = get_algorithm("xxh3-64").mhl_tag + path.parent.mkdir(parents=True, exist_ok=True) + rows = "".join( + f"{relative}" + + (f"<{tag}>{digest}" if digest else "") + + "" + for relative, digest in entries.items()) + path.write_text( + '' + f"{finished}" + f"{finished}{rows}", + encoding="utf-8") + return path + + +SAME = "2026-10-11T12:00:00+00:00" +EARLIER = "2026-10-11T11:00:00+00:00" + + +@pytest.mark.parametrize( + "first_name, first_at, first_hashed, second_name, second_at, second_hashed, " + "passes", + [ + # Equal timestamps: the numbering decides, so -2 is the later run. + ("A001_Reports", SAME, False, "A001_Reports-2", SAME, True, True), + ("A001_Reports", SAME, True, "A001_Reports-2", SAME, False, False), + # Timestamps decide when they differ, whatever the names say. + ("A001_Reports", SAME, True, "A001_Reports-2", EARLIER, False, True), + ("A001_Reports", EARLIER, True, "A001_Reports-2", SAME, False, False), + # Equal timestamps and no shared numbering: nothing is known to be + # newer, so the unhashed entry keeps failing. + ("A001_Reports", SAME, False, "B001_Reports", SAME, True, False), + ("B001_Reports", SAME, True, "A001_Reports", SAME, False, False), + ]) +def test_which_manifest_is_newer_comes_from_its_timestamp_then_its_number( + tmp_path, capsys, first_name, first_at, first_hashed, second_name, + second_at, second_hashed, passes): + dest = tmp_path / "dst" + clip = dest / "Clips" / "C001.mov" + clip.parent.mkdir(parents=True) + clip.write_bytes(os.urandom(3000)) + digest = hash_file(clip, "xxh3-64") + relative = "../Clips/C001.mov" + _classic_manifest(dest / first_name / "JobReport.mhl", first_at, + {relative: digest if first_hashed else None}) + _classic_manifest(dest / second_name / "JobReport.mhl", second_at, + {relative: digest if second_hashed else None}) + + assert cli.main(["verify", str(dest), "--quiet"]) == (0 if passes else 1) + out = capsys.readouterr().out + assert ("superseded by" in out) is passes + + +def test_supersession_uses_the_numbered_manifest_name_in_one_folder(tmp_path): + """A reused `--report-dir` numbers the file rather than the folder.""" + dest = tmp_path / "dst" + clip = dest / "C001.mov" + dest.mkdir() + clip.write_bytes(os.urandom(3000)) + digest = hash_file(clip, "xxh3-64") + paperwork = dest / "paperwork_Reports" + later = _classic_manifest(paperwork / "JobReport-2.mhl", SAME, + {"../C001.mov": digest}) + earlier = _classic_manifest(paperwork / "JobReport.mhl", SAME, + {"../C001.mov": None}) + + reports = verify.verify_tree(dest) + assert all(r.passed for r in reports) + (verdict,) = _report_for(reports, earlier).superseded + assert verdict.superseded_by == later + + # --------------------------------------------- P1: unlistable directories fail From 44a941da0e7604edd82313b8d4ce0a235205ba8f Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:22:18 -0600 Subject: [PATCH 13/14] Add a replace-existing option to presets and the desktop app OffloadOptions.replace_existing could only be set with --replace-existing, and the engine's refusal pointed app users at that flag. Presets now carry replace_existing through to_options, to_dict and from_dict, the preset editor and Simple mode have a checkbox for it beside the other copy options, off by default, and the refusal names both the flag and the checkbox. --- CHANGELOG.md | 7 ++++ README.md | 2 +- docs/data-safety.md | 2 +- src/offloader/engine.py | 4 ++- src/offloader/gui/preset_editor.py | 17 +++++++++- src/offloader/gui/simple_mode.py | 14 ++++++-- src/offloader/presets.py | 6 ++++ tests/test_gui.py | 52 +++++++++++++++++++++++++++++- tests/test_presets.py | 15 +++++++++ 9 files changed, 112 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c748132..1cde0ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,6 +162,13 @@ project uses [semantic versioning][semver]. ### Added +- **The desktop app can set `replace_existing`.** The engine refuses to replace + an existing destination file whose content differs, and its message pointed + at `--replace-existing`, a flag the app could not pass. Presets now carry + `replace_existing` (saved and reloaded, off for presets written before it), + the preset editor and Simple mode have a *Replace existing files whose content + differs* checkbox under Options, off by default, and the refusal names both + the flag and the checkbox. - **"Start offload" says "Add to queue" when that is what it does.** Jobs run one at a time; while one is running the button enqueues, and the ready line says the job runs after the current one. diff --git a/README.md b/README.md index 622feb9..6b76486 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ offloader verify D:\video\080426\A001 | `--exclude GLOB` | extra filename pattern to skip; repeatable | | `--flat` | do not recreate the source folder structure; refused if two files would land on one path | | `--skip-existing` | skip files already present at matching size | -| `--replace-existing` | let a copy replace an existing destination file whose content differs; by default that file is kept and the copy fails (offload only) | +| `--replace-existing` | let a copy replace an existing destination file whose content differs; by default that file is kept and the copy fails (offload only; in the app, *Replace existing files whose content differs* under Options) | | `--paranoid` | read each source file twice and compare (offload only) | | `--retries N` | attempts per failing read on a transient error (default 3, 1 disables) | | `--retry-wait SECONDS` | pause before the first retry, backing off after (default 2) | diff --git a/docs/data-safety.md b/docs/data-safety.md index cf20455..f12968b 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -270,7 +270,7 @@ folder used. | Report writer crashing | Caught; a bad PDF never changes a file's status. Every failure is recorded on the job, which then reads `Verified, reports failed` rather than `Verified`; the GUI lists the failures when the job finishes and the CLI prints them and exits `5`. | | A source folder that cannot be listed | The job fails, naming the folder, and the CLI exits `1`. Its manifests carry an unhashed entry for the folder, so `verify` fails too. It used to be dropped silently and the job read Verified. | | A file deleted after the scan | That file fails with "source vanished after scan"; the rest of the job carries on and its paperwork is written. | -| A different card with the same folder layout | A finished copy never replaces an existing destination file whose content differs. The existing file is hashed and compared; identical content is replaced (that changes nothing), different content fails that file with the reason and keeps the existing copy. `--replace-existing` overrides this. | +| A different card with the same folder layout | A finished copy never replaces an existing destination file whose content differs. The existing file is hashed and compared; identical content is replaced (that changes nothing), different content fails that file with the reason and keeps the existing copy. `--replace-existing` (in the app, *Replace existing files whose content differs*, off by default) overrides this. | | A source that already holds `_Reports` | Reports go to `_Reports-2` (or the next free number) with a warning; the copied folder is left as copied. | | Same-named clips in different folders | Thumbnail files are named by a hash of each clip's relative path, so one clip's frames never replace another's in the report. | | Corrupt config file | Treated as empty. Config must never stand between someone and their card. | diff --git a/src/offloader/engine.py b/src/offloader/engine.py index 9f113c6..159b0a8 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -1197,7 +1197,9 @@ def _existing_file_refusal(target: Path, partial: Path, source_checksum: str, what a re-offload of the same card does. Anything else, or anything that cannot be read to find out, is refused. """ - advice = "use --replace-existing to overwrite it" + # Engine code stays free of Qt, so the app's checkbox is named literally. + advice = ("use --replace-existing, or the app's \"Replace existing files " + "whose content differs\" option, to overwrite it") try: if longpath.stat(target).st_size != longpath.stat(partial).st_size: return ("an existing file with different content is already there; " diff --git a/src/offloader/gui/preset_editor.py b/src/offloader/gui/preset_editor.py index d161b45..42936aa 100644 --- a/src/offloader/gui/preset_editor.py +++ b/src/offloader/gui/preset_editor.py @@ -42,6 +42,16 @@ "the one fault no checksum can see, because the checksum is taken from what " "the read returned.\n\nCosts a second full pass over the card." ) +#: The engine's refusal message names this label, so it is quoted there too: +#: change both together (a test holds them to each other). +REPLACE_EXISTING_LABEL = "Replace existing files whose content differs" +REPLACE_EXISTING_TOOLTIP = ( + "Lets the job overwrite an existing destination file whose content differs " + "from the file being copied.\n\nOff by default: such a file is kept and " + "that copy fails, because a second card with the same folder layout would " + "otherwise replace the first card's verified copies, and the first card " + "may already have been formatted. Identical files are replaced either way." +) def _form() -> QFormLayout: @@ -155,6 +165,9 @@ def __init__(self, preset: Preset | None = None, parent: QWidget | None = None) self._skip.setToolTip( "Compares size only, never contents. A speed option, not a safety " "one — do not use it on a tree whose integrity is in question.") + self._replace = QCheckBox(REPLACE_EXISTING_LABEL) + self._replace.setChecked(self._source.replace_existing) + self._replace.setToolTip(REPLACE_EXISTING_TOOLTIP) self._paranoid = QCheckBox(PARANOID_LABEL) self._paranoid.setChecked(self._source.paranoid) self._paranoid.setToolTip(PARANOID_TOOLTIP) @@ -184,7 +197,8 @@ def __init__(self, preset: Preset | None = None, parent: QWidget | None = None) copying.addRow("Verification", self._verification) copying.addRow("Exclude", self._excludes) copying.addRow("Options", - column(self._preserve, self._skip, self._paranoid)) + column(self._preserve, self._skip, self._replace, + self._paranoid)) paperwork = _form() paperwork.addRow("Reports", row(*report_row)) @@ -236,6 +250,7 @@ def _accept(self) -> None: naming_template=self._naming.text().strip() or "{card}", preserve_structure=self._preserve.isChecked(), skip_existing=self._skip.isChecked(), + replace_existing=self._replace.isChecked(), paranoid=self._paranoid.isChecked(), excludes=excludes, logo=Path(logo_text) if logo_text else None, diff --git a/src/offloader/gui/simple_mode.py b/src/offloader/gui/simple_mode.py index ca9f227..8785dd0 100644 --- a/src/offloader/gui/simple_mode.py +++ b/src/offloader/gui/simple_mode.py @@ -24,7 +24,13 @@ from ..presets import Preset from ..reports import WRITERS from ..volumes import volume_label -from .preset_editor import PARANOID_LABEL, PARANOID_TOOLTIP, VERIFICATION_LABELS +from .preset_editor import ( + PARANOID_LABEL, + PARANOID_TOOLTIP, + REPLACE_EXISTING_LABEL, + REPLACE_EXISTING_TOOLTIP, + VERIFICATION_LABELS, +) from .widgets import DestinationList, SourceDropZone, button, column, label, row @@ -86,6 +92,8 @@ def __init__(self, parent: QWidget | None = None) -> None: self._preserve = QCheckBox("Recreate the source folder structure") self._preserve.setChecked(True) + self._replace = QCheckBox(REPLACE_EXISTING_LABEL) + self._replace.setToolTip(REPLACE_EXISTING_TOOLTIP) self._paranoid = QCheckBox(PARANOID_LABEL) self._paranoid.setToolTip(PARANOID_TOOLTIP) @@ -98,7 +106,8 @@ def __init__(self, parent: QWidget | None = None) -> None: form.addRow("Verification", self._verification) form.addRow("Thumbnails", self._thumbnails) form.addRow("Reports", row(*report_row)) - form.addRow("Options", column(self._preserve, self._paranoid)) + form.addRow("Options", column(self._preserve, self._replace, + self._paranoid)) self._start = button("Start offload", accent=True) self._start.clicked.connect(self._start_clicked) @@ -197,6 +206,7 @@ def build_preset(self) -> Preset: reports=[key for key, box in self._reports.items() if box.isChecked()], preserve_structure=self._preserve.isChecked(), paranoid=self._paranoid.isChecked(), + replace_existing=self._replace.isChecked(), ) def _start_clicked(self) -> None: diff --git a/src/offloader/presets.py b/src/offloader/presets.py index 698840a..a24ca16 100644 --- a/src/offloader/presets.py +++ b/src/offloader/presets.py @@ -47,6 +47,9 @@ class Preset: skip_existing: bool = False #: Read every source file twice and compare. For irreplaceable material. paranoid: bool = False + #: Let a finished copy replace an existing destination file whose content + #: differs. Off by default; see `OffloadOptions.replace_existing`. + replace_existing: bool = False excludes: list[str] = field(default_factory=list) naming_template: str = DEFAULT_TEMPLATE retry_attempts: int = 3 @@ -113,6 +116,7 @@ def to_options(self, job_name: str | None = None) -> OffloadOptions: retry=RetryPolicy(attempts=max(1, self.retry_attempts), delay=max(0.0, self.retry_wait)), paranoid=self.paranoid, + replace_existing=self.replace_existing, ) # ---------------------------------------------------------------- codec @@ -128,6 +132,7 @@ def to_dict(self) -> dict: "preserve_structure": self.preserve_structure, "skip_existing": self.skip_existing, "paranoid": self.paranoid, + "replace_existing": self.replace_existing, "excludes": list(self.excludes), "naming_template": self.naming_template, "retry_attempts": self.retry_attempts, @@ -197,6 +202,7 @@ def as_list(key): preserve_structure=bool(value("preserve_structure", True)), skip_existing=bool(value("skip_existing", False)), paranoid=bool(value("paranoid", False)), + replace_existing=bool(value("replace_existing", False)), excludes=[e for e in as_list("excludes") if isinstance(e, str)], naming_template=str(value("naming_template", DEFAULT_TEMPLATE)), retry_attempts=as_int("retry_attempts", 3), diff --git a/tests/test_gui.py b/tests/test_gui.py index cd81ea0..ed7013e 100644 --- a/tests/test_gui.py +++ b/tests/test_gui.py @@ -315,6 +315,55 @@ def test_the_preset_editor_round_trips_the_second_read(qapp): assert editor.result_preset.paranoid is False +def test_simple_mode_offers_replace_existing_and_it_reaches_the_engine(qapp): + panel = SimpleModePanel() + assert panel.build_preset().replace_existing is False + assert panel.build_preset().to_options().replace_existing is False + + panel._replace.setChecked(True) + assert panel.build_preset().to_options().replace_existing is True + + +def test_the_preset_editor_round_trips_replace_existing(qapp): + from offloader.gui.preset_editor import PresetEditor + + assert not PresetEditor(Preset(name="p"))._replace.isChecked() + + editor = PresetEditor(Preset(name="Ingest", replace_existing=True)) + assert editor._replace.isChecked() + assert "content differs" in editor._replace.toolTip() + assert "Off by default" in editor._replace.toolTip() + + editor._replace.setChecked(False) + editor._accept() + assert editor.result_preset.replace_existing is False + + editor = PresetEditor(Preset(name="Ingest")) + editor._replace.setChecked(True) + editor._accept() + assert editor.result_preset.to_options().replace_existing is True + + +def test_the_refusal_names_the_checkbox_as_well_as_the_flag(qapp, tmp_path): + """An operator in the app was told to use a CLI flag it cannot pass.""" + from offloader.gui.preset_editor import REPLACE_EXISTING_LABEL + + relative = "DCIM/IMG_0001.CR3" + cards = {} + for name, data in (("cardA", b"A" * 2048), ("cardB", b"B" * 2048)): + path = tmp_path / name / relative + path.parent.mkdir(parents=True) + path.write_bytes(data) + cards[name] = tmp_path / name + preset = Preset(name="p", destinations=[tmp_path / "dst"], thumbnail_count=0) + + assert engine.run(cards["cardA"], preset.to_options()).final_status == "Verified" + second = engine.run(cards["cardB"], preset.to_options()) + error = second.files[0].destinations[0].error + assert "--replace-existing" in error + assert REPLACE_EXISTING_LABEL in error + + def test_the_preset_editor_is_grouped_rather_than_one_flat_list(qapp): """Sixteen fields in a single column read as a wall. The sections are the difference between scanning for a setting and hunting for it.""" @@ -330,7 +379,8 @@ def test_the_preset_editor_is_grouped_rather_than_one_flat_list(qapp): # Regrouping a form is exactly the change that silently drops a field. for name in ("_name", "_color", "_destinations", "_algorithm", "_verification", "_thumbnails", "_naming", "_excludes", - "_logo", "_footer", "_preserve", "_skip", "_paranoid"): + "_logo", "_footer", "_preserve", "_skip", "_replace", + "_paranoid"): assert getattr(editor, name).parent() is not None, f"{name} is orphaned" diff --git a/tests/test_presets.py b/tests/test_presets.py index 510946d..478d84a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -121,6 +121,7 @@ def test_to_options_carries_settings_through(tmp_path: Path): preserve_structure=False, skip_existing=True, paranoid=True, + replace_existing=True, ) options = preset.to_options(job_name="A001") @@ -132,6 +133,7 @@ def test_to_options_carries_settings_through(tmp_path: Path): assert options.preserve_structure is False assert options.skip_existing is True assert options.paranoid is True + assert options.replace_existing is True assert "*.tmp" in options.excludes assert ".DS_Store" in options.excludes # defaults still applied @@ -207,3 +209,16 @@ def test_paranoid_survives_a_save_and_reload(tmp_path: Path): assert PresetStore(tmp_path / "presets.json").presets[0].paranoid is True # And a preset written before the option existed still loads. assert Preset.from_dict({"name": "old"}).paranoid is False + + +def test_replace_existing_survives_a_save_and_reload_and_defaults_off(tmp_path: Path): + store = PresetStore(tmp_path / "presets.json") + store.presets = [Preset(name="Ingest", destinations=[tmp_path / "d"], + replace_existing=True)] + store.save() + + assert PresetStore(tmp_path / "presets.json").presets[0].replace_existing is True + assert Preset(name="new").replace_existing is False + assert Preset(name="new").to_options().replace_existing is False + # A preset written before the option existed still loads, refusing. + assert Preset.from_dict({"name": "old"}).replace_existing is False From 3db321610589fae4c7ae326224bf38ed81fd7e75 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:28:45 -0600 Subject: [PATCH 14/14] Update test counts --- CONTRIBUTING.md | 2 +- README.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 59b77fd..55fcf55 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ pip install -e ".[dev]" `ffmpeg` and `ffprobe` on `PATH` are optional — the suite runs without them. ```sh -pytest # 579 tests, about 20s +pytest # 594 tests, about 20s pytest --fuzz # property tests at 3000 examples each, about 3 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index 6b76486..61d1d19 100644 --- a/README.md +++ b/README.md @@ -414,13 +414,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 579 tests, ~20s +pytest # 594 tests, ~20s pytest --fuzz # same suite, 3000 examples per property (~3 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -579 tests at 87% line coverage. They cover formatting against the reference's +594 tests at 87% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing,