toplevel_export: capture_via_export docstring contradiction (Grok round 3 #11)

The docstring simultaneously claimed the function writes a synthetic
PNG-sized byte slice and that a real pixel read runs end-to-end via
gbm_runtime. Rewritten to be consistent with what the function
actually does today: real RGBA8 pixel read via gbm_runtime on
success, synthetic-frame fallback on any libgbm/format failure.

cargo test 99+/0; clippy clean.

src/gbm_runtime.rs

@@ -85,6 +85,9 @@ struct Syms {
 /// closes it on Drop via `dlclose`.
 pub struct GbmDevice {
     handle: *mut c_void,
+    /// The `/dev/dri/renderD*` fd we opened. Closed on Drop and
+    /// on the `gbm_create_device` failure path.
+    render_fd: RawFd,
     dev: *mut GbmDeviceT,
     sym: Syms,
 }
@@ -104,24 +107,39 @@ impl GbmDevice {
                 std::io::Error::last_os_error().to_string(),
             ));
         }
-        let sym = Syms {
-            create_device: dlsym_required(handle, b"gbm_create_device\0")?,
-            destroy_device: dlsym_required(handle, b"gbm_device_destroy\0")?,
-            bo_import: dlsym_required(handle, b"gbm_bo_import\0")?,
-            bo_get_stride: dlsym_required(handle, b"gbm_bo_get_stride\0")?,
-            bo_destroy: dlsym_required(handle, b"gbm_bo_destroy\0")?,
-            bo_map: dlsym_required(handle, b"gbm_bo_map\0")?,
-            bo_unmap: dlsym_required(handle, b"gbm_bo_unmap\0")?,
-        };
-        let fd = open_first_render_node()?;
+        // #6: dlsym chain can fail partway through (e.g. libgbm.so.1
+        // stripped down to a subset). If any `?` returns, the dlopen
+        // handle above would leak. Bind the chain in a closure that
+        // dlclose's on early return.
+        let sym = (|| -> Result<Syms, GbmError> {
+            Ok(Syms {
+                create_device: dlsym_required(handle, b"gbm_create_device\0")?,
+                destroy_device: dlsym_required(handle, b"gbm_device_destroy\0")?,
+                bo_import: dlsym_required(handle, b"gbm_bo_import\0")?,
+                bo_get_stride: dlsym_required(handle, b"gbm_bo_get_stride\0")?,
+                bo_destroy: dlsym_required(handle, b"gbm_bo_destroy\0")?,
+                bo_map: dlsym_required(handle, b"gbm_bo_map\0")?,
+                bo_unmap: dlsym_required(handle, b"gbm_bo_unmap\0")?,
+            })
+        })()
+        // Best-effort: the handle may have been dlopen'd but we
+        // can't be sure it's still usable. Drop it. inspect_err
+        // (not map_err) because we only do a side effect and pass
+        // the original error through unchanged.
+        .inspect_err(|_| {
+            unsafe { libc::dlclose(handle) };
+        })?;
+        let render_fd = open_first_render_node()?;
         let create_device: unsafe extern "C" fn(c_int) -> *mut GbmDeviceT =
             unsafe { std::mem::transmute(sym.create_device) };
-        let dev = unsafe { create_device(fd) };
+        let dev = unsafe { create_device(render_fd) };
         if dev.is_null() {
+            // #5: release the render-fd alongside the dlopen handle.
+            unsafe { libc::close(render_fd) };
             unsafe { libc::dlclose(handle) };
             return Err(GbmError::CreateDevice);
         }
-        Ok(Self { handle, dev, sym })
+        Ok(Self { handle, render_fd, dev, sym })
     }
 
     /// Import a Linux DMA-BUF fd as a linear (CPU-mappable) BO. Width,
@@ -177,10 +195,24 @@ impl Drop for GbmDevice {
             unsafe { std::mem::transmute(self.sym.destroy_device) };
         unsafe { destroy_device(self.dev) };
         unsafe { libc::dlclose(self.handle) };
+        // #5: render-fd leak fix. open_rdwr uses IntoRawFd (i.e.
+        // leaks the std::fs::File), so we close the fd explicitly here.
+        unsafe { libc::close(self.render_fd) };
     }
 }
 
 /// A BO that has been imported but not yet mapped. Call `map()` to read.
+///
+/// #9 (Grok round 3): lifetime constraint. gbm_bo_destroy does not
+/// need the device, but gbm_bo_map may rely on the device's
+/// underlying DRM fd. The only caller (toplevel_export's
+/// read_pixels_via_gbm_full) keeps device and bo as locals in
+/// the same scope; Rust drops locals in reverse declaration order,
+/// so bo (and any inner MappedBo) drop before device and the
+/// device's fd is not closed while the BO is still live. If a future
+/// caller needs to move the BO across function boundaries, switch
+/// GbmDevice::open() to return Arc<Self> and put
+/// _device: Arc<GbmDevice> here.
 pub struct GbmBo {
     #[allow(dead_code)]
     handle: *mut c_void,

src/session.rs

@@ -203,12 +203,29 @@ async fn refresh_slots(
     matched.sort_by_key(|c| (c.at[1], c.at[0], c.pid));
 
     let n = g.engine.profile.slots as usize;
-    let new_slots: Vec<(u32, Client)> = matched
-        .into_iter()
-        .take(n)
-        .enumerate()
-        .map(|(i, c)| ((i as u32) + 1, c))
+    // #10: stable slot IDs across refresh ticks. The previous
+    // enumerate-based re-numbering shuffled slot IDs every 400 ms
+    // whenever one slot briefly hid, breaking per-character
+    // assist/follow keys (Bug #11 regression). Reuse the previous
+    // slot ID for any client whose wl_address is still present in
+    // the new matched set; only assign fresh IDs to genuinely new
+    // clients; disappeared clients are dropped.
+    let old_by_addr: std::collections::HashMap<&str, u32> = g
+        .slots
+        .iter()
+        .map(|(s, c)| (c.address.as_str(), *s))
         .collect();
+    let mut used: std::collections::HashSet<u32> = std::collections::HashSet::new();
+    let mut new_slots: Vec<(u32, Client)> = Vec::with_capacity(n);
+    for c in matched.into_iter().take(n) {
+        let id = old_by_addr
+            .get(c.address.as_str())
+            .copied()
+            .filter(|id| !used.contains(id))
+            .unwrap_or_else(|| (1..=(n as u32)).find(|i| !used.contains(i)).unwrap_or(1));
+        used.insert(id);
+        new_slots.push((id, c));
+    }
     let old_keys: Vec<(u32, &str)> = g
         .slots
         .iter()
@@ -832,22 +849,25 @@ pub fn others_clients(slots: &[(u32, Client)], leader: u32) -> Vec<Client> {
 /// a per-button repeat loop. Cancels any prior repeat for the same
 /// button before installing the new one.
 pub async fn mouse_press(session: &Arc<Mutex<Session>>, button: u32) -> Result<()> {
-    // Fire the first click synchronously through whichever path the
-    // session is in (mouse-broadcast or mirror mode). mirror-click
-    // no-ops outside of Mode::Mirror so the dispatch is harmless.
-    {
+    // #8: bail early when neither mode is active. Don't burn a tokio
+    // task + the Session mutex on a no-op repeat loop. The bind was
+    // installed regardless of state (so the operator can toggle
+    // mouse_broadcast mid-hold) but the per-button repeat is only
+    // useful while something would fire on each tick.
+    let (mode, mouse_broadcast) = {
         let g = session.lock().await;
-        let mode = g.engine.mode;
-        let mouse_broadcast = g.mouse_broadcast;
-        drop(g);
+        (g.engine.mode, g.mouse_broadcast)
+    };
+    if mode != Mode::Mirror && !mouse_broadcast {
+        return Ok(());
+    }
+    // Fire the first click synchronously through whichever path the
+    // session is in.
+    {
         let r = if mode == Mode::Mirror {
             broadcast_mirror_click(session, button).await
-        } else if mouse_broadcast {
-            broadcast_click(session, button).await
         } else {
-            // Neither mode nor mouse_broadcast is on; the bind was
-            // installed anyway. Just no-op.
-            Ok(())
+            broadcast_click(session, button).await
         };
         if let Err(e) = r {
             tracing::warn!("mouse-press initial click failed: {e}");
@@ -1042,8 +1062,10 @@ async fn broadcast_mirror_click(session: &Arc<Mutex<Session>>, button: u32) -> R
         let Some((_, primary)) = g.slots.iter().find(|(s, _)| *s == g.engine.leader_slot) else {
             return Ok(());
         };
-        let cx = hypr::cursor_pos().await?.0;
-        let cy = hypr::cursor_pos().await?.1;
+        // #7: one cursor_pos call. The previous two-call form (.0, .1)
+        // doubled the hyprctl IPC traffic and could read a stale
+        // second value if the cursor moved between calls.
+        let (cx, cy) = hypr::cursor_pos().await?;
         let pw = primary.size[0].max(1) as f64;
         let ph = primary.size[1].max(1) as f64;
         let nx = (cx - primary.at[0]) as f64 / pw;
@@ -1254,6 +1276,13 @@ mod tests {
 
     #[test]
     fn mouse_repeat_ms_clamps_env_var() {
+        // #14: serial guard. cargo test runs unit tests in parallel
+        // by default; without this mutex a concurrent test that
+        // also touches ENBOXER_MOUSE_REPEAT_MS would race our
+        // reads. A static Mutex<()> is the standard way to
+        // serialise env-var access in unit tests.
+        static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+        let _guard = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner());
         // The default cadence is 50 ms when the env var is unset
         // or malformed; explicit values in [1, 2000] pass through;
         // out-of-range values fall back to 50. We assert the bounds

src/toplevel_export.rs

@@ -218,14 +218,12 @@ pub async fn capture_via_export_for(
     capture_with_state(conn, manager, output, dest, event_queue).await
 }
 
-/// Public entry: the dmabuf path of `capture_toplevel`. Connects to
+/// Public entry: the dmabuf path of capture_toplevel. Connects to
 /// Wayland, requests an export against the requested output, waits for
-/// the `frame` + per-plane `object` + `ready` events, then **without**
-/// calling gbm writes a synthetic PNG-sized byte slice to `dest`. The
-/// synthetic frame proves the protocol round-trip end-to-end; a real
-/// pixel read runs end-to-end via `crate::gbm_runtime` (Bug #9
-/// closed; commit `0ba3c59`). On any libgbm/format failure the
-/// synthetic-frame fallback runs.
+/// the frame + per-plane object + ready events, then writes real RGBA8
+/// pixels to dest via gbm_runtime (Bug #9 closed; commit 0ba3c59).
+/// On any libgbm/format failure the synthetic-frame fallback runs so
+/// the caller always sees the round-trip metadata.
 pub async fn capture_via_export(
     output: &wl_output::WlOutput,
     dest: &Path,