From 5ddbf43ab24cc0bb5e40636fd39198ba24b5ac22 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Sun, 10 May 2026 18:47:05 -0700 Subject: [PATCH] disc/patch: leave failed reads NonTrimmed mid-multipass, not Unreadable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User design call after watching Pass 2 mark ~20 KB as "Cosmetic" (permanently Unreadable) after just 10 retries within a single pass: "i think it's good or maybe until all passes are done. then it's gone." That contradicts what the multi-pass design promises a user. The project goal in CLAUDE.md is "recover 100% of readable data from any optical disc, automatically." Marking sectors Unreadable after a SINGLE pass's per-range retry budget gives up on sectors that subsequent passes might recover — drive reads are stochastic, the sector that fails 10 times in Pass 2 may succeed on attempt 1 in Pass 3 after temperature / bus state / prior-read patterns shift. The patch.rs doc comment already noted ~36% of patch-marked Unreadable sectors turned out to be readable in re-rip experiments. Three sites in `Disc::patch` were emitting `PatchItem::Unreadable` mid-pass: - backtrack hit damage (line ~2659) - all-retries-exhausted on a single LBA (line ~2846) - redundant second mark after the wedge-suspicion log (line ~2970) All three now emit `PatchItem::NonTrimmed` instead. Failed bytes stay "maybe" (NonTrimmed) so the next pass gets another shot. The per-range skip-limit (10) and per-pass wedge-threshold (50) still bound time-per-pass; they just no longer turn the bytes terminal. The `PatchItem::Unreadable` variant stays in the enum (with #[allow(dead_code)]) because the orchestrator-side end-of-recovery promotion will use it: autorip, after the final retry pass completes, scans the mapfile and promotes still-NonTrimmed → Unreadable. That promotion lands in a follow-up commit on the autorip side — separable from this libfreemkv change. Loss accounting unchanged: `bytes_pending + bytes_unreadable` is the "lost or pending" total that `abort_on_lost_secs` consults (disc/mod.rs:1327). Moving bytes from one bucket to the other mid-pass doesn't affect whether the rip would abort; it only affects display (UI shows "Maybe" vs "Cosmetic") and whether subsequent passes retry the bytes (the actual fix). Test update: `test_pass_progress_separates_unreadable_from_pending` was renamed to `test_pass2_leaves_failed_reads_as_pending_not_unreadable` and rewritten to assert the new invariant — Pass 2 leaves all failed bytes as bytes_pending (no mid-pass Unreadable promotion). Original assertions were checking the pre-design-call behavior. Precommit (cargo +1.86 fmt + clippy + test) green. --- src/disc/mod.rs | 40 +++++++++++++++++++------- src/disc/patch.rs | 31 ++++++++++++++++---- tests/integration_progress_and_halt.rs | 40 ++++++++++++++++---------- 3 files changed, 79 insertions(+), 32 deletions(-) diff --git a/src/disc/mod.rs b/src/disc/mod.rs index f107193..f239b37 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -2654,9 +2654,17 @@ impl Disc { } Err(_err) => { blocks_read_failed += 1; + // Leave NonTrimmed (not Unreadable) so a later + // pass gets another shot. Per the project goal + // — "recover 100% of readable data" — and the + // multi-pass design's promise: bytes stay + // Good-or-Maybe across passes; promotion to + // Unreadable is the orchestrator's job at + // end-of-recovery (final retry pass complete). + // Reference: 2026-05-11 design call. send_or_abort( &pipe, - PatchItem::Unreadable { + PatchItem::NonTrimmed { pos: bt_pos, len: span, }, @@ -2840,10 +2848,21 @@ impl Disc { } } - // All retries exhausted - mark as Unreadable + // All retries exhausted IN THIS PASS — leave NonTrimmed + // so a subsequent pass gets another shot. Bytes stay + // Good-or-Maybe across passes; only the orchestrator + // (autorip) promotes still-NonTrimmed → Unreadable + // after the FINAL retry pass completes. Reference: + // 2026-05-11 design call ("good or maybe until all + // passes are done, then it's gone"). Pre-fix the + // patch loop marked Unreadable here, which gave up + // on sectors that a later pass might have recovered + // (drive reads are stochastic — same sector that + // fails 10x in Pass 2 might succeed on attempt 1 in + // Pass 3 after the drive state has shifted). send_or_abort( &pipe, - PatchItem::Unreadable { + PatchItem::NonTrimmed { pos, len: block_bytes, }, @@ -2960,25 +2979,24 @@ impl Disc { } } - // Redundant second Unreadable mark — preserved - // bit-for-bit from the pre-split loop (`record` - // is idempotent for same-status replacement of - // the same range). Routes through the consumer - // like every other state change. + // Pair with the earlier NonTrimmed dispatch — same + // bytes, same state. Pre-2026-05-11 this was a + // second Unreadable mark; now it's NonTrimmed for + // the same reason: cross-pass retry survival. send_or_abort( &pipe, - PatchItem::Unreadable { + PatchItem::NonTrimmed { pos, len: block_bytes, }, )?; tracing::info!( target: "freemkv::disc", - phase = "patch_mapfile_record_unreadable", + phase = "patch_mapfile_record_nontrimmed", pos, block_bytes, consecutive_failures, - "Mapfile record dispatched as Unreadable" + "Mapfile record dispatched as NonTrimmed (retry next pass)" ); let pause_secs = if err.is_bridge_degradation() { diff --git a/src/disc/patch.rs b/src/disc/patch.rs index 5a6c3f8..00126da 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -70,14 +70,33 @@ pub(super) enum PatchItem { /// Producer exhausted retries on `[pos, pos+len)`. Consumer records /// the range as `Unreadable`. No file write — the existing zero-fill /// from sweep is preserved in place. + /// + /// Currently unused by `Disc::patch` itself (2026-05-11 design call: + /// patch never marks `Unreadable` mid-multipass; bytes stay + /// `NonTrimmed` so future passes get another shot at them). Kept + /// in the enum for the orchestrator-side end-of-recovery promotion + /// (autorip, after the final retry pass completes, promotes + /// still-NonTrimmed bytes to Unreadable). When that ships, this + /// becomes the variant the orchestrator emits to the same + /// PatchSink. + #[allow(dead_code)] Unreadable { pos: u64, len: u64 }, - /// Producer hit the per-range skip limit and is leaving the - /// remaining bytes as `NonTrimmed` for a future pass. CRITICAL: - /// this is not the same as `Unreadable` — sectors we never tried - /// stay hopeful. (See the comment at the skip-limit branch in - /// `Disc::patch`: ~36% of patch-marked Unreadable sectors are - /// actually readable on a later pass.) No file write. + /// Producer marks `[pos, pos+len)` as `NonTrimmed`. Used for BOTH + /// the per-range skip-limit case (remaining bytes never tried) AND + /// individual sector failures (tried-but-failed within a pass). + /// Both stay "hopeful" — a later pass retries them. + /// + /// CRITICAL: "NonTrimmed in pass N" does NOT mean "Unreadable + /// forever." Drive reads are stochastic: the same sector that + /// fails 10 times in Pass 2 may succeed on attempt 1 in Pass 3 + /// after temperature / bus state / prior-read patterns shift. + /// Pre-2026-05-11 patch marked individual failures Unreadable, + /// which gave up on sectors that subsequent passes could have + /// recovered (historical: ~36% of patch-marked Unreadable + /// sectors turned out to be readable in re-rip experiments). + /// Promotion to true Unreadable is the orchestrator's job, + /// applied once after all retry passes complete. NonTrimmed { pos: u64, len: u64 }, } diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index e920726..7870f3f 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -655,12 +655,16 @@ fn test_disc_copy_marks_failed_ecc_blocks_as_nontrimmed() { // ── 9. PassProgress carries separate unreadable vs pending byte counts ───── // -// The video-damage-time display needs bytes_unreadable_total (confirmed dead) -// separate from bytes_pending_total (might still recover). This test verifies -// that a Pass 2 with some confirmed failures produces correct field values. +// 2026-05-11 design call: Pass N never marks bytes as `Unreadable` mid-multipass — +// failed reads stay `NonTrimmed` so the next pass can retry them. The orchestrator +// (autorip) promotes still-NonTrimmed bytes to Unreadable after the FINAL retry +// pass completes. This test was rewritten from its pre-design-call shape (which +// asserted Pass 2 produced bytes_unreadable > 0) to verify the new invariant: +// pass-level retries keep failed bytes in `bytes_pending` so subsequent passes +// get more shots at them. #[test] -fn test_pass_progress_separates_unreadable_from_pending() { +fn test_pass2_leaves_failed_reads_as_pending_not_unreadable() { let capacity_sectors: u32 = 128; let total_bytes: u64 = capacity_sectors as u64 * SECTOR_SIZE as u64; @@ -734,24 +738,30 @@ fn test_pass_progress_separates_unreadable_from_pending() { pass2.bytes_good, 0, "pass2: still no good sectors (reader always fails)" ); - assert!( - pass2.bytes_unreadable > 0, - "pass2: some sectors confirmed unreadable" + // 2026-05-11 design: pass-level retries do NOT promote failed bytes + // to Unreadable. Failed bytes stay NonTrimmed (pending) so a later + // pass can retry. End-of-recovery promotion is an orchestrator + // concern (autorip), not the patch loop's. + assert_eq!( + pass2.bytes_unreadable, 0, + "pass2: Disc::patch never marks Unreadable mid-multipass — orchestrator promotes after final pass" ); - assert!( - pass2.bytes_pending < pass1.bytes_pending, - "pass2: fewer pending sectors than pass1" + // bytes_pending stays at total_bytes because everything still + // failed and nothing got recovered or promoted out of pending. + assert_eq!( + pass2.bytes_pending, total_bytes, + "pass2: failed bytes remain NonTrimmed for the next pass to retry" ); let observed_unreadable = last_unreadable.load(Ordering::Relaxed); let observed_pending = last_pending.load(Ordering::Relaxed); - assert!( - observed_unreadable > 0, - "progress should report confirmed unreadable bytes" + assert_eq!( + observed_unreadable, 0, + "progress should report zero confirmed-unreadable mid-pass under the new design" ); assert!( - observed_pending == 0 || observed_pending < total_bytes, - "pending should shrink as sectors are confirmed unreadable" + observed_pending > 0, + "progress should report pending bytes as the reader keeps failing" ); // Video damage time: unreadable / total * duration