From d65b776a8e1199dc830be3d67785b7a79fc3ab9a Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Tue, 30 Jun 2026 19:55:37 -0700 Subject: [PATCH] patch: replace grind-until-wedge loop with bounded handler chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass-N recovery is now a chain of time-bounded recovery handlers instead of one monolithic per-range loop that could grind the front of a bad range for up to 30 min, wedge the drive, and abort the whole pass. A bad range is a SubRanges set; recovery is an ordered list of SectionHandlers (Linear{reverse,fast} covering back/forward x fast/slow, and Bisect). A coordinator runs each handler with a hard per-handler deadline: a handler recovers what it can (removing it from the still-bad set) and hands the rest to the next handler; whatever is still bad after the chain becomes NonTrimmed residue and we move on to the next range. Guarantees, now structural rather than bolted-on: - never hangs: every handler is deadline-bounded; the loop always drains to recovered-or-residue. - always moves on: a range that cannot be finished leaves residue and advances; only a genuine transport fault or user halt ends the pass. - extensible: a new recovery idea is one SectionHandler impl added to the chain; a proven-ineffective one is removed. The engine never changes. Removes ~1.9k lines of the old inner loop (watchdogs, skip escalation, NOT_READY grind, wedge counters) and their tests. fast_capture is now inert (the chain supersedes it); breadth-first ordering becomes a future scheduler concern. New module: disc/section_recover.rs (8 fixture tests, injectable clock — bounded/never-hang proven without touching a drive). Two A/B tests updated to the chain's strictly-better recovery counts. --- src/disc/mod.rs | 1 + src/disc/patch.rs | 2402 +++-------------------------------- src/disc/section_recover.rs | 738 +++++++++++ tests/passn_handler_ab.rs | 93 +- 4 files changed, 918 insertions(+), 2316 deletions(-) create mode 100644 src/disc/section_recover.rs diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 76cf722..57cde4e 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -16,6 +16,7 @@ mod extract; pub mod mapfile; mod patch; pub mod read_error; +mod section_recover; mod sweep; pub mod verify; diff --git a/src/disc/patch.rs b/src/disc/patch.rs index 7f08a0c..99ca206 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -58,6 +58,45 @@ use crate::error::{Error, Result}; use crate::io::pipeline::{Flow, Sink}; use super::mapfile::{self, MapStats, Mapfile, SectorStatus}; +use super::section_recover::{ + Bisect, HandlerCtx, HandlerOutcome, Linear, RecoverySink, SectionHandler, run_handlers, +}; + +/// Wall-clock budget one recovery handler gets on a section before the chain +/// moves to the next idea (#55). Tight and bounded — this is what guarantees a +/// pass never hangs: a handler that can't shrink the still-bad set within this +/// window returns, the next handler tries a different idea, and whatever is +/// still bad becomes NonTrimmed residue so recovery advances to the next range. +/// Replaces the old 1800 s/range + 3600 s/pass grind budgets on the live path. +const PER_HANDLER_BUDGET_SECS: u64 = 60; + +/// Bridges the decoupled [`RecoverySink`] a handler writes to onto the live +/// patch consumer pipe: each recovered span becomes a [`PatchItem::Recovered`] +/// the consumer thread seeks + writes + records `Finished`. `recovered` can't +/// return an error (the trait is infallible so handlers stay simple), so a +/// pipe-closed / halt error is captured in `err` and surfaced by the caller +/// after `run_handlers` returns. +struct PatchRecoverySink<'a> { + pipe: &'a Pipeline, + err: Option, +} + +impl RecoverySink for PatchRecoverySink<'_> { + fn recovered(&mut self, pos: u64, buf: &[u8]) { + if self.err.is_some() { + return; + } + if let Err(e) = send_or_abort( + self.pipe, + PatchItem::Recovered { + pos, + buf: buf.to_vec(), + }, + ) { + self.err = Some(e); + } + } +} /// Item the producer hands to the patch consumer. One per per-sector /// recovery decision. @@ -303,70 +342,10 @@ use super::{Disc, DiscTitle, PatchOptions, PatchOutcome, bytes_bad_in_title}; use crate::io::pipeline::Pipeline; use crate::sector::SectorSource; -// Pass-N tunables. Hoisted to module scope so helpers (extracted from -// the original `Disc::patch` body) can reference them without inheriting -// the function's local-const scope. -// Mirror of sweep path (read_error.rs NOT_READY_MAX_RETRIES = 3): cap -// per-LBA NOT_READY retries so a persistently-not-ready disc cannot burn -// up to RANGE_BUDGET_CAP_SECS per range on a single LBA. -const NOT_READY_MAX_RETRIES_PER_LBA: u32 = 3; /// Cooldown between patch ranges that actually grinded (dropped to the slow /// recovery speed). Lets the drive settle before the next range re-enters at max /// speed. Gated on "grinded" so a many-small-range pass doesn't stall on it. const INTER_RANGE_COOLDOWN_SECS: u64 = 10; -const BRIDGE_DEGRADATION_PAUSE_SECS: u64 = 10; -const POST_FAILURE_PAUSE_SECS: u64 = 1; -const CONSECUTIVE_FAIL_LONG_PAUSE: u64 = 5; -const CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD: u64 = 10; -// Adaptive batching: climb back to `initial_batch` after this many -// consecutive clean single-sector successes. -// Wedge-family (HARDWARE_ERROR / ILLEGAL_REQUEST) cooldown and abort -// thresholds — see `handle_read_failure` below for context. -// Single source of truth lives in `disc::read_error` so this cannot -// drift from `ZONE_ENTRY_COOLDOWN_SECS`. -const WEDGE_FAMILY_COOLDOWN_SECS: u64 = crate::disc::read_error::ZONE_ENTRY_COOLDOWN_SECS; -const WEDGE_ABORT_THRESHOLD: u32 = 16; -// Whole-pass stall watchdog: bytes_good must increase within -// STALL_SECS or the pass bails out as wedged. -const STALL_SECS: u64 = 3600; -// Per-range budget = sectors_in_range × SECONDS_PER_SECTOR, capped at -// RANGE_BUDGET_CAP_SECS. See `Disc::patch` block-comment for details. -const SECONDS_PER_SECTOR: u64 = 25; -const RANGE_BUDGET_CAP_SECS: u64 = 1800; -const MAX_SKIPS_PER_RANGE: u32 = 10; -// Pass-N damage-window / skip tunables. -const PASSN_DAMAGE_WINDOW: usize = 16; -// Single source of truth lives in `disc::read_error` so the patch loop's -// `compute_damage_skip` cannot drift from `ReadCtx::for_patch()`'s -// `damage_threshold_pct`. See `PATCH_DAMAGE_THRESHOLD_PCT` for context. -// (v0.20.8 release: the larger "route Pass-N MEDIUM/NOT_READY through -// handle_read_error" unification was attempted and backed out — the -// patch loop's size-aware `range_remaining/4` skip cap has no -// equivalent in `handle_read_error::JumpAhead`, and routing through -// the unified handler would regress the size-aware-skip A/B fixture -// in `tests/passn_handler_ab.rs`. Pulling the threshold into the -// shared constant is the safe, behavior-preserving first step.) -const PASSN_DAMAGE_THRESHOLD_PCT: usize = crate::disc::read_error::PATCH_DAMAGE_THRESHOLD_PCT; -const PASSN_SKIP_SECTORS_BASE: u64 = 32; -const PASSN_SKIP_SECTORS_CAP: u64 = 4096; -const PASSN_ESCALATION_RESET_GOOD: u32 = 4; - -/// Probe-offset escalation: returns a per-probe skip distance in -/// sectors of `PASSN_SKIP_SECTORS_BASE << (3 × idx)` (i.e. multiplies -/// by 8 per index), capped at `PASSN_SKIP_SECTORS_CAP`. Used by the -/// wedge-vs-bad-sector probe to -/// scatter its sample LBAs across the failing region rather than -/// hammering the same neighborhood. -pub(super) fn skip_sectors_for_probe(idx: usize) -> u64 { - let escalation = (idx.saturating_mul(3)).min(u32::MAX as usize) as u32; - // Saturating shift: a large `idx` would overflow a fixed-width shift - // (32 << 60 = 2^65), so fall back to the cap instead of panicking - // (debug) or wrapping to 0 (release). - PASSN_SKIP_SECTORS_BASE - .checked_shl(escalation) - .unwrap_or(PASSN_SKIP_SECTORS_CAP) - .min(PASSN_SKIP_SECTORS_CAP) -} /// Send a `PatchItem` and translate a `SendError` (consumer thread died /// / panicked) into a library error so the caller propagates cleanly. @@ -671,40 +650,18 @@ pub(super) struct PatchLoopState { pub blocks_read_ok: u64, pub blocks_read_failed: u64, pub unreadable_count: u64, - pub wedge_count: u32, pub work_done: u64, - // Per-range scratch (reset at each range boundary) - pub consecutive_failures: u64, - pub consecutive_skips_without_recovery: u32, - pub consecutive_good_since_skip: u32, - pub last_skip_from: Option, - pub skip_count: u32, - pub damage_window: Vec, - // Per-LBA NOT_READY retry cap (mirrors sweep NOT_READY_MAX_RETRIES=3). - // Reset whenever the current LBA changes (i.e. the cursor advances to - // a new sector). NOT_READY retries that push past NOT_READY_MAX_RETRIES_PER_LBA - // fall through to normal failure handling (NonTrimmed + cursor advance). - pub not_ready_retries_per_lba: u32, - pub not_ready_lba: Option, - // Stall tracking + // Progress baseline for the region-exit "bytes recovered" log. pub bytes_good_last: u64, - pub stall_start: std::time::Instant, - pub range_start: std::time::Instant, - pub range_bytes_good: u64, - // Clock seam: the watchdog reads wall time through this rather than calling - // `Instant::now()` inline, so deterministic tests can advance a fake clock to - // prove the stall/range timeouts trip. Production uses `Instant::now` - // (see `PatchLoopState::new`), so behaviour is byte-identical. + // 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. pub now: fn() -> std::time::Instant, - // Adaptive batch - pub current_batch: u16, - // Snapshot at construction — these stay constant for the whole pass + // Snapshot at construction — these stay constant for the whole pass. pub bytes_good_before: u64, - pub bytes_good_start: u64, #[allow(dead_code)] pub total_bytes: u64, pub initial_batch: u16, - pub recovery: bool, pub work_total: u64, } @@ -713,7 +670,6 @@ impl PatchLoopState { bytes_good_before: u64, total_bytes: u64, initial_batch: u16, - recovery: bool, work_total: u64, ) -> Self { // Production clock: the real monotonic wall clock. @@ -721,25 +677,21 @@ impl PatchLoopState { bytes_good_before, total_bytes, initial_batch, - recovery, work_total, std::time::Instant::now, ) } - /// Like `new`, but with an injectable monotonic clock. The watchdog reads - /// time exclusively through `now`, so a test can wind a fake clock forward to - /// drive the stall/range timeouts deterministically. `new` passes - /// `Instant::now`, so the production loop is unchanged. + /// Like `new`, but with an injectable monotonic clock so a test can wind a + /// fake clock forward to drive the per-handler deadline deterministically. + /// `new` passes `Instant::now`, so the production path is unchanged. pub(super) fn new_with_clock( bytes_good_before: u64, total_bytes: u64, initial_batch: u16, - recovery: bool, work_total: u64, now: fn() -> std::time::Instant, ) -> Self { - let t0 = now(); Self { halted: false, wedged_exit: false, @@ -747,994 +699,35 @@ impl PatchLoopState { blocks_read_ok: 0, blocks_read_failed: 0, unreadable_count: 0, - wedge_count: 0, work_done: 0, - consecutive_failures: 0, - consecutive_skips_without_recovery: 0, - consecutive_good_since_skip: 0, - last_skip_from: None, - skip_count: 0, - damage_window: Vec::with_capacity(PASSN_DAMAGE_WINDOW), - not_ready_retries_per_lba: 0, - not_ready_lba: None, bytes_good_last: bytes_good_before, - stall_start: t0, - range_start: t0, - range_bytes_good: bytes_good_before, now, - current_batch: initial_batch, bytes_good_before, - bytes_good_start: bytes_good_before, total_bytes, initial_batch, - recovery, work_total, } } } -/// Phase F: the Ok arm of the patch read result. Records the recovery, -/// dispatches the bytes to the consumer, runs the stall guard, and -/// (in reverse mode) runs the post-recovery backtrack that fills the -/// gap left by the most recent damage-skip. Returns `OuterAction` — -/// `Break` if the stall guard fired or backtrack hit a halt. -#[allow(clippy::too_many_arguments)] -pub(super) fn handle_read_success( - state: &mut PatchLoopState, - frame: &RangeFrame, - opts: &PatchOptions, - lba: u32, - count: u16, - pos: u64, - block_bytes: u64, - bytes: usize, - buf: &mut [u8], - read_duration_ms: u128, - pipe: &Pipeline, - shared: &Mutex, - reader: &mut R, -) -> Result { - state.blocks_read_ok += 1; - state.consecutive_failures = 0; - state.consecutive_good_since_skip += 1; - // A successful read breaks any in-progress wedge-family streak. - // wedge_count tracks CONSECUTIVE wedge-family (HARDWARE_ERROR / - // ILLEGAL_REQUEST) senses; a good read proves the drive is still - // responding so the streak is over. Without this reset, intermittent - // good reads interspersed with wedge-family failures accumulate - // wedge_count monotonically, triggering WEDGE_ABORT_THRESHOLD (16) - // prematurely on ranges that are actually making progress. - // Note: handle_read_failure already resets wedge_count on any - // non-wedge-family failure; this mirrors that for the success path. - state.wedge_count = 0; - // A successful read means this LBA is resolved; clear the NOT_READY - // per-LBA counter so any future failure at a different LBA starts fresh. - state.not_ready_retries_per_lba = 0; - state.not_ready_lba = None; - if state.consecutive_good_since_skip >= PASSN_ESCALATION_RESET_GOOD { - state.consecutive_skips_without_recovery = 0; - } - // Adaptive batch re-grow: the partner to handle_read_failure's - // halve-on-failure (bisect). On ANY successful read below - // initial_batch, double the batch — so it converges on the right - // granularity (tiny across damage, climbing back through clean runs) - // and a mid-size batch left by a bisect (8/4/2) still climbs back, - // not just count==1. Mirrors the sweep's read_error adaptive batch: - // halve down, double up. - if state.current_batch < state.initial_batch { - let grown = state - .current_batch - .saturating_mul(2) - .min(state.initial_batch); - if grown != state.current_batch { - tracing::debug!( - target: "freemkv::disc", - phase = "patch.batch.upscale", - from = state.current_batch, - to = grown, - lba, - "adaptive batching: clean read, doubling batch toward initial_batch" - ); - state.current_batch = grown; - } - } - state.damage_window.push(true); - if state.damage_window.len() > PASSN_DAMAGE_WINDOW { - state.damage_window.remove(0); - } - - tracing::info!( - target: "freemkv::disc", - phase = "patch.read.ok", - lba, - count, - bytes, - blocks_read_ok = state.blocks_read_ok, - consecutive_failures = state.consecutive_failures, - read_duration_ms, - range_idx = frame.range_idx, - pos, - "Read succeeded" - ); - // Plaintext: DecryptingSectorSource applied AACS / CSS in-place - // during the read_sectors call above. The pre-0.18 inline - // decrypt_sectors call lived here. - let write_start = std::time::Instant::now(); - tracing::debug!( - target: "freemkv::disc", - phase = "patch.write.start", - pos, - bytes, - "Starting ISO write" - ); - // Hand the recovered bytes off to the consumer: seek + write + - // mapfile.record(Finished) all happen on the consumer thread, - // so the producer can immediately move on to the next read while - // these bytes are being committed. - send_or_abort( - pipe, - PatchItem::Recovered { - pos, - buf: buf[..bytes].to_vec(), - }, - )?; - let write_duration_ms = write_start.elapsed().as_millis(); - tracing::info!( - target: "freemkv::disc", - phase = "patch.write.ok", - pos, - bytes, - write_duration_ms, - "ISO write succeeded" - ); - tracing::info!( - target: "freemkv::disc", - phase = "patch.record.ok", - pos, - block_bytes, - "Mapfile record dispatched" - ); - - // Stall guard: watch bytes_good (real progress), not pos - // (advances on skips). With the consumer running in its own - // thread, this read can lag by up to one item; the watchdog - // operates at STALL_SECS=3600 granularity so single-item lag is - // irrelevant. - let bytes_good_now = { - let g = shared - .lock() - .expect("PatchSink shared state mutex poisoned"); - g.stats.bytes_good - }; - if bytes_good_now > state.bytes_good_last { - state.stall_start = (state.now)(); - state.bytes_good_last = bytes_good_now; - } - let stall_elapsed = (state.now)().duration_since(state.stall_start); - if stall_elapsed > std::time::Duration::from_secs(STALL_SECS) { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.stall", - elapsed_secs = stall_elapsed.as_secs(), - bytes_good = bytes_good_now, - bytes_good_start = state.bytes_good_start, - "Patch stalled - no recovery for {}s, exiting pass", - STALL_SECS - ); - state.wedged_exit = true; - return Ok(OuterAction::Break); - } - - if let Some(skip_from) = state.last_skip_from.take() { - let backtrack_start = frame.block_end; - let backtrack_end = skip_from; - if opts.reverse && backtrack_start < backtrack_end { - tracing::info!( - target: "freemkv::disc", - phase = "patch.backtrack.start", - from_lba = pos, - to_lba = backtrack_end / 2048, - "recovered after skip; backtracking into gap" - ); - let mut bt_pos = backtrack_start; - while bt_pos < backtrack_end { - // Honor cancellation inside the backtrack inner loop. - // A long backtrack span can run minutes of single- - // sector reads; without this check the outer halt - // only takes effect when control returns to the - // per-range loop. - if let Some(h) = &opts.halt { - if h.load(std::sync::atomic::Ordering::Relaxed) { - return Err(crate::error::Error::Halted); - } - } - let span = - // Backtrack always at count=1: this path fills a - // gap that the main loop's damage-window skip - // jumped over. Using batched reads here would - // lump good sectors into NonTrimmed marks when - // the gap contains even one bad sector. Backtrack - // is rare enough that the per-sector cost is fine. - (backtrack_end - bt_pos).min(2048); - let bt_lba = (bt_pos / 2048) as u32; - let bt_count = (span / 2048) as u16; - let bt_bytes = bt_count as usize * 2048; - match reader.read_sectors(bt_lba, bt_count, &mut buf[..bt_bytes], state.recovery) { - Ok(_) => { - state.blocks_read_ok += 1; - // Plaintext via DecryptingSectorSource - // wrapping; same path the main read takes - // above. - send_or_abort( - pipe, - PatchItem::Recovered { - pos: bt_pos, - buf: buf[..bt_bytes].to_vec(), - }, - )?; - } - Err(_err) => { - state.blocks_read_failed += 1; - // Feed the damage window / wedge counter exactly as the - // main loop does, so a string of backtrack failures - // escalates skip distance and trips wedge detection - // rather than being silently under-counted. - state.damage_window.push(false); - if state.damage_window.len() > PASSN_DAMAGE_WINDOW { - state.damage_window.remove(0); - } - state.consecutive_failures += 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::NonTrimmed { - pos: bt_pos, - len: span, - }, - )?; - tracing::info!( - target: "freemkv::disc", - phase = "patch.backtrack.stop", - lba = bt_lba, - "backtrack hit damage; stopping" - ); - break; - } - } - state.work_done = state.work_done.saturating_add(span); - bt_pos += span; - } - } - } - Ok(OuterAction::Continue) -} - -/// Phase G: the Err arm of the patch read result. Handles the -/// adaptive-batch split decision (count > 1 failures don't count), -/// records the failure, applies the NOT_READY retry pause, dispatches -/// the NonTrimmed PatchItem, runs the stall guard, runs the probe- -/// wedge-vs-bad-sector diagnostic, classifies the wedge family and -/// picks the right cooldown pause, then sleeps. Returns the verdict -/// for the outer loop. -/// -/// Left as one large function (not sub-split into -/// `handle_not_ready_retry` / `probe_drive_responsive` / -/// `classify_wedge_family`) — that's the gold-plating tier deferred -/// to a future PR. -#[allow(clippy::too_many_arguments)] -pub(super) fn handle_read_failure( - state: &mut PatchLoopState, - frame: &RangeFrame, - opts: &PatchOptions, - err: &Error, - lba: u32, - count: u16, - pos: u64, - block_bytes: u64, - bytes: usize, - read_duration_ms: u128, - pipe: &Pipeline, - shared: &Mutex, - reader: &mut R, -) -> Result { - // Transport failure (status=0xFF: USB-bridge crash / disconnect) is not a - // recoverable bad sector — the bridge is wedged and every further read fails - // identically. Abort the pass immediately (symmetric with the sweep's - // read_error::handle_read_error AbortPass and single-pass mux's fill_extents), - // so autorip can drop and re-enumerate the bridge instead of hammering a - // crashed device sector-by-sector until the per-range watchdog expires. - // Checked before the batch-split below: a 0xFF on a batch read is still a - // bridge crash, not an ambiguous bad sector. - if err.is_scsi_transport_failure() { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.transport_fault", - lba, - count, - "transport failure (bridge crash) during patch — aborting pass" - ); - state.wedged_exit = true; - return Ok(FailureAction::BreakOuter); - } - - // Adaptive batching split decision: a batch-read failure - // (count > 1) is NOT a recorded failure. We don't yet know which - // sector in the batch was actually bad — could be one, could be - // many. BISECT: halve the batch and retry the SAME starting position. - // If the bad sector is in the upper half, the lower half now SUCCEEDS - // and recovers in BULK (one read, not N single reads); if it's in the - // lower half, we halve again. Either way the bad sector is isolated in - // O(log n) reads instead of single-stepping all n. Collapsing straight - // to count=1 (the pre-bisect behavior) single-walked the entire failed - // batch — on a mostly-good NonTrimmed range that is the dominant cost. - // Cursor stays put; loop continues. Invariants preserved: only a - // count==1 failure ever marks NonTrimmed, so no good sector is lumped - // into a bad mark; a batch failure still doesn't touch - // consecutive_failures / the damage window. - if count > 1 { - let halved = (count / 2).max(1); - tracing::info!( - target: "freemkv::disc", - phase = "patch.batch.split", - lba, - count, - from_batch = state.current_batch, - to_batch = halved, - err_code = err.code(), - "batch read failed, bisecting (halving) to isolate the bad sector" - ); - state.current_batch = halved; - return Ok(FailureAction::ContinueInner); - } - - state.blocks_read_failed += 1; - state.consecutive_good_since_skip = 0; - state.unreadable_count += 1; - - // Reset the per-LBA NOT_READY counter whenever the LBA changes. - // NOT_READY retries hold the cursor in place (ContinueInner), so the - // same LBA is re-attempted each iteration until we either succeed or - // exhaust NOT_READY_MAX_RETRIES_PER_LBA. A different LBA means the - // cursor has advanced (or we're on a new range), so start fresh. - if state.not_ready_lba != Some(lba) { - state.not_ready_retries_per_lba = 0; - state.not_ready_lba = Some(lba); - } - - // Check if this is a NOT_READY error that should be retried BEFORE - // incrementing consecutive_failures so NOT_READY retries do not - // count toward the wedge threshold (Fix 3: false-wedge prevention). - // Mirror of sweep path (read_error.rs handle_read_error): NOT_READY - // is capped at NOT_READY_MAX_RETRIES and not counted toward - // wedge/skip counters. - let sense = err.scsi_sense(); - - // ASC values (under NOT READY, sense_key 0x02) indicating temporary - // drive unresponsiveness worth retrying: - // 0x02 = LUN not ready, no reference position (mechanism still seeking) - // 0x03 = LUN not ready, manual intervention required - // 0x04 = LUN not ready, in process of becoming ready / initializing - // (Medium-not-present is ASC 0x3A, not handled here — nothing to retry.) - let is_not_ready_retryable = sense - .map(|s| s.sense_key == 0x02 && (s.asc == 0x02 || s.asc == 0x03 || s.asc == 0x04)) - .unwrap_or(false); - - // Only count toward consecutive_failures / wedge detector when this - // is NOT a retryable NOT_READY — those are handled below and return - // ContinueInner without advancing the cursor. - if !is_not_ready_retryable { - state.consecutive_failures += 1; - } - - tracing::warn!( - target: "freemkv::disc", - phase = "patch.read.fail", - lba, - count, - bytes, - blocks_read_failed = state.blocks_read_failed, - consecutive_failures = state.consecutive_failures, - read_duration_ms, - error_code = err.code(), - range_idx = frame.range_idx, - pos, - "Read failed" - ); - - // For retryable NOT_READY errors, pause longer and don't mark as Unreadable yet — - // but only up to NOT_READY_MAX_RETRIES_PER_LBA times per LBA. Beyond that, fall - // through to normal failure handling (NonTrimmed dispatch + cursor advance) so a - // persistently-not-ready disc cannot loop indefinitely on a single LBA and burn - // up to RANGE_BUDGET_CAP_SECS per range. Mirrors the sweep path cap in - // read_error.rs (NOT_READY_MAX_RETRIES = 3). - if is_not_ready_retryable { - if state.not_ready_retries_per_lba < NOT_READY_MAX_RETRIES_PER_LBA { - state.not_ready_retries_per_lba += 1; - tracing::info!( - target: "freemkv::disc", - phase = "patch.read.not_ready.retry", - lba, - not_ready_retries_per_lba = state.not_ready_retries_per_lba, - not_ready_max = NOT_READY_MAX_RETRIES_PER_LBA, - consecutive_failures = state.consecutive_failures, - err_asc = sense.map(|s| s.asc as u32).unwrap_or(0), - "NOT_READY with ASC in 0x02/0x03/0x04; pausing for drive recovery before retry" - ); - - // Extended pause for NOT_READY - let drive complete internal mechanical recovery. - // Use sleep_secs_or_halt so a halt token can interrupt the 15 s wait - // early (Fix 2: halt-responsive NOT_READY pause). - let pause_secs = 15u64; - tracing::debug!( - target: "freemkv::disc", - phase = "patch.read.not_ready.pause", - lba, - consecutive_failures = state.consecutive_failures, - pause_secs, - "Waiting for drive to become ready" - ); - super::sleep_secs_or_halt(pause_secs, opts.halt.as_ref()); - - // Check stall guard here — the NOT_READY retry path bypasses the - // normal failure path's stall guard, so total runtime could - // otherwise grow as num_ranges × RANGE_BUDGET_CAP_SECS (disc- - // controlled). (Fix 1: DoS prevention.) - let bytes_good_now = { - let g = shared - .lock() - .expect("PatchSink shared state mutex poisoned"); - g.stats.bytes_good - }; - if bytes_good_now > state.bytes_good_last { - state.stall_start = (state.now)(); - state.bytes_good_last = bytes_good_now; - } - let stall_elapsed = (state.now)().duration_since(state.stall_start); - if stall_elapsed > std::time::Duration::from_secs(STALL_SECS) { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.stall", - elapsed_secs = stall_elapsed.as_secs(), - bytes_good = bytes_good_now, - bytes_good_start = state.bytes_good_start, - "Patch stalled (NOT_READY path) - no recovery for {}s, exiting pass", - STALL_SECS - ); - state.wedged_exit = true; - return Ok(FailureAction::BreakOuter); - } - - // Don't mark as Unreadable yet - will retry on next iteration - state.damage_window.push(false); - if state.damage_window.len() > PASSN_DAMAGE_WINDOW { - state.damage_window.remove(0); - } - return Ok(FailureAction::ContinueInner); - } - - // Per-LBA cap exhausted: fall through to normal failure handling - // (NonTrimmed dispatch + cursor advance). The drive isn't coming - // back for this LBA in this pass; a later pass can retry. - tracing::warn!( - target: "freemkv::disc", - phase = "patch.read.not_ready.cap_exceeded", - lba, - not_ready_retries_per_lba = state.not_ready_retries_per_lba, - not_ready_max = NOT_READY_MAX_RETRIES_PER_LBA, - "NOT_READY cap exceeded for this LBA; falling through to normal failure handling" - ); - // Count toward consecutive_failures now that we're giving up on this LBA. - state.consecutive_failures += 1; - } - - // (Removed in 0.20.2) The previous code retried non-NOT_READY - // errors on encrypted discs with an "exponential backoff: 2s, 4s, - // 8s" comment — but `retry_count` was declared inside the per- - // iteration `Err` arm so it reset to 0 every iteration. The - // "MAX_NON_NOT_READY_RETRIES=3" budget actually fired exactly - // once (1s pause + 1 retry), then fell through to the NonTrimmed - // dispatch below. The block was a 100-line illusion. Cross-pass - // NonTrimmed retry (next pass gives the same sectors another - // shot) already covers the recovery case it was supposed to - // handle — and it gives the drive minutes between attempts - // instead of 1-8 seconds, which empirically matters for stochastic - // recovery on the BU40N. - - // 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::NonTrimmed { - pos, - len: block_bytes, - }, - )?; - - state.damage_window.push(false); - if state.damage_window.len() > PASSN_DAMAGE_WINDOW { - state.damage_window.remove(0); - } - - // Stall guard: check on failures too, not just successes - let bytes_good_now = { - let g = shared - .lock() - .expect("PatchSink shared state mutex poisoned"); - g.stats.bytes_good - }; - if bytes_good_now > state.bytes_good_last { - state.stall_start = (state.now)(); - state.bytes_good_last = bytes_good_now; - } - let stall_elapsed = (state.now)().duration_since(state.stall_start); - if stall_elapsed > std::time::Duration::from_secs(STALL_SECS) { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.stall", - elapsed_secs = stall_elapsed.as_secs(), - consecutive_failures = state.consecutive_failures, - bytes_good = bytes_good_now, - bytes_good_start = state.bytes_good_start, - "Patch stalled - no recovery for {}s, exiting pass", - STALL_SECS - ); - state.wedged_exit = true; - return Ok(FailureAction::BreakOuter); - } - - // Log every 10 failures or when approaching wedged threshold - if state.consecutive_failures % 10 == 0 || state.consecutive_failures >= opts.wedged_threshold { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.read.fail.count", - lba, - consecutive_failures = state.consecutive_failures, - wedged_threshold = opts.wedged_threshold, - "Failure count" - ); - } - - // Probe good sectors to differentiate wedge vs bad sector. - // `skip_sectors_for_probe` returns a SECTOR distance; scale to bytes - // before adding to `pos` (a byte offset). The previous code compared - // a sector count against `block_bytes` and added a sector count to a - // byte offset, so the only probe that ran landed back on the failing - // LBA — the responsive-vs-wedged heuristic never scattered. - if state.consecutive_failures >= 3 && state.consecutive_failures % 5 == 0 { - let probe_offsets_sectors: [u64; 3] = - [0, skip_sectors_for_probe(1), skip_sectors_for_probe(2)]; - let mut probes_ok = 0; - - for (probe_idx, &offset_sectors) in probe_offsets_sectors.iter().enumerate() { - // Honor cancellation inside the probe loop. Each probe - // read can block up to READ_RECOVERY_TIMEOUT_MS (60 s) on a - // wedged drive; 3 probes × 60 s = up to 180 s before a - // /api/stop is honored. Check the halt token before each - // probe so cancellation is bounded by one read, not the - // whole loop. - if let Some(h) = &opts.halt { - if h.load(std::sync::atomic::Ordering::Relaxed) { - return Err(crate::error::Error::Halted); - } - } - let offset = offset_sectors.saturating_mul(2048); - let probe_pos = pos.saturating_add(offset); - // Skip the zero-distance re-read until failures are well - // established (it just re-confirms the current LBA), and - // never probe past the end of the current bad range (the - // probe scatters sample LBAs across the failing region — - // `block_bytes`, one block, was the wrong bound and in the - // wrong units). - if probe_pos >= frame.end || (offset == 0 && state.consecutive_failures < 5) { - continue; - } - - let probe_lba = (probe_pos / 2048) as u32; - let probe_count = 1u16; - let mut probe_buf = [0u8; 2048]; - - match reader.read_sectors(probe_lba, probe_count, &mut probe_buf[..], state.recovery) { - Ok(_) => { - probes_ok += 1; - tracing::debug!( - target: "freemkv::disc", - phase = "patch.probe.ok", - lba = probe_lba, - offset_from_current = offset, - probe_idx, - "Probe read succeeded — drive responsive" - ); - } - Err(_) => { - tracing::debug!( - target: "freemkv::disc", - phase = "patch.probe.miss", - lba = probe_lba, - offset_from_current = offset, - probe_idx, - "Probe read failed" - ); - } - } - } - - if probes_ok > 0 { - tracing::info!( - target: "freemkv::disc", - phase = "patch.probe.responsive", - consecutive_failures = state.consecutive_failures, - probes_ok, - total_probes = 3, - lba, - range_idx = frame.range_idx, - "Drive responsive — bad sector cluster, not wedged" - ); - } else if probes_ok == 0 && state.consecutive_failures >= 10 { - // Heuristic suspicion of wedge — NOT the confirmed - // wedge_transition log that fires when the SCSI sense - // family flips into Hardware/IllegalRequest. This log - // just says "the local zone is fully bad" which could - // mean a real wedge OR a fully-bad cluster on a non- - // wedged drive. The wedge_skip handler in read_error.rs - // is what actually decides + acts. - tracing::warn!( - target: "freemkv::disc", - phase = "patch.probe.zone_bad", - consecutive_failures = state.consecutive_failures, - lba, - range_idx = frame.range_idx, - "patch zone fully bad (10+ failures, all probes failed); \ - not a wedge unless read_error.rs's wedge_transition also fires" - ); - } - } - - // (Removed in 0.20.2) Duplicate NonTrimmed dispatch. The earlier - // `send_or_abort(PatchItem::NonTrimmed)` already recorded the - // range. `Mapfile::record` is idempotent so it wasn't a - // correctness bug, but it doubled the consumer's per-failure work. - - // Wedge-family detection: HARDWARE_ERROR / ILLEGAL_REQUEST are - // the senses the BU40N's firmware fast-fail mode returns. When - // the drive is wedged, every subsequent read returns these in - // <100ms — exactly the rapid-retry cadence that bricks the drive - // further. Long cooldown (WEDGE_FAMILY_COOLDOWN_SECS, sourced from - // read_error::ZONE_ENTRY_COOLDOWN_SECS) gives the firmware - // breathing room to clear the fast-fail state. After - // WEDGE_ABORT_THRESHOLD consecutive wedge senses with no recovery, - // bail to autorip so it can eject + reload (the only thing that - // reliably clears a real wedge). - let is_wedge_family = err - .scsi_sense() - .map(|s| { - s.sense_key == crate::scsi::SENSE_KEY_HARDWARE_ERROR - || s.sense_key == crate::scsi::SENSE_KEY_ILLEGAL_REQUEST - }) - .unwrap_or(false); - - let pause_secs = if is_wedge_family { - state.wedge_count += 1; - tracing::warn!( - target: "freemkv::disc", - phase = "patch.wedge.family", - lba, - wedge_count = state.wedge_count, - wedge_abort_threshold = WEDGE_ABORT_THRESHOLD, - sense_key = err.scsi_sense().map(|s| s.sense_key as u32).unwrap_or(0), - "HARDWARE_ERROR / ILLEGAL_REQUEST sense — wedge family, applying long cooldown" - ); - if state.wedge_count >= WEDGE_ABORT_THRESHOLD { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.wedge.abort", - wedge_count = state.wedge_count, - WEDGE_ABORT_THRESHOLD, - "Drive appears wedged ({} consecutive wedge-family senses); aborting pass for autorip eject+reload", - state.wedge_count - ); - state.wedged_exit = true; - return Ok(FailureAction::BreakOuter); - } - WEDGE_FAMILY_COOLDOWN_SECS - } else if err.is_bridge_degradation() { - tracing::debug!( - target: "freemkv::disc", - phase = "patch.wedge.bridge_degradation", - lba, - consecutive_failures = state.consecutive_failures, - error = %err, - "bridge degradation; cooling down" - ); - BRIDGE_DEGRADATION_PAUSE_SECS - } else if state.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD { - CONSECUTIVE_FAIL_LONG_PAUSE - } else { - POST_FAILURE_PAUSE_SECS - }; - - // Any non-wedge-family read clears the wedge counter. - if !is_wedge_family { - state.wedge_count = 0; - } - - tracing::debug!( - target: "freemkv::disc", - phase = "patch.read.post_failure_pause", - lba, - consecutive_failures = state.consecutive_failures, - pause_secs, - "breathing room after failure" - ); - // Halt-responsive: a stop request must interrupt this pause rather than - // block for up to pause_secs (which escalates per failure), so /api/stop - // stays responsive during the most error-prone phase of a rip. - super::sleep_secs_or_halt(pause_secs, opts.halt.as_ref()); - Ok(FailureAction::Continue) -} - -/// Return value of [`handle_read_success`] and [`handle_read_failure`]: -/// tells the outer coordination loop whether to break out of the -/// `'outer` for-loop entirely (`Break`) or fall through to the -/// per-iteration damage-skip / progress logic (`Continue`). -pub(super) enum OuterAction { - /// Continue with the remaining iteration body (damage-skip, - /// progress dispatch, wedged-threshold check). - Continue, - /// Break out of the outer `'outer` loop. `state.halted` / - /// `state.wedged_exit` has already been set by the helper. - Break, -} - -/// Failure helper's outcome — distinguished from `OuterAction` because -/// the failure path has its own special "continue inner loop without -/// running per-iteration damage-skip / progress dispatch" verdict for -/// NOT_READY retries and adaptive-split decisions. -pub(super) enum FailureAction { - /// Run the per-iteration damage-skip / wedge-threshold / progress - /// logic, then loop. Same as `OuterAction::Continue`. - Continue, - /// Skip the per-iteration damage-skip / progress logic and `continue` - /// the inner loop directly. Used for the NOT_READY retry path - /// (don't advance cursor, retry same LBA next iteration) and the - /// adaptive batch-split decision (drop to count=1, retry). - ContinueInner, - /// Break out of the outer `'outer` loop. `state.wedged_exit` has - /// already been set. - BreakOuter, -} - /// Why [`PatchCtx::patch_region`] returned. The orchestrator -/// ([`PatchCtx::run`]) advances to the next bad range on `Completed` / -/// `SkipLimit` / `BudgetExceeded`, and ends the whole pass on `Wedged` / -/// `Halted` / `TransportFault` — for which the matching `state.halted` / -/// `state.wedged_exit` flag was already set, so `build_outcome` reports -/// it. (A pass also ends, by `?`-propagation, if a read inside the -/// region returns `Err(Halted)` from the backtrack inner loop.) +/// ([`PatchCtx::run`]) advances to the next bad range on `Completed` (the +/// handler chain always drains a section to recovered-or-residue, so there is +/// no per-range abort), and ends the whole pass only on `Halted` or +/// `TransportFault` — for which the matching `state.halted` / `state.wedged_exit` +/// flag was already set, so `build_outcome` reports it. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum RegionOutcome { - /// Walked (or converged on) the entire range. + /// Section drained: recovered what was readable, left the rest NonTrimmed. Completed, - /// Hit `MAX_SKIPS_PER_RANGE`; remaining bytes left NonTrimmed. - SkipLimit, - /// Per-range watchdog fired (`range_budget_secs` elapsed with no - /// forward progress). - BudgetExceeded, - /// Drive wedged — whole-pass stall guard, wedge-family abort, or the - /// consecutive-failure wedge threshold. `state.wedged_exit` is set. - Wedged, /// Halt requested — the halt token or the progress reporter. /// `state.halted` is set. Halted, - /// USB-bridge transport fault (status 0xFF): a dead bus, not a bad - /// sector. `state.wedged_exit` is set. + /// USB-bridge transport fault: a dead bus, not a bad sector. + /// `state.wedged_exit` is set. TransportFault, } -/// Per-range constants captured once when the outer loop enters a -/// range. Avoids threading `range_pos`, `range_size`, derived `end` / -/// `range_sectors` / `range_budget_secs` through every helper. The -/// only field that changes between iterations is `block_end` — the -/// per-iteration cursor; helpers that move it (damage-skip, advance) -/// take `&mut RangeFrame`. -pub(super) struct RangeFrame { - pub range_idx: usize, - pub range_pos: u64, - #[allow(dead_code)] - pub range_size: u64, - pub end: u64, - pub block_end: u64, - pub range_budget_secs: u64, - pub range_sectors: u64, -} - -/// Per-range watchdog: combines the elapsed-budget check with a -/// no-forward-progress check (reset `state.range_start` whenever -/// `bytes_good` advances). Returns `true` when the inner loop should -/// `break` out to the next range. Emits the same `patch_range_timeout` -/// / `patch_range_stall` warnings as the inline original. -pub(super) fn check_range_watchdog( - state: &mut PatchLoopState, - frame: &RangeFrame, - shared: &Mutex, -) -> bool { - // Refresh the forward-progress baseline FIRST, then do a single - // elapsed-vs-budget check. Reading bytes_good before the budget - // test means a range that committed a recovered sector since the - // previous tick resets its clock instead of being abandoned in the - // budget-boundary window. - let bytes_good_now = { - let g = shared - .lock() - .expect("PatchSink shared state mutex poisoned"); - g.stats.bytes_good - }; - if bytes_good_now > state.range_bytes_good { - state.range_bytes_good = bytes_good_now; - state.range_start = (state.now)(); - } - let range_elapsed = (state.now)().duration_since(state.range_start); - if range_elapsed.as_secs() >= frame.range_budget_secs { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.region.watchdog", - range_lba = frame.range_pos / 2048, - range_sectors = frame.range_sectors, - elapsed_secs = range_elapsed.as_secs(), - budget_secs = frame.range_budget_secs, - bytes_recovered = state.range_bytes_good.saturating_sub(state.bytes_good_before), - "Range stalled - moving to next range" - ); - return true; - } - false -} - -/// Phase D3: skip-limit reached for the current range. Emit the -/// `patch_skip_limit` warn and dispatch the appropriate NonTrimmed -/// PatchItem for the remaining (never-attempted) bytes. Caller breaks -/// the inner loop after this returns. -pub(super) fn handle_skip_limit( - state: &PatchLoopState, - frame: &RangeFrame, - opts: &PatchOptions, - pipe: &Pipeline, -) -> Result<()> { - tracing::warn!( - target: "freemkv::disc", - phase = "patch.skip.limit", - range_lba = frame.range_pos / 2048, - skip_count = state.skip_count, - "Skip limit reached - leaving remaining bytes NonTrimmed for next pass", - ); - // CRITICAL: don't mark sectors we NEVER ATTEMPTED as Unreadable. - // Only sectors we actually read+failed get the terminal `-` - // status. Sectors we jumped over are hopeful — the drive may read - // them on a later pass when state has evolved (cache, mechanical - // settle). 2026-05-07 dd-as-oracle test confirmed ~36% of patch- - // marked Unreadable sectors are actually readable. - if let Some((pos, len)) = - skip_limit_remainder(opts.reverse, frame.range_pos, frame.end, frame.block_end) - { - send_or_abort(pipe, PatchItem::NonTrimmed { pos, len })?; - } - Ok(()) -} - -/// The never-attempted remainder of a range when the skip limit is -/// reached, as `Some((pos, len))` or `None` if nothing is left. -/// -/// `block_end` is the per-iteration cursor. In reverse mode it moved -/// DOWN from `end` toward `range_pos`, so the attempted region is -/// `[block_end, end)` and the remainder is `[range_pos, block_end)`. In -/// forward mode it moved UP from `range_pos` toward `end`, so the -/// attempted region is `[range_pos, block_end)` and the remainder is -/// `[block_end, end)`. The pre-fix forward formula -/// `range_pos + (end - block_end)` was a mirror reflection that, once -/// `block_end` passed the midpoint, produced a start BELOW `block_end` -/// and overlapped the already-recovered region — downgrading Finished -/// sectors to NonTrimmed. -fn skip_limit_remainder( - reverse: bool, - range_pos: u64, - end: u64, - block_end: u64, -) -> Option<(u64, u64)> { - if reverse { - let len = block_end.saturating_sub(range_pos); - (len > 0).then_some((range_pos, len)) - } else { - let len = end.saturating_sub(block_end); - (len > 0).then_some((block_end, len)) - } -} - -/// Damage-cluster size-aware skip decision. Inspects `state.damage_window` -/// against the `PASSN_DAMAGE_THRESHOLD_PCT` threshold; if crossed, -/// advances `frame.block_end` by an escalating skip (capped at 1/4 of -/// the remaining bad range so a single jump can't blow past a good -/// middle). Returns `true` iff a skip was applied — caller then -/// suppresses the normal block-cursor advance. -pub(super) fn compute_damage_skip( - state: &mut PatchLoopState, - frame: &mut RangeFrame, - opts: &PatchOptions, - lba: u32, - _block_bytes: u64, -) -> bool { - let bad_count = state.damage_window.iter().filter(|&&b| !b).count(); - if !(state.damage_window.len() >= PASSN_DAMAGE_WINDOW - && bad_count * 100 / state.damage_window.len() >= PASSN_DAMAGE_THRESHOLD_PCT) - { - return false; - } - - // Size-aware cap: never skip more than 1/4 of the remaining bad - // range. A 100-sector bad range is really 25-bad + 50-good + 25- - // bad in disguise; a hardcoded MB-scale skip would leap over the - // entire thing and miss the good middle. Capping at - // range_remaining/4 forces convergence on the actual bad sub-zones. - let range_remaining_bytes = if opts.reverse { - frame.block_end.saturating_sub(frame.range_pos) - } else { - frame.end.saturating_sub(frame.block_end) - }; - let range_remaining_sectors = range_remaining_bytes / 2048; - let range_quarter = (range_remaining_sectors / 4).max(1); - let escalated = PASSN_SKIP_SECTORS_BASE - .checked_shl(state.consecutive_skips_without_recovery) - .unwrap_or(PASSN_SKIP_SECTORS_CAP) - .min(PASSN_SKIP_SECTORS_CAP); - let skip_sectors = escalated.min(range_quarter); - let skip_bytes = skip_sectors * 2048; - let new_block_end = if opts.reverse { - frame - .block_end - .saturating_sub(skip_bytes) - .max(frame.range_pos) - } else { - (frame.block_end + skip_bytes).min(frame.end) - }; - if new_block_end == frame.block_end { - return false; - } - tracing::info!( - target: "freemkv::disc", - phase = "patch.skip", - from_lba = lba, - skip_sectors, - escalation = state.consecutive_skips_without_recovery, - bad_pct = bad_count * 100 / state.damage_window.len(), - "damage cluster detected; skipping within range" - ); - let gap_bytes = if opts.reverse { - frame.block_end.saturating_sub(new_block_end) - } else { - new_block_end.saturating_sub(frame.block_end) - }; - state.work_done = state.work_done.saturating_add(gap_bytes); - state.last_skip_from = Some(frame.block_end); - frame.block_end = new_block_end; - state.consecutive_skips_without_recovery += 1; - state.skip_count += 1; - true -} - /// Per-pass coordination state for one `Disc::patch` run: the decrypting /// reader, the consumer pipe + its shared mapfile snapshot, the options, /// and the accumulating [`PatchLoopState`]. Bundling these lets the @@ -1743,9 +736,9 @@ pub(super) fn compute_damage_skip( /// functions threading a dozen arguments. `state` carries ACROSS ranges /// (counters, stall timers, NOT_READY/last-skip cursors); the per-range /// scratch inside it is reset at the top of each `patch_region`. -struct PatchCtx<'a, 'o, R: SectorSource + ?Sized> { +struct PatchCtx<'a, 'o> { disc: &'a Disc, - reader: &'a mut R, + reader: &'a mut dyn SectorSource, pipe: &'a Pipeline, shared: &'a Mutex, opts: &'a PatchOptions<'o>, @@ -1756,12 +749,9 @@ struct PatchCtx<'a, 'o, R: SectorSource + ?Sized> { /// Gated on "grinded" so a many-small-range pass doesn't stall on it. cooldown_pending: bool, state: PatchLoopState, - /// Recovery read buffer, sized to `initial_batch` sectors and reused - /// across reads so the per-iteration read doesn't reallocate. - buf: Vec, } -impl PatchCtx<'_, '_, R> { +impl PatchCtx<'_, '_> { /// Orchestrator (one pass): walk the ordered bad ranges. Apply the /// inter-range cooldown only after a range that grinded, then recover /// the range; stop the whole pass the moment a range reports @@ -1793,10 +783,8 @@ impl PatchCtx<'_, '_, R> { "region finished" ); match outcome { - RegionOutcome::Completed - | RegionOutcome::SkipLimit - | RegionOutcome::BudgetExceeded => {} - RegionOutcome::Wedged | RegionOutcome::Halted | RegionOutcome::TransportFault => { + RegionOutcome::Completed => {} + RegionOutcome::Halted | RegionOutcome::TransportFault => { break; } } @@ -1824,282 +812,102 @@ impl PatchCtx<'_, '_, R> { range_size_mb = range_size as f64 / 1_048_576.0, "entering patch range" ); - let end = range_pos + range_size; - let range_sectors = range_size / 2048; - let range_budget_secs = (range_sectors * SECONDS_PER_SECTOR).min(RANGE_BUDGET_CAP_SECS); - let mut frame = RangeFrame { - range_idx, - range_pos, - range_size, - end, - block_end: if self.opts.reverse { end } else { range_pos }, - range_budget_secs, - range_sectors, - }; - // Per-range reset (was the inline range-boundary block): a fresh - // range starts with an empty damage window, zeroed escalation / - // good / skip / wedge / failure counters, and the full initial - // batch. `current_batch` carries across ranges, so resetting it - // here stops a prior range's single-sector grind from starting - // this range slow. The range timer's forward-progress baseline is - // the CURRENT bytes_good (Fix 4 — NOT the pass-start value, else a - // prior range's recovery would refill this range's budget for - // free on its first watchdog tick). - self.state.damage_window.clear(); - self.state.consecutive_skips_without_recovery = 0; - self.state.consecutive_good_since_skip = 0; - self.state.range_start = (self.state.now)(); - self.state.range_bytes_good = { - let g = self - .shared - .lock() - .expect("PatchSink shared state mutex poisoned"); - g.stats.bytes_good - }; - self.state.skip_count = 0; - self.state.wedge_count = 0; - self.state.consecutive_failures = 0; - tracing::debug!( - target: "freemkv::disc", - phase = "patch.region.budget", - range_lba = range_pos / 2048, - range_sectors, - range_budget_secs, - "per-range time budget computed" - ); - - // Enter at MAX speed + the full initial batch: read the clean - // overshoot fast. `retried_once` flips on the first read failure - // (below), dropping to the slow recovery speed for the rest of - // the range and arming the inter-range cooldown. + // Enter at max read speed; a handler drops to the deep-recovery read + // itself via its `fast` flag. self.reader.set_speed(0xFFFF); - tracing::info!( - target: "freemkv::disc", - phase = "patch.speed", - range_lba = range_pos / 2048, - range_sectors, - speed = "0xFFFF", - "range entering at MAX read speed (drops to slow recovery on first failure)" - ); - self.state.current_batch = self.state.initial_batch; - let mut retried_once = false; - loop { - if let Some(ref h) = self.opts.halt { - if h.load(std::sync::atomic::Ordering::Relaxed) { - self.state.halted = true; - return Ok(RegionOutcome::Halted); - } - } + // The section's still-bad set. Handlers shrink it via `SubRanges::remove` + // as they recover spans; whatever survives the chain is this pass's + // residue. + let mut bad = SubRanges::from_section(range_pos, range_size); - if check_range_watchdog(&mut self.state, &frame, self.shared) { - return Ok(RegionOutcome::BudgetExceeded); - } + // The recovery-idea chain, cheapest first: fast reverse, fast forward, + // slow reverse, slow forward, then bisect for readable islands inside a + // mostly-dead range. Each is deadline-bounded; when one can't shrink the + // set the next tries a different idea. Adding an idea is one more entry + // here (#55). + let mut handlers: Vec> = vec![ + Box::new(Linear { + reverse: true, + fast: true, + }), + Box::new(Linear { + reverse: false, + fast: true, + }), + Box::new(Linear { + reverse: true, + fast: false, + }), + Box::new(Linear { + reverse: false, + fast: false, + }), + Box::new(Bisect), + ]; - if self.state.skip_count >= MAX_SKIPS_PER_RANGE { - handle_skip_limit(&self.state, &frame, self.opts, self.pipe)?; - return Ok(RegionOutcome::SkipLimit); - } + // Clock seam: handlers read wall time through this so tests can wind a + // fake clock (the same seam the pass uses for its own timing). + let now_ptr = self.state.now; + let now_fn = move || now_ptr(); - let (pos, block_bytes) = if self.opts.reverse { - if frame.block_end <= frame.range_pos { - return Ok(RegionOutcome::Completed); - } - let span = - (frame.block_end - frame.range_pos).min(self.state.current_batch as u64 * 2048); - (frame.block_end - span, span) - } else { - if frame.block_end >= frame.end { - return Ok(RegionOutcome::Completed); - } - let span = - (frame.end - frame.block_end).min(self.state.current_batch as u64 * 2048); - (frame.block_end, span) + let mut sink = PatchRecoverySink { + pipe: self.pipe, + err: None, + }; + + let outcome = { + let mut ctx = HandlerCtx { + reader: &mut *self.reader, + sink: &mut sink, + now: &now_fn, + halt: self.opts.halt.as_deref(), + decrypt_is_aacs: self.decrypt_is_aacs, }; - let lba = (pos / 2048) as u32; - let count = (block_bytes / 2048) as u16; - let bytes = count as usize * 2048; - self.state.blocks_attempted += 1; + run_handlers(&mut ctx, &mut handlers, &mut bad, |_bad| { + now_ptr() + std::time::Duration::from_secs(PER_HANDLER_BUDGET_SECS) + }) + }; - tracing::debug!( - target: "freemkv::disc", - phase = "patch.read.start", - lba, - count, - bytes, - attempt_num = self.state.blocks_attempted, - range_index = range_idx, - pos_byte = pos, - "starting sector read" - ); + // A pipe-closed / halt error captured while emitting recovered spans is + // fatal to the pass. + if let Some(e) = sink.err.take() { + return Err(e); + } - // Single-shot read (no inline retry — see the historical note - // in handle_read_failure). `recovery_read` widens a mid-unit - // AACS window to the aligned unit; otherwise it's a plain read. - // Fast-capture reads FAIL-FAST (no deep-recovery timeout): a block - // it can't read quickly is deferred to a granular pass anyway, so - // there's no point spending the drive's 60s recovery grind on it - // here — that just freezes the breadth-first sweep on a bad cluster - // (~25s/block). The granular passes (fast_capture = false) do the - // deep recovery on what's left. - let recovery = self.state.recovery && !self.opts.fast_capture; - let read_start = std::time::Instant::now(); - let read_result = recovery_read( - self.reader, - self.decrypt_is_aacs, - lba, - count, - &mut self.buf, - recovery, - ); - let read_duration_ms = read_start.elapsed().as_millis(); + // Everything still bad is this pass's residue: record NonTrimmed and MOVE + // ON to the next range. A later pass — or a future handler — gets another + // shot; the orchestrator promotes still-NonTrimmed to Unreadable only + // after the final pass completes. + for &(pos, len) in bad.ranges() { + send_or_abort(self.pipe, PatchItem::NonTrimmed { pos, len })?; + } - match read_result { - Ok(_) => { - match handle_read_success( - &mut self.state, - &frame, - self.opts, - lba, - count, - pos, - block_bytes, - bytes, - &mut self.buf, - read_duration_ms, - self.pipe, - self.shared, - self.reader, - )? { - // Break == the whole-pass stall guard fired - // (wedged_exit already set). - OuterAction::Break => return Ok(RegionOutcome::Wedged), - OuterAction::Continue => {} - } - } - Err(err) => { - // Fast-capture pass: don't grind this block. Mark it - // NonTrimmed for a later granular pass and move on, so the - // readable blocks of EVERY range are captured before any - // single range's slow per-sector recovery. A transport fault - // (bridge crash) still falls through below — it isn't a - // recoverable bad sector. No data is dropped: the block stays - // NonTrimmed until a granular pass recovers or gives up on it. - if self.opts.fast_capture && !err.is_scsi_transport_failure() { - send_or_abort( - self.pipe, - PatchItem::NonTrimmed { - pos, - len: block_bytes, - }, - )?; - self.state.blocks_read_failed += 1; - if self.opts.reverse { - frame.block_end = frame.block_end.saturating_sub(block_bytes); - } else { - frame.block_end += block_bytes; - } - continue; - } + // The whole section is now processed (recovered or left as residue); + // account it for progress/ETA and report once. + self.state.work_done = self.state.work_done.saturating_add(range_size); + if self + .disc + .report_patch_progress(&self.state, self.opts, self.total_bytes, self.shared) + { + self.state.halted = true; + return Ok(RegionOutcome::Halted); + } - // First failure in this range: the fast-batched pass over - // the clean overshoot is done. A genuine transport fault - // (bridge crash) is NOT a recoverable bad sector — let - // handle_read_failure abort immediately. Otherwise re-attempt - // the read ONCE (stochastic media: a marginal sector often - // reads on a retry) and fall through. Stay at MAX speed: - // direct probing on the live BU40N/UHD proved MAX reads a - // marginal sector ~12x faster than slow AND slow never - // recovered one MAX didn't — so the old drop-to-0x0000 only - // wasted time on the GOOD sectors of a bad range (~3x slower - // overall). `retried_once` gates the retry to once per range. - if !retried_once && !err.is_scsi_transport_failure() { - self.reader.set_speed(0xFFFF); - retried_once = true; - continue; - } - - // The retry also failed. Hand off to handle_read_failure - // (NOT_READY pause/retry, batch bisect, NonTrimmed mark, - // wedge/transport abort). (Scatter-recovery was removed: live - // probing proved recalibration does not change a sector's - // recovery — the drive's per-sector ECC is media-bound, not - // approach-bound — so it only cost time.) - match handle_read_failure( - &mut self.state, - &frame, - self.opts, - &err, - lba, - count, - pos, - block_bytes, - bytes, - read_duration_ms, - self.pipe, - self.shared, - self.reader, - )? { - FailureAction::Continue => {} - FailureAction::ContinueInner => continue, - // BreakOuter fires for both a transport fault and a - // wedge-family abort; distinguish for the exit reason. - FailureAction::BreakOuter => { - return Ok(if err.is_scsi_transport_failure() { - RegionOutcome::TransportFault - } else { - RegionOutcome::Wedged - }); - } - } - } - } - - let did_skip = - compute_damage_skip(&mut self.state, &mut frame, self.opts, lba, block_bytes); - - if !did_skip { - if self.opts.reverse { - frame.block_end = frame.block_end.saturating_sub(block_bytes); - } else { - frame.block_end += block_bytes; - } - } - - if self.opts.wedged_threshold > 0 - && self.state.consecutive_failures >= self.opts.wedged_threshold - { - // Only exit wedged after attempting multiple ranges with - // zero recovery. A single-range terminal failure should not - // abort the whole pass. - let multi_range_attempted = frame.range_idx > 0; - if multi_range_attempted { - tracing::info!( - target: "freemkv::disc", - phase = "patch.wedge.exit", - consecutive_failures = self.state.consecutive_failures, - blocks_read_failed = self.state.blocks_read_failed, - blocks_read_ok = self.state.blocks_read_ok, - range_index = frame.range_idx, - total_ranges = num_ranges, - "giving up — drive appears wedged after multiple ranges" - ); - self.state.wedged_exit = true; - return Ok(RegionOutcome::Wedged); - } - } - - self.state.work_done = self.state.work_done.saturating_add(block_bytes); - - if self.disc.report_patch_progress( - &self.state, - self.opts, - self.total_bytes, - self.shared, - ) { + match outcome { + // Whether the chain cleared the section or left residue, we always + // advance to the next range — never hang, never abort mid-pass. + HandlerOutcome::Complete | HandlerOutcome::Remaining => Ok(RegionOutcome::Completed), + HandlerOutcome::Halted => { self.state.halted = true; - return Ok(RegionOutcome::Halted); + Ok(RegionOutcome::Halted) + } + // Bridge/transport crash: end the pass so the orchestrator can + // spin-cycle the drive and resume from the mapfile next pass. + HandlerOutcome::TransportFault => { + self.state.wedged_exit = true; + Ok(RegionOutcome::TransportFault) } } } @@ -2357,14 +1165,7 @@ impl Disc { total_bytes, decrypt_is_aacs, cooldown_pending: false, - state: PatchLoopState::new( - bytes_good_before, - total_bytes, - initial_batch, - recovery, - work_total, - ), - buf: vec![0u8; initial_batch as usize * 2048], + state: PatchLoopState::new(bytes_good_before, total_bytes, initial_batch, work_total), }; ctx.run(&bad_ranges)?; let PatchCtx { state, .. } = ctx; @@ -2441,919 +1242,6 @@ impl Disc { mod tests { use super::*; - #[test] - fn skip_sectors_for_probe_does_not_overflow_for_large_idx() { - // idx=20 (escalation 60) and idx=21 (63) previously overflowed - // i64 via `32i64 << escalation`. Must saturate to the cap. - for idx in [0usize, 1, 2, 20, 21, 100, usize::MAX] { - let v = skip_sectors_for_probe(idx); - assert!( - v <= PASSN_SKIP_SECTORS_CAP, - "idx {idx}: {v} exceeds cap {PASSN_SKIP_SECTORS_CAP}" - ); - } - // Small indices still escalate as before. - assert_eq!(skip_sectors_for_probe(0), PASSN_SKIP_SECTORS_BASE); - assert_eq!(skip_sectors_for_probe(1), PASSN_SKIP_SECTORS_BASE << 3); - } - - #[test] - fn skip_limit_remainder_forward_does_not_overlap_recovered_region() { - // Forward mode: range [1000, 2000), cursor advanced past the - // midpoint to block_end=1700. The recovered region is - // [1000, 1700); the never-attempted remainder must be exactly - // [1700, 2000) — NOT a mirror start below block_end. - let r = skip_limit_remainder(false, 1000, 2000, 1700); - assert_eq!(r, Some((1700, 300))); - // The pre-fix mirror formula would have produced start = - // 1000 + (2000 - 1700) = 1300, which overlaps [1000, 1700). - assert!(r.unwrap().0 >= 1700, "must not overlap recovered region"); - } - - #[test] - fn skip_limit_remainder_forward_none_when_fully_attempted() { - assert_eq!(skip_limit_remainder(false, 1000, 2000, 2000), None); - } - - #[test] - fn skip_limit_remainder_reverse_marks_low_unattempted_region() { - // Reverse mode: cursor moved down to block_end=1300, so - // [1300, 2000) was attempted and [1000, 1300) is the remainder. - let r = skip_limit_remainder(true, 1000, 2000, 1300); - assert_eq!(r, Some((1000, 300))); - assert_eq!(skip_limit_remainder(true, 1000, 2000, 1000), None); - } - - // ---------------------------------------------------------------- - // compute_damage_skip - range-boundary + size-aware-cap coverage. - // - // These exercise the Pass-N damage-cluster skip documented in - // CLAUDE.md "Patch (Pass N)": skip is capped at 1/4 of the - // remaining bad range "so a single jump can't blow past a good - // middle", and the per-iteration cursor (`block_end`) must never - // cross the range boundary in either walk direction. A bug here - // silently abandons recoverable sectors (over-skip) or downgrades - // already-recovered sectors (cursor crossing the boundary). - // - // All byte offsets are multiples of 2048 (the sector size the code - // divides by at `range_remaining_bytes / 2048`). - // ---------------------------------------------------------------- - - /// Build a `PatchOptions` with only `reverse` meaningful for the - /// pure helpers under test (no I/O is performed). - fn opts_with_reverse(reverse: bool) -> crate::disc::PatchOptions<'static> { - crate::disc::PatchOptions { - decrypt: false, - block_sectors: Some(1), - full_recovery: false, - reverse, - wedged_threshold: 50, - progress: None, - halt: None, - - key_fetch: None, - fast_capture: false, - } - } - - /// A `PatchLoopState` whose damage window is full (16 entries) with - /// exactly `bad` failures - enough to evaluate the - /// `PASSN_DAMAGE_THRESHOLD_PCT` gate. `escalation` seeds - /// `consecutive_skips_without_recovery` so we can drive the - /// `PASSN_SKIP_SECTORS_BASE << escalation` size. - fn state_with_window(bad: usize, escalation: u32) -> PatchLoopState { - let mut s = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - s.damage_window.clear(); - for i in 0..PASSN_DAMAGE_WINDOW { - s.damage_window.push(i >= bad); // first `bad` entries = false - } - s.consecutive_skips_without_recovery = escalation; - s - } - - #[test] - fn damage_skip_forward_advances_cursor_toward_end_and_stays_in_range() { - // Forward walk: the per-iteration cursor moves UP, toward `end`, - // so the attempted region grows as [range_pos, block_end). A - // damage skip must push block_end FORWARD (higher) and never - // past `end` (the call site breaks on `block_end >= end`). Range - // [0, 80 KiB) = 40 sectors, cursor mid-range at 20 KiB. Spec: - // CLAUDE.md Pass-N reverse=false walks start->end. - // Mutation that makes this RED: swap the forward branch to - // subtract (reverse direction) e.g. `block_end - skip_bytes` -> - // the cursor moves the WRONG way and re-attempts recovered - // sectors / never converges. (Confirmed: the assertion - // block_end > before fails.) - let mut state = state_with_window(4, 0); - let opts = opts_with_reverse(false); - let mut frame = RangeFrame { - range_idx: 0, - range_pos: 0, - range_size: 80 * 1024, - end: 80 * 1024, - block_end: 20 * 1024, // 10 sectors in - range_budget_secs: 1, - range_sectors: 40, - }; - let before = frame.block_end; - let did = compute_damage_skip(&mut state, &mut frame, &opts, 0, 2048); - assert!(did, "threshold crossed: a skip must apply"); - assert!( - frame.block_end > before, - "forward skip must advance the cursor UP (toward end): {} !> {}", - frame.block_end, - before - ); - assert!( - frame.block_end <= frame.end, - "forward cursor {} overshot range end {}", - frame.block_end, - frame.end - ); - } - - #[test] - fn damage_skip_reverse_moves_cursor_toward_range_start_and_stays_in_range() { - // Reverse walk (the recovery walker default): the cursor moves - // DOWN, toward `range_pos`, so the attempted region grows as - // [block_end, end). A damage skip must push block_end BACKWARD - // (lower) and never below `range_pos` (the call site breaks on - // `block_end <= range_pos`). Spec: CLAUDE.md "Patch (Pass N) - - // Default: reverse mode ... within each range from end to start." - // Mutation that makes this RED: the reverse branch adds instead - // of subtracts (copy-paste of the forward formula) -> cursor - // moves UP, away from range_pos, and the walk never converges on - // the low end of the range, silently abandoning those sectors. - let mut state = state_with_window(4, 0); - let opts = opts_with_reverse(true); - let range_pos = 40 * 1024; - let mut frame = RangeFrame { - range_idx: 0, - range_pos, - range_size: 80 * 1024, - end: range_pos + 80 * 1024, - block_end: range_pos + 60 * 1024, // 30 sectors above range_pos - range_budget_secs: 1, - range_sectors: 40, - }; - let before = frame.block_end; - let did = compute_damage_skip(&mut state, &mut frame, &opts, 0, 2048); - assert!(did, "threshold crossed: a skip must apply"); - assert!( - frame.block_end < before, - "reverse skip must move the cursor DOWN (toward range_pos): {} !< {}", - frame.block_end, - before - ); - assert!( - frame.block_end >= frame.range_pos, - "reverse cursor {} descended below range_pos {}", - frame.block_end, - frame.range_pos - ); - } - - #[test] - fn damage_skip_caps_at_one_quarter_of_remaining_range() { - // CLAUDE.md: skip "capped at 1/4 of the remaining bad range so - // a single jump can't blow past a good middle." Forward walk, - // range [0, 80 KiB) = 40 sectors, cursor at start (block_end=0) - // so remaining = 40 sectors and quarter = 10 sectors. Drive a - // large escalation so the raw escalated skip far exceeds 10. - // The applied gap must be <= quarter (10 sectors = 20480 bytes). - // Mutation that makes this RED: remove `.min(range_quarter)` - // from `skip_sectors` -> the jump leaps the entire good middle. - let mut state = state_with_window(4, 10); - let opts = opts_with_reverse(false); - let mut frame = RangeFrame { - range_idx: 0, - range_pos: 0, - range_size: 80 * 1024, - end: 80 * 1024, - block_end: 0, - range_budget_secs: 1, - range_sectors: 40, - }; - let did = compute_damage_skip(&mut state, &mut frame, &opts, 0, 2048); - assert!(did, "threshold crossed: a skip must apply"); - let quarter_bytes = (40u64 / 4) * 2048; // 10 sectors - assert!( - frame.block_end <= quarter_bytes, - "skip advanced cursor to {} bytes, exceeding the 1/4 cap of {} bytes", - frame.block_end, - quarter_bytes - ); - assert!(frame.block_end > 0, "a real skip must advance the cursor"); - } - - #[test] - fn damage_skip_below_threshold_does_not_skip_or_mutate_state() { - // With an all-good window (bad=0) the damage threshold is NOT - // crossed, so compute_damage_skip must be a no-op: it must NOT - // advance the cursor, increment skip_count, or charge work_done. - // A spurious skip here silently abandons readable sectors that - // the patch loop would otherwise retry. - // Mutation that makes this RED: invert/weaken the threshold - // guard (e.g. `>=` -> `<`) so a clean window still skips. - let mut state = state_with_window(/*bad=*/ 0, /*escalation=*/ 0); - let opts = opts_with_reverse(false); - let work_before = state.work_done; - let skips_before = state.skip_count; - let mut frame = RangeFrame { - range_idx: 0, - range_pos: 0, - range_size: 80 * 1024, - end: 80 * 1024, - block_end: 4096, - range_budget_secs: 1, - range_sectors: 40, - }; - let did = compute_damage_skip(&mut state, &mut frame, &opts, 0, 2048); - assert!(!did, "clean window must not trigger a damage skip"); - assert_eq!(frame.block_end, 4096, "cursor must not move on a no-op"); - assert_eq!( - state.work_done, work_before, - "no-op must not charge work_done" - ); - assert_eq!( - state.skip_count, skips_before, - "no-op must not bump skip_count" - ); - } - - #[test] - fn damage_skip_work_done_equals_actual_gap_skipped() { - // Progress accounting: when a skip fires, `work_done` must grow - // by EXACTLY the number of bytes the cursor moved (the gap), - // and skip_count must increment by exactly 1. Reverse range - // [0, 64 KiB) = 32 sectors, cursor at the top so remaining = - // 32, quarter = 8 sectors, escalation 0 -> escalated = base - // (32) capped to quarter (8). Expected gap = 8 sectors. - // Mutation that makes this RED: compute `gap_bytes` from the - // wrong endpoints or double-add it. - let mut state = state_with_window(/*bad=*/ 4, /*escalation=*/ 0); - let opts = opts_with_reverse(true); - let end = 64 * 1024; - let mut frame = RangeFrame { - range_idx: 0, - range_pos: 0, - range_size: end, - end, - block_end: end, - range_budget_secs: 1, - range_sectors: 32, - }; - let before = frame.block_end; - let work_before = state.work_done; - let did = compute_damage_skip(&mut state, &mut frame, &opts, 0, 2048); - assert!(did, "threshold crossed: a skip must apply"); - let expected_gap = 8 * 2048u64; // min(base=32, quarter=8) sectors - let actual_gap = before - frame.block_end; // reverse: cursor moved down - assert_eq!( - actual_gap, expected_gap, - "reverse skip should move the cursor down by the quarter-cap gap" - ); - assert_eq!( - state.work_done - work_before, - actual_gap, - "work_done must grow by exactly the gap skipped" - ); - assert_eq!(state.skip_count, 1, "exactly one skip must be recorded"); - } - - // ---------------------------------------------------------------- - // Regression tests for the four audit fixes. - // ---------------------------------------------------------------- - - /// Fix 3: NOT_READY retryable errors must NOT increment - /// `consecutive_failures`. Pre-fix the increment happened before the - /// `is_not_ready_retryable` check, so repeated NOT_READY events on - /// a sluggish drive could push the counter past `wedged_threshold` - /// (50) and trigger a false wedged_exit that skipped the rest of the - /// pass. The fix moves the increment inside an `if !is_not_ready_retryable` - /// guard. This test verifies that the classification logic and the - /// conditional correctly identify the NOT_READY case and leave the - /// counter unchanged. - #[test] - fn fix3_not_ready_does_not_count_toward_consecutive_failures() { - // Construct a NOT_READY sense triple (sense_key=0x02, ASC=0x04). - let not_ready_sense = crate::scsi::ScsiSense { - sense_key: 0x02, - asc: 0x04, - ascq: 0x00, - }; - // Verify the is_not_ready_retryable predicate on the sense triple - // (mirrors the production code exactly — both the old and new code - // use the same predicate; this pins its correctness). - let is_not_ready_retryable = { - let s = ¬_ready_sense; - s.sense_key == 0x02 && (s.asc == 0x02 || s.asc == 0x03 || s.asc == 0x04) - }; - assert!( - is_not_ready_retryable, - "sense_key=0x02 asc=0x04 must be classified as retryable NOT_READY" - ); - - // Simulate the corrected increment logic: if is_not_ready_retryable, - // do NOT increment consecutive_failures. - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - let failures_before = state.consecutive_failures; - if !is_not_ready_retryable { - state.consecutive_failures += 1; - } - assert_eq!( - state.consecutive_failures, failures_before, - "NOT_READY retry must not increment consecutive_failures" - ); - - // Non-NOT_READY error (sense_key=0x03 = MEDIUM_ERROR) must still - // increment the counter. - let medium_err_sense = crate::scsi::ScsiSense { - sense_key: 0x03, - asc: 0x11, - ascq: 0x00, - }; - let is_not_ready_medium = { - let s = &medium_err_sense; - s.sense_key == 0x02 && (s.asc == 0x02 || s.asc == 0x03 || s.asc == 0x04) - }; - assert!(!is_not_ready_medium, "MEDIUM_ERROR must not be NOT_READY"); - let failures_before2 = state.consecutive_failures; - if !is_not_ready_medium { - state.consecutive_failures += 1; - } - assert_eq!( - state.consecutive_failures, - failures_before2 + 1, - "non-NOT_READY error must increment consecutive_failures" - ); - } - - /// Fix 3 (ASC coverage): verify all three retryable ASC values (0x02, - /// 0x03, 0x04) are recognised and that ASC 0x3A (medium not present, - /// NOT retryable) is NOT recognised. - #[test] - fn fix3_not_ready_asc_coverage() { - let check = |sense_key: u8, asc: u8| -> bool { - let s = crate::scsi::ScsiSense { - sense_key, - asc, - ascq: 0, - }; - s.sense_key == 0x02 && (s.asc == 0x02 || s.asc == 0x03 || s.asc == 0x04) - }; - assert!(check(0x02, 0x02), "ASC 0x02 must be retryable"); - assert!(check(0x02, 0x03), "ASC 0x03 must be retryable"); - assert!(check(0x02, 0x04), "ASC 0x04 must be retryable"); - assert!( - !check(0x02, 0x3A), - "ASC 0x3A (medium not present) must NOT be retryable" - ); - assert!( - !check(0x03, 0x04), - "sense_key != 0x02 must not be retryable" - ); - } - - /// Fix 1 + Fix 2: the stall guard and halt-interruptibility of the - /// NOT_READY pause path. Since `handle_read_failure` requires a full - /// Pipeline (non-trivially constructable in unit tests), this test - /// directly exercises the two sub-behaviors that Fix 1 and Fix 2 add - /// to that path: - /// - /// * Fix 1: when `stall_start` is already past STALL_SECS ago, - /// `wedged_exit` must be set and `BreakOuter` returned — the same - /// stall guard that fires in the normal failure path must also fire - /// on the NOT_READY retry path. - /// * Fix 2: `sleep_secs_or_halt` exits immediately when the halt - /// token is already set, so the 15 s NOT_READY pause does not block - /// cancellation. - #[test] - fn fix1_and_fix2_not_ready_stall_guard_and_halt_responsiveness() { - // Fix 2: halt token pre-set — sleep must return in well under 1 s. - use std::sync::{Arc, atomic::AtomicBool}; - let halt = Arc::new(AtomicBool::new(true)); // already signalled - let start = std::time::Instant::now(); - // `sleep_secs_or_halt` lives in disc/mod.rs (pub(crate)); from - // this test module (inside patch.rs which is a child of disc), - // `super` is the patch module and `super::super` is disc. - super::super::sleep_secs_or_halt(15, Some(&halt)); - let elapsed = start.elapsed(); - assert!( - elapsed < std::time::Duration::from_millis(500), - "sleep_secs_or_halt with pre-set halt must return immediately, \ - elapsed={elapsed:?}" - ); - - // Fix 1: stall guard logic — simulate the stall check that the - // NOT_READY path now executes after the sleep. The guard fires - // when stall_start is older than STALL_SECS and bytes_good has - // not advanced. Pre-fix: the NOT_READY path returned ContinueInner - // before this check so it was never reached. - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - // Wind the clock back past the stall threshold. - state.stall_start = std::time::Instant::now() - .checked_sub(std::time::Duration::from_secs(STALL_SECS + 10)) - .unwrap_or(state.stall_start); - // bytes_good hasn't moved (same as bytes_good_last = 0). - let bytes_good_now = state.bytes_good_last; // no progress - // Reproduce the stall guard condition added to the NOT_READY path. - let stall_fires = state.stall_start.elapsed() > std::time::Duration::from_secs(STALL_SECS); - assert!( - stall_fires, - "stall guard must fire when stall_start is older than STALL_SECS \ - and bytes_good has not advanced (bytes_good_now={bytes_good_now})" - ); - // If it fires, the fix sets wedged_exit and returns BreakOuter. - state.wedged_exit = true; // mirror what the production code does - assert!( - state.wedged_exit, - "wedged_exit must be set when the NOT_READY stall guard fires" - ); - } - - /// Fix 4: `range_bytes_good` must be initialized to the CURRENT - /// bytes_good at range entry, not the pass-start value - /// `bytes_good_before`. Pre-fix: after range 0 recovers N bytes, - /// range 1 entered with `range_bytes_good = bytes_good_before`, so - /// the first `check_range_watchdog` tick saw `bytes_good_now > - /// range_bytes_good` (because of range 0's recovery) and spuriously - /// reset `range_start` — giving range 1 a free budget refill it - /// hadn't earned. - /// - /// This test verifies that if `range_bytes_good` is set to the CURRENT - /// value (no new recovery yet in this range), the watchdog does NOT - /// reset the timer on its first tick. - #[test] - fn fix4_range_watchdog_does_not_spuriously_reset_after_prior_range_recovery() { - use std::sync::{Arc, Mutex}; - - // Simulate a SharedPatchState where bytes_good has already - // advanced (due to prior range recovery). - let current_bytes_good: u64 = 1024 * 1024; // some non-zero recovery - let shared = Arc::new(Mutex::new(SharedPatchState { - stats: MapStats { - bytes_total: 0, - bytes_good: current_bytes_good, - bytes_pending: 0, - bytes_unreadable: 0, - bytes_retryable: 0, - bytes_nontried: 0, - num_bad_ranges: 0, - main_lost_ms: 0.0, - }, - bad_ranges: vec![], - })); - - // Fix 4 (corrected): range_bytes_good = current_bytes_good. - // The watchdog should see bytes_good_now == range_bytes_good and - // NOT reset range_start. - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - state.range_bytes_good = current_bytes_good; // correct: current value - let original_range_start = state.range_start; - - // Set range budget to something generous so we only test the - // timer-reset path, not the budget-exceeded path. - let frame = RangeFrame { - range_idx: 1, - range_pos: 0, - range_size: 2048, - end: 2048, - block_end: 2048, - range_budget_secs: 9999, - range_sectors: 1, - }; - - let timed_out = check_range_watchdog(&mut state, &frame, &shared); - assert!(!timed_out, "range must not time out immediately"); - // With correct initialization bytes_good_now == range_bytes_good, - // so the `bytes_good_now > range_bytes_good` branch does NOT fire - // and range_start is NOT reset. - // - // The pre-fix bug: range_bytes_good = bytes_good_before (0) while - // bytes_good_now = current_bytes_good (1 MiB), so the first tick - // would unconditionally reset range_start, masking stalls in ranges - // that followed productive ones. - assert_eq!( - state.range_bytes_good, current_bytes_good, - "range_bytes_good must stay at the current value (no new recovery yet)" - ); - // Verify the timer was not reset: range_start should be at or - // before the original value (it could be the same Instant or - // marginally later due to the lock, but it must not have jumped - // forward). We check that range_start did not advance by more than - // 1 ms (the watchdog logic sets it to Instant::now() on reset). - let drift = state - .range_start - .checked_duration_since(original_range_start) - .unwrap_or_default(); - assert!( - drift < std::time::Duration::from_millis(100), - "range_start must not be reset on the first tick when no new recovery \ - occurred in this range (drift={drift:?})" - ); - } - - // ---- Clock seam: deterministic watchdog timeouts ------------------ - // - // `PatchLoopState::new_with_clock` lets a test inject a monotonic clock so - // the per-range / whole-pass watchdogs can be driven WITHOUT real wall time. - // The fake clock is a free `fn() -> Instant` (the seam's type), backed by a - // process-wide millisecond offset. Tests that use it serialize on a mutex so - // the shared offset can't be clobbered by a concurrently-running clock test. - - use std::sync::atomic::{AtomicU64, Ordering}; - - static FAKE_CLOCK_OFFSET_MS: AtomicU64 = AtomicU64::new(0); - static FAKE_CLOCK_LOCK: Mutex<()> = Mutex::new(()); - - /// The injectable clock: a fixed base plus the current offset. `OnceLock` - /// pins the base so every call within a test advances from the same origin. - fn fake_now() -> std::time::Instant { - use std::sync::OnceLock; - static BASE: OnceLock = OnceLock::new(); - let base = *BASE.get_or_init(std::time::Instant::now); - base + std::time::Duration::from_millis(FAKE_CLOCK_OFFSET_MS.load(Ordering::SeqCst)) - } - - /// Advance the fake clock by `secs` seconds. - fn advance_fake_clock(secs: u64) { - FAKE_CLOCK_OFFSET_MS.fetch_add(secs * 1000, Ordering::SeqCst); - } - - fn shared_with_bytes_good(bytes_good: u64) -> Arc> { - Arc::new(Mutex::new(SharedPatchState { - stats: MapStats { - bytes_total: 0, - bytes_good, - bytes_pending: 0, - bytes_unreadable: 0, - bytes_retryable: 0, - bytes_nontried: 0, - num_bad_ranges: 0, - main_lost_ms: 0.0, - }, - bad_ranges: vec![], - })) - } - - /// The per-range watchdog must NOT trip before the budget elapses and MUST - /// trip once the injected clock passes the budget — with zero forward - /// progress (bytes_good frozen). Driven entirely by `advance_fake_clock`, - /// so it proves the real `check_range_watchdog` timeout branch executes. - #[test] - fn range_watchdog_trips_on_budget_exhaustion_via_fake_clock() { - let _guard = FAKE_CLOCK_LOCK.lock().unwrap(); - FAKE_CLOCK_OFFSET_MS.store(0, Ordering::SeqCst); - - let shared = shared_with_bytes_good(0); - let mut state = PatchLoopState::new_with_clock(0, 1 << 40, 1, false, 1 << 40, fake_now); - - // A 10 s budget. range_start was seeded from fake_now() at offset 0. - let frame = RangeFrame { - range_idx: 1, - range_pos: 0, - range_size: 2048, - end: 2048, - block_end: 2048, - range_budget_secs: 10, - range_sectors: 1, - }; - - // Just under budget: no trip. - advance_fake_clock(9); - assert!( - !check_range_watchdog(&mut state, &frame, &shared), - "watchdog must not trip before the range budget elapses" - ); - - // Past budget with no recovery: trip. - advance_fake_clock(2); // total 11 s >= 10 s budget - assert!( - check_range_watchdog(&mut state, &frame, &shared), - "watchdog must trip once the injected clock passes the range budget" - ); - } - - /// Forward progress (bytes_good advancing) must reset the per-range clock so - /// the watchdog does NOT trip even though more than `budget` seconds of fake - /// time have passed in aggregate — proving the reset branch reads the seam, - /// not real time. - #[test] - fn range_watchdog_forward_progress_resets_clock_via_fake_clock() { - let _guard = FAKE_CLOCK_LOCK.lock().unwrap(); - FAKE_CLOCK_OFFSET_MS.store(0, Ordering::SeqCst); - - let shared = shared_with_bytes_good(0); - let mut state = PatchLoopState::new_with_clock(0, 1 << 40, 1, false, 1 << 40, fake_now); - - let frame = RangeFrame { - range_idx: 1, - range_pos: 0, - range_size: 2048, - end: 2048, - block_end: 2048, - range_budget_secs: 10, - range_sectors: 1, - }; - - // 8 s, then recovery commits (bytes_good advances) — clock resets. - advance_fake_clock(8); - shared.lock().unwrap().stats.bytes_good = 4096; - assert!( - !check_range_watchdog(&mut state, &frame, &shared), - "progress tick must not trip" - ); - - // 8 more seconds (16 s total, but only 8 since the reset): still under - // budget because the productive tick reset range_start. - advance_fake_clock(8); - assert!( - !check_range_watchdog(&mut state, &frame, &shared), - "watchdog must not trip when forward progress kept resetting the clock" - ); - - // Now freeze progress and exceed the budget from the last reset. - advance_fake_clock(11); - assert!( - check_range_watchdog(&mut state, &frame, &shared), - "watchdog must trip once progress stops and the budget elapses" - ); - } - - /// The whole-pass stall watchdog predicate (`STALL_SECS` on no bytes_good - /// movement) must be governed by the injected clock. This drives the exact - /// comparison the production stall guard runs — `(state.now)().duration_since - /// (state.stall_start) > STALL_SECS` — through `new_with_clock`, proving the - /// seam reaches the stall path too (which is inline in helpers that need a - /// full Pipeline, so we assert the predicate the helpers evaluate). - #[test] - fn whole_pass_stall_predicate_governed_by_fake_clock() { - let _guard = FAKE_CLOCK_LOCK.lock().unwrap(); - FAKE_CLOCK_OFFSET_MS.store(0, Ordering::SeqCst); - - let state = PatchLoopState::new_with_clock(0, 1 << 40, 1, false, 1 << 40, fake_now); - - // Before STALL_SECS: predicate false. - advance_fake_clock(STALL_SECS - 1); - assert!( - (state.now)().duration_since(state.stall_start) - <= std::time::Duration::from_secs(STALL_SECS), - "stall must not fire before STALL_SECS of injected time" - ); - - // Past STALL_SECS with no progress: predicate true → production sets - // wedged_exit and breaks the outer loop. - advance_fake_clock(2); - assert!( - (state.now)().duration_since(state.stall_start) - > std::time::Duration::from_secs(STALL_SECS), - "stall guard fires once injected time exceeds STALL_SECS" - ); - } - - /// NOT_READY per-LBA cap: after NOT_READY_MAX_RETRIES_PER_LBA retries - /// on the same LBA the cap is exhausted and the next NOT_READY is treated - /// as a normal failure (consecutive_failures incremented, retry refused). - /// A different LBA resets the counter so transient NOT_READY can still - /// recover. Mirrors the sweep path cap (read_error.rs - /// NOT_READY_MAX_RETRIES = 3). - /// - /// Regression for: NOT_READY retries had no per-LBA bound, so a - /// persistently-not-ready disc could loop on a single LBA until the - /// whole-pass STALL_SECS watchdog fired (up to 3600 s per range). - #[test] - fn not_ready_per_lba_cap_stops_retrying_and_resets_on_new_lba() { - let lba_a: u32 = 100; - let lba_b: u32 = 200; - - // Simulate the per-LBA counter logic that handle_read_failure applies: - // - on entry: reset counter if lba changed - // - if is_not_ready_retryable && counter < cap: increment, return ContinueInner - // - else if is_not_ready_retryable && counter >= cap: fall through, increment consecutive_failures - let simulate = |state: &mut PatchLoopState, lba: u32| -> bool { - // Reset on LBA change (mirrors production code). - if state.not_ready_lba != Some(lba) { - state.not_ready_retries_per_lba = 0; - state.not_ready_lba = Some(lba); - } - let is_not_ready = true; // all calls in this test are NOT_READY - if is_not_ready { - if state.not_ready_retries_per_lba < NOT_READY_MAX_RETRIES_PER_LBA { - state.not_ready_retries_per_lba += 1; - return true; // ContinueInner (retry) - } - // cap exceeded: fall through — count toward consecutive_failures - state.consecutive_failures += 1; - } - false // not retried - }; - - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - - // First NOT_READY_MAX_RETRIES_PER_LBA calls on lba_a must be retried. - for i in 1..=NOT_READY_MAX_RETRIES_PER_LBA { - let retried = simulate(&mut state, lba_a); - assert!( - retried, - "retry {i}/{NOT_READY_MAX_RETRIES_PER_LBA} on lba_a must return ContinueInner" - ); - assert_eq!( - state.not_ready_retries_per_lba, i, - "counter must be {i} after {i} retries" - ); - assert_eq!( - state.consecutive_failures, 0, - "consecutive_failures must stay 0 during retries" - ); - } - - // The (cap+1)-th NOT_READY on the SAME lba_a must NOT be retried - // and must increment consecutive_failures. - let retried = simulate(&mut state, lba_a); - assert!( - !retried, - "NOT_READY on lba_a after cap must NOT return ContinueInner" - ); - assert_eq!( - state.consecutive_failures, 1, - "consecutive_failures must be incremented when cap is exceeded" - ); - - // Switching to lba_b must reset the counter: the first NOT_READY on - // lba_b should be retried again (counter = 1). - let retried = simulate(&mut state, lba_b); - assert!( - retried, - "first NOT_READY on lba_b (new LBA) must return ContinueInner \ - (counter reset on LBA change)" - ); - assert_eq!( - state.not_ready_retries_per_lba, 1, - "counter must restart at 1 after LBA change" - ); - assert_eq!( - state.consecutive_failures, 1, - "consecutive_failures must not change on a successful NOT_READY retry after LBA change" - ); - } - - /// Fix 5: probe for-loop halt-token check. - /// - /// Pre-fix: the probe loop in `handle_read_failure` had no halt-token - /// check. Each probe read can block up to READ_RECOVERY_TIMEOUT_MS - /// (60 s); with 3 probes a /api/stop could take up to ~180 s to be - /// honored. - /// - /// The fix adds the same pattern used by the backtrack inner loop - /// (~line 785): - /// - /// if let Some(h) = &opts.halt { - /// if h.load(Ordering::Relaxed) { return Err(Halted); } - /// } - /// - /// `handle_read_failure` is not unit-testable in isolation because it - /// requires a live `Pipeline` sink. This test verifies the two - /// sub-behaviors the fix relies on: - /// - /// 1. The probe block is reached when `consecutive_failures >= 3 - /// && consecutive_failures % 5 == 0` — confirmed by checking the - /// gate condition directly. - /// 2. An `AtomicBool` pre-set to `true` loaded with `Ordering::Relaxed` - /// returns `true` immediately (i.e., the early-exit logic is sound). - /// - /// Together these guarantee that a pre-set halt token causes the loop - /// to exit on the first iteration without issuing a read. - #[test] - fn fix5_probe_loop_honors_halt_token() { - use std::sync::{ - Arc, - atomic::{AtomicBool, Ordering}, - }; - - // 1. Gate condition: consecutive_failures = 5 triggers probe block. - // (first value satisfying >= 3 && % 5 == 0) - let consecutive_failures: u64 = 5; - assert!( - consecutive_failures >= 3 && consecutive_failures % 5 == 0, - "probe block gate must be entered at consecutive_failures=5" - ); - - // 2. Pre-set halt token must be detected immediately via Relaxed load. - // Use Arc to match the production type (Option>). - let halt = Arc::new(AtomicBool::new(true)); - let detected = halt.load(Ordering::Relaxed); - assert!( - detected, - "Relaxed load of pre-set AtomicBool must return true — \ - the halt check in the probe loop relies on this" - ); - - // 3. Zero-offset probe (offset_sectors = 0, probe_idx = 0) fires - // only when consecutive_failures >= 5; validate that gate too. - // (The halt check comes before this guard, so it fires first - // regardless — but confirm the gate would otherwise let it through.) - assert!( - consecutive_failures >= 5, - "zero-offset probe guard requires consecutive_failures >= 5; \ - halt check must fire before this gate is even evaluated" - ); - } - - /// Regression for MED bug: `wedge_count` must be CONSECUTIVE, reset on - /// success. - /// - /// Pre-fix: `handle_read_success` never touched `wedge_count`. A - /// sequence of wedge-family failures interspersed with good reads - /// accumulated `wedge_count` monotonically, hitting - /// `WEDGE_ABORT_THRESHOLD` (16) and aborting the pass even though the - /// drive was actually making forward progress. The fix adds - /// `state.wedge_count = 0` in `handle_read_success` so only a run of - /// CONSECUTIVE wedge-family senses (with no intervening success) can - /// reach the threshold. - /// - /// Scenario A: failures with an intervening success must NOT reach the - /// threshold. - /// - /// Scenario B: a true run of consecutive wedge-family failures (no - /// intervening success) must still reach the threshold and set - /// `wedged_exit`. - #[test] - fn wedge_count_resets_on_success_prevents_premature_abort() { - // Simulate the wedge_count mutation that handle_read_success now - // performs (state.wedge_count = 0) and the wedge increment that - // handle_read_failure performs for is_wedge_family errors. - - // Helper: apply one wedge-family failure — mirrors the production path - // in handle_read_failure (is_wedge_family branch). - let wedge_failure = |state: &mut PatchLoopState| { - state.wedge_count += 1; - }; - - // Helper: apply one success — mirrors the production path in - // handle_read_success after the fix. - let success = |state: &mut PatchLoopState| { - state.wedge_count = 0; - }; - - // ── Scenario A: intermittent wedge failures interspersed with a - // success do NOT reach WEDGE_ABORT_THRESHOLD. ────────────────────── - { - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - - // Drive 10 wedge-family failures. - for _ in 0..10 { - wedge_failure(&mut state); - } - assert_eq!( - state.wedge_count, 10, - "wedge_count must be 10 after 10 consecutive wedge failures" - ); - - // A successful read resets the streak. - success(&mut state); - assert_eq!( - state.wedge_count, 0, - "wedge_count must reset to 0 on a successful read" - ); - - // Drive 10 more wedge-family failures after the reset. - for _ in 0..10 { - wedge_failure(&mut state); - } - assert_eq!( - state.wedge_count, 10, - "wedge_count must restart at 10 after reset + 10 more failures" - ); - - // Total events so far: 20 wedge failures across the whole pass, - // but the longest consecutive streak is only 10 — below threshold. - assert!( - state.wedge_count < WEDGE_ABORT_THRESHOLD, - "intermittent pattern (10 + success + 10) must not reach \ - WEDGE_ABORT_THRESHOLD ({WEDGE_ABORT_THRESHOLD}); \ - wedge_count = {}", - state.wedge_count - ); - } - - // ── Scenario B: an unbroken run of WEDGE_ABORT_THRESHOLD consecutive - // wedge failures DOES reach the threshold. ───────────────────────── - { - let mut state = PatchLoopState::new(0, 1 << 40, 1, false, 1 << 40); - - for _ in 0..WEDGE_ABORT_THRESHOLD { - wedge_failure(&mut state); - } - assert!( - state.wedge_count >= WEDGE_ABORT_THRESHOLD, - "a true run of {WEDGE_ABORT_THRESHOLD} consecutive wedge failures \ - must reach the threshold; wedge_count = {}", - state.wedge_count - ); - } - } - /// Transport failure (status=0xFF, USB-bridge crash) must be recognised by /// the gate `handle_read_failure` now checks FIRST, so it aborts the pass /// (wedged_exit + BreakOuter) instead of treating the bridge crash as an diff --git a/src/disc/section_recover.rs b/src/disc/section_recover.rs new file mode 100644 index 0000000..ec75186 --- /dev/null +++ b/src/disc/section_recover.rs @@ -0,0 +1,738 @@ +//! Handler-chain recovery of a single bad section (Pass-N rework, #55). +//! +//! The pre-existing patch loop grinds one bad range end-to-end, and when the +//! drive wedges it aborts the WHOLE pass — so a dead cluster at the *front* of +//! a range starves every later range of any attempt. This module replaces that +//! with a chain of time-bounded recovery *handlers*, each a single recovery +//! *idea* (read backwards, forwards, fast, slow, bisect...). A coordinator runs +//! them in sequence over one section's still-bad sub-ranges: +//! +//! - each handler gets a hard wall-clock `deadline` and MUST return promptly +//! once it passes — no handler ever blocks unbounded (that is the whole +//! point); +//! - a handler recovers what it can, shrinking the shared [`SubRanges`] via +//! [`SubRanges::remove`], and returns [`HandlerOutcome::Remaining`] with the +//! rest still bad — the NEXT handler then tries a different idea on what is +//! left; +//! - whatever is still bad after every handler is the residue the caller +//! records as loss (NonTrimmed) before MOVING ON to the next section. +//! +//! Adding a new recovery idea is one new [`SectionHandler`] impl pushed onto the +//! chain — nothing else changes. +//! +//! This module is deliberately decoupled from the live `patch` machinery +//! (`PatchSink`, `PatchItem`, mapfile locks): recovered bytes flow through the +//! tiny [`RecoverySink`] trait, and the clock is injected as `&dyn Fn`, so every +//! handler and the coordinator are unit-testable against a synthetic +//! `SectorSource` with a fake clock — no live drive, no real sleeps. +//! +//! Wired into `patch_region` (#55): [`run_handlers`] is the live Pass-N recovery +//! engine. `SubRanges` stays the shared still-bad set. + +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Instant; + +use super::patch::{SubRanges, recovery_read}; +use crate::sector::SectorSource; + +/// One 2048-byte sector. +const SECTOR: u64 = 2048; +/// Batch size a linear handler reads at once (sectors). A partially-dead batch +/// falls back to single-sector reads, so this only trades throughput on clean +/// spans against granularity on dead ones. +const BATCH_SECTORS: u64 = 32; + +/// Where a handler left the section after its bounded attempt. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum HandlerOutcome { + /// The still-bad set is now empty — the section is fully recovered. The + /// coordinator stops the chain. + Complete, + /// The handler finished or hit its deadline with bad sub-ranges remaining — + /// the coordinator moves to the next handler. + Remaining, + /// The caller's halt token was observed set — abort the chain. + Halted, + /// A transport-layer fault (bridge wedge / dead bus) — the device never + /// answered. The coordinator returns this so the caller can un-wedge + /// (spin-cycle) before deciding whether to continue. + TransportFault, +} + +/// Receives sectors a handler successfully read back. Kept minimal and +/// decoupled from `PatchSink` so handlers are unit-testable in isolation; the +/// live wiring maps `recovered` onto the mapfile write + Finished mark. +pub(super) trait RecoverySink { + /// `buf` holds the plaintext bytes for the byte-range `[pos, pos+buf.len())` + /// (all multiples of [`SECTOR`]). + fn recovered(&mut self, pos: u64, buf: &[u8]); +} + +/// Everything a handler needs, borrowed for the duration of one `recover` call. +/// The `deadline` is passed separately to `recover` (not stored here) so each +/// handler invocation is independently bounded. +pub(super) struct HandlerCtx<'a> { + pub reader: &'a mut dyn SectorSource, + pub sink: &'a mut dyn RecoverySink, + /// Clock seam — handlers read wall time through this, never `Instant::now()` + /// inline, so tests advance a fake clock deterministically. + pub now: &'a dyn Fn() -> Instant, + pub halt: Option<&'a AtomicBool>, + /// Widen mid-unit reads to the aligned AACS unit (see [`recovery_read`]). + pub decrypt_is_aacs: bool, +} + +impl HandlerCtx<'_> { + fn halted(&self) -> bool { + self.halt.is_some_and(|h| h.load(Ordering::Relaxed)) + } + + fn past(&self, deadline: Instant) -> bool { + (self.now)() >= deadline + } +} + +/// Outcome of one physical read attempt, before the caller decides what to do +/// with the still-bad set. +enum ReadHit { + /// Bytes came back and were handed to the sink. + Good, + /// A recoverable bad-sector error (media / check-condition). Leave the span + /// bad and move on. + Bad, + /// Transport-layer fault — the bus is gone. Abort now. + Transport, +} + +/// Read `count` sectors at byte offset `pos` and, on success, hand them to the +/// sink. Does NOT touch the still-bad set — the caller removes recovered spans +/// so the read helper stays independent of `SubRanges`. +fn read_span( + ctx: &mut HandlerCtx, + buf: &mut [u8], + pos: u64, + count: u16, + recovery: bool, +) -> ReadHit { + let lba = (pos / SECTOR) as u32; + let bytes = count as usize * SECTOR as usize; + match recovery_read(ctx.reader, ctx.decrypt_is_aacs, lba, count, buf, recovery) { + Ok(_) => { + ctx.sink.recovered(pos, &buf[..bytes]); + ReadHit::Good + } + Err(e) if e.is_scsi_transport_failure() => ReadHit::Transport, + Err(_) => ReadHit::Bad, + } +} + +/// One recovery idea, given a bounded shot at the section's still-bad set. +/// +/// Contract: check `ctx.halted()` and `ctx.past(deadline)` between reads and +/// return promptly (`Halted` / `Remaining`) — never loop past the deadline. On a +/// good read call `ctx.sink.recovered` and [`SubRanges::remove`] the span; on a +/// bad read leave it in `bad` and advance (skip-and-move-on); on a transport +/// fault return [`HandlerOutcome::TransportFault`] immediately. +pub(super) trait SectionHandler { + fn name(&self) -> &'static str; + fn recover( + &mut self, + ctx: &mut HandlerCtx, + bad: &mut SubRanges, + deadline: Instant, + ) -> HandlerOutcome; +} + +/// Linear sweep of each bad sub-range. `reverse` walks end→start (the disc +/// sweep overshoots forward, so a NonTrimmed range's good data sits at its tail +/// — reverse hits it first); `!reverse` walks start→end (the front the reverse +/// pass kept dying on). `fast` selects the single-attempt read (`recovery = +/// false`) over the 60 s deep-recovery read. The two bools give backwards / +/// forwards / fast / slow from one handler. +pub(super) struct Linear { + pub reverse: bool, + pub fast: bool, +} + +impl Linear { + /// A batch read failed as a unit — retry it sector-by-sector so the readable + /// sectors of a partially-dead batch are still recovered and only the dead + /// ones stay bad. Bounded: at most `span_bytes / SECTOR` single reads. + fn narrow_batch( + &self, + ctx: &mut HandlerCtx, + bad: &mut SubRanges, + deadline: Instant, + pos: u64, + span_bytes: u64, + ) -> Option { + let recovery = !self.fast; + let mut buf = [0u8; SECTOR as usize]; + let mut off = 0; + while off < span_bytes { + if ctx.halted() { + return Some(HandlerOutcome::Halted); + } + if ctx.past(deadline) { + return Some(HandlerOutcome::Remaining); + } + let spos = pos + off; + match read_span(ctx, &mut buf, spos, 1, recovery) { + ReadHit::Good => bad.remove(spos, SECTOR), + ReadHit::Bad => {} + ReadHit::Transport => return Some(HandlerOutcome::TransportFault), + } + off += SECTOR; + } + None + } +} + +impl SectionHandler for Linear { + fn name(&self) -> &'static str { + match (self.reverse, self.fast) { + (true, true) => "linear:reverse:fast", + (true, false) => "linear:reverse:slow", + (false, true) => "linear:forward:fast", + (false, false) => "linear:forward:slow", + } + } + + fn recover( + &mut self, + ctx: &mut HandlerCtx, + bad: &mut SubRanges, + deadline: Instant, + ) -> HandlerOutcome { + let recovery = !self.fast; + let batch_bytes = BATCH_SECTORS * SECTOR; + let mut buf = vec![0u8; batch_bytes as usize]; + // Snapshot the sub-ranges: we mutate `bad` via remove() as we recover, + // and iterating the snapshot keeps that from disturbing the walk. + let mut snapshot: Vec<(u64, u64)> = bad.ranges().to_vec(); + if self.reverse { + snapshot.reverse(); + } + + for (rp, rl) in snapshot { + // Position within the range, in bytes, walked from whichever end. + let mut done = 0u64; + while done < rl { + if ctx.halted() { + return HandlerOutcome::Halted; + } + if ctx.past(deadline) { + return HandlerOutcome::Remaining; + } + let span = batch_bytes.min(rl - done); + let pos = if self.reverse { + rp + (rl - done - span) + } else { + rp + done + }; + let count = (span / SECTOR) as u16; + match read_span(ctx, &mut buf, pos, count, recovery) { + ReadHit::Good => bad.remove(pos, span), + ReadHit::Bad => { + // Recover the readable sectors inside the dead batch, + // leave the truly-dead ones bad, and keep moving. + if let Some(o) = self.narrow_batch(ctx, bad, deadline, pos, span) { + return o; + } + } + ReadHit::Transport => return HandlerOutcome::TransportFault, + } + done += span; + } + } + + if bad.is_empty() { + HandlerOutcome::Complete + } else { + HandlerOutcome::Remaining + } + } +} + +/// Probe the MIDDLE sector of each bad sub-range; if it reads, remove it and +/// recurse on the two halves to converge on good centers. If the middle is dead, +/// leave that chunk for another handler / pass. Finds islands of readable data +/// inside a mostly-dead range that a linear sweep would tar with one failing +/// batch. +pub(super) struct Bisect; + +impl SectionHandler for Bisect { + fn name(&self) -> &'static str { + "bisect" + } + + fn recover( + &mut self, + ctx: &mut HandlerCtx, + bad: &mut SubRanges, + deadline: Instant, + ) -> HandlerOutcome { + let mut buf = [0u8; SECTOR as usize]; + // Explicit work stack of (pos, len) chunks still to probe. Each good + // probe removes one sector and pushes its two halves; each read consumes + // a sector, so the stack drains in bounded steps. + let mut stack: Vec<(u64, u64)> = bad.ranges().to_vec(); + while let Some((rp, rl)) = stack.pop() { + if rl == 0 { + continue; + } + if ctx.halted() { + return HandlerOutcome::Halted; + } + if ctx.past(deadline) { + return HandlerOutcome::Remaining; + } + // Middle sector, floored to a sector boundary. + let sectors = rl / SECTOR; + let mid = rp + (sectors / 2) * SECTOR; + match read_span(ctx, &mut buf, mid, 1, true) { + ReadHit::Good => { + bad.remove(mid, SECTOR); + // Left half [rp, mid), right half [mid+SECTOR, rp+rl). + if mid > rp { + stack.push((rp, mid - rp)); + } + let right = mid + SECTOR; + if right < rp + rl { + stack.push((right, rp + rl - right)); + } + } + // Dead middle: leave the chunk bad and move on. + ReadHit::Bad => {} + ReadHit::Transport => return HandlerOutcome::TransportFault, + } + } + + if bad.is_empty() { + HandlerOutcome::Complete + } else { + HandlerOutcome::Remaining + } + } +} + +/// Run the handler chain over one section's still-bad set. This is the +/// never-hang guarantee: each handler is bounded by the deadline +/// `section_deadline_for(bad)` returns, and the loop always drains to +/// `Complete`/`Remaining` (whatever is still bad is the caller's residue to +/// record as loss). `Halted` / `TransportFault` short-circuit so the caller can +/// abort or un-wedge. +pub(super) fn run_handlers( + ctx: &mut HandlerCtx, + handlers: &mut [Box], + bad: &mut SubRanges, + section_deadline_for: impl Fn(&SubRanges) -> Instant, +) -> HandlerOutcome { + for handler in handlers.iter_mut() { + if bad.is_empty() { + return HandlerOutcome::Complete; + } + let before = bad.total_len(); + let deadline = section_deadline_for(bad); + let outcome = handler.recover(ctx, bad, deadline); + tracing::info!( + target: "freemkv::disc", + phase = "section_recover.handler", + handler = handler.name(), + bad_bytes_before = before, + bad_bytes_after = bad.total_len(), + outcome = ?outcome, + "handler finished; remaining bad bytes carry to the next handler" + ); + match outcome { + HandlerOutcome::Complete => return HandlerOutcome::Complete, + HandlerOutcome::Remaining => continue, + HandlerOutcome::Halted => return HandlerOutcome::Halted, + HandlerOutcome::TransportFault => return HandlerOutcome::TransportFault, + } + } + if bad.is_empty() { + HandlerOutcome::Complete + } else { + HandlerOutcome::Remaining + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::{Error, Result}; + use std::collections::{HashMap, HashSet}; + use std::sync::Arc; + use std::sync::atomic::AtomicU64; + use std::time::Duration; + + /// Synthetic disc: a set of dead LBAs, an optional transport-fault LBA, and + /// an injectable per-read time cost that advances a shared fake clock. No + /// real sleeps — the clock is an `AtomicU64` of nanoseconds so the reader + /// (which owns `&mut self`) and the `now` closure share one timeline while + /// staying `Send`. + struct FakeDisc { + dead: HashSet, + transport_at: Option, + clock_nanos: Arc, + per_read: Duration, + reads: Arc, + } + + impl SectorSource for FakeDisc { + fn read_sectors( + &mut self, + lba: u32, + count: u16, + buf: &mut [u8], + _recovery: bool, + ) -> Result { + self.reads.fetch_add(1, Ordering::Relaxed); + self.clock_nanos + .fetch_add(self.per_read.as_nanos() as u64, Ordering::Relaxed); + if let Some(t) = self.transport_at { + if (lba..lba + count as u32).contains(&t) { + return Err(Error::ScsiError { + opcode: crate::scsi::SCSI_READ_10, + status: crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, + }); + } + } + for l in lba..lba + count as u32 { + if self.dead.contains(&l) { + // Non-transport bad-sector error (CHECK CONDITION, 0x02). + return Err(Error::DiscRead { + sector: l as u64, + status: Some(0x02), + sense: None, + }); + } + } + let bytes = count as usize * SECTOR as usize; + for (i, b) in buf[..bytes].iter_mut().enumerate() { + *b = (lba as usize + i / SECTOR as usize) as u8; + } + Ok(bytes) + } + } + + /// Records every recovered span so a test can assert which sectors came back. + #[derive(Default)] + struct RecordSink { + got: HashMap, // pos -> bytes + } + impl RecoverySink for RecordSink { + fn recovered(&mut self, pos: u64, buf: &[u8]) { + self.got.insert(pos, buf.len()); + } + } + + /// A fake clock plus a disc sharing its timeline. + struct Harness { + clock_nanos: Arc, + reads: Arc, + base: Instant, + } + + impl Harness { + fn build(dead: &[u32], transport_at: Option, per_read: Duration) -> (Self, FakeDisc) { + let clock_nanos = Arc::new(AtomicU64::new(0)); + let reads = Arc::new(AtomicU64::new(0)); + let disc = FakeDisc { + dead: dead.iter().copied().collect(), + transport_at, + clock_nanos: clock_nanos.clone(), + per_read, + reads: reads.clone(), + }; + ( + Harness { + clock_nanos, + reads, + base: Instant::now(), + }, + disc, + ) + } + + fn now_fn(&self) -> impl Fn() -> Instant { + let c = self.clock_nanos.clone(); + let base = self.base; + move || base + Duration::from_nanos(c.load(Ordering::Relaxed)) + } + + fn read_count(&self) -> u64 { + self.reads.load(Ordering::Relaxed) + } + } + + fn lba(pos: u64) -> u32 { + (pos / SECTOR) as u32 + } + + #[test] + fn linear_forward_recovers_all_readable_and_leaves_only_dead() { + // Section [0, 10 sectors). Dead: sectors 3 and 7. Forward linear must + // recover the other 8 and leave ONLY 3 and 7 bad — proving it moves past + // a dead sector instead of stalling on it. Batch=1-effective here since + // the dead sectors force the narrow path; use a small section. + let dead = [3u32, 7u32]; + let (h, disc) = Harness::build(&dead, None, Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 10 * SECTOR); + // Generous deadline: 10 s from start. + let deadline = (ctx.now)() + Duration::from_secs(10); + let mut lin = Linear { + reverse: false, + fast: false, + }; + let out = lin.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::Remaining); + // Exactly the two dead sectors remain. + assert_eq!(bad.total_len(), 2 * SECTOR); + for &(p, l) in bad.ranges() { + assert_eq!(l, SECTOR); + assert!( + lba(p) == 3 || lba(p) == 7, + "unexpected bad sector {}", + lba(p) + ); + } + // All eight readable sectors were handed to the sink. + assert_eq!(sink.got.len(), 8); + } + + #[test] + fn linear_forward_front_dead_still_reaches_readable_tail() { + // THE bug: front dead, tail readable. Section [0, 40 sectors). First 32 + // (one whole batch) are dead; the tail 8 are readable. Forward linear + // must recover the tail — it does not hang at the front. + let dead: Vec = (0..32).collect(); + let (h, disc) = Harness::build(&dead, None, Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 40 * SECTOR); + let deadline = (ctx.now)() + Duration::from_secs(10); + let mut lin = Linear { + reverse: false, + fast: false, + }; + let out = lin.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::Remaining); + // The 32 dead front sectors remain; the 8-sector readable tail is + // recovered as one clean batch (one sink span covering 8 sectors). + assert_eq!(bad.total_len(), 32 * SECTOR); + assert_eq!(sink.got.len(), 1, "tail is one clean 8-sector batch"); + assert_eq!( + sink.got.get(&(32 * SECTOR)).copied(), + Some(8 * SECTOR as usize), + "tail batch not recovered" + ); + } + + #[test] + fn linear_honors_deadline_and_returns_promptly() { + // 1000 clean sectors, but each read costs 1 s and the budget is 3 s. The + // handler must stop after ~3 reads, NOT drain all 1000 — proving bounded + // wall-clock even on a huge range. + let (h, disc) = Harness::build(&[], None, Duration::from_secs(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 1000 * SECTOR); + let deadline = (ctx.now)() + Duration::from_secs(3); + let mut lin = Linear { + reverse: false, + fast: true, + }; + let out = lin.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::Remaining); + // Batch=32 clean sectors per read: a handful of reads at most, not 1000. + assert!( + h.read_count() <= 5, + "ran {} reads, expected <=5", + h.read_count() + ); + assert!(bad.total_len() > 0, "should not have drained the range"); + } + + #[test] + fn bisect_finds_good_middle_in_mostly_dead_range() { + // 9 sectors, only the middle (sector 4) readable. Bisect probes the + // middle first, recovers it, and the recursive halves' middles are dead. + let dead: Vec = (0..9).filter(|&l| l != 4).collect(); + let (h, disc) = Harness::build(&dead, None, Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 9 * SECTOR); + let deadline = (ctx.now)() + Duration::from_secs(10); + let mut bis = Bisect; + let out = bis.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::Remaining); + assert!( + sink.got.contains_key(&(4 * SECTOR)), + "good middle not found" + ); + assert_eq!( + bad.total_len(), + 8 * SECTOR, + "only the middle should recover" + ); + } + + #[test] + fn coordinator_reverse_then_forward_makes_progress_direction_matters() { + // Two dead sectors at opposite ends won't both be cleared by one + // direction alone in this contrived fixture, but the CHAIN clears every + // readable sector regardless of order. Prove the coordinator runs + // handler after handler and drains the readable set. + let dead = [0u32, 15u32]; // ends of a 16-sector section + let (h, disc) = Harness::build(&dead, None, Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 16 * SECTOR); + let mut handlers: Vec> = vec![ + Box::new(Linear { + reverse: true, + fast: false, + }), + Box::new(Linear { + reverse: false, + fast: false, + }), + Box::new(Bisect), + ]; + let deadline_base = (ctx.now)(); + let out = run_handlers(&mut ctx, &mut handlers, &mut bad, |_| { + deadline_base + Duration::from_secs(30) + }); + assert_eq!(out, HandlerOutcome::Remaining); + // 14 readable sectors recovered, only the two dead ends remain. + assert_eq!(bad.total_len(), 2 * SECTOR); + for &(p, _) in bad.ranges() { + assert!(lba(p) == 0 || lba(p) == 15); + } + } + + #[test] + fn coordinator_completes_when_no_dead_sectors() { + // A clean section drains to Complete on the first handler. + let (h, disc) = Harness::build(&[], None, Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + let mut bad = SubRanges::from_section(0, 64 * SECTOR); + let mut handlers: Vec> = vec![Box::new(Linear { + reverse: false, + fast: true, + })]; + let base = (ctx.now)(); + let out = run_handlers(&mut ctx, &mut handlers, &mut bad, |_| { + base + Duration::from_secs(30) + }); + assert_eq!(out, HandlerOutcome::Complete); + assert!(bad.is_empty()); + } + + #[test] + fn transport_fault_short_circuits() { + // A transport fault mid-range returns TransportFault immediately so the + // caller can un-wedge the drive. + let (h, disc) = Harness::build(&[], Some(5), Duration::from_millis(1)); + let mut disc = disc; + 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, + }; + // Single-sector batches so the transport LBA is hit directly. + let mut bad = SubRanges::from_section(0, 8 * SECTOR); + let deadline = (ctx.now)() + Duration::from_secs(10); + let mut lin = Linear { + reverse: false, + fast: true, + }; + let out = lin.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::TransportFault); + } + + #[test] + fn halt_token_returns_promptly() { + // Halt set before the call: the handler returns Halted on its first + // check, having done no reads. + let (h, disc) = Harness::build(&[], None, Duration::from_millis(1)); + let mut disc = disc; + let mut sink = RecordSink::default(); + let now = h.now_fn(); + let halt = AtomicBool::new(true); + let mut ctx = HandlerCtx { + reader: &mut disc, + sink: &mut sink, + now: &now, + halt: Some(&halt), + decrypt_is_aacs: false, + }; + let mut bad = SubRanges::from_section(0, 100 * SECTOR); + let deadline = (ctx.now)() + Duration::from_secs(10); + let mut lin = Linear { + reverse: false, + fast: true, + }; + let out = lin.recover(&mut ctx, &mut bad, deadline); + assert_eq!(out, HandlerOutcome::Halted); + assert_eq!(h.read_count(), 0, "halt must precede any read"); + } +} diff --git a/tests/passn_handler_ab.rs b/tests/passn_handler_ab.rs index 1758ca1..be03194 100644 --- a/tests/passn_handler_ab.rs +++ b/tests/passn_handler_ab.rs @@ -699,31 +699,27 @@ fn profile_07_medium_then_good() { trace, ); - // GOLDEN: cache-priming and scatter-recovery are BOTH gone (ruled out - // by live drive probing — neither improves recovery; the drive's - // per-sector ECC is media-bound, not approach-bound). With the script - // "fail, fail, ok" per bad sector, recovery in ONE pass now hinges on - // bisect re-reading a sector enough times to consume its two failing - // steps: a sector caught in a failed batch is re-read as the batch - // halves (32→16→8→4→2→1), which for most of the cluster reaches the - // Ok step. The 2 sectors bisect reads fewest times stay NonTrimmed and - // recover on the NEXT pass — exactly the multi-pass design this test's - // header describes ("bad sectors stay NonTrimmed in this pass"). So - // 254 of 256 sectors recover here; 2 (4096 B) defer. + // GOLDEN (handler-chain engine): with the script "fail, fail, ok" per bad + // sector, each bad sector needs three reads to recover. The chain re-reads a + // sector across successive handlers — linear reverse/forward (each narrows a + // failed batch to per-sector reads) then bisect — so every sector in the + // cluster is read enough times to consume its two failing steps and reach + // the Ok step WITHIN one pass. So all 256 sectors recover here; none defer. + // (The old batch-halving loop reached only 254; the chain is strictly + // better because more handlers re-touch each sector.) assert_eq!( stats.bytes_good, - 254 * 2048, - "07_medium_then_good bytes_good (bisect re-reads consume the \ - fail,fail,ok script for most of the cluster; 2 defer to next pass)" + 256 * 2048, + "07_medium_then_good bytes_good (handler chain re-reads each sector \ + across handlers, consuming the fail,fail,ok script for the whole cluster)" ); assert_eq!( stats.bytes_unreadable, 0, "07_medium_then_good bytes_unreadable (NonTrimmed, never terminal in one pass)" ); assert_eq!( - stats.bytes_pending, - 2 * 2048, - "07_medium_then_good bytes_pending (2 sectors deferred to the next pass)" + stats.bytes_pending, 0, + "07_medium_then_good bytes_pending (whole cluster recovered in one pass)" ); assert!(!pr.halted, "07_medium_then_good halted"); assert!( @@ -820,19 +816,21 @@ fn profile_08_batch_fail_singles_ok() { // injection point in `handle_read_failure` and extend this fixture // with the wedge/NOT_READY profiles too. -// ─────────────── Fast-capture (breadth-first) recovery — #50 ─────────────── +// ──────── Handler chain recovers re-readable sectors inside a bad block ──────── // -// `fast_capture = true` reads each bad range ONCE at the batch size and leaves -// every FAILED block NonTrimmed for a later pass — no bisection, no per-sector -// grind. This is the breadth-first "fast-capture every section first, then -// escalate" ordering: a first retry pass grabs every range's readable blocks -// quickly instead of grinding section 1 to exhaustion before touching section 2. +// A bad range holds one genuinely-dead sector surrounded by readable ones. The +// handler chain's linear pass narrows a failed batch to per-sector reads, so it +// recovers EVERY re-readable sector and leaves ONLY the dead sector NonTrimmed — +// strictly better than the old fast-capture path, which left the whole failed +// 32-block untouched. (`fast_capture` is now inert: the chain supersedes it. The +// breadth-first "fast on all ranges, then escalate" ORDERING it once provided is +// a scheduling concern for the handler scheduler, tracked separately.) // -// The load-bearing invariant: NO data is dropped. A failed block becomes -// NonTrimmed (pending, retried by a later granular pass), NEVER Unreadable. +// The load-bearing invariant is unchanged: NO data is dropped. A still-bad +// sector becomes NonTrimmed (pending, retried by a later pass), NEVER Unreadable. #[test] -fn fast_capture_keeps_readable_blocks_and_leaves_bad_nontrimmed_unbisected() { +fn handler_chain_recovers_readable_sectors_leaving_only_dead_pending() { let capacity_sectors: u32 = 256; let (mut reader, trace) = ScriptedSectorReader::new(capacity_sectors); // One bad sector at LBA 130 — inside the LOW 32-sector block of the range. @@ -875,48 +873,25 @@ fn fast_capture_keeps_readable_blocks_and_leaves_bad_nontrimmed_unbisected() { let map_path = libfreemkv::disc::mapfile_path_for(&iso_path); let stats = Mapfile::load(&map_path).unwrap().stats(); - // The clean 32-block [160,192) recovered (+32 sectors over the 192 already - // Finished); the bad 32-block [128,160) is left NonTrimmed — NOT Unreadable. - // fast_capture never gives up; the next (granular) pass retries it. - // Conservation: the 64-sector range split into 32 good + 32 still-pending, - // nothing lost. + // The clean sectors of [128,192) all recover; only the one always-dead + // sector (LBA 130) stays NonTrimmed — NOT Unreadable. The chain narrows the + // failed batch to per-sector reads, so 63 of the 64 range sectors come back. + // Conservation: 255 good + 1 still-pending = the full 256, nothing lost. assert_eq!( stats.bytes_unreadable, 0, - "fast capture must never mark Unreadable" + "recovery must never mark Unreadable in a pass" ); assert_eq!( - stats.bytes_pending, - 32 * 2048, - "the bad block stays NonTrimmed for the next pass" + stats.bytes_pending, 2048, + "only the single always-dead sector (LBA 130) stays NonTrimmed" ); assert_eq!( stats.bytes_good, - 224 * 2048, - "192 pre-Finished + 32 newly recovered" + 255 * 2048, + "every sector except the one dead LBA is recovered" ); - // No bisection: the range [128,192) is read in exactly TWO 32-sector batch - // reads (clean half + bad half). Full mode would halve [128,160) into - // count=16,8,…,1 reads to isolate sector 130; fast capture marks the whole - // 32-block NonTrimmed in one read. (Reads outside the range — e.g. a lone - // count=1 probe at the capacity edge — are unrelated and ignored.) - let t = trace.lock().unwrap(); - let range_reads: Vec<_> = t - .iter() - .filter(|&&(lba, _, _)| (128..192).contains(&lba)) - .collect(); - assert_eq!( - range_reads.len(), - 2, - "range read in 2 batches (clean + bad), no bisection; trace={:?}", - *t - ); - assert!( - range_reads.iter().all(|&&(_, count, _)| count == 32), - "fast capture must not bisect — both range reads are the full batch; trace={:?}", - *t - ); - drop(t); + let _ = trace; // read trace retained by the fixture; no ordering assertion here let _ = std::fs::remove_file(&iso_path); let _ = std::fs::remove_file(&map_path);