The remaining triage items after tonight's HIGH fixes: 1,290 lines, almost
all tests. Covers disc/mod.rs's DVD scan path (with real minimal VMG/VTS IFO
fixtures rather than mocks), drive/mod.rs, labels/class_reader.rs and
labels/mod.rs — the two biggest untriaged survivor clusters in the crate —
plus hevc.rs and ps.rs.
One production change, and it is an extraction rather than a behaviour
change: MacScsiTransport::open mapped the shim's negative failure sentinels
to typed errors inline, where nothing could reach it without a real IOKit
FFI call. It is now map_shim_open_error, so the mapping can be pinned. It
matters because collapsing -5 into the DeviceNotFound catch-all turns
"another process holds the drive" into "no such drive", and an operator
chasing the wrong problem is worse than a blunt error.
Gate green on the pinned toolchain including the secrets scanner.
Thirteen agents triaging src/labels and src/disc died on a saturated
machine, leaving 5,836 insertions across 28 files uncommitted in a
worktree. Recovered by 3-way apply onto twelve commits of drift; zero
conflicts. The diff was archived to freemkv-private first, because a
worktree is not a backup and this one had already nearly been lost.
One production change, and it is the right one: mpls_universal::parse
read every playlist off the disc AND converted the entries to labels in
a single function, so the conversion — stream-type mapping, dedup key,
the dense global counters — could only be reached through a synthetic
UDF image. Extracted to build_labels(&[Playlist]), which unit tests can
drive from already-parsed values. Behaviour-preserving: same iteration
order, same skip-on-error.
Two collisions resolved by hand:
A second mod pass_progress_tests, written independently against the
same survivors as the one committed in c610285. Kept mine — it covers
the distinct-counters case and the Progress blanket impl, which theirs
does not — but theirs had three clamp tests mine lacked: good_pct,
bad_pct and pending_pct also clamp an overshoot, and I had only tested
that for work_pct. Merged those in as one test and proved each of the
three clamps load-bearing by removing them individually.
An unused_parens warning in a new fixture.
Method note, recorded because it cost real time: git apply --3way
STAGES its result, so `git diff` reads empty and the tree looks
untouched. I nearly concluded the patch had silently failed. Worse, the
first attempt piped through `head -20`, so `echo exit=$?` reported
head's status rather than git's — the same mistake this audit has
already documented once. Check the real exit status, and check
--cached, not just the working tree.
The worst of the five was a regression suite that never touched the code
it guarded: nine batch-count tests called `safe_batch_count` and
`buggy_batch_count`, both defined in the test file itself. The u16
truncation they exist to prevent could be reintroduced in
sector/prefetched.rs with every one of them green. They now drive the real
producer through the public API, and reinstating the truncation fails five
of the nine. Worth recording that the symptom has changed since the
original fix: the unit-alignment clamp below floors a zero batch at three
sectors, so the bug is now a twenty-fold throughput cliff rather than the
stall it once was.
The MP4 reserve test's only numeric case was dominated by the floor and
the buffer, so BYTES_PER_SAMPLE could be zeroed without failing it. It now
has a case where the per-sample term dominates. The zero-count guard in
FileSectorSource was likewise unfalsifiable — seek-past-EOF and a
zero-length read both succeed — so the test now observes the file cursor.
The AACS media-key ambiguity guard had no test at all; the pool scan is
extracted so the verifier can be injected, because a genuine two-key
collision needs one ciphertext decrypting under two AES-128 keys to
plaintexts sharing a 64-bit magic, which is a 2^64 search and not a
fixture.
macOS implemented the documented cheap, side-effect-free presence probe by
building a full exclusive transport — which force-unmounts the disc. Linux
and Windows issue one TEST UNIT READY with no unmount; macOS was the
outlier. It now walks the IOKit registry for the media object instead.
The C shim's registry reads assumed CoreFoundation types the registry does
not guarantee, so a driver publishing a CFNumber where a CFString was
expected aborted the process from inside public API. Types are checked and
a wrong type treated as absent. The unbounded waitpid on the unmount child
is now a polled deadline, and the last-resort match gained the NULL check
its two siblings already had.
The empty-CDB guard existed only on Linux while a shared helper's comment
claimed all three backends had it. Moved into the helper, so the comment
is now true and macOS and Windows are covered.
One finding was REJECTED with evidence rather than fixed. The TrueHD
buffer-cap test was indeed bogus, but MAX_TRUEHD_BUF turns out to be
unreachable by any input: the parser only retains data when the buffer is
shorter than the declared AU, and that declaration is twelve bits, so the
worst case is 8189 bytes against a 256 KiB cap. An exhaustive sweep over
all 65536 AU headers confirmed it. The fixture now sits at the reachable
ceiling and asserts that instead. The cap itself is left in place as
defence, unreachable by construction, matching how the AC-3 resync guard
was handled earlier in this audit.
Two behaviour changes worth naming: Linux's empty-CDB error becomes
InvalidCdbLength rather than a transport failure, and an unknown device
now reports absent media rather than a not-found error, because the
registry cannot tell an empty drive from a missing one. The latter is a
conflation of the kind this audit has fixed three times; it is recorded
for the next round rather than left silent.
Both backends discarded the platform's own error code on the execute hot
path — the one every READ(10) of a rip goes through — and collapsed every
cause to the same status-0xFF transport failure.
On Windows, open() and reset() in the same file both capture the Win32
error; execute() did not. That left ERROR_INVALID_PARAMETER (a struct
layout regression, the exact class this file's SDK-layout tests exist to
catch), ERROR_ACCESS_DENIED and ERROR_GEN_FAILURE (a genuinely wedged
drive) indistinguishable, with nothing in the log to tell a code bug from
a hardware one.
On macOS the same, and worse: the file had no tracing calls at all, where
the Linux and Windows backends both log their execute failures. Its open()
carefully decodes the shim's sentinel into typed variants instead of
flattening them, but execute() threw the IOKit return away — so another
process taking exclusive access mid-rip and a real hardware wedge produced
identical, empty diagnostics.
Logged rather than added to the error type: the typed variant is public
API, and the recovery classification is deliberately the same for all of
these. What was missing is the breadcrumb, not the distinction.
Two further findings from the same sweep were rejected. Windows reset()
always returning Ok(()) and macOS ignoring timeout_ms are both already
documented in the code as deliberate, and the reporter flagged them for
completeness rather than as defects.
Neither fix has a test: reaching either branch needs a failing ioctl or a
failing IOKit call, and both files are compiled only on their own platform.
Four round-6 findings.
FileSectorSource::read_sectors guarded its output buffer with a
debug_assert, which is compiled out in release — so an undersized buffer
panicked with 'range end index out of range' instead of returning an
error, out of a public SectorSource impl where the length is caller input.
Drive::read_fua already carries this exact guard, with a comment recording
the same panic being fixed there, and PrefetchedSectorSource has a
regression test for the same case; this impl had been given neither. The
new test is red in release for precisely the predicted reason: 'range end
index 8192 out of range for slice of length 2049'.
parse_track's sample-rate ladder ended in an unconditional S48, so any
SamplingFrequency below 44100 was recorded as 48 kHz. A 32000 Hz AC-3 or
DTS track is legal and common in broadcast-sourced content, and the wrong
rate then propagated into the reconstructed AudioStream. Anything below
the lowest mapped rate is now Unknown, which is what the crate's canonical
SampleRate::from_hz already returned — the ladder disagreed with it. The
ladder itself stays, because the MKV element is a float and wants
tolerance rather than exact equality.
shim_open_exclusive used the mach port from IOMainPort without checking
the return; on failure the port is left untouched and every IOKit call
below ran against an uninitialised value. shim_list_drives in the same
file does check it.
build.rs treated cc and ar as successful if the process merely SPAWNED, so
a genuine compile error in the macOS C shim produced no object file and
surfaced later as an unexplained link failure against a missing symbol.
The shim is macOS-only and is neither linted nor compiled on the other two
platforms, so a mistake in it has exactly one chance to be noticed.
The last two have no test: one needs IOMainPort to fail, the other needs a
deliberately broken C shim, and neither is reachable from the test
harness. Both mirror a correct sibling in the same file, which is the
evidence available.
CI's clippy job runs on ubuntu-latest, so cfg(target_os = "linux") code is
what the gate actually compiles — and none of it is built by clippy on a
Mac. Five `-D warnings` errors were sitting in drive/linux.rs,
scsi/linux.rs, io/writeback/linux.rs and the Linux arm of drive/mod.rs:
four collapsible let-chains and one manual `% n == 0`. CI was red on the
lint job while the local gate reported all green.
Collapsed into let-chains, which the declared toolchain supports, and
folded drive/mod.rs's length precondition into its chain so the body no
longer needs a nested block.
Found because an agent working on the SCSI backends reported the lints in
passing while checking that a Windows-only file compiled. Worth noting how
it stayed hidden: every one of these files is cfg-gated to a platform this
machine is not, so no amount of local gating would have surfaced them. The
companion change to the precommit script closes that hole.
Two fixes from round 5.
The Linux and Windows backends truncated a CDB longer than 16 bytes
(`cdb.len().min(16)`) where macOS returned InvalidCdbLength. Under SPC-4 a
command's length is fixed by its opcode group code, so a shortened CDB is
not a shorter form of the same command — it is a DIFFERENT command, and
the drive executes it and answers GOOD with data for a request nobody
made. A silently wrong result on the layer everything else sits on.
Rather than mirror the guard a third time it now lives in scsi::mod as
checked_cdb_len, with all three backends routed through it, so it cannot
drift per platform again. That also makes it testable everywhere: each
platform module is cfg-gated to its own host, so a guard inlined into
linux.rs and windows.rs would have had no test coverage on any single
machine. The shared helper is the only place the behaviour can be
asserted on every platform's CI.
The two existing macOS tests were tautological — they replicated the
guard's logic inline instead of calling it, so they would have passed with
the guard deleted. They now call the real helper.
Separately, the H.264 keyframe parameter-set re-assert grew a
few-hundred-byte prefix buffer to the full access-unit size, copied the
whole frame into it, and dropped the presized buffer: one extra
whole-frame allocation and copy per keyframe. A UHD title is ~200,000
frames of 150-400 KB with a keyframe every second or two, so that is
thousands of avoidable multi-hundred-KB copies per title, each large
enough to go through mmap. It now splices into the reserved headroom in
place. This mirrors the identical fix already made in hevc.rs, which the
H.264 path had drifted from.
Byte-for-byte equivalence is pinned by a test whose expected literals
were captured from the pre-change implementation, and which I confirmed
still passes when the old build-and-copy code is restored. The
no-reallocation claim is measured rather than argued: a counter over 30
bare keyframes, which reports 30 of 30 against the old path and 0 with
the splice.
The reallocation test initially passed even with PARAM_REASSERT_HEADROOM
set to zero, because a small parameter set fits in the presize's
incidental slack — it proved the fixture did not reallocate, not that the
headroom prevented it. Its SPS is now large enough that the constant is
load-bearing, so zeroing it fails the test.
Not verified: no runtime behaviour on Linux or Windows: no drive, no
ioctl. Both files were confirmed to compile for their own targets.
Disc::sweep, Disc::patch, Disc::copy, SweepOptions and PatchOptions have
zero occurrences in src/ — recovery moved to freemkv-engine — but they were
still documented in 30 places across README.md, TROUBLESHOOTING.md, six
files under docs/, seven src/ doc comments and a Cargo.toml comment.
README.md is the crate's GitHub front page and carried a full multi-pass
code example that cannot compile.
Two of the src/ references were intra-doc LINKS to deleted items
([`disc::Disc::copy`], [`disc::Disc::patch`] in scsi/mod.rs). They produced
no warning on a normal `cargo doc` only because they sit on pub(crate)
items; `--document-private-items` reports both, and they are gone now.
The README example is deleted rather than rewritten against the engine's
API: libfreemkv documenting a downstream crate's API on its own front page
is the drift that produced this, and it cannot even depend on it. The src/
references become plain code spans naming freemkv_engine::recovery::* —
deliberately not links, for the same reason.
docs/rip-recovery.md was 202 lines about relocated code. It now documents
only what this crate owns — Drive::read, SenseFamily, DiscStream's adaptive
batch halving — plus the read-path design constraints, which belong with the
code that enforces them, and points at freemkv-engine/src/recovery/ for the
strategy. api-design.md's module tree is regenerated from the real src/disc/
and src/drive/ layouts instead of hand-patched; it had listed sweep.rs,
patch.rs, mapfile.rs and read_error.rs, none of which exist.
Three stale facts surfaced while rewriting and are corrected: the read
timeouts are 10 s / 60 s, not the documented 1.5 s / 30 s; Drive::reset and
SgIoTransport::reset no longer exist at all, so "no SCSI reset from any read
path" is now stated as the stronger fact it has become; and verify_title,
listed as a progress-emitting operation, was removed entirely.
CHANGELOG.md keeps its references — those are the historical record of the
releases that shipped the API.
Moved SenseFamily::from_sense_key + is_wedge_family from disc/read_error.rs
into scsi/mod.rs (with its own tests) and re-exported at the crate root.
This is pure SCSI sense-code classification -- objective hardware fact, zero
recovery-policy opinion -- so it belongs in the library primitives, unlike
the retry-DECISION state machine (ReadCtx/PassSummary/ReadAction/
handle_read_error) built on top of it, which is freemkv's specific recovery
strategy and is moving to freemkv-engine next.
disc/read_error.rs and disc/section_recover.rs now import SenseFamily from
crate::scsi instead of defining/re-exporting their own copy. No behavior
change. Precommit green on Rust 1.86 (fmt+clippy+test).
mp4 demuxer (untrusted input): bound every allocation sized from a box
field (stsz/stco/stsc counts, stts/ctts run-lengths, per-sample and moov
sizes, plus an absolute cap so a sparse file can't inflate file_len);
guard the parse_stsd slice and a zero mdhd timescale; cap track count so
the per-track PID can't overflow; rewrite read_moov to handle size==0 /
size<8 / 64-bit largesize; parse esds/AudioSpecificConfig for AAC; write
tkhd duration in the movie timescale.
decrypt: resolve_mux_key_map now fails loud on an extent no key can
classify instead of inheriting the previous extent's key, so a keymap
never silently carries a wrong key; the sweep/patch key-fetch recovery
fails loud when a unit is still unresolved after the retry.
AACS: reject inverted forensic segments in both range builders; compare
the forensic index in u16 space so an out-of-range value can't truncate
onto a valid u8 index. RECOVERED_ERROR no longer latches the damage zone,
preserving the 30s wedge cooldown for a following hard error.
audio: AAC/MP2/MP3/FLAC carry the last PTS across a PES with no timestamp;
the DTS-HD extension-sync search is bounded to after the core; the MP4
16.16 sample-rate field saturates. demux_sink records the video reference
before the kind filter so audio:// / sub:// keep multi-clip PTS continuity
and the DELAY tag.
Remove a dead error variant and the AACS-unsupported-video code; codec
comments cite the primary format specs; assorted doc/naming fixes and
regression tests throughout.
- CSS: unlock scrambled-sector reads on enforcing drives via bus-auth
only; classify sense 6F/03 as CSS-locked; early-bail on a fully locked
scan; gate the AACS handshake off DVD discs.
- DVD first-play menu no longer prepended to the feature: read the title
VOBS base from vtstt_vobs (0xC4), not the menu VOBS vtsm_vobs (0xC0).
- Interlaced field-duration (DefaultDecodedFieldDuration) written as a
direct TrackEntry child rather than inside Video, so Windows reports
the correct frame rate.
- Audio channel count read from the AC-3 bitstream; FieldOrder set to
TFF; per-track BPS tags.
- Structured disc diagnostics at --log-level 3; reduced per-operation
log spam.
Audited every #[repr(C)] struct and IOCTL/flag constant in scsi/windows.rs
against the authoritative Windows SDK headers (ntddscsi.h, winioctl.h,
devioctl.h, winnt.h, fileapi.h). All correct except the already-reverted
ScsiPassThroughDirect packing. Add the missing regression guards:
- StoragePropertyQuery layout (STORAGE_PROPERTY_QUERY: 0/4/8, size 12).
- IOCTL/flag constants, with IOCTLs asserted against an independent CTL_CODE
re-derivation (not a tautological literal) so a mistyped code is caught.
Validated compiling via cargo xwin check --target x86_64-pc-windows-msvc.
rc.4 added #[repr(C, packed(4))] to ScsiPassThroughDirect on the false premise
that ntddscsi.h wraps SCSI_PASS_THROUGH_DIRECT in #pragma pack(push, 4). It does
NOT — verified against the Windows SDK ntddscsi.h: the struct has no pragma pack
and uses natural alignment. On 64-bit Windows (LLP64) that puts DataBuffer at
offset 24 and the struct at 56 bytes, which bare #[repr(C)] produces and which
DeviceIoControl expects.
packed(4) instead imposed offset 20 / 48 bytes — the layout of the SDK's
SEPARATE 32-bit thunk struct SCSI_PASS_THROUGH_DIRECT32 (VOID* POINTER_32). Using
that on a 64-bit host malformed every IOCTL_SCSI_PASS_THROUGH_DIRECT, so the
INQUIRY in drive enumeration failed and autorip/CLI reported zero drives
('RC4 no longer detects my drive'). rc.3.1 (bare repr(C)) worked for the same
users; this restores that layout.
Replace the tautological packed-layout test (which asserted the same wrong
offsets the struct produced) with one cross-checked against the SDK header:
DataBuffer@24, SenseInfoOffset@32, Cdb@36, size 56. Verified compiling via
cargo xwin check for x86_64-pc-windows-msvc.
ntddscsi.h wraps SCSI_PASS_THROUGH_DIRECT in #pragma pack(push, 4),
forcing the PVOID DataBuffer field to 4-byte alignment even on 64-bit
hosts. The Rust struct used bare #[repr(C)], so the compiler applied
natural 8-byte pointer alignment and inserted 4 padding bytes after
TimeOutValue. That shifted DataBuffer to offset 24 (SDK: 20),
SenseInfoOffset to 32 (28), and Cdb to 36 (32), and grew the struct to
56 bytes (48). DeviceIoControl reads at the SDK offsets, so every SPTI
ioctl on 64-bit Windows either got rejected or interpreted garbage as
the CDB and DataBuffer pointer.
Add #[repr(C, packed(4))] to ScsiPassThroughDirect and the companion
SptwbDirect (so offset_of!(SptwbDirect, sense) stays correct for
SenseInfoOffset), plus a layout regression test asserting DataBuffer at
offset 20 and a 48-byte struct size.
STORAGE_ADAPTER_DESCRIPTOR.BusType is STORAGE_BUS_TYPE, an int-sized
(4-byte) enum, but the repr(C) struct declared it as u8. The total
size stayed 32 bytes by coincidence of alignment padding, and the two
fields actually read today (MaximumTransferLength at offset 8 and
AlignmentMask at offset 16) sit ahead of BusType, so there was no
runtime impact. But BusMajorVersion and BusMinorVersion landed at
offsets 26 and 28 instead of the SDK's 28 and 30, so any future reader
of those fields would have gotten wrong values.
Widen BusType to u32 so BusMajorVersion/BusMinorVersion fall at the
correct offsets, and add a layout regression test asserting every
field offset and the 32-byte total against the winioctl.h layout.
SptiTransport::reset() unconditionally slept 2 seconds after sending
IOCTL_STORAGE_RESET_DEVICE, even when the IOCTL failed (e.g.
ERROR_INVALID_FUNCTION on a driver that does not support the reset).
On failure no reset occurred, so there is nothing to settle and the
2-second penalty was pure waste. Gate the settle sleep on the IOCTL
return so it only fires when the drive was actually reset.
SptiTransport::reset() discarded the DeviceIoControl return value, so a
wrong or unsupported reset IOCTL would fail with ERROR_INVALID_FUNCTION
and silently no-op while the unconditional 2s settle sleep made it look
like a reset happened. That is exactly the regression class the doc block
records for the two earlier (incorrect) code values.
Bind the result and warn (with GetLastError) when the reset fails, debug
on success. Lift IOCTL_STORAGE_RESET_DEVICE to module scope and add a
test recomputing it from the CTL_CODE formula so a wrong value can't slip
back in unnoticed.
The dual-model Windows audit (Sonnet) caught that the prior 'fix' (0x002DD000)
was also wrong: that decodes to the OBSOLETE RESET_BUS code (function 0x400,
R|W access) which class drivers reject. Canonical ntddstor.h:
IOCTL_STORAGE_RESET_DEVICE = CTL_CODE(0x2D, 0x0401, METHOD_BUFFERED, FILE_READ_ACCESS)
= (0x2D<<16)|(1<<14)|(0x401<<2)|0 = 0x002D5004.
Only affects the best-effort drive-reset recovery path (return value is
discarded); the normal read/unlock/rip path never calls it, so this does not
change normal Windows operation. Verified correct-by-construction; the
AlignmentMask finding is deferred to rc4 (benign on USB optical bridges,
needs a real Windows SCSI-HBA rig to validate).
- patch (Pass N) now aborts immediately on transport failure (status=0xFF),
symmetric with the sweep and single-pass mux. Previously a USB-bridge crash
was treated as an ordinary bad sector and the pass hammered the crashed
device sector-by-sector until the per-range watchdog expired. (medium)
- patch AACS recovery reads are now unit-aligned: a mid-unit single-sector
read on an AACS disc was rejected by the decrypting reader (DecryptFailed)
and the sector abandoned without asking the drive. The read is now widened
to the enclosing whole 3-sector unit and the requested window copied out,
leaving all recovery accounting (pos/block_bytes/cursor) untouched so it
cannot desync. Only affected CLI decrypt-to-ISO --multipass re-runs. (low)
- IOCTL_STORAGE_RESET_DEVICE corrected 0x002D1004 -> 0x002DD000 (the old value
decoded to function 0x401 with the access bits cleared, so DeviceIoControl
would fail ERROR_INVALID_FUNCTION instead of resetting). Windows-only. (low)
Adds a transport-failure classification regression test.
Three fixes for a Windows ASUS Blu-ray drive that failed/spammed errors:
- resolve.rs: accept disk:// as an alias for disc:// (identical behavior;
empty = auto-detect, path = device). Windows users commonly type
disk://i: after the drive-letter convention.
- drive::find_drive: prefer a drive that reports media present. Enumerate
all optical drives, query Drive::drive_status() (GET EVENT STATUS, works
regardless of firmware), and return the first reporting DiscPresent;
fall back to the first enumerated drive when none report a disc so
single-drive / quirky setups don't regress. Selection policy split into
select_drive_with_media() for unit testing.
- READ chunking: add ScsiTransport::max_transfer_bytes() (default 1 MiB).
Windows SPTI overrides it with the adapter MaximumTransferLength queried
via IOCTL_STORAGE_QUERY_PROPERTY / StorageAdapterProperty, clamped to a
64 KiB floor (fallback on query failure). Drive::read now caps each
READ(10) to that limit: small reads take the unchanged single-CDB path,
larger reads loop over read_one() chunks, reporting the failing chunk's
LBA on error. This stops the 16 MiB single read that exceeded the
adapter limit, made DeviceIoControl fail, and spammed transport-failure
warnings with slow tiny-read fallbacks.
Tests added for the disk:// alias, media-preference selection, and READ
chunk decomposition / per-chunk error LBA.
Test-hardening release, no runtime changes. Adds spec-grounded unit tests
across the silent-corruption surfaces — UDF/MPLS/CLPI/IFO parsing, BD/DVD
title + extent assembly, AACS/CSS key handling, TS/PS demux + codec parsers,
MKV/EBML container output, the mux pipeline, sector prefetch + decrypt
decorator, drive/SCSI sense decoding, label extraction, and core I/O. Each
test is grounded in the format spec or real on-disc behavior and verified to
fail under a targeted source mutation. No behavior changed.
Library-wide review-and-fix pass: tightened AACS keydb/handshake/variant
handling and trailing-partial-unit policy, corrected MPLS mark offset and
added UDF allocation bounds, hardened the mux/codec framing and M2TS paths,
guarded SCSI READ CAPACITY short transfers and unified error mapping, added
overflow guards on untrusted disc input, and made prefetch shutdown
deterministic. Release profile now builds with thin LTO + single codegen unit.
Direct-SATA BU40N + Dune Part Two UHD live testing exposed that the
v0.17.3 single-shot SCSI READ path matched 0/22 of the small bad-
sector LBAs that dd if=/dev/sr0 recovers on the same drive. This
release closes that gap and fixes adjacent bugs silently capping
recovery.
- /dev/sr0 pread fallback in Drive::read (Linux only): on SCSI READ
Err, fall back to posix_fadvise(DONTNEED) + pread() against the
corresponding block device. Kernel sr_mod runs ~5 internal retries
with no per-attempt mid-layer escalation overhead — the mechanism
behind dd's recovery advantage. End-to-end byte verification
confirms the fallback path returns real disc data.
- Disc::patch per-range watchdog fix: MAX_RANGE_SECS was breaking
'outer (one slow range killed the entire patch). Now skips to the
next range. Pre-fix patch died after 4 sectors of range 1 of 47.
- Per-sector range budget: range_budget = sectors × 25 s, capped at
1800 s. Replaces the flat 180 s/range that was unfair to medium
ranges and pointlessly generous to single-sector ones.
- consecutive_failures resets per range. The wedge-exit detector is
for stuck-on-one-range, not many-small-ranges-with-one-fail-each.
- Reverted inline 5× retry experiment (was hurting: each retry paid
kernel SCSI escalation overhead). Restored READ_RECOVERY_TIMEOUT_MS
to 60 s. The kernel-auto-retry pattern is now provided by sr0
fallback.
Empirical: pass 1 recovered 94.6 MB / 11 s of main title (33 sr0
saves). Pass 2 added 0.6 MB. Remaining ~233 MB on the test disc
appears physically unrecoverable on this hardware.
macOS SCSI transport rewritten from hybrid MMC+pread to single-path
raw CDB dispatch through SCSITaskDeviceInterface. All CDBs (INQUIRY,
READ, REPORT KEY, etc.) now go through ExecuteTaskSync — 1:1 with
the Linux SG_IO backend.
Key changes:
- New macos_shim.c: diskutil unmount → find IOBDServices →
ObtainExclusiveAccess → raw CDB dispatch. Eliminates Rust-side
IOKit COM vtable complexity.
- build.rs compiles macos_shim.c via cc into static lib
- macos.rs simplified to three FFI calls (open/close/execute)
- disc/mod.rs: graduated batch restore after errors, skip-ahead
through bad zones, configurable error pause
- as_bytes() requires OsStrExt import which is platform-specific
- as_encoded_bytes() is the portable API available on all platforms
- Fixes CI failure on Linux (Ubuntu) in GitHub Actions
bytes_pending was an opaque aggregate of NonTried + NonTrimmed +
NonScraped. UIs that wanted a "will retry in Pass 2-N" bucket were
stuck showing the entire unread disc as Maybe at pct=0.
Adds two granular fields to MapStats:
bytes_nontried — Pass 1 hasn't read these yet
bytes_retryable — NonTrimmed + NonScraped, Pass 2-N will retry
bytes_pending stays for back-compat (= bytes_nontried + bytes_retryable).
Also picks up the cargo fmt --check lint that's been red on main CI
since v0.13.18 (rustfmt fold differences on a few long format-string
layouts; functional no-op).
Through the entire 0.13.x line, every CHECK CONDITION reply from the
drive (the standard way SCSI tells you why a sector failed) was being
collapsed into a synthetic status=0xFF, sense_key=0 transport-wedge
sentinel and the actual sense data was thrown away. Confirmed live on
the BU40N reading Dune 2 on 2026-04-27: drive returned host_status=0,
driver_status=8, status=2, exec_elapsed_ms=1416 on every bad sector
— a clean CHECK CONDITION carrying full sense data — and Disc::copy
was bailing on it as if the bridge had wedged.
Root cause: scsi/linux.rs's wedge check was
`host_status != 0 || driver_status != 0`
SG's DRIVER_SENSE bit (0x08) is set on every CHECK CONDITION reply
just to flag "sense buffer is populated" — it's not a transport
failure on its own. Pre-fix we conflated the two and silently lost
every drive-reported error reason. macOS and Windows backends had
the same shape: they extracted sense_key only, dropping ASC/ASCQ.
API restructure (clean separation):
Error::ScsiError {
opcode: u8,
status: u8, // 0xFF = synthetic transport-failure
sense: Option<ScsiSense>, // None ⇔ no sense delivered
}
pub struct ScsiSense { sense_key: u8, asc: u8, ascq: u8 }
impl ScsiSense {
pub fn is_marginal(&self) -> bool // keys 0/1/3/B
pub fn is_medium_error(&self) -> bool
pub fn is_hardware_error(&self) -> bool
pub fn is_unit_attention(&self) -> bool
pub fn is_data_protect(&self) -> bool
pub fn is_not_ready(&self) -> bool
pub fn is_illegal_request(&self) -> bool
pub fn is_aborted_command(&self) -> bool
}
impl Error {
pub fn scsi_sense(&self) -> Option<&ScsiSense>
pub fn is_scsi_transport_failure(&self) -> bool
pub fn is_marginal_read(&self) -> bool
}
SCSI protocol constants (SCSI_STATUS_*, SENSE_KEY_*) moved from
error.rs to scsi/mod.rs where they belong alongside SCSI_INQUIRY,
SCSI_READ_10, etc. parse_sense replaces parse_sense_key (returns the
full triple, not just the key); inline tests now exercise ASC/ASCQ
extraction at the right offsets for both descriptor (0x72/0x73) and
fixed (0x70/0x71) sense formats.
Disc::copy + Disc::patch sense-aware dispatch:
- marginal sense (MEDIUM ERROR / ABORTED COMMAND / RECOVERED ERROR
/ NO SENSE) → engage hysteresis (Block→Single, bpt=1)
- non-marginal sense (HARDWARE / DATA PROTECT / UNIT ATTENTION /
NOT READY / ILLEGAL REQUEST / transport failure / kernel
IoError) → bail with full sense info preserved; caller (autorip)
surfaces "physical replug" / "drive failing" / "media changed"
Pre-fix: every CHECK CONDITION → 0xFF synthetic → Disc::copy bailed
→ bytes_good froze at the bad zone. The hysteresis from v0.13.22
was correct but never got to run. This release unblocks it.
Disc::patch's wedged_threshold (50 consecutive failures) stays as
defense-in-depth for chains of marginal failures; a single
non-marginal sense now short-circuits it.
New phase=bail trace event records the bail reason with the sense
triple. phase=transport_err remains for genuine bridge wedges /
kernel timeouts; phase=scsi_err carries the parsed sense_key, asc,
ascq for drive-reported errors.
All 350 tests pass. Clippy clean across all targets.
Fixes the BU40N wedge cycle that has been chasing us through
v0.13.18-20. Two changes, both backed by empirical live-hardware
probes recorded in (internal)/docs/TEST_PLAN.md:
1. scsi/mod.rs: READ_TIMEOUT_MS 1500 → 10000 ms.
Cold-start seek on the BU40N takes ~1.5 s. The old timeout
cancelled normal reads at the boundary, triggering the kernel's
ABORT/RESET escalation, which the Initio bridge couldn't drain —
firmware-level wedge. 10 s catches every legitimate slow read
(max successful ECC recovery: 2.6 s; cold-start: 1.5 s) with
margin and short-circuits truly bad sectors at ~10 s.
2. disc/mod.rs: Disc::copy bisect-on-fail (replaces skip-forward).
Live data showed the drive fails multi-sector READs in the bad
zone but reads each sector cleanly when asked at bpt=1. Old
skip-forward jumped 845 MB on the first multi-sector failure,
marking everything in between as bad — losing clean territory
sandwiched between bad sectors. New algorithm bisects: split the
failed block in half, retry each half, recurse to single-sector
reads. Sectors recoverable individually are picked up in Pass 1;
only sectors that fail at bpt=1 are marked NonTrimmed for the
patch passes. Stack-based DFS, log2(batch) = 6 levels for the
default 60-sector batch.
Multi-pass machinery is untouched. Pass 2..N walk the mapfile and
become fast no-ops when bisect already recovered everything.
Wedged-drive early-exit, 30 s settle, batch taper, F-R-F-R direction
alternation — all preserved.
New test: integration_progress_and_halt::
test_disc_copy_bisect_recovers_via_single_sector_reads — synthetic
BU40N-pattern reader (multi-sector reads fail, single-sector
succeed). Pre-patch: lost everything to skip-forward. Post-patch:
100 % bytes_good. Plus the 10 sense-key parser tests from the
0.13.20 test-coverage pass.
Empirical recovery on Dune 2 UHD on the BU40N (per TEST_PLAN.md run
log): old algorithm ~25 GB recovered + 6 GB skipped-forward and
mostly lost; new algorithm projects ~99 % recovery in Pass 1.
Audits + raw probe data:
- (internal)/docs/TEST_PLAN.md (run log)
- (internal)/docs/audits/2026-04-26-scsi-architecture-research.md
- scsi/linux.rs: full rewrite from async write/poll/read+1.5s timeout+
close-on-timeout to one synchronous ioctl(fd, SG_IO, &hdr). Kernel
honors hdr.timeout and runs its own ABORT/RESET escalation. Errors
check host_status and driver_status (both 0xFF-synthesised) plus
status. Sense-key parser handles descriptor (0x72/0x73) + fixed
(0x70/0x71) formats. Deleted fd_recovery, bg close+open thread, fd
swap dance. -331/+155 lines.
- scsi/macos.rs: try_recover() removed (userspace handle-recovery on
task failure was the same anti-pattern stripped from Linux). bsd_name
field deleted. Errors bubble up directly.
- scsi/windows.rs: try_recover() removed, wide_path field deleted,
INVALID_HANDLE guard removed.
- scsi/mod.rs: parse_sense_key() helper extracted (used by all three
platforms now — single canonical sense-key parse rather than three
inlined copies). +10 unit tests covering descriptor format, fixed
format, truncated buffers, unknown response codes.
- drive/mod.rs: Drive::reset() deleted (escalating eject + STOP/START +
reinit recovery — per audit, kernel handles its own escalation;
userspace shouldn't).
pub fn find_drives() -> Vec<Drive> deleted (opened N drives just to
throw most away). find_drive() now uses discover_drives() directly.
wait_ready() simplified — drops the reset path on sense_key=5,
just keeps polling TUR for 60 iterations.
- lib.rs: find_drives re-export removed.
- benches/sgio_read.rs: switched to find_drive() (no longer iterates a
drive list).
Net: 9 files changed, 226 insertions(+), 473 deletions(-). 329 tests
pass, clippy -D warnings clean. No consumer breakage (CLI, autorip,
bdemu compile + test green).
Architecture decision documented in
(internal)/docs/audits/2026-04-26-scsi-architecture-research.md
(primary-source survey of MakeMKV, sg_dd, ddrescue, and the kernel
mid-layer's own scsi_eh.rst escalation ladder).
v0.13.12 shipped the async fd_recovery design but a live test on Dune 2
showed Pass 1 sat for 14 minutes with bytes_good=0 — the inner loop iterates
(throttled on_progress log fires every 78s) but each iteration evidently
takes ~60s instead of the microseconds the design promises on fast-fail.
Without trace-level telemetry at the SCSI + Disc::copy boundaries we
can't tell where the time goes.
This release is instrumentation only — no behavior change.
- New dep: tracing 0.1. Per project docs, debug/trace logging is allowed in
libfreemkv (the no-English rule applies to errors). Consumers wire a
tracing subscriber.
- SgIoTransport::execute (Linux): trace at every state transition (entry,
recovery_swap_ok, recovery_pending, write_ok / write_err, poll_done,
timeout_spawn_recovery, scsi_err, read_err, ok). Each event includes
opcode + elapsed timing. The bg recovery thread also traces close_ms +
open_ms so we can see if the kernel really takes 60s to close+open on a
wedged Initio bridge.
- Disc::copy: trace at copy_start, outer_loop, region_enter, every 100
inner-loop iterations (iter_progress with pos/region_end/skip_size/
bytes_good/read_ok_count/read_err_count/last_read_ms/copy_elapsed_ms),
copy_done.
- All trace events use targets `freemkv::scsi` and `freemkv::disc` so
consumers can filter by subsystem (e.g. autorip /api/debug?q=freemkv::scsi).
Next: run the live test on Dune 2 again, read the autorip JSONL log,
diagnose why each iter is slow, fix the actual bug.
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.
v0.13.10's 'fd=-1 on first poll timeout' was too aggressive: a single
transient killed the entire transport, Pass 1 finished in 45ms with
0 GB good on Dune 2.
Revert to spawn-close + main-thread-reopen (the v0.13.5/8 pattern).
Per-timeout cost is up to ~60s while the kernel completes the
abandoned command, but the v0.13.9 Disc::copy stall guard caps
catastrophic stalls at 120s of bytes_good non-advance. Pass 1 bails
cleanly with NonTrimmed ranges; Pass 2 has a working Drive for
retries with recovery=true + 30s timeouts.
Fixes the silent Pass 1 hang observed on Dune 2 with v0.13.8 (drive
grinding through bad sectors at 0 KB/s, errs=0, no error surfaced).
Root cause: SgIoTransport::execute's reopen-after-poll-timeout opened
a fresh /dev/sg* fd on the main thread, which serialized against the
spawned close() of the old fd via the kernel's per-device state lock.
The userspace 1.5s timeout still fired, but the abandon-and-reopen
recovery itself blocked the main thread for as long as close() took.
Net: reads returned slowly, skip-forward fired on every iteration,
bytes_good never advanced.
- SgIoTransport::execute: on poll timeout, spawn close, set fd=-1,
return Err. No reopen on the main thread. The transport is now
invalidated until the consumer creates a fresh Drive.
- Disc::copy: add stall guard. CopyOptions.stall_secs (default 120s).
If bytes_good doesn't advance for the threshold, break 'outer
cleanly with complete=false, bytes_pending > 0 so Pass 2 retries
pick up the NonTrimmed ranges with recovery=true 30s timeouts.
- New regression test: test_disc_copy_stall_detection_triggers_
skip_forward in tests/integration_progress_and_halt.rs.
Drive::read is now single-shot. Phase 1/2/3 retries + scsi::reset+reopen
removed (~80 lines). recovery=true bumps timeout to 30s; recovery=false
stays at 1.5s. On any failure returns Err(DiscRead) immediately — caller
(Disc::patch outer loop, DiscStream batch halver) handles retries.
Inline reset+reopen WAS the wedge primitive on the LG BU40N. Per prior
post-mortem, every USB/SCSI reset path tested fails to recover the
wedged Initio bridge — the inline retry was pure cost.
SgIoTransport::reset (Linux) trimmed to kernel SG_IO state flush +
ALLOW MEDIUM REMOVAL. SG_SCSI_RESET ioctl + STOP/START UNIT escalation
removed. macOS reset removed (no-op). scsi::reset() top-level family
removed (no callers).
EventKind::BytesRead { bytes, total } now actually emitted from
DiscStream::fill_extents after each successful sector read. Was
declared in 0.13.0, never fired. Drives autorip per-device progress
in direct mode.
EventKind::Retry / SectorRecovered no longer emitted (variants kept
for forward compat). SpeedChange still emitted via Drive::set_speed
public path.
Tests: new tests/integration_progress_and_halt.rs (5 tests). 233 unit
tests + 5 integration green.
USB/SCSI recovery escalation in drive_has_disc (0.13.1-0.13.3) tested
on LG BU40N USB BD-RE: USBDEVFS_RESET, authorized toggle, driver
unbind/rebind, SCSI host rescan — all succeed at the USB transport
layer but the drive firmware below the bridge stays locked. Only
physical unplug-replug clears it. Rolled back so consumers can
surface the real failure to the user.
New: list_drives falls back to sysfs-cached vendor/model/rev from
/sys/class/scsi_generic/sgN/device/ when live INQUIRY returns empty,
so wedged drives still show their identity in UIs.
Removed: scsi::usb_reset, usb_reset_with_timeout, per-platform
usb_reset methods, recover_then_probe, is_wedge_signature. Breadcrumb
comment in scsi/linux.rs::drive_has_disc points at v0.13.3 tag for
the full implementation if future hardware needs it back.
Linux/macOS/Windows pass-through symmetric; 233 tests passing.
Matthew Jackson