fix(remote): review follow-ups on dialect-named servers

Six findings from a review pass over the two commits. One commit
because they are all corrections to the same change.

- The progress bar the `Restarting` phase was added for never appeared
  for the flow that needs it most. The switcher only reads a machine's
  phase while *this* window is connecting or showing an error, and
  "Restart Server…" in the machine's `⋯` menu is deliberately offered
  whatever the link is doing — so restarting a connected or offline
  machine recorded a phase nobody read, and the click had no visible
  effect for the length of two timeouts. A `Restarting` phase now
  counts on its own: it is only ever recorded by an action a user
  asked for, it ends every session on that machine including the ones
  other windows are showing, and it is the one flow that transfers
  nothing and so has no other way to say the click landed. The header
  says "restarting…" rather than borrowing "installing…", which
  described bytes that are not moving.

- "Restart Server" on the error card silently did nothing when the
  machine could no longer be addressed: `replace_remote_server` logged
  the failure and returned, past a prompt that had just promised the
  server would be replaced. It reports it, like every other failure on
  that path.

- `is_ssh` gates an action that ends every session on a machine, and
  spelled the three yes-variants as a `matches!` — so a new
  `RemoteTarget` would inherit "not SSH" by falling off the end of the
  pattern, which is the opposite of what its own test claims. An
  exhaustive `match` makes the compiler ask.

- The mismatch prompt's detail explains its buttons by name and the
  buttons were written out again at the prompt. `Keep Sessions` was
  one of them until this branch removed it, which is exactly the drift
  worth preventing twice: `MISMATCH_ANSWERS` is named once beside the
  detail, and a test asserts the detail explains every answer offered.

- `unique_temp`'s pid is private to a process, not to a client: two
  clients on different machines can share a pid. Left alone, because
  the new `--protocol` check stands behind it — interleaved bytes
  cannot answer with our dialect, so they are refused rather than
  published — but said, along with the fact that two installs inside
  one process share the path too and are the locks' job, not this
  function's.

- Ten doc sites still said the installed file is named after the
  version, including the module table's step 2, `client_version`'s
  "the server that matches me", `Installer::run`'s postcondition and
  `ensure_remote_server`'s returned path. Also corrected the claim
  that WSL and `LocalStdio` have no daemon to restart: WSL's is
  started by this client, which is why "stop it and reconnect" is the
  whole verb — the reason the router refuses them, not the absence of
  a daemon.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
thomas
2026-07-30 19:48:51 +08:00
co-authored by Claude Opus 5
parent 385d1d6536
commit d9ba7b6e23
10 changed files with 133 additions and 50 deletions
+3 -2
View File
@@ -23,8 +23,9 @@
//! [`HookTarget`] — the three facts that differ between machines: where `~` is,
//! which filesystem to write through, and which executable answers
//! `agent-hook`. Locally that is this binary; remotely it is the
//! `tty7-server-<version>` the installer published there, which carries the
//! same emitter for exactly this reason (`crates/tty7-server/src/main.rs`).
//! `tty7-server-c<control>p<protocol>` the installer published there, which
//! carries the same emitter for exactly this reason
//! (`crates/tty7-server/src/main.rs`).
use std::io;
use std::io::{IsTerminal as _, Read as _};
+20 -11
View File
@@ -298,18 +298,27 @@ impl RemoteTarget {
/// Whether this machine is reached over SSH.
///
/// The question "Restart Server" asks. The other two variants have no
/// long-lived daemon on the far side to restart: a WSL distribution's server
/// is started by this client, and a `LocalStdio` machine is a child process
/// per connection — which is why
/// [`router::restart_server`](crate::daemon::router) refuses them. Asked
/// here rather than re-spelled at each call site, so the UI that offers the
/// verb and the router that carries it out cannot disagree about who has it.
/// The question "Restart Server" asks, and the answer
/// [`router::restart_server`](crate::daemon::router) already gives: it routes
/// the action for SSH machines and refuses the other two. A `LocalStdio`
/// machine is a child process per connection, so there is nothing there to
/// stop and start; a WSL distribution's server is started by this client,
/// which makes "stop it and reconnect" the whole of the verb and not
/// something a routed action has to carry out. Asked here rather than
/// re-spelled at each call site, so the UI that offers the verb and the
/// router that carries it out cannot disagree about who has it.
///
/// Spelled out variant by variant rather than as a `matches!` of the three
/// that say yes: this gates an action that ends every session on a machine,
/// and a new [`RemoteTarget`] must not inherit an answer to that by falling
/// off the end of a pattern. The compiler asks instead.
pub fn is_ssh(&self) -> bool {
matches!(
self,
RemoteTarget::Profile { .. } | RemoteTarget::Alias { .. } | RemoteTarget::Direct { .. }
)
match self {
RemoteTarget::Profile { .. }
| RemoteTarget::Alias { .. }
| RemoteTarget::Direct { .. } => true,
RemoteTarget::Wsl { .. } | RemoteTarget::LocalStdio { .. } => false,
}
}
/// The in-process id this target resolves to.
+34 -16
View File
@@ -1,14 +1,14 @@
//! Installing, launching and version-matching `tty7-server` on a remote machine.
//! Installing, launching and dialect-matching `tty7-server` on a remote machine.
//!
//! The six steps, in order:
//!
//! | | Step | Where |
//! |---|---|---|
//! | 1 | `uname -sm` → the release asset that runs there | [`asset::asset_for_uname`] |
//! | 2 | SFTP-stat `~/.local/share/tty7/bin/tty7-server-<client version>` | [`Installer::run`] |
//! | 2 | SFTP-stat `~/.local/share/tty7/bin/tty7-server-c<control>p<protocol>` | [`Installer::run`] |
//! | 3 | absent → download the asset **on the client** + sha256-verify it | [`download`], [`checksums`] |
//! | 4 | SFTP-put into `bin/.tty7-server-<ver>.tmp` | [`RemoteOps::put`] |
//! | 5 | `chmod 0755` then `rename` — atomic publish | [`RemoteOps::rename`] |
//! | 4 | SFTP-put into `bin/.tty7-server-c<c>p<p>.<pid>.tmp` | [`RemoteOps::put`] |
//! | 5 | `chmod 0755`, `--protocol` to earn the name, then `rename` — atomic publish | [`RemoteOps::rename`] |
//! | 6 | probe the remote control socket; nothing there → launch a detached daemon | [`Installer::ensure_daemon`] |
//!
//! ## Why the client downloads
@@ -60,9 +60,13 @@ pub use checksums::ChecksumError;
use crate::daemon::ssh::SshConnection;
/// The client version, which is also the version of the server it installs.
/// Client and server ship from the same workspace version, so "the server that
/// matches me" is always `tty7-server-<this>`.
/// The client version, which is also the version of the server it installs
/// client and server ship from the same workspace version.
///
/// Names the release to download and labels this client in a prompt, and that is
/// all it may be used for. **"Which server matches me" is a question about
/// dialects**, not about this string; two builds between releases share it and
/// need not speak to each other. See [`asset::binary_name`].
pub fn client_version() -> &'static str {
env!("CARGO_PKG_VERSION")
}
@@ -1162,8 +1166,10 @@ impl<'a> Installer<'a> {
self
}
/// The whole flow. On `Ok`, the machine has `tty7-server-<version>` installed
/// and a daemon answering on its control socket.
/// The whole flow. On `Ok`, a `tty7-server` this client can speak to is
/// answering on the machine's control socket — either the one published at
/// `tty7-server-c<control>p<protocol>`, or one that was already running and
/// said it speaks our dialects.
pub fn run(&self) -> Result<InstallReport, InstallError> {
// --- 1. uname -sm --------------------------------------------------
let uname = self
@@ -1653,6 +1659,17 @@ fn launch_script(binary: &str, settle: Option<String>) -> String {
/// `rename`) is the price of the collision it prevents, and it is bounded by how
/// often that happens — which is "almost never", against "every time two clients
/// install the same dialect at once" for the shared name.
///
/// **A pid, so private to a process and not to a client.** Two tty7 processes on
/// one machine (the released build and the one you are compiling) cannot collide;
/// two on *different* machines that happen to share a pid still can. That
/// remainder is left alone because the `--protocol` check now stands behind it:
/// bytes from two uploads interleaved into one file do not answer with our
/// dialect, so the outcome is a [`InstallError::DialectMismatch`] and a removed
/// temp rather than a published binary that lies. Two installs from *within* one
/// process share a pid and so share this path too — that is what `wsl`'s
/// `INSTALL_LOCKS` and `SshManager`'s per-key `ConnSlot` are for, and this is not
/// a second attempt at their job.
fn unique_temp(shared: &str) -> String {
let pid = std::process::id();
match shared.strip_suffix(".tmp") {
@@ -1695,14 +1712,15 @@ fn connection_label(conn: &SshConnection) -> String {
/// binary plus a live daemon costs two SSH commands and one SFTP stat, no
/// download, no prompt.
///
/// The returned path is **absolute and version-qualified**
/// (`~/.local/share/tty7/bin/tty7-server-<version>`), and the session-channel
/// fallback must use it rather than the bare name. Nothing puts that directory
/// on a non-interactive `PATH`, and the file is not even called `tty7-server` —
/// so `exec tty7-server --stdio` is a `command not found` on a machine where the
/// install just succeeded.
/// The returned path is **absolute and never the bare name**
/// (`~/.local/share/tty7/bin/tty7-server-c<control>p<protocol>`, or the path of a
/// server already running there that answered with our dialects), and the
/// session-channel fallback must use it rather than the bare name. Nothing puts
/// that directory on a non-interactive `PATH`, and the file is not even called
/// `tty7-server` — so `exec tty7-server --stdio` is a `command not found` on a
/// machine where the install just succeeded.
///
/// A version mismatch is *not* an error: an older daemon still owns every live
/// A dialect mismatch is *not* an error: an older daemon still owns every live
/// pane on that machine, so it keeps serving and the mismatch is recorded for
/// [`take_mismatched_remote_daemons`] to raise. Only a machine we cannot install
/// on, cannot verify a download for, or cannot get a daemon running on fails.
+1 -1
View File
@@ -519,7 +519,7 @@ fn first_install_runs_all_six_steps() {
/// **Atomic replacement.** The final path must only ever be produced by
/// renaming a temp that is *already* executable — never written to directly,
/// and never chmod'ed after it is visible. Both would leave a window in which a
/// concurrent connect finds `tty7-server-<ver>` present and unusable.
/// concurrent connect finds `tty7-server-c<c>p<p>` present and unusable.
#[test]
fn the_final_path_is_only_ever_reached_by_renaming_a_ready_temp() {
let remote = FakeRemote::new();
+2 -1
View File
@@ -891,7 +891,8 @@ impl ServerBinarySource for BundledServerBinary {
/// opens one routed connection per pane, all at once, on separate daemon
/// threads. Without this they run the installer concurrently against the same
/// distribution, and the interleaving is destructive rather than merely wasteful:
/// two runs both write `.tty7-server-<ver>.tmp`, the first renames it into place
/// two runs in *this* process share a pid and so share
/// `.tty7-server-c<c>p<p>.<pid>.tmp`, the first renames it into place
/// and reports success, and the second's rename then fails — which sends it down
/// [`Installer::install`]'s recovery branch, whose `remove_file(&paths.binary)`
/// **deletes the binary the first run just published**. Every later pane then
+2 -2
View File
@@ -251,9 +251,9 @@ fn spawn_stdio_owned(program: &str, args: &[String]) -> io::Result<ProcessStream
///
/// **Only a fallback for links that skip the install pass.** SSH links do not:
/// `SshManager::open_remote_link` runs `install::ensure_remote_server` first and
/// uses the absolute, version-qualified path it returns. That matters because
/// uses the absolute, dialect-qualified path it returns. That matters because
/// nothing puts `~/.local/share/tty7/bin` on a non-interactive `PATH`, and the
/// file there is `tty7-server-<version>` — this bare name would be a
/// file there is `tty7-server-c<control>p<protocol>` — this bare name would be a
/// `command not found` on a machine the install had just succeeded on.
/// [`super::router::RouteHeader::server_command`] overrides either.
pub const DEFAULT_REMOTE_SERVER_CMD: &str = "tty7-server --stdio";
+3 -2
View File
@@ -436,8 +436,9 @@ impl SshManager {
// The installed binary's **absolute** path, not the bare name. Nothing
// puts `~/.local/share/tty7/bin` on a non-interactive `PATH`, and the
// file there is `tty7-server-<version>` — so `exec tty7-server --stdio`
// is a `command not found` on a machine the install just succeeded on.
// file there is `tty7-server-c<control>p<protocol>` — so
// `exec tty7-server --stdio` is a `command not found` on a machine the
// install just succeeded on.
// The install pass we just ran is what knows the path, so it hands it
// over rather than leaving the transport to guess.
let base = match server_command {
+31 -2
View File
@@ -495,8 +495,9 @@ pub fn connect_blocking(
/// Machines whose agent hooks this process has already looked at.
static HOOKS_REFRESHED: Mutex<Vec<HostId>> = Mutex::new(Vec::new());
/// Heal this machine's stale tty7 agent hooks — the ones pointing at a
/// `tty7-server-<version>` an upgrade replaced (see
/// Heal this machine's stale tty7 agent hooks — the ones naming a server binary
/// that is no longer the one this client installs, whether because a wire break
/// moved the name or because an older, version-naming client wrote them (see
/// [`crate::core::agent_hooks::refresh_remote_hooks`]).
///
/// Off the connect's own thread, and once per machine per run: it is a config
@@ -1059,6 +1060,16 @@ pub(crate) fn claim_mailbox() -> std::sync::MutexGuard<'static, ()> {
// 7. Remote daemon version skew
// ---------------------------------------------------------------------------
/// The answers the dialect-mismatch prompt offers, in the order `window.prompt`
/// takes them — **index 1 is the destructive one**, which is what
/// `prompt_remote_daemon_mismatch` matches on.
///
/// Written down here rather than at the prompt because [`mismatch_detail`] spells
/// both out by name in its body: a detail explaining a button that is no longer
/// there is worse than no explanation at all. `Keep Sessions` used to be index 0
/// and had to go, which is precisely the drift this prevents repeating.
pub const MISMATCH_ANSWERS: [&str; 2] = ["Cancel", "Restart Server"];
/// The restart-or-cancel question for a remote `tty7-server` this client cannot
/// talk to.
///
@@ -1404,6 +1415,24 @@ mod tests {
assert!(mismatch_detail(&unknown).contains("an unknown build"));
}
/// The detail explains the buttons by name, so it has to name the ones that
/// are actually there. This is a prompt whose whole job is to make a
/// destructive choice legible; a body describing an answer the prompt does
/// not offer (as it did while `Keep Sessions` was one of them) turns that
/// back into a guess.
#[test]
fn the_mismatch_detail_explains_every_answer_the_prompt_offers() {
let detail = mismatch_detail(&MismatchedRemoteDaemon {
host: "me@build-box:22".into(),
running_version: Some("0.8.0".into()),
running_exe: None,
wanted_version: "0.9.1".into(),
});
for answer in MISMATCH_ANSWERS {
assert!(detail.contains(answer), "{answer} is unexplained: {detail}");
}
}
#[test]
fn endpoint_labels_hide_the_default_port() {
assert_eq!(endpoint_label("me", "box.local", 22), "me@box.local");
+11 -3
View File
@@ -836,7 +836,8 @@ impl Tty7App {
PromptLevel::Warning,
&title,
Some(&detail),
&["Cancel", "Restart Server"],
// Named once, beside the detail that explains them.
&remote_connect::MISMATCH_ANSWERS,
cx,
);
cx.spawn(async move |this, cx| {
@@ -1017,8 +1018,8 @@ impl Tty7App {
if !matches!(answer.await, Ok(1)) {
return;
}
let _ = this.update_in(cx, |this, _window, cx| {
this.replace_remote_server(target, label, cx);
let _ = this.update_in(cx, |this, window, cx| {
this.replace_remote_server(target, label, window, cx);
});
})
.detach();
@@ -1030,12 +1031,19 @@ impl Tty7App {
&mut self,
target: RemoteTarget,
label: String,
window: &mut Window,
cx: &mut Context<Self>,
) {
let route = match remote_connect::control_route(&target, cx) {
Ok(header) => header.replace_server(),
// Said out loud, for the reason every other failure on this path is:
// the user answered a prompt that promised the machine's server
// would be replaced, and a log line is not an answer to that. The
// failure this catches — no route to the machine any more — is one
// where nothing was touched, which the wording already allows for.
Err(e) => {
log::warn!("could not address {label} to replace its server: {e}");
Tty7App::report_restart_failure(&label, &e, window, cx);
return;
}
};
+26 -10
View File
@@ -452,13 +452,24 @@ impl Tty7App {
// install is its own business, and a bar under a row this panel is
// not driving would have no "Try Again" to turn into.
//
// `error` counts too, and is not an exception to that: it is the
// state a machine is in while the error card.s "Restart Server" — a
// exists inside this panel's own error card — is working on it. Left
// out, the one flow that transfers nothing would show nothing at all
// for the length of two timeouts.
if group.link == Link::Connecting || group.error.is_some() {
group.installing = remote_connect::install_progress_for(id);
// `error` counts too, and is not an exception to that: a machine
// being worked on by the "Restart Server" inside this panel's own
// error card is still a machine this panel is driving.
//
// A restart is the exception, and has to be. It is offered from the
// machine's `⋯` menu whatever the link is doing — a server worth
// restarting is most often one nothing can reach — so gating it on
// *this* window's connect would hide the bar in the ordinary case.
// And it is the one flow that transfers nothing, which makes the bar
// the only thing that says the click landed, for the length of two
// timeouts. Showing another window's restart is right rather than
// merely tolerable: it is about to end the sessions in this one too.
let reported = remote_connect::install_progress_for(id);
if group.link == Link::Connecting
|| group.error.is_some()
|| matches!(reported, Some(InstallPhase::Restarting))
{
group.installing = reported;
}
// Read app-wide, not from this window's snapshot: any window's
// connect, and every reconnect, records the machine's `$HOME` — and
@@ -1133,7 +1144,11 @@ impl Tty7App {
Link::Connected => (Some(gpui::rgb(crate::ui::tab_strip::LIVE_DOT).into()), None),
// "installing…" while bytes are moving: the bar underneath says how
// far along, and a header still reading "connecting…" over it would
// describe a step that finished a while ago.
// describe a step that finished a while ago. A restart moves no
// bytes, so it gets its own word rather than borrowing that one.
Link::Connecting if matches!(group.installing, Some(InstallPhase::Restarting)) => {
(Some(theme.warning), Some("restarting…"))
}
Link::Connecting if group.installing.is_some() => {
(Some(theme.warning), Some("installing…"))
}
@@ -1648,8 +1663,9 @@ fn group_menu(
);
if !restartable {
// A WSL distribution's server is started by this client and a
// `LocalStdio` one is a child process per connection, so there is no
// daemon over there to restart — the router says the same. Absent rather
// `LocalStdio` one is a child process per connection, so neither has a
// daemon a routed action could restart — the router refuses both, and
// `RemoteTarget::is_ssh` is where the two agree. Absent rather
// than greyed out, for the reason "Disconnect" is absent from the local
// group: a permanently disabled row only invites the question.
return menu;