diff --git a/crates/tty7-core/src/core/agent_hooks.rs b/crates/tty7-core/src/core/agent_hooks.rs index 74edf7de..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 _}; diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index bb9e5959..97fc4ec1 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -298,18 +298,27 @@ impl RemoteTarget { /// Whether this machine is reached over SSH. /// - /// The question "Restart Server" asks. The other two variants have no - /// long-lived daemon on the far side to restart: a WSL distribution's server - /// is started by this client, and a `LocalStdio` machine is a child process - /// per connection — which is why - /// [`router::restart_server`](crate::daemon::router) refuses them. Asked - /// here rather than re-spelled at each call site, so the UI that offers the - /// verb and the router that carries it out cannot disagree about who has it. + /// The question "Restart Server" asks, and the answer + /// [`router::restart_server`](crate::daemon::router) already gives: it routes + /// the action for SSH machines and refuses the other two. A `LocalStdio` + /// machine is a child process per connection, so there is nothing there to + /// stop and start; a WSL distribution's server is started by this client, + /// which makes "stop it and reconnect" the whole of the verb and not + /// something a routed action has to carry out. Asked here rather than + /// re-spelled at each call site, so the UI that offers the verb and the + /// router that carries it out cannot disagree about who has it. + /// + /// Spelled out variant by variant rather than as a `matches!` of the three + /// that say yes: this gates an action that ends every session on a machine, + /// and a new [`RemoteTarget`] must not inherit an answer to that by falling + /// off the end of a pattern. The compiler asks instead. pub fn is_ssh(&self) -> bool { - matches!( - self, - RemoteTarget::Profile { .. } | RemoteTarget::Alias { .. } | RemoteTarget::Direct { .. } - ) + match self { + RemoteTarget::Profile { .. } + | RemoteTarget::Alias { .. } + | RemoteTarget::Direct { .. } => true, + RemoteTarget::Wsl { .. } | RemoteTarget::LocalStdio { .. } => false, + } } /// The in-process id this target resolves to. diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index 7e30601b..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") } @@ -1162,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 @@ -1653,6 +1659,17 @@ fn launch_script(binary: &str, settle: Option) -> String { /// `rename`) is the price of the collision it prevents, and it is bounded by how /// often that happens — which is "almost never", against "every time two clients /// install the same dialect at once" for the shared name. +/// +/// **A pid, so private to a process and not to a client.** Two tty7 processes on +/// one machine (the released build and the one you are compiling) cannot collide; +/// two on *different* machines that happen to share a pid still can. That +/// remainder is left alone because the `--protocol` check now stands behind it: +/// bytes from two uploads interleaved into one file do not answer with our +/// dialect, so the outcome is a [`InstallError::DialectMismatch`] and a removed +/// temp rather than a published binary that lies. Two installs from *within* one +/// process share a pid and so share this path too — that is what `wsl`'s +/// `INSTALL_LOCKS` and `SshManager`'s per-key `ConnSlot` are for, and this is not +/// a second attempt at their job. fn unique_temp(shared: &str) -> String { let pid = std::process::id(); match shared.strip_suffix(".tmp") { @@ -1695,14 +1712,15 @@ fn connection_label(conn: &SshConnection) -> String { /// binary plus a live daemon costs two SSH commands and one SFTP stat, no /// download, no prompt. /// -/// The returned path is **absolute and version-qualified** -/// (`~/.local/share/tty7/bin/tty7-server-`), 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. diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 50d0c9eb..5fd681d8 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -519,7 +519,7 @@ fn first_install_runs_all_six_steps() { /// **Atomic replacement.** The final path must only ever be produced by /// renaming a temp that is *already* executable — never written to directly, /// and never chmod'ed after it is visible. Both would leave a window in which a -/// concurrent connect finds `tty7-server-` 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(); diff --git a/crates/tty7-core/src/daemon/install/wsl.rs b/crates/tty7-core/src/daemon/install/wsl.rs index f11f4649..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 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/ssh/mod.rs b/crates/tty7-core/src/daemon/ssh/mod.rs index 26155d46..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 { diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index 5d6640cd..89496160 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -495,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 @@ -1059,6 +1060,16 @@ pub(crate) fn claim_mailbox() -> std::sync::MutexGuard<'static, ()> { // 7. Remote daemon version skew // --------------------------------------------------------------------------- +/// The answers the dialect-mismatch prompt offers, in the order `window.prompt` +/// takes them — **index 1 is the destructive one**, which is what +/// `prompt_remote_daemon_mismatch` matches on. +/// +/// Written down here rather than at the prompt because [`mismatch_detail`] spells +/// both out by name in its body: a detail explaining a button that is no longer +/// there is worse than no explanation at all. `Keep Sessions` used to be index 0 +/// and had to go, which is precisely the drift this prevents repeating. +pub const MISMATCH_ANSWERS: [&str; 2] = ["Cancel", "Restart Server"]; + /// The restart-or-cancel question for a remote `tty7-server` this client cannot /// talk to. /// @@ -1404,6 +1415,24 @@ mod tests { assert!(mismatch_detail(&unknown).contains("an unknown build")); } + /// The detail explains the buttons by name, so it has to name the ones that + /// are actually there. This is a prompt whose whole job is to make a + /// destructive choice legible; a body describing an answer the prompt does + /// not offer (as it did while `Keep Sessions` was one of them) turns that + /// back into a guess. + #[test] + fn the_mismatch_detail_explains_every_answer_the_prompt_offers() { + let detail = mismatch_detail(&MismatchedRemoteDaemon { + host: "me@build-box:22".into(), + running_version: Some("0.8.0".into()), + running_exe: None, + wanted_version: "0.9.1".into(), + }); + for answer in MISMATCH_ANSWERS { + assert!(detail.contains(answer), "{answer} is unexplained: {detail}"); + } + } + #[test] fn endpoint_labels_hide_the_default_port() { assert_eq!(endpoint_label("me", "box.local", 22), "me@box.local"); diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 283bde45..3847e00f 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -836,7 +836,8 @@ impl Tty7App { PromptLevel::Warning, &title, Some(&detail), - &["Cancel", "Restart Server"], + // Named once, beside the detail that explains them. + &remote_connect::MISMATCH_ANSWERS, cx, ); cx.spawn(async move |this, cx| { @@ -1017,8 +1018,8 @@ impl Tty7App { if !matches!(answer.await, Ok(1)) { return; } - let _ = this.update_in(cx, |this, _window, cx| { - this.replace_remote_server(target, label, cx); + let _ = this.update_in(cx, |this, window, cx| { + this.replace_remote_server(target, label, window, cx); }); }) .detach(); @@ -1030,12 +1031,19 @@ impl Tty7App { &mut self, target: RemoteTarget, label: String, + window: &mut Window, cx: &mut Context, ) { 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; } }; diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 24280475..21378b16 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -452,13 +452,24 @@ impl Tty7App { // install is its own business, and a bar under a row this panel is // not driving would have no "Try Again" to turn into. // - // `error` counts too, and is not an exception to that: it is the - // state a machine is in while the error card.s "Restart Server" — a - // exists inside this panel's own error card — is working on it. Left - // out, the one flow that transfers nothing would show nothing at all - // for the length of two timeouts. - if group.link == Link::Connecting || group.error.is_some() { - group.installing = remote_connect::install_progress_for(id); + // `error` counts too, and is not an exception to that: a machine + // being worked on by the "Restart Server" inside this panel's own + // error card is still a machine this panel is driving. + // + // A restart is the exception, and has to be. It is offered from the + // machine's `⋯` menu whatever the link is doing — a server worth + // restarting is most often one nothing can reach — so gating it on + // *this* window's connect would hide the bar in the ordinary case. + // And it is the one flow that transfers nothing, which makes the bar + // the only thing that says the click landed, for the length of two + // timeouts. Showing another window's restart is right rather than + // merely tolerable: it is about to end the sessions in this one too. + let reported = remote_connect::install_progress_for(id); + if group.link == Link::Connecting + || group.error.is_some() + || matches!(reported, Some(InstallPhase::Restarting)) + { + group.installing = reported; } // Read app-wide, not from this window's snapshot: any window's // connect, and every reconnect, records the machine's `$HOME` — and @@ -1133,7 +1144,11 @@ impl Tty7App { Link::Connected => (Some(gpui::rgb(crate::ui::tab_strip::LIVE_DOT).into()), None), // "installing…" while bytes are moving: the bar underneath says how // far along, and a header still reading "connecting…" over it would - // describe a step that finished a while ago. + // describe a step that finished a while ago. A restart moves no + // bytes, so it gets its own word rather than borrowing that one. + Link::Connecting if matches!(group.installing, Some(InstallPhase::Restarting)) => { + (Some(theme.warning), Some("restarting…")) + } Link::Connecting if group.installing.is_some() => { (Some(theme.warning), Some("installing…")) } @@ -1648,8 +1663,9 @@ fn group_menu( ); if !restartable { // A WSL distribution's server is started by this client and a - // `LocalStdio` one is a child process per connection, so there is no - // daemon over there to restart — the router says the same. Absent rather + // `LocalStdio` one is a child process per connection, so neither has a + // daemon a routed action could restart — the router refuses both, and + // `RemoteTarget::is_ssh` is where the two agree. Absent rather // than greyed out, for the reason "Disconnect" is absent from the local // group: a permanently disabled row only invites the question. return menu;