From cf13838f126304b904cde2c83fd61ed62bb9cf9c Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:07:08 -0700 Subject: [PATCH] =?UTF-8?q?recovery:=20audit=20fixes=20=E2=80=94=20live=20?= =?UTF-8?q?at-risk=20excludes=20NonTried,=20latency-gated=20wedge,=20drop?= =?UTF-8?q?=20dead=20block=20counters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Live located drilldown (sweep + patch progress snapshots) now excludes NonTried, matching the one-shot path. Including the unread remainder made main_at_risk_ms show ~full-movie at sweep start and melt to 0 as it progressed — unread is unknown, not damage. - Wedge abort now requires the failure to be FAST (< WEDGE_FASTFAIL_MS, 500ms) as well as wedge-family sense: a real uncorrectable sector on Hardware-error media spends ECC-recovery time before failing, so it no longer false-trips the fast-fail wedge abort. New regression test. - Removed the always-zero blocks_attempted/read_ok/read_failed/ unreadable_count from PatchOutcome + PatchLoopState (dead residue from the old grind loop; the HandlerScoreboard supersedes them) so the patch.done log no longer emits misleading zeros. --- src/disc/mod.rs | 6 --- src/disc/patch.rs | 25 +++-------- src/disc/section_recover.rs | 87 +++++++++++++++++++++++++++++++++---- src/disc/sweep.rs | 8 +++- 4 files changed, 91 insertions(+), 35 deletions(-) diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 571fba9..2d64d3b 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -2930,9 +2930,6 @@ impl Disc { tracing::info!( target: "freemkv::disc", phase = "patch_done", - blocks_attempted = pr.blocks_attempted, - blocks_read_ok = pr.blocks_read_ok, - blocks_read_failed = pr.blocks_read_failed, bytes_recovered = pr.bytes_recovered_this_pass, halted = pr.halted, wedged_exit = pr.wedged_exit, @@ -3870,9 +3867,6 @@ pub struct PatchOutcome { pub bytes_pending: u64, pub bytes_recovered_this_pass: u64, pub halted: bool, - pub blocks_attempted: u64, - pub blocks_read_ok: u64, - pub blocks_read_failed: u64, pub wedged_exit: bool, pub wedged_threshold: u64, } diff --git a/src/disc/patch.rs b/src/disc/patch.rs index 321b00d..f9af681 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -148,10 +148,11 @@ pub(super) enum PatchItem { /// Mapfile snapshot the sink republishes after every record so the /// producer can drive its stall / progress logic without holding the -/// mapfile lock for long. `bad_ranges` mirrors what -/// `Mapfile::ranges_with(&[NonTrimmed, Unreadable, NonScraped, NonTried])` -/// would return — same set the pre-split patch loop computed inline -/// for the progress callback. +/// mapfile lock for long. `bad_ranges` is the DAMAGE set +/// (`NonTrimmed + Unreadable + NonScraped`) — NOT NonTried, which is the unread +/// remainder, not damage. Including NonTried inflated the live located drilldown +/// (at-risk movie time + range count) with unread sectors; excluding it matches +/// the one-shot progress path. pub(super) struct SharedPatchState { pub stats: MapStats, pub bad_ranges: Vec<(u64, u64)>, @@ -169,7 +170,6 @@ impl SharedPatchState { SectorStatus::NonTrimmed, SectorStatus::Unreadable, SectorStatus::NonScraped, - SectorStatus::NonTried, ]); bad_ranges.truncate(Self::MAX_BAD_RANGES); Self { @@ -614,10 +614,6 @@ pub(super) fn build_outcome( tracing::info!( target: "freemkv::disc", phase = "patch.done", - blocks_attempted = state.blocks_attempted, - blocks_read_ok = state.blocks_read_ok, - blocks_read_failed = state.blocks_read_failed, - unreadable_count = state.unreadable_count, wedged_exit = state.wedged_exit, halted = state.halted, bytes_recovered = stats.bytes_good.saturating_sub(state.bytes_good_before), @@ -635,9 +631,6 @@ pub(super) fn build_outcome( bytes_pending: stats.bytes_pending, bytes_recovered_this_pass: stats.bytes_good.saturating_sub(state.bytes_good_before), halted: state.halted, - blocks_attempted: state.blocks_attempted, - blocks_read_ok: state.blocks_read_ok, - blocks_read_failed: state.blocks_read_failed, wedged_exit: state.wedged_exit, wedged_threshold, } @@ -651,10 +644,6 @@ pub(super) struct PatchLoopState { // Counters pub halted: bool, pub wedged_exit: bool, - pub blocks_attempted: u64, - pub blocks_read_ok: u64, - pub blocks_read_failed: u64, - pub unreadable_count: u64, // Clock seam: the handler chain reads wall time through this rather than // calling `Instant::now()` inline, so the per-handler deadline is driven by // an injectable clock and deterministic tests can wind it forward. @@ -697,10 +686,6 @@ impl PatchLoopState { Self { halted: false, wedged_exit: false, - blocks_attempted: 0, - blocks_read_ok: 0, - blocks_read_failed: 0, - unreadable_count: 0, now, bytes_good_before, total_bytes, diff --git a/src/disc/section_recover.rs b/src/disc/section_recover.rs index b702626..c9e9d97 100644 --- a/src/disc/section_recover.rs +++ b/src/disc/section_recover.rs @@ -77,6 +77,15 @@ const UNPRODUCTIVE_YIELD: u32 = 4; /// by `wedge_streak_persists_across_sections_for_tier1`. const WEDGE_ABORT_STREAK: u32 = 16; +/// A wedge-family failure only counts toward [`WEDGE_ABORT_STREAK`] if it came +/// back faster than this — the fast-fail wedge rejects a CDB in <100ms with no +/// recovery attempt, whereas a genuine uncorrectable sector on Hardware-error +/// media spends real time on ECC recovery before failing. Gating on latency stops +/// slow, real damage that happens to report a Hardware sense from false-tripping +/// the wedge abort. Generous (500ms) so a slow bus adds margin without admitting +/// a true fast-fail. +const WEDGE_FASTFAIL_MS: u64 = 500; + /// Where a handler left the section after its bounded attempt. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum HandlerOutcome { @@ -199,6 +208,7 @@ fn read_span( count >= 1 && pos % SECTOR == 0, "read_span requires a sector-aligned, >=1-sector span (pos={pos}, count={count})" ); + let read_started = (ctx.now)(); let hit = match recovery_read(ctx.reader, ctx.decrypt_is_aacs, lba, count, buf, recovery) { Ok(_) => { ctx.sink.recovered(pos, &buf[..bytes]); @@ -206,17 +216,24 @@ fn read_span( } Err(e) if e.is_scsi_transport_failure() => ReadHit::Transport, Err(e) => { - // Wedge watch: a Hardware / IllegalRequest sense is the drive's - // fast-fail signature. Count consecutive ones; a sustained run means - // the firmware wedged (rejecting every CDB), so escalate to Transport - // and let the pass abort + spin-cycle rather than grind on. A medium - // error or any success below resets the streak — real bad sectors - // never trip it. - let wedge = e + // Wedge watch: the drive's fast-fail wedge REJECTS every CDB in <100ms + // without attempting recovery, with a Hardware / IllegalRequest sense. + // Both signals are required to count toward the streak: + // (1) wedge-family sense (Hardware / IllegalRequest), AND + // (2) the failure came back FAST (< WEDGE_FASTFAIL_MS). + // The latency gate is what keeps a genuine uncorrectable sector on + // Hardware-error media from false-tripping the wedge abort: a real + // ECC-recovery attempt takes far longer than a fast-fail rejection, so + // a SLOW Hardware-error is real damage (resets the streak, retried + // next pass), while only the fast rejections — the actual wedge — + // accumulate. A medium error or any success below also resets it. + let sense_is_wedge = e .scsi_sense() .map(|s| SenseFamily::from_sense_key(s.sense_key).is_wedge_family()) .unwrap_or(false); - if wedge { + let elapsed = (ctx.now)().duration_since(read_started); + let fast_fail = elapsed.as_millis() < WEDGE_FASTFAIL_MS as u128; + if sense_is_wedge && fast_fail { ctx.wedge_streak = ctx.wedge_streak.saturating_add(1); if ctx.wedge_streak >= WEDGE_ABORT_STREAK { ReadHit::Transport @@ -1141,6 +1158,60 @@ mod tests { ); } + #[test] + fn slow_hardware_error_media_does_not_false_trip_wedge_abort() { + // A genuine uncorrectable sector on Hardware-error media reports a + // wedge-FAMILY sense (IllegalRequest here) but comes back SLOW — the drive + // spent real time on ECC recovery before failing. That must NOT count + // toward the wedge abort (which targets the drive's <100ms fast-fail + // rejection). Each read here costs 600ms (> WEDGE_FASTFAIL_MS), so even a + // wholly-"wedge-sense" section never escalates to TransportFault — it just + // leaves the residue bad for the next pass, exactly like ordinary damage. + let (h, disc) = Harness::build(&[], None, Duration::from_millis(600)); + let mut disc = disc; + disc.wedge = (0..1000u32).collect(); + let mut sink = RecordSink::default(); + let now = h.now_fn(); + let mut ctx = HandlerCtx { + reader: &mut disc, + sink: &mut sink, + now: &now, + halt: None, + decrypt_is_aacs: false, + tick: None, + unproductive: 0, + wedge_streak: 0, + }; + let mut bad = SubRanges::from_section(0, 1000 * SECTOR); + let mut handlers: Vec> = vec![ + Box::new(Bisect), + Box::new(Jump), + Box::new(Linear { + reverse: true, + fast: true, + }), + Box::new(Linear { + reverse: false, + fast: true, + }), + ]; + let mut scoreboard = HandlerScoreboard::default(); + // Long per-handler deadline so the deadline (not the wedge) is never the + // reason a handler stops — we're isolating the wedge-escalation decision. + let out = run_handlers(&mut ctx, &mut handlers, &mut bad, &mut scoreboard, |_| { + (h.now_fn())() + Duration::from_secs(3600) + }); + assert_ne!( + out, + HandlerOutcome::TransportFault, + "slow (ECC-recovery) Hardware-error reads must NOT trip the fast-fail wedge abort" + ); + assert_eq!( + ctx.wedge_streak, 0, + "slow wedge-family reads must not accumulate the streak" + ); + } + #[test] fn wedge_streak_persists_across_sections_for_tier1() { // Tier 1 is only TWO handlers, so one wedged section builds at most diff --git a/src/disc/sweep.rs b/src/disc/sweep.rs index 414abc4..1361329 100644 --- a/src/disc/sweep.rs +++ b/src/disc/sweep.rs @@ -190,11 +190,17 @@ impl Sink for SweepSink { } WorkItem::StatsRequest => { let stats = self.map.stats(); + // DAMAGE only — NOT NonTried. NonTried is the unread remainder + // ahead of the sweep head, not damage; including it made the live + // located drilldown (at-risk movie time + range count) treat the + // whole unread disc as confirmed damage, so at sweep start it + // showed ~full-movie at-risk and melted to 0 as the sweep + // progressed. Matches the one-shot progress path, which already + // excludes NonTried. let bad_ranges = self.map.ranges_with(&[ SectorStatus::NonTrimmed, SectorStatus::Unreadable, SectorStatus::NonScraped, - SectorStatus::NonTried, ]); // Best-effort: drop on backpressure; producer's cache // stays current enough.