fix(vdisplay/windows): a failed resize left a phantom monitor, and a refresh-only one silently didn't happen

Three defects in the mid-stream resize path.

**A failed re-arrival put a DEAD monitor back in the slot.** `re_add` REMOVEs the old
driver monitor before it ADDs the new one, so its only fallible step past that point
means the old monitor is gone — yet the caller answered an `Err` by re-inserting the
`Monitor` struct it had handed in, complete with a dead `key`, a departed `target_id`
and a stale `gdi_name`. Every later `acquire` then joined a monitor that did not exist.

It now rolls back: on ADD failure it re-ADDs at the OLD mode, so the session keeps
streaming what it already had. The outcome is a three-way `ReAdd` enum because the
distinction matters and a `Result` cannot carry it — `Arrived` stores the new monitor,
`RolledBack` stores the RECOVERED one (a re-arrival mints a fresh `target_id`, so the
struct the caller handed in is stale either way), and `Lost` means both ADDs failed and
the slot must be left EMPTY so the next acquire creates a monitor rather than joining a
phantom.

**A refresh-only change reported success at the old refresh.** `wait_mode_advertised`
compared resolution only, so a same-WxH new-Hz request answered "already advertised":
the fast path skipped the `IOCTL_UPDATE_MODES` that would have taught the driver the new
rate, `set_active_mode` fell back to the best advertised rate <= requested, and the
resize returned Ok. It matches `dmDisplayFrequency` now, so an unadvertised refresh
routes to UPDATE_MODES / the re-arrival instead.

And `mon.mode = mode` recorded what was ASKED FOR. `set_active_mode` deliberately falls
back to a lower advertised refresh rather than lose the client's resolution, so the
field could claim a rate the display was not running — and it is what the next resize
diffs against and what `/display/state` reports. It records what actually committed now,
read back via the new `active_mode` (`active_resolution` delegates to it), and logs when
the two differ. Deliberately NOT done by tightening `wait_mode_settled`: that gates the
re-arrival fallback, so requiring an exact refresh there would force a full re-arrival
every time the OS legitimately picked a lower rate.

**A short IOCTL_ADD reply leaked an IddCx slot.** The IOCTL had SUCCEEDED — the driver
already created the monitor and took a slot; only the reply was short — and the bail
undid none of it. The slot pool is small enough that ~16 leaks wedge every later ADD at
0x80070490, which is the wedge the ghost-reap in this same function exists to recover
from. It now issues the compensating REMOVE with the session id already in scope, and
says so at error level if that fails too.

Windows runner: clippy --all-targets clean, pf-vdisplay 55 passed, pf-capture 18 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

crates/pf-vdisplay/src/vdisplay/windows/manager.rs

@@ -165,6 +165,27 @@ struct GroupState {
     ccd_exclusive: bool,
 }
 
+/// How a mid-stream re-arrival ([`ManagerInner::re_add`]) ended.
+///
+/// Three-way on purpose. `re_add` REMOVEs the old driver monitor before it ADDs the new one, so
+/// once the ADD fails the old monitor is GONE — and the caller used to answer that by putting its
+/// `Monitor` struct back in the slot, complete with a dead `key`, a departed `target_id` and a
+/// stale `gdi_name`. Every later `acquire` then joined a monitor that did not exist.
+enum ReAdd {
+    /// Re-arrived at the requested mode.
+    Arrived(Box<Monitor>),
+    /// The ADD failed and the OLD mode was re-ADDed instead, so the session keeps streaming what it
+    /// already had. The monitor is REAL but new — a re-arrival mints a fresh `target_id` — which is
+    /// why the caller must store THIS one and not the struct it handed in.
+    RolledBack {
+        mon: Box<Monitor>,
+        err: anyhow::Error,
+    },
+    /// The ADD failed and so did the rollback: this slot has no driver monitor at all. The caller
+    /// must leave it EMPTY so the next `acquire` creates one, rather than joining a phantom.
+    Lost(anyhow::Error),
+}
+
 /// What a NON-LAST-member teardown owes the group's topology.
 ///
 /// Split out of [`ManagerInner::teardown_removed`] so the gate is testable without a driver, a CCD
@@ -665,18 +686,35 @@ impl VirtualDisplayManager {
                 };
                 // 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).
-                let new_mon =
-                    match unsafe { self.re_add(dev, &mut inner, slot, &mon, mode, client_hdr) } {
-                        Ok(m) => m,
-                        Err(e) => {
-                            // The re-arrival failed — put the OLD monitor back so the session keeps
-                            // streaming its current mode (the control task already acked the switch; the
-                            // rebuild reuses the old target and Fix 2's corrective ack tells the client the
-                            // resolution didn't change). Its `gen`/`refs` are intact, so leases stay valid.
-                            inner.slots.insert(slot, SlotState::Active { mon, refs });
-                            return Err(e).context("mid-stream resize re-arrival");
-                        }
-                    };
+                let new_mon = match unsafe {
+                    self.re_add(dev, &mut inner, slot, &mon, mode, client_hdr)
+                } {
+                    ReAdd::Arrived(m) => *m,
+                    ReAdd::RolledBack {
+                        mon: recovered,
+                        err,
+                    } => {
+                        // The session keeps streaming its current mode (the control task already
+                        // acked the switch; Fix 2's corrective ack tells the client the resolution
+                        // did not change). Store the RECOVERED monitor, not the one handed in:
+                        // that one's driver monitor was REMOVEd before the failed ADD, so its
+                        // `key`/`target_id`/`gdi_name` are all dead. `gen`/`refs` are preserved,
+                        // so leases stay valid either way.
+                        inner.slots.insert(
+                            slot,
+                            SlotState::Active {
+                                mon: *recovered,
+                                refs,
+                            },
+                        );
+                        return Err(err).context("mid-stream resize re-arrival");
+                    }
+                    ReAdd::Lost(err) => {
+                        // No driver monitor for this slot. Leave it EMPTY so the next acquire
+                        // does a clean fresh ADD rather than joining a phantom.
+                        return Err(err).context("mid-stream resize re-arrival (slot left empty)");
+                    }
+                };
                 // `re_add` preserved `gen`, so both the old session's lease and this new one match on
                 // release. +1 ref for the new (build-then-drop overlap) lease.
                 let out = self.output_for(slot, &new_mon, quit);
@@ -1380,12 +1418,36 @@ impl VirtualDisplayManager {
                 "in-place mode set did not commit within 1.5s (advertised after {advertised_ms} ms)"
             );
         }
+        // Record what actually COMMITTED, not what was asked for. `set_active_mode` deliberately
+        // falls back to the highest advertised refresh <= requested rather than lose the client's
+        // resolution, so `mon.mode = mode` claimed a rate the display might not be running — and
+        // `mon.mode` is what the next resize diffs against and what `/display/state` reports.
+        let committed = pf_win_display::win_display::active_mode(mon.target_id);
+        let landed = match committed {
+            Some((w, h, hz)) => Mode {
+                width: w,
+                height: h,
+                refresh_hz: hz,
+            },
+            // The settle above already verified the resolution; if the read-back races we still
+            // know the size took, so trust the request rather than leaving `mon.mode` stale.
+            None => mode,
+        };
+        if landed.refresh_hz != mode.refresh_hz {
+            tracing::info!(
+                requested_hz = mode.refresh_hz,
+                committed_hz = landed.refresh_hz,
+                "in-place resize: the OS committed a different refresh than requested (the driver \
+                 does not advertise it) — recording what it actually runs"
+            );
+        }
         tracing::info!(
             advertised_ms,
             settle_ms = settle_start.elapsed().as_millis() as u64,
+            mode = format!("{}x{}@{}", landed.width, landed.height, landed.refresh_hz),
             "in-place resize committed (verified-state wait)"
         );
-        mon.mode = mode;
+        mon.mode = landed;
         Ok(())
     }
 
@@ -1416,7 +1478,7 @@ impl VirtualDisplayManager {
         old: &Monitor,
         mode: Mode,
         client_hdr: Option<punktfunk_core::quic::HdrMeta>,
-    ) -> Result<Monitor> {
+    ) -> ReAdd {
         tracing::info!(
             slot,
             old = format!(
@@ -1454,10 +1516,45 @@ impl VirtualDisplayManager {
         let render_pin = resolve_render_pin();
         // SAFETY: `dev` is the live control handle; `render_pin`/`client_hdr` are owned `Copy`/`Option`
         // values passed by value — no borrow crosses the call.
-        let added = unsafe {
+        // SAFETY (both ADDs): `dev` is the live control handle; `render_pin`/`client_hdr` are owned
+        // `Copy`/`Option` values passed by value — no borrow crosses the call.
+        let (added, mode, rollback_err) = match unsafe {
             self.driver
                 .add_monitor(dev, mode, render_pin, slot, client_hdr, old.hw_cursor)
-                .context("re-arrival ADD at the new mode")?
+        } {
+            Ok(a) => (a, mode, None),
+            Err(e) => {
+                // The old monitor is already REMOVEd, so there is nothing to "keep". Re-ADD it at
+                // the mode it had: the resize fails, but the session keeps streaming instead of
+                // being handed a slot whose driver monitor does not exist.
+                let e = e.context("re-arrival ADD at the new mode");
+                tracing::warn!(
+                    slot,
+                    error = %format!("{e:#}"),
+                    "re-arrival ADD failed — rolling back to the previous mode"
+                );
+                // SAFETY: as the ADD above — live control handle, owned `Copy`/`Option` args.
+                match unsafe {
+                    self.driver.add_monitor(
+                        dev,
+                        old.mode,
+                        render_pin,
+                        slot,
+                        client_hdr,
+                        old.hw_cursor,
+                    )
+                } {
+                    Ok(a) => (a, old.mode, Some(e)),
+                    Err(e2) => {
+                        tracing::error!(
+                            slot,
+                            error = %format!("{e2:#}"),
+                            "re-arrival rollback ADD also failed — the slot now has NO monitor"
+                        );
+                        return ReAdd::Lost(e.context(format!("rollback also failed: {e2:#}")));
+                    }
+                }
+            }
         };
         self.ensure_pinger();
         // 3. Resolve the NEW target's GDI name (target_id changes across a re-arrival).
@@ -1494,7 +1591,7 @@ impl VirtualDisplayManager {
         }
         // 5. Rebuild the Monitor from the ADD reply, PRESERVING `gen` (lease/refcount continuity) and
         //    the group-layout `position`. A fresh `gen` would strand the old session's lease release.
-        Ok(Monitor {
+        let mon = Box::new(Monitor {
             key: added.key,
             target_id: added.target_id,
             luid: added.luid,
@@ -1509,7 +1606,11 @@ impl VirtualDisplayManager {
             // Fresh from THIS reply, not `old`: the driver's per-target declare registry is the
             // ground truth (this session may itself have declared since the original ADD).
             cursor_excluded: added.cursor_excluded,
-        })
+        });
+        match rollback_err {
+            None => ReAdd::Arrived(mon),
+            Some(err) => ReAdd::RolledBack { mon, err },
+        }
     }
 
     /// Re-isolate the composited display set after a mid-stream monitor re-arrival ([`re_add`]) put a

crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs

@@ -647,6 +647,37 @@ impl VdisplayDriver for PfVdisplayDriver {
         // the `cursor_excluded` tail; `out` is zero-initialized, so the missing tail reads `0`
         // (= unknown/clean — exactly what a driver that can't track declares should report).
         if (n as usize) < control::ADD_REPLY_LEGACY_SIZE {
+            // The IOCTL SUCCEEDED — the driver has already created the monitor and taken an IddCx
+            // slot; only its reply was short. Bailing without undoing that leaks both, and the slot
+            // pool is small enough that ~16 leaks wedge every later ADD at 0x80070490 (the wedge the
+            // ghost-reap above exists to recover from). Compensate with the REMOVE this session's id
+            // addresses, then fail.
+            let req = control::RemoveRequest { session_id };
+            let mut none: [u8; 0] = [];
+            // SAFETY: `dev` is the live control handle (`add_monitor`'s contract); `bytes_of(&req)`
+            // borrows a local alive across this synchronous call, and `none` is the empty output the
+            // IOCTL expects.
+            let undo = unsafe {
+                ioctl(
+                    dev,
+                    control::IOCTL_REMOVE,
+                    bytemuck::bytes_of(&req),
+                    &mut none,
+                )
+            };
+            match undo {
+                Ok(_) => tracing::warn!(
+                    session_id,
+                    "pf-vdisplay ADD returned a short reply — removed the monitor it had already \
+                     created so its IddCx slot is not leaked"
+                ),
+                Err(e) => tracing::error!(
+                    session_id,
+                    error = %format!("{e:#}"),
+                    "pf-vdisplay ADD returned a short reply AND the compensating REMOVE failed — \
+                     this monitor's IddCx slot is leaked until the driver is cycled"
+                ),
+            }
             anyhow::bail!(
                 "pf-vdisplay ADD returned {n} bytes, expected at least {}",
                 control::ADD_REPLY_LEGACY_SIZE

crates/pf-win-display/src/win_display.rs

@@ -285,6 +285,14 @@ pub fn resolve_gdi_name(target_id: u32) -> Option<String> {
 ///
 /// Safe to call from any thread.
 pub fn active_resolution(target_id: u32) -> Option<(u32, u32)> {
+    active_mode(target_id).map(|(w, h, _)| (w, h))
+}
+
+/// The target's CURRENT active mode as `(width, height, refresh_hz)` — what the OS actually
+/// committed, which is not always what was asked for: `set_active_mode` deliberately falls back to
+/// the highest advertised refresh <= requested rather than losing the client's resolution. Callers
+/// that RECORD a mode must record this, or they claim a refresh the display is not running.
+pub fn active_mode(target_id: u32) -> Option<(u32, u32, u32)> {
     let gdi = resolve_gdi_name(target_id)?;
     let wname: Vec<u16> = gdi.encode_utf16().chain(std::iter::once(0)).collect();
     let mut dm = DEVMODEW {
@@ -299,7 +307,7 @@ pub fn active_resolution(target_id: u32) -> Option<(u32, u32)> {
     if !ok || dm.dmPelsWidth == 0 || dm.dmPelsHeight == 0 {
         return None;
     }
-    Some((dm.dmPelsWidth, dm.dmPelsHeight))
+    Some((dm.dmPelsWidth, dm.dmPelsHeight, dm.dmDisplayFrequency))
 }
 
 /// Verified-state topology-settle wait (latency plan P0.2): poll the CCD state until the target is
@@ -399,7 +407,12 @@ pub fn advertised_resolutions(gdi_name: &str) -> Vec<(u32, u32)> {
 /// gate between a driver-side mode-list refresh (`IOCTL_UPDATE_MODES`, latency plan P2) and the
 /// CCD/GDI force-set: the OS re-evaluates an indirect display's settable modes asynchronously after
 /// `IddCxMonitorUpdateModes2`, so an immediate `set_active_mode` could race the re-enumeration and
-/// silently leave the old mode. Returns `true` once any refresh at the requested WxH is enumerable.
+/// silently leave the old mode. Returns `true` once the requested `WxH@Hz` is enumerable.
+///
+/// The REFRESH is part of the match. It used to compare resolution only, which made a refresh-only
+/// change (same WxH, new Hz) report "already advertised" — so the caller's fast path skipped the
+/// `IOCTL_UPDATE_MODES` that would have taught the driver the new rate, `set_active_mode` fell back
+/// to the best advertised rate <= requested, and the resize reported success at the OLD refresh.
 pub fn wait_mode_advertised(gdi_name: &str, mode: Mode, ceiling: std::time::Duration) -> bool {
     let wname: Vec<u16> = gdi_name.encode_utf16().chain(std::iter::once(0)).collect();
     let deadline = std::time::Instant::now() + ceiling;
@@ -424,7 +437,10 @@ pub fn wait_mode_advertised(gdi_name: &str, mode: Mode, ceiling: std::time::Dura
             if !ok {
                 break;
             }
-            if dm.dmPelsWidth == mode.width && dm.dmPelsHeight == mode.height {
+            if dm.dmPelsWidth == mode.width
+                && dm.dmPelsHeight == mode.height
+                && dm.dmDisplayFrequency == mode.refresh_hz
+            {
                 return true;
             }
             i += 1;