From 439f19ff85cd06a3576bb757a7a0a4ad825ad694 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:00:54 +0800 Subject: [PATCH] Stop waiting on remote round trips from the UI thread Dropping a RemoteWatch sent WatchClose with a blocking call, and the last handle is usually let go on the UI thread: from Tty7App::render via scm_sync_watchers, and from scm_watch_opened when an open lands after its subscription moved on. Every window froze for a round trip each time, up to WatchClose's 5 s deadline on a quiet link. Sampling a live instance caught ~1.1 s of such stalls in 10 s. - ControlClient::post sends a request without registering for its reply; the reader already drops replies nobody is waiting for. - RemoteWatch::drop posts WatchClose instead of calling it. - pane_workspace_for built the full SSH spec, keychain lookups included, only to strip the secrets again. It now builds it from NoCredentials, which takes the securityd trips off pane_liveness::sweep. Claude-Session: https://claude.ai/code/session_01YX786Hyr4ivijWxv66zVkf --- crates/tty7-core/src/daemon/control.rs | 58 ++++++++++++++++++++++++++ crates/tty7-core/src/host/remote.rs | 6 ++- src/core/keychain.rs | 23 ++++++++++ src/ui/remote_connect.rs | 21 ++++++++-- src/ui/remote_workspace.rs | 2 +- src/ui/ssh_connect.rs | 23 ++++++++++ 6 files changed, 128 insertions(+), 5 deletions(-) diff --git a/crates/tty7-core/src/daemon/control.rs b/crates/tty7-core/src/daemon/control.rs index 3320ec0b..136a3164 100644 --- a/crates/tty7-core/src/daemon/control.rs +++ b/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 { 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::(); + 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; diff --git a/crates/tty7-core/src/host/remote.rs b/crates/tty7-core/src/host/remote.rs index c90896f4..5454e5fa 100644 --- a/crates/tty7-core/src/host/remote.rs +++ b/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 }); } } } diff --git a/src/core/keychain.rs b/src/core/keychain.rs index b94fa841..cf29130b 100644 --- a/src/core/keychain.rs +++ b/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> { + 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 { diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index f8f1bc85..05768185 100644 --- a/src/ui/remote_connect.rs +++ b/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 { + 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 { + spec_from(target, cx, &crate::core::keychain::NoCredentials) +} + +fn spec_from( + target: &RemoteTarget, + cx: &App, + store: &dyn crate::core::keychain::CredentialStore, +) -> Result { let cfg = cx.global::(); match target { RemoteTarget::Profile { id } => { @@ -271,7 +286,7 @@ pub fn spec_for(target: &RemoteTarget, cx: &App) -> Result Result Result Option { 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 diff --git a/src/ui/ssh_connect.rs b/src/ui/ssh_connect.rs index 373968ba..c780c7bd 100644 --- a/src/ui/ssh_connect.rs +++ b/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