Merge pull request #943 from l0ng-ai/fix/watch-close-off-ui-thread

Stop waiting on remote round trips from the UI thread

crates/tty7-core/src/daemon/control.rs

@@ -1096,6 +1096,25 @@ impl ControlClient {
         self.call_full(req, blob).map(|r| r.reply)
     }
 
+    /// Sends `req` without waiting for its answer. The reply, when it comes,
+    /// finds nobody waiting for its `req_id` and the reader drops it.
+    ///
+    /// For requests whose answer nobody acts on, made from places that cannot
+    /// afford a round trip: a `Drop` that may run on the UI thread, where a
+    /// `call` parks every window for as long as the peer takes to answer — up
+    /// to the request's whole deadline on a link that has gone quiet.
+    pub fn post(&self, req: ControlRequest) -> io::Result<()> {
+        if !self.is_connected() {
+            return Err(io::Error::new(
+                io::ErrorKind::ConnectionReset,
+                "control connection is down",
+            ));
+        }
+        let req_id = self.inner.next_req_id.fetch_add(1, Ordering::Relaxed);
+        log::debug!(target: "tty7::control", "#{req_id} {req:?} (not awaited)");
+        self.inner.send(&ControlClientMsg::Request { req_id, req })
+    }
+
     pub fn call_full(&self, req: ControlRequest, blob: &[u8]) -> io::Result<ControlResponse> {
         let deadline = req.deadline();
         self.call_with_deadline(req, blob, deadline)
@@ -2488,6 +2507,45 @@ mod tests {
         ));
     }
 
+    /// `post` returns while the peer is still sitting on its answer, and that
+    /// answer, when it does arrive, is not handed to the next caller.
+    #[test]
+    fn a_post_does_not_wait_for_its_answer() {
+        let (seen_tx, seen_rx) = mpsc::channel::<u64>();
+        let (answer_tx, answer_rx) = mpsc::channel::<()>();
+        let client = client_with_peer(no_events(), move |mut sock| {
+            let closed = match ControlClientMsg::read(&mut sock).unwrap() {
+                ControlClientMsg::Request {
+                    req_id,
+                    req: ControlRequest::WatchClose { id: 7 },
+                } => req_id,
+                other => panic!("unexpected message {other:?}"),
+            };
+            seen_tx.send(closed).unwrap();
+            answer_rx.recv().unwrap();
+            reply_to(&mut sock, closed, ReplyOk::Unit);
+            match ControlClientMsg::read(&mut sock).unwrap() {
+                ControlClientMsg::Request { req_id, .. } => {
+                    reply_to(&mut sock, req_id, ReplyOk::Path("/next".into()))
+                }
+                other => panic!("unexpected message {other:?}"),
+            }
+        });
+
+        client.post(ControlRequest::WatchClose { id: 7 }).unwrap();
+        seen_rx
+            .recv_timeout(Duration::from_secs(5))
+            .expect("the post reached the peer");
+        answer_tx.send(()).unwrap();
+
+        let next = client
+            .call(ControlRequest::Canonicalize {
+                path: "/next".into(),
+            })
+            .unwrap();
+        assert_eq!(next, ReplyOk::Path("/next".into()));
+    }
+
     #[test]
     fn concurrent_callers_each_get_their_own_reply() {
         const N: u64 = 16;

crates/tty7-core/src/host/remote.rs

@@ -642,8 +642,12 @@ impl WatchHandle for RemoteWatch {
 impl Drop for RemoteWatch {
     fn drop(&mut self) {
         self.watches.remove(self.id);
+        // Posted, not called: the last handle is often let go on the UI
+        // thread — a watcher whose repository changed, an open that landed
+        // after its subscription moved on — and waiting there for the peer's
+        // acknowledgement froze every window for a round trip each time.
         if self.client.is_connected() {
-            let _ = self.client.call(ControlRequest::WatchClose { id: self.id });
+            let _ = self.client.post(ControlRequest::WatchClose { id: self.id });
         }
     }
 }

src/core/keychain.rs

@@ -100,6 +100,29 @@ impl CredentialStore for OsCredentialStore {
     }
 }
 
+/// A store with nothing in it, for building a spec whose secrets are going to
+/// be stripped anyway. Asking the real one costs a trip to `securityd` per
+/// credential, and the callers that want no secrets are the ones on the UI
+/// thread.
+#[derive(Debug, Default, Clone, Copy)]
+pub struct NoCredentials;
+
+impl CredentialStore for NoCredentials {
+    fn get(&self, _service: &str, _account: &str) -> CredentialResult<Option<String>> {
+        Ok(None)
+    }
+
+    fn set(&self, _service: &str, _account: &str, _secret: &str) -> CredentialResult<()> {
+        Err(CredentialError::Backend(
+            "this store holds no credentials".into(),
+        ))
+    }
+
+    fn delete(&self, _service: &str, _account: &str) -> CredentialResult<()> {
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 #[derive(Debug, Default)]
 pub struct InMemoryCredentialStore {

src/ui/remote_connect.rs

@@ -260,6 +260,21 @@ fn endpoint_label(user: &str, host: &str, port: u16) -> String {
 }
 
 pub fn spec_for(target: &RemoteTarget, cx: &App) -> Result<NativeSshSpec, String> {
+    spec_from(target, cx, &crate::core::keychain::OsCredentialStore)
+}
+
+/// [`spec_for`] with no secrets in it, built without asking the keychain for
+/// any: for callers that only describe the route, which run on the UI thread
+/// and would strip the password again the moment they had it.
+pub fn public_spec_for(target: &RemoteTarget, cx: &App) -> Result<NativeSshSpec, String> {
+    spec_from(target, cx, &crate::core::keychain::NoCredentials)
+}
+
+fn spec_from(
+    target: &RemoteTarget,
+    cx: &App,
+    store: &dyn crate::core::keychain::CredentialStore,
+) -> Result<NativeSshSpec, String> {
     let cfg = cx.global::<Config>();
     match target {
         RemoteTarget::Profile { id } => {
@@ -271,7 +286,7 @@ pub fn spec_for(target: &RemoteTarget, cx: &App) -> Result<NativeSshSpec, String
             Ok(crate::ui::ssh_connect::build_native_ssh_spec(
                 profile,
                 &cfg.ssh_profiles,
-                &crate::core::keychain::OsCredentialStore,
+                store,
                 cfg.verify_host_keys,
             ))
         }
@@ -281,7 +296,7 @@ pub fn spec_for(target: &RemoteTarget, cx: &App) -> Result<NativeSshSpec, String
             Ok(crate::ui::ssh_connect::native_spec_from_transient_profile(
                 &resolved.profile,
                 resolved.proxy_jump,
-                &crate::core::keychain::OsCredentialStore,
+                store,
                 cfg.verify_host_keys,
                 &crate::ui::ssh_connect::config_alias_resolver,
             ))
@@ -294,7 +309,7 @@ pub fn spec_for(target: &RemoteTarget, cx: &App) -> Result<NativeSshSpec, String
             Ok(crate::ui::ssh_connect::build_native_ssh_spec(
                 &profile,
                 &cfg.ssh_profiles,
-                &crate::core::keychain::OsCredentialStore,
+                store,
                 cfg.verify_host_keys,
             ))
         }

src/ui/remote_workspace.rs

@@ -1213,7 +1213,7 @@ pub(crate) fn pane_workspace_for(
     workspace: WorkspaceId,
 ) -> Option<crate::terminal::PaneWorkspace> {
     let host = WorkspaceStore::remote_ref(cx, workspace)?;
-    let spec = remote_connect::spec_for(&host.target, cx)
+    let spec = remote_connect::public_spec_for(&host.target, cx)
         .ok()
         .map(|spec| Box::new(spec.without_secrets()));
     // Answered here because the terminal cannot ask the network itself: the

src/ui/ssh_connect.rs

@@ -755,6 +755,29 @@ mod tests {
         assert_eq!(spec.password, None);
     }
 
+    /// A pane's route is built on the UI thread with the keychain left out of
+    /// it; that must describe the same connection as the full spec with its
+    /// secrets stripped, jump host and all.
+    #[test]
+    fn a_spec_built_without_credentials_is_the_full_one_stripped() {
+        let store = InMemoryCredentialStore::new();
+        store
+            .set_password("deploy", "10.0.0.5", 22, "hunter2")
+            .unwrap();
+        store.set_password("ops", "bastion", 22, "s3cret").unwrap();
+        let jump = profile("bastion", "bastion", "ops");
+        let mut p = profile("web", "10.0.0.5", "deploy");
+        p.jump_host = Some(jump.id);
+        let profiles = [p.clone(), jump];
+
+        let full = build_native_ssh_spec(&p, &profiles, &store, true);
+        assert_eq!(full.password.as_deref(), Some("hunter2"));
+        assert_eq!(
+            build_native_ssh_spec(&p, &profiles, &crate::core::keychain::NoCredentials, true),
+            full.without_secrets()
+        );
+    }
+
     /// The pane wears this until the remote shell titles itself, so a host
     /// nobody bothered to name still says where it went (#438). Every host
     /// imported from `~/.ssh/config` used to arrive nameless, and every one