perf(ssh): prove the remote server once per connection, not once per pane (#695)

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
This commit is contained in:
l0ng-ai
2026-09-09 17:54:45 +08:00
parent 59dbe83913
commit 8adebdf24d
4 changed files with 383 additions and 25 deletions
+129 -25
View File
@@ -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/<pid>/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<MismatchedRemoteDaemon>,
}
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<ProvedServer>,
prove: impl FnOnce() -> io::Result<ProvedServer>,
) -> io::Result<String> {
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<SshConnection>) -> io::Result<String> {
let host = connection_label(conn);
ensure_remote_server_labeled(conn, &host)
}
pub fn ensure_remote_server_labeled(conn: &Arc<SshConnection>, host: &str) -> io::Result<String> {
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<SshConnection>) -> io::Result<()> {
@@ -1379,6 +1477,11 @@ pub fn restart_remote_daemon(conn: &Arc<SshConnection>) -> 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<SshConnection>) -> 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(())
}
@@ -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<usize>,
}
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<String> {
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<String, String> {
*self.sftp_reads.lock().unwrap() += 1;
Ok(HOME.to_string())
}
@@ -290,6 +313,7 @@ impl RemoteOps for FakeRemote {
}
fn stat(&self, path: &str) -> Result<Option<RemoteStat>, 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<ProvedServer> {
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<Mutex<Vec<MismatchedRemoteDaemon>>> = 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<Mutex<Vec<MismatchedRemoteDaemon>>> =
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"
);
}
}
+19
View File
@@ -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");
@@ -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<Option<RemoteEntry>>,
/// What this connection's server probe proved, once it has. See
/// [`SshConnection::proved_server`].
proved_server: Mutex<Option<ProvedServer>>,
}
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<ProvedServer>> {
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<String> {
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,