diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b89a4d..bb56c82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,87 @@ # Changelog +## 0.13.12 (2026-04-25) + +### Fix: delete stall guard from `Disc::copy` (RIP_DESIGN.md §6 Fix 1) + +The v0.13.9 stall guard at `disc/mod.rs` exited Pass 1 early when +`bytes_good` was flat for `stall_secs` (default 120s). This violated the +ddrescue model: Pass 1 must sweep end-to-end, marking failed reads +NonTrimmed for Pass 2 retry. The guard caused Pass 1 to bail at 30% on +Dune 2 with 56 GB still NonTried, leaving Pass 2 nothing useful to do. + +- Deleted the stall-guard state vars and the `if cur_good != ... + break 'outer;` block. +- Deleted `CopyOptions::stall_secs` field — no longer wired. +- Replaced the broken regression test + `test_disc_copy_stall_detection_triggers_skip_forward` with + `test_disc_copy_completes_full_disc_with_failing_reader` (asserts Pass 1 + walks to end-of-disc with everything NonTrimmed when reads keep failing) + and added `test_disc_copy_halts_promptly_on_failing_reader` (halt flag + honored within 2s mid-skip-forward). + +### Fix: async SCSI transport recovery (RIP_DESIGN.md §6 Fix 2 / §7) + +`SgIoTransport::execute` (Linux) previously did close-in-background + +synchronous open-on-main-thread on poll timeout. The kernel serialized +the main-thread `open()` against the in-flight `close()` of the same +`/dev/sg*`, blocking the rip thread up to ~60s per timeout. + +- Added `fd_recovery: Arc` field. On poll timeout, both + `close(old_fd)` AND `open(new_fd)` run in a background thread; the new + fd is published to `fd_recovery`. Returns Err immediately. Main thread + is never blocked beyond the `poll()` budget (~1.5 s). +- Top of `execute()`: if `self.fd < 0`, swap from `fd_recovery`. If + recovery is also pending, return `DeviceNotFound` and let the caller's + retry loop come back later. +- Drop drains any pending `fd_recovery` so the fd doesn't leak. +- Stripped the v0.13.9 stall-guard narrative comment that justified + the deleted behavior. + +### Fix: cross-platform SCSI parity — Windows + macOS recovery (RIP_DESIGN.md §15.1) + +Per the platform parity rule (no stubs), Windows and macOS now have the +same observable recovery contract as Linux: + +- `SptiTransport` (Windows): added `try_recover()` that calls + `CloseHandle` + `CreateFileW` synchronously after a failed + `DeviceIoControl`. Stripped English error string ("run as + administrator") from `open()`. Fixed the ms→s timeout truncation + (1500ms now rounds up to 2s, was 1s). +- `MacScsiTransport` (macOS): added `try_recover()` that releases the + IOKit interface (`RELEASE_EXCLUSIVE` + `com_release`) and re-acquires + via the new `acquire_device_iface()` helper. Stores `bsd_name` so + recovery can re-call `find_scsi_service`. +- All three platforms: top of `execute()` returns `DeviceNotFound` + immediately if a prior `try_recover()` left the transport in an + invalid state. Drop guards null'd-out interfaces. +- Send is auto-derived on all three (i32 fd / isize HANDLE / IOKit + interface ref are Send-safe); explicit comments document the + intentional implicit Send and the absence of Sync. + +### Fix: instrument `Disc::patch` — diagnostic counters (RIP_DESIGN.md §6 Fix 4) + +`PatchResult` now reports `blocks_attempted`, `blocks_read_ok`, +`blocks_read_failed`. Pass 2's "100 minutes recovered 0 bytes" mystery +(Dune 2) becomes diagnosable from these counters: distinguish "drive +returned Ok but write/record dropped data" from "every read was Err for +the entire range" without instrumenting from outside the lib. + +### Fix: honor `PatchOptions::full_recovery` + +The field was previously read into `let _ = opts.full_recovery;` and +ignored — `read_sectors(..., true)` was hardcoded. Now routed to +`read_sectors(..., opts.full_recovery)`. Behavior unchanged for +default callers (which pass `true`). + +### Doc: `CopyOptions::batch_sectors` accuracy + +Doc comment said "Defaults to 32 sectors (64 KB)". Updated to describe +the actual production path: callers should resolve via +`detect_max_batch_sectors(device_path)` (kernel-reported sysfs value, +typically 60 sectors / ~120 KB on the BU40N). The 32-sector internal +fallback is only reached when `batch_sectors=None AND skip_forward=true`. + ## 0.13.11 (2026-04-25) ### Fix: revert SgIoTransport timeout path to keep transport alive diff --git a/Cargo.toml b/Cargo.toml index 6911cb5..9732e3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "0.13.11" +version = "0.13.12" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 3911694..5e85f31 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1286,9 +1286,6 @@ impl Disc { let mut buf = vec![0u8; batch as usize * 2048]; let mut bytes_done = 0u64; let mut halt_requested = false; - let stall_threshold = std::time::Duration::from_secs(opts.stall_secs.unwrap_or(120)); - let mut last_good_advance = std::time::Instant::now(); - let mut last_observed_good: u64 = 0; // Iterate over not-yet-finished regions from the mapfile. We re-read the // mapfile after each block because record() mutates the region list. @@ -1318,30 +1315,6 @@ impl Disc { break 'outer; } } - // Stall guard. The signal is "bytes_good (Finished sectors) - // hasn't advanced for stall_threshold." pos may still be - // advancing via the skip_on_error branch; that's not real - // progress because skip-forward only marks ranges - // NonTrimmed for Pass 2 to retry. If we go stall_threshold - // without ANY successful read, the drive is grinding - // unproductively (or kernel is silently stalling reads - // past their per-CDB timeout — observed live on Dell with - // SgIoTransport's reopen-after-timeout serializing - // against close). Bail Pass 1; Pass 2 (Disc::patch with - // recovery=true, 30 s timeouts) will retry the - // NonTrimmed ranges. - let cur_good = map.stats().bytes_good; - if cur_good != last_observed_good { - last_observed_good = cur_good; - last_good_advance = std::time::Instant::now(); - } else if last_good_advance.elapsed() > stall_threshold { - // Stall: bail Pass 1. Return cleanly with - // bytes_pending > 0 and complete = false so the - // caller's retry path (Disc::patch with - // recovery=true, 30s timeouts) gets a shot at the - // NonTrimmed ranges. - break 'outer; - } let block_bytes = (region_end - pos).min(batch as u64 * 2048); let lba = (pos / 2048) as u32; let count = (block_bytes / 2048) as u16; @@ -1419,8 +1392,10 @@ pub struct CopyOptions<'a> { /// Resume from existing mapfile + ISO if present. Without this, any /// existing mapfile is wiped and the ISO recreated. pub resume: bool, - /// Override the default block size. Defaults to 32 sectors (64 KB) in - /// `skip_forward` mode, `DEFAULT_BATCH_SECTORS` otherwise. + /// Override the default block size in sectors. Callers should resolve + /// this with `detect_max_batch_sectors(device_path)` for live drives. + /// When `None`, falls back to 32 sectors (64 KB BD ECC block) in + /// `skip_forward` mode or `DEFAULT_BATCH_SECTORS=60` otherwise. pub batch_sectors: Option, /// Zero-fill bad blocks in the ISO, mark them in the mapfile, continue. /// Uses fast reads (no drive-level recovery loop). @@ -1431,15 +1406,6 @@ pub struct CopyOptions<'a> { pub skip_forward: bool, pub on_progress: Option<&'a dyn Fn(u64, u64)>, pub halt: Option>, - /// Wall-clock stall threshold in seconds. If the inner copy loop runs - /// this long without `pos` advancing, the current block is treated as - /// a read failure (skip-forward in `skip_on_error` mode, else - /// `Err(DiscRead)`). Defaults to 120 s. Defensive guard for - /// kernel-level hangs that bypass the per-CDB SCSI timeout — e.g. the - /// v0.13.8 case where SgIoTransport's reopen-after-timeout serialized - /// against the in-flight close, blocking the main thread for tens of - /// seconds per read. - pub stall_secs: Option, } /// Result of `Disc::copy`. `complete=true` means every byte reached a terminal @@ -1476,7 +1442,8 @@ pub struct PatchOptions<'a> { pub halt: Option>, } -/// Result of `Disc::patch` — how many bad bytes were recovered. +/// Result of `Disc::patch` — how many bad bytes were recovered, plus +/// per-block counters for diagnosing why a pass made or didn't make progress. #[derive(Debug, Clone, Copy)] pub struct PatchResult { pub bytes_total: u64, @@ -1485,6 +1452,12 @@ pub struct PatchResult { pub bytes_pending: u64, pub bytes_recovered_this_pass: u64, pub halted: bool, + /// Total inner-loop iterations this pass (one per block attempted). + pub blocks_attempted: u64, + /// Reads that returned Ok and were promoted to `Finished`. + pub blocks_read_ok: u64, + /// Reads that returned Err and were marked `Unreadable`. + pub blocks_read_failed: u64, } impl Disc { @@ -1518,13 +1491,13 @@ impl Disc { .map_err(|e| Error::IoError { source: e })?; let block_sectors = opts.block_sectors.unwrap_or(1); - // Patch always reads with full drive recovery — this is the pass where - // we want the drive's ECC retry machinery. Consumers who want fast-fail - // use Disc::copy with skip_on_error instead. - let _ = opts.full_recovery; + let recovery = opts.full_recovery; let bytes_good_before = map.stats().bytes_good; let mut halted = false; + let mut blocks_attempted: u64 = 0; + let mut blocks_read_ok: u64 = 0; + let mut blocks_read_failed: u64 = 0; let mut buf = vec![0u8; block_sectors as usize * 2048]; // Collect bad ranges up front. Iterating while mutating is fragile; @@ -1551,10 +1524,12 @@ impl Disc { let lba = (pos / 2048) as u32; let count = (block_bytes / 2048) as u16; let bytes = count as usize * 2048; + blocks_attempted += 1; let read_ok = reader - .read_sectors(lba, count, &mut buf[..bytes], true) + .read_sectors(lba, count, &mut buf[..bytes], recovery) .is_ok(); if read_ok { + blocks_read_ok += 1; if opts.decrypt { crate::decrypt::decrypt_sectors(&mut buf[..bytes], &keys, 0)?; } @@ -1565,6 +1540,7 @@ impl Disc { map.record(pos, block_bytes, mapfile::SectorStatus::Finished) .map_err(|e| Error::IoError { source: e })?; } else { + blocks_read_failed += 1; map.record(pos, block_bytes, mapfile::SectorStatus::Unreadable) .map_err(|e| Error::IoError { source: e })?; } @@ -1586,6 +1562,9 @@ impl Disc { bytes_pending: stats.bytes_pending, bytes_recovered_this_pass: stats.bytes_good.saturating_sub(bytes_good_before), halted, + blocks_attempted, + blocks_read_ok, + blocks_read_failed, }) } } diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index 32adab5..77f0967 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -54,8 +54,18 @@ const _: () = assert!(std::mem::size_of::() == 64); pub struct SgIoTransport { fd: i32, device_path: std::path::PathBuf, + /// Background-recovered fd. After a poll timeout `execute()` spawns a + /// thread that closes `self.fd` and opens a fresh fd; the new fd is + /// stored here. The next call to `execute()` swaps it into `self.fd`. + /// `-1` means no recovery is ready (or the recovery open failed). See + /// RIP_DESIGN.md §7 for the design rationale. + fd_recovery: std::sync::Arc, } +// SgIoTransport's contained types (i32, PathBuf, Arc) are all +// Send; the auto-derived Send is intentional. Sync is NOT — callers must +// hold &mut for execute(), which the trait object dispatch enforces. + impl SgIoTransport { /// Open a SCSI device for use. Resets the drive first to ensure /// a known good state, then opens a fresh fd for commands. @@ -75,6 +85,7 @@ impl SgIoTransport { Ok(SgIoTransport { fd, device_path: device, + fd_recovery: std::sync::Arc::new(std::sync::atomic::AtomicI32::new(-1)), }) } @@ -140,10 +151,7 @@ impl SgIoTransport { let err = std::io::Error::last_os_error(); Err(if err.kind() == std::io::ErrorKind::PermissionDenied { Error::DevicePermission { - path: format!( - "{}: permission denied (try running as root)", - device.display() - ), + path: device.display().to_string(), } } else { Error::DeviceNotFound { @@ -219,6 +227,13 @@ impl Drop for SgIoTransport { let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000); unsafe { libc::close(self.fd) }; } + // Drain any background-recovered fd so it doesn't leak. + let recovered = self + .fd_recovery + .swap(-1, std::sync::atomic::Ordering::Acquire); + if recovered >= 0 { + unsafe { libc::close(recovered) }; + } } } @@ -245,10 +260,21 @@ impl ScsiTransport for SgIoTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { + // Recover from a prior timeout: if a background reopen produced a + // fresh fd, swap it in. If recovery is still pending (-1), the + // background thread hasn't finished — return DeviceNotFound and let + // the caller's retry loop come back later. if self.fd < 0 { - return Err(Error::DeviceNotFound { - path: self.device_path.display().to_string(), - }); + let recovered = self + .fd_recovery + .swap(-1, std::sync::atomic::Ordering::Acquire); + if recovered >= 0 { + self.fd = recovered; + } else { + return Err(Error::DeviceNotFound { + path: self.device_path.display().to_string(), + }); + } } let mut sense = [0u8; 32]; @@ -320,43 +346,37 @@ impl ScsiTransport for SgIoTransport { }; if pr <= 0 { - // Timeout (0) or fatal poll error (-1). - // Command is still pending in the kernel. Spawn a background - // close of the old fd (which blocks until the kernel - // completes/aborts the pending command) and open a fresh fd - // on the main thread. The main-thread open() can serialize - // against the in-flight close via the kernel's per-device - // state lock — so this call may block up to ~60 s while - // the kernel finishes the abandoned command. That's the - // cost of keeping the Drive alive across a timeout. The - // Disc::copy stall guard (v0.13.9, default 120 s of - // bytes_good non-advance) is the upper bound that prevents - // a catastrophic grind on a wedged read region. - // - // History: - // - 0.13.5 and earlier: same as this — but with no upper - // bound, hence 45-min hangs. - // - 0.13.10: tried "set fd=-1, no reopen" — too aggressive, - // one transient timeout killed the whole transport, Pass - // 1 finished in 45 ms with everything NonTrimmed. - // - 0.13.11 (this): same close+reopen as 0.13.5/8 BUT with - // the v0.13.9 stall guard ensuring Disc::copy bails out - // cleanly within 120 s of zero forward progress. + // Timeout (0) or fatal poll error (-1). Command is still pending + // in the kernel. Per RIP_DESIGN.md §4(b)/§7: close + reopen run + // in a background thread so the main thread is never blocked + // beyond the poll() budget. The recovered fd is published to + // `fd_recovery`; the next call to execute() picks it up. let old_fd = self.fd; self.fd = -1; - - std::thread::spawn(move || { - unsafe { libc::close(old_fd) }; - }); - let c_path = Self::to_c_path(&self.device_path); - let new_fd = unsafe { - libc::open( - c_path.as_ptr() as *const libc::c_char, - libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC, - ) - }; - self.fd = if new_fd >= 0 { new_fd } else { -1 }; + let recovery = self.fd_recovery.clone(); + std::thread::spawn(move || { + // Close blocks until the kernel finishes/aborts the + // abandoned command. Then we open a fresh fd. Both happen + // off the main thread. + unsafe { libc::close(old_fd) }; + let new_fd = unsafe { + libc::open( + c_path.as_ptr() as *const libc::c_char, + libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC, + ) + }; + if new_fd >= 0 { + let prev = recovery.swap(new_fd, std::sync::atomic::Ordering::Release); + if prev >= 0 { + // Stale recovery fd from a prior unclaimed attempt; + // close it so it doesn't leak. + unsafe { libc::close(prev) }; + } + } else { + recovery.store(-1, std::sync::atomic::Ordering::Release); + } + }); return Err(Error::ScsiError { opcode: cdb[0], diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs index d8d8180..8f2818d 100644 --- a/src/scsi/macos.rs +++ b/src/scsi/macos.rs @@ -175,6 +175,9 @@ const VTIDX_EXECUTE_SYNC: usize = 15; pub struct MacScsiTransport { device_iface: ComRef, exclusive: bool, + /// BSD name (e.g. "disk2") retained for `try_recover()` after a + /// task-level failure. Without it we can't re-call `find_scsi_service`. + bsd_name: String, } // IOKit COM interface pointers are Mach port references — safe to send between threads. @@ -195,6 +198,19 @@ impl MacScsiTransport { dev_str }; + let device_iface = Self::acquire_device_iface(bsd_name)?; + + Ok(MacScsiTransport { + device_iface, + exclusive: true, + bsd_name: bsd_name.to_string(), + }) + } + + /// Resolve the BSD name → IOKit SCSITaskDeviceInterface with exclusive + /// access. Shared between `open()` and `try_recover()`. Returns the + /// COM ref the caller must release. + fn acquire_device_iface(bsd_name: &str) -> Result { let service = find_scsi_service(bsd_name)?; // Create IOKit plugin for the MMC device @@ -213,7 +229,7 @@ impl MacScsiTransport { if kr != K_IO_RETURN_SUCCESS || plugin.is_null() { return Err(Error::IoKitPluginFailed { - path: dev_str.to_string(), + path: bsd_name.to_string(), kr: kr as u32, }); } @@ -233,7 +249,7 @@ impl MacScsiTransport { if hr != 0 || device_iface.is_null() { return Err(Error::ScsiInterfaceUnavailable { - path: dev_str.to_string(), + path: bsd_name.to_string(), }); } @@ -245,19 +261,48 @@ impl MacScsiTransport { }; if kr != K_IO_RETURN_SUCCESS { com_release(device_iface); - // No "Try: diskutil unmountDisk" hint — that's the CLI's job. - // The typed variant carries device path + IOReturn so the - // caller can render the right message in the right language. return Err(Error::DeviceLocked { - path: dev_str.to_string(), + path: bsd_name.to_string(), kr: kr as u32, }); } - Ok(MacScsiTransport { - device_iface, - exclusive: true, - }) + Ok(device_iface) + } + + /// Recover the IOKit interface after a task-level failure. Releases + /// the current device_iface and re-acquires fresh state via + /// `acquire_device_iface`. Same observable contract as the Linux fd + /// recovery: after `try_recover()`, the next `execute()` either uses a + /// fresh interface or returns `DeviceNotFound` if recovery failed. + /// + /// Synchronous because IOKit `RELEASE_EXCLUSIVE` + `com_release` don't + /// block on in-flight CDBs the way Linux SG_IO `close` does. + fn try_recover(&mut self) { + if !self.device_iface.is_null() { + if self.exclusive { + unsafe { + type Fn = unsafe extern "C" fn(ComRef) -> IOReturn; + let f: Fn = vtable_fn(self.device_iface, VTIDX_RELEASE_EXCLUSIVE); + f(self.device_iface); + } + self.exclusive = false; + } + com_release(self.device_iface); + self.device_iface = std::ptr::null_mut(); + } + match Self::acquire_device_iface(&self.bsd_name) { + Ok(new_iface) => { + self.device_iface = new_iface; + self.exclusive = true; + } + Err(_) => { + // Leave device_iface null; next execute() returns + // DeviceNotFound. Caller's retry path will reopen Drive. + self.device_iface = std::ptr::null_mut(); + self.exclusive = false; + } + } } // `reset()` removed in 0.13.6 — see scsi/mod.rs for rationale. @@ -337,6 +382,9 @@ const K_SENSE_KEY_NOT_READY: u8 = 2; impl Drop for MacScsiTransport { fn drop(&mut self) { + if self.device_iface.is_null() { + return; + } if self.exclusive { unsafe { type Fn = unsafe extern "C" fn(ComRef) -> IOReturn; @@ -356,6 +404,15 @@ impl ScsiTransport for MacScsiTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { + // Per RIP_DESIGN.md §15.1: parity with Linux/Windows recovery + // contract. If a prior execute() invalidated the interface and + // try_recover() also failed, fail fast. + if self.device_iface.is_null() { + return Err(Error::DeviceNotFound { + path: self.bsd_name.clone(), + }); + } + // Create a SCSI task let task: ComRef = unsafe { type Fn = unsafe extern "C" fn(ComRef) -> ComRef; @@ -363,6 +420,7 @@ impl ScsiTransport for MacScsiTransport { f(self.device_iface) }; if task.is_null() { + self.try_recover(); return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, @@ -433,6 +491,9 @@ impl ScsiTransport for MacScsiTransport { com_release(task); if kr != K_IO_RETURN_SUCCESS { + // Task-level failure (timeout / IOKit error). Recover the + // interface so the caller's retry path can resume. + self.try_recover(); return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 3b6994e..d7f27b9 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -85,8 +85,16 @@ unsafe extern "system" { pub struct SptiTransport { handle: isize, + /// Wide-encoded device path used by `try_recover()` to reopen the + /// handle after a failed DeviceIoControl. Saved from `open()` so we + /// don't have to re-resolve the device path on recovery. + wide_path: Vec, } +// SptiTransport contains an isize HANDLE and a Vec; both Send. The +// auto-derived Send is intentional. Sync is NOT — handle mutation in +// execute() requires &mut, enforced by the trait object dispatch. + /// Normalize a device path to Windows \\.\X: format. /// /// NOTE: A near-identical `normalize_path` exists in `drive::windows`. @@ -129,12 +137,48 @@ impl SptiTransport { }; if handle == INVALID_HANDLE_VALUE { - return Err(Error::DeviceNotFound { - path: format!("{}: cannot open device (run as administrator)", dev_str), + // Map last-os-error → Error variant; don't embed English hints + // in the path field (the CLI handles localization). + let err = std::io::Error::last_os_error(); + return Err(if err.kind() == std::io::ErrorKind::PermissionDenied { + Error::DevicePermission { + path: dev_str.to_string(), + } + } else { + Error::DeviceNotFound { + path: dev_str.to_string(), + } }); } - Ok(SptiTransport { handle }) + Ok(SptiTransport { + handle, + wide_path: wide, + }) + } + + /// Recover the handle after a failed DeviceIoControl. Closes the bad + /// handle and opens a fresh one synchronously (CloseHandle/CreateFileW + /// are fast on Windows — no in-flight CDB to drain like Linux SG_IO). + /// On success, `self.handle` is replaced and the next `execute()` call + /// uses the new handle. On failure, `self.handle` is set to + /// INVALID_HANDLE_VALUE and subsequent calls return `DeviceNotFound`. + fn try_recover(&mut self) { + if self.handle != INVALID_HANDLE_VALUE { + unsafe { CloseHandle(self.handle) }; + } + let new_handle = unsafe { + CreateFileW( + self.wide_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + std::ptr::null(), + OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, + std::ptr::null(), + ) + }; + self.handle = new_handle; } /// Reset the drive to a known good state. @@ -222,8 +266,10 @@ pub(super) fn drive_has_disc(path: &Path) -> Result { impl Drop for SptiTransport { fn drop(&mut self) { - unsafe { - CloseHandle(self.handle); + if self.handle != INVALID_HANDLE_VALUE { + unsafe { + CloseHandle(self.handle); + } } } } @@ -236,6 +282,15 @@ impl ScsiTransport for SptiTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { + // Per RIP_DESIGN.md §15.1: parity with Linux's recovery contract. + // If a prior call invalidated the handle and try_recover() also + // failed, fail fast. + if self.handle == INVALID_HANDLE_VALUE { + return Err(Error::DeviceNotFound { + path: String::new(), + }); + } + // Zero the data buffer for reads to prevent returning uninitialized data // if the driver doesn't fully update DataTransferLength. if direction == DataDirection::FromDevice { @@ -254,7 +309,11 @@ impl ScsiTransport for SptiTransport { DataDirection::ToDevice => SCSI_IOCTL_DATA_OUT, }; sptwb.spt.DataTransferLength = data.len() as u32; - sptwb.spt.TimeOutValue = (timeout_ms / 1000).max(1) as u32; + // Round up to the next whole second so a 1500ms request gets at + // least 2s, not 1s. SPTI's TimeOutValue is u32 seconds with no + // sub-second resolution; biasing toward "more time" is safer than + // truncating (truncation broke 1500ms fast-reads on Drive::read). + sptwb.spt.TimeOutValue = ((timeout_ms + 999) / 1000).max(1); sptwb.spt.DataBuffer = if data.is_empty() { std::ptr::null_mut() } else { @@ -280,6 +339,12 @@ impl ScsiTransport for SptiTransport { }; if ok == 0 { + // Driver-level failure (timeout, handle gone, etc.). Recover + // the handle so the caller's retry loop can resume — same + // observable contract as Linux's async fd recovery, but + // synchronous because Windows's CloseHandle/CreateFileW don't + // block on in-flight CDBs. + self.try_recover(); return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index e706d0e..eda901d 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -10,7 +10,7 @@ use libfreemkv::{ }; use std::io::Write; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::time::{Duration, Instant}; const SECTOR_SIZE: usize = 2048; @@ -339,75 +339,52 @@ fn test_file_sector_reader_round_trip() { assert_eq!(all, data, "bulk read mismatch"); } -// ── 6. Disc::copy stall detection triggers skip-forward (TDD red) ───────── +// ── 6. Pass 1 sweeps the entire disc even when every read fails ─────────── // -// Regression guard for the Dell-host hang where `read_sectors` blocked inside -// a kernel-level USB stall and `Disc::copy` sat frozen for 10+ minutes with -// no progress and no error. The fix introduces `CopyOptions::stall_secs: -// Option` — when elapsed-since-last-`bytes_good`-advance exceeds the -// threshold, `Disc::copy` treats the current block as a read failure and -// triggers the skip-forward path so the rip can advance. -// -// THIS TEST IS EXPECTED TO FAIL UNTIL THE PARALLEL FIX LANDS. -// - Until `stall_secs` exists on `CopyOptions`, the test will not compile. -// - Once the field exists but the stall guard isn't wired, the spawned copy -// thread will never exit (test fails on the 5s join bound). -// - Once the guard is wired, copy returns within ~stall_secs with -// `complete=false` and `bytes_pending>0`. +// Per RIP_DESIGN.md §2.1 + §3: Disc::copy must reach the end of the disc +// regardless of how many reads fail. The only legitimate early exit is the +// halt flag. With `skip_on_error + skip_forward` and a reader that returns +// Err for every read, Pass 1 must: +// - mark every sector NonTrimmed (so Pass 2 can retry them) +// - return cleanly (no panic, no hang) +// - bytes_good = 0 +// - bytes_pending = total_bytes (NonTrimmed counts as pending in mapfile +// accounting; see disc/mapfile.rs::stats) +// - bytes_unreadable = 0 (only Pass 2 marks Unreadable) +// - complete = false (work remains for Pass 2) +// - halted = false (no user stop) +// - ISO file is `total_bytes` size on disk (sparse zeros) -/// Reader that returns Ok for sectors `< block_after`, then returns Err for -/// any sector `>= block_after` after a small per-call delay. Models the -/// realistic Dell-host symptom: reads keep returning Err (skip-forward fires) -/// but no `bytes_good` ever accrues; without a stall guard, Pass 1 grinds -/// silently for tens of minutes. -struct StallingSectorReader { +/// Reader that returns Err for every read. Models the worst case where the +/// drive can read nothing on this disc — Pass 1 must still walk to end of disc. +struct FailingSectorReader { capacity: u32, - block_after: u32, - /// Per-call delay for sectors >= block_after (simulates slow reads). + /// Per-call delay so the test exercises the skip-forward path realistically + /// without burning real wallclock. err_delay_ms: u64, - release: Arc, - /// Retained so callers can release the reader; unused now that the - /// reader returns Err instead of blocking, but kept so the test's - /// existing release plumbing compiles. - park: Arc<(Mutex<()>, std::sync::Condvar)>, } -impl StallingSectorReader { - fn new(capacity: u32, block_after: u32) -> Self { +impl FailingSectorReader { + fn new(capacity: u32) -> Self { Self { capacity, - block_after, - err_delay_ms: 100, - release: Arc::new(AtomicBool::new(false)), - park: Arc::new((Mutex::new(()), std::sync::Condvar::new())), + err_delay_ms: 0, } } - - fn release_handle(&self) -> (Arc, Arc<(Mutex<()>, std::sync::Condvar)>) { - (self.release.clone(), self.park.clone()) - } } -impl SectorReader for StallingSectorReader { +impl SectorReader for FailingSectorReader { fn read_sectors( &mut self, lba: u32, - count: u16, - buf: &mut [u8], + _count: u16, + _buf: &mut [u8], _recovery: bool, ) -> Result { - if lba >= self.block_after { - // Realistic stall model: read takes err_delay_ms then returns - // Err. With skip_on_error+skip_forward, Disc::copy will keep - // skip-forwarding through this region — no bytes_good accrues. - // The stall guard fires when bytes_good is unchanged for - // stall_secs. + if self.err_delay_ms > 0 { std::thread::sleep(Duration::from_millis(self.err_delay_ms)); - return Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 }); } - let bytes = count as usize * SECTOR_SIZE; - buf[..bytes].fill(0); - Ok(bytes) + Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 }) } fn capacity(&self) -> u32 { @@ -416,89 +393,135 @@ impl SectorReader for StallingSectorReader { } #[test] -fn test_disc_copy_stall_detection_triggers_skip_forward() { - // 1024 sectors total. Reader serves the first 64 sectors instantly, then - // every later read blocks forever. With stall_secs=2, copy should bail - // out of the stalled block within ~2s and either skip forward or finish - // with bytes_pending > 0 / complete=false. +fn test_disc_copy_completes_full_disc_with_failing_reader() { + // 1024 sectors = 2 MB. Reader fails every read. With skip_on_error + + // skip_forward, Pass 1 must mark every sector NonTrimmed and return + // cleanly — no bail, no hang. let capacity_sectors: u32 = 1024; - let block_after: u32 = 64; - - let reader = StallingSectorReader::new(capacity_sectors, block_after); - let (release_flag, park) = reader.release_handle(); - let mut reader = reader; + let total_bytes: u64 = capacity_sectors as u64 * SECTOR_SIZE as u64; + let mut reader = FailingSectorReader::new(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 iso_path_for_thread = iso_path.clone(); - - let join = std::thread::spawn(move || { - let opts = CopyOptions { - decrypt: false, - skip_on_error: true, - skip_forward: true, - // ASSUMPTION: parallel fix adds `pub stall_secs: Option` to - // CopyOptions. If the field name differs, update here. - stall_secs: Some(2), - ..Default::default() - }; - let t0 = Instant::now(); - let res = disc.copy(&mut reader, &iso_path_for_thread, &opts); - (res, t0.elapsed()) - }); - - // Bound the join to ~5s. With stall_secs=2 the copy should exit well - // within this window. If it doesn't, the stall guard isn't working. - let started = Instant::now(); - let mut joined = None; - while started.elapsed() < Duration::from_millis(5000) { - if join.is_finished() { - joined = Some(join.join().expect("thread join")); - break; - } - std::thread::sleep(Duration::from_millis(50)); - } - - // Whether or not the join succeeded, release the parked reader thread so - // it can exit (its &mut reader is owned by the spawned thread; releasing - // lets that thread unwind cleanly). - release_flag.store(true, Ordering::Relaxed); - park.1.notify_all(); - - let (result, elapsed) = match joined { - Some(v) => v, - None => { - // Wait a bit longer for the thread to drain after release so we - // don't leave it dangling, then fail the test. - std::thread::sleep(Duration::from_millis(500)); - panic!( - "Disc::copy did not return within 5s of stall_secs=2 — \ - stall guard not wired (TDD red until fix lands)" - ); - } + let opts = CopyOptions { + decrypt: false, + skip_on_error: true, + skip_forward: true, + ..Default::default() }; + let t0 = Instant::now(); + let result = disc + .copy(&mut reader, &iso_path, &opts) + .expect("copy returns Ok"); + let elapsed = t0.elapsed(); + // Cleanup let _ = std::fs::remove_file(&iso_path); let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&iso_path)); - let copy_result = result.expect("copy returns Ok with stall handling"); + // Hard bound — even at 0 ms per read, 1024 sectors with skip-forward + // should complete in well under a second on any host. If this test runs + // for minutes, something has regressed (e.g. stall guard reintroduced + // with infinite-loop semantics, or Pass 1 is hanging on each read). + assert!( + elapsed < Duration::from_secs(5), + "Pass 1 took {elapsed:?} on a 2 MB synthetic disc — expected < 5 s" + ); + + // Per RIP_DESIGN.md §2.1: Pass 1 must reach end of disc regardless of + // read outcomes. + assert_eq!( + result.bytes_total, total_bytes, + "bytes_total must match disc capacity" + ); + assert_eq!( + result.bytes_good, 0, + "no reads succeeded, bytes_good must be 0" + ); + assert_eq!( + result.bytes_unreadable, 0, + "Pass 1 does not mark Unreadable; only Pass 2 (Disc::patch) does" + ); + assert_eq!( + result.bytes_pending, total_bytes, + "every sector must be NonTrimmed → counted as pending. \ + Got bytes_pending={} of total {}", + result.bytes_pending, total_bytes + ); + assert!( + !result.complete, + "complete=false because NonTrimmed regions remain (work for Pass 2)" + ); + assert!( + !result.halted, + "no halt was set; halted must be false" + ); + + // ISO file should be the full disc size on disk (sparse zeros where + // reads failed). + // Note: tempfile was dropped above; the file may or may not still exist + // depending on cleanup ordering. We only assert what we can observe in + // the CopyResult. +} + +// ── 7. Halt during Pass 1 of an all-failing-read sweep returns promptly ─── +// +// Per RIP_DESIGN.md §3: halt is the only legitimate early exit from Pass 1. +// Even when every read is failing (skip-forward path), a halt must be +// honored within a small bounded time. + +#[test] +fn test_disc_copy_halts_promptly_on_failing_reader() { + let capacity_sectors: u32 = 1024 * 1024; // 2 GB synthetic disc — plenty of work + let mut reader = FailingSectorReader { + capacity: capacity_sectors, + err_delay_ms: 1, // small per-read delay so halt has something to interrupt + }; + 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 halt = Arc::new(AtomicBool::new(false)); + let halt_setter = halt.clone(); + + // Trigger halt after 200 ms. + std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(200)); + halt_setter.store(true, Ordering::Relaxed); + }); + + let opts = CopyOptions { + decrypt: false, + skip_on_error: true, + skip_forward: true, + halt: Some(halt), + ..Default::default() + }; + + let t0 = Instant::now(); + let result = disc + .copy(&mut reader, &iso_path, &opts) + .expect("copy returns Ok on halt"); + let elapsed = t0.elapsed(); + + // Cleanup + let _ = std::fs::remove_file(&iso_path); + let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&iso_path)); assert!( - elapsed < Duration::from_millis(5000), - "copy elapsed {elapsed:?} exceeded 5s bound (stall_secs=2)" + elapsed < Duration::from_secs(2), + "halt must return within 2 s; took {elapsed:?}" ); + assert!(result.halted, "result.halted must be true"); assert!( - copy_result.bytes_pending > 0, - "expected bytes_pending > 0 after stall-triggered skip; got {}", - copy_result.bytes_pending - ); - assert!( - !copy_result.complete, - "expected complete=false after stall-triggered skip" + !result.complete, + "halted run cannot be complete (bytes_pending > 0 likely)" ); }