diff --git a/CHANGELOG.md b/CHANGELOG.md index 212b4ec..58c4c72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,56 @@ # Changelog +## 0.13.21 (2026-04-26) + +### Fix: Disc::copy bisect-on-fail (replaces skip-forward) + +Empirical live-hardware testing on the LG BU40N (see +`freemkv-private/docs/audits/2026-04-26-test-plan-audit.md` and the +TEST_PLAN.md run log) revealed that the drive often **fails +multi-sector READ commands** in damaged regions but **succeeds when +asked one sector at a time**. The old skip-forward strategy responded +to multi-sector failures by jumping up to 1 % of the disc forward, +marking everything in between as bad — losing **clean territory** +sandwiched between bad sectors. + +`Disc::copy` now bisects on read failure: split the failed block in +half, retry each half, recurse down to single-sector reads. Sectors +the drive can read individually are recovered in Pass 1; only sectors +that fail at bpt=1 are marked NonTrimmed for the patch passes. + +Empirical results on Dune 2 UHD on the BU40N: +- Old algorithm: 25 GB read in Pass 1, then ~6 GB skip-forwarded; + retry passes failed to recover most of the skipped zone. +- New algorithm: ~99 % of disc recovered in Pass 1; only the truly + unreadable cluster (~14 % of a 2 MB hot zone) marked NonTrimmed. + +Implementation: stack-based DFS in the inner read loop. log₂(batch) +levels max — for the default 60-sector batch, 6 levels. Multi-pass +machinery is untouched: Pass 2 .. N walk the mapfile and become fast +no-ops when bisect already recovered everything. New integration test +`test_disc_copy_bisect_recovers_via_single_sector_reads` validates +the behavior against a synthetic BU40N-pattern reader. + +### Fix: READ_TIMEOUT_MS bumped 1.5 s → 10 s (caller-side) + +The 0.13.20 SCSI rewrite gave the kernel mid-layer the ability to run +its own ABORT/RESET escalation. But callers (`Drive::read` for the +fast path) still passed `timeout_ms=1500`. Cold-start seek on the +BU40N can take ~1.5 s, which means **normal reads were being +cancelled at the boundary**, triggering the kernel mid-layer's +escalation, which the Initio bridge couldn't drain — resulting in the +firmware-level wedge that only physical replug recovers. + +Live-hardware probe data: +- Sustained sequential read: 3–7 ms +- Cold-start seek + read: up to ~1500 ms +- Successful ECC recovery: 1.6–2.6 s +- Confirmed unreadable: 3.6–8.8 s (kernel timeout) + +10 s is calibrated to cover every legitimate read with margin while +still short-circuiting truly bad sectors before the kernel runs full +LUN/BUS/HOST reset. `READ_RECOVERY_TIMEOUT_MS` (60 s) unchanged. + ## 0.13.20 (2026-04-26) ### Architecture: SCSI transport — sync blocking SG_IO diff --git a/Cargo.toml b/Cargo.toml index 285d1c1..c1a6a31 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "0.13.20" +version = "0.13.21" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 66b69ea..45f329a 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1278,11 +1278,6 @@ impl Disc { None => DEFAULT_BATCH_SECTORS, }; - // Skip-forward state. - let skip_init = 256 * 1024u64; // 256 KB - let skip_max = (total_bytes / 100).max(skip_init); // cap at 1% of disc - let mut skip_size = skip_init; - let mut buf = vec![0u8; batch as usize * 2048]; let mut bytes_done = 0u64; let mut halt_requested = false; @@ -1296,8 +1291,6 @@ impl Disc { phase = "copy_start", total_bytes, batch, - skip_init, - skip_max, "Disc::copy entered" ); @@ -1344,58 +1337,88 @@ impl Disc { } } let block_bytes = (region_end - pos).min(batch as u64 * 2048); - let lba = (pos / 2048) as u32; - let count = (block_bytes / 2048) as u16; - let bytes = count as usize * 2048; + let block_lba = (pos / 2048) as u32; + let block_count = (block_bytes / 2048) as u16; + let recovery = !opts.skip_on_error; - let recovery = !opts.skip_on_error; // fast reads when skipping - iter_count += 1; - let read_t0 = std::time::Instant::now(); - let read_ok = reader - .read_sectors(lba, count, &mut buf[..bytes], recovery) - .is_ok(); - let read_elapsed_ms = read_t0.elapsed().as_millis() as u64; - - if read_ok { - read_ok_count += 1; - if opts.decrypt { - crate::decrypt::decrypt_sectors(&mut buf[..bytes], &keys, 0)?; - } - file.seek(SeekFrom::Start(pos)) - .map_err(|e| Error::IoError { source: e })?; - file.write_all(&buf[..bytes]) - .map_err(|e| Error::IoError { source: e })?; - map.record(pos, block_bytes, mapfile::SectorStatus::Finished) - .map_err(|e| Error::IoError { source: e })?; - bytes_done = bytes_done.saturating_add(block_bytes); - skip_size = skip_init; // reset after success - pos += block_bytes; - } else if opts.skip_on_error { - read_err_count += 1; - // Zero-fill this block, mark non-trimmed for later patch trim. - buf[..bytes].fill(0); - file.seek(SeekFrom::Start(pos)) - .map_err(|e| Error::IoError { source: e })?; - file.write_all(&buf[..bytes]) - .map_err(|e| Error::IoError { source: e })?; - map.record(pos, block_bytes, mapfile::SectorStatus::NonTrimmed) - .map_err(|e| Error::IoError { source: e })?; - pos += block_bytes; - - if opts.skip_forward && pos < region_end { - // Skip ahead; mark skipped bytes as non-trimmed too. - let jump = skip_size.min(region_end - pos); - if jump > 0 { - map.record(pos, jump, mapfile::SectorStatus::NonTrimmed) - .map_err(|e| Error::IoError { source: e })?; - pos += jump; + // Bisect-on-fail (0.13.21): try the full block first; on read + // failure, recursively split into halves down to single-sector + // reads. Recovers data the drive can read individually but + // fails as a multi-sector block — empirically the BU40N's + // bad-zone pattern (see freemkv-private/docs/TEST_PLAN.md). + // + // Pre-0.13.21 we skip-forwarded by an exponentially-growing + // jump (capped at 1% of disc), which marked vast tracts of + // *clean* territory as bad just because it sat past one bad + // block. With bisection we descend only into the ~14% of + // bisection leaves that are truly unreadable; the other ~86% + // recover at smaller block sizes within the same pass. + let mut work: Vec<(u32, u16)> = vec![(block_lba, block_count)]; + while let Some((sub_lba, sub_count)) = work.pop() { + if let Some(ref h) = opts.halt { + if h.load(std::sync::atomic::Ordering::Relaxed) { + halt_requested = true; + break 'outer; } - skip_size = (skip_size * 2).min(skip_max); } - } else { - // Current behavior (pre-0.11.21): abort on first bad sector. - return Err(Error::DiscRead { sector: lba as u64 }); + iter_count += 1; + let sub_bytes = sub_count as usize * 2048; + let sub_pos = sub_lba as u64 * 2048; + let read_t0 = std::time::Instant::now(); + let read_ok = reader + .read_sectors(sub_lba, sub_count, &mut buf[..sub_bytes], recovery) + .is_ok(); + let read_elapsed_ms = read_t0.elapsed().as_millis() as u64; + + if read_ok { + read_ok_count += 1; + if opts.decrypt { + crate::decrypt::decrypt_sectors(&mut buf[..sub_bytes], &keys, 0)?; + } + file.seek(SeekFrom::Start(sub_pos)) + .map_err(|e| Error::IoError { source: e })?; + file.write_all(&buf[..sub_bytes]) + .map_err(|e| Error::IoError { source: e })?; + map.record(sub_pos, sub_bytes as u64, mapfile::SectorStatus::Finished) + .map_err(|e| Error::IoError { source: e })?; + bytes_done = bytes_done.saturating_add(sub_bytes as u64); + } else if opts.skip_on_error && sub_count > 1 { + // Bisect: split this sub-block in half. LIFO push + // (second half first) so the first half is processed + // next — keeps reads roughly in-order, helping the + // drive's read-ahead cache. + let half = sub_count / 2; + work.push((sub_lba + half as u32, sub_count - half)); + work.push((sub_lba, half)); + tracing::trace!( + target: "freemkv::disc", + phase = "bisect", + sub_lba, + sub_count, + half, + read_elapsed_ms, + "bisecting failed block" + ); + } else if opts.skip_on_error { + // Single-sector failure — truly unreadable. + // Zero-fill, mark NonTrimmed for the patch passes. + read_err_count += 1; + buf[..sub_bytes].fill(0); + file.seek(SeekFrom::Start(sub_pos)) + .map_err(|e| Error::IoError { source: e })?; + file.write_all(&buf[..sub_bytes]) + .map_err(|e| Error::IoError { source: e })?; + map.record(sub_pos, sub_bytes as u64, mapfile::SectorStatus::NonTrimmed) + .map_err(|e| Error::IoError { source: e })?; + } else { + // skip_on_error=false: abort on first bad sector + // (Disc::copy's strict mode, used by some callers). + return Err(Error::DiscRead { + sector: sub_lba as u64, + }); + } } + pos += block_bytes; // Throttled iter telemetry — every 100 inner iterations. if iter_count - last_log_iter >= 100 { @@ -1407,10 +1430,8 @@ impl Disc { iter_count, read_ok_count, read_err_count, - last_read_ms = read_elapsed_ms, pos, region_end, - skip_size, bytes_good = stats.bytes_good, bytes_pending = stats.bytes_pending, copy_elapsed_ms = copy_t0.elapsed().as_millis() as u64, diff --git a/src/drive/mod.rs b/src/drive/mod.rs index 0d03858..577c1bf 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -417,12 +417,16 @@ impl Drive { /// Read sectors from the disc. Single-shot — no inline retries, no /// SCSI reset. /// - /// `recovery=true` bumps the per-CDB timeout to 30 s for the - /// `Disc::patch` pass; `recovery=false` uses 1.5 s for `Disc::copy`'s - /// fast skip-forward sweep. On any failure returns `Err(DiscRead)` - /// immediately. The orchestration layer (`Disc::patch`'s outer loop - /// for the patch pass, `DiscStream`'s adaptive batch halving for the - /// stream path) handles retries. + /// `recovery=true` uses [`crate::scsi::READ_RECOVERY_TIMEOUT_MS`] (60 s, + /// matches sg_dd) for the `Disc::patch` pass; `recovery=false` uses + /// [`crate::scsi::READ_TIMEOUT_MS`] (30 s, matches the kernel's + /// `/sys/block/sr*/device/timeout` default) for `Disc::copy`'s fast + /// skip-forward sweep. Both budgets are generous enough that the drive + /// can finish ECC recovery on a marginal sector — pre-0.13.21 this was + /// 1.5 s on the fast path which forced the kernel mid-layer to time + /// out and escalate while we waited anyway. On any failure returns + /// `Err(DiscRead)` immediately; orchestration (`Disc::patch` multi-pass, + /// `DiscStream` adaptive batch halving) handles retry policy. /// /// Inline retry phases (5× gentle + reset+reopen + 5× more) were /// removed in 0.13.6. Per @@ -432,7 +436,11 @@ impl Drive { /// layers (Disc::patch multi-pass, DiscStream batch halving) do not /// touch the wedge-prone reset path. pub fn read(&mut self, lba: u32, count: u16, buf: &mut [u8], recovery: bool) -> Result { - let timeout_ms = if recovery { 30_000 } else { 1_500 }; + let timeout_ms = if recovery { + crate::scsi::READ_RECOVERY_TIMEOUT_MS + } else { + crate::scsi::READ_TIMEOUT_MS + }; let cdb = [ crate::scsi::SCSI_READ_10, 0x00, diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 7ac30c9..6e1b8a6 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -43,6 +43,38 @@ pub const AACS_KEY_CLASS: u8 = 0x02; /// a poll-loop tick. pub(crate) const TUR_TIMEOUT_MS: u32 = 5_000; +/// Timeout for content READ commands (READ_10 / READ_12) on the fast +/// path — the [`disc::Disc::copy`] sweep that bisects-on-failure. +/// +/// 10 s is calibrated from live empirical data on an LG BU40N + Initio +/// 1618L bridge ripping a UHD with marginal sectors: +/// +/// - Sustained sequential reads: 3 – 7 ms +/// - Cold-start seek + read: up to ~1500 ms +/// - Successful ECC recovery: 1.6 – 2.6 sec +/// - Confirmed unreadable sector: 3.6 – 8.8 sec (kernel timeout) +/// +/// 10 s catches every legitimate slow read with comfortable margin and +/// short-circuits truly bad sectors at ~10 s rather than letting the +/// kernel mid-layer escalate for 30 s+. See run log in +/// `freemkv-private/docs/TEST_PLAN.md` and the audit at +/// `freemkv-private/docs/audits/2026-04-26-scsi-architecture-research.md`. +/// +/// Pre-0.13.21 this was 1.5 s, which forced the kernel mid-layer to +/// time out *normal* reads (cold-start often takes ~1.5 s) and run its +/// full ABORT TASK / LUN RESET / BUS RESET escalation while userspace +/// kept submitting fresh reads. The Initio bridge couldn't drain the +/// resulting command queue and entered a wedge state that only physical +/// replug recovered — proven by the v0.13.18 + v0.13.20 live tests. +pub(crate) const READ_TIMEOUT_MS: u32 = 10_000; + +/// Timeout for content READ commands on the recovery path — +/// [`disc::Disc::patch`]'s targeted retries on bad ranges. Doubles +/// the fast-path budget so a sector that fails at 30 s gets one more +/// honest attempt. Matches sg_dd's default per-command timeout +/// (`DEF_TIMEOUT = 60000`). +pub(crate) const READ_RECOVERY_TIMEOUT_MS: u32 = 60_000; + // ── Sense-key parsing ─────────────────────────────────────────────────────── /// Extract the SPC-4 sense key from a sense buffer. diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index 1729628..a7a4fa6 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -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 { + 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" + ); +} diff --git a/tests/scsi_error_decoding.rs b/tests/scsi_error_decoding.rs new file mode 100644 index 0000000..e7599eb --- /dev/null +++ b/tests/scsi_error_decoding.rs @@ -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, + 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, + 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) -> 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 { + 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}" + ); + } +}