Merge pull request #305 from lahfir/fix/review-window-clipboard

fix: await queued windows and reclaim stale clipboard records

Author: lahfir

crates/linux/src/input/clipboard/owner/owner_files.rs

@@ -72,31 +72,41 @@ pub(crate) fn read_named(dir: &Path, name: &str) -> io::Result<Vec<u8>> {
     read_file_at(&directory, std::ffi::OsStr::new(name))
 }
 
-pub(crate) fn read_matching(dir: &Path, prefix: &str, suffix: &str) -> io::Result<Vec<Vec<u8>>> {
+pub(crate) fn read_matching(
+    dir: &Path,
+    prefix: &str,
+    suffix: &str,
+    deadline: agent_desktop_core::Deadline,
+    mut retain: impl FnMut(&[u8]) -> bool,
+) -> io::Result<Vec<Vec<u8>>> {
     let directory = match open_private_dir(dir) {
         Ok(directory) => directory,
         Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(Vec::new()),
         Err(error) => return Err(error),
     };
     let mut entries = Dir::read_from(&directory)?;
     let mut records = Vec::new();
-    let mut matched_count = 0;
     let mut total_bytes: usize = 0;
     while let Some(entry) = entries.read() {
+        deadline.ensure_remaining().map_err(|_| {
+            io::Error::new(io::ErrorKind::TimedOut, "clipboard record scan timed out")
+        })?;
         let entry = entry?;
         let name = entry.file_name();
         let bytes = name.to_bytes();
         if bytes.starts_with(prefix.as_bytes()) && bytes.ends_with(suffix.as_bytes()) {
-            matched_count += 1;
-            if matched_count > MAX_RECORD_COUNT {
-                return Err(io::Error::new(
-                    io::ErrorKind::InvalidData,
-                    "the clipboard owner record count exceeds the limit",
-                ));
-            }
             let name = std::ffi::OsStr::from_bytes(bytes);
             match read_file_at(&directory, name) {
                 Ok(record) => {
+                    if !retain(&record) {
+                        continue;
+                    }
+                    if records.len() >= MAX_RECORD_COUNT {
+                        return Err(io::Error::new(
+                            io::ErrorKind::InvalidData,
+                            "the clipboard owner record count exceeds the limit",
+                        ));
+                    }
                     total_bytes = total_bytes.checked_add(record.len()).ok_or_else(|| {
                         io::Error::new(
                             io::ErrorKind::InvalidData,

crates/linux/src/input/clipboard/owner/owner_record.rs

@@ -140,13 +140,40 @@ pub(crate) fn read_outcome(dir: &Path, display: &str, nonce: &str) -> Option<Out
     outcome_of(&owner_files::read_named(dir, &name).ok()?)
 }
 
-pub(crate) fn records_in(dir: &Path, display: &str) -> std::io::Result<Vec<OwnerRecord>> {
+pub(crate) fn records_in(
+    dir: &Path,
+    display: &str,
+    deadline: agent_desktop_core::Deadline,
+) -> std::io::Result<Vec<OwnerRecord>> {
     let wanted = normalise_display(display);
-    Ok(owner_files::read_matching(dir, FILE_PREFIX, FILE_SUFFIX)?
-        .into_iter()
-        .filter_map(|bytes| OwnerRecord::from_bytes(&bytes))
-        .filter(|record| record.display == wanted)
-        .collect())
+    let retain = |bytes: &[u8]| {
+        let Some(record) = OwnerRecord::from_bytes(bytes) else {
+            return false;
+        };
+        if record.display != wanted {
+            return false;
+        }
+        let stale = match start_ticks_of(record.pid) {
+            Some(ticks) => ticks != record.start_ticks,
+            None => std::fs::metadata(format!("/proc/{}", record.pid))
+                .is_err_and(|error| error.kind() == std::io::ErrorKind::NotFound),
+        };
+        if stale {
+            remove_own(dir, &record);
+        }
+        !stale
+    };
+    Ok(owner_files::read_matching(
+        dir,
+        &format!("{FILE_PREFIX}{wanted}-"),
+        FILE_SUFFIX,
+        deadline,
+        retain,
+    )?
+    .into_iter()
+    .filter_map(|bytes| OwnerRecord::from_bytes(&bytes))
+    .filter(|record| record.display == wanted)
+    .collect())
 }
 
 pub(crate) fn start_ticks_of(pid: u32) -> Option<u64> {

crates/linux/src/input/clipboard/owner/owner_record_tests.rs

@@ -1,5 +1,6 @@
 use super::*;
 use crate::input::clipboard::test_nonce::test_nonce;
+use agent_desktop_core::Deadline;
 
 fn record(nonce: &str) -> OwnerRecord {
     OwnerRecord {
@@ -62,20 +63,26 @@ fn a_failure_outcome_carries_its_message_and_is_not_a_record() {
         read_outcome(&dir, ":7", &nonce),
         Some(Outcome::Failed("payload missing".into()))
     );
-    assert!(records_in(&dir, ":7").expect("list").is_empty());
+    assert!(
+        records_in(&dir, ":7", Deadline::standard().unwrap())
+            .expect("list")
+            .is_empty()
+    );
 }
 
 #[test]
 fn records_are_listed_per_display_only() {
     let dir = scratch("listing");
     let seven_nonce = test_nonce();
     let mut seven = record(&seven_nonce);
+    seven.pid = std::process::id();
+    seven.start_ticks = start_ticks_of(seven.pid).expect("live process");
     seven.display = ":7".into();
     let mut eight = record(&test_nonce());
     eight.display = ":8".into();
     publish(&dir, &seven).expect("seven");
     publish(&dir, &eight).expect("eight");
-    let found = records_in(&dir, ":7.0").expect("list");
+    let found = records_in(&dir, ":7.0", Deadline::standard().unwrap()).expect("list");
     assert_eq!(
         found,
         [OwnerRecord {
@@ -88,13 +95,56 @@ fn records_are_listed_per_display_only() {
     );
 }
 
+#[test]
+fn stale_and_other_display_records_do_not_block_the_live_owner() {
+    let dir = scratch("stale-limit");
+    let mut live = record(&test_nonce());
+    live.pid = std::process::id();
+    live.start_ticks = start_ticks_of(live.pid).expect("live process");
+    for index in 0..300 {
+        let mut stale = record(&format!("stale{index:08}"));
+        stale.pid = if index % 2 == 0 { u32::MAX } else { live.pid };
+        stale.start_ticks = live.start_ticks + 1;
+        publish(&dir, &stale).expect("stale record");
+        let mut foreign = live.clone();
+        foreign.display = ":8".into();
+        foreign.nonce = format!("foreign{index:08}");
+        publish(&dir, &foreign).expect("other display");
+    }
+    publish(&dir, &live).expect("live owner");
+    let found = records_in(&dir, ":7", Deadline::standard().unwrap())
+        .expect("stale records must not block discovery");
+    assert_eq!(found.len(), 1);
+    assert_eq!(found[0].nonce, live.nonce);
+    assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 301);
+    assert_eq!(
+        records_in(&dir, ":8", Deadline::standard().unwrap())
+            .unwrap_err()
+            .kind(),
+        std::io::ErrorKind::InvalidData
+    );
+}
+
+#[test]
+fn an_expired_scan_does_not_read_or_reclaim_records() {
+    let dir = scratch("expired-scan");
+    publish(&dir, &record(&test_nonce())).unwrap();
+    let error = records_in(&dir, ":7", Deadline::after(0).unwrap()).unwrap_err();
+    assert_eq!(error.kind(), std::io::ErrorKind::TimedOut);
+    assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1);
+}
+
 #[test]
 fn an_exiting_owner_removes_its_own_record() {
     let dir = scratch("own");
     let own = record(&test_nonce());
     publish(&dir, &own).expect("publish");
     remove_own(&dir, &own);
-    assert!(records_in(&dir, ":7").expect("list").is_empty());
+    assert!(
+        records_in(&dir, ":7", Deadline::standard().unwrap())
+            .expect("list")
+            .is_empty()
+    );
 }
 
 #[test]
@@ -121,7 +171,10 @@ fn records_in_rejects_a_symlink_record() {
     let target = dir.join("target.json");
     std::fs::write(&target, record(&test_nonce()).to_bytes()).expect("target");
     symlink(&target, dir.join(name)).expect("symlink");
-    assert!(records_in(&dir, ":7").is_err(), "symlinks are refused");
+    assert!(
+        records_in(&dir, ":7", Deadline::standard().unwrap()).is_err(),
+        "symlinks are refused"
+    );
 }
 
 #[cfg(unix)]
@@ -141,7 +194,7 @@ fn records_in_rejects_a_fifo_without_waiting_for_a_writer() {
     )
     .expect("fifo");
     assert_eq!(
-        records_in(&dir, ":7")
+        records_in(&dir, ":7", Deadline::standard().unwrap())
             .expect_err("FIFOs are refused")
             .kind(),
         std::io::ErrorKind::InvalidData
@@ -160,7 +213,7 @@ fn records_in_rejects_an_oversized_record() {
     )
     .expect("large record");
     assert_eq!(
-        records_in(&dir, ":7")
+        records_in(&dir, ":7", Deadline::standard().unwrap())
             .expect_err("oversized records are refused")
             .kind(),
         std::io::ErrorKind::InvalidData
@@ -177,7 +230,7 @@ fn records_in_rejects_a_permissive_directory() {
     std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o777))
         .expect("permissive mode");
     assert_eq!(
-        records_in(&dir, ":7")
+        records_in(&dir, ":7", Deadline::standard().unwrap())
             .expect_err("permissive directory is refused")
             .kind(),
         std::io::ErrorKind::PermissionDenied

crates/linux/src/input/clipboard/set_clear.rs

@@ -33,6 +33,16 @@ fn failed(message: impl Into<String>, disposition: DeliverySemantics) -> Adapter
     AdapterError::new(ErrorCode::ActionFailed, message).with_disposition(disposition)
 }
 
+fn record_read_error(error: std::io::Error, disposition: DeliverySemantics) -> AdapterError {
+    let message = format!("Clipboard owner records could not be read: {error}");
+    let error = if error.kind() == std::io::ErrorKind::TimedOut {
+        AdapterError::timeout(message)
+    } else {
+        AdapterError::new(ErrorCode::ActionFailed, message)
+    };
+    error.with_disposition(disposition)
+}
+
 fn discard(dir: &Path, display: &str, nonce: &str) {
     if let Some(name) = owner_record::file_name(display, nonce) {
         let _ = super::owner::owner_files::remove(dir, &name);
@@ -118,12 +128,9 @@ pub(crate) fn set_with(
         .server
         .owner(display, deadline)
         .map_err(|fault| fault_error(fault, None))?;
-    let earlier = owner_record::records_in(env.record_dir, display).map_err(|error| {
-        failed(
-            format!("Clipboard owner records could not be read: {error}"),
-            DeliverySemantics::not_delivered(),
-        )
-    })?;
+    let earlier = owner_record::records_in(env.record_dir, display, deadline)
+        .map_err(|error| record_read_error(error, DeliverySemantics::not_delivered()))?;
+    deadline.ensure_remaining().map_err(not_delivered)?;
     let mut child = start_owner(env, display, &payload)?;
     let mut probe = ChildProbe {
         child: &mut child,
@@ -182,12 +189,8 @@ pub(crate) fn clear_with(
     env.server
         .release(display, deadline)
         .map_err(|fault| fault_error(fault, None))?;
-    let records = owner_record::records_in(env.record_dir, display).map_err(|error| {
-        failed(
-            format!("Clipboard owner records could not be read after clearing: {error}"),
-            DeliverySemantics::delivered_unverified(),
-        )
-    })?;
+    let records = owner_record::records_in(env.record_dir, display, deadline)
+        .map_err(|error| record_read_error(error, DeliverySemantics::delivered_unverified()))?;
     for record in records {
         retire(env, display, &record);
     }

crates/linux/src/input/clipboard/set_clear_live_support.rs

@@ -153,7 +153,14 @@ pub(super) fn foreign_read(display: &str, target: &str) -> Option<Vec<u8>> {
 
 pub(super) fn records(display: &str) -> Vec<OwnerRecord> {
     owner_record::record_dir()
-        .and_then(|dir| owner_record::records_in(&dir, display).ok())
+        .and_then(|dir| {
+            owner_record::records_in(
+                &dir,
+                display,
+                agent_desktop_core::Deadline::standard().unwrap(),
+            )
+            .ok()
+        })
         .unwrap_or_default()
 }
 

crates/linux/src/input/clipboard/set_clear_tests.rs

@@ -344,5 +344,40 @@ fn no_payload_descriptor_stays_open_after_a_failed_set() {
 #[path = "set_clear_clear_tests.rs"]
 mod clear_tests;
 
+#[test]
+fn record_scan_timeout_preserves_set_and_clear_delivery_states() {
+    for clear in [false, true] {
+        let world = World::new(if clear { "scan-clear" } else { "scan-set" });
+        write_record(&world, u32::MAX, 1, &test_nonce());
+        let server = FakeServer::new(&[None], &[]);
+        let parent = |name: &str| world.env_var(name);
+        let env = Env {
+            helper: Path::new("/nonexistent/helper"),
+            record_dir: &world.records,
+            server: &server,
+            parent_env: &parent,
+            nonce: &world.nonce,
+        };
+        let lease = lease(100);
+        std::thread::sleep(std::time::Duration::from_millis(120));
+        let result = if clear {
+            clear_with(&env, DISPLAY, &lease)
+        } else {
+            set_with(&env, DISPLAY, &text("unchanged"), &lease)
+        };
+        let error = result.expect_err("expired record scan");
+        assert_eq!(error.code, ErrorCode::Timeout);
+        assert_eq!(server.released.get(), u32::from(clear));
+        assert_eq!(
+            error.disposition,
+            if clear {
+                DeliverySemantics::delivered_unverified()
+            } else {
+                DeliverySemantics::not_delivered()
+            }
+        );
+    }
+}
+
 #[path = "set_clear_reaper_tests.rs"]
 mod reaper_tests;

crates/windows/src/system/window/mod.rs

@@ -6,5 +6,6 @@ pub(crate) mod window_identity;
 pub(crate) mod window_op;
 pub(crate) mod window_op_readback;
 pub(crate) mod window_op_verb;
+pub(crate) mod window_op_wait;
 pub(crate) mod window_ops;
 pub(crate) mod window_resolve;