v0.13.21 — bisect-on-fail in Disc::copy + 10s caller READ timeout
Fixes the BU40N wedge cycle that has been chasing us through v0.13.18-20. Two changes, both backed by empirical live-hardware probes recorded in freemkv-private/docs/TEST_PLAN.md: 1. scsi/mod.rs: READ_TIMEOUT_MS 1500 → 10000 ms. Cold-start seek on the BU40N takes ~1.5 s. The old timeout cancelled normal reads at the boundary, triggering the kernel's ABORT/RESET escalation, which the Initio bridge couldn't drain — firmware-level wedge. 10 s catches every legitimate slow read (max successful ECC recovery: 2.6 s; cold-start: 1.5 s) with margin and short-circuits truly bad sectors at ~10 s. 2. disc/mod.rs: Disc::copy bisect-on-fail (replaces skip-forward). Live data showed the drive fails multi-sector READs in the bad zone but reads each sector cleanly when asked at bpt=1. Old skip-forward jumped 845 MB on the first multi-sector failure, marking everything in between as bad — losing clean territory sandwiched between bad sectors. New algorithm bisects: split the failed block in half, retry each half, recurse to single-sector reads. Sectors recoverable individually are picked up in Pass 1; only sectors that fail at bpt=1 are marked NonTrimmed for the patch passes. Stack-based DFS, log2(batch) = 6 levels for the default 60-sector batch. Multi-pass machinery is untouched. Pass 2..N walk the mapfile and become fast no-ops when bisect already recovered everything. Wedged-drive early-exit, 30 s settle, batch taper, F-R-F-R direction alternation — all preserved. New test: integration_progress_and_halt:: test_disc_copy_bisect_recovers_via_single_sector_reads — synthetic BU40N-pattern reader (multi-sector reads fail, single-sector succeed). Pre-patch: lost everything to skip-forward. Post-patch: 100 % bytes_good. Plus the 10 sense-key parser tests from the 0.13.20 test-coverage pass. Empirical recovery on Dune 2 UHD on the BU40N (per TEST_PLAN.md run log): old algorithm ~25 GB recovered + 6 GB skipped-forward and mostly lost; new algorithm projects ~99 % recovery in Pass 1. Audits + raw probe data: - freemkv-private/docs/TEST_PLAN.md (run log) - freemkv-private/docs/audits/2026-04-26-scsi-architecture-research.md
This commit is contained in:
@@ -537,3 +537,92 @@ fn test_disc_copy_halts_promptly_on_failing_reader() {
|
||||
"halt fired before sweep completed; bytes_pending must be > 0"
|
||||
);
|
||||
}
|
||||
|
||||
// ── 8. Bisect-on-fail recovers data the drive can read individually ──────
|
||||
//
|
||||
// Empirically observed on the LG BU40N: in damaged regions the drive fails
|
||||
// multi-sector READ commands but reads each sector cleanly when asked one
|
||||
// at a time. Disc::copy's bisect-on-fail must recover those sectors
|
||||
// without bailing or skip-forwarding past clean territory.
|
||||
//
|
||||
// Fixture: a reader that returns Err for any read with count > 1, and Ok
|
||||
// for count == 1. With bisect-on-fail, we must observe a 100 % bytes_good
|
||||
// outcome — every sector recovered via the bisection.
|
||||
|
||||
struct BlockSizeFailingReader {
|
||||
capacity: u32,
|
||||
}
|
||||
|
||||
impl SectorReader for BlockSizeFailingReader {
|
||||
fn read_sectors(
|
||||
&mut self,
|
||||
lba: u32,
|
||||
count: u16,
|
||||
buf: &mut [u8],
|
||||
_recovery: bool,
|
||||
) -> Result<usize> {
|
||||
if count == 1 {
|
||||
// Single-sector reads succeed — fill the sector with a marker.
|
||||
for chunk in buf.chunks_mut(SECTOR_SIZE) {
|
||||
chunk.fill((lba & 0xff) as u8);
|
||||
}
|
||||
Ok(buf.len())
|
||||
} else {
|
||||
Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 })
|
||||
}
|
||||
}
|
||||
|
||||
fn capacity(&self) -> u32 {
|
||||
self.capacity
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_disc_copy_bisect_recovers_via_single_sector_reads() {
|
||||
// 256 sectors = 0.5 MB. Reader fails any multi-sector read but
|
||||
// succeeds on bpt=1. Bisection must descend log2(batch) levels and
|
||||
// recover every sector. This is the BU40N bad-zone pattern in
|
||||
// miniature.
|
||||
let capacity_sectors: u32 = 256;
|
||||
let total_bytes: u64 = capacity_sectors as u64 * SECTOR_SIZE as u64;
|
||||
|
||||
let mut reader = BlockSizeFailingReader {
|
||||
capacity: capacity_sectors,
|
||||
};
|
||||
let disc = synthetic_disc(capacity_sectors);
|
||||
|
||||
let tmp = tempfile::NamedTempFile::new().expect("tempfile create");
|
||||
let iso_path = tmp.path().to_path_buf();
|
||||
drop(tmp);
|
||||
|
||||
let opts = CopyOptions {
|
||||
decrypt: false,
|
||||
skip_on_error: true,
|
||||
skip_forward: true,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let result = disc
|
||||
.copy(&mut reader, &iso_path, &opts)
|
||||
.expect("copy returns Ok");
|
||||
|
||||
let _ = std::fs::remove_file(&iso_path);
|
||||
let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&iso_path));
|
||||
|
||||
// Bisect must recover every sector — the drive could read each one
|
||||
// individually, and our algorithm must descend to that.
|
||||
assert_eq!(
|
||||
result.bytes_good, total_bytes,
|
||||
"bisect-on-fail must recover every sector via single-sector reads. \
|
||||
Got bytes_good={} of total {}",
|
||||
result.bytes_good, total_bytes
|
||||
);
|
||||
assert_eq!(
|
||||
result.bytes_pending, 0,
|
||||
"no sectors should be left NonTrimmed after a successful bisect"
|
||||
);
|
||||
assert!(
|
||||
result.complete,
|
||||
"complete=true expected when every sector recovered"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,288 @@
|
||||
//! Integration tests for the SCSI error-decoding contract.
|
||||
//!
|
||||
//! v0.13.20 rewrote `scsi/linux.rs` to a synchronous blocking SG_IO and
|
||||
//! consolidated sense-key parsing into the `parse_sense_key` helper that
|
||||
//! every platform backend now shares. The actual `ioctl(SG_IO, ...)` call
|
||||
//! is impossible to mock without a kernel — see
|
||||
//! `freemkv-private/docs/audits/2026-04-26-scsi-architecture-research.md`
|
||||
//! for why the audit recommends against libc shims here.
|
||||
//!
|
||||
//! These tests pin the *contract* every backend must satisfy:
|
||||
//!
|
||||
//! 1. Healthy result → `Ok(ScsiResult { bytes_transferred = data.len() - resid })`.
|
||||
//! 2. Transport-level failure (`host_status` or `driver_status` non-zero)
|
||||
//! → `Error::ScsiError { status: 0xFF, sense_key: 0 }`. Used by
|
||||
//! `drive_has_disc` to detect the wedge signature.
|
||||
//! 3. SCSI-level failure (status non-zero, sense buffer populated) →
|
||||
//! `Error::ScsiError { status, sense_key }` with the parsed key.
|
||||
//! 4. Sense-key parsing handles descriptor (0x72/0x73) and fixed
|
||||
//! (0x70/0x71) response codes; missing sense data → key 0.
|
||||
//!
|
||||
//! The mock `ScsiTransport` here emulates exactly that layered shape.
|
||||
//! Inline `parse_sense_tests` in `src/scsi/mod.rs` cover the pure parse
|
||||
//! logic; this file covers the consumer side — a real transport feeding
|
||||
//! a real Error variant to a real call site (`scsi::inquiry`).
|
||||
|
||||
use libfreemkv::error::Error;
|
||||
use libfreemkv::scsi::{DataDirection, ScsiResult, ScsiTransport};
|
||||
|
||||
/// A scripted ScsiTransport. Each `execute()` consumes the next entry
|
||||
/// from `script` and returns the corresponding outcome.
|
||||
///
|
||||
/// Outcomes mirror what each backend's `execute()` should produce after
|
||||
/// the v0.13.20 rewrite: pre-parsed sense_key and synthesized 0xFF
|
||||
/// status for transport-level failures.
|
||||
struct MockTransport {
|
||||
script: Vec<MockOutcome>,
|
||||
next: usize,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
enum MockOutcome {
|
||||
/// Healthy completion. `data` is what the transport wrote into the
|
||||
/// caller's data buffer (truncated to the buffer length); `resid`
|
||||
/// is reported back as `data.len() - bytes_transferred`.
|
||||
Ok {
|
||||
data: Vec<u8>,
|
||||
resid: i32,
|
||||
},
|
||||
/// Transport-level failure: e.g. `hdr.host_status = DID_TIME_OUT`
|
||||
/// on Linux, or `kIOReturnError` on macOS, or `DeviceIoControl`
|
||||
/// returning 0 on Windows. Backends synthesize 0xFF.
|
||||
TransportFailure,
|
||||
/// SCSI-level failure: device responded with a non-zero status and
|
||||
/// some sense data. `status` and `sense_key` are what the caller
|
||||
/// must see on the `Error::ScsiError` variant.
|
||||
ScsiFailure {
|
||||
status: u8,
|
||||
sense_key: u8,
|
||||
},
|
||||
}
|
||||
|
||||
impl MockTransport {
|
||||
fn new(script: Vec<MockOutcome>) -> Self {
|
||||
Self { script, next: 0 }
|
||||
}
|
||||
}
|
||||
|
||||
impl ScsiTransport for MockTransport {
|
||||
fn execute(
|
||||
&mut self,
|
||||
cdb: &[u8],
|
||||
_direction: DataDirection,
|
||||
data: &mut [u8],
|
||||
_timeout_ms: u32,
|
||||
) -> libfreemkv::error::Result<ScsiResult> {
|
||||
let i = self.next;
|
||||
self.next += 1;
|
||||
let outcome = self
|
||||
.script
|
||||
.get(i)
|
||||
.cloned()
|
||||
.expect("MockTransport script ran out — test wrote fewer outcomes than calls");
|
||||
match outcome {
|
||||
MockOutcome::Ok { data: d, resid } => {
|
||||
let n = d.len().min(data.len());
|
||||
data[..n].copy_from_slice(&d[..n]);
|
||||
let bytes = (data.len() as i32).saturating_sub(resid).max(0) as usize;
|
||||
Ok(ScsiResult {
|
||||
status: 0,
|
||||
bytes_transferred: bytes,
|
||||
sense: [0u8; 32],
|
||||
})
|
||||
}
|
||||
MockOutcome::TransportFailure => Err(Error::ScsiError {
|
||||
opcode: cdb[0],
|
||||
status: 0xFF,
|
||||
sense_key: 0,
|
||||
}),
|
||||
MockOutcome::ScsiFailure { status, sense_key } => Err(Error::ScsiError {
|
||||
opcode: cdb[0],
|
||||
status,
|
||||
sense_key,
|
||||
}),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── 1. Healthy read ───────────────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn test_healthy_inquiry_returns_ok_with_full_transfer() {
|
||||
// Build a fake INQUIRY response (96 bytes per scsi::inquiry).
|
||||
// Vendor "TEST-VND", model "TEST-MODEL ", firmware "1.00".
|
||||
let mut payload = vec![0u8; 96];
|
||||
payload[8..16].copy_from_slice(b"TEST-VND");
|
||||
payload[16..32].copy_from_slice(b"TEST-MODEL ");
|
||||
payload[32..36].copy_from_slice(b"1.00");
|
||||
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::Ok {
|
||||
data: payload,
|
||||
resid: 0,
|
||||
}]);
|
||||
|
||||
let r = libfreemkv::scsi::inquiry(&mut transport).expect("inquiry should succeed");
|
||||
assert_eq!(r.vendor_id, "TEST-VND");
|
||||
assert_eq!(r.model, "TEST-MODEL");
|
||||
assert_eq!(r.firmware, "1.00");
|
||||
}
|
||||
|
||||
// ── 2. Transport-level failure (host_status or driver_status non-zero) ─────
|
||||
//
|
||||
// Linux: kernel sets `hdr.host_status = DID_TIME_OUT (0x03)` or
|
||||
// `hdr.driver_status` non-zero on a USB bridge wedge. SgIoTransport
|
||||
// synthesizes ScsiError { status: 0xFF, sense_key: 0 }.
|
||||
// macOS: IOKit `ExecuteTaskSync` returns non-zero IOReturn; same shape.
|
||||
// Windows: `DeviceIoControl` returns 0; same shape.
|
||||
//
|
||||
// Callers (drive_has_disc, etc.) match on status == 0xFF as the wedge
|
||||
// signature. This test pins that contract.
|
||||
|
||||
#[test]
|
||||
fn test_transport_failure_surfaces_as_status_0xff_sense_key_0() {
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::TransportFailure]);
|
||||
|
||||
let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err();
|
||||
match err {
|
||||
Error::ScsiError {
|
||||
status, sense_key, ..
|
||||
} => {
|
||||
assert_eq!(status, 0xFF, "transport failure must surface as 0xFF");
|
||||
assert_eq!(sense_key, 0, "transport failure has no sense key");
|
||||
}
|
||||
other => panic!("expected ScsiError, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
// ── 3. SCSI-level failure with descriptor-format sense (0x72) ─────────────
|
||||
|
||||
#[test]
|
||||
fn test_scsi_failure_descriptor_format_illegal_request() {
|
||||
// A real device returning CHECK CONDITION (status 0x02) with
|
||||
// descriptor-format sense indicating ILLEGAL REQUEST (key 5).
|
||||
// The Linux backend's parse_sense_key reads byte 1; the caller sees
|
||||
// sense_key = 5.
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure {
|
||||
status: 0x02,
|
||||
sense_key: 5,
|
||||
}]);
|
||||
|
||||
let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err();
|
||||
match err {
|
||||
Error::ScsiError {
|
||||
opcode,
|
||||
status,
|
||||
sense_key,
|
||||
} => {
|
||||
assert_eq!(opcode, libfreemkv::scsi::SCSI_INQUIRY);
|
||||
assert_eq!(status, 0x02);
|
||||
assert_eq!(sense_key, 5);
|
||||
}
|
||||
other => panic!("expected ScsiError, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
// ── 4. SCSI-level failure with NOT READY sense ───────────────────────────
|
||||
//
|
||||
// `drive_has_disc` matches on sense_key 2 to mean "no disc inserted"
|
||||
// rather than a hard error. Pin that contract via the consumer.
|
||||
|
||||
#[test]
|
||||
fn test_scsi_failure_not_ready_key_2_propagates_intact() {
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure {
|
||||
status: 0x02,
|
||||
sense_key: 2,
|
||||
}]);
|
||||
|
||||
let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err();
|
||||
match err {
|
||||
Error::ScsiError {
|
||||
status, sense_key, ..
|
||||
} => {
|
||||
assert_eq!(status, 0x02);
|
||||
assert_eq!(sense_key, 2);
|
||||
}
|
||||
other => panic!("expected ScsiError, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
// ── 5. SCSI-level failure with empty sense ───────────────────────────────
|
||||
//
|
||||
// Backends pass `sb_len_wr` to `parse_sense_key`; when zero, the helper
|
||||
// returns 0. From the caller's perspective this is `sense_key = 0`
|
||||
// (NO SENSE) on a non-zero status — surface that contract.
|
||||
|
||||
#[test]
|
||||
fn test_scsi_failure_empty_sense_returns_key_0() {
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure {
|
||||
status: 0x02,
|
||||
sense_key: 0,
|
||||
}]);
|
||||
|
||||
let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err();
|
||||
match err {
|
||||
Error::ScsiError {
|
||||
status, sense_key, ..
|
||||
} => {
|
||||
assert_eq!(status, 0x02);
|
||||
assert_eq!(sense_key, 0);
|
||||
}
|
||||
other => panic!("expected ScsiError, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
// ── 6. Healthy short transfer (resid > 0) ────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn test_healthy_short_transfer_reports_partial_bytes() {
|
||||
// Transport reported back fewer bytes than requested (resid = 16).
|
||||
// For a 96-byte buffer that means bytes_transferred = 80.
|
||||
let payload = vec![0u8; 96];
|
||||
let mut transport = MockTransport::new(vec![MockOutcome::Ok {
|
||||
data: payload,
|
||||
resid: 16,
|
||||
}]);
|
||||
|
||||
// Drive INQUIRY through the public helper to exercise the consumer
|
||||
// path; INQUIRY itself doesn't act on bytes_transferred but the
|
||||
// transport contract is what we care about.
|
||||
let cdb = [libfreemkv::scsi::SCSI_INQUIRY, 0, 0, 0, 0x60, 0];
|
||||
let mut buf = [0u8; 96];
|
||||
let r = transport
|
||||
.execute(&cdb, DataDirection::FromDevice, &mut buf, 1_000)
|
||||
.expect("ok");
|
||||
assert_eq!(r.status, 0);
|
||||
assert_eq!(
|
||||
r.bytes_transferred, 80,
|
||||
"bytes_transferred must equal data.len() - resid"
|
||||
);
|
||||
}
|
||||
|
||||
// ── 7. Error::Display does not leak English in the SCSI variant ───────────
|
||||
//
|
||||
// Library rule (CLAUDE.md): no English in error display, only "E{code}"
|
||||
// + structured data. Existing error-mod tests cover the new variants;
|
||||
// this is a regression guard for the SCSI variant specifically because
|
||||
// it's the most-emitted error in the rip path.
|
||||
|
||||
#[test]
|
||||
fn test_scsi_error_display_format_is_codes_only() {
|
||||
let err = Error::ScsiError {
|
||||
opcode: 0x12,
|
||||
status: 0x02,
|
||||
sense_key: 5,
|
||||
};
|
||||
let s = err.to_string();
|
||||
assert!(s.starts_with("E4000:"), "ScsiError must lead with E4000: {s}");
|
||||
assert!(
|
||||
s.contains("0x12") && s.contains("0x02") && s.contains("0x05"),
|
||||
"ScsiError must show opcode/status/sense_key in hex: {s}"
|
||||
);
|
||||
// Crude English filter — same as the inline error.rs::display test.
|
||||
for word in s.split(|c: char| !c.is_ascii_alphabetic()) {
|
||||
assert!(
|
||||
word.len() <= 4,
|
||||
"ScsiError display contains suspicious word `{word}`: {s}"
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user