From 66fcc95e2e94c24cb64bf7f80644f94c171dbebb Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Thu, 30 Jul 2026 19:54:40 +0800 Subject: [PATCH] fix(install): name remote servers by dialect, and prove the name before publishing (#264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(install): name remote servers by dialect, and prove the name before publishing A remote tty7-server was installed as `tty7-server-`, and one stat of that path decided whether anything got installed at all. The version string is not an identity: every build between releases carries the same one, so a client speaking control v3 found the released v2 binary sitting at the exact path it looks for, skipped the install, adopted a server it cannot talk to, and died in the handshake with no way out. `check_running_build` compared the same string and so never even probed. Name the file after the two numbers that actually decide compatibility — `tty7-server-cp` — and after uploading, ask the binary what it speaks and publish it only if the answer matches the name it is about to get. That turns "the filename is the dialect" from a convention into a fact a stale bundle or an older release tag cannot break, and surfaces an architecture mismatch at install time rather than as `Exec format error` inside a later daemon launch. The keep-or-restart prompt offered "Keep Sessions" for a state where keeping means the connect fails a moment later; it now offers Cancel. A handshake that does fail gets a Restart Server button, gated on that one error so no other connect failure can reach a destructive action that would not fix it. * fix(remote): offer "Restart Server" only where there is one, and keep a torn-down pane's resume record `RemoteTarget::is_ssh` answers the question the verb depends on, in one place rather than re-spelled at each call site: a WSL distribution's server is started by this client and a `LocalStdio` machine is a child process per connection, so neither has a long-lived daemon to stop and start — which is exactly why `router::restart_server` refuses them. The UI that offers the action and the router that carries it out can no longer disagree about who has one. "Restart Daemon…" now asks the window which daemon serves it. Run from a remote window it used to end every local session in every other window and leave the machine in front of the user untouched — a destructive button that did nothing its label promised. A pane torn down with its coding agent no longer publishes an observation. The kill hangs up the whole process group, so the agent dies before the PTY EOFs and a poll firing on the bytes still in flight sees nothing recognizable; publishing that cleared the record's agent and session id, and the reopened workspace came back to a bare shell instead of the conversation. The same answer about a pane nobody is tearing down still means the agent exited on its own. * 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) --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Co-authored-by: thomas Co-authored-by: Claude Opus 5 (1M context) --- crates/tty7-core/src/core/agent_hooks.rs | 33 +- crates/tty7-core/src/core/machine.rs | 8 + crates/tty7-core/src/core/session.rs | 62 +++ crates/tty7-core/src/daemon/control.rs | 60 ++- crates/tty7-core/src/daemon/install/asset.rs | 153 ++++--- crates/tty7-core/src/daemon/install/mod.rs | 340 +++++++++++++-- crates/tty7-core/src/daemon/install/tests.rs | 415 +++++++++++++++++-- crates/tty7-core/src/daemon/install/wsl.rs | 28 +- crates/tty7-core/src/daemon/pane.rs | 106 ++++- crates/tty7-core/src/daemon/remote_link.rs | 4 +- crates/tty7-core/src/daemon/router.rs | 65 ++- crates/tty7-core/src/daemon/ssh/mod.rs | 25 +- src/ui/app.rs | 61 ++- src/ui/remote_connect.rs | 75 +++- src/ui/remote_workspace.rs | 228 +++++++++- src/ui/settings.rs | 2 +- src/ui/switcher.rs | 211 +++++++--- 17 files changed, 1597 insertions(+), 279 deletions(-) diff --git a/crates/tty7-core/src/core/agent_hooks.rs b/crates/tty7-core/src/core/agent_hooks.rs index aa517f36..ce62a24e 100644 --- a/crates/tty7-core/src/core/agent_hooks.rs +++ b/crates/tty7-core/src/core/agent_hooks.rs @@ -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-` the installer published there, which carries the -//! same emitter for exactly this reason (`crates/tty7-server/src/main.rs`). +//! `tty7-server-cp` 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 _}; @@ -475,7 +476,7 @@ impl<'a> HookTarget<'a> { } /// A remote machine: the `$HOME` its handshake reported, and the - /// `tty7-server-` this client published into it. + /// `tty7-server-cp` this client published into it. /// /// The *installed* binary, not the one the running daemon was launched /// from. The two can differ — a user who kept an older daemon alive rather @@ -483,10 +484,16 @@ impl<'a> HookTarget<'a> { /// a one-shot child of the agent that writes an escape sequence to its own /// tty and exits. It never talks to the daemon, so the binary only has to /// exist, and the one this client installed is the one it can prove does. + /// + /// Naming it stays a pure function of `home` because the name is built from + /// this client's own dialect numbers — nothing has to be asked of the remote + /// to know what tty7 called the file it put there. pub fn remote(host: &'a dyn Host, home: PathBuf) -> HookTarget<'a> { + let dialect = crate::daemon::install::RemoteProtocol::of_this_build(); let binary = crate::daemon::install::asset::remote_paths( &home.to_string_lossy(), - crate::daemon::install::client_version(), + dialect.control, + dialect.protocol, ) .binary; HookTarget { @@ -1564,19 +1571,18 @@ mod tests { } /// The hook a remote machine runs is the binary that lives *there*. The - /// local exe path is meaningless over there, and the version is in the - /// server binary's filename — which is what makes a server upgrade leave - /// hooks pointing at a path that no longer exists (see [`refresh_hooks`]). + /// local exe path is meaningless over there, and the dialects are in the + /// server binary's filename — which is what makes a wire break leave hooks + /// pointing at a path that no longer exists (see [`refresh_hooks`]). #[test] fn the_hook_command_names_the_binary_on_that_machine() { let host = FakeRemote::shared(); let target = HookTarget::remote(&*host, PathBuf::from("/home/me")); - let version = crate::daemon::install::client_version(); + let dialect = crate::daemon::install::RemoteProtocol::of_this_build(); + let name = format!("tty7-server-c{}p{}", dialect.control, dialect.protocol); assert_eq!( target.hook_command(HookAgent::Claude, "stop"), - format!( - "\"/home/me/.local/share/tty7/bin/tty7-server-{version}\" agent-hook claude stop" - ) + format!("\"/home/me/.local/share/tty7/bin/{name}\" agent-hook claude stop") ); // And locally it is still this process's own executable. @@ -1607,9 +1613,10 @@ mod tests { assert_eq!(hooks_state(&target, agent), HooksState::Installed); // The file really is there, under the remote-shaped path. let path = agent.target_path(&target); + let dialect = crate::daemon::install::RemoteProtocol::of_this_build(); assert!(std::fs::read_to_string(&path).unwrap().contains(&format!( - "tty7-server-{}", - crate::daemon::install::client_version() + "tty7-server-c{}p{}", + dialect.control, dialect.protocol ))); uninstall_hooks(&target, agent).expect("uninstall succeeds"); assert_eq!(hooks_state(&target, agent), HooksState::NotInstalled); diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index d4f636c8..881920c6 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -1690,6 +1690,13 @@ pub(crate) fn withdraw_observations() { *OBSERVED.lock().unwrap_or_else(|e| e.into_inner()) = None; } +/// Test-only: [`OBSERVED`] is one slot for the whole process, so a test that +/// installs a store must hold this for as long as it needs its observations to +/// land there — otherwise a test elsewhere in the binary withdraws the store +/// mid-run and the observation is silently dropped. +#[cfg(test)] +pub(crate) static OBSERVE_SLOT: Mutex<()> = Mutex::new(()); + /// Copy a file we are about to stop honouring somewhere the user can find it. fn quarantine(path: &Path) { let aside = quarantine_path(path); @@ -2398,6 +2405,7 @@ mod tests { /// unconditionally. #[test] fn published_observations_land_in_the_installed_store() { + let _slot = OBSERVE_SLOT.lock().unwrap_or_else(|e| e.into_inner()); observe_pane(1, |p| p.cwd = Some("/nowhere".into())); let (store, _dir, _ws, _tab) = store_with_tab(); diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index bd3530f2..97fc4ec1 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -296,6 +296,31 @@ impl RemoteTarget { } } + /// Whether this machine is reached over SSH. + /// + /// 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 { + match self { + RemoteTarget::Profile { .. } + | RemoteTarget::Alias { .. } + | RemoteTarget::Direct { .. } => true, + RemoteTarget::Wsl { .. } | RemoteTarget::LocalStdio { .. } => false, + } + } + /// The in-process id this target resolves to. /// /// This is the **only** bridge between the persisted world and the runtime @@ -713,6 +738,43 @@ mod tests { ); } + /// Which machines can be told to restart their server. The two that cannot + /// are not an omission: their server is this client's own doing, so there is + /// nothing on the far side to stop and start, and the router refuses the + /// action for exactly the same reason. A new variant has to answer this + /// question rather than inherit an answer. + #[test] + fn only_ssh_machines_have_a_server_to_restart() { + assert!( + RemoteTarget::Profile { + id: uuid::Uuid::nil() + } + .is_ssh() + ); + assert!( + RemoteTarget::Alias { + alias: "devbox".into() + } + .is_ssh() + ); + assert!(RemoteTarget::direct("me", "box.local", 22).is_ssh()); + assert!( + !RemoteTarget::Wsl { + distro: "Ubuntu".into() + } + .is_ssh(), + "a distribution's server is started by this client" + ); + assert!( + !RemoteTarget::LocalStdio { + program: "tty7-server".into(), + args: vec!["--stdio".into()], + } + .is_ssh(), + "a stdio machine is a child process per connection" + ); + } + #[test] fn direct_targets_normalize_and_reuse_the_quick_connect_parser() { // The port defaults to 22, the scheme is optional, and the host folds diff --git a/crates/tty7-core/src/daemon/control.rs b/crates/tty7-core/src/daemon/control.rs index 0cf54f01..d5e39c05 100644 --- a/crates/tty7-core/src/daemon/control.rs +++ b/crates/tty7-core/src/daemon/control.rs @@ -118,6 +118,35 @@ use super::protocol::{MAX_FRAME, read_frame, write_frame}; /// - **v1** — the dialect at the time remote workspaces landed. pub const CONTROL_VERSION: u32 = 3; +/// The phrase every control-dialect refusal contains. +/// +/// Written down once so the message and the test that recognises it cannot +/// drift apart. See [`is_dialect_refusal`]. +const DIALECT_MARKER: &str = "speaks control v"; + +/// The message a handshake between two incompatible control dialects fails +/// with. +fn dialect_refusal(peer_build: &str, peer: u32, ours: u32) -> String { + format!("control peer (build {peer_build}) {DIALECT_MARKER}{peer}, this build speaks v{ours}") +} + +/// Whether an error message is a control-dialect refusal rather than one of the +/// dozen other reasons a connect fails (a dead host, a bad key, a refused +/// port). +/// +/// A marker in the text rather than a typed error because this message crosses +/// two process boundaries — the daemon's route ack, then the GUI's error card — +/// as a `String`, and rebuilding a type on the far side of that would be more +/// machinery than the one question needs. +/// +/// The question is worth asking because the answer is unusually specific: a +/// dialect refusal is the *only* connect failure that a reinstall on the far end +/// fixes, and the only one where offering that (destructive) action is +/// justified. Everything else it must not offer it for. +pub fn is_dialect_refusal(message: &str) -> bool { + message.contains(DIALECT_MARKER) +} + /// This process's identity as a control server, minted once on first use. /// /// Answers "am I still talking to the same server?" — the question no other @@ -1538,10 +1567,7 @@ impl ControlClient { // connection dropped". return Err(io::Error::new( io::ErrorKind::Unsupported, - format!( - "control peer (build {}) speaks control v{}, this build speaks v{}", - ok.build, ok.control_version, hello.control_version - ), + dialect_refusal(&ok.build, ok.control_version, hello.control_version), )); } @@ -3493,6 +3519,32 @@ mod tests { msg.contains(&format!("v{CONTROL_VERSION}")), "names ours: {msg}" ); + assert!( + is_dialect_refusal(&msg), + "and the GUI can tell this apart from an unreachable host, which is what \ + decides whether it may offer to replace the far end's binary: {msg}" + ); + } + + /// **Only a dialect refusal is recognised as one.** + /// + /// [`is_dialect_refusal`] gates a destructive offer — reinstalling the + /// server on someone's machine and dropping every pane on it. A connect that + /// failed for any other reason must never reach that button, because + /// replacing the binary would not fix it and would cost the user their + /// sessions to find out. + #[test] + fn other_connect_failures_are_not_dialect_refusals() { + for other in [ + "Connection refused (os error 61)", + "ssh: handshake failed: no matching key exchange method", + "the remote tty7-server did not start: exit status 127", + "could not resolve the remote home directory: Permission denied", + "control peer answered the handshake with Bye instead of HELLO_OK", + ] { + assert!(!is_dialect_refusal(other), "{other}"); + } + assert!(is_dialect_refusal(&dialect_refusal("26.7.6", 2, 3))); } /// A peer that answers the handshake with something other than `HELLO_OK` diff --git a/crates/tty7-core/src/daemon/install/asset.rs b/crates/tty7-core/src/daemon/install/asset.rs index adaa30e3..1dbff30d 100644 --- a/crates/tty7-core/src/daemon/install/asset.rs +++ b/crates/tty7-core/src/daemon/install/asset.rs @@ -148,7 +148,7 @@ pub fn download_url(tag: &str, asset: &str) -> String { format!("{RELEASE_BASE}/{tag}/{asset}") } -/// Absolute remote paths for one client version's server binary. +/// Absolute remote paths for one *dialect*'s server binary. /// /// Built with explicit `/` joins from an absolute `$HOME` the remote resolved for /// us (SFTP does not expand `~`, and `PathBuf::join` would emit `\` on a Windows @@ -157,25 +157,27 @@ pub fn download_url(tag: &str, asset: &str) -> String { pub struct RemotePaths { /// `$HOME/.local/share/tty7/bin`. pub bin_dir: String, - /// `$HOME/.local/share/tty7/bin/tty7-server-` — the atomically - /// published binary. The version is *in the path* so two clients of different - /// versions can coexist on one machine; only the running daemon is singular. + /// `$HOME/.local/share/tty7/bin/tty7-server-cp` — the + /// atomically published binary. See [`binary_name`] for why the dialects, and + /// not the version, are what the name carries. pub binary: String, - /// `$HOME/.local/share/tty7/bin/.tty7-server-.tmp` — where the bytes - /// land before `chmod` + `rename`. + /// `$HOME/.local/share/tty7/bin/.tty7-server-cp.tmp` — + /// where the bytes land before `chmod`, the `--protocol` check, and `rename`. /// /// A dotfile, so a half-written upload is not mistaken for an installed - /// server by anything globbing the directory, and a *fixed* name per version - /// so an install killed mid-upload leaves one reusable file behind instead of - /// accumulating random-suffixed litter on someone else's disk. + /// server by anything reading the directory. The installer adds a per-process + /// suffix (`super::unique_temp`) before writing: one file per dialect means + /// two clients installing the same dialect at once would otherwise interleave + /// their bytes into one name. pub temp: String, /// Every directory that must exist before the upload, outermost first. SFTP /// has no recursive mkdir, so the installer walks this. pub dir_chain: Vec, } -/// Build the remote paths for `version` under an absolute remote `home`. -pub fn remote_paths(home: &str, version: &str) -> RemotePaths { +/// Build the remote paths for a server speaking `control`/`protocol` under an +/// absolute remote `home`. +pub fn remote_paths(home: &str, control: u32, protocol: u32) -> RemotePaths { let home = home.trim_end_matches('/'); let mut dir_chain = Vec::with_capacity(INSTALL_DIR_COMPONENTS.len()); let mut cursor = home.to_string(); @@ -184,17 +186,35 @@ pub fn remote_paths(home: &str, version: &str) -> RemotePaths { dir_chain.push(cursor.clone()); } let bin_dir = cursor; + let name = binary_name(control, protocol); RemotePaths { - binary: format!("{bin_dir}/{}", binary_name(version)), - temp: format!("{bin_dir}/.tty7-server-{version}.tmp"), + binary: format!("{bin_dir}/{name}"), + temp: format!("{bin_dir}/.{name}.tmp"), dir_chain, bin_dir, } } -/// The filename a `version`'s server binary is installed under. -pub fn binary_name(version: &str) -> String { - format!("tty7-server-{version}") +/// The filename a server speaking `control`/`protocol` is installed under. +/// +/// **The dialects are the name, and the version is nowhere in it.** Everything +/// the installer decides — is there something usable here, can the daemon that +/// is running talk to us — is a question about dialects, and a name built from +/// them answers it with a `stat` the client can address without asking the +/// remote anything. A name built from the version answers a *different* +/// question, and answers this one wrong in both directions: two builds that +/// share a version string but not a dialect (any two dev builds between +/// releases) look interchangeable, and two builds that share a dialect but not a +/// version look incompatible and cost an 8 MB upload that changes nothing. +/// +/// One file per dialect, so a machine accumulates at most one binary per wire +/// break rather than one per release. Which *build* is sitting behind a given +/// dialect is a separate question, answered by [`PROTOCOL_FLAG`][flag] and by +/// the control handshake — not by the filename. +/// +/// [flag]: super::PROTOCOL_FLAG +pub fn binary_name(control: u32, protocol: u32) -> String { + format!("tty7-server-c{control}p{protocol}") } /// [`RemotePaths`] pointing at a binary that is **already on the machine**, @@ -210,27 +230,33 @@ pub fn binary_name(version: &str) -> String { /// where a later install would write. Nothing writes anything on the adoption /// path, so they are unused there; keeping them well-formed means a caller that /// falls back to installing does not need a second `RemotePaths`. -pub fn remote_paths_for_binary(home: &str, binary: &str) -> RemotePaths { - let version = version_from_path(binary); - let mut paths = remote_paths(home, version.as_deref().unwrap_or("unknown")); +pub fn remote_paths_for_binary( + home: &str, + binary: &str, + control: u32, + protocol: u32, +) -> RemotePaths { + let mut paths = remote_paths(home, control, protocol); paths.binary = binary.to_string(); paths } -/// The version encoded in an installed binary's *path*, if it is one of ours. +/// The dialects encoded in an installed binary's *path*, if it is one of ours. /// -/// This is how the running daemon's build is identified without asking it: the -/// install path carries the version by construction, so `readlink /proc//exe` -/// on the remote answers "which tty7-server is serving this machine" for every -/// build we have ever shipped — including ones older than any handshake we could -/// send them. -pub fn version_from_path(path: &str) -> Option { +/// This is how the running daemon is identified without asking it: the install +/// path carries the dialects by construction, so `readlink /proc//exe` on +/// the remote answers "can the thing serving this machine talk to us" in the +/// round trip that found it. +/// +/// `None` for anything else, and that deliberately includes every binary +/// installed by a client that named files after versions: an old name carries no +/// dialect, so it gets no opinion, and the probe (`--protocol`) is what settles +/// it. Guessing a dialect from a version string is the exact inference this +/// naming exists to make impossible. +pub fn dialect_from_path(path: &str) -> Option<(u32, u32)> { let name = path.rsplit('/').next()?; - let rest = name.strip_prefix("tty7-server-")?; - if rest.is_empty() { - return None; - } - Some(rest.to_string()) + let (control, protocol) = name.strip_prefix("tty7-server-c")?.split_once('p')?; + Some((control.parse().ok()?, protocol.parse().ok()?)) } #[cfg(test)] @@ -369,16 +395,13 @@ mod tests { /// what `PathBuf::join` would produce on a Windows client) would create a file /// named `.local\share\tty7\bin` in the remote home directory. #[test] - fn remote_paths_are_posix_and_versioned() { - let p = remote_paths("/home/me", "26.7.5"); + fn remote_paths_are_posix_and_named_by_dialect() { + let p = remote_paths("/home/me", 3, 4); assert_eq!(p.bin_dir, "/home/me/.local/share/tty7/bin"); - assert_eq!( - p.binary, - "/home/me/.local/share/tty7/bin/tty7-server-26.7.5" - ); + assert_eq!(p.binary, "/home/me/.local/share/tty7/bin/tty7-server-c3p4"); assert_eq!( p.temp, - "/home/me/.local/share/tty7/bin/.tty7-server-26.7.5.tmp" + "/home/me/.local/share/tty7/bin/.tty7-server-c3p4.tmp" ); assert_eq!( p.dir_chain, @@ -400,7 +423,7 @@ mod tests { /// mistaken for an installed server. #[test] fn temp_path_is_a_hidden_sibling_of_the_binary() { - let p = remote_paths("/home/me", "26.7.5"); + let p = remote_paths("/home/me", 3, 4); let dir = |s: &str| s.rsplit_once('/').unwrap().0.to_string(); assert_eq!(dir(&p.temp), dir(&p.binary)); assert!(p.temp.rsplit('/').next().unwrap().starts_with('.')); @@ -412,36 +435,52 @@ mod tests { #[test] fn trailing_slash_on_home_is_absorbed() { assert_eq!( - remote_paths("/root/", "1.0.0").binary, - "/root/.local/share/tty7/bin/tty7-server-1.0.0" + remote_paths("/root/", 1, 1).binary, + "/root/.local/share/tty7/bin/tty7-server-c1p1" ); // Root as home is degenerate but must still be well-formed. - assert_eq!(remote_paths("/", "1.0.0").bin_dir, "/.local/share/tty7/bin"); + assert_eq!(remote_paths("/", 1, 1).bin_dir, "/.local/share/tty7/bin"); } /// The inverse used to identify a *running* daemon from its executable path. #[test] - fn version_is_recoverable_from_an_install_path() { + fn dialects_are_recoverable_from_an_install_path() { assert_eq!( - version_from_path("/home/me/.local/share/tty7/bin/tty7-server-26.7.4").as_deref(), - Some("26.7.4") + dialect_from_path("/home/me/.local/share/tty7/bin/tty7-server-c3p4"), + Some((3, 4)) ); - assert_eq!( - version_from_path("tty7-server-26.7.6-nightly.20260727").as_deref(), - Some("26.7.6-nightly.20260727") - ); - // Not ours, or not versioned: no opinion rather than a wrong one. - assert_eq!(version_from_path("/usr/bin/tty7-server"), None); - assert_eq!(version_from_path("/usr/local/bin/tty7-server-"), None); - assert_eq!(version_from_path("/bin/bash"), None); + assert_eq!(dialect_from_path("tty7-server-c12p30"), Some((12, 30))); + // Not ours, or not dialect-named: no opinion rather than a wrong one. + assert_eq!(dialect_from_path("/usr/bin/tty7-server"), None); + assert_eq!(dialect_from_path("/bin/bash"), None); + assert_eq!(dialect_from_path("tty7-server-c3"), None); + assert_eq!(dialect_from_path("tty7-server-cxpy"), None); + } + + /// Every name a version-naming client ever installed reads as "no opinion". + /// + /// The whole point of the rename is that a version string can no longer be + /// mistaken for a dialect; a parser that squeezed `3` out of `26.7.3` would + /// reintroduce exactly that, and on the paths of binaries already sitting on + /// users' machines. + #[test] + fn legacy_version_named_binaries_carry_no_dialect() { + for legacy in [ + "/home/me/.local/share/tty7/bin/tty7-server-26.7.4", + "tty7-server-26.7.6-nightly.20260727", + "tty7-server-0.1.0", + "/usr/local/bin/tty7-server-", + ] { + assert_eq!(dialect_from_path(legacy), None, "{legacy}"); + } } /// Round-trip: the name we install under is the name we recognise later. #[test] - fn install_path_and_version_extraction_round_trip() { - for version in ["26.7.5", "0.1.0", "26.7.6-nightly.20260727"] { - let p = remote_paths("/home/me", version); - assert_eq!(version_from_path(&p.binary).as_deref(), Some(version)); + fn install_path_and_dialect_extraction_round_trip() { + for (c, p) in [(1u32, 1u32), (3, 4), (26, 7)] { + let paths = remote_paths("/home/me", c, p); + assert_eq!(dialect_from_path(&paths.binary), Some((c, p))); } } } diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index ab19d654..b9f2e09f 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -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-` | [`Installer::run`] | +//! | 2 | SFTP-stat `~/.local/share/tty7/bin/tty7-server-cp` | [`Installer::run`] | //! | 3 | absent → download the asset **on the client** + sha256-verify it | [`download`], [`checksums`] | -//! | 4 | SFTP-put into `bin/.tty7-server-.tmp` | [`RemoteOps::put`] | -//! | 5 | `chmod 0755` then `rename` — atomic publish | [`RemoteOps::rename`] | +//! | 4 | SFTP-put into `bin/.tty7-server-cp

..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-`. +/// 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") } @@ -531,6 +535,16 @@ pub enum InstallPhase { /// Writing the verified bytes to the remote over SFTP. `total` is exact: /// the bytes are in memory by now. Uploading { done: u64, total: u64 }, + /// Stopping the server that was running and starting the one we want, with + /// no bytes involved either way. + /// + /// Carries no counts because there is nothing to count: it is a SIGTERM, a + /// poll until the socket goes quiet, a launch, and a poll until it answers — + /// up to `REMOTE_SHUTDOWN_TIMEOUT + REMOTE_STARTUP_TIMEOUT` of a GUI that + /// would otherwise sit there looking like nothing had been clicked. Reported + /// precisely because the case it exists for ("Replace Server" onto a binary + /// already present) transfers nothing and so would report nothing at all. + Restarting, } impl InstallPhase { @@ -539,6 +553,8 @@ impl InstallPhase { let (done, total) = match *self { InstallPhase::Downloading { done, total } => (done, total?), InstallPhase::Uploading { done, total } => (done, total), + // Indeterminate by nature: the wait is two timeouts, not a transfer. + InstallPhase::Restarting => return None, }; if total == 0 { return None; @@ -672,6 +688,13 @@ impl RemoteProtocol { } } + /// The two numbers that decide everything, without the build string that + /// decides nothing. This is what names the installed file — see + /// [`asset::binary_name`]. + pub fn dialect(&self) -> (u32, u32) { + (self.control, self.protocol) + } + /// Whether a server speaking `self` can serve a client speaking `other`. /// /// **Both numbers, both exactly equal** — the same judgement @@ -845,6 +868,27 @@ pub enum InstallError { Write { path: String, reason: String }, /// The daemon would not start, or would not answer after starting. Launch { reason: String }, + /// The bytes were uploaded, made executable, asked what they speak — and + /// answered with something other than the dialect the filename they were + /// about to be published under promises. + /// + /// Terminal, and the temp file is removed rather than published. This is the + /// check that makes "the filename is the dialect" a fact instead of a + /// convention: without it a source that hands over the wrong build (a + /// [`wsl::BUNDLED_DIR_ENV`] pointing at a stale cross-compile, a release tag + /// that predates a wire break) writes a file that lies, and the *next* + /// connect trusts the name and fails in the handshake with nothing to + /// blame. + /// + /// `spoke` is `None` when the binary could not answer at all — it did not + /// exec, or it is older than [`PROTOCOL_FLAG`]. Both mean the same thing + /// here: nothing may be published under a name that has not been earned. + DialectMismatch { + /// Where the bytes came from, so the message can name the thing to fix. + origin: String, + wanted: RemoteProtocol, + spoke: Option, + }, } impl std::fmt::Display for InstallError { @@ -877,6 +921,25 @@ impl std::fmt::Display for InstallError { write!(f, "could not write {path} on the remote machine: {reason}") } Self::Launch { reason } => write!(f, "the remote tty7-server did not start: {reason}"), + Self::DialectMismatch { + origin, + wanted, + spoke, + } => { + let spoken = match spoke { + Some(s) => format!("control v{}, protocol v{}", s.control, s.protocol), + None => "nothing this client understands".to_string(), + }; + write!( + f, + "this build needs a tty7-server speaking control v{} and protocol v{}, \ + but {origin} speaks {spoken}; nothing was installed. \ + Point {} at a directory holding a matching server binary.", + wanted.control, + wanted.protocol, + wsl::BUNDLED_DIR_ENV, + ) + } } } } @@ -886,9 +949,9 @@ impl std::error::Error for InstallError {} impl From for io::Error { fn from(e: InstallError) -> io::Error { let kind = match &e { - InstallError::Unsupported(_) | InstallError::MissingBundled { .. } => { - io::ErrorKind::Unsupported - } + InstallError::Unsupported(_) + | InstallError::MissingBundled { .. } + | InstallError::DialectMismatch { .. } => io::ErrorKind::Unsupported, InstallError::Declined { .. } => io::ErrorKind::PermissionDenied, InstallError::Checksum(_) => io::ErrorKind::InvalidData, InstallError::Launch { .. } => io::ErrorKind::TimedOut, @@ -944,7 +1007,12 @@ pub struct Installer<'a> { source: Option<&'a dyn ServerBinarySource>, confirm: &'a dyn InstallConfirm, host: String, + /// Which release to download, and what to call this client in a prompt. + /// **Never a decision input** — see [`asset::binary_name`]. version: String, + /// What this client speaks, and therefore which file on the remote is the + /// one that can serve it. + dialect: RemoteProtocol, /// Overridable so tests do not spend the real budget waiting for a daemon /// that a fake will never start. startup_timeout: Duration, @@ -965,6 +1033,7 @@ impl<'a> Installer<'a> { confirm, host: host.into(), version: client_version().to_string(), + dialect: RemoteProtocol::of_this_build(), startup_timeout: REMOTE_STARTUP_TIMEOUT, poll_interval: REMOTE_POLL_INTERVAL, } @@ -987,18 +1056,109 @@ impl<'a> Installer<'a> { confirm, host: host.into(), version: client_version().to_string(), + dialect: RemoteProtocol::of_this_build(), startup_timeout: REMOTE_STARTUP_TIMEOUT, poll_interval: REMOTE_POLL_INTERVAL, } } - /// Install a specific version instead of this build's. Tests only — a real - /// client can only speak its own dialect. + /// Download a specific version's release instead of this build's. Tests + /// only. Does **not** move the install path — that follows the dialect. pub fn with_version(mut self, version: impl Into) -> Self { self.version = version.into(); + self.dialect.build = self.version.clone(); self } + /// Pretend this client speaks `control`/`protocol`. Tests only — a real + /// client can only speak its own dialect, and every decision in this module + /// keys off it, so the fakes need a way to stand somewhere else. + pub fn with_dialect(mut self, control: u32, protocol: u32) -> Self { + self.dialect.control = control; + self.dialect.protocol = protocol; + self + } + + /// The paths this client's dialect installs to under `home`. + fn paths_for(&self, home: &str) -> RemotePaths { + asset::remote_paths(home, self.dialect.control, self.dialect.protocol) + } + + /// Make this machine's *running* server one that speaks to us, installing a + /// binary first only if the one at our dialect's path cannot — "Replace + /// server on this host". + /// + /// The action a failed handshake offers, and it covers both ways a connect + /// can reach a server it cannot talk to: + /// + /// | What is wrong | What this does | + /// |---|---| + /// | An older daemon is serving; our binary is there and fine | Restart onto it. **No download.** | + /// | The binary at our dialect's path is missing, or answers with something else | Install ours, then restart | + /// + /// The first row is the common one and the reason this asks before it + /// downloads: [`run`](Self::run) leaves a machine in exactly that state + /// every time it declines to kill a daemon that owns live panes, so the + /// button under the handshake error must not need a network — or a released + /// asset that speaks our dialect, which for a dev build does not exist — to + /// fix the case it was written for. + /// + /// The second row is the only thing anywhere that overwrites a published + /// binary. Every other path trusts `tty7-server-cp

` to speak c/p, + /// because [`install`](Self::install) proves that before publishing it; only + /// something outside tty7 can put a file there that lies, and this is the + /// way out when it does. + /// + /// **Every pane the running server hosts dies**, in both rows, for the same + /// reason as [`restart_daemon`](Self::restart_daemon). Only ever call it + /// with a user's explicit answer behind it. + pub fn replace(&self) -> Result<(), InstallError> { + let home = self.ops.home_dir().map_err(InstallError::NoHome)?; + let paths = self.paths_for(&home); + + if !self.published_binary_serves_us(&paths)? { + let uname = self + .ops + .run("uname -sm") + .map_err(InstallError::Probe) + .and_then(|out| { + if out.success() { + Ok(out.stdout) + } else { + Err(InstallError::Probe(out.failure_reason())) + } + })?; + let asset = asset::asset_for_uname(&uname).map_err(InstallError::Unsupported)?; + self.install(asset, &paths)?; + } + + self.restart_daemon() + } + + /// Whether the binary at our dialect's path is there, runnable, and really + /// speaks what its name claims. + /// + /// The one place that spends a probe on a `stat` hit. [`run`](Self::run) + /// deliberately does not — it would pay a round trip on every connect to + /// re-check something the install already proved. Here the caller is about + /// to either download 8 MB or drop every pane on the machine, so one + /// question first is cheap by comparison. + fn published_binary_serves_us(&self, paths: &RemotePaths) -> Result { + let stat = self + .ops + .stat(&paths.binary) + .map_err(|reason| InstallError::Write { + path: paths.binary.clone(), + reason, + })?; + if !stat.is_some_and(|s| !s.is_dir && s.mode & 0o100 != 0) { + return Ok(false); + } + Ok(self + .probe_protocol(&paths.binary) + .is_some_and(|spoken| spoken.serves(&self.dialect))) + } + /// Shorten the daemon-startup budget. Tests only. pub fn with_timeouts(mut self, startup: Duration, poll: Duration) -> Self { self.startup_timeout = startup; @@ -1006,8 +1166,10 @@ impl<'a> Installer<'a> { self } - /// The whole flow. On `Ok`, the machine has `tty7-server-` 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-cp`, or one that was already running and + /// said it speaks our dialects. pub fn run(&self) -> Result { // --- 1. uname -sm -------------------------------------------------- let uname = self @@ -1023,9 +1185,14 @@ impl<'a> Installer<'a> { })?; let asset = asset::asset_for_uname(&uname).map_err(InstallError::Unsupported)?; - // --- 2. is the matching version already there? ---------------------- + // --- 2. is a server that can serve us already there? ------------------ + // + // One `stat` of a path built entirely from this client's own two + // dialect numbers. Nothing is asked of the remote to decide *which* + // path to look at, which is what keeps this cheap enough to run before + // every link and correct on a machine that cannot reach GitHub. let home = self.ops.home_dir().map_err(InstallError::NoHome)?; - let paths = asset::remote_paths(&home, &self.version); + let paths = self.paths_for(&home); let already = self .ops @@ -1068,7 +1235,12 @@ impl<'a> Installer<'a> { spoken.protocol, self.version, ); - report.paths = asset::remote_paths_for_binary(&home, &exe); + report.paths = asset::remote_paths_for_binary( + &home, + &exe, + self.dialect.control, + self.dialect.protocol, + ); report.reused = Some(spoken); } None => { @@ -1101,7 +1273,7 @@ impl<'a> Installer<'a> { let Some(spoken) = self.probe_protocol(&exe) else { return Ok(None); }; - if !spoken.serves(&RemoteProtocol::of_this_build()) { + if !spoken.serves(&self.dialect) { return Ok(None); } Ok(Some((exe, spoken))) @@ -1144,6 +1316,10 @@ impl<'a> Installer<'a> { bytes, origin: asset_url, } = self.load_binary(asset)?; + // Kept past the consent prompt, which consumes the original: if the + // upload turns out to speak the wrong dialect, "where did these bytes + // come from" is the whole content of the error. + let asset_origin = asset_url.clone(); // --- consent, once per machine -------------------------------------- let confirmed = if self.is_first_install(paths) { @@ -1178,29 +1354,53 @@ impl<'a> Installer<'a> { // refuses SETSTAT) must not block an install that will otherwise work. let _ = self.ops.chmod(&paths.bin_dir, DIR_MODE); + // The dialect names one file, so two clients installing the same dialect + // at once would otherwise write the same temp path and interleave their + // bytes into it. The pid makes the staging area private; the final name + // is still the shared one, and `rename` is still what publishes it. + let temp = unique_temp(&paths.temp); + let sink = install_progress(); let total = bytes.len() as u64; self.ops - .put_with_progress(&paths.temp, &bytes, &|done| { + .put_with_progress(&temp, &bytes, &|done| { sink.report(&self.host, InstallPhase::Uploading { done, total }); }) .map_err(|reason| InstallError::Write { - path: paths.temp.clone(), + path: temp.clone(), reason, })?; - // --- 5. chmod then rename -------------------------------------------- + // --- 5. chmod, ask what it speaks, then rename ----------------------- // // chmod *before* the rename, so the binary is never visible at its final // path in a non-executable state: a concurrent connect that finds - // `tty7-server-` present would otherwise try to exec a 0644 file. + // `tty7-server-cp

` present would otherwise try to exec a 0644 file. self.ops - .chmod(&paths.temp, BINARY_MODE) + .chmod(&temp, BINARY_MODE) .map_err(|reason| InstallError::Write { - path: paths.temp.clone(), + path: temp.clone(), reason, })?; - if let Err(reason) = self.ops.rename(&paths.temp, &paths.binary) { + + // The file is about to be published under a name that *claims* a + // dialect. Earn the claim: the binary is on the machine and executable, + // so ask it, and publish nothing if the answer is not the one the name + // promises. Also the first moment an architecture mistake can surface as + // itself — a binary for the wrong machine cannot exec, so it cannot + // answer, and it is refused here instead of dying as `Exec format error` + // inside a daemon launch that has no visible connection to `uname`. + let spoke = self.probe_protocol(&temp); + if !spoke.as_ref().is_some_and(|s| s.serves(&self.dialect)) { + let _ = self.ops.remove_file(&temp); + return Err(InstallError::DialectMismatch { + origin: asset_origin, + wanted: self.dialect.clone(), + spoke, + }); + } + + if let Err(reason) = self.ops.rename(&temp, &paths.binary) { // Some SFTP servers refuse a rename onto an existing name. The only // way that path exists here is a leftover from an interrupted run // (a *usable* binary short-circuits in `run`), so removing it and @@ -1208,7 +1408,7 @@ impl<'a> Installer<'a> { // location. let _ = self.ops.remove_file(&paths.binary); self.ops - .rename(&paths.temp, &paths.binary) + .rename(&temp, &paths.binary) .map_err(|_| InstallError::Write { path: paths.binary.clone(), reason, @@ -1326,18 +1526,18 @@ impl<'a> Installer<'a> { fn check_running_build(&self, paths: &RemotePaths) -> Option { let exe = self.running_server_exe()?; let exe = exe.as_str(); - let running_version = asset::version_from_path(exe); - if running_version.as_deref() == Some(self.version.as_str()) || exe == paths.binary { + // The name carries the dialects, so most of the time the path we already + // had in hand is the whole answer and no second round trip is spent. + if asset::dialect_from_path(exe) == Some(self.dialect.dialect()) || exe == paths.binary { return None; } - // A different build, so ask the only question that decides anything. - // An unanswerable probe leaves the old behaviour in place: a server that - // predates the flag really might not understand us, and the prompt is - // the honest response to not knowing. - if self - .probe_protocol(exe) - .is_some_and(|spoken| spoken.serves(&RemoteProtocol::of_this_build())) - { + // Either a dialect that is not ours, or a legacy version-named binary + // that claims nothing. Ask it directly. An unanswerable probe leaves the + // old behaviour in place: a server that predates the flag really might + // not understand us, and the prompt is the honest response to not + // knowing. + let spoken = self.probe_protocol(exe); + if spoken.as_ref().is_some_and(|s| s.serves(&self.dialect)) { log::info!( "remote {} is served by {exe}, a different build this client speaks to anyway", self.host, @@ -1346,7 +1546,7 @@ impl<'a> Installer<'a> { } let entry = MismatchedRemoteDaemon { host: self.host.clone(), - running_version, + running_version: spoken.map(|s| s.build), running_exe: Some(exe.to_string()), wanted_version: self.version.clone(), }; @@ -1365,7 +1565,10 @@ impl<'a> Installer<'a> { /// dies; that is what the prompt warns about. pub fn restart_daemon(&self) -> Result<(), InstallError> { let home = self.ops.home_dir().map_err(InstallError::NoHome)?; - let paths = asset::remote_paths(&home, &self.version); + let paths = self.paths_for(&home); + // Before the first timeout rather than after it: this is the only signal + // the user gets that the click landed. + install_progress().report(&self.host, InstallPhase::Restarting); // SIGTERM by the pid whose executable is a tty7-server: the daemon tears // down like a local `Shutdown`, hanging every pane's child up with its @@ -1445,6 +1648,36 @@ fn launch_script(binary: &str, settle: Option) -> String { } } +/// A staging path private to this process, from the shared per-dialect one. +/// +/// `.tty7-server-c3p4.tmp` → `.tty7-server-c3p4.4711.tmp`. Inserted before the +/// suffix rather than appended so the name still ends in `.tmp` and still starts +/// with a dot: both are what keep a half-written upload from being mistaken for +/// an installed server. +/// +/// The litter this can leave (one file per install killed between `put` and +/// `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") { + Some(stem) => format!("{stem}.{pid}.tmp"), + None => format!("{shared}.{pid}"), + } +} + /// POSIX single-quote escaping. Home directories with spaces, apostrophes or /// `$` in them are rare but real, and every command here interpolates a path. pub(crate) fn shell_quote(s: &str) -> String { @@ -1479,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-`), 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-cp`, 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. @@ -1526,7 +1760,7 @@ pub fn ensure_remote_server_labeled(conn: &Arc, host: &str) -> io } /// Restart the remote daemon at this client's build, dropping every pane it -/// hosts. The "restart the service" answer to the version-mismatch prompt. +/// hosts. The "restart the service" answer to the dialect-mismatch prompt. pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { let host = connection_label(conn); let ops = ssh_ops::SshRemoteOps::new(conn.clone()); @@ -1536,6 +1770,20 @@ pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { Ok(()) } +/// Reinstall this client's server on `conn`'s machine even though one is +/// already at its path, and restart the daemon onto it. See +/// [`Installer::replace`] — this is what a handshake that failed against a +/// binary whose name lied about its dialect offers as the way out. +pub fn replace_remote_server(conn: &Arc) -> io::Result<()> { + let host = connection_label(conn); + let ops = ssh_ops::SshRemoteOps::new(conn.clone()); + let fetch = default_fetcher(); + let confirm = install_confirm(); + let source = BundledOrRelease::from_env(fetch.as_ref()); + Installer::with_source(&ops, &source, confirm.as_ref(), host).replace()?; + Ok(()) +} + /// The HTTPS fetcher, when this build has one. #[cfg(feature = "remote-install")] fn default_fetcher() -> Arc { diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index fe8d7db5..5fd681d8 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -20,10 +20,23 @@ use super::*; use crate::daemon::install::asset::{ASSET_X86_64, CHECKSUMS_ASSET}; const VERSION: &str = "26.7.5"; +/// The dialects the fixture's client speaks. Fixed literals rather than +/// [`RemoteProtocol::of_this_build`] so [`BINARY`] can be asserted as a string: +/// these tests are about *how* the name is built, and a name derived from the +/// same constants it is checked against would assert nothing. +const CONTROL: u32 = 3; +const PROTOCOL: u32 = 4; const HOME: &str = "/home/me"; const BIN_DIR: &str = "/home/me/.local/share/tty7/bin"; -const BINARY: &str = "/home/me/.local/share/tty7/bin/tty7-server-26.7.5"; -const TEMP: &str = "/home/me/.local/share/tty7/bin/.tty7-server-26.7.5.tmp"; +const BINARY: &str = "/home/me/.local/share/tty7/bin/tty7-server-c3p4"; +/// The shared per-dialect staging name. What actually gets written is +/// [`temp()`] — `unique_temp` of this. +const TEMP_BASE: &str = "/home/me/.local/share/tty7/bin/.tty7-server-c3p4.tmp"; + +/// The staging path this process writes to, which carries its pid. +fn temp() -> String { + unique_temp(TEMP_BASE) +} /// Stand-in for the release asset. Content is irrelevant; only its digest is. const SERVER_BYTES: &[u8] = b"\x7fELF...a static musl tty7-server, pretend it is 6 MB"; @@ -68,6 +81,14 @@ struct FakeRemote { /// models a server too old to know the flag: the probe fails, and the /// installer falls back to having no opinion. speaks: Mutex>, + /// What a *freshly uploaded* binary answers. Registered by `put` against the + /// path written, because the real installer asks the bytes it just staged + /// what they speak before publishing them — a fake whose uploads stayed mute + /// would model every install as a failed one. + /// + /// `None` models bytes that cannot answer at all: the wrong architecture, or + /// a build older than the flag. + installed_speaks: Option, } impl FakeRemote { @@ -90,6 +111,7 @@ impl FakeRemote { running_exe: Mutex::new(None), launch_works: true, speaks: Mutex::new(HashMap::new()), + installed_speaks: Some(ours()), } } @@ -99,15 +121,36 @@ impl FakeRemote { self } - /// A machine tty7 has installed on before (so consent is not re-asked). - fn with_previous_install(self, version: &str) -> Self { - self.preinstall(&format!("{BIN_DIR}/tty7-server-{version}"), 0o755); + /// Make whatever gets uploaded answer with `spoken` — a source that hands + /// over a build other than the one the client asked for. `None` for bytes + /// that cannot answer at all. + fn uploads_speaking(mut self, spoken: Option) -> Self { + self.installed_speaks = spoken; self } + /// A machine tty7 has installed on before (so consent is not re-asked), with + /// this client's own dialect already published. + fn with_previous_install(self) -> Self { + self.preinstall(BINARY, 0o755); + self.speaks + .lock() + .unwrap() + .insert(BINARY.to_string(), ours()); + self + } + + /// A machine an *older, version-naming* client installed on: consent was + /// given once, and what it left behind claims no dialect. Returns the path. + fn with_legacy_install(self, version: &str) -> (Self, String) { + let path = format!("{BIN_DIR}/tty7-server-{version}"); + self.preinstall(&path, 0o755); + (self, path) + } + fn preinstall(&self, path: &str, mode: u32) { let mut files = self.files.lock().unwrap(); - for dir in asset::remote_paths(HOME, VERSION).dir_chain { + for dir in asset::remote_paths(HOME, CONTROL, PROTOCOL).dir_chain { files.entry(dir).or_insert(FakeFile { bytes: Vec::new(), mode: 0o700, @@ -270,6 +313,13 @@ impl RemoteOps for FakeRemote { is_dir: false, }, ); + // Uploaded bytes are a binary that can be asked what it speaks, which is + // exactly what the installer does with them next. + let mut speaks = self.speaks.lock().unwrap(); + match &self.installed_speaks { + Some(spoken) => speaks.insert(path.to_string(), spoken.clone()), + None => speaks.remove(path), + }; Ok(()) } @@ -282,6 +332,12 @@ impl RemoteOps for FakeRemote { match files.remove(from) { Some(f) => { files.insert(to.to_string(), f); + // The binary keeps its answer when it changes name. + let mut speaks = self.speaks.lock().unwrap(); + match speaks.remove(from) { + Some(spoken) => speaks.insert(to.to_string(), spoken), + None => speaks.remove(to), + }; Ok(()) } None => Err("2: No such file".into()), @@ -409,6 +465,7 @@ fn installer<'a>( ) -> Installer<'a> { Installer::new(remote, release, user, host) .with_version(VERSION) + .with_dialect(CONTROL, PROTOCOL) .with_timeouts(Duration::from_millis(200), Duration::from_millis(10)) } @@ -445,7 +502,7 @@ fn first_install_runs_all_six_steps() { assert_eq!(installed.bytes, SERVER_BYTES, "the verified bytes landed"); assert_eq!(installed.mode, 0o755, "and are executable"); assert!( - remote.file(TEMP).is_none(), + remote.file(&temp()).is_none(), "the temp name is consumed by the rename" ); @@ -462,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-` present and unusable. +/// concurrent connect finds `tty7-server-cp

` present and unusable. #[test] fn the_final_path_is_only_ever_reached_by_renaming_a_ready_temp() { let remote = FakeRemote::new(); @@ -484,15 +541,17 @@ fn the_final_path_is_only_ever_reached_by_renaming_a_ready_temp() { let put = writes .iter() - .position(|j| matches!(j, Journal::Put { path, .. } if path == TEMP)) + .position(|j| matches!(j, Journal::Put { path, .. } if path == &temp())) .expect("the bytes go to the temp path"); let chmod = writes .iter() - .position(|j| matches!(j, Journal::Chmod { path, mode } if path == TEMP && *mode == 0o755)) + .position( + |j| matches!(j, Journal::Chmod { path, mode } if path == &temp() && *mode == 0o755), + ) .expect("the temp is made executable"); let rename = writes .iter() - .position(|j| matches!(j, Journal::Rename { from, to } if from == TEMP && to == BINARY)) + .position(|j| matches!(j, Journal::Rename { from, to } if from == &temp() && to == BINARY)) .expect("the temp is renamed onto the binary"); assert!(put < chmod, "bytes before mode: {writes:?}"); @@ -571,7 +630,7 @@ fn a_sha256_mismatch_aborts_before_touching_the_remote() { "nothing may be written after a failed verification: {:?}", remote.writes() ); - assert!(remote.file(TEMP).is_none()); + assert!(remote.file(&temp()).is_none()); assert!(remote.file(BINARY).is_none()); assert!( user.asked().is_empty(), @@ -681,7 +740,7 @@ fn the_default_confirmation_declines() { /// about "may tty7 put binaries here", and it was given. #[test] fn upgrading_a_known_machine_does_not_ask_again() { - let remote = FakeRemote::new().with_previous_install("26.7.4"); + let (remote, legacy) = FakeRemote::new().with_legacy_install("26.7.4"); let release = FakeRelease::new(); let user = FakeUser::declining(); // would refuse if asked @@ -695,12 +754,9 @@ fn upgrading_a_known_machine_does_not_ask_again() { user.asked().is_empty(), "no prompt on a machine we already use" ); - // The older binary is still there: versioned paths coexist. - assert!( - remote - .file(&format!("{BIN_DIR}/tty7-server-26.7.4")) - .is_some() - ); + // The old binary is still there: one file per dialect, and the older one may + // still be the one a running daemon was exec'd from. + assert!(remote.file(&legacy).is_some()); assert!(remote.file(BINARY).is_some()); } @@ -800,13 +856,13 @@ fn a_failed_write_names_the_path_and_does_not_fall_back() { ref path, ref reason, } => { - assert_eq!(path, TEMP, "the exact path that failed"); + assert_eq!(path, &temp(), "the exact path that failed"); assert!(reason.contains("no space left"), "the server's own reason"); } other => panic!("expected a write failure, got {other}"), } let message = err.to_string(); - assert!(message.contains(TEMP), "{message}"); + assert!(message.contains(&temp()), "{message}"); assert!(message.contains("no space left"), "{message}"); // One attempt at one path. No second put, no alternative directory. @@ -891,15 +947,21 @@ fn a_daemon_that_never_answers_is_an_error() { } } -/// **Version mismatch: keep the old daemon, record the mismatch.** It owns every +/// **Dialect mismatch: keep the old daemon, record the mismatch.** It owns every /// live pane on that machine; ending them at connect time is the user's call, /// not the installer's — exactly as `spawn::ensure_running` treats the local /// daemon. #[test] fn an_older_running_daemon_is_kept_and_reported() { - let remote = FakeRemote::new() - .with_previous_install("26.7.4") - .serving(&format!("{BIN_DIR}/tty7-server-26.7.4")); + 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 release = FakeRelease::new(); let user = FakeUser::approving(); @@ -942,7 +1004,7 @@ fn an_unidentifiable_running_daemon_is_not_a_mismatch() { #[test] fn restart_replaces_the_running_daemon() { let remote = FakeRemote::new() - .with_previous_install("26.7.4") + .with_previous_install() .serving(&format!("{BIN_DIR}/tty7-server-26.7.4")); remote.preinstall(BINARY, 0o755); let release = FakeRelease::new(); @@ -1168,7 +1230,7 @@ fn a_bundle_that_lacks_the_asset_does_not_fall_back_to_the_network() { let _ = std::fs::remove_dir_all(&dir); } -/// The path the installer publishes to is **absolute and version-qualified**, +/// The path the installer publishes to is **absolute and dialect-qualified**, /// and that is what the session-channel fallback has to exec. /// /// Observed for real: the transport exec'd the bare name `tty7-server`, which @@ -1177,19 +1239,19 @@ fn a_bundle_that_lacks_the_asset_does_not_fall_back_to_the_network() { /// file there is not even called `tty7-server`. The remote process died at /// once, taking the pane with it. #[test] -fn the_published_path_is_absolute_and_version_qualified() { - // Built from *this crate's* version rather than the fixture's `VERSION`: - // what the transport execs is whatever `client_version()` currently names, - // and pinning the shape to a literal would only re-assert the literal (and - // go red on every release bump, which is how it used to behave). - let published = asset::remote_paths(HOME, client_version()).binary; +fn the_published_path_is_absolute_and_dialect_qualified() { + // Built from *this crate's* dialects rather than the fixture's, because + // what the transport execs is whatever this build currently names. + let real = RemoteProtocol::of_this_build(); + let published = asset::remote_paths(HOME, real.control, real.protocol).binary; assert!( published.starts_with('/'), "a relative path would resolve against whatever directory the exec landed in" ); - assert!( - published.ends_with(&format!("tty7-server-{}", client_version())), - "the filename carries the version, so the bare name never names it: {published}" + assert_eq!( + asset::dialect_from_path(&published), + Some((real.control, real.protocol)), + "the filename carries the dialects, so the bare name never names it: {published}" ); assert_ne!( published.rsplit('/').next(), @@ -1378,7 +1440,7 @@ fn every_report_carries_the_host() { /// ignore it on the one connect where it means something. #[test] fn a_present_binary_reports_no_progress() { - let remote = FakeRemote::new().with_previous_install(VERSION); + let remote = FakeRemote::new().with_previous_install(); let release = FakeRelease::new(); let user = FakeUser::approving(); let reports = Arc::new(Reports::default()); @@ -1467,12 +1529,16 @@ fn a_fraction_is_either_absent_or_in_range() { /// What this client speaks, which is what a remote has to match. fn ours() -> RemoteProtocol { RemoteProtocol { + control: CONTROL, + protocol: PROTOCOL, build: VERSION.to_string(), - ..RemoteProtocol::of_this_build() } } const OTHER_BUILD: &str = "26.7.9-nightly.20260801"; +/// A server installed by a client that named files after *versions* — every +/// binary already sitting on a user's machine when this naming shipped. Its path +/// claims no dialect, so it can only be adopted by being asked. const OTHER_EXE: &str = "/home/me/.local/share/tty7/bin/tty7-server-26.7.9-nightly.20260801"; /// **A newer server this client can talk to is adopted, not overwritten.** @@ -1620,9 +1686,7 @@ fn a_server_that_cannot_be_probed_is_installed_over() { /// before should cost a `stat` and nothing more. #[test] fn the_matching_version_still_costs_no_probe() { - let remote = FakeRemote::new() - .with_previous_install(VERSION) - .serving(BINARY); + let remote = FakeRemote::new().with_previous_install().serving(BINARY); let release = FakeRelease::new(); let user = FakeUser::approving(); @@ -1695,3 +1759,272 @@ fn a_noisy_shell_does_not_break_the_probe() { assert_eq!(RemoteProtocol::parse(""), None); assert_eq!(RemoteProtocol::parse("not json at all"), None); } + +// --------------------------------------------------------------------------- +// The name is a promise, and it is checked before it is published. +// --------------------------------------------------------------------------- + +/// **Bytes that speak the wrong dialect are never published.** +/// +/// The whole naming scheme rests on `tty7-server-cp

` really speaking +/// c/p, and nothing upstream of the upload can guarantee that: a +/// `TTY7_BUNDLED_SERVER_DIR` can hold a stale cross-compile, and a release tag +/// can predate a wire break. Publishing anyway writes a file that lies, and the +/// *next* connect trusts the name, skips the install, and dies in the handshake +/// with nothing to blame. +#[test] +fn an_upload_that_speaks_the_wrong_dialect_is_not_published() { + let remote = FakeRemote::new().uploads_speaking(Some(RemoteProtocol { + control: CONTROL - 1, + protocol: PROTOCOL, + build: "26.7.4".to_string(), + })); + let release = FakeRelease::new(); + let user = FakeUser::approving(); + + let err = installer(&remote, &release, &user, "me@stale-box:22") + .run() + .unwrap_err(); + + match err { + InstallError::DialectMismatch { ref spoke, .. } => assert_eq!( + spoke.as_ref().map(|s| s.dialect()), + Some((CONTROL - 1, PROTOCOL)), + "the error quotes what the bytes actually said" + ), + other => panic!("expected a dialect mismatch, got {other}"), + } + assert!( + remote.file(BINARY).is_none(), + "nothing may sit at the published name" + ); + assert!( + remote.file(&temp()).is_none(), + "and the staged file is cleaned up rather than left to be found" + ); + assert!( + err.to_string().contains(wsl::BUNDLED_DIR_ENV), + "the message points at the one lever that fixes it: {err}" + ); +} + +/// **Bytes that cannot answer at all are refused the same way.** +/// +/// A binary for the wrong architecture cannot exec, so it cannot answer. This +/// is the first moment that mistake can surface as itself; without the check it +/// used to reach a daemon launch and die as `Exec format error`, which names +/// nothing about `uname`. +#[test] +fn an_upload_that_cannot_answer_is_not_published() { + let remote = FakeRemote::new().uploads_speaking(None); + let release = FakeRelease::new(); + let user = FakeUser::approving(); + + let err = installer(&remote, &release, &user, "me@wrong-arch:22") + .run() + .unwrap_err(); + + assert!( + matches!(err, InstallError::DialectMismatch { spoke: None, .. }), + "got {err}" + ); + assert!(remote.file(BINARY).is_none()); +} + +/// **A dialect already installed is reused without downloading or asking.** +/// +/// The hot path, stated as a cost: one `stat` of a path built from this +/// client's own two numbers, and no network at all — which is what has to hold +/// on a machine that cannot reach GitHub. +#[test] +fn a_machine_with_our_dialect_installed_costs_nothing() { + let remote = FakeRemote::new().with_previous_install().serving(BINARY); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let report = installer(&remote, &release, &user, "me@ready-box:22") + .run() + .expect("connect"); + + assert!(!report.installed); + assert!(report.mismatch.is_none()); + assert!(remote.writes().is_empty(), "{:?}", remote.writes()); + assert!(release.fetched().is_empty(), "{:?}", release.fetched()); +} + +/// **A different build behind our dialect is used as-is.** +/// +/// The deliberate limit of the whole scheme: dialects decide whether a connect +/// works, and "is this the build I just compiled" is a different question that +/// must not cost an 8 MB upload on every connect. Someone else's install, or an +/// older client's, serves us fine. +#[test] +fn another_build_at_our_dialect_is_used_rather_than_replaced() { + let remote = FakeRemote::new().with_previous_install().serving(BINARY); + // Same file, same dialect, a build string from a different release. + let remote = remote.speaking( + BINARY, + RemoteProtocol { + build: OTHER_BUILD.to_string(), + ..ours() + }, + ); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let report = installer(&remote, &release, &user, "me@shared-box:22") + .run() + .expect("connect"); + + assert!(!report.installed, "{:?}", remote.writes()); + assert!( + report.mismatch.is_none(), + "a different build at the same dialect is not a question for the user" + ); +} + +/// **A legacy version-named binary is not adopted on the strength of its name.** +/// +/// Every machine tty7 had already installed on carries one. The name claims no +/// dialect, so the only honest thing to do is ask — and if it cannot answer, +/// install ours beside it. +#[test] +fn a_legacy_named_binary_is_probed_not_assumed() { + let (remote, legacy) = FakeRemote::new().with_legacy_install(VERSION); + // It answers, and it happens to speak our dialects: adopt it, no upload. + let remote = remote.serving(&legacy).speaking( + &legacy, + RemoteProtocol { + build: VERSION.to_string(), + ..ours() + }, + ); + let release = FakeRelease::new(); + let user = FakeUser::approving(); + + let report = installer(&remote, &release, &user, "me@legacy-box:22") + .run() + .expect("connect"); + + assert!( + !report.installed, + "it speaks our dialects, so it serves: {:?}", + remote.writes() + ); + assert_eq!( + report.paths.binary, legacy, + "and that is what we connect to" + ); + assert!(report.mismatch.is_none()); +} + +/// **The staging name is private to this process.** +/// +/// One file per dialect means the shared temp name is the same string for every +/// client installing that dialect, so two of them at once would interleave +/// their bytes into it. The published name stays shared — `rename` is still +/// what makes an install visible. +#[test] +fn the_staging_path_carries_the_pid() { + let staged = temp(); + assert_ne!(staged, TEMP_BASE); + assert!(staged.contains(&std::process::id().to_string()), "{staged}"); + assert!(staged.ends_with(".tmp"), "still recognisable as staging"); + assert!( + staged.rsplit('/').next().unwrap().starts_with('.'), + "still hidden, so a killed upload is not mistaken for an install" + ); + assert_eq!( + staged.rsplit_once('/').unwrap().0, + BINARY.rsplit_once('/').unwrap().0, + "same directory, so the publishing rename is still atomic" + ); +} + +// --------------------------------------------------------------------------- +// "Replace Server" — the way out of a handshake this client lost. +// --------------------------------------------------------------------------- + +/// **A good binary already at our path is restarted onto, not re-downloaded.** +/// +/// The state `run` leaves behind every time it refuses to kill a daemon that +/// owns live panes: our binary published, an older one still serving. It is the +/// common case behind the handshake error, and the button that offers to fix it +/// must not need a network — least of all a released asset speaking a dialect +/// that, for any build between releases, does not exist yet. +#[test] +fn replacing_reuses_a_published_binary_that_already_serves_us() { + let (remote, legacy) = FakeRemote::new().with_legacy_install("26.7.4"); + let remote = remote.with_previous_install().serving(&legacy).speaking( + &legacy, + RemoteProtocol { + control: CONTROL - 1, + protocol: PROTOCOL, + build: "26.7.4".to_string(), + }, + ); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + installer(&remote, &release, &user, "me@stuck-box:22") + .replace() + .expect("the binary is already there and it speaks to us"); + + assert!( + release.fetched().is_empty(), + "nothing to download: {:?}", + release.fetched() + ); + assert!( + !remote + .writes() + .iter() + .any(|j| matches!(j, Journal::Put { .. })), + "and nothing to upload: {:?}", + remote.writes() + ); + assert!( + remote + .journal() + .iter() + .any(|j| matches!(j, Journal::Launch)), + "but the daemon really is restarted: {:?}", + remote.journal() + ); +} + +/// **A binary whose name lies is overwritten.** +/// +/// The other reason a handshake fails against a path this client trusts: +/// something outside tty7 put a file there. `run` cannot catch it — it trusts +/// the name, which is what makes the connect cheap — so this is the only thing +/// that does. +#[test] +fn replacing_overwrites_a_published_binary_that_does_not_serve_us() { + let remote = FakeRemote::new().with_previous_install(); + // Someone replaced it: the name says our dialect, the bytes disagree. + let remote = remote.speaking( + BINARY, + RemoteProtocol { + control: CONTROL - 1, + protocol: PROTOCOL, + build: "hand-placed".to_string(), + }, + ); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + installer(&remote, &release, &user, "me@tampered-box:22") + .replace() + .expect("ours is written over it"); + + assert!( + remote + .writes() + .iter() + .any(|j| matches!(j, Journal::Put { .. })), + "the file had to be rewritten: {:?}", + remote.writes() + ); + assert!(!release.fetched().is_empty(), "which means downloading it"); +} diff --git a/crates/tty7-core/src/daemon/install/wsl.rs b/crates/tty7-core/src/daemon/install/wsl.rs index da4eb457..40f7a815 100644 --- a/crates/tty7-core/src/daemon/install/wsl.rs +++ b/crates/tty7-core/src/daemon/install/wsl.rs @@ -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-.tmp`, the first renames it into place +/// two runs in *this* process share a pid and so share +/// `.tty7-server-cp

..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 @@ -1754,6 +1755,24 @@ mod tests { }) }; } + if let Some(exe) = cmd + .trim() + .strip_suffix(crate::daemon::install::PROTOCOL_FLAG) + { + // Every binary this fake holds is one the installer just put + // there, so it speaks what this build speaks. Anything else + // cannot answer, exactly like a server older than the flag. + let exe = exe.trim().trim_matches('\''); + return if self.files.lock().unwrap().contains_key(exe) { + ok(&crate::daemon::install::RemoteProtocol::of_this_build().to_line()) + } else { + Ok(ExecOutput { + status: Some(1), + stdout: String::new(), + stderr: "unknown flag".into(), + }) + }; + } // The `/proc` sweep: no other build is running. ok("") } @@ -1882,9 +1901,14 @@ mod tests { assert!(report.installed); assert!(report.launched); assert!(report.confirmed, "a first install asks"); + let dialect = crate::daemon::install::RemoteProtocol::of_this_build(); assert_eq!( report.paths.binary, - "/home/me/.local/share/tty7/bin/tty7-server-26.7.5" + format!( + "/home/me/.local/share/tty7/bin/tty7-server-c{}p{}", + dialect.control, dialect.protocol + ), + "the name follows the dialect, not the `--with_version` release" ); assert_eq!( ops.files.lock().unwrap()[&report.paths.binary].0, diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 8798335d..a5f9d112 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -1316,7 +1316,22 @@ impl DaemonPane { let alive = st.alive; let facts_after = may_change_facts.then(|| observed_facts(&st)); drop(st); - if let (Some(before), Some(after)) = (facts_before, facts_after) + // …and a third time, on teardown. From `hangup` on, + // nothing this thread still reads describes a pane + // in use — while the facts in the record are what + // the *next* open builds a successor from. The kill + // takes the whole process group down, so a poll + // landing between the coding agent's death and the + // PTY's EOF reports "nothing recognizable in the + // foreground" and would publish that as "the agent + // left", wiping the session id `--resume` needs. + // That race is why ending a workspace's sessions + // sometimes came back to a bare shell instead of + // the conversation. The last steady-state answer is + // the one worth keeping; `live` is not ours to + // write here either — `DeathReporter` owns it. + if !shutting_down.load(Ordering::SeqCst) + && let (Some(before), Some(after)) = (facts_before, facts_after) && facts_changed(&before, &after) { let (cwd, agent) = after; @@ -3838,6 +3853,95 @@ mod tests { ); } + /// Ending a workspace's sessions has to leave a record its successor can + /// resume from. The kill hangs up the whole process group, so the coding + /// agent dies before the PTY EOFs — and a poll firing on whatever bytes + /// still come out then sees nothing recognizable in the foreground. + /// Published, that answer clears the record's agent, session id and all, and + /// the reopened workspace comes back to a bare shell instead of the + /// conversation. So a teardown publishes nothing. + /// + /// The second half is the behaviour that must *not* change: the same answer + /// about a pane nobody is tearing down means the agent exited on its own. + #[test] + fn a_pane_killed_with_its_agent_keeps_the_facts_a_resume_needs() { + use crate::core::cli_agent::{AgentSessionState, CLIAgent}; + use crate::core::machine::{ + AgentFacts, MACHINE_FILE, MachineStore, OBSERVE_SLOT, PaneSeed, publish_observations, + withdraw_observations, + }; + + const PANE: u64 = 77; + let _slot = OBSERVE_SLOT.lock().unwrap_or_else(|e| e.into_inner()); + let dir = tempfile::TempDir::new().unwrap(); + let store = MachineStore::open(dir.path().join(MACHINE_FILE)); + let ws = store.workspace_create(None, None, None).unwrap(); + store + .tab_create( + ws.id, + None, + PaneSeed { + pane: PANE, + cwd: Some("/work/api".to_string()), + ssh_spec: None, + agent: Some(AgentFacts { + agent: CLIAgent::Claude, + session_id: Some("sess-1".to_string()), + launch_argv: Some(vec!["claude".to_string()]), + status: None, + }), + }, + None, + None, + ) + .unwrap(); + publish_observations(&store); + + // One read carrying a prompt mark — which is what opens the publish + // gate — while the poll answers "nothing recognizable in the + // foreground", the reading a hung-up agent produces. + let run = |shutting_down: bool| { + let mut state = test_state(true); + state.id = PANE; + state.agent = Some(CLIAgent::Claude); + state.agent_session = Some(AgentSessionState { + session_id: Some("sess-1".to_string()), + ..Default::default() + }); + DaemonPane::spawn_reader( + Arc::new(Mutex::new(state)), + Arc::new(AtomicBool::new(shutting_down)), + Arc::new(OutputGate::new()), + Box::new(std::io::Cursor::new(b"\x1b]133;D;0\x07".to_vec())), + || false, + ForegroundProbes { + remote: Box::new(|| None), + agent: Box::new(|| Some(None)), + cwd: Box::new(|| None), + }, + Arc::new(DeathReporter::new(|| {})), + ) + .join() + .unwrap(); + }; + + run(true); + let kept = store + .pane(PANE) + .expect("the record outlives the pane") + .agent + .expect("a teardown must not report the agent away"); + assert_eq!(kept.session_id.as_deref(), Some("sess-1")); + + run(false); + assert!( + store.pane(PANE).unwrap().agent.is_none(), + "an agent that left a pane still in use is a fact, and clears" + ); + + withdraw_observations(); + } + /// The full daemon-side rich-status path: sentinel OSC events sniffed out /// of the byte stream drive the pane's session state machine, identify the /// agent when argv detection hasn't, and stream every change to the diff --git a/crates/tty7-core/src/daemon/remote_link.rs b/crates/tty7-core/src/daemon/remote_link.rs index 73d8cbe4..5ba6cff7 100644 --- a/crates/tty7-core/src/daemon/remote_link.rs +++ b/crates/tty7-core/src/daemon/remote_link.rs @@ -251,9 +251,9 @@ fn spawn_stdio_owned(program: &str, args: &[String]) -> io::Result` — this bare name would be a +/// file there is `tty7-server-cp` — 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"; diff --git a/crates/tty7-core/src/daemon/router.rs b/crates/tty7-core/src/daemon/router.rs index 1a13c2db..c69519c4 100644 --- a/crates/tty7-core/src/daemon/router.rs +++ b/crates/tty7-core/src/daemon/router.rs @@ -226,8 +226,19 @@ pub enum RouteAction { /// daemon that was serving is the one being replaced. /// /// This drops every pane that daemon hosts. It happens only when - /// a user has answered the keep-or-restart prompt with "Restart Server". + /// a user has answered the dialect-mismatch prompt with "Restart Server". RestartServer, + /// [`RestartServer`](Self::RestartServer), plus rewriting the binary first. + /// + /// The one action that installs over a server already sitting at the path + /// this client's dialect names. Nothing on the connect path does that — the + /// name is checked against the binary before it is published, so a name that + /// lies can only come from outside tty7. The handshake is what discovers it, + /// and this is what its error offers as the way out. + /// + /// Drops every pane, same as `RestartServer`, and needs the same explicit + /// answer from a user behind it. + ReplaceServer, } /// The frame that turns a local connection into a routed one. @@ -307,6 +318,14 @@ impl RouteHeader { self } + /// The same machine, asking the daemon to rewrite the `tty7-server` binary + /// this client's dialect names and restart onto it. Same warning as + /// [`restart_server`](Self::restart_server): every pane there dies. + pub fn replace_server(mut self) -> RouteHeader { + self.action = RouteAction::ReplaceServer; + self + } + /// Route to a WSL distribution on this machine. /// /// The name is not validated here: a header is data, and refusing it at @@ -660,14 +679,19 @@ impl RouteAck { } } - /// The answer to a [`RouteAction::RestartServer`] header: the machine's + /// The answer to a header that asked for a one-shot action: the machine's /// server is this client's build again, and there is no link because there /// is nothing more to say on this connection. - fn restarted() -> RouteAck { + /// + /// Echoes back the action it performed rather than hard-coding one, because + /// that echo is exactly what [`RouteAck::performed`] checks — an ack naming + /// the wrong action would read to the client as an older daemon that + /// silently did something else. + fn acted(action: RouteAction) -> RouteAck { RouteAck { ok: true, link: None, - action: Some(RouteAction::RestartServer), + action: Some(action), error: None, } } @@ -987,9 +1011,9 @@ async fn drive(local: Stream, header: &RouteHeader) -> io::Result<()> { // the last thing this connection carries. The client reconnects to // the new one on its own — the supervisor's reconnect is already the // path for "the machine's server went away". - Ok(Performed::Restarted) => { - log::info!("restarted tty7's server on {}", header.describe()); - let payload = ack_payload(&RouteAck::restarted())?; + Ok(Performed::Acted(action)) => { + log::info!("performed {action:?} on {}", header.describe()); + let payload = ack_payload(&RouteAck::acted(action))?; write_frame(&mut write_half, ROUTE_KIND, &payload).await?; None } @@ -1104,8 +1128,8 @@ enum Performed { /// The link and the connection that has to outlive it. Boxed because a /// `RemoteLink` is two orders of magnitude larger than the other variant. Linked(Box, Option>), - /// [`RouteAction::RestartServer`] ran. Nothing is left to forward. - Restarted, + /// A one-shot action ran. Nothing is left to forward. + Acted(RouteAction), } /// Do what the header asks — open a link, or carry out a one-shot action. @@ -1120,9 +1144,9 @@ async fn perform(header: &RouteHeader, setup: &RouteSetup) -> anyhow::Result { - restart_server(header, setup).await?; - Ok(Performed::Restarted) + action @ (RouteAction::RestartServer | RouteAction::ReplaceServer) => { + restart_server(header, setup, action).await?; + Ok(Performed::Acted(action)) } } } @@ -1135,9 +1159,18 @@ async fn perform(header: &RouteHeader, setup: &RouteSetup) -> anyhow::Result anyhow::Result<()> { - match &header.target { - RouteTarget::Ssh(spec) => { +async fn restart_server( + header: &RouteHeader, + setup: &RouteSetup, + action: RouteAction, +) -> anyhow::Result<()> { + match (&header.target, action) { + (RouteTarget::Ssh(spec), RouteAction::ReplaceServer) => { + SshManager::global() + .replace_remote_server(spec, setup) + .await + } + (RouteTarget::Ssh(spec), _) => { SshManager::global() .restart_remote_server(spec, setup) .await @@ -1762,7 +1795,7 @@ mod tests { assert_eq!(ack.action, None); assert!(!ack.performed(RouteAction::RestartServer)); - assert!(RouteAck::restarted().performed(RouteAction::RestartServer)); + assert!(RouteAck::acted(RouteAction::RestartServer).performed(RouteAction::RestartServer)); // A forwarding ack is not one either, which is the same daemon answering // a header whose action it *did* understand. let forwarded = RouteAck { diff --git a/crates/tty7-core/src/daemon/ssh/mod.rs b/crates/tty7-core/src/daemon/ssh/mod.rs index 49c2dc68..a63281e4 100644 --- a/crates/tty7-core/src/daemon/ssh/mod.rs +++ b/crates/tty7-core/src/daemon/ssh/mod.rs @@ -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-` — so `exec tty7-server --stdio` - // is a `command not found` on a machine the install just succeeded on. + // file there is `tty7-server-cp` — 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 { @@ -529,6 +530,26 @@ impl SshManager { Ok(()) } + /// Reinstall this client's `tty7-server` on `spec`'s host over whatever is at + /// its path, then restart the daemon onto it — "Replace Server", and **it + /// drops every pane that server is hosting**. + /// + /// Unlike [`restart_remote_server`](Self::restart_remote_server) this *does* + /// write: it is the answer to a handshake that failed against a binary whose + /// name promised a dialect it does not speak, so the file itself is what has + /// to change. See [`crate::daemon::install::Installer::replace`]. + pub async fn replace_remote_server( + &self, + spec: &NativeSshSpec, + setup: &RouteSetup, + ) -> anyhow::Result<()> { + let (conn, _reused) = self.open_connection(spec, &setup.broker).await?; + setup + .blocking(move || crate::daemon::install::replace_remote_server(&conn)) + .await??; + Ok(()) + } + /// [`open_remote_link`](Self::open_remote_link) for the daemon's std threads /// (the router runs on one). Safe from any thread that is not itself a /// runtime worker — the server's connection threads never are. diff --git a/src/ui/app.rs b/src/ui/app.rs index 21393f72..4d7b4f08 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1965,9 +1965,44 @@ impl Tty7App { .detach(); } - /// Restart the persistent background daemon: shut the running one down (which - /// stops every live shell) and bring a fresh one up, then rebuild the tabs - /// from the just-saved session so the layout returns with fresh shells. + /// The "Restart Daemon…" action: restart whichever daemon serves *this* + /// window. + /// + /// A remote window's shells live in another machine's `tty7-server`, and + /// [`restart_daemon`](Self::restart_daemon) is about this computer's. Running + /// it from a remote window ended every local session in every *other* window + /// and left the machine in front of the user untouched — a destructive button + /// that did nothing the label promised. So the action asks the window which + /// machine it is showing, and the local method keeps meaning the local daemon + /// (which is what the Settings button under "Daemon" says it does). + pub(crate) fn restart_window_daemon(&mut self, window: &mut Window, cx: &mut Context) { + let Some(remote) = WorkspaceStore::remote_ref(cx, self.workspace) else { + self.restart_daemon(window, cx); + return; + }; + let target = remote.target.clone(); + let label = crate::ui::remote_connect::label_for(&target, cx); + if !target.is_ssh() { + // Nothing to restart *over there*: this client starts the server on + // those machines itself. Said rather than silently falling back to + // restarting the local daemon, which would end sessions on a machine + // the user was not looking at. + window.push_notification( + format!( + "tty7 can only restart the server on machines it reaches over SSH. \ + {label} is served from this computer — end its sessions instead." + ), + cx, + ); + return; + } + self.confirm_restart_remote_server(target, label, window, cx); + } + + /// Restart the persistent background daemon **on this computer**: shut the + /// running one down (which stops every live shell) and bring a fresh one up, + /// then rebuild the tabs from the just-saved session so the layout returns + /// with fresh shells. /// /// A general escape hatch for the otherwise invisible, always-on daemon: /// picking up a macOS permission granted after it started (Full Disk Access @@ -4738,7 +4773,7 @@ impl Tty7App { OpenDiscord => cx.open_url(DISCORD_URL), ReportIssue => cx.open_url(ISSUES_URL), Quit => cx.quit(), - RestartDaemon => self.restart_daemon(window, cx), + RestartDaemon => self.restart_window_daemon(window, cx), ToggleSftp => self.toggle_sftp(window, cx), ShowSshForwards => self.show_ssh_forwards(window, cx), ToggleCodePanel => self.toggle_code_panel(window, cx), @@ -7077,7 +7112,7 @@ impl Render for Tty7App { this.toggle_settings(window, cx) })) .on_action(cx.listener(|this, _: &RestartDaemon, window, cx| { - this.restart_daemon(window, cx) + this.restart_window_daemon(window, cx) })) .on_action( cx.listener(|this, _: &ToggleSftp, window, cx| this.toggle_sftp(window, cx)), @@ -7239,7 +7274,21 @@ fn agent_resume_command( if !cx.global::().restore_agent_sessions { return None; } - agent.as_ref()?.resume_command(session_id?, launch_argv) + let agent = agent.as_ref()?; + // Said out loud, because it is the one step of the restore nobody can + // reconstruct afterwards: the pane comes back as a bare shell either way, + // and whether that is "no id was ever captured" (hooks not installed on + // that machine, or its record lost them) or "the agent declined to resume" + // is the whole diagnosis. A leaf that ran no agent at all is the ordinary + // case and says nothing. + let Some(session_id) = session_id else { + log::info!( + "{}'s pane had no captured session id; it comes back as a plain shell", + agent.display_name() + ); + return None; + }; + agent.resume_command(session_id, launch_argv) } /// Convert a live `Pane` tree into its serializable mirror, reading each diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index e79e4479..89496160 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -133,6 +133,21 @@ pub fn available_hosts(cx: &App) -> Vec { out } +/// The name the picker shows for `target`. +/// +/// Not `RemoteTarget`'s `Display`, which for a saved profile is its *uuid* — the +/// type deliberately cannot reach into the profile store, so anything putting a +/// machine's name in front of the user has to do this lookup. Falls back to the +/// `Display` for a machine no longer on file, which is the honest answer: that +/// is all tty7 still knows about it. +pub fn label_for(target: &RemoteTarget, cx: &App) -> String { + available_hosts(cx) + .into_iter() + .find(|host| host.target == *target) + .map(|host| host.label) + .unwrap_or_else(|| target.to_string()) +} + /// The machines matching `query`, best match first. /// /// A `~/.ssh/config` with fifty `Host` blocks is normal, and a list that long @@ -480,8 +495,9 @@ pub fn connect_blocking( /// Machines whose agent hooks this process has already looked at. static HOOKS_REFRESHED: Mutex> = Mutex::new(Vec::new()); -/// Heal this machine's stale tty7 agent hooks — the ones pointing at a -/// `tty7-server-` 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 @@ -1044,13 +1060,27 @@ pub(crate) fn claim_mailbox() -> std::sync::MutexGuard<'static, ()> { // 7. Remote daemon version skew // --------------------------------------------------------------------------- -/// The keep-or-restart question for a remote `tty7-server` at a different build. +/// 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. /// -/// The local analogue is `Tty7App::prompt_daemon_version_mismatch`, and the -/// trade is identical: the running daemon owns every live pane on that machine, -/// so restarting it throws that work away, while keeping it means talking an -/// older dialect. The one thing that differs is whose machine it is, which is -/// why the title names the host. +/// 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. +/// +/// **There is no "keep and carry on" here, and the wording must not imply one.** +/// A mismatch is only ever recorded when the running daemon's *dialects* are not +/// ours (`Installer::check_running_build`) — a merely different build that can +/// still speak to us is reused in silence and never reaches this prompt. The +/// workspace connects to the daemon that is running, so leaving it in place +/// means the connection fails in the handshake. The real choice is between +/// ending that machine's sessions and not connecting at all, and saying so is +/// the difference between a decision and a trick. pub fn mismatch_detail(m: &MismatchedRemoteDaemon) -> String { let running = match (&m.running_version, &m.running_exe) { (Some(v), Some(exe)) => format!("{v} (from {exe})"), @@ -1059,10 +1089,12 @@ pub fn mismatch_detail(m: &MismatchedRemoteDaemon) -> String { (None, None) => "an unknown build".to_string(), }; format!( - "{host} is already serving tty7 sessions from {running}, but this client is {wanted}.\n\ + "{host} is serving tty7 sessions from {running}, which speaks a protocol \ + this client ({wanted}) cannot. tty7 has installed a matching server there, \ + but the one already running is the one your sessions are on.\n\ \n\ - Keep Sessions\u{2003}everything running on {host} stays up, over the older protocol.\n\ - Restart Server\u{2003}starts {wanted} there and ends every session it is hosting.", + Restart Server\u{2003}starts {wanted} there and ends every session it is hosting.\n\ + Cancel\u{2003}leaves {host} exactly as it is. This window will not connect.", host = m.host, wanted = m.wanted_version, ) @@ -1091,6 +1123,7 @@ pub fn mismatch_target(m: &MismatchedRemoteDaemon) -> Option { /// The connection is a setup window and nothing else: the daemon acks and both /// ends close. Reconnecting afterwards is the supervisor's job, not this one's. pub fn restart_server_blocking(header: RouteHeader, label: &str) -> Result<(), String> { + let action = header.action; crate::daemon::spawn::ensure_running() .map_err(|e| format!("tty7's local daemon could not be started: {e}"))?; let mut stream = crate::daemon::transport::connect() @@ -1101,7 +1134,7 @@ pub fn restart_server_blocking(header: RouteHeader, label: &str) -> Result<(), S // connection instead — a link, not a restart. Saying nothing happened is the // only honest answer; the alternative is a "done" over a server still // running the old build. - if !ack.performed(crate::daemon::router::RouteAction::RestartServer) { + if !ack.performed(action) { return Err(format!( "this machine's tty7 daemon is an older build and cannot restart the server on \ {label}. Quit tty7 (which stops the daemon) and open it again, then retry." @@ -1382,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"); diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 2a953624..3847e00f 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -821,13 +821,13 @@ impl Tty7App { .detach(); } - /// Version skew, for a remote server. + /// Dialect skew, for a remote server. /// - /// Same shape as the local `prompt_daemon_version_mismatch`, and for the - /// same reason: the running daemon owns every live pane on that machine, so - /// this is a choice between an old dialect and losing the work. `take` - /// semantics on the producer side mean this fires once per discovery rather - /// than once per window. + /// A mismatch is only recorded when the running daemon cannot speak to this + /// client at all, so "leave it alone" is the same answer as "do not connect" + /// — the buttons say that rather than offering a Keep that would fail in the + /// handshake a moment later. `take` semantics on the producer side mean this + /// fires once per discovery rather than once per window. pub(crate) fn prompt_remote_daemon_mismatch(window: &mut Window, cx: &mut Context) { for mismatch in crate::daemon::install::take_mismatched_remote_daemons() { let title = remote_connect::mismatch_title(&mismatch); @@ -836,25 +836,84 @@ impl Tty7App { PromptLevel::Warning, &title, Some(&detail), - &["Keep Sessions", "Restart Server"], + // Named once, beside the detail that explains them. + &remote_connect::MISMATCH_ANSWERS, cx, ); cx.spawn(async move |this, cx| { - // Index 1 is Restart Server. Dismissing the prompt keeps the - // sessions, which is the answer that destroys nothing. + // Index 1 is Restart Server. Dismissing the prompt is Cancel, + // which is the answer that destroys nothing. if !matches!(answer.await, Ok(1)) { return; } let _ = this.update_in(cx, |this, window, cx| { - this.restart_remote_server(mismatch, window, cx); + this.restart_mismatched_remote_server(mismatch, window, cx); }); }) .detach(); } } - /// Carry out "Restart Server": replace the `tty7-server` on a - /// machine with this client's build. + /// [`Self::restart_remote_server`] for the machine a mismatch names: the + /// prompt knows the daemon by the record that reported it, and the record + /// has to be turned back into something addressable before anything can be + /// asked of it. + fn restart_mismatched_remote_server( + &mut self, + mismatch: crate::daemon::install::MismatchedRemoteDaemon, + window: &mut Window, + cx: &mut Context, + ) { + let label = mismatch.host.clone(); + match remote_connect::mismatch_target(&mismatch) + .ok_or_else(|| format!("tty7 no longer has a way to reach {label}")) + { + Ok(target) => self.restart_remote_server(target, label, window, cx), + Err(e) => Tty7App::report_restart_failure(&label, &e, window, cx), + } + } + + /// "Restart Server" for a machine with nothing wrong with it — the + /// switcher's machine menu, and where a remote window's "Restart Daemon…" + /// lands. + /// + /// Same outcome and same warning as the two repair paths above; the only + /// difference is that nothing is broken, so the wording claims nothing is. + /// Confirmed for the reason all three are: every session on that machine + /// ends, including the ones other windows are showing. + pub(crate) fn confirm_restart_remote_server( + &mut self, + target: RemoteTarget, + label: String, + window: &mut Window, + cx: &mut Context, + ) { + let answer = window.prompt( + PromptLevel::Warning, + &format!("Restart tty7's server on \u{201c}{label}\u{201d}?"), + Some(&format!( + "This stops every session on {label} — anything still running in them \ + will be terminated, including sessions this window is not showing. \ + Workspaces and layouts are kept and come back with fresh shells." + )), + &["Cancel", "Restart Server"], + cx, + ); + cx.spawn(async move |this, cx| { + // Index 1 is Restart Server; a dismissed prompt is Cancel. + if !matches!(answer.await, Ok(1)) { + return; + } + let _ = this.update_in(cx, |this, window, cx| { + this.restart_remote_server(target, label, window, cx); + }); + }) + .detach(); + } + + /// Carry out "Restart Server": stop the `tty7-server` on a machine and start + /// this client's build in its place. The half every entry point shares, past + /// whichever prompt asked. /// /// **This throws work away and says so.** Every pane the old server hosts /// dies with it — that is what the prompt the user just answered warns @@ -864,15 +923,12 @@ impl Tty7App { /// layout: same tabs and splits, new shells, nothing running in them. fn restart_remote_server( &mut self, - mismatch: crate::daemon::install::MismatchedRemoteDaemon, + target: RemoteTarget, + label: String, window: &mut Window, cx: &mut Context, ) { - let label = mismatch.host.clone(); - let header = match remote_connect::mismatch_target(&mismatch) - .ok_or_else(|| format!("tty7 no longer has a way to reach {label}")) - .and_then(|target| remote_connect::control_route(&target, cx)) - { + let header = match remote_connect::control_route(&target, cx) { Ok(header) => header.restart_server(), Err(e) => { Tty7App::report_restart_failure(&label, &e, window, cx); @@ -880,11 +936,16 @@ impl Tty7App { } }; let host = header.target.origin_key(); + // From the target, not from the connection key: that is how the switcher + // derives the id it looks the phase up under, and a bar keyed to a + // different id than the panel reads is a bar nobody ever sees. + let host_id = target.host_id(); log::info!("restarting tty7's server on {label} at the user's request"); // The same watcher the connect flow uses: a restart re-opens the - // machine's connection, so it can raise a password sheet on the way in. + // machine's connection, so it can raise a password sheet on the way in, + // and it reports a phase the panel has to be told to look at. let running = Arc::new(std::sync::atomic::AtomicBool::new(true)); - self.watch_for_restart_consent(running.clone(), cx); + self.watch_for_restart_consent(host_id, running.clone(), cx); cx.spawn(async move |this, cx| { let for_task = label.clone(); let outcome = cx @@ -892,6 +953,7 @@ impl Tty7App { .spawn(async move { remote_connect::restart_server_blocking(header, &for_task) }) .await; running.store(false, std::sync::atomic::Ordering::Relaxed); + remote_connect::clear_install_progress(host_id); let _ = this.update_in(cx, |_, window, cx| match outcome { Ok(()) => { log::info!("{label} is now serving this client's build"); @@ -906,6 +968,117 @@ impl Tty7App { .detach(); } + /// "Restart Server", from the error card: put a `tty7-server` this client + /// can talk to on the machine — writing one first only if the binary at our + /// dialect's path cannot — and restart the daemon onto it. + /// + /// **The same button, the same words, and the same outcome as the mismatch + /// prompt's.** Both make that machine's running server one we can speak to + /// and end everything on it; they differ only in what had to happen for the + /// machine to get into each state, which is not the user's problem. Naming + /// the two apart ("Replace" here, "Restart" there) asked them to tell + /// identical outcomes apart by a distinction that only exists inside + /// [`Installer::replace`]. The internal names stay split because the actions + /// really are a superset and a subset. + /// + /// **Confirmed first, because it destroys work.** The connect that failed + /// proves nothing about the *other* panes on that machine — an older daemon + /// can be serving them perfectly well over its own dialect — and they all go + /// with it. The failure the button sits under is a good reason to offer + /// this, never a reason to do it unasked. + /// + /// Reached only from a connect error that [`is_dialect_refusal`] recognises, + /// which is the one failure this can fix. + /// + /// [`is_dialect_refusal`]: crate::daemon::control::is_dialect_refusal + /// [`Installer::replace`]: crate::daemon::install::Installer::replace + pub(crate) fn confirm_replace_remote_server( + &mut self, + target: RemoteTarget, + label: String, + window: &mut Window, + cx: &mut Context, + ) { + let answer = window.prompt( + PromptLevel::Warning, + &format!("Restart tty7's server on \u{201c}{label}\u{201d}?"), + Some(&format!( + "The tty7-server running on {label} speaks a protocol this client cannot. \ + tty7 will restart the service there onto one that does, installing it \ + first if {label} does not already have it.\n\ + \n\ + Every session running on {label} ends, including any this window is not \ + connected to." + )), + &["Cancel", "Restart Server"], + cx, + ); + cx.spawn(async move |this, cx| { + // Index 1 is Restart Server; a dismissed prompt is Cancel. + if !matches!(answer.await, Ok(1)) { + return; + } + let _ = this.update_in(cx, |this, window, cx| { + this.replace_remote_server(target, label, window, cx); + }); + }) + .detach(); + } + + /// The half of [`Self::confirm_replace_remote_server`] that runs after the + /// user has said yes. + fn replace_remote_server( + &mut self, + target: RemoteTarget, + label: String, + window: &mut Window, + cx: &mut Context, + ) { + 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; + } + }; + let host = route.target.origin_key(); + let host_id = target.host_id(); + log::info!("replacing tty7's server on {label} at the user's request"); + // The same watcher the restart path uses: replacing re-opens the + // machine's connection, so it can raise a password sheet on the way in, + // and it writes, so it can raise the install-consent sheet too. + let running = Arc::new(std::sync::atomic::AtomicBool::new(true)); + self.watch_for_restart_consent(host_id, running.clone(), cx); + cx.spawn(async move |this, cx| { + let for_task = label.clone(); + let outcome = cx + .background_executor() + .spawn(async move { remote_connect::restart_server_blocking(route, &for_task) }) + .await; + running.store(false, std::sync::atomic::Ordering::Relaxed); + // Either way the bar is over. On failure the error card takes the + // space back, which it cannot do while a phase is still recorded. + remote_connect::clear_install_progress(host_id); + let _ = this.update_in(cx, |_, window, cx| match outcome { + Ok(()) => { + log::info!("{label} is now serving this client's build"); + reconnect_after_restart(&host, cx); + } + Err(e) => { + log::warn!("could not replace tty7's server on {label}: {e}"); + Tty7App::report_restart_failure(&label, &e, window, cx); + } + }); + }) + .detach(); + } + /// Tell the user a restart did not happen, rather than leaving the old /// server running behind a prompt that closed as if it had. /// @@ -948,11 +1121,24 @@ impl Tty7App { /// so it costs nothing when nothing is restarting. fn watch_for_restart_consent( &self, + host: HostId, running: Arc, cx: &mut Context, ) { - cx.spawn(async move |_, cx| { + cx.spawn(async move |this, cx| { + let mut painted: Option = None; while running.load(std::sync::atomic::Ordering::Relaxed) { + // The same repaint `watch_for_install_consent` does, and needed + // for the same reason: the sink is written from the routed + // connection's reader thread and nothing else would ask the + // panel to look at it. Without this a restart that transfers + // nothing — the common "Restart Server" — leaves the click with + // no visible effect for the length of two timeouts. + let reported = remote_connect::install_progress_for(host); + if reported != painted { + painted = reported; + let _ = this.update(cx, |_, cx| cx.notify()); + } cx.update(pump_auth_sheets); cx.background_executor() .timer(Duration::from_millis(100)) diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 5eec28c1..03805c3c 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -5528,7 +5528,7 @@ impl Tty7App { .child("Daemon"), ) .child(div().text_sm().text_color(muted_fg).child( - "Restart the daemon to pick up a newly granted macOS permission, recover if it stops responding, or start from a clean slate. This ends all running sessions; your tabs and layout reopen with fresh shells.", + "Restart the daemon on this computer to pick up a newly granted macOS permission, recover if it stops responding, or start from a clean slate. This ends all running sessions here; your tabs and layout reopen with fresh shells. A remote machine's server is restarted from its own menu in the workspace switcher.", )) .child( h_flex().child( diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 7ae6e0e2..21378b16 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -451,8 +451,25 @@ impl Tty7App { // Only while *this* window is the one connecting. Another window's // install is its own business, and a bar under a row this panel is // not driving would have no "Try Again" to turn into. - if group.link == Link::Connecting { - 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 @@ -878,50 +895,97 @@ impl Tty7App { // A failure is a resting state — it stays on screen with its reason // in full and its next move one click away, rather than reverting the // panel and leaving the user to guess between VPN, keys and the box. - if let Some(error) = group.error.as_ref() { + // Not while something is being done about it: a stale reason sitting + // above a live progress bar reads as two states at once, and the two + // buttons under it are exactly what must not be clicked twice. + if let Some(error) = group.error.as_ref().filter(|_| group.installing.is_none()) { let retry = GroupRef::of(group); + let replace = retry.clone(); let theme = cx.theme(); - block = block.child( - v_flex() - .gap(px(4.)) - .ml(px(KID_INDENT)) - .mr(px(4.)) - .mb(px(2.)) - .px(px(10.)) - .py(px(8.)) - .rounded(px(6.)) - .border_1() - .border_color(theme.danger.opacity(0.35)) - .child( - div() - .text_xs() - .text_color(theme.muted_foreground) - .child(error.clone()), - ) - .child( - Button::new(gpui::SharedString::from(format!( - "switcher-retry:{}", - group.key - ))) - .label("Try Again") - .ghost() - .xsmall() - .on_click(cx.listener( - move |this, _, _window, cx| { - if let Some(target) = retry.target.clone() { - this.connect_to_host( - HostChoice { - target, - label: retry.label.clone(), - detail: String::new(), - }, - cx, - ); - } - }, - )), - ), - ); + block = + block.child( + v_flex() + .gap(px(4.)) + .ml(px(KID_INDENT)) + .mr(px(4.)) + .mb(px(2.)) + .px(px(10.)) + .py(px(8.)) + .rounded(px(6.)) + .border_1() + .border_color(theme.danger.opacity(0.35)) + .child( + div() + .text_xs() + .text_color(theme.muted_foreground) + .child(error.clone()), + ) + .child( + h_flex() + .gap(px(4.)) + .child( + Button::new(gpui::SharedString::from(format!( + "switcher-retry:{}", + group.key + ))) + .label("Try Again") + .ghost() + .xsmall() + .on_click(cx.listener(move |this, _, _window, cx| { + if let Some(target) = retry.target.clone() { + this.connect_to_host( + HostChoice { + target, + label: retry.label.clone(), + detail: String::new(), + }, + cx, + ); + } + })), + ) + // Only for the one failure a reinstall fixes. Every + // other reason a connect fails (unreachable, refused + // key, no route) would cost the user every pane on + // that machine and not help — so the button is not + // there to be misread as a general retry. + .when( + crate::daemon::control::is_dialect_refusal(error) + && replace.target.is_some(), + |row| { + row.child( + Button::new(gpui::SharedString::from(format!( + "switcher-replace:{}", + group.key + ))) + // The same words as the mismatch + // prompt's button, because it is the + // same thing to the user: this + // machine's server becomes one this + // client can talk to, and everything + // running on it ends. Whether a binary + // has to be written on the way is an + // implementation detail, and a second + // verb for it only asks the user to + // tell two identical outcomes apart. + .label("Restart Server") + .ghost() + .xsmall() + .on_click(cx.listener(move |this, _, window, cx| { + if let Some(target) = replace.target.clone() { + this.confirm_replace_remote_server( + target, + replace.label.clone(), + window, + cx, + ); + } + })), + ) + }, + ), + ), + ); } // A machine with no workspaces on it renders as its header alone. There // used to be a "New Workspace" row to fill the space; it lives in the @@ -957,21 +1021,27 @@ impl Tty7App { // The same warning colour the header's dot and "installing…" already // use, so the row and the bar read as one state and not two. let accent = theme.warning; - let (verb, done, total) = match phase { - InstallPhase::Downloading { done, total } => ("Downloading", done, total), - InstallPhase::Uploading { done, total } => ("Copying", done, Some(total)), - }; // An unknown total (no Content-Length) still gets a line of text and a // bar — just an empty one. A bar that guessed at a fraction would be // lying, and one that vanished would read as the install having stopped. + // A restart is the same shape for a different reason: it is two timeouts + // and no transfer, so there is nothing it could honestly fill. let fraction = phase.fraction().unwrap_or(0.0); - let caption = match total { - Some(total) => format!( - "{verb} tty7's server… {} / {}", + let caption = match phase { + InstallPhase::Restarting => "Restarting tty7's server\u{2026}".to_string(), + InstallPhase::Downloading { done, total } => match total { + Some(total) => format!( + "Downloading tty7's server\u{2026} {} / {}", + human_bytes(done), + human_bytes(total) + ), + None => format!("Downloading tty7's server\u{2026} {}", human_bytes(done)), + }, + InstallPhase::Uploading { done, total } => format!( + "Copying tty7's server\u{2026} {} / {}", human_bytes(done), human_bytes(total) ), - None => format!("{verb} tty7's server… {}", human_bytes(done)), }; v_flex() @@ -1074,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…")) } @@ -1559,7 +1633,7 @@ fn group_menu( group: &GroupRef, app: gpui::WeakEntity, ) -> gpui_component::menu::PopupMenu { - let (a1, a2) = (app.clone(), app); + let (a1, a2, a3) = (app.clone(), app.clone(), app); let gref = group.clone(); // A remote machine can only be given a workspace once a handshake has said // where its `$HOME` is — `~` guessed from this client would be the wrong @@ -1578,12 +1652,39 @@ fn group_menu( return menu; }; let connected = group.link == Link::Connected; - menu.separator().item( + let restartable = target.is_ssh(); + let (label, for_restart) = (group.label.clone(), target.clone()); + let menu = menu.separator().item( PopupMenuItem::new("Disconnect") .disabled(!connected) .on_click(move |_, _window, cx| { let _ = a2.update(cx, |this, cx| this.switcher_disconnect(&target, cx)); }), + ); + if !restartable { + // A WSL distribution's server is started by this client and a + // `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; + } + // Deliberately not gated on `connected`. A server that has to be restarted + // is most often one this client *cannot* reach any more, and the action + // opens its own connection to do the work — requiring a live link would + // withhold the verb from exactly the machine that needs it. + // + // And deliberately *not* closing the panel, unlike the row's destructive + // items. This panel is where a restart has anything to show — the phase bar + // under the machine's header, its rows going dead and coming back — and the + // error card's identical button already leaves it open for that reason. + menu.item( + PopupMenuItem::new("Restart Server…").on_click(move |_, window, cx| { + let _ = a3.update(cx, |this, cx| { + this.confirm_restart_remote_server(for_restart.clone(), label.clone(), window, cx); + }); + }), ) }