Retry transient read failures at the failing chunk

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

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

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,

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

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.

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)")

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(

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)

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}")) == []