v0.13.12 — Fix 1+2+4 + cross-platform SCSI parity (RIP_DESIGN.md §6, §7, §15.1)

Fix 1: delete stall guard from Disc::copy. Pass 1 must sweep end-to-end
per ddrescue model (RIP_DESIGN.md §2.1, §3, §9). The v0.13.9 guard at
disc/mod.rs broke Pass 1 at 30% on Dune 2 with 56 GB still NonTried.
Removed stall_secs field, narrative comment in scsi/linux.rs, and the
broken regression test. Replaced with test_disc_copy_completes_full_disc_
with_failing_reader and test_disc_copy_halts_promptly_on_failing_reader.

Fix 2: async SCSI transport recovery. Added Arc<AtomicI32> fd_recovery
on SgIoTransport. On poll timeout: spawn close + spawn open in
background, return Err immediately. Top of execute() swaps fd from
recovery atomic. Main thread never blocked beyond ~1.5s poll budget
(was up to ~60s per timeout because kernel serialized main-thread
open() against in-flight close()). Drop drains pending recovery fd.

§15.1 cross-platform parity: Windows + macOS now have the same
observable recovery contract. SptiTransport gets try_recover()
(synchronous CloseHandle + CreateFileW; Windows close is fast, no
in-flight CDB drain like Linux). MacScsiTransport gets try_recover()
(release IOKit interface + reacquire via new acquire_device_iface()
helper); stores bsd_name for re-resolution. Drop guards null'd-out
interfaces. Stripped English error strings ("try as root" / "run as
administrator") on Linux + Windows. Fixed Windows TimeOutValue
ms→s ceiling so 1500ms gets 2s (was 1s; broke Drive::read fast path).

Fix 4: instrument Disc::patch arms. PatchResult exposes
blocks_attempted, blocks_read_ok, blocks_read_failed so the v0.13.11
mystery (Dune 2 Pass 2 recovered 0 bytes in 100 min) is diagnosable
from the live device log without re-instrumenting from outside.

Cleanup: honor PatchOptions::full_recovery (was read into _ and
ignored; now routed to read_sectors recovery arg). Updated
CopyOptions::batch_sectors doc to describe the actual production
path (sysfs detect_max_batch_sectors, typically 60 sectors / ~120 KB
on BU40N) rather than the test-only 32-sector internal default.

All four crates clippy-clean and tests green on the host targets
(macOS native + cargo check on Linux). Cross-platform CI watches
Linux + Windows + macOS builds + tests.
This commit is contained in:
MattJackson
2026-04-25 17:30:25 -07:00
parent 92dc145864
commit b4951b1c5b
7 changed files with 449 additions and 219 deletions
+82
View File
@@ -1,5 +1,87 @@
# Changelog # 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<AtomicI32>` 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) ## 0.13.11 (2026-04-25)
### Fix: revert SgIoTransport timeout path to keep transport alive ### Fix: revert SgIoTransport timeout path to keep transport alive
+1 -1
View File
@@ -1,6 +1,6 @@
[package] [package]
name = "libfreemkv" name = "libfreemkv"
version = "0.13.11" version = "0.13.12"
edition = "2024" edition = "2024"
rust-version = "1.86" rust-version = "1.86"
license = "AGPL-3.0-only" license = "AGPL-3.0-only"
+23 -44
View File
@@ -1286,9 +1286,6 @@ impl Disc {
let mut buf = vec![0u8; batch as usize * 2048]; let mut buf = vec![0u8; batch as usize * 2048];
let mut bytes_done = 0u64; let mut bytes_done = 0u64;
let mut halt_requested = false; 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 // Iterate over not-yet-finished regions from the mapfile. We re-read the
// mapfile after each block because record() mutates the region list. // mapfile after each block because record() mutates the region list.
@@ -1318,30 +1315,6 @@ impl Disc {
break 'outer; 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 block_bytes = (region_end - pos).min(batch as u64 * 2048);
let lba = (pos / 2048) as u32; let lba = (pos / 2048) as u32;
let count = (block_bytes / 2048) as u16; 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 /// Resume from existing mapfile + ISO if present. Without this, any
/// existing mapfile is wiped and the ISO recreated. /// existing mapfile is wiped and the ISO recreated.
pub resume: bool, pub resume: bool,
/// Override the default block size. Defaults to 32 sectors (64 KB) in /// Override the default block size in sectors. Callers should resolve
/// `skip_forward` mode, `DEFAULT_BATCH_SECTORS` otherwise. /// 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<u16>, pub batch_sectors: Option<u16>,
/// Zero-fill bad blocks in the ISO, mark them in the mapfile, continue. /// Zero-fill bad blocks in the ISO, mark them in the mapfile, continue.
/// Uses fast reads (no drive-level recovery loop). /// Uses fast reads (no drive-level recovery loop).
@@ -1431,15 +1406,6 @@ pub struct CopyOptions<'a> {
pub skip_forward: bool, pub skip_forward: bool,
pub on_progress: Option<&'a dyn Fn(u64, u64)>, pub on_progress: Option<&'a dyn Fn(u64, u64)>,
pub halt: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>, pub halt: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>,
/// 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<u64>,
} }
/// Result of `Disc::copy`. `complete=true` means every byte reached a terminal /// Result of `Disc::copy`. `complete=true` means every byte reached a terminal
@@ -1476,7 +1442,8 @@ pub struct PatchOptions<'a> {
pub halt: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>, pub halt: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>,
} }
/// 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)] #[derive(Debug, Clone, Copy)]
pub struct PatchResult { pub struct PatchResult {
pub bytes_total: u64, pub bytes_total: u64,
@@ -1485,6 +1452,12 @@ pub struct PatchResult {
pub bytes_pending: u64, pub bytes_pending: u64,
pub bytes_recovered_this_pass: u64, pub bytes_recovered_this_pass: u64,
pub halted: bool, 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 { impl Disc {
@@ -1518,13 +1491,13 @@ impl Disc {
.map_err(|e| Error::IoError { source: e })?; .map_err(|e| Error::IoError { source: e })?;
let block_sectors = opts.block_sectors.unwrap_or(1); let block_sectors = opts.block_sectors.unwrap_or(1);
// Patch always reads with full drive recovery — this is the pass where let recovery = opts.full_recovery;
// 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 bytes_good_before = map.stats().bytes_good; let bytes_good_before = map.stats().bytes_good;
let mut halted = false; 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]; let mut buf = vec![0u8; block_sectors as usize * 2048];
// Collect bad ranges up front. Iterating while mutating is fragile; // Collect bad ranges up front. Iterating while mutating is fragile;
@@ -1551,10 +1524,12 @@ impl Disc {
let lba = (pos / 2048) as u32; let lba = (pos / 2048) as u32;
let count = (block_bytes / 2048) as u16; let count = (block_bytes / 2048) as u16;
let bytes = count as usize * 2048; let bytes = count as usize * 2048;
blocks_attempted += 1;
let read_ok = reader let read_ok = reader
.read_sectors(lba, count, &mut buf[..bytes], true) .read_sectors(lba, count, &mut buf[..bytes], recovery)
.is_ok(); .is_ok();
if read_ok { if read_ok {
blocks_read_ok += 1;
if opts.decrypt { if opts.decrypt {
crate::decrypt::decrypt_sectors(&mut buf[..bytes], &keys, 0)?; crate::decrypt::decrypt_sectors(&mut buf[..bytes], &keys, 0)?;
} }
@@ -1565,6 +1540,7 @@ impl Disc {
map.record(pos, block_bytes, mapfile::SectorStatus::Finished) map.record(pos, block_bytes, mapfile::SectorStatus::Finished)
.map_err(|e| Error::IoError { source: e })?; .map_err(|e| Error::IoError { source: e })?;
} else { } else {
blocks_read_failed += 1;
map.record(pos, block_bytes, mapfile::SectorStatus::Unreadable) map.record(pos, block_bytes, mapfile::SectorStatus::Unreadable)
.map_err(|e| Error::IoError { source: e })?; .map_err(|e| Error::IoError { source: e })?;
} }
@@ -1586,6 +1562,9 @@ impl Disc {
bytes_pending: stats.bytes_pending, bytes_pending: stats.bytes_pending,
bytes_recovered_this_pass: stats.bytes_good.saturating_sub(bytes_good_before), bytes_recovered_this_pass: stats.bytes_good.saturating_sub(bytes_good_before),
halted, halted,
blocks_attempted,
blocks_read_ok,
blocks_read_failed,
}) })
} }
} }
+52 -32
View File
@@ -54,8 +54,18 @@ const _: () = assert!(std::mem::size_of::<sg_io_hdr>() == 64);
pub struct SgIoTransport { pub struct SgIoTransport {
fd: i32, fd: i32,
device_path: std::path::PathBuf, 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<std::sync::atomic::AtomicI32>,
} }
// SgIoTransport's contained types (i32, PathBuf, Arc<AtomicI32>) 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 { impl SgIoTransport {
/// Open a SCSI device for use. Resets the drive first to ensure /// Open a SCSI device for use. Resets the drive first to ensure
/// a known good state, then opens a fresh fd for commands. /// a known good state, then opens a fresh fd for commands.
@@ -75,6 +85,7 @@ impl SgIoTransport {
Ok(SgIoTransport { Ok(SgIoTransport {
fd, fd,
device_path: device, 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(); let err = std::io::Error::last_os_error();
Err(if err.kind() == std::io::ErrorKind::PermissionDenied { Err(if err.kind() == std::io::ErrorKind::PermissionDenied {
Error::DevicePermission { Error::DevicePermission {
path: format!( path: device.display().to_string(),
"{}: permission denied (try running as root)",
device.display()
),
} }
} else { } else {
Error::DeviceNotFound { Error::DeviceNotFound {
@@ -219,6 +227,13 @@ impl Drop for SgIoTransport {
let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000); let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000);
unsafe { libc::close(self.fd) }; 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,11 +260,22 @@ impl ScsiTransport for SgIoTransport {
data: &mut [u8], data: &mut [u8],
timeout_ms: u32, timeout_ms: u32,
) -> Result<ScsiResult> { ) -> Result<ScsiResult> {
// 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 { if self.fd < 0 {
let recovered = self
.fd_recovery
.swap(-1, std::sync::atomic::Ordering::Acquire);
if recovered >= 0 {
self.fd = recovered;
} else {
return Err(Error::DeviceNotFound { return Err(Error::DeviceNotFound {
path: self.device_path.display().to_string(), path: self.device_path.display().to_string(),
}); });
} }
}
let mut sense = [0u8; 32]; let mut sense = [0u8; 32];
@@ -320,43 +346,37 @@ impl ScsiTransport for SgIoTransport {
}; };
if pr <= 0 { if pr <= 0 {
// Timeout (0) or fatal poll error (-1). // Timeout (0) or fatal poll error (-1). Command is still pending
// Command is still pending in the kernel. Spawn a background // in the kernel. Per RIP_DESIGN.md §4(b)/§7: close + reopen run
// close of the old fd (which blocks until the kernel // in a background thread so the main thread is never blocked
// completes/aborts the pending command) and open a fresh fd // beyond the poll() budget. The recovered fd is published to
// on the main thread. The main-thread open() can serialize // `fd_recovery`; the next call to execute() picks it up.
// 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.
let old_fd = self.fd; let old_fd = self.fd;
self.fd = -1; self.fd = -1;
std::thread::spawn(move || {
unsafe { libc::close(old_fd) };
});
let c_path = Self::to_c_path(&self.device_path); let c_path = Self::to_c_path(&self.device_path);
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 { let new_fd = unsafe {
libc::open( libc::open(
c_path.as_ptr() as *const libc::c_char, c_path.as_ptr() as *const libc::c_char,
libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC, libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC,
) )
}; };
self.fd = if new_fd >= 0 { new_fd } else { -1 }; 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 { return Err(Error::ScsiError {
opcode: cdb[0], opcode: cdb[0],
+71 -10
View File
@@ -175,6 +175,9 @@ const VTIDX_EXECUTE_SYNC: usize = 15;
pub struct MacScsiTransport { pub struct MacScsiTransport {
device_iface: ComRef, device_iface: ComRef,
exclusive: bool, 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. // IOKit COM interface pointers are Mach port references — safe to send between threads.
@@ -195,6 +198,19 @@ impl MacScsiTransport {
dev_str 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<ComRef> {
let service = find_scsi_service(bsd_name)?; let service = find_scsi_service(bsd_name)?;
// Create IOKit plugin for the MMC device // Create IOKit plugin for the MMC device
@@ -213,7 +229,7 @@ impl MacScsiTransport {
if kr != K_IO_RETURN_SUCCESS || plugin.is_null() { if kr != K_IO_RETURN_SUCCESS || plugin.is_null() {
return Err(Error::IoKitPluginFailed { return Err(Error::IoKitPluginFailed {
path: dev_str.to_string(), path: bsd_name.to_string(),
kr: kr as u32, kr: kr as u32,
}); });
} }
@@ -233,7 +249,7 @@ impl MacScsiTransport {
if hr != 0 || device_iface.is_null() { if hr != 0 || device_iface.is_null() {
return Err(Error::ScsiInterfaceUnavailable { 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 { if kr != K_IO_RETURN_SUCCESS {
com_release(device_iface); 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 { return Err(Error::DeviceLocked {
path: dev_str.to_string(), path: bsd_name.to_string(),
kr: kr as u32, kr: kr as u32,
}); });
} }
Ok(MacScsiTransport { Ok(device_iface)
device_iface, }
exclusive: true,
}) /// 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. // `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 { impl Drop for MacScsiTransport {
fn drop(&mut self) { fn drop(&mut self) {
if self.device_iface.is_null() {
return;
}
if self.exclusive { if self.exclusive {
unsafe { unsafe {
type Fn = unsafe extern "C" fn(ComRef) -> IOReturn; type Fn = unsafe extern "C" fn(ComRef) -> IOReturn;
@@ -356,6 +404,15 @@ impl ScsiTransport for MacScsiTransport {
data: &mut [u8], data: &mut [u8],
timeout_ms: u32, timeout_ms: u32,
) -> Result<ScsiResult> { ) -> Result<ScsiResult> {
// 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 // Create a SCSI task
let task: ComRef = unsafe { let task: ComRef = unsafe {
type Fn = unsafe extern "C" fn(ComRef) -> ComRef; type Fn = unsafe extern "C" fn(ComRef) -> ComRef;
@@ -363,6 +420,7 @@ impl ScsiTransport for MacScsiTransport {
f(self.device_iface) f(self.device_iface)
}; };
if task.is_null() { if task.is_null() {
self.try_recover();
return Err(Error::ScsiError { return Err(Error::ScsiError {
opcode: cdb[0], opcode: cdb[0],
status: 0xFF, status: 0xFF,
@@ -433,6 +491,9 @@ impl ScsiTransport for MacScsiTransport {
com_release(task); com_release(task);
if kr != K_IO_RETURN_SUCCESS { 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 { return Err(Error::ScsiError {
opcode: cdb[0], opcode: cdb[0],
status: 0xFF, status: 0xFF,
+69 -4
View File
@@ -85,8 +85,16 @@ unsafe extern "system" {
pub struct SptiTransport { pub struct SptiTransport {
handle: isize, 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<u16>,
} }
// SptiTransport contains an isize HANDLE and a Vec<u16>; 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. /// Normalize a device path to Windows \\.\X: format.
/// ///
/// NOTE: A near-identical `normalize_path` exists in `drive::windows`. /// NOTE: A near-identical `normalize_path` exists in `drive::windows`.
@@ -129,12 +137,48 @@ impl SptiTransport {
}; };
if handle == INVALID_HANDLE_VALUE { if handle == INVALID_HANDLE_VALUE {
return Err(Error::DeviceNotFound { // Map last-os-error → Error variant; don't embed English hints
path: format!("{}: cannot open device (run as administrator)", dev_str), // 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. /// Reset the drive to a known good state.
@@ -222,11 +266,13 @@ pub(super) fn drive_has_disc(path: &Path) -> Result<bool> {
impl Drop for SptiTransport { impl Drop for SptiTransport {
fn drop(&mut self) { fn drop(&mut self) {
if self.handle != INVALID_HANDLE_VALUE {
unsafe { unsafe {
CloseHandle(self.handle); CloseHandle(self.handle);
} }
} }
} }
}
impl ScsiTransport for SptiTransport { impl ScsiTransport for SptiTransport {
fn execute( fn execute(
@@ -236,6 +282,15 @@ impl ScsiTransport for SptiTransport {
data: &mut [u8], data: &mut [u8],
timeout_ms: u32, timeout_ms: u32,
) -> Result<ScsiResult> { ) -> Result<ScsiResult> {
// 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 // Zero the data buffer for reads to prevent returning uninitialized data
// if the driver doesn't fully update DataTransferLength. // if the driver doesn't fully update DataTransferLength.
if direction == DataDirection::FromDevice { if direction == DataDirection::FromDevice {
@@ -254,7 +309,11 @@ impl ScsiTransport for SptiTransport {
DataDirection::ToDevice => SCSI_IOCTL_DATA_OUT, DataDirection::ToDevice => SCSI_IOCTL_DATA_OUT,
}; };
sptwb.spt.DataTransferLength = data.len() as u32; 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() { sptwb.spt.DataBuffer = if data.is_empty() {
std::ptr::null_mut() std::ptr::null_mut()
} else { } else {
@@ -280,6 +339,12 @@ impl ScsiTransport for SptiTransport {
}; };
if ok == 0 { 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 { return Err(Error::ScsiError {
opcode: cdb[0], opcode: cdb[0],
status: 0xFF, status: 0xFF,
+135 -112
View File
@@ -10,7 +10,7 @@ use libfreemkv::{
}; };
use std::io::Write; use std::io::Write;
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
use std::sync::{Arc, Mutex}; use std::sync::Arc;
use std::time::{Duration, Instant}; use std::time::{Duration, Instant};
const SECTOR_SIZE: usize = 2048; const SECTOR_SIZE: usize = 2048;
@@ -339,75 +339,52 @@ fn test_file_sector_reader_round_trip() {
assert_eq!(all, data, "bulk read mismatch"); 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 // Per RIP_DESIGN.md §2.1 + §3: Disc::copy must reach the end of the disc
// a kernel-level USB stall and `Disc::copy` sat frozen for 10+ minutes with // regardless of how many reads fail. The only legitimate early exit is the
// no progress and no error. The fix introduces `CopyOptions::stall_secs: // halt flag. With `skip_on_error + skip_forward` and a reader that returns
// Option<u64>` — when elapsed-since-last-`bytes_good`-advance exceeds the // Err for every read, Pass 1 must:
// threshold, `Disc::copy` treats the current block as a read failure and // - mark every sector NonTrimmed (so Pass 2 can retry them)
// triggers the skip-forward path so the rip can advance. // - return cleanly (no panic, no hang)
// // - bytes_good = 0
// THIS TEST IS EXPECTED TO FAIL UNTIL THE PARALLEL FIX LANDS. // - bytes_pending = total_bytes (NonTrimmed counts as pending in mapfile
// - Until `stall_secs` exists on `CopyOptions`, the test will not compile. // accounting; see disc/mapfile.rs::stats)
// - Once the field exists but the stall guard isn't wired, the spawned copy // - bytes_unreadable = 0 (only Pass 2 marks Unreadable)
// thread will never exit (test fails on the 5s join bound). // - complete = false (work remains for Pass 2)
// - Once the guard is wired, copy returns within ~stall_secs with // - halted = false (no user stop)
// `complete=false` and `bytes_pending>0`. // - ISO file is `total_bytes` size on disk (sparse zeros)
/// Reader that returns Ok for sectors `< block_after`, then returns Err for /// Reader that returns Err for every read. Models the worst case where the
/// any sector `>= block_after` after a small per-call delay. Models the /// drive can read nothing on this disc — Pass 1 must still walk to end of disc.
/// realistic Dell-host symptom: reads keep returning Err (skip-forward fires) struct FailingSectorReader {
/// but no `bytes_good` ever accrues; without a stall guard, Pass 1 grinds
/// silently for tens of minutes.
struct StallingSectorReader {
capacity: u32, capacity: u32,
block_after: u32, /// Per-call delay so the test exercises the skip-forward path realistically
/// Per-call delay for sectors >= block_after (simulates slow reads). /// without burning real wallclock.
err_delay_ms: u64, err_delay_ms: u64,
release: Arc<AtomicBool>,
/// 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 { impl FailingSectorReader {
fn new(capacity: u32, block_after: u32) -> Self { fn new(capacity: u32) -> Self {
Self { Self {
capacity, capacity,
block_after, err_delay_ms: 0,
err_delay_ms: 100, }
release: Arc::new(AtomicBool::new(false)),
park: Arc::new((Mutex::new(()), std::sync::Condvar::new())),
} }
} }
fn release_handle(&self) -> (Arc<AtomicBool>, Arc<(Mutex<()>, std::sync::Condvar)>) { impl SectorReader for FailingSectorReader {
(self.release.clone(), self.park.clone())
}
}
impl SectorReader for StallingSectorReader {
fn read_sectors( fn read_sectors(
&mut self, &mut self,
lba: u32, lba: u32,
count: u16, _count: u16,
buf: &mut [u8], _buf: &mut [u8],
_recovery: bool, _recovery: bool,
) -> Result<usize> { ) -> Result<usize> {
if lba >= self.block_after { if self.err_delay_ms > 0 {
// 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.
std::thread::sleep(Duration::from_millis(self.err_delay_ms)); 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; Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 })
buf[..bytes].fill(0);
Ok(bytes)
} }
fn capacity(&self) -> u32 { fn capacity(&self) -> u32 {
@@ -416,89 +393,135 @@ impl SectorReader for StallingSectorReader {
} }
#[test] #[test]
fn test_disc_copy_stall_detection_triggers_skip_forward() { fn test_disc_copy_completes_full_disc_with_failing_reader() {
// 1024 sectors total. Reader serves the first 64 sectors instantly, then // 1024 sectors = 2 MB. Reader fails every read. With skip_on_error +
// every later read blocks forever. With stall_secs=2, copy should bail // skip_forward, Pass 1 must mark every sector NonTrimmed and return
// out of the stalled block within ~2s and either skip forward or finish // cleanly — no bail, no hang.
// with bytes_pending > 0 / complete=false.
let capacity_sectors: u32 = 1024; let capacity_sectors: u32 = 1024;
let block_after: u32 = 64; let total_bytes: u64 = capacity_sectors as u64 * SECTOR_SIZE as u64;
let reader = StallingSectorReader::new(capacity_sectors, block_after);
let (release_flag, park) = reader.release_handle();
let mut reader = reader;
let mut reader = FailingSectorReader::new(capacity_sectors);
let disc = synthetic_disc(capacity_sectors); let disc = synthetic_disc(capacity_sectors);
let tmp = tempfile::NamedTempFile::new().expect("tempfile create"); let tmp = tempfile::NamedTempFile::new().expect("tempfile create");
let iso_path = tmp.path().to_path_buf(); let iso_path = tmp.path().to_path_buf();
drop(tmp); drop(tmp);
let iso_path_for_thread = iso_path.clone();
let join = std::thread::spawn(move || {
let opts = CopyOptions { let opts = CopyOptions {
decrypt: false, decrypt: false,
skip_on_error: true, skip_on_error: true,
skip_forward: true, skip_forward: true,
// ASSUMPTION: parallel fix adds `pub stall_secs: Option<u64>` to
// CopyOptions. If the field name differs, update here.
stall_secs: Some(2),
..Default::default() ..Default::default()
}; };
let t0 = Instant::now(); let t0 = Instant::now();
let res = disc.copy(&mut reader, &iso_path_for_thread, &opts); let result = disc
(res, t0.elapsed()) .copy(&mut reader, &iso_path, &opts)
}); .expect("copy returns Ok");
let elapsed = 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)"
);
}
};
// Cleanup // Cleanup
let _ = std::fs::remove_file(&iso_path); let _ = std::fs::remove_file(&iso_path);
let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&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!( assert!(
elapsed < Duration::from_millis(5000), elapsed < Duration::from_secs(2),
"copy elapsed {elapsed:?} exceeded 5s bound (stall_secs=2)" "halt must return within 2 s; took {elapsed:?}"
); );
assert!(result.halted, "result.halted must be true");
assert!( assert!(
copy_result.bytes_pending > 0, !result.complete,
"expected bytes_pending > 0 after stall-triggered skip; got {}", "halted run cannot be complete (bytes_pending > 0 likely)"
copy_result.bytes_pending
);
assert!(
!copy_result.complete,
"expected complete=false after stall-triggered skip"
); );
} }