From 93ad1f4854cf4037c31c95515f7eafae111bae75 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 5 Aug 2026 21:20:29 -0700 Subject: [PATCH] Give Windows a free-space gate, and stop two tests timing the scheduler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three release-profile failures on macOS and Windows, all found by the qa gate on its first run. Release-profile tests on those platforms had never run before it existed, so none of these were regressions — they had simply never been visible. available_space returned None on every non-unix target. That did not merely skip a test, it skipped the GATE: a Windows user extracting a disc to a full volume got a confusing failure part-way through instead of a clear refusal up front, and Windows is where the GUI ships. GetDiskFreeSpaceExW is declared directly against kernel32, matching how scsi::windows already reaches Win32 rather than pulling in a binding crate for one call. It asks for FreeBytesAvailableToCaller, which accounts for per-user quotas — the same question f_bavail answers on unix. The other two asserted on wall-clock timing with no margin: - sleep_until_halted_wakes_mid_sleep bounded the wait at 350 ms and measured 377 ms on a loaded runner. That bound measures the scheduler, not the wake. What the test is for is distinguishing "woke because the flag flipped" from "woke because the 10 s timeout expired", and 2 s does that just as well. - abandon_loses_to_a_close_already_committed released at 600 ms against two 300 ms grace windows plus a 250 ms poll cadence, so the windows could expire first and the caller abandoned — a race, not a defect. The intervals are scaled up so jitter is small relative to them; the ordering under test is unchanged, only the margin. --- src/disc/extract.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++- src/drive/mod.rs | 14 +++++++++++-- src/io/pipeline.rs | 16 +++++++++++++-- 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/disc/extract.rs b/src/disc/extract.rs index 53877f0..ff75a26 100644 --- a/src/disc/extract.rs +++ b/src/disc/extract.rs @@ -684,7 +684,53 @@ fn available_space(dir: &Path) -> Option { Some(avail) } -#[cfg(not(unix))] +/// Windows has no `statvfs`. This returned `None` unconditionally, which does +/// not merely skip a test — it skipped the free-space GATE, so a Windows user +/// extracting a disc to a full volume got a confusing failure part-way through +/// instead of a clear refusal up front. libfreemkv ships a Windows GUI, so that +/// is the platform where the friendly error matters most. +/// +/// `GetDiskFreeSpaceExW` is declared directly against kernel32, matching how +/// `scsi::windows` already reaches the Win32 API rather than pulling in a +/// binding crate for one call. +#[cfg(windows)] +fn available_space(dir: &Path) -> Option { + use std::os::windows::ffi::OsStrExt; + + unsafe extern "system" { + fn GetDiskFreeSpaceExW( + lpDirectoryName: *const u16, + lpFreeBytesAvailableToCaller: *mut u64, + lpTotalNumberOfBytes: *mut u64, + lpTotalNumberOfFreeBytes: *mut u64, + ) -> i32; + } + + // Wide, NUL-terminated. An interior NUL cannot reach the API, so reject it + // rather than silently truncating the path and measuring the wrong volume. + let mut wide: Vec = dir.as_os_str().encode_wide().collect(); + if wide.contains(&0) { + return None; + } + wide.push(0); + + let mut avail: u64 = 0; + // FreeBytesAvailableToCaller, not TotalNumberOfFreeBytes: it accounts for + // per-user quotas, which is what "can I actually write this much" means and + // what `statvfs`'s `f_bavail` gives on the unix side. + let ok = unsafe { + GetDiskFreeSpaceExW( + wide.as_ptr(), + &mut avail, + std::ptr::null_mut(), + std::ptr::null_mut(), + ) + }; + if ok == 0 { None } else { Some(avail) } +} + +/// Neither unix nor Windows: no way to ask, so the gate is skipped. +#[cfg(not(any(unix, windows)))] fn available_space(_dir: &Path) -> Option { None } diff --git a/src/drive/mod.rs b/src/drive/mod.rs index a11183c..926cf4c 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -1406,8 +1406,18 @@ mod halt_tests { let r = sleep_until_halted(&flag, Duration::from_secs(10)); assert!(matches!(r, Err(Error::Halted))); let waited = t0.elapsed(); - // Flag flipped at ~150 ms; we wake within one 100 ms slice → <300 ms. - assert!(waited < Duration::from_millis(350), "waited {waited:?}"); + // What this test is for: the sleep must end because the flag flipped, + // NOT because the 10 s timeout expired. Anything comfortably under + // that proves it, and the lower bound proves it did not return early + // for some other reason. + // + // The upper bound used to be 350 ms — flag at ~150 ms plus one 100 ms + // poll slice, with a little slack. That measures the SCHEDULER, not + // this function: a loaded CI runner took 377 ms and failed, which says + // nothing about whether the wake worked. Bound it well below the + // timeout instead, so the assertion still distinguishes the two + // outcomes it exists to distinguish. + assert!(waited < Duration::from_secs(2), "waited {waited:?}"); assert!(waited >= Duration::from_millis(140), "waited {waited:?}"); } diff --git a/src/io/pipeline.rs b/src/io/pipeline.rs index 4310a0c..c28060b 100644 --- a/src/io/pipeline.rs +++ b/src/io/pipeline.rs @@ -1839,11 +1839,23 @@ mod tests { thread::spawn(move || { // Past the first grace window (and past the 250 ms poll cadence that // bounds when the window is actually observed), inside the second. - thread::sleep(Duration::from_millis(600)); + // + // These intervals used to be 600 ms against a 300 ms grace, which + // left NO margin: two 300 ms windows end at 600 ms, and the 250 ms + // poll cadence can push the observation later still, so on a loaded + // runner the second window expired first and the caller abandoned — + // failing with Err(Halted) against a race, not a defect. + // + // Scaled up so the jitter is small relative to the intervals: the + // first window ends at ~1.0-1.25 s and the second at ~2.0-2.25 s, + // so releasing at 1.6 s sits well inside the second with roughly + // 350 ms of slack on either side. The ordering under test is + // unchanged; only the margin is. + thread::sleep(Duration::from_millis(1600)); rel.store(true, Ordering::SeqCst); }); - let grace = Duration::from_millis(300); + let grace = Duration::from_secs(1); let res = finish_with_grace(handle, &state, grace, Error::Halted); assert!( matches!(res, Ok(42)),