From 2dd98c3e32d985597e59ab5999d7f425f2e24e53 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 1 Jul 2026 09:14:32 -0700 Subject: [PATCH] recovery: fresh-eyes audit fixes (handlers + Pass-N engine + sweep) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handlers (section_recover.rs): - Bisect expand loops now honor ctx.halted() (were deadline-only, so a Stop could hang up to 60s vacuuming a readable island). - read_span: explicit Transport arm so a bus-abort read isn't counted as unproductive grinding; debug_assert the sector-aligned span invariant. - Scoreboard rank: an attempted-but-zero-time handler (e.g. returned Halted on its first check) now ranks BOTTOM, not top — it no longer crowds out proven performers. - Document the wedge tier-size coupling + new regression test that a 2-handler (tier-1) chain still catches a wedge via cross-section streak. Pass-N engine (patch.rs): - Rebuild PatchOutcome stats AFTER the post-read re-verify downgrade flush (was snapshotting before it, over-reporting bytes_good / recovered and risking a 'perfect rip' verdict on an imperfect one). - Progress 'recovered' composes the still-bad set to MATCH work_total (subtract NonTried, add Unreadable) so the bar can't pin at 0 on a partially-swept disc or run backward on the Unreadable→NonTrimmed relabel. - Remove dead work_done field; rewrite the stale 'adaptive batching' comment to describe the handler chain and mark block_sectors/full_recovery as informational-only. Sweep (disc/mod.rs): - Saturating arithmetic at the damage-jump position math (honor the read_error side's documented defence-in-depth guarantee). Deferred (noted, need focused passes): fast_capture re-introduction, Pass-1 halt-misclassified-as-jump, bytes_good display inflation, the always-zero blocks_* telemetry, Pass-1 jump-on-first-error policy. --- src/disc/mod.rs | 10 +++- src/disc/patch.rs | 59 +++++++++++++---------- src/disc/section_recover.rs | 95 ++++++++++++++++++++++++++++++++++++- 3 files changed, 136 insertions(+), 28 deletions(-) diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 57cde4e..61c2393 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -3505,7 +3505,15 @@ impl Disc { ); } - let jump_pos = (pos + block_bytes + sectors * 2048).min(region_end); + // Saturating throughout — the read_error side + // computes the sector count with saturating_mul as + // "defence in depth"; honor the same guarantee at + // the consuming multiply/add so a pathological jump + // distance can't wrap. + let jump_pos = pos + .saturating_add(block_bytes) + .saturating_add(sectors.saturating_mul(2048)) + .min(region_end); let gap_start = pos + block_bytes; let gap_bytes = jump_pos.saturating_sub(gap_start); if gap_bytes > 0 { diff --git a/src/disc/patch.rs b/src/disc/patch.rs index 0146bb3..321b00d 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -655,7 +655,6 @@ pub(super) struct PatchLoopState { pub blocks_read_ok: u64, pub blocks_read_failed: u64, pub unreadable_count: u64, - pub work_done: u64, // Clock seam: the handler chain reads wall time through this rather than // calling `Instant::now()` inline, so the per-handler deadline is driven by // an injectable clock and deterministic tests can wind it forward. @@ -702,7 +701,6 @@ impl PatchLoopState { blocks_read_ok: 0, blocks_read_failed: 0, unreadable_count: 0, - work_done: 0, now, bytes_good_before, total_bytes, @@ -955,7 +953,6 @@ impl PatchCtx<'_, '_> { for &(pos, len) in bad.ranges() { send_or_abort(self.pipe, PatchItem::NonTrimmed { pos, len })?; } - self.state.work_done = self.state.work_done.saturating_add(range_size); } if self @@ -1021,13 +1018,25 @@ impl Disc { .map(|t| bytes_bad_in_title(t, &bad_ranges_now)) .unwrap_or(0); let main_title = self.titles.first(); - // Progress = bytes RECOVERED so far (initial bad − still-pending), not a + // Progress = bytes RECOVERED so far (initial bad − still-bad), not a // per-range counter. With breadth-first tiers the readable bulk comes // back during tier 0 before any range is "finished", so a range-counter // sits at 0% while hundreds of MB are actually recovered. Deriving it - // from the live pending count makes the bar (and the speed the client + // from the live still-bad count makes the bar (and the speed the client // computes from its delta) reflect real recovery the instant it happens. - let recovered = state.work_total.saturating_sub(s.bytes_pending); + // + // Compose the still-bad set to MATCH `work_total` (= the initial + // NonTrimmed + NonScraped + Unreadable, no NonTried). `bytes_pending` + // alone is the wrong denominator: it INCLUDES NonTried (so on a partially + // swept disc it exceeds `work_total` and saturating_sub pins the bar at 0) + // and EXCLUDES Unreadable (so the final-tier Unreadable→NonTrimmed relabel + // would drive `recovered` backward). Subtract NonTried and add Unreadable + // back so the two sets line up and progress stays monotonic. + let still_bad_work = s + .bytes_pending + .saturating_sub(s.bytes_nontried) + .saturating_add(s.bytes_unreadable); + let recovered = state.work_total.saturating_sub(still_bad_work); let pp = crate::progress::PassProgress { kind, work_done: recovered, @@ -1188,24 +1197,16 @@ impl Disc { ); } - // Adaptive batching: read at `state.current_batch`, HALVE on a - // batch-read failure (bisect to isolate the bad sector), and - // DOUBLE back toward `state.initial_batch` on each clean read. - // Rationale: dense damage scattered through a - // NonTrimmed range is rare — most "bad ranges" in pass N have - // lots of good sectors that swept-by-default landed inside. - // Batch reads walk those at ~32x the speed of singles, - // dropping to 1 only when the drive actually returns an error. - // Guarantees: - // - no good sector is ever marked NonTrimmed because it - // was bundled in a failed batch — failed batches are - // "split decisions", not recorded failures - // - drop-to-1 retries the SAME starting position, so every - // sector in the failed batch is individually probed - // Clamp to at least 1 sector. block_sectors is public - // (Option); Some(0) would compute a zero-length read per - // iteration, never advance block_end, and busy-spin the range - // until its watchdog fired. + // Read sizing and fast-vs-deep recovery are owned by the handler chain + // (`section_recover.rs`): it reads at a fixed `BATCH_SECTORS`, bisects to + // isolate readable islands, and selects fast vs 60 s deep reads per + // handler/tier. The old adaptive `current_batch` / halve-on-failure / + // double-back loop that this comment used to describe no longer exists. + // `block_sectors` and `full_recovery` therefore no longer drive behavior + // — they survive only as the PassKind label and the diagnostics logged + // below (informational-only; a caller can't change read sizing or the + // recovery timeout through them). Clamp to ≥1 so the label math never + // underflows on a `Some(0)`. let initial_batch = opts.block_sectors.unwrap_or(1).max(1); let recovery = opts.full_recovery; log_patch_start_snapshot(&initial_entries, &initial_stats, bytes_good_before); @@ -1255,7 +1256,7 @@ impl Disc { // sink's summary. `close` failing on a regular-file sync_all is // surfaced here as `Error::IoError`, matching pre-split // behaviour. - let summary = pipe.finish()?; + let mut summary = pipe.finish()?; // Scoped post-read re-verify (decrypt-fail == bad read). The consumer // has flushed the ISO + mapfile; re-read each clip unit this pass touched @@ -1286,6 +1287,14 @@ impl Disc { ); } let _ = m.flush(); + // The re-verify ran AFTER `pipe.finish()` snapshotted + // `summary.stats`, so those stats still count the just- + // downgraded units as good. Refresh from the mapfile so + // `build_outcome` reports the true post-downgrade picture + // (bytes_good ↓, bytes_pending ↑) — otherwise the caller + // over-reports recovery and can call an imperfect rip + // "complete". + summary.stats = m.stats(); tracing::info!( target: "freemkv::verify", phase = "patch.reverify", diff --git a/src/disc/section_recover.rs b/src/disc/section_recover.rs index d7499e1..b702626 100644 --- a/src/disc/section_recover.rs +++ b/src/disc/section_recover.rs @@ -68,6 +68,13 @@ const UNPRODUCTIVE_YIELD: u32 = 4; /// even when every bad sub-range is smaller than the streak. Learned the hard way /// (2026-07-01): the handler chain ground a wedged drive for 28 min at 0 B/s /// because a fast-fail sense was classified as an ordinary bad sector. +/// +/// Detection latency scales with how much streak one section can build. Tier 0's +/// 4 handlers × [`UNPRODUCTIVE_YIELD`] = 16 reads, so a single large wedged +/// section trips it within one `run_handlers` call. Tier 1 has only 2 handlers +/// (max 8 per section), so a wedge seen only in tier 1 relies on the pass-level +/// streak PERSISTING across sections to reach the threshold — regression-tested +/// by `wedge_streak_persists_across_sections_for_tier1`. const WEDGE_ABORT_STREAK: u32 = 16; /// Where a handler left the section after its bounded attempt. @@ -184,6 +191,14 @@ fn read_span( ) -> ReadHit { let lba = (pos / SECTOR) as u32; let bytes = count as usize * SECTOR as usize; + // Every SubRange enters via `from_section` / `remove`, which keep byte + // offsets sector-aligned, so a handler never asks for a sub-sector span. Pin + // that invariant: a zero `count` (span < SECTOR) would be a 0-sector read + // that silently "recovers" nothing — surface the caller bug in tests. + debug_assert!( + count >= 1 && pos % SECTOR == 0, + "read_span requires a sector-aligned, >=1-sector span (pos={pos}, count={count})" + ); let hit = match recovery_read(ctx.reader, ctx.decrypt_is_aacs, lba, count, buf, recovery) { Ok(_) => { ctx.sink.recovered(pos, &buf[..bytes]); @@ -221,7 +236,13 @@ fn read_span( ctx.unproductive = 0; ctx.wedge_streak = 0; } - _ => ctx.unproductive = ctx.unproductive.saturating_add(1), + // A Bad read is unproductive grinding — advance the yield streak. + ReadHit::Bad => ctx.unproductive = ctx.unproductive.saturating_add(1), + // A Transport hit aborts the handler immediately (bus fault / wedge + // escalation), so it is NOT unproductive grinding — leave the streak + // untouched (the counter is never read again after TransportFault, but + // keep the semantics honest in case an arm is ever reordered). + ReadHit::Transport => {} } // Heartbeat after every read (the tick closure throttles to ~250 ms) so the // UI's bar/speed move DURING a handler, not just when the section finishes. @@ -370,6 +391,9 @@ impl SectionHandler for Bisect { let mut fwd = mid + SECTOR; let mut step = batch; while fwd < end { + if ctx.halted() { + return HandlerOutcome::Halted; + } if ctx.timed_out(deadline) { return HandlerOutcome::Remaining; } @@ -398,6 +422,9 @@ impl SectionHandler for Bisect { let mut bwd = mid; let mut step = batch; while bwd > rp { + if ctx.halted() { + return HandlerOutcome::Halted; + } if ctx.timed_out(deadline) { return HandlerOutcome::Remaining; } @@ -569,8 +596,14 @@ impl HandlerScoreboard { /// Ranking key (higher runs earlier). Untried → top, so it gets calibrated. fn rank(&self, name: &str) -> u64 { match self.stats.get(name) { + // Never attempted → top, so every handler is calibrated once. None => u64::MAX, - Some(s) if s.nanos == 0 => u64::MAX, + // Attempted but recorded no measurable time — e.g. it returned + // `Halted` on its first check or did zero reads. It proved nothing, + // so rank it at the BOTTOM (0), not the top: otherwise a called-but- + // idle handler perpetually crowds out proven performers. (An entry + // exists only after `record`, so `Some` always means attempts ≥ 1.) + Some(s) if s.nanos == 0 => 0, Some(s) => Self::rate(s), } } @@ -1108,6 +1141,64 @@ mod tests { ); } + #[test] + fn wedge_streak_persists_across_sections_for_tier1() { + // Tier 1 is only TWO handlers, so one wedged section builds at most + // 2 × UNPRODUCTIVE_YIELD = 8 streak — below WEDGE_ABORT_STREAK (16). The + // wedge is caught only because the pass-level wedge_streak PERSISTS across + // sections. Simulate what PatchCtx does: carry wedge_streak in/out of each + // per-section run_handlers call, and assert the abort lands on a LATER + // section, not the first. + let (h, disc) = Harness::build(&[], None, Duration::from_millis(1)); + let mut disc = disc; + disc.wedge = (0..4000u32).collect(); + let mut sink = RecordSink::default(); + let now = h.now_fn(); + let mut carried = 0u32; // the pass-level wedge_streak + let mut caught_on: Option = None; + for section in 0..6usize { + let mut ctx = HandlerCtx { + reader: &mut disc, + sink: &mut sink, + now: &now, + halt: None, + decrypt_is_aacs: false, + tick: None, + unproductive: 0, + wedge_streak: carried, + }; + // Distinct 100-sector section per iteration, all within the wedge set. + let pos = (section as u64) * 100 * SECTOR; + let mut bad = SubRanges::from_section(pos, 100 * SECTOR); + // Tier-1 shape: two slow Linear handlers, nothing that reaches 16 alone. + let mut handlers: Vec> = vec![ + Box::new(Linear { + reverse: true, + fast: false, + }), + Box::new(Linear { + reverse: false, + fast: false, + }), + ]; + let mut sb = HandlerScoreboard::default(); + let out = run_handlers(&mut ctx, &mut handlers, &mut bad, &mut sb, |_| { + (h.now_fn())() + Duration::from_secs(60) + }); + carried = ctx.wedge_streak; + if out == HandlerOutcome::TransportFault { + caught_on = Some(section); + break; + } + } + let caught = caught_on.expect("a two-handler tier must still catch the wedge"); + assert!( + caught >= 1, + "one 2-handler section can't reach the streak alone; the wedge must be \ + caught via cross-section accumulation, not on section 0 (caught on {caught})" + ); + } + #[test] fn halt_token_returns_promptly() { // Halt set before the call: the handler returns Halted on its first