diff --git a/CHANGELOG.md b/CHANGELOG.md index a022452..6d24ef6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ project uses [semantic versioning][semver]. its budget unspent, and the failure closed the whole-file retry as well. A reader that is briefly off the bus now costs one attempt of the chunk's own budget, and the chunk resumes at its own offset. +- **A cancel during a chunk's retry backoff waited the backoff out.** The wait + between chunk attempts was one uninterruptible sleep, so on a card failing + over a stretch a cancel or pause landed only after it. The wait is now taken + in short slices that check for both. - **Originals filed under a folder called `Proxy` were reported as belonging to another clip.** The classification read the folder name alone, so a card that happens to file camera originals there had every one of them treated as a diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2cb1dfc..03b5ad1 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 # 453 tests +pytest # 476 tests pytest --fuzz # property tests at 3000 examples each, about 2 min ruff check src tests pytest --cov=offloader --cov-report=term-missing diff --git a/README.md b/README.md index 2470f08..f7bd8bf 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ offloader verify D:\video\080426\A001 | `--flat` | do not recreate the source folder structure | | `--skip-existing` | skip files already present at matching size | | `--paranoid` | read each source file twice and compare (offload only) | -| `--retries N` | attempts per file on a transient read failure (default 3, 1 disables) | +| `--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 | @@ -207,6 +207,46 @@ assumed, now stated for any large data rather than only camera originals. It is deliberately not a sync tool — no two-way reconciliation, conflict resolution or partial-file updates. See [`ROADMAP.md`](ROADMAP.md). +## Compared with robocopy + +robocopy moves bytes fast; Offloader proves the bytes arrived, and gives you +the paperwork to prove it again later. The overlap is "copy a tree to another +drive", but each is the wrong tool for the other's job. + +What Offloader does that robocopy cannot: + +- **Verification.** robocopy has no integrity checking (`/V` is verbose + logging, not verification): it trusts the OS write path. Offloader checksums + every byte as it is read and, with `--verify full`, evicts the page cache and + reads each copy back off the platter. A flaky USB bridge or failing cable + that corrupts data in transit passes robocopy and fails Offloader. +- **Manifests.** CSV, MHL and ASC MHL are written beside every copy, so anyone + can re-verify the tree months later without the source. robocopy leaves + nothing behind but a log. +- **One read, many destinations.** `--dest` repeats, so a slow card is read + once and fanned out. robocopy reads the source again for every destination. +- **An answer to the real question.** "Is it safe to erase the source?" + robocopy can only say it issued the writes. + +What robocopy does that Offloader deliberately will not (these are decisions, +recorded under "Not planned" in [`ROADMAP.md`](ROADMAP.md)): + +- **Mirroring and sync.** `/MIR`, deleting extras from the destination, + incremental reconciliation. Offloader is one-way and write-once because that + assumption is exactly what makes the "Verified" verdict meaningful, and a + tool that can delete from a destination is the wrong shape for one whose + verdict authorises erasing the source. +- **Metadata fidelity beyond timestamps.** NTFS ACLs, alternate data streams, + junctions (`/COPYALL`, `/SEC`, backup mode). Camera cards have none of these. + Replicating a server share with permissions intact is robocopy's job, and it + does it well. + +So the rule of thumb: replication or sync on trusted hardware, use robocopy. A +one-way transfer of data you cannot get back, use Offloader. (Using robocopy +*inside* Offloader as the copy loop was measured and rejected: it copies faster +but emits no checksums, so the verified workflow it implies costs two extra +passes over the data. See [`docs/performance.md`](docs/performance.md).) + ## The desktop app ```sh @@ -280,9 +320,10 @@ The short version: proves nothing about the device. - Empty files, and verifications that may have been served from cache, are reported as warnings rather than folded into a "Verified" verdict. -- Reads that fail for a transient reason are retried, and a file that only - succeeded on a later attempt is reported — a card that needs retries today is - a card to stop using. +- Reads that fail for a transient reason are retried at the failing chunk, so + one marginal sector costs a re-read of 8 MiB rather than of the whole clip. + A file that only succeeded on a later attempt is reported, because a card + that needs retries today is a card to stop using. - Destinations past Windows' 260-character limit use the extended-length path prefix. `offloader info` reports whether your machine needs it. @@ -350,13 +391,13 @@ what makes the report layer testable without moving bytes. ```sh pip install -e ".[dev]" -pytest # 453 tests +pytest # 476 tests pytest --fuzz # same suite, 3000 examples per property (~2 min) ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -477 tests at 86% line coverage. They cover formatting against the reference's +482 tests at 86% 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/data-safety.md b/docs/data-safety.md index 068d845..cc37533 100644 --- a/docs/data-safety.md +++ b/docs/data-safety.md @@ -203,6 +203,12 @@ 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. +- **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 + what was read rather than any one version of the source. The tool cannot + freeze the source; offload from media nothing else is writing to. (A swap + that changes the size is caught by the size check.) ## Marginal media @@ -227,6 +233,12 @@ handle re-established. Once a chunk has had every attempt the policy allows, the file is not started again from the top: that would only repeat the same attempts against the same fault. +The budget is per chunk, not per file: a card with several marginal sectors gets +`--retries` attempts at each one, the way a recovery tool would, rather than one +budget the whole file must share. On a badly failing card that costs time, never +integrity, so pause and cancel are honoured during the backoff waits rather than +after them. + A failed *write* does restart the whole file, because a write that fails part-way leaves the destination at a length the copy loop does not know. The partial is discarded and the progress it claimed is given back, so a retry diff --git a/docs/performance.md b/docs/performance.md index bb301c4..7d77a55 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -99,7 +99,9 @@ Two of the three have since been adopted; both are covered in machine that has `LongPathsEnabled` set. - **Retry on flaky media.** `/R` and `/W` retry a failing read, which matters with a marginal card or reader. *Since adopted* — `--retries` and - `--retry-wait`, restricted to errors with a plausible transient cause. + `--retry-wait`, restricted to errors with a plausible transient cause, and + 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. diff --git a/src/offloader/cli.py b/src/offloader/cli.py index c596596..87a4e61 100644 --- a/src/offloader/cli.py +++ b/src/offloader/cli.py @@ -171,8 +171,8 @@ def _common_options(parser: argparse.ArgumentParser) -> None: parser.add_argument("--exclude", action="append", default=[], metavar="GLOB", help="extra filename pattern to skip (repeatable)") parser.add_argument("--retries", type=int, default=3, metavar="N", - help="attempts per file when a read fails for a " - "transient reason (default: %(default)s, 1 disables)") + help="attempts per failing read when the error looks " + "transient (default: %(default)s, 1 disables)") parser.add_argument("--retry-wait", type=float, default=2.0, metavar="SECONDS", help="pause before the first retry, backing off after " "(default: %(default)s)") diff --git a/src/offloader/engine.py b/src/offloader/engine.py index c2ab180..aaab047 100644 --- a/src/offloader/engine.py +++ b/src/offloader/engine.py @@ -13,6 +13,7 @@ import queue import shutil import threading +import time from collections.abc import Callable, Iterable, Sequence from dataclasses import dataclass, field from pathlib import Path @@ -345,12 +346,27 @@ def recover() -> None: nonlocal stale stale = True + def back_off(pause: float) -> None: + # Sleep in slices so a pause or cancel is honoured while the + # backoff is waited out. A card failing over a stretch can + # spend several seconds per chunk here, and a cancel that only + # lands once the stretch is over is not much of a cancel. + deadline = time.monotonic() + pause + while not stop.is_set(): + if control is not None: + control.checkpoint() + remaining = deadline - time.monotonic() + if remaining <= 0: + return + time.sleep(min(0.2, remaining)) + while not stop.is_set(): if control is not None: control.checkpoint() try: chunk, attempts = retry_mod.call(read_one, retry, - before_retry=recover) + before_retry=recover, + sleep=back_off) except OSError as exc: if retry.enabled and retry_mod.is_transient(exc): raise retry_mod.Exhausted( diff --git a/tests/test_data_safety.py b/tests/test_data_safety.py index 8ecf39c..92f1af4 100644 --- a/tests/test_data_safety.py +++ b/tests/test_data_safety.py @@ -133,11 +133,14 @@ def __init__(self, handle): def read(self, _size=-1): raise OSError("The device is not ready") + def close(self): + self._handle.close() + def __enter__(self): return self def __exit__(self, *args): - self._handle.close() + self.close() def flaky_open(path, mode="r", *args, **kwargs): handle = real_open(path, mode, *args, **kwargs) diff --git a/tests/test_retry.py b/tests/test_retry.py index 0258374..4a73491 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -10,6 +10,8 @@ import builtins import errno +import threading +import time from pathlib import Path import pytest @@ -200,7 +202,7 @@ def __enter__(self): return self def __exit__(self, *args): - self._handle.close() + self.close() def _patch_source_reads(monkeypatch, card: Path, failures: dict, limit: int): @@ -293,7 +295,7 @@ def __enter__(self): return self def __exit__(self, *args): - self._handle.close() + self.close() def flaky_open(path, mode="r", *args, **kwargs): handle = real_open(path, mode, *args, **kwargs) @@ -332,11 +334,14 @@ def read(self, size=-1): attempts["n"] += 1 raise _os_error(errno.ENOENT) + def close(self): + self._handle.close() + def __enter__(self): return self def __exit__(self, *args): - self._handle.close() + self.close() def gone_open(path, mode="r", *args, **kwargs): handle = real_open(path, mode, *args, **kwargs) @@ -662,3 +667,65 @@ def test_a_single_recovered_sector_is_still_named_exactly(tmp_path: Path, recovered = [w for w in job.warnings if "recovered" in w] assert len(recovered) == 1, recovered assert "at byte 8192 on attempt 2" in recovered[0] + + +def test_each_marginal_chunk_gets_its_own_attempt_budget(tmp_path: Path, + monkeypatch): + """The budget is per chunk, not per file. Two chunks that each need a second + attempt under a two-attempt policy would exhaust a budget the whole file + shared; per chunk, each gets its own, the way a recovery tool would, and the + file copies without the whole-file retry ever being reached.""" + monkeypatch.setattr(engine, "CHUNK_SIZE", 4096) + card, payload = _chunked_card(tmp_path, 3) + real_open = builtins.open + seen: set[int] = set() + + def flaky_open(path, mode="r", *args, **kwargs): + handle = real_open(path, mode, *args, **kwargs) + try: + inside = Path(path).resolve().is_relative_to(card.resolve()) + except (OSError, ValueError): + inside = False + if inside and "r" in str(mode) and "b" in str(mode): + return _FlakySectors(handle, {0, 4096}, seen) + return handle + + monkeypatch.setattr(builtins, "open", flaky_open) + job = engine.run(card, _options( + tmp_path, retry=retry.RetryPolicy(attempts=2, delay=0))) + monkeypatch.undo() + + assert job.final_status == "Verified" + assert (tmp_path / "dest" / "A001_C001.mov").read_bytes() == payload + assert not any("copied on attempt" in w for w in job.warnings), job.warnings + recovered = [w for w in job.warnings if "recovered" in w] + assert len(recovered) == 1, recovered + assert "2 failed reads between byte 0 and byte 4096" in recovered[0] + + +def test_a_cancel_lands_during_the_backoff_not_after_it(tmp_path: Path, + monkeypatch): + """A card failing over a stretch spends most of its time waiting out + backoffs. A cancel pressed during one has to take effect then, not once the + wait is over, and must not spend another attempt on the way out.""" + monkeypatch.setattr(engine, "CHUNK_SIZE", 4096) + card, _payload = _chunked_card(tmp_path, 3) + log: list[int] = [] + _patch_bad_sector(monkeypatch, card, log, {"n": 0}, offset=4096, times=99) + + control = engine.JobControl() + timer = threading.Timer(0.3, control.cancel) + timer.start() + started = time.monotonic() + try: + engine.run(card, _options( + tmp_path, retry=retry.RetryPolicy(attempts=3, delay=10)), + control=control) + finally: + timer.cancel() + monkeypatch.undo() + elapsed = time.monotonic() - started + + assert elapsed < 5, f"the cancel waited out the backoff ({elapsed:.1f}s)" + assert log.count(4096) == 1, f"an attempt was spent after the cancel: {log}" + assert list((tmp_path / "dest").rglob(f"*{engine.PARTIAL_SUFFIX}")) == []