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); + }); + }), ) }