recovery: fresh-eyes audit fixes (handlers + Pass-N engine + sweep)
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.
This commit is contained in:
+9
-1
@@ -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_start = pos + block_bytes;
|
||||||
let gap_bytes = jump_pos.saturating_sub(gap_start);
|
let gap_bytes = jump_pos.saturating_sub(gap_start);
|
||||||
if gap_bytes > 0 {
|
if gap_bytes > 0 {
|
||||||
|
|||||||
+34
-25
@@ -655,7 +655,6 @@ pub(super) struct PatchLoopState {
|
|||||||
pub blocks_read_ok: u64,
|
pub blocks_read_ok: u64,
|
||||||
pub blocks_read_failed: u64,
|
pub blocks_read_failed: u64,
|
||||||
pub unreadable_count: u64,
|
pub unreadable_count: u64,
|
||||||
pub work_done: u64,
|
|
||||||
// Clock seam: the handler chain reads wall time through this rather than
|
// Clock seam: the handler chain reads wall time through this rather than
|
||||||
// calling `Instant::now()` inline, so the per-handler deadline is driven by
|
// calling `Instant::now()` inline, so the per-handler deadline is driven by
|
||||||
// an injectable clock and deterministic tests can wind it forward.
|
// an injectable clock and deterministic tests can wind it forward.
|
||||||
@@ -702,7 +701,6 @@ impl PatchLoopState {
|
|||||||
blocks_read_ok: 0,
|
blocks_read_ok: 0,
|
||||||
blocks_read_failed: 0,
|
blocks_read_failed: 0,
|
||||||
unreadable_count: 0,
|
unreadable_count: 0,
|
||||||
work_done: 0,
|
|
||||||
now,
|
now,
|
||||||
bytes_good_before,
|
bytes_good_before,
|
||||||
total_bytes,
|
total_bytes,
|
||||||
@@ -955,7 +953,6 @@ impl PatchCtx<'_, '_> {
|
|||||||
for &(pos, len) in bad.ranges() {
|
for &(pos, len) in bad.ranges() {
|
||||||
send_or_abort(self.pipe, PatchItem::NonTrimmed { pos, len })?;
|
send_or_abort(self.pipe, PatchItem::NonTrimmed { pos, len })?;
|
||||||
}
|
}
|
||||||
self.state.work_done = self.state.work_done.saturating_add(range_size);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
if self
|
if self
|
||||||
@@ -1021,13 +1018,25 @@ impl Disc {
|
|||||||
.map(|t| bytes_bad_in_title(t, &bad_ranges_now))
|
.map(|t| bytes_bad_in_title(t, &bad_ranges_now))
|
||||||
.unwrap_or(0);
|
.unwrap_or(0);
|
||||||
let main_title = self.titles.first();
|
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
|
// per-range counter. With breadth-first tiers the readable bulk comes
|
||||||
// back during tier 0 before any range is "finished", so a range-counter
|
// 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
|
// 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.
|
// 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 {
|
let pp = crate::progress::PassProgress {
|
||||||
kind,
|
kind,
|
||||||
work_done: recovered,
|
work_done: recovered,
|
||||||
@@ -1188,24 +1197,16 @@ impl Disc {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Adaptive batching: read at `state.current_batch`, HALVE on a
|
// Read sizing and fast-vs-deep recovery are owned by the handler chain
|
||||||
// batch-read failure (bisect to isolate the bad sector), and
|
// (`section_recover.rs`): it reads at a fixed `BATCH_SECTORS`, bisects to
|
||||||
// DOUBLE back toward `state.initial_batch` on each clean read.
|
// isolate readable islands, and selects fast vs 60 s deep reads per
|
||||||
// Rationale: dense damage scattered through a
|
// handler/tier. The old adaptive `current_batch` / halve-on-failure /
|
||||||
// NonTrimmed range is rare — most "bad ranges" in pass N have
|
// double-back loop that this comment used to describe no longer exists.
|
||||||
// lots of good sectors that swept-by-default landed inside.
|
// `block_sectors` and `full_recovery` therefore no longer drive behavior
|
||||||
// Batch reads walk those at ~32x the speed of singles,
|
// — they survive only as the PassKind label and the diagnostics logged
|
||||||
// dropping to 1 only when the drive actually returns an error.
|
// below (informational-only; a caller can't change read sizing or the
|
||||||
// Guarantees:
|
// recovery timeout through them). Clamp to ≥1 so the label math never
|
||||||
// - no good sector is ever marked NonTrimmed because it
|
// underflows on a `Some(0)`.
|
||||||
// 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<u16>); Some(0) would compute a zero-length read per
|
|
||||||
// iteration, never advance block_end, and busy-spin the range
|
|
||||||
// until its watchdog fired.
|
|
||||||
let initial_batch = opts.block_sectors.unwrap_or(1).max(1);
|
let initial_batch = opts.block_sectors.unwrap_or(1).max(1);
|
||||||
let recovery = opts.full_recovery;
|
let recovery = opts.full_recovery;
|
||||||
log_patch_start_snapshot(&initial_entries, &initial_stats, bytes_good_before);
|
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
|
// sink's summary. `close` failing on a regular-file sync_all is
|
||||||
// surfaced here as `Error::IoError`, matching pre-split
|
// surfaced here as `Error::IoError`, matching pre-split
|
||||||
// behaviour.
|
// behaviour.
|
||||||
let summary = pipe.finish()?;
|
let mut summary = pipe.finish()?;
|
||||||
|
|
||||||
// Scoped post-read re-verify (decrypt-fail == bad read). The consumer
|
// Scoped post-read re-verify (decrypt-fail == bad read). The consumer
|
||||||
// has flushed the ISO + mapfile; re-read each clip unit this pass touched
|
// has flushed the ISO + mapfile; re-read each clip unit this pass touched
|
||||||
@@ -1286,6 +1287,14 @@ impl Disc {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
let _ = m.flush();
|
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!(
|
tracing::info!(
|
||||||
target: "freemkv::verify",
|
target: "freemkv::verify",
|
||||||
phase = "patch.reverify",
|
phase = "patch.reverify",
|
||||||
|
|||||||
@@ -68,6 +68,13 @@ const UNPRODUCTIVE_YIELD: u32 = 4;
|
|||||||
/// even when every bad sub-range is smaller than the streak. Learned the hard way
|
/// 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
|
/// (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.
|
/// 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;
|
const WEDGE_ABORT_STREAK: u32 = 16;
|
||||||
|
|
||||||
/// Where a handler left the section after its bounded attempt.
|
/// Where a handler left the section after its bounded attempt.
|
||||||
@@ -184,6 +191,14 @@ fn read_span(
|
|||||||
) -> ReadHit {
|
) -> ReadHit {
|
||||||
let lba = (pos / SECTOR) as u32;
|
let lba = (pos / SECTOR) as u32;
|
||||||
let bytes = count as usize * SECTOR as usize;
|
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) {
|
let hit = match recovery_read(ctx.reader, ctx.decrypt_is_aacs, lba, count, buf, recovery) {
|
||||||
Ok(_) => {
|
Ok(_) => {
|
||||||
ctx.sink.recovered(pos, &buf[..bytes]);
|
ctx.sink.recovered(pos, &buf[..bytes]);
|
||||||
@@ -221,7 +236,13 @@ fn read_span(
|
|||||||
ctx.unproductive = 0;
|
ctx.unproductive = 0;
|
||||||
ctx.wedge_streak = 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
|
// 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.
|
// 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 fwd = mid + SECTOR;
|
||||||
let mut step = batch;
|
let mut step = batch;
|
||||||
while fwd < end {
|
while fwd < end {
|
||||||
|
if ctx.halted() {
|
||||||
|
return HandlerOutcome::Halted;
|
||||||
|
}
|
||||||
if ctx.timed_out(deadline) {
|
if ctx.timed_out(deadline) {
|
||||||
return HandlerOutcome::Remaining;
|
return HandlerOutcome::Remaining;
|
||||||
}
|
}
|
||||||
@@ -398,6 +422,9 @@ impl SectionHandler for Bisect {
|
|||||||
let mut bwd = mid;
|
let mut bwd = mid;
|
||||||
let mut step = batch;
|
let mut step = batch;
|
||||||
while bwd > rp {
|
while bwd > rp {
|
||||||
|
if ctx.halted() {
|
||||||
|
return HandlerOutcome::Halted;
|
||||||
|
}
|
||||||
if ctx.timed_out(deadline) {
|
if ctx.timed_out(deadline) {
|
||||||
return HandlerOutcome::Remaining;
|
return HandlerOutcome::Remaining;
|
||||||
}
|
}
|
||||||
@@ -569,8 +596,14 @@ impl HandlerScoreboard {
|
|||||||
/// Ranking key (higher runs earlier). Untried → top, so it gets calibrated.
|
/// Ranking key (higher runs earlier). Untried → top, so it gets calibrated.
|
||||||
fn rank(&self, name: &str) -> u64 {
|
fn rank(&self, name: &str) -> u64 {
|
||||||
match self.stats.get(name) {
|
match self.stats.get(name) {
|
||||||
|
// Never attempted → top, so every handler is calibrated once.
|
||||||
None => u64::MAX,
|
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),
|
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<usize> = 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<Box<dyn SectionHandler>> = 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]
|
#[test]
|
||||||
fn halt_token_returns_promptly() {
|
fn halt_token_returns_promptly() {
|
||||||
// Halt set before the call: the handler returns Halted on its first
|
// Halt set before the call: the handler returns Halted on its first
|
||||||
|
|||||||
Reference in New Issue
Block a user