Skip to content

Retry transient read failures at the failing chunk - #3

Open
owenpkent wants to merge 10 commits into
roadmap-trust-itemsfrom
retry-resume
Open

owenpkent wants to merge 10 commits into
roadmap-trust-itemsfrom
retry-resume

Conversation

@owenpkent

Copy link
Copy Markdown
Owner

Implements the roadmap item "Retry the source, not just the read": a transient read failure no longer restarts the whole file.

What changed

  • The read-ahead thread in _copy_fanout now retries a failing chunk in place: it reopens the source (after an I/O error the handle's buffered state cannot be trusted), seeks back to the last chunk boundary it delivered, and reads on. One marginal sector costs a re-read of 8 MiB, not of a 79 GB clip.
  • No hasher rewind turned out to be needed, which the roadmap had anticipated. The hashers only ever see chunks that were read successfully, so the running checksum state is already at the resume point.
  • The whole-file retry in run() stays as the outer fallback for everything the chunk retry cannot reach: opening a target, a write to a blipping network destination, or a chunk that never reads good within its attempts. retry.py is unchanged, as the roadmap specified.
  • A chunk-level recovery is still reported, aggregated into one warning per file so a card with many marginal sectors cannot flood the report. Backoff sleeps run in slices so pause and cancel still take effect promptly.

Tests

Suite is 412 tests (was 409), all passing, ruff clean. New tests assert:

  • A mid-file failure causes exactly one reopen with a seek to the failed chunk boundary, the first chunk is never re-read, and the destination verifies byte-for-byte.
  • A chunk-level recovery still produces the "source may be failing" warning.
  • Exhausted chunk retries fall back to a whole-file restart, total attempts stay bounded at attempts x attempts, and no partial file is left behind.
  • Existing guarantees hold: permanent errors are never retried, progress is not double-counted, no partials survive.

Existing test fakes grew seek/close because the reader now manages handles directly instead of a with block.

Docs

docs/data-safety.md documented the old restart-the-file behaviour and is rewritten; ROADMAP moves the item to Done; CHANGELOG entry added; README flag table and CLI help no longer say "per file"; docs/performance.md's robocopy comparison notes the retry now resumes at the failing chunk.

A transient read no longer restarts the whole file. The read-ahead
thread reopens the source, seeks back to the last chunk it delivered
and reads on; the hashers only ever see chunks that were read
successfully, so the running checksum needs no rewind and one marginal
sector costs a re-read of 8 MiB rather than of the whole clip.

The whole-file retry stays as the outer fallback for everything the
chunk retry cannot reach: opening a target, a write to a blipping
network destination, or a chunk that never reads good within its
attempts. A chunk-level recovery is still reported as a job warning,
aggregated per file so a card with many marginal sectors cannot flood
the report.
The CLI help and the README flag table still described --retries as
attempts per file, and the robocopy comparison in docs/performance.md
did not mention that the adopted retry now resumes at the failing
chunk.
The security review of this branch confirmed the integrity guarantees
hold across the chunk retry, and surfaced two things worth recording
rather than fixing:

- The retry budget is per chunk, so a card with many marginal sectors
  accrues attempts x chunks reads in the worst case, not attempts x
  attempts. That is deliberate recovery behaviour and now stated in
  data-safety.md and the changelog, pinned by a test that a shared
  per-file budget would fail, and the exhaustion test no longer
  overstates its bound.
- A source modified mid-copy, or swapped between a retry's close and
  reopen, was never protected against by any single-pass copy tool,
  including this one before the chunk retry. It is now listed under
  'What is still not protected'.
@owenpkent

Copy link
Copy Markdown
Owner Author

Security review of this branch found no vulnerabilities. The integrity guarantees hold across the chunk retry: the resume offset counts only bytes actually delivered, the hashers and destination writes consume one stream, a shrunken or swapped-shorter source is caught by the size check, and the retry tally has no concurrent access (the reader thread is joined before it is read).

Two observations from the review are addressed in 4a08ef7 by documenting and pinning them rather than changing behaviour:

  • The retry budget is per chunk, so a card with many marginal sectors accrues attempts x chunks reads in the worst case. Deliberate recovery behaviour; now stated in data-safety.md, pinned by a new test that a shared per-file budget would fail.
  • A source modified mid-copy (or swapped between a retry's close and reopen) yields a copy that matches what was read, not any one version of the source. True of any single-pass copy tool and of this one before the chunk retry; now listed under "What is still not protected".

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the chunk retry implementation and its integration with whole-file recovery. I did not find an additional actionable defect introduced by this diff.

The resume offset advances only after a successful read; reopened handles seek back to that offset; the source and destination hashers consume only delivered chunks. The per-chunk attempt counter resets after success, while exhausted chunk retries deliberately retain the documented outer file-restart fallback. Recovered reads remain visible in the per-file warning, and a whole-file restart resets both progress and the chunk tally.

Validation on 4a08ef7: 56 passed across test_retry.py, test_data_safety.py, and test_control.py. The existing comment already covers the same-size source-change limitation and multiplied retry budget; neither is treated as a new finding here. This review did not exercise failing physical media or a live network disconnect.

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4a08ef7 again for chunk offsets, retry budgeting, warning aggregation, and the interaction with whole-file recovery.

No additional defect found in this diff. The read offset advances only after a successful chunk, a recovered chunk does not rehash the prefix, and exhausting the chunk budget still reaches this branch's documented whole-file fallback. That last behavior differs deliberately from the Exhausted path in #2, so resolving the overlap between those PRs should retain an explicit choice and its tests.

Validation: 4 targeted regressions passed on this head: mid-file resume, recovery warning, exhausted-chunk fallback, and independent attempt budgets for successive marginal chunks. This used injected read failures on Linux, without physical media or a live network drop.

@owenpkent
owenpkent changed the base branch from main to roadmap-trust-items October 9, 2026 00:10

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additional actionable defect found in this incremental diff.

The current diff principally adds responsive retry backoff and verifies independent per-chunk budgets. I traced retryability, reopen/seek placement, Exhausted propagation, progress accounting and JobControl waiting through the engine and tests. This assessment follows the current diff rather than the older original description.

Validation: all 40 retry tests passed in the exact-head detached worktree. Current-head CI has successful supported-OS, minimum-Python, ffmpeg, property-soak and distribution jobs. The diff check passes.

Limits: synthetic error injection and controlled waits, without a live network disconnect or failing card. The parent #2 paranoid-reread cancellation finding belongs to that parent change and is not duplicated here.

Pinned scope: declared base f9d11e3717b3ed988ee3fbc50e4243f78e4c4e3f, merge base f9d11e3717b3ed988ee3fbc50e4243f78e4c4e3f, head 3a688ba2c4f32aec7c7c9c82fa408fa1dc679e99. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant