pq/libfreemkv
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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:
matthew
2026-04-25 17:30:25 -07:00
parent 9d3022d457
commit 870623dc86
7 changed files with 449 additions and 219 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+82
View File
@@ -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<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)
### Fix: revert SgIoTransport timeout path to keep transport alive
Cargo.toml
+1 -1
View File
@@ -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"
src/disc/mod.rs
+23 -44
View File
@@ -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<u16>,
/// 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<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
@@ -1476,7 +1442,8 @@ pub struct PatchOptions<'a> {
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)]
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,
})
}
}
src/scsi/linux.rs
+52 -32
View File
@@ -54,8 +54,18 @@ const _: () = assert!(std::mem::size_of::<sg_io_hdr>() == 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<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 {
/// 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,11 +260,22 @@ impl ScsiTransport for SgIoTransport {
data: &mut [u8],
timeout_ms: u32,
) -> 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 {
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 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,
)
};
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 {
opcode: cdb[0],
src/scsi/macos.rs
+71 -10
View File
@@ -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<ComRef> {
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<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
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,
src/scsi/windows.rs
+69 -4
View File
@@ -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<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.
///
/// 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,11 +266,13 @@ pub(super) fn drive_has_disc(path: &Path) -> Result<bool> {
impl Drop for SptiTransport {
fn drop(&mut self) {
if self.handle != INVALID_HANDLE_VALUE {
unsafe {
CloseHandle(self.handle);
}
}
}
}
impl ScsiTransport for SptiTransport {
fn execute(
@@ -236,6 +282,15 @@ impl ScsiTransport for SptiTransport {
data: &mut [u8],
timeout_ms: u32,
) -> 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
// 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,
tests/integration_progress_and_halt.rs
+135 -112
View File
@@ -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<u64>` — 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<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 {
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<AtomicBool>, 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<usize> {
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<u64>` 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 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)"
);
}