disc/patch: leave failed reads NonTrimmed mid-multipass, not Unreadable
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.
This commit is contained in:
+29
-11
@@ -2654,9 +2654,17 @@ impl Disc {
|
|||||||
}
|
}
|
||||||
Err(_err) => {
|
Err(_err) => {
|
||||||
blocks_read_failed += 1;
|
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(
|
send_or_abort(
|
||||||
&pipe,
|
&pipe,
|
||||||
PatchItem::Unreadable {
|
PatchItem::NonTrimmed {
|
||||||
pos: bt_pos,
|
pos: bt_pos,
|
||||||
len: span,
|
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(
|
send_or_abort(
|
||||||
&pipe,
|
&pipe,
|
||||||
PatchItem::Unreadable {
|
PatchItem::NonTrimmed {
|
||||||
pos,
|
pos,
|
||||||
len: block_bytes,
|
len: block_bytes,
|
||||||
},
|
},
|
||||||
@@ -2960,25 +2979,24 @@ impl Disc {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Redundant second Unreadable mark — preserved
|
// Pair with the earlier NonTrimmed dispatch — same
|
||||||
// bit-for-bit from the pre-split loop (`record`
|
// bytes, same state. Pre-2026-05-11 this was a
|
||||||
// is idempotent for same-status replacement of
|
// second Unreadable mark; now it's NonTrimmed for
|
||||||
// the same range). Routes through the consumer
|
// the same reason: cross-pass retry survival.
|
||||||
// like every other state change.
|
|
||||||
send_or_abort(
|
send_or_abort(
|
||||||
&pipe,
|
&pipe,
|
||||||
PatchItem::Unreadable {
|
PatchItem::NonTrimmed {
|
||||||
pos,
|
pos,
|
||||||
len: block_bytes,
|
len: block_bytes,
|
||||||
},
|
},
|
||||||
)?;
|
)?;
|
||||||
tracing::info!(
|
tracing::info!(
|
||||||
target: "freemkv::disc",
|
target: "freemkv::disc",
|
||||||
phase = "patch_mapfile_record_unreadable",
|
phase = "patch_mapfile_record_nontrimmed",
|
||||||
pos,
|
pos,
|
||||||
block_bytes,
|
block_bytes,
|
||||||
consecutive_failures,
|
consecutive_failures,
|
||||||
"Mapfile record dispatched as Unreadable"
|
"Mapfile record dispatched as NonTrimmed (retry next pass)"
|
||||||
);
|
);
|
||||||
|
|
||||||
let pause_secs = if err.is_bridge_degradation() {
|
let pause_secs = if err.is_bridge_degradation() {
|
||||||
|
|||||||
+25
-6
@@ -70,14 +70,33 @@ pub(super) enum PatchItem {
|
|||||||
/// Producer exhausted retries on `[pos, pos+len)`. Consumer records
|
/// Producer exhausted retries on `[pos, pos+len)`. Consumer records
|
||||||
/// the range as `Unreadable`. No file write — the existing zero-fill
|
/// the range as `Unreadable`. No file write — the existing zero-fill
|
||||||
/// from sweep is preserved in place.
|
/// 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 },
|
Unreadable { pos: u64, len: u64 },
|
||||||
|
|
||||||
/// Producer hit the per-range skip limit and is leaving the
|
/// Producer marks `[pos, pos+len)` as `NonTrimmed`. Used for BOTH
|
||||||
/// remaining bytes as `NonTrimmed` for a future pass. CRITICAL:
|
/// the per-range skip-limit case (remaining bytes never tried) AND
|
||||||
/// this is not the same as `Unreadable` — sectors we never tried
|
/// individual sector failures (tried-but-failed within a pass).
|
||||||
/// stay hopeful. (See the comment at the skip-limit branch in
|
/// Both stay "hopeful" — a later pass retries them.
|
||||||
/// `Disc::patch`: ~36% of patch-marked Unreadable sectors are
|
///
|
||||||
/// actually readable on a later pass.) No file write.
|
/// 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 },
|
NonTrimmed { pos: u64, len: u64 },
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -655,12 +655,16 @@ fn test_disc_copy_marks_failed_ecc_blocks_as_nontrimmed() {
|
|||||||
|
|
||||||
// ── 9. PassProgress carries separate unreadable vs pending byte counts ─────
|
// ── 9. PassProgress carries separate unreadable vs pending byte counts ─────
|
||||||
//
|
//
|
||||||
// The video-damage-time display needs bytes_unreadable_total (confirmed dead)
|
// 2026-05-11 design call: Pass N never marks bytes as `Unreadable` mid-multipass —
|
||||||
// separate from bytes_pending_total (might still recover). This test verifies
|
// failed reads stay `NonTrimmed` so the next pass can retry them. The orchestrator
|
||||||
// that a Pass 2 with some confirmed failures produces correct field values.
|
// (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]
|
#[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 capacity_sectors: u32 = 128;
|
||||||
let total_bytes: u64 = capacity_sectors as u64 * SECTOR_SIZE as u64;
|
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.bytes_good, 0,
|
||||||
"pass2: still no good sectors (reader always fails)"
|
"pass2: still no good sectors (reader always fails)"
|
||||||
);
|
);
|
||||||
assert!(
|
// 2026-05-11 design: pass-level retries do NOT promote failed bytes
|
||||||
pass2.bytes_unreadable > 0,
|
// to Unreadable. Failed bytes stay NonTrimmed (pending) so a later
|
||||||
"pass2: some sectors confirmed unreadable"
|
// 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!(
|
// bytes_pending stays at total_bytes because everything still
|
||||||
pass2.bytes_pending < pass1.bytes_pending,
|
// failed and nothing got recovered or promoted out of pending.
|
||||||
"pass2: fewer pending sectors than pass1"
|
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_unreadable = last_unreadable.load(Ordering::Relaxed);
|
||||||
let observed_pending = last_pending.load(Ordering::Relaxed);
|
let observed_pending = last_pending.load(Ordering::Relaxed);
|
||||||
assert!(
|
assert_eq!(
|
||||||
observed_unreadable > 0,
|
observed_unreadable, 0,
|
||||||
"progress should report confirmed unreadable bytes"
|
"progress should report zero confirmed-unreadable mid-pass under the new design"
|
||||||
);
|
);
|
||||||
assert!(
|
assert!(
|
||||||
observed_pending == 0 || observed_pending < total_bytes,
|
observed_pending > 0,
|
||||||
"pending should shrink as sectors are confirmed unreadable"
|
"progress should report pending bytes as the reader keeps failing"
|
||||||
);
|
);
|
||||||
|
|
||||||
// Video damage time: unreadable / total * duration
|
// Video damage time: unreadable / total * duration
|
||||||
|
|||||||
Reference in New Issue
Block a user