From 8adebdf24deb0752a42df9d902956b01fe62a252 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:54:45 +0800 Subject: [PATCH] perf(ssh): prove the remote server once per connection, not once per pane (#695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a second tab on a machine tty7 was already connected to and already serving cost the same wait as the first one. The SSH connection is reused, so none of that wait was handshake cost: every route called `ensure_remote_server` unconditionally, and that runs the whole installer probe again — `uname -sm`, an SFTP realpath for the home directory, an SFTP stat, a control probe that spawns the server binary, and `check_running_build`, which walks `/proc/[0-9]*` with a `readlink` per PID and shells out to `ps` on the machines that have no `/proc`. Five serial round trips before the pane's own channel opened, to re-learn what the pane before it had just learned. WSL fixed exactly this in #479 by remembering where a distro's server was last proved to be. SSH now does the same, with one difference that matters: a distro name is the whole identity of a WSL target, but an SSH connection can die and be replaced under the same key, so the note is kept on the `SshConnection` rather than in a map beside its key. Keying by connection generation is then not a discipline anyone has to keep — a reconnect is a new `SshConnection` with an empty slot, and nothing has to remember to forget. Memoizing must not quietly cancel the version check, which is the one thing that could make this a bad trade. Three things keep it honest. The note carries the build mismatch the probe found and re-files it on every hit, because the warning is raised inside `Installer::run` and each route drains its own sink — without that, only the first pane on a connection would ever hear that a different build is serving the machine, and every window after it would attach in silence. `replace_remote_server` and `restart_remote_daemon` forget before they act, not after, so a restart that fails halfway leaves the next pane looking rather than trusting a note written before the upheaval. And the router forgets when a routed link closes without the remote sending a byte, the way it already does for a WSL bridge: `exec` succeeds whatever the command turns out to be, so a binary deleted or moved since the probe is discovered exactly there. A failed probe is deliberately not remembered. A host that was briefly unreachable, or an install the user declined once, must not pin every later pane on that connection into the same failure — the slot is written only when the probe got all the way through. The note carries the binary path and the mismatch and nothing else: `installed`, `launched` and `confirmed` describe an event rather than a state, and serving them again to a later pane would only make the log lie. Left alone on purpose: the probe itself, which is unchanged and still the only thing that decides what a pane runs; the WSL memo, which keeps its own shape; and the macOS-server half of #695, which shipped in v26.9.1. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-core/src/daemon/install/mod.rs | 154 ++++++++++++--- crates/tty7-core/src/daemon/install/tests.rs | 198 +++++++++++++++++++ crates/tty7-core/src/daemon/router.rs | 19 ++ crates/tty7-core/src/daemon/ssh/session.rs | 37 ++++ 4 files changed, 383 insertions(+), 25 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index bc73c4d4..157970bd 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -1341,37 +1341,135 @@ fn connection_label(conn: &SshConnection) -> String { conn.key().as_str().to_string() } +/// What one SSH connection's server probe proved, kept so that the panes after +/// the first one do not pay for proving it again. +/// +/// `Installer::run` is four to six serial round trips — `uname -sm`, an SFTP +/// realpath for the home directory, an SFTP stat, a control probe that spawns +/// the server binary, and `check_running_build`, which walks `/proc//exe` +/// with a `readlink` per PID (or shells out to `ps` where there is no `/proc`). +/// That is a fair price once for a machine and an absurd one per pane: issue +/// #695 is a user watching `connecting to ...` for ten seconds every time they +/// open a tab on a host tty7 was already connected to and already serving. The +/// SSH connection itself is reused, so none of that wait is handshake cost. +/// +/// Deliberately not a global map keyed by host, the way [`wsl`]'s is. A distro +/// name is the whole identity of a WSL target, but an SSH connection can die +/// and be replaced under the same key, and a note about the previous link must +/// not answer for the next one. Keying by connection generation *is* keeping +/// the note on the connection — see `SshConnection::proved_server`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ProvedServer { + /// The server binary the probe settled on, which is what the route runs. + pub binary: String, + /// The build mismatch the probe found, if it found one. + /// + /// Kept because the warning is raised inside `Installer::run`, and the + /// whole point of the memo is that `run` does not happen again: without + /// this, only the first pane on a connection would ever hear that a + /// different build is serving the machine, and every pane after it — every + /// window, since a connection outlives one — would attach in silence. That + /// is the one way memoizing could quietly cancel the version check, so it + /// is the one thing the note carries besides the path. + pub mismatch: Option, +} + +impl ProvedServer { + fn from_report(report: InstallReport) -> ProvedServer { + ProvedServer { + binary: report.paths.binary, + mismatch: report.mismatch, + } + } +} + +/// Answer from the note if there is one, otherwise prove it and leave a note. +/// +/// Two rules the callers depend on: +/// +/// - A failed probe is not remembered. `prove` returning an error leaves the +/// slot exactly as it found it, so a host that was briefly unreachable, or an +/// install the user declined once, is retried by the next pane rather than +/// pinned into permanent failure for the life of the connection. +/// - A remembered mismatch is re-filed on every hit. Each route carries its own +/// mismatch sink (`RouteSetup::mismatches`), drained into a prompt as the +/// route is set up, so re-filing is what makes the *n*th pane's client hear +/// what the first pane's probe found. +fn proved_or_prove( + slot: &mut Option, + prove: impl FnOnce() -> io::Result, +) -> io::Result { + if let Some(known) = slot.as_ref() { + if let Some(mismatch) = known.mismatch.clone() { + record_remote_mismatches(vec![mismatch]); + } + return Ok(known.binary.clone()); + } + let proved = prove()?; + let binary = proved.binary.clone(); + *slot = Some(proved); + Ok(binary) +} + pub fn ensure_remote_server(conn: &Arc) -> io::Result { let host = connection_label(conn); ensure_remote_server_labeled(conn, &host) } pub fn ensure_remote_server_labeled(conn: &Arc, host: &str) -> io::Result { - let ops = ssh_ops::SshRemoteOps::new(conn.clone()); - let fetch = default_fetcher(); - let confirm = install_confirm(); - let source = BundledOrRelease::discover(fetch.as_ref()); - let report = Installer::with_source(&ops, &source, confirm.as_ref(), host).run()?; - log::info!( - "remote {host}: {} at {} ({}{})", - if report.installed { - "installed tty7-server" - } else { - "tty7-server already present" - }, - report.paths.binary, - if report.launched { - "daemon launched" - } else { - "daemon already running" - }, - if report.mismatch.is_some() { - ", build mismatch recorded" - } else { - "" - }, - ); - Ok(report.paths.binary) + // The lock is held across the probe, not just across the read: two panes + // opening at once on a cold connection would otherwise both install, and + // the loser would be uploading over the very file the winner is renaming + // into place. Waiting out an install is what the second pane wants to do + // anyway — it needs the same answer. + let mut slot = conn.proved_server(); + if let Some(known) = slot.as_ref() { + log::debug!( + "remote {host}: tty7-server was already proved at {} on this connection", + known.binary, + ); + } + proved_or_prove(&mut slot, || { + let ops = ssh_ops::SshRemoteOps::new(conn.clone()); + let fetch = default_fetcher(); + let confirm = install_confirm(); + let source = BundledOrRelease::discover(fetch.as_ref()); + let report = Installer::with_source(&ops, &source, confirm.as_ref(), host).run()?; + log::info!( + "remote {host}: {} at {} ({}{})", + if report.installed { + "installed tty7-server" + } else { + "tty7-server already present" + }, + report.paths.binary, + if report.launched { + "daemon launched" + } else { + "daemon already running" + }, + if report.mismatch.is_some() { + ", build mismatch recorded" + } else { + "" + }, + ); + Ok(ProvedServer::from_report(report)) + }) +} + +/// Drop what we thought we knew about a connection's server, so that the next +/// `ensure_remote_server` on it proves the whole thing again. +/// +/// Three callers, and between them they are the memo's correctness argument: +/// [`restart_remote_daemon`] and [`replace_remote_server`], which change which +/// build is serving the machine and therefore what the note claims, and the +/// router, which calls this when a routed link closes without the remote ever +/// sending a byte — the only way a path that has stopped working is found out. +/// A reconnect needs no caller at all: the note lives on the connection, and a +/// new connection has none. +pub fn forget_remote_server(conn: &SshConnection) { + *conn.proved_server() = None; } pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { @@ -1379,6 +1477,11 @@ pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { let ops = ssh_ops::SshRemoteOps::new(conn.clone()); let fetch = default_fetcher(); let confirm = install_confirm(); + // Forget first, not afterwards: this deliberately changes what is running + // over there, which is most of what the note claims, and a restart that + // fails halfway must leave the next pane looking rather than trusting a + // note written before the upheaval. + forget_remote_server(conn); Installer::new(&ops, fetch.as_ref(), confirm.as_ref(), host).restart_daemon()?; Ok(()) } @@ -1389,6 +1492,7 @@ pub fn replace_remote_server(conn: &Arc) -> io::Result<()> { let fetch = default_fetcher(); let confirm = install_confirm(); let source = BundledOrRelease::discover(fetch.as_ref()); + forget_remote_server(conn); Installer::with_source(&ops, &source, confirm.as_ref(), host).replace()?; Ok(()) } diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index dbb59194..1422e5bc 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -59,6 +59,10 @@ struct FakeRemote { /// a login shell that could not read the script, which is the shape the /// no-`/proc` bug took on every Mac. stop_fails: bool, + /// SFTP metadata reads — the realpath behind `home_dir` and every `stat`. + /// The journal carries commands and writes; these are the other half of + /// what a probe spends on the wire, and #695 is a count of both. + sftp_reads: Mutex, } impl FakeRemote { @@ -87,6 +91,7 @@ impl FakeRemote { speaks: Mutex::new(HashMap::new()), installed_speaks: Some(ours()), stop_fails: false, + sftp_reads: Mutex::new(0), } } @@ -183,10 +188,28 @@ impl FakeRemote { .filter(|j| !matches!(j, Journal::Exec(_))) .collect() } + + /// The commands this remote was asked to run, in order. + fn execs(&self) -> Vec { + self.journal() + .into_iter() + .filter_map(|j| match j { + Journal::Exec(cmd) => Some(cmd), + _ => None, + }) + .collect() + } + + /// Everything that would have crossed the wire: commands, SFTP metadata + /// reads, and the writes an install makes. + fn round_trips(&self) -> usize { + self.journal().len() + *self.sftp_reads.lock().unwrap() + } } impl RemoteOps for FakeRemote { fn home_dir(&self) -> Result { + *self.sftp_reads.lock().unwrap() += 1; Ok(HOME.to_string()) } @@ -290,6 +313,7 @@ impl RemoteOps for FakeRemote { } fn stat(&self, path: &str) -> Result, String> { + *self.sftp_reads.lock().unwrap() += 1; Ok(self.file(path).map(|f| RemoteStat { size: f.bytes.len() as u64, mode: f.mode, @@ -2256,3 +2280,177 @@ fn replacing_overwrites_a_published_binary_that_does_not_serve_us() { ); assert!(!release.fetched().is_empty(), "which means downloading it"); } + +/// The probe every pane used to pay for, and the note that spares the second +/// one — issue #695. See [`ProvedServer`]. +mod proving_the_server_once_per_connection { + use super::*; + + /// A warm machine: this build's server is installed and already serving. + /// Every pane after the first on a connection to it finds exactly this. + fn warm() -> FakeRemote { + FakeRemote::new().with_previous_install().serving(BINARY) + } + + fn prove(remote: &FakeRemote, user: &FakeUser, host: &str) -> io::Result { + let release = FakeRelease::new(); + Ok(ProvedServer::from_report( + installer(remote, &release, user, host).run()?, + )) + } + + /// The measurement the issue asks for, from the fake's own books: what the + /// first pane on a connection spends, and what the second one spends after + /// it. The chain is asserted by name rather than by count so that a probe + /// growing a step is a failure here and not a slow tab somewhere. + #[test] + fn the_second_pane_on_a_connection_spends_nothing() { + let remote = warm(); + let user = FakeUser::approving(); + let mut slot = None; + + let first = proved_or_prove(&mut slot, || prove(&remote, &user, "me@warm-box:22")) + .expect("the server is there and serving"); + assert_eq!(first, BINARY); + assert_eq!( + remote.execs(), + vec![ + "uname -sm".to_string(), + format!("{} --stdio --bridge < /dev/null", shell_quote(BINARY)), + RUNNING_EXE_COMMAND.to_string(), + ], + "the probe: what to install, is a daemon answering, and what build is serving" + ); + assert_eq!( + remote.round_trips(), + 5, + "three commands and two SFTP reads — the realpath for $HOME and the stat" + ); + + let paid = remote.round_trips(); + let second = proved_or_prove(&mut slot, || { + panic!("the second pane must not probe again"); + }) + .expect("the note answers"); + assert_eq!(second, BINARY); + assert_eq!( + remote.round_trips(), + paid, + "the second pane pays nothing for what the first one proved" + ); + } + + /// A transient failure must not pin every later pane on the connection into + /// the same failure. Nothing is written to the note unless the probe got + /// all the way through, so the next pane goes and asks again. + #[test] + fn a_probe_that_failed_is_not_remembered() { + let remote = FakeRemote::new(); + let user = FakeUser::declining(); + let mut slot = None; + + let refused = proved_or_prove(&mut slot, || prove(&remote, &user, "me@shy-box:22")) + .expect_err("the user said no"); + assert!( + format!("{refused}").contains("was not confirmed"), + "the refusal is the install prompt's, not something else: {refused}" + ); + assert_eq!(slot, None, "a failure leaves the slot exactly as it was"); + assert_eq!(user.asked().len(), 1); + + let _ = proved_or_prove(&mut slot, || prove(&remote, &user, "me@shy-box:22")); + assert_eq!( + user.asked().len(), + 2, + "the pane after a refusal asks again rather than inheriting the refusal" + ); + } + + /// The one thing memoizing could quietly cancel: the version check. The + /// probe is what notices that a different build is serving the machine, and + /// the note has to keep filing that warning for the panes that never run + /// the probe — each route drains its own sink, so a warning filed only once + /// would reach only the first pane's client. + #[test] + fn a_remembered_mismatch_is_filed_again_for_every_pane() { + let (remote, legacy) = FakeRemote::new().with_legacy_install("26.7.4"); + let remote = remote.serving(&legacy).speaking( + &legacy, + RemoteProtocol { + control: CONTROL - 1, + protocol: PROTOCOL, + build: "26.7.4".to_string(), + }, + ); + let user = FakeUser::approving(); + let mut slot = None; + + let first_route: Arc>> = Arc::new(Mutex::new(Vec::new())); + with_mismatch_sink(first_route.clone(), || { + proved_or_prove(&mut slot, || prove(&remote, &user, "me@old-box:22")) + .expect("an old daemon is kept, not a failure") + }); + assert_eq!( + first_route.lock().unwrap().len(), + 1, + "the probe found the mismatch" + ); + + let spent = remote.round_trips(); + let second_route: Arc>> = + Arc::new(Mutex::new(Vec::new())); + with_mismatch_sink(second_route.clone(), || { + proved_or_prove(&mut slot, || panic!("the note answers this one")).expect("remembered") + }); + + let filed = second_route.lock().unwrap().clone(); + assert_eq!(filed.len(), 1, "the second pane's client hears it too"); + assert_eq!(filed[0].running_version.as_deref(), Some("26.7.4")); + assert_eq!(filed[0].wanted_version, VERSION); + assert_eq!( + remote.round_trips(), + spent, + "and hears it without a round trip" + ); + } + + /// The wiring, over a real SSH connection: `ensure_remote_server` reads the + /// note off the connection it was handed, and `forget_remote_server` takes + /// it away again. The fake sshd counts session channels, so "no round trip" + /// is measured here rather than argued. + #[tokio::test] + async fn a_proved_connection_answers_the_next_pane_off_the_wire() { + use crate::daemon::ssh::test_support::{Exec, FakeSshd}; + + let sshd = FakeSshd::connect(Exec::Exits, None).await; + assert_eq!( + sshd.conn.remembered_server(), + None, + "a new link knows nothing" + ); + + *sshd.conn.proved_server() = Some(ProvedServer { + binary: BINARY.to_string(), + mismatch: None, + }); + assert_eq!( + ensure_remote_server(&sshd.conn).expect("the note answers"), + BINARY + ); + assert_eq!( + sshd.opened(), + 0, + "a proved connection opens no channel for the next pane" + ); + assert_eq!(sshd.conn.remembered_server().as_deref(), Some(BINARY)); + + // What `replace_remote_server`, `restart_remote_daemon` and a routed + // link that closed without answering all do before they act. + forget_remote_server(&sshd.conn); + assert_eq!( + sshd.conn.remembered_server(), + None, + "the next pane proves it again the long way" + ); + } +} diff --git a/crates/tty7-core/src/daemon/router.rs b/crates/tty7-core/src/daemon/router.rs index 4c759568..9585cc85 100644 --- a/crates/tty7-core/src/daemon/router.rs +++ b/crates/tty7-core/src/daemon/router.rs @@ -622,6 +622,25 @@ async fn drive(local: Stream, header: &RouteHeader) -> io::Result<()> { log::info!("wsl:{distro}: the bridge closed without answering; proving it again next time"); crate::daemon::install::wsl::forget_wsl_server(distro); } + // The same reasoning over SSH, where the note is the one this connection's + // probe left behind. `exec` on a session channel succeeds whatever the + // command turns out to be, so a server binary that has been deleted or + // moved since the probe proved it is discovered exactly here, by a link + // that opened and then said nothing. Forget it and the next pane on this + // connection pays for a fresh probe once; leave it and every pane on the + // connection repeats the same silent failure. + if let (RouteTarget::Ssh(_), Some(conn)) = (&header.target, conn.as_ref()) + && header.server_command.is_none() + && !copied + .as_ref() + .is_ok_and(|(_, from_remote)| *from_remote > 0) + { + log::info!( + "ssh {}: the routed link closed without answering; proving the server again next time", + conn.key().as_str(), + ); + crate::daemon::install::forget_remote_server(conn); + } let (to_remote, to_local) = copied?; log::debug!("routed connection closed after {to_remote} up / {to_local} down bytes"); diff --git a/crates/tty7-core/src/daemon/ssh/session.rs b/crates/tty7-core/src/daemon/ssh/session.rs index dd25437d..fe38f8fe 100644 --- a/crates/tty7-core/src/daemon/ssh/session.rs +++ b/crates/tty7-core/src/daemon/ssh/session.rs @@ -5,6 +5,7 @@ use std::sync::{Arc, Mutex, Weak}; use russh::client::Msg; use russh::{Channel, ChannelMsg}; +use crate::daemon::install::ProvedServer; use crate::daemon::protocol::WinSize; use crate::daemon::remote_link::RemoteEntry; @@ -230,6 +231,9 @@ pub struct SshConnection { remote_forwards: RemoteForwardTable, alive: AtomicBool, remote_entry: tokio::sync::Mutex>, + /// What this connection's server probe proved, once it has. See + /// [`SshConnection::proved_server`]. + proved_server: Mutex>, } impl SshConnection { @@ -244,6 +248,7 @@ impl SshConnection { remote_forwards, alive: AtomicBool::new(true), remote_entry: tokio::sync::Mutex::new(None), + proved_server: Mutex::new(None), }) } @@ -334,6 +339,38 @@ impl SshConnection { *self.remote_entry.lock().await = Some(entry); } + /// Where this connection's server was proved to be, and the lock that + /// makes the second pane wait for the first rather than prove it again. + /// + /// The memo lives on the connection rather than beside its key, and that + /// is the whole of the invalidation story for a reconnect: a dropped or + /// evicted link is a dropped `SshConnection`, and the one dialled in its + /// place starts with an empty slot. Nothing has to remember to forget. + /// What does have to remember is anything that changes the server *over + /// there* while the link stays up — see + /// [`crate::daemon::install::forget_remote_server`]. + /// + /// A blocking `Mutex` on purpose: the probe behind it is a chain of + /// blocking SSH round trips run on a blocking thread, and a pane that + /// arrives mid-install wants to wait for that install rather than start a + /// second one. + pub(crate) fn proved_server(&self) -> std::sync::MutexGuard<'_, Option> { + self.proved_server + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + + /// Where this connection's server was last proved to be, or `None` if the + /// next pane would have to go and ask — including while it is being asked, + /// since this never waits. A hint for callers deciding whether a failure is + /// worth re-proving; the answer itself comes from `ensure_remote_server`. + pub fn remembered_server(&self) -> Option { + self.proved_server + .try_lock() + .ok() + .and_then(|slot| slot.as_ref().map(|proved| proved.binary.clone())) + } + pub async fn add_remote_forward( &self, bind_host: &str,