diff --git a/crates/pf-capture/src/windows/idd_push.rs b/crates/pf-capture/src/windows/idd_push.rs index 3ab04d22..ed1d674f 100644 --- a/crates/pf-capture/src/windows/idd_push.rs +++ b/crates/pf-capture/src/windows/idd_push.rs @@ -547,7 +547,8 @@ pub struct IddPushCapturer { _keepalive: Box, } // SAFETY: `IddPushCapturer` is `!Send` only because of its `*mut SharedHeader` raw pointer (and the -// COM interfaces / the broker's bare control `HANDLE`, which is process-global and never closed). It is +// COM interfaces; the frame/cursor delivery closures own `Arc` clones of the control device and are +// `Send + Sync` on their own). It is // created, used, and dropped by a SINGLE thread — the owning capture/encode thread — never shared: the // `ID3D11DeviceContext` is the device's IMMEDIATE context (single-threaded by D3D11 contract) and is // only ever touched from that thread, and the header pointer (into the mapping this struct owns) is diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index 313a9bb6..eaa84a17 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -299,16 +299,21 @@ struct Pinger { /// The manager's control-device cache. Reopenable: a driver upgrade / WUDFHost restart kills the /// cached handle (every IOCTL fails with a gone-class code forever), so such a failure RETIRES it and /// the next [`VirtualDisplayManager::ensure_device`] reopens the (new) device interface, re-running -/// the version handshake. Retired handles are deliberately kept alive — never closed — for the -/// process lifetime: the pinger/linger threads and every capturer's `ChannelBroker` hold BARE -/// `HANDLE` copies whose soundness contract is "never closed"; a retired handle only ever FAILS -/// IOCTLs, which every holder already tolerates. Reopens are rare (a driver restart), so the retained -/// list is bounded in practice. +/// the version handshake. +/// +/// Ownership is `Arc` all the way out: every consumer — `acquire`'s IOCTL runs, the pinger/linger +/// threads, the capture layer's delivery closures — holds its OWN clone across its use, so retiring +/// here merely drops the manager's reference and the handle CLOSES when the last in-flight user +/// drains. That close is load-bearing, not housekeeping: an open control handle is exactly what +/// vetoes the PnP disable/restart the wake-from-sleep recovery leans on (field 2026-08-08 — every +/// reload REFUSED `Generic failure`; `reset-pf-vdisplay.ps1` stops the whole host service precisely +/// to get its handles closed, and Arc ownership buys the same release without dying). The previous +/// contract kept retired handles open for the process lifetime because bare `HANDLE` copies were +/// smuggled into threads and closures; those copies are gone, and nothing may rely on a dead +/// handle staying open again. #[derive(Default)] struct DeviceSlot { current: Option>, - /// Never dropped — see the type doc (bare-`HANDLE` holders rely on no-close). - retired: Vec>, /// `CLEAR_ALL` (crashed-host orphan reap) runs only on the FIRST open of the process; a reopen /// races sessions this process still considers live and must not raze them. opened_once: bool, @@ -397,11 +402,6 @@ pub fn vdm() -> &'static VirtualDisplayManager { .expect("VirtualDisplayManager used before a backend initialised it") } -/// The live pf-vdisplay control-device handle, for the IDD-push capturer's sealed-channel delivery -/// (`IOCTL_SET_FRAME_CHANNEL`). Safe to hand out as a bare `HANDLE`: cached handles are never closed -/// for the process lifetime — a dead one is RETIRED (kept alive, see [`DeviceSlot`]), so a stale copy -/// can only fail IOCTLs, never dangle. `None` before the first backend open — impossible for a -/// capturer, which only exists on a monitor the manager created. /// Can this host's pf-vdisplay driver run the v5 hardware-cursor channel? Reads the /// handshake-latched protocol version, opening the control device once if no session has /// opened it yet this service run (the same open every session performs anyway) — so the @@ -421,7 +421,13 @@ pub fn hw_cursor_capable() -> bool { m.driver_proto.load(Ordering::Relaxed) >= 5 } -pub fn control_device_handle() -> Option { +/// The live pf-vdisplay control device, for the IDD-push capturer's sealed-channel delivery +/// (`IOCTL_SET_FRAME_CHANNEL`) — an `Arc` clone the caller (and every closure it builds) holds for +/// as long as it may issue IOCTLs: the handle stays open while any holder lives and closes when the +/// last drains, which is what lets the wake-from-sleep recovery's PnP disable proceed once the +/// manager retires it (see [`DeviceSlot`]). `None` before the first backend open — impossible for a +/// capturer, which only exists on a monitor the manager created. +pub fn control_device_handle() -> Option> { VDM.get().and_then(VirtualDisplayManager::device_handle) } @@ -497,17 +503,28 @@ fn is_device_gone(e: &anyhow::Error) -> bool { GONE.contains(&w.code().0) } +/// The transient raw `HANDLE` view of an Arc-held control device, for the backend IOCTL surface. +/// Sound only while the `Arc` it borrows from is held — which the borrow makes structural: every +/// use site necessarily has the owning clone alive across the call, so a concurrent retire (which +/// now really closes the handle once its users drain — see [`DeviceSlot`]) can never close it +/// mid-IOCTL. +fn dev_raw(dev: &OwnedHandle) -> HANDLE { + HANDLE(dev.as_raw_handle()) +} + impl VirtualDisplayManager { pub(crate) fn backend_name(&self) -> &'static str { self.driver.name() } /// Open + cache the control device; REOPEN when a gone-classified failure retired the cached one - /// (driver upgrade / WUDFHost restart). The `device` mutex serializes racing opens. - fn ensure_device(&self) -> Result { + /// (driver upgrade / WUDFHost restart). The `device` mutex serializes racing opens. Returns an + /// `Arc` clone the caller holds across every IOCTL it derives from it — a concurrent retire then + /// drops only the manager's reference and closes nothing under the caller (see [`DeviceSlot`]). + fn ensure_device(&self) -> Result> { let mut slot = self.device.lock().unwrap(); if let Some(d) = &slot.current { - return Ok(HANDLE(d.as_raw_handle())); + return Ok(d.clone()); } let reap = !slot.opened_once; claim_instance()?; @@ -519,35 +536,33 @@ impl VirtualDisplayManager { slot.opened_once = true; self.watchdog_s.store(watchdog_s, Ordering::Relaxed); self.driver_proto.store(driver_proto, Ordering::Relaxed); - let raw = HANDLE(handle.as_raw_handle()); - slot.current = Some(Arc::new(handle)); + let dev = Arc::new(handle); + slot.current = Some(dev.clone()); if !reap { tracing::info!("virtual-display control device reopened (retired handle replaced)"); } - Ok(raw) + Ok(dev) } - /// The live control handle for the pinger/linger threads. `None` before the first acquire opened - /// it, or between a retire and the next reopen. - fn device_handle(&self) -> Option { - self.device - .lock() - .unwrap() - .current - .as_ref() - .map(|d| HANDLE(d.as_raw_handle())) + /// The live control device for the pinger/linger threads — an `Arc` clone the caller holds + /// across its IOCTLs. `None` before the first acquire opened it, or between a retire and the + /// next reopen. + fn device_handle(&self) -> Option> { + self.device.lock().unwrap().current.clone() } - /// Retire the cached control handle after a gone-classified IOCTL failure. The handle is retained - /// un-closed (see [`DeviceSlot`]); the next [`ensure_device`](Self::ensure_device) reopens the - /// (new) device interface and re-runs the version handshake. + /// Retire the cached control handle after a gone-classified IOCTL failure: drop the manager's + /// reference, so the handle CLOSES once the last in-flight user drains (see [`DeviceSlot`]) — + /// the release the wake-from-sleep recovery needs before it can cycle the adapter devnode. The + /// next [`ensure_device`](Self::ensure_device) reopens the (new) device interface and re-runs + /// the version handshake. fn invalidate_device(&self, why: &anyhow::Error) { let mut slot = self.device.lock().unwrap(); - if let Some(cur) = slot.current.take() { + if slot.current.take().is_some() { tracing::warn!( - "virtual-display control device retired — reopening on next use (cause: {why:#})" + "virtual-display control device retired — closes when its last user drains, \ + reopening on next use (cause: {why:#})" ); - slot.retired.push(cur); } } @@ -620,11 +635,11 @@ impl VirtualDisplayManager { old_target, "IDD-push reconnect — preempting the kept (lingering/pinned) monitor, recreating a fresh one" ); - // SAFETY: `teardown_removed` requires `dev` to be a valid control handle; `dev` is the - // value `ensure_device()` returned above (cached handles are never closed — a dead one - // is retired, kept alive; see `DeviceSlot`). `mon` was just removed from the map, so it + // SAFETY: `teardown_removed` requires `dev` to be a valid control handle; the `dev` + // Arc `ensure_device()` returned above is held across this call, so the handle stays + // open even against a concurrent retire. `mon` was just removed from the map, so it // is exclusively owned here — no aliasing. - unsafe { self.teardown_removed(dev, &mut inner, mon) }; + unsafe { self.teardown_removed(dev_raw(&dev), &mut inner, mon) }; // Let the OS finish the ASYNC monitor departure before the next ADD; a back-to-back // REMOVE→ADD races the teardown and the ADD IOCTL is rejected under reconnect churn. // Verified-state wait, ceiling = the old fixed 400 ms settle (latency plan P0.3). @@ -657,11 +672,11 @@ impl VirtualDisplayManager { wudf_pid = mon.wudf_pid, "virtual monitor's WUDFHost is gone — preempting the dead monitor, recreating" ); - // SAFETY: `teardown_removed` requires a valid control handle; `dev` is the value - // `ensure_device()` returned above (cached handles are never closed — a dead one is - // retired, kept alive; see `DeviceSlot`). `mon` was just removed from the map, so it + // SAFETY: `teardown_removed` requires a valid control handle; the `dev` Arc + // `ensure_device()` returned above is held across this call, so the handle stays + // open even against a concurrent retire. `mon` was just removed from the map, so it // is exclusively owned here — no aliasing. - unsafe { self.teardown_removed(dev, &mut inner, mon) }; + unsafe { self.teardown_removed(dev_raw(&dev), &mut inner, mon) }; // Same async-departure settle as the reconnect preempt above (verified wait, P0.3). let _ = wait_target_departed(old_target, Duration::from_millis(400)); } @@ -693,9 +708,10 @@ impl VirtualDisplayManager { else { unreachable!("just matched Active"); }; - // SAFETY: `dev` is the handle `ensure_device()` returned above; the CCD - // waits inside run under the held `state` lock (this fn's discipline). - match unsafe { self.resize_in_place(dev, mon, mode) } { + // SAFETY: the `dev` Arc `ensure_device()` returned above is held across + // this call (so the handle stays open); the CCD waits inside run under + // the held `state` lock (this fn's discipline). + match unsafe { self.resize_in_place(dev_raw(&dev), mon, mode) } { Ok(()) => { // Same join semantics as the re-arrival: +1 ref for the new // (build-then-drop overlap) lease; `gen` untouched, so the old @@ -734,10 +750,11 @@ impl VirtualDisplayManager { let Some(SlotState::Active { mon, refs }) = inner.slots.remove(&slot) else { unreachable!("just matched Active"); }; - // SAFETY: `dev` is the handle `ensure_device()` returned above; `re_add` touches the - // live topology under the held `state` lock. `mon` is owned here (removed from the map). + // SAFETY: the `dev` Arc `ensure_device()` returned above is held across this call + // (so the handle stays open); `re_add` touches the live topology under the held + // `state` lock. `mon` is owned here (removed from the map). let new_mon = match unsafe { - self.re_add(dev, &mut inner, slot, &mon, mode, client_hdr) + self.re_add(dev_raw(&dev), &mut inner, slot, &mon, mode, client_hdr) } { ReAdd::Arrived(m) => *m, ReAdd::RolledBack { @@ -815,11 +832,11 @@ impl VirtualDisplayManager { } // The slot is empty: create a fresh monitor for it. - // SAFETY: `create_monitor` requires `dev` to be a valid control handle; `dev` is the handle - // `ensure_device()` returned above (cached handles are never closed — a dead one is retired, - // kept alive; see `DeviceSlot`), and we hold the `state` lock. + // SAFETY: `create_monitor` requires `dev` to be a valid control handle; the `dev` Arc + // `ensure_device()` returned above is held across this call (so the handle stays open even + // against a concurrent retire), and we hold the `state` lock. let mon = match unsafe { - self.create_monitor(dev, mode, slot, client_hdr, hw_cursor, &mut inner) + self.create_monitor(dev_raw(&dev), mode, slot, client_hdr, hw_cursor, &mut inner) } { // The cached device died under us (driver upgrade / WUDFHost restart, detected only // now — e.g. the host sat idle past the pinger-less window). Retire it, reopen, and @@ -831,9 +848,18 @@ impl VirtualDisplayManager { tracing::info!( "virtual-display control device reopened — retrying the monitor create" ); - // SAFETY: as above — `dev` is the handle the reopening `ensure_device` just - // returned, and the `state` lock is still held. - unsafe { self.create_monitor(dev, mode, slot, client_hdr, hw_cursor, &mut inner)? } + // SAFETY: as above — the `dev` Arc the reopening `ensure_device` just returned is + // held across this call, and the `state` lock is still held. + unsafe { + self.create_monitor( + dev_raw(&dev), + mode, + slot, + client_hdr, + hw_cursor, + &mut inner, + )? + } } r => r?, }; @@ -887,13 +913,12 @@ impl VirtualDisplayManager { let mut warned = false; while !stop_t.load(Ordering::Relaxed) { if let Some(h) = vdm().device_handle() { - // SAFETY: `ping` requires `dev` to be a valid control handle. `h` is from - // `device_handle()` (the `Some` branch) — cached handles are NEVER closed for the - // process lifetime (a dead one is retired, kept alive; see `DeviceSlot`), so the - // handle stays valid for this call even if it was retired concurrently — at worst - // the IOCTL fails. The pinger thread only spins while the `&'static` manager - // singleton lives. - match unsafe { vdm().driver.ping(h) } { + // SAFETY: `ping` requires `dev` to be a valid control handle. The `h` Arc from + // `device_handle()` is held across this call, so the handle stays open even if + // it is retired concurrently — at worst the IOCTL fails (the retire drops only + // the manager's reference; see `DeviceSlot`). The pinger thread only spins + // while the `&'static` manager singleton lives. + match unsafe { vdm().driver.ping(dev_raw(&h)) } { Ok(()) => warned = false, Err(e) if is_device_gone(&e) => { // The device itself is gone (driver upgrade / WUDFHost restart) — pings @@ -1897,12 +1922,11 @@ impl VirtualDisplayManager { slot, "virtual-display: last session left (deliberate quit) — tearing down now, linger skipped" ); - // SAFETY: `teardown_removed` requires `dev` to be the live control handle; `dev` - // is the cached process-lifetime `OwnedHandle` from `device_handle()` (the `Some` - // checked above; cached handles are never closed — a dead one is retired, kept - // alive). `mon` was moved out of the map under the `state` lock, so it is - // exclusively owned here — no aliasing. - unsafe { self.teardown_removed(dev, &mut inner, mon) }; + // SAFETY: `teardown_removed` requires `dev` to be the live control handle; the + // `dev` Arc from `device_handle()` (the `Some` checked above) is held across + // this call, so the handle stays open. `mon` was moved out of the map under the + // `state` lock, so it is exclusively owned here — no aliasing. + unsafe { self.teardown_removed(dev_raw(&dev), &mut inner, mon) }; } None => { inner.slots.insert( @@ -1980,10 +2004,10 @@ impl VirtualDisplayManager { "IDD-push setup: force-preempting the stuck-Active prior monitor (its IddCx swap-chain is dead)" ); // SAFETY: `teardown_removed` requires `dev` to be the live control handle; - // `dev` is the cached process-lifetime `OwnedHandle` from `device_handle()` - // (the `Some` checked above). `mon` was moved out of the map under the - // `state` lock, so it is exclusively owned here — no aliasing. - unsafe { self.teardown_removed(dev, &mut inner, mon) }; + // the `dev` Arc from `device_handle()` (the `Some` checked above) is held + // across this call, so the handle stays open. `mon` was moved out of the + // map under the `state` lock, so it is exclusively owned here — no aliasing. + unsafe { self.teardown_removed(dev_raw(&dev), &mut inner, mon) }; // Let the OS finish the ASYNC departure before the next ADD (mirrors the // acquire() Lingering-preempt settle). thread::sleep(Duration::from_millis(400)); @@ -2051,11 +2075,12 @@ impl VirtualDisplayManager { // its session. Lock order stays state → device (teardown's invalidate // path), same as every other holder; the pinger takes only the device // lock — no inversion. - // SAFETY: `teardown_removed` requires a valid control handle; `dev` is - // from `self.device_handle()` (cached handles are never closed — a dead - // one is retired, kept alive; see `DeviceSlot`). `mon` was moved out of - // the map under the lock, so it is exclusively owned here. - unsafe { self.teardown_removed(dev, &mut g, mon) }; + // SAFETY: `teardown_removed` requires a valid control handle; the `dev` + // Arc from `self.device_handle()` is held across this call, so the + // handle stays open (a concurrent retire drops only the manager's + // reference; see `DeviceSlot`). `mon` was moved out of the map under + // the lock, so it is exclusively owned here. + unsafe { self.teardown_removed(dev_raw(&dev), &mut g, mon) }; } } }) @@ -2218,11 +2243,11 @@ impl VirtualDisplayManager { if let Some(SlotState::Lingering { mon, .. } | SlotState::Pinned { mon }) = inner.slots.remove(&k) { - // SAFETY: `teardown_removed` needs a live control handle; `dev` is from - // `device_handle()` (cached handles are never closed — a dead one is retired, kept - // alive; see `DeviceSlot`). `mon` was moved out of the map under the `state` lock, - // so it is exclusively owned here — no aliasing. - unsafe { self.teardown_removed(dev, &mut inner, mon) }; + // SAFETY: `teardown_removed` needs a live control handle; the `dev` Arc from + // `device_handle()` is held across this call, so the handle stays open (see + // `DeviceSlot`). `mon` was moved out of the map under the `state` lock, so it is + // exclusively owned here — no aliasing. + unsafe { self.teardown_removed(dev_raw(&dev), &mut inner, mon) }; released += 1; } } diff --git a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs index 9c5cf45a..e456a3b7 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs @@ -100,14 +100,27 @@ unsafe fn ioctl(h: HANDLE, code: u32, input: &[u8], output: &mut [u8]) -> Result /// `reset-pf-vdisplay.ps1` step 2 (proven on-box). Best-effort + idempotent: only NOT-present nodes /// (`Status != OK`) are removed, so the LIVE session's monitor (`Status OK`) is never touched; any /// failure is logged and swallowed. Returns the number removed. +/// +/// The outcome is logged UNCONDITIONALLY, as found + removed: the old script counted only removals +/// and the host spoke only when that count was positive, so a reap whose pnputil never launched and +/// a box with no ghosts produced byte-identical logs (silence) — the same vacuous-signal family as +/// the `status=OK` trap [`reload_vdisplay_adapter`] answers — while ghosts ratcheted toward the +/// wedge with every sleep cycle. fn reap_ghost_monitors() -> u32 { // Mirrors reset-pf-vdisplay.ps1 step 2. powershell is always present for the SYSTEM service; the // matched tokens ('OK', 'punktfunk', the InstanceId) are locale-invariant, so this is safe on a // non-English box (unlike a .ps1 *file* read in the machine codepage). + // + // pnputil is resolved by full path and `$LASTEXITCODE` pre-seeded to failure before every + // launch, exactly like the reload path below: a LocalSystem service's PATH need not include + // System32 (and a SYSTEM process must not trust PATH anyway — a planted `pnputil.exe` would run + // elevated), and the old bare-name call failed INVISIBLY there — `SilentlyContinue` swallowed + // the miss, no exit code was written, and the ghosts stayed to wedge `IOCTL_ADD` at 0x80070490. const REAP_PS: &str = "$ErrorActionPreference='SilentlyContinue'; \ - $g = Get-PnpDevice -Class Monitor | Where-Object { $_.Status -ne 'OK' -and $_.FriendlyName -match 'punktfunk' }; \ - $n = 0; foreach ($d in $g) { pnputil /remove-device $d.InstanceId *> $null; if ($LASTEXITCODE -eq 0) { $n++ } }; \ - Write-Output $n"; + $g = @(Get-PnpDevice -Class Monitor | Where-Object { $_.Status -ne 'OK' -and $_.FriendlyName -match 'punktfunk' }); \ + $pnp = ($env:SystemRoot + '\\System32\\pnputil.exe'); \ + $n = 0; foreach ($d in $g) { $LASTEXITCODE = 1; if (Test-Path $pnp) { & $pnp /remove-device $d.InstanceId *> $null }; if ($LASTEXITCODE -eq 0) { $n++ } }; \ + Write-Output ($g.Count.ToString() + ' ' + $n)"; // Resolve powershell by full path — the LocalSystem service's PATH is not guaranteed to include // System32 — with a bare-name fallback. let ps = std::env::var("SystemRoot") @@ -125,17 +138,29 @@ fn reap_ghost_monitors() -> u32 { .output() { Ok(o) => { - let n = String::from_utf8_lossy(&o.stdout) - .trim() - .parse::() - .unwrap_or(0); - if n > 0 { + let raw = String::from_utf8_lossy(&o.stdout); + let Some((found, removed)) = parse_reap_output(&raw) else { tracing::warn!( - reaped = n, + output = %raw.trim(), + "pf-vdisplay: ghost-monitor reap died before reporting — ghost nodes (if any) still pin IddCx monitor slots" + ); + return 0; + }; + if found == 0 { + tracing::info!("pf-vdisplay: no ghost (not-present) virtual-monitor nodes to reap"); + } else if removed < found { + tracing::warn!( + found, + removed, + "pf-vdisplay: ghost-monitor reap could NOT remove every ghost node — the leftovers keep pinning IddCx monitor slots toward the 0x80070490 wedge" + ); + } else { + tracing::warn!( + reaped = removed, "pf-vdisplay: reaped ghost (not-present) virtual-monitor nodes — IddCx slot-exhaustion prevention" ); } - n + removed } Err(e) => { tracing::warn!(error = %e, "pf-vdisplay: ghost-monitor reap could not spawn powershell"); @@ -144,6 +169,18 @@ fn reap_ghost_monitors() -> u32 { } } +/// Parse [`reap_ghost_monitors`]'s script output — `" "`. Split out to be testable +/// without a box, like [`classify_reload_output`]: the field failure this answers was a reap whose +/// outcome could not be decoded from the log at all, so the decoding is worth pinning down. `None` +/// = the script died before reporting (callers treat that as "removed nothing", loudly). +fn parse_reap_output(out: &str) -> Option<(u32, u32)> { + let mut it = out.split_whitespace().map(str::parse::); + match (it.next(), it.next()) { + (Some(Ok(found)), Some(Ok(removed))) => Some((found, removed)), + _ => None, + } +} + /// What an adapter-cycle attempt actually DID — deliberately NOT the devnode's PnP status afterwards. /// The old script reported that status, and a device it had failed to touch at all still reads `OK`, /// so a no-op cycle was indistinguishable from a real one in the log (field report 2026-08-02: a @@ -178,6 +215,14 @@ fn reload_vdisplay_adapter() -> AdapterCycle { // device description — locale-invariant). Same spawn shape as `reap_ghost_monitors` above; the // reported tokens are ours, so parsing them is locale-invariant too. // + // The selector prefers LIVE devnodes: `Get-PnpDevice` also lists not-present PHANTOMS (an + // upgrade/reinstall leftover), and the old `Select-Object -First 1` could hand every recovery + // attempt a phantom — whose disable AND restart both fail — while a live node sat unexamined. + // A phantom-only state gets its own truthful refusal: no reload lever can revive a devnode + // record whose device is GONE; only re-creating the node (reinstall) can. `Present` is the + // authoritative bit, with `Status -ne 'Unknown'` as the fallback should it read null; live + // `OK` nodes sort ahead of problem-state ones. + // // Every step that can fail is `-ErrorAction Stop` inside a `try` — the old script ran the whole // cycle under `SilentlyContinue` and then reported `(Get-PnpDevice …).Status`, which reports the // DEVICE, not the cycle: a disable that was refused left the device untouched, started, and @@ -188,10 +233,19 @@ fn reload_vdisplay_adapter() -> AdapterCycle { // let "never ran" read as "returned 0". Pre-seeding a failure means only a real exit 0 reports a // reload. pnputil is resolved by full path — a LocalSystem service's PATH need not include // System32. + // + // The REFUSED line carries the evidence a field log needs to tell the failure modes apart + // (2026-08-08: a woken box logged only `REFUSED Generic failure` — the WMI catch-all — leaving + // handle-veto vs phantom vs problem-state undecidable): how many devnodes matched and how many + // are live, the chosen node's PnP Status + ConfigManager problem code, and the pnputil + // /restart-device exit code the old script threw away (3010 = needs a reboot, which is its own + // diagnosis). const CYCLE_PS: &str = "$ErrorActionPreference='SilentlyContinue'; \ - $ad = Get-PnpDevice -Class Display | Where-Object { $_.FriendlyName -match 'punktfunk Virtual Display' } | Select-Object -First 1; \ - if (-not $ad) { Write-Output 'ABSENT'; exit }; \ - $id = $ad.InstanceId; $err = ''; \ + $all = @(Get-PnpDevice -Class Display | Where-Object { $_.FriendlyName -match 'punktfunk Virtual Display' }); \ + if ($all.Count -eq 0) { Write-Output 'ABSENT'; exit }; \ + $live = @($all | Where-Object { $_.Present -or $_.Status -ne 'Unknown' } | Sort-Object { $_.Status -ne 'OK' }); \ + if ($live.Count -eq 0) { Write-Output ('REFUSED only phantom (not-present) adapter devnodes remain (' + $all.Count + ') - the device node itself is gone and no reload can revive it; reinstalling the host re-creates it'); exit }; \ + $ad = $live[0]; $id = $ad.InstanceId; $err = ''; \ try { \ Disable-PnpDevice -InstanceId $id -Confirm:$false -ErrorAction Stop; Start-Sleep -Seconds 2; \ try { Enable-PnpDevice -InstanceId $id -Confirm:$false -ErrorAction Stop } \ @@ -201,9 +255,11 @@ fn reload_vdisplay_adapter() -> AdapterCycle { } catch { $err = ($_.Exception.Message -replace '\\s+', ' ') }; \ $pnp = ($env:SystemRoot + '\\System32\\pnputil.exe'); $LASTEXITCODE = 1; \ if (Test-Path $pnp) { & $pnp /restart-device $id *> $null }; \ - if ($LASTEXITCODE -eq 0) { Start-Sleep -Seconds 2; \ + $rx = $LASTEXITCODE; \ + if ($rx -eq 0) { Start-Sleep -Seconds 2; \ Write-Output ('RELOADED restart ' + (Get-PnpDevice -InstanceId $id).Status) } \ - else { Enable-PnpDevice -InstanceId $id -Confirm:$false; Write-Output ('REFUSED ' + $err) }"; + else { Enable-PnpDevice -InstanceId $id -Confirm:$false; \ + Write-Output ('REFUSED devnodes=' + $all.Count + ' live=' + $live.Count + ' status=' + $ad.Status + ' problem=' + $ad.ConfigManagerErrorCode + ' restart_exit=' + $rx + ' ' + $err) }"; let ps = std::env::var("SystemRoot") .map(|r| format!(r"{r}\System32\WindowsPowerShell\v1.0\powershell.exe")) .unwrap_or_else(|_| "powershell.exe".to_string()); @@ -1050,10 +1106,12 @@ const BRIEF_RETRY: Duration = Duration::from_secs(3); /// them rather than N interleaved ones — each of which tears down the stack the others are waiting /// on. The second caller through typically finds the interface already up and returns at once. /// -/// Taken ONLY by [`ensure_available`], which holds no manager lock, and released before the retire -/// hook below takes the manager's `device` mutex. That is what keeps the lock order one-way: -/// [`VdisplayDriver::open`] runs *inside* that same `device` mutex, so if it could also take this -/// lock the two orders would invert and deadlock. It cannot — it never reloads. +/// Taken ONLY by [`ensure_available`], which holds no manager lock. The lock order is one-way — +/// `RECOVERY` → `device`: the recovery's handle-release hooks (`invalidate_cached_device`, which +/// drops the manager's reference so the control handle can CLOSE before the PnP cycle) take the +/// `device` mutex while this is held. It must stay one-way: [`VdisplayDriver::open`] runs *inside* +/// that same `device` mutex, so if it could also take this lock the two orders would invert and +/// deadlock. It cannot — it never reloads. static RECOVERY: std::sync::Mutex<()> = std::sync::Mutex::new(()); /// [`is_available`], with self-heal — and with PATIENCE, which is the part that matters after a @@ -1069,10 +1127,11 @@ pub fn ensure_available() -> Result<()> { let _serialize = RECOVERY.lock().unwrap_or_else(|e| e.into_inner()); wait_for_interface(NOT_READY_GRACE, true) }; - // OUTSIDE the recovery lock, by the ordering contract on `RECOVERY`. A reload tore the driver - // stack down and back up, so any control handle a previous session cached is dead by - // construction — retire it while we know that for certain, rather than leaving the next session - // to discover it by having an IOCTL fail. No-op before any backend opened the device. + // A reload tore the driver stack down and back up, so any control handle cached MEANWHILE (a + // racing open during the arrival window) is dead by construction — retire it while we know + // that for certain, rather than leaving the next session to discover it by having an IOCTL + // fail. Usually a no-op now: the recovery path already released the manager's reference + // before the reload (the handle-drain that lets the PnP cycle proceed at all). if reloaded { super::manager::invalidate_cached_device( "the pf-vdisplay adapter was reloaded (hostless-zombie recovery)", @@ -1119,12 +1178,33 @@ fn wait_for_interface(not_ready_grace: Duration, reload: bool) -> (Result= ABSENT_SETTLE); if reload && !reloaded && (absent_long_enough || Instant::now() >= deadline) { + // The not-ready path reaches here without the absent-sighting release above — drop the + // manager's reference now for the same reason (idempotent: a second call is a no-op). + super::manager::invalidate_cached_device( + "adapter reload imminent — releasing the host's own device handle (open handles \ + veto the PnP cycle)", + ); match reload_vdisplay_adapter() { // No devnode at all — waiting cannot conjure a driver. Fail immediately rather than // burning the arrival window on a box that simply does not have it installed. @@ -1195,6 +1275,32 @@ mod tests { } } + /// A refusal must carry evidence, not just a verdict. The 2026-08-08 field log showed only + /// `REFUSED Generic failure` — the WMI catch-all — leaving handle-veto vs phantom vs + /// problem-state undecidable from the log. The enriched line's tokens (devnode counts, PnP + /// status, problem code, the pnputil restart exit code the old script discarded) must survive + /// decoding verbatim, and the phantom-only state must decode as a refusal too — a reload + /// cannot revive a devnode record whose device is gone. + #[test] + fn a_refusal_keeps_its_evidence() { + let why = match classify_reload_output( + "REFUSED devnodes=2 live=1 status=OK problem=0 restart_exit=3010 Generic failure", + ) { + AdapterCycle::Refused(why) => why, + other => panic!("expected Refused, got {}", variant(&other)), + }; + for token in ["devnodes=2", "live=1", "status=OK", "restart_exit=3010"] { + assert!(why.contains(token), "{token} must survive: {why:?}"); + } + assert!(matches!( + classify_reload_output( + "REFUSED only phantom (not-present) adapter devnodes remain (2) - the device node \ + itself is gone and no reload can revive it; reinstalling the host re-creates it" + ), + AdapterCycle::Refused(why) if why.contains("phantom") + )); + } + /// The outcomes callers branch on: `NotInstalled` fails a session fast, `Reloaded` earns the /// arrival window, and the lever that worked stays visible in the log (`restart` means the /// disable was refused and something still holds the device open). @@ -1226,6 +1332,29 @@ mod tests { )); } + /// The reap's outcome must decode losslessly — the field ratchet (0.23→0.25) was a reap whose + /// bare-named pnputil never launched under the LocalSystem PATH while the host stayed silent: + /// "no ghosts" and "removed nothing" were byte-identical. Found and removed now travel + /// separately so a leftover ghost is loud, and the old single-number output (or a powershell + /// that died before reporting) must not decode as anything. + #[test] + fn reap_output_decodes_found_and_removed() { + assert_eq!(parse_reap_output("3 3\r\n"), Some((3, 3))); + assert_eq!( + parse_reap_output("4 0"), + Some((4, 0)), + "pnputil unlaunchable" + ); + assert_eq!(parse_reap_output("0 0"), Some((0, 0)), "clean box"); + for dead in ["5", "", " ", "garbage", "OK"] { + assert_eq!( + parse_reap_output(dead), + None, + "{dead:?} is not a reap report" + ); + } + } + /// `is_absent` is what decides between WAITING and performing device surgery, so the two states /// it separates are pinned here. An interface that is registered but not yet ACTIVE is a devnode /// mid-transition — the wake-from-sleep case — and reloading the adapter under it only lengthens diff --git a/crates/punktfunk-host/src/capture.rs b/crates/punktfunk-host/src/capture.rs index d4226972..6b4662d9 100644 --- a/crates/punktfunk-host/src/capture.rs +++ b/crates/punktfunk-host/src/capture.rs @@ -194,27 +194,29 @@ pub fn capture_virtual_output( crate::inject::set_stream_target(Some(target.target_id)); let pref = vout.preferred_mode; let keep = vout.keepalive; - // The sealed-channel delivery seam: resolve the pf-vdisplay control device ONCE (it is - // process-global — a dead one is retired, kept alive — so the raw value is stable for the - // process) and wrap `send_frame_channel` in a `Send + Sync` closure the IDD-push capturer calls - // at ring attach. This is the ONE reach into `crate::vdisplay` the capturer would otherwise make; - // building it here keeps the capture→vdisplay dependency out of pf-capture (plan §W6). + // The sealed-channel delivery seam: resolve the pf-vdisplay control device ONCE and wrap + // `send_frame_channel` in a `Send + Sync` closure the IDD-push capturer calls at ring attach. + // This is the ONE reach into `crate::vdisplay` the capturer would otherwise make; building it + // here keeps the capture→vdisplay dependency out of pf-capture (plan §W6). let control = crate::vdisplay::manager::control_device_handle().ok_or_else(|| { anyhow::anyhow!( "pf-vdisplay control device not open (monitor not created via the manager?)" ) })?; - // `HANDLE` is not `Send`; capture the raw value and rebuild it inside the closure (the control - // device is never closed for the process lifetime, so the value stays valid). - let control_raw = control.0 as isize; + // Each closure keeps its own `Arc` clone (`Send + Sync`), so the handle is open + // for exactly as long as any delivery closure lives — and CLOSES once the manager retires it + // and the last session drops, which is what lets the wake-from-sleep recovery's PnP device + // cycle proceed (an open control handle vetoes it). + let control_frame = control.clone(); let sender: pf_capture::FrameChannelSender = std::sync::Arc::new( move |req: &pf_driver_proto::control::SetFrameChannelRequest| { - // SAFETY: `control_raw` is the pf-vdisplay control handle resolved above; it is never - // closed for the process lifetime, so reconstructing the `HANDLE` and issuing the - // `IOCTL_SET_FRAME_CHANNEL` is sound (`send_frame_channel`'s precondition). + // SAFETY: the captured `control_frame` Arc keeps the control handle open across this + // call — `send_frame_channel`'s precondition. unsafe { crate::vdisplay::driver::send_frame_channel( - windows::Win32::Foundation::HANDLE(control_raw as *mut core::ffi::c_void), + windows::Win32::Foundation::HANDLE( + std::os::windows::io::AsRawHandle::as_raw_handle(&*control_frame), + ), req, ) } @@ -231,14 +233,17 @@ pub fn capture_virtual_output( // Cursor-forward sessions (M2c): hand the capturer the v5 cursor-channel delivery closure — // its presence opts the session in (the capturer creates + delivers the CursorShm section, // the driver declares the IddCx hardware cursor). Built exactly like `sender` above. + let control_cursor = control.clone(); let cursor_sender: Option = want.hw_cursor.then(|| { std::sync::Arc::new( move |req: &pf_driver_proto::control::SetCursorChannelRequest| { - // SAFETY: `control_raw` is the pf-vdisplay control handle resolved above; it is - // never closed for the process lifetime (`send_cursor_channel`'s precondition). + // SAFETY: the captured `control_cursor` Arc keeps the control handle open across + // this call (`send_cursor_channel`'s precondition). unsafe { crate::vdisplay::driver::send_cursor_channel( - windows::Win32::Foundation::HANDLE(control_raw as *mut core::ffi::c_void), + windows::Win32::Foundation::HANDLE( + std::os::windows::io::AsRawHandle::as_raw_handle(&*control_cursor), + ), req, ) } @@ -261,11 +266,13 @@ pub fn capture_virtual_output( target_id, enable: enable as u32, }; - // SAFETY: `control_raw` is the pf-vdisplay control handle resolved above; it is - // never closed for the process lifetime (`send_cursor_forward`'s precondition). + // SAFETY: the captured `control` Arc keeps the control handle open across this call + // (`send_cursor_forward`'s precondition). unsafe { crate::vdisplay::driver::send_cursor_forward( - windows::Win32::Foundation::HANDLE(control_raw as *mut core::ffi::c_void), + windows::Win32::Foundation::HANDLE( + std::os::windows::io::AsRawHandle::as_raw_handle(&*control), + ), &req, )?; }