From e98586bdbd6d0adeffb86cef0028c6d6df3a0994 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 6 Sep 2026 23:08:49 +0800 Subject: [PATCH] fix(remote): keep the reason a remote server failed to start (#774) (#777) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A remote workspace could sit in a loop nobody could get out of: every reconnect failed with "started but nothing was answering on the control socket after 15s", the strip showed a copy bar frozen at 100%, and no button was offered. The daemon is the root of it. When its control listener would not open it logged one line and kept running — and a running daemon holds the single-server lock, so every later --daemon stood down at once and every client probe failed, forever. Whether something else is serving cannot be read off the errno: bind_control_socket clears the leftovers it can, but a path it cannot clear comes back AddrInUse in the same words a live server does. Ask by connecting, and exit when nothing answers. The reason was thrown away twice over: the daemon's stdout and stderr went to /dev/null, and the readiness probe kept only out.success(). Both are kept now — output and exit status land beside the binary, stamped with the launch's own nonce so a restart never reads the outgoing daemon's status as the incoming one's. A start that has already failed no longer waits out the full timeout. The UI half: an automatic reconnect never retired its install progress, and a leftover entry draws an install in flight instead of the failure and its button. And a long error stretched the status card to 1978px in a 1440px window, taking the retry button off the screen with it. Closes part of #774. The Vim :wq cursor and the btop re-attach items in that issue are not touched. Claude-Session: https://claude.ai/code/session_015q6HRem76HYy33T39bp34c --- crates/tty7-core/src/daemon/install/mod.rs | 274 +++++++++++++++--- crates/tty7-core/src/daemon/install/tests.rs | 283 ++++++++++++++++++- crates/tty7-core/src/daemon/server.rs | 26 +- crates/tty7-core/src/host/server.rs | 84 +++++- src/ui/app.rs | 21 +- src/ui/home.rs | 159 ++++++++++- src/ui/i18n/en.rs | 2 +- src/ui/i18n/ja.rs | 2 +- src/ui/i18n/mod.rs | 2 +- src/ui/i18n/zh.rs | 2 +- src/ui/remote_workspace.rs | 130 ++++++++- 11 files changed, 898 insertions(+), 87 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index a23d2dff..220d085a 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -301,8 +301,17 @@ pub fn install_confirm() -> Arc { #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "snake_case")] pub enum InstallPhase { - Downloading { done: u64, total: Option }, - Uploading { done: u64, total: u64 }, + Downloading { + done: u64, + total: Option, + }, + Uploading { + done: u64, + total: u64, + }, + /// The bytes are there and the far end is being waited on. Both a first + /// install and a restart end here, which is why its caption says starting + /// rather than restarting. Restarting, } @@ -777,6 +786,18 @@ impl<'a> Installer<'a> { let (confirmed, _) = self.install(asset, &paths)?; report.installed = true; report.confirmed = confirmed; + // The upload's caption was the last thing reported, so the + // strip sat on "copying… 100%" through the startup wait — + // and when the wait failed, that is the frame the user was + // left looking at. + // + // Reported here rather than in the wait itself: an + // `ensure_daemon` with nothing to install has to stay + // silent. A pane route quietly restarting a dead remote + // daemon goes through that path, and nothing on it retires + // a progress entry — one left there freezes the strip and + // takes its button away for the rest of the session. + install_progress().report(&self.host, InstallPhase::Restarting); } } } @@ -915,9 +936,21 @@ impl<'a> Installer<'a> { ReleaseDownload { fetch }.load_with_progress(&self.version, asset, &on_progress) } + /// Whether this machine has never had a tty7 server on it — the question + /// the consent prompt turns on. + /// + /// A launch leaves `tty7-server-c8p6.startup.log` and `.startup.exit` + /// beside the binary, and both begin the same way it does. Counting one as + /// a server would let someone who deleted the binary to uninstall tty7 get + /// a new one downloaded and written without ever being asked. fn is_first_install(&self, paths: &RemotePaths) -> bool { + let a_server = |name: &String| { + name.starts_with("tty7-server-") + && !name.ends_with(STARTUP_LOG_SUFFIX) + && !name.ends_with(STARTUP_EXIT_SUFFIX) + }; match self.ops.list_dir(&paths.bin_dir) { - Ok(Some(entries)) => !entries.iter().any(|name| name.starts_with("tty7-server-")), + Ok(Some(entries)) => !entries.iter().any(a_server), Ok(None) => true, Err(_) => true, } @@ -931,41 +964,71 @@ impl<'a> Installer<'a> { return Ok((false, self.check_running_build(paths))); } - self.launch_daemon(paths)?; + let log = self.launch_daemon(paths)?; + self.wait_for_launch(paths, &log)?; + Ok((true, self.check_running_build(paths))) + } + /// Wait out a daemon this call just launched, and say what happened if it + /// never answers. + /// + /// Two things used to be thrown away here, and between them they made every + /// remote startup failure read the same: the probe's exit status and stderr + /// (only `out.success()` survived) and the daemon's own output ([`launch_command`] + /// sent it to `/dev/null`). All anyone ever saw was "nothing was answering + /// after 15s", which names the symptom and not one cause. + fn wait_for_launch(&self, paths: &RemotePaths, log: &StartupLog) -> Result<(), InstallError> { let deadline = Instant::now() + self.startup_timeout; loop { - if self.daemon_is_serving(paths)? { - return Ok((true, self.check_running_build(paths))); + let probe = match self.control_probe(paths)? { + None => return Ok(()), + Some(reason) => reason, + }; + // A process that has already *failed* will not start answering. + // Ask before sleeping again: waiting out the full timeout for a + // daemon that died in the first 200ms is fifteen seconds of + // nothing. + // + // A clean exit is not that. `run_daemon` returns success when + // another server already holds the single-server lock, so status 0 + // means "someone else is the server here" — which is good news + // arriving early. Keep probing for that someone; only the deadline + // ends this. + let exit = log.exit_status(self.ops).filter(|status| status != "0"); + if exit.is_none() && Instant::now() < deadline { + std::thread::sleep(self.poll_interval); + continue; } - if Instant::now() >= deadline { - return Err(InstallError::Launch { - reason: format!( - "{} started but nothing was answering on the control socket after {:?}", - paths.binary, self.startup_timeout - ), - }); - } - std::thread::sleep(self.poll_interval); + return Err(InstallError::Launch { + reason: log.explain(self.ops, &paths.binary, &probe, exit, self.startup_timeout), + }); } } - fn daemon_is_serving(&self, paths: &RemotePaths) -> Result { + /// `None` when the control socket answered, `Some(why)` when it did not. + fn control_probe(&self, paths: &RemotePaths) -> Result, InstallError> { let cmd = format!( "{} --stdio --bridge < /dev/null", shell_quote(&paths.binary) ); match self.ops.run(&cmd) { - Ok(out) => Ok(out.success()), + Ok(out) if out.success() => Ok(None), + Ok(out) => Ok(Some(out.failure_reason())), Err(reason) => Err(InstallError::Launch { reason }), } } - fn launch_daemon(&self, paths: &RemotePaths) -> Result<(), InstallError> { + fn daemon_is_serving(&self, paths: &RemotePaths) -> Result { + Ok(self.control_probe(paths)?.is_none()) + } + + fn launch_daemon(&self, paths: &RemotePaths) -> Result { + let log = StartupLog::for_binary(&paths.binary); let settle = self.ops.launch_settle(&paths.binary); self.ops - .spawn_detached(&launch_script(&paths.binary, settle)) - .map_err(|reason| InstallError::Launch { reason }) + .spawn_detached(&launch_script(&paths.binary, &log, settle)) + .map_err(|reason| InstallError::Launch { reason })?; + Ok(log) } fn check_running_build(&self, paths: &RemotePaths) -> Option { @@ -1065,19 +1128,8 @@ impl<'a> Installer<'a> { std::thread::sleep(self.poll_interval); } - self.launch_daemon(paths)?; - let deadline = Instant::now() + self.startup_timeout; - loop { - if self.daemon_is_serving(paths)? { - return Ok(()); - } - if Instant::now() >= deadline { - return Err(InstallError::Launch { - reason: format!("{} was restarted but never started answering", paths.binary), - }); - } - std::thread::sleep(self.poll_interval); - } + let log = self.launch_daemon(paths)?; + self.wait_for_launch(paths, &log) } } @@ -1110,19 +1162,163 @@ const RUNNING_EXE_COMMAND: &str = r#"if [ -d /proc ]; then for p in /proc/[0-9]* const TERMINATE_RUNNING_COMMAND: &str = r#"if [ -d /proc ]; then for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) kill -TERM "${p#/proc/}" 2>/dev/null; break;; esac; done; else ps -xwwo pid=,comm= 2>/dev/null | while read -r pid e; do case "$e" in */tty7-server-*) kill -TERM "$pid" 2>/dev/null; break;; esac; done; fi; true"#; -fn launch_command(binary: &str) -> String { +/// The two files a launch writes beside the server binary. Named here because +/// `is_first_install` reads the same directory and must not mistake one of +/// these for an installed server. +pub(crate) const STARTUP_LOG_SUFFIX: &str = ".startup.log"; +pub(crate) const STARTUP_EXIT_SUFFIX: &str = ".startup.exit"; + +/// Where a launched daemon's output and exit status land on the far end. +/// +/// One pair of files per binary, truncated at each launch rather than named +/// uniquely per attempt: these exist to be read seconds later by the client +/// that wrote them, and a unique name per launch would leave a file behind on +/// every reconnect for nobody to ever delete. +/// +/// What makes the fixed name safe is the nonce. A restart tells the old daemon +/// to stop and the new one to start, and the old one's wrapper records its +/// status only once the old process is fully gone — which is after its socket +/// stopped answering, so it can land *after* the new launch truncated the file. +/// Reading that as the new daemon's status would fail a restart that is going +/// fine. The status is written with the nonce that asked for it, and a status +/// carrying anyone else's is not an answer to this launch. +pub(crate) struct StartupLog { + log: String, + exit: String, + nonce: String, +} + +/// How much of the far end's startup log to fetch. +const TAIL_BYTES: usize = 4096; + +/// How much of it reaches the error string. The whole tail goes to this +/// client's log; a status strip gets the last few lines, which is where the +/// reason always is. +const TAIL_LINES: usize = 4; + +impl StartupLog { + pub(crate) fn for_binary(binary: &str) -> Self { + Self { + log: format!("{binary}{STARTUP_LOG_SUFFIX}"), + exit: format!("{binary}{STARTUP_EXIT_SUFFIX}"), + nonce: uuid::Uuid::new_v4().simple().to_string(), + } + } + + /// The exit status the wrapper recorded for *this* launch, or `None` while + /// the daemon is still running. Only the wrapper writes this file, and only + /// once it has waited for the daemon, so a status carrying this launch's + /// nonce is the death certificate. + fn exit_status(&self, ops: &dyn RemoteOps) -> Option { + let cmd = format!("cat {} 2>/dev/null", shell_quote(&self.exit)); + let out = ops.run(&cmd).ok()?; + let (nonce, status) = out.stdout.trim().split_once(' ')?; + (nonce == self.nonce && !status.is_empty()).then(|| status.to_string()) + } + + fn tail(&self, ops: &dyn RemoteOps) -> String { + let cmd = format!( + "tail -c {TAIL_BYTES} {} 2>/dev/null", + shell_quote(&self.log) + ); + ops.run(&cmd).map(|out| out.stdout).unwrap_or_default() + } + + /// Why the daemon never answered, in a sentence a status strip can show. + /// + /// The full tail goes to this client's log; the message keeps the last few + /// lines of it, because the interesting one is always the last thing the + /// server managed to say before it gave up. + fn explain( + &self, + ops: &dyn RemoteOps, + binary: &str, + probe: &str, + exit: Option, + waited: Duration, + ) -> String { + let tail = self.tail(ops); + if !tail.trim().is_empty() { + log::warn!("remote {binary} startup log ({}):\n{tail}", self.log); + } + let what = match &exit { + Some(status) => format!("{binary} exited with status {status} before it answered"), + None => format!("{binary} was still not answering after {waited:?}"), + }; + let mut out = format!("{what} on the control socket"); + if !probe.trim().is_empty() { + out.push_str(&format!("; last probe said: {}", probe.trim())); + } + let said: Vec<&str> = tail + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .rev() + .take(TAIL_LINES) + .collect(); + if said.is_empty() { + out.push_str(&format!("; it logged nothing to {}", self.log)); + } else { + let said: Vec<&str> = said.into_iter().rev().collect(); + out.push_str(&format!( + "; it said: {} (full log at {})", + said.join(" / "), + self.log + )); + } + out + } +} + +/// Start the daemon detached, keeping what it says and how it ends. +/// +/// The output used to go to `/dev/null`, which is why a remote that refused to +/// come up could only ever be reported as silence. Everything the server +/// prints on the way up — the control socket it bound, the listener it could +/// not bind, the other server already holding the single-server lock — is a +/// `startup_note!` on stderr, and all of it was being discarded. +fn launch_command(binary: &str, log: &StartupLog) -> String { let bin = shell_quote(binary); + let log_path = shell_quote(&log.log); + let exit_path = shell_quote(&log.exit); + let nonce = &log.nonce; + // The wrapper outlives the daemon by one line: it waits, then records the + // status, stamped with the nonce that asked for it. + let supervised = shell_quote(&format!( + "{bin} --daemon; s=$?; printf '%s %s' {nonce} \"$s\" > {exit_path}; exit \"$s\"" + )); + // Three things this preamble has to get right, each of them a way to break + // a machine that used to work: + // + // - The `umask` is scoped to the subshell. Left bare it would apply to the + // whole login shell, so the daemon would inherit it and so would every + // pane shell it forks — and a `git clone` in a remote pane would produce + // files nobody but the owner can read. + // - Both files are removed and re-created here, under that umask, and only + // truncated by the wrapper. `>` on an existing file keeps whatever mode + // that file already had, so truncating alone would let one left at 0644 — + // by an older build, by anything — stay that way forever. + // - A home that is full, read-only, or not ours must not stop the daemon + // from starting. If the files cannot be made, the redirect falls back to + // `/dev/null` and the launch goes ahead without diagnostics, which is + // exactly what it did before. The whole attempt sits inside a subshell so + // that a redirection failure cannot take the login shell down with it — + // on a POSIX shell a redirection error on the special builtin `:` ends a + // non-interactive shell outright, and `|| true` never gets to run. format!( - "if command -v setsid >/dev/null 2>&1; then \ - setsid {bin} --daemon < /dev/null > /dev/null 2>&1 & \ + "out={log_path}; \ + (umask 077; rm -f {log_path} {exit_path}; : > {log_path}; : > {exit_path}) 2>/dev/null \ + || out=/dev/null; \ + if command -v setsid >/dev/null 2>&1; then \ + setsid sh -c {supervised} < /dev/null >> \"$out\" 2>&1 & \ else \ - nohup {bin} --daemon < /dev/null > /dev/null 2>&1 & \ + nohup sh -c {supervised} < /dev/null >> \"$out\" 2>&1 & \ fi" ) } -fn launch_script(binary: &str, settle: Option) -> String { - let launch = launch_command(binary); +fn launch_script(binary: &str, log: &StartupLog, settle: Option) -> String { + let launch = launch_command(binary, log); match settle { Some(settle) => format!("{launch}\n{settle}"), None => launch, diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index fbe348cf..dbb59194 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -45,6 +45,14 @@ struct FakeRemote { daemon_running: Mutex, running_exe: Mutex>, launch_works: bool, + /// What a launch leaves behind on the far end: whatever the daemon printed + /// on its way up, and — once the wrapper has watched it exit — the status + /// it ended on. A daemon still running has written no status yet. + startup_log: Mutex, + startup_exit: Mutex>, + /// The nonce the launch in flight stamped its status with. + startup_nonce: Mutex, + dies_with: Option<(String, String)>, speaks: Mutex>, installed_speaks: Option, /// The stop command comes back as a failure and the daemon keeps serving — @@ -72,6 +80,10 @@ impl FakeRemote { daemon_running: Mutex::new(false), running_exe: Mutex::new(None), launch_works: true, + startup_log: Mutex::new(String::new()), + startup_exit: Mutex::new(None), + startup_nonce: Mutex::new(String::new()), + dies_with: None, speaks: Mutex::new(HashMap::new()), installed_speaks: Some(ours()), stop_fails: false, @@ -83,6 +95,30 @@ impl FakeRemote { self } + /// A daemon that starts, complains, and exits — the shape a machine whose + /// control socket cannot be bound actually takes. + fn dying_at_startup(mut self, status: &str, said: &str) -> Self { + self.launch_works = false; + self.dies_with = Some((status.to_string(), said.to_string())); + self + } + + /// A daemon that finds another server already holding the lock and exits + /// cleanly. Nobody failed; this one simply is not the server. + fn standing_down(mut self, said: &str) -> Self { + self.launch_works = false; + self.dies_with = Some(("0".to_string(), said.to_string())); + self + } + + /// A daemon that stays up and never answers. Nothing writes an exit + /// status, so the wait can only end at its deadline. + fn hanging_at_startup(mut self, said: &str) -> Self { + self.launch_works = false; + *self.startup_log.lock().unwrap() = said.to_string(); + self + } + fn speaking(self, exe: &str, spoken: RemoteProtocol) -> Self { self.speaks.lock().unwrap().insert(exe.to_string(), spoken); self @@ -193,6 +229,21 @@ impl RemoteOps for FakeRemote { *self.running_exe.lock().unwrap() = None; return ok(""); } + if let Some(path) = cmd + .strip_prefix("cat ") + .and_then(|rest| rest.strip_suffix(" 2>/dev/null")) + && path.trim_matches('\'').ends_with(".startup.exit") + { + return ok(&self + .startup_exit + .lock() + .unwrap() + .clone() + .unwrap_or_default()); + } + if cmd.starts_with("tail -c") && cmd.contains(".startup.log") { + return ok(&self.startup_log.lock().unwrap()); + } if cmd.contains("--stdio --bridge") { let running = *self.daemon_running.lock().unwrap(); return Ok(ExecOutput { @@ -207,12 +258,27 @@ impl RemoteOps for FakeRemote { } if cmd.contains("--daemon") { self.journal.lock().unwrap().push(Journal::Launch); + // The wrapper truncates the status file before starting and stamps + // what it writes with this launch's nonce, so a launch in flight is + // always distinguishable from one that ended — and from the launch + // before it. + let nonce = cmd + .split_whitespace() + .find(|word| word.len() == 32 && word.chars().all(|c| c.is_ascii_hexdigit())) + .unwrap_or_default() + .to_string(); + *self.startup_nonce.lock().unwrap() = nonce; + *self.startup_exit.lock().unwrap() = None; if self.launch_works { *self.daemon_running.lock().unwrap() = true; let mut exe = self.running_exe.lock().unwrap(); if exe.is_none() { *exe = Some(BINARY.to_string()); } + } else if let Some((status, said)) = &self.dies_with { + let nonce = self.startup_nonce.lock().unwrap().clone(); + *self.startup_log.lock().unwrap() = said.clone(); + *self.startup_exit.lock().unwrap() = Some(format!("{nonce} {status}")); } return ok(""); } @@ -842,6 +908,133 @@ fn a_daemon_that_never_answers_is_an_error() { } } +/// A daemon that exited will not start answering, so there is nothing to wait +/// for. The old loop polled the full timeout anyway, and then reported the +/// timeout — which reads as "the far end is slow" when the truth was that the +/// server had already given up, with a reason, in the first fraction of a +/// second. +#[test] +fn a_daemon_that_died_at_startup_fails_at_once_and_in_its_own_words() { + let remote = FakeRemote::new().dying_at_startup( + "1", + "tty7-server: control listener unavailable: Permission denied (os error 13)\n", + ); + remote.preinstall(BINARY, 0o755); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let err = installer(&remote, &release, &user, "me@broken-box:22") + .run() + .unwrap_err(); + + let InstallError::Launch { reason } = &err else { + panic!("{err:?}"); + }; + assert!( + reason.contains("exited with status 1"), + "the status the daemon ended on: {reason}" + ); + assert!( + reason.contains("control listener unavailable"), + "and what it said before it did: {reason}" + ); + assert!( + reason.contains(&format!("{BINARY}.startup.log")), + "and where the rest of it is: {reason}" + ); + + let probes = remote + .journal() + .into_iter() + .filter(|j| matches!(j, Journal::Exec(c) if c.contains("--stdio --bridge"))) + .count(); + assert!( + probes <= 2, + "one probe before the launch and one after it is the whole wait: {probes}" + ); +} + +/// A restart tells the old daemon to stop and the new one to start, and the old +/// one's wrapper records its status only once it is fully gone — which is after +/// its socket stopped answering, so it can land after the new launch truncated +/// the file. Reading someone else's `143` as this launch's answer would fail a +/// restart that is going perfectly well. +#[test] +fn a_status_from_the_launch_before_is_not_this_launch_s_answer() { + let remote = FakeRemote::new(); + remote.preinstall(BINARY, 0o755); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + // The corpse of a previous launch, stamped with a nonce nobody asked for. + *remote.startup_exit.lock().unwrap() = Some("0123456789abcdef0123456789abcdef 143".into()); + + installer(&remote, &release, &user, "me@box:22") + .run() + .expect("the daemon this launch started came up, whatever the old one did"); +} + +/// `run_daemon` returns success when another server already holds the +/// single-server lock, so a recorded status of 0 means "someone else is the +/// server here" — good news arriving early, not a failure. Treating any status +/// as terminal turned a transient probe miss into a hard error the old loop +/// would have recovered from inside its fifteen seconds. +#[test] +fn a_daemon_that_stood_down_cleanly_is_not_a_failed_start() { + let remote = FakeRemote::new() + .standing_down("tty7-server: another server already serves this config dir; exiting\n"); + remote.preinstall(BINARY, 0o755); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let err = installer(&remote, &release, &user, "me@box:22") + .run() + .unwrap_err(); + let InstallError::Launch { reason } = &err else { + panic!("{err:?}"); + }; + assert!( + reason.contains("still not answering"), + "it waited for the server that was supposed to be there, and said so \ + when nothing turned up: {reason}" + ); + + let probes = remote + .journal() + .into_iter() + .filter(|j| matches!(j, Journal::Exec(c) if c.contains("--stdio --bridge"))) + .count(); + assert!( + probes > 3, + "and it kept probing rather than failing on the exit status: {probes}" + ); +} + +/// The other half: a daemon that is up and simply never binds the socket. It +/// leaves no exit status, so this one does wait out the deadline — but it still +/// has to say what the probe was told and where to read the rest. +#[test] +fn a_daemon_that_never_answers_names_the_probe_and_the_log() { + let remote = FakeRemote::new().hanging_at_startup("tty7-server: still opening the tree\n"); + remote.preinstall(BINARY, 0o755); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let err = installer(&remote, &release, &user, "me@slow-box:22") + .run() + .unwrap_err(); + + let InstallError::Launch { reason } = &err else { + panic!("{err:?}"); + }; + assert!(reason.contains("still not answering"), "{reason}"); + assert!( + reason.contains("no control server"), + "the probe's own stderr, which used to be dropped for its exit code: {reason}" + ); + assert!(reason.contains("still opening the tree"), "{reason}"); +} + #[test] fn an_older_running_daemon_is_kept_and_reported() { let (remote, legacy) = FakeRemote::new().with_legacy_install("26.7.4"); @@ -982,7 +1175,8 @@ fn replacing_installs_the_matching_server_and_then_restarts_into_it() { #[test] fn the_launch_command_detaches_and_closes_every_stream() { - let cmd = launch_command("/home/me/.local/share/tty7/bin/tty7-server-26.7.5"); + let binary = "/home/me/.local/share/tty7/bin/tty7-server-26.7.5"; + let cmd = launch_command(binary, &StartupLog::for_binary(binary)); assert!(cmd.contains("setsid"), "{cmd}"); assert!( cmd.contains("nohup"), @@ -990,19 +1184,61 @@ fn the_launch_command_detaches_and_closes_every_stream() { ); assert!(cmd.contains("--daemon"), "{cmd}"); assert!(cmd.contains("< /dev/null"), "{cmd}"); - assert!(cmd.contains("> /dev/null 2>&1"), "{cmd}"); assert!( cmd.trim_end().ends_with("fi"), "both branches background it: {cmd}" ); } +fn scoped_umask(binary: &str) -> String { + format!( + "(umask 077; rm -f '{binary}.startup.log' '{binary}.startup.exit'; \ + : > '{binary}.startup.log'; : > '{binary}.startup.exit')" + ) +} + +/// The daemon's stdout and stderr used to go to `/dev/null`, and everything it +/// says on the way up is a `startup_note!` on stderr. Discarding them is what +/// left "nothing was answering after 15s" as the only thing a failed remote +/// start could ever report. +#[test] +fn the_launch_command_keeps_what_the_daemon_says_and_how_it_ends() { + let binary = "/home/me/.local/share/tty7/bin/tty7-server-26.7.5"; + let cmd = launch_command(binary, &StartupLog::for_binary(binary)); + assert!( + !cmd.contains("> /dev/null 2>&1"), + "stderr is the diagnosis, not noise: {cmd}" + ); + assert!( + cmd.contains(&format!("{binary}.startup.log")), + "output lands in a file this client can read back: {cmd}" + ); + assert!( + cmd.contains(&format!("{binary}.startup.exit")), + "and so does the exit status: {cmd}" + ); + assert!( + cmd.contains(&scoped_umask(binary)), + "both are created private, and the umask is scoped to that — bare, it \ + would reach the daemon and every pane shell it forks: {cmd}" + ); + assert!( + cmd.contains("|| out=/dev/null"), + "a home that cannot hold the files still gets its daemon started: {cmd}" + ); +} + #[test] fn a_launch_settle_follows_the_launch_and_never_replaces_it() { - let plain = launch_script(BINARY, None); - assert_eq!(plain, launch_command(BINARY), "no settle, no wrapping"); + let log = StartupLog::for_binary(BINARY); + let plain = launch_script(BINARY, &log, None); + assert_eq!( + plain, + launch_command(BINARY, &log), + "no settle, no wrapping" + ); - let settled = launch_script(BINARY, Some("sleep 1\n".to_string())); + let settled = launch_script(BINARY, &log, Some("sleep 1\n".to_string())); assert!( settled.starts_with(&plain), "the launch survives: {settled}" @@ -1035,8 +1271,13 @@ fn remote_paths_are_shell_quoted() { #[test] fn the_launch_command_quotes_its_binary() { - let cmd = launch_command("/home/me/a b/tty7-server-1.0.0"); + let binary = "/home/me/a b/tty7-server-1.0.0"; + let cmd = launch_command(binary, &StartupLog::for_binary(binary)); assert!(cmd.contains("'/home/me/a b/tty7-server-1.0.0'"), "{cmd}"); + assert!( + cmd.contains("'/home/me/a b/tty7-server-1.0.0.startup.log'"), + "and so are the paths derived from it: {cmd}" + ); } #[test] @@ -1529,6 +1770,36 @@ fn a_present_binary_reports_no_progress() { ); } +/// The upload's caption used to be the last thing an install said, so the +/// strip sat on "copying… 100%" through the whole startup wait — and when the +/// wait failed, that stale caption is what the user was left looking at. The +/// last word has to be the phase actually in progress. +#[test] +fn the_caption_moves_on_once_the_bytes_are_across() { + let remote = FakeRemote::new(); + let release = FakeRelease::new(); + let user = FakeUser::approving(); + let reports = Arc::new(Reports::default()); + + with_install_progress(reports.clone(), || { + installer(&remote, &release, &user, "me@build-box:22").run() + }) + .expect("install"); + + let phases = reports.phases(); + assert!( + phases + .iter() + .any(|p| matches!(p, InstallPhase::Uploading { .. })), + "the copy is still reported: {phases:?}" + ); + assert_eq!( + phases.last(), + Some(&InstallPhase::Restarting), + "and the wait for the far end is what the caption ends on: {phases:?}" + ); +} + #[test] fn a_scoped_progress_sink_outranks_the_global_one() { let scoped = Arc::new(Reports::default()); diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index aca6d6d8..6c45a081 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -339,7 +339,31 @@ pub fn run_daemon() -> anyhow::Result<()> { services, ) { Ok(path) => startup_note!("tty7-server: control socket at {}", path.display()), - Err(e) => startup_note!("tty7-server: control listener unavailable: {e}"), + // Someone else already answers there. That is the ordinary local + // shape: the GUI hosts the control listener and this process was + // started only to own the panes. Carry on. + // + // Ask rather than read the errno. `bind_control_socket` clears an + // ordinary leftover socket itself, but a path it cannot clear — a + // directory in the way, a file owned by someone else — comes back + // `AddrInUse` in the same words a live server does. + Err(e) if crate::host::server::control_endpoint_answers() => { + startup_note!( + "tty7-server: control listener unavailable ({e}); \ + another server is answering there, so serving panes only" + ); + } + // Nothing is answering, and this process cannot answer either. Do + // not stay alive: a running daemon holds the single-server lock, so + // every later `--daemon` stands down at once and every client probe + // of the control socket fails, forever. That pair is exactly the + // "started but nothing was answering after 15s" a remote install + // reports — with the reason, right here, thrown away. Exiting + // releases the lock and hands the reason to whoever launched us. + Err(e) => { + startup_note!("tty7-server: control listener unavailable: {e}"); + return Err(e.into()); + } } } #[cfg(not(any(unix, windows)))] diff --git a/crates/tty7-core/src/host/server.rs b/crates/tty7-core/src/host/server.rs index 5262b141..7626eff0 100644 --- a/crates/tty7-core/src/host/server.rs +++ b/crates/tty7-core/src/host/server.rs @@ -1489,6 +1489,24 @@ mod sock { p.as_os_str().as_bytes().len() <= MAX_SOCKET_PATH_BYTES } + /// Whether a control server is answering at `path` right now. + /// + /// This, and not the errno a failed bind carries, is the question that + /// matters to a daemon whose listener would not open. [`bind_control_socket`] + /// does clear the ordinary leftover — it connects first and unlinks a socket + /// nobody is behind — but what it cannot clear it hands back as `AddrInUse` + /// all the same: a directory standing where the socket goes, a file owned by + /// another user. Those look exactly like a live server in the error, and + /// nothing is listening behind either. Only a connect that completes tells + /// them apart. + pub(crate) fn control_socket_answers(path: &Path) -> bool { + UnixStream::connect(path).is_ok() + } + + pub fn control_endpoint_answers() -> bool { + control_socket_path().is_ok_and(|path| control_socket_answers(&path)) + } + pub fn bind_control_socket(path: &Path) -> io::Result { let parent = path.parent().unwrap_or(Path::new(".")); if !parent.exists() { @@ -1580,8 +1598,8 @@ mod sock { pub(crate) use sock::socket_path_in; #[cfg(unix)] pub use sock::{ - bind_control_socket, control_socket_path, serve_listener, serve_listener_with, - spawn_control_listener, spawn_control_listener_with, + bind_control_socket, control_endpoint_answers, control_socket_path, serve_listener, + serve_listener_with, spawn_control_listener, spawn_control_listener_with, }; #[cfg(windows)] @@ -1635,6 +1653,19 @@ mod wsock { spawn_control_listener_with(host, Services::none()) } + /// See the unix arm: a bind that failed only means "serve panes only" when + /// something else is actually answering there. + /// + /// Gated on the pidfile the same way [`spawn_control_listener_with`] gates + /// its own probe, and for the same reason: a TCP connect that completes + /// proves only that *something* accepted on the recorded port, and a port a + /// dead daemon wrote can have been recycled by a stranger since. When the + /// daemon that wrote it is gone, nothing behind that port is ours. + pub fn control_endpoint_answers() -> bool { + !crate::daemon::spawn::recorded_daemon_is_dead() + && transport::connect_endpoint(CONTROL_PORT_FILE).is_ok() + } + pub fn serve_listener_with( listener: TcpListener, token: transport::Token, @@ -1678,8 +1709,8 @@ mod wsock { #[cfg(windows)] pub use wsock::{ - CONTROL_PORT_FILE, connect_control, control_endpoint_path, remove_control_endpoint, - spawn_control_listener, spawn_control_listener_with, + CONTROL_PORT_FILE, connect_control, control_endpoint_answers, control_endpoint_path, + remove_control_endpoint, spawn_control_listener, spawn_control_listener_with, }; #[cfg(test)] @@ -2946,6 +2977,51 @@ mod tests { assert!(err.to_string().contains(CONTROL_SOCK_ENV), "{err}"); } + /// `run_daemon` keeps serving panes past a control listener it could not + /// open only when something else is answering there, and the errno cannot + /// make that call. `bind_control_socket` clears the leftovers it can, but a + /// path it cannot clear still comes back `AddrInUse` — indistinguishable + /// from a live server, with nothing listening behind it. A daemon that read + /// the error as "someone else is serving" stayed alive holding the + /// single-server lock, answering nothing, forever. + #[cfg(unix)] + #[test] + fn only_a_connect_tells_a_live_server_from_a_blocked_path() { + let dir = tempfile::TempDir::new().unwrap(); + + let sock = dir.path().join("s.sock"); + let listener = bind_control_socket(&sock).unwrap(); + std::thread::spawn(move || { + for stream in listener.incoming() { + drop(stream); + } + }); + assert_eq!( + bind_control_socket(&sock).unwrap_err().kind(), + io::ErrorKind::AddrInUse, + "a live server is in use" + ); + assert!( + sock::control_socket_answers(&sock), + "and it answers, which is what makes it live" + ); + + // A directory where the socket goes. Nothing is listening, the connect + // fails so there is nothing to unlink, and `bind` refuses it in the + // same words it used for the live server above. + let blocked = dir.path().join("blocked.sock"); + std::fs::create_dir(&blocked).unwrap(); + assert_eq!( + bind_control_socket(&blocked).unwrap_err().kind(), + io::ErrorKind::AddrInUse, + "same error, and no server anywhere near it" + ); + assert!( + !sock::control_socket_answers(&blocked), + "which is the difference the daemon has to act on" + ); + } + #[test] fn binding_clears_a_socket_a_crash_left_behind() { let dir = tempfile::TempDir::new().unwrap(); diff --git a/src/ui/app.rs b/src/ui/app.rs index ff7269c0..8c336ee3 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6883,34 +6883,23 @@ impl Tty7App { ), None => message, }; - let bar = gpui_component::v_flex() + let bar = crate::ui::remote_workspace::status_card(cx) .occlude() - .gap(px(6.)) - .px_3() - .py_1p5() - .rounded_lg() - .bg(theme.popover) - .border_1() - .border_color(theme.border) .shadow_md() - .text_xs() - .text_color(theme.muted_foreground) .child( - gpui_component::h_flex() - .items_center() - .gap_2() + crate::ui::remote_workspace::status_row() .child(gpui_component::Icon::new(gpui_component::IconName::Globe)) .child( - div() + crate::ui::remote_workspace::status_message(message) .font_weight(gpui::FontWeight::MEDIUM) - .text_color(theme.foreground) - .child(message), + .text_color(theme.foreground), ) .when_some(action, |this, (label, action)| { use gpui_component::Sizable as _; use gpui_component::button::ButtonVariants as _; this.child( gpui_component::button::Button::new("remote-status-action") + .flex_shrink_0() .label(label) .primary() .small() diff --git a/src/ui/home.rs b/src/ui/home.rs index 7d9c9051..d16c7fb8 100644 --- a/src/ui/home.rs +++ b/src/ui/home.rs @@ -267,7 +267,6 @@ impl Tty7App { .is_none() .then(|| self.remote_strip_action(&status, cx)) .flatten(); - let theme = cx.theme(); let message = match installing { Some(phase) => format!( "{machine} — {}", @@ -276,25 +275,15 @@ impl Tty7App { None => message, }; Some( - v_flex() - .gap(px(6.)) - .px(px(12.)) - .py(px(6.)) - .rounded(px(10.)) - .bg(theme.popover) - .border_1() - .border_color(theme.border) - .text_xs() - .text_color(theme.muted_foreground) + crate::ui::remote_workspace::status_card(cx) .child( - h_flex() - .items_center() - .gap_2() + crate::ui::remote_workspace::status_row() .child(gpui_component::Icon::new(IconName::Globe)) - .child(message) + .child(crate::ui::remote_workspace::status_message(message)) .when_some(action, |this, (label, action)| { this.child( Button::new("home-remote-status-action") + .flex_shrink_0() .label(label) .ghost() .small() @@ -314,6 +303,146 @@ impl Tty7App { } } +#[cfg(test)] +mod strip_layout_tests { + use crate::core::session::{RemoteRef, RemoteTarget, WindowView, WindowViews, WorkspaceStore}; + use crate::daemon::install::{InstallPhase, InstallProgress as _}; + use crate::ui::remote_connect::HostChoice; + use crate::ui::remote_workspace::ConnectFlow; + use gpui::{TestAppContext, VisualTestContext, px, size}; + + fn box_at(port: u16) -> RemoteTarget { + RemoteTarget::Direct { + user: "hw".into(), + host: "build-box".into(), + port, + } + } + + /// The home page on a remote workspace, sized to order, with the strip + /// showing either an install in flight or a failure to explain. + fn home_strip( + cx: &mut TestAppContext, + width: f32, + installing: Option, + failure: Option<&str>, + ) -> VisualTestContext { + let (app, mut vcx) = crate::ui::app::test_window::harness(cx); + let workspace = app.read_with(&vcx, |app, _| app.workspace); + // A port per case: the install progress table is a process-wide static + // keyed by machine, and these tests share a process. + let target = box_at(if installing.is_some() { 22 } else { 2222 }); + vcx.update(|_, cx| { + WorkspaceStore::install_for_test( + cx, + WindowViews { + views: vec![WindowView { + id: workspace, + host: Some(RemoteRef::new(target.clone(), workspace)), + open: true, + ..Default::default() + }], + active: Some(workspace), + }, + ); + }); + if let Some(phase) = installing { + crate::ui::remote_connect::GuiInstallProgress.report(&target.connection_key(), phase); + } + if let Some(error) = failure { + app.update_in(&mut vcx, |app, _, _| { + app.connect = Some(ConnectFlow::Failed { + choice: HostChoice { + target: target.clone(), + label: "build-box".into(), + detail: String::new(), + }, + error: error.to_string(), + }); + }); + } + vcx.simulate_resize(size(px(width), px(900.))); + app.update_in(&mut vcx, |_, _, cx| cx.notify()); + vcx.run_until_parked(); + vcx + } + + /// #774's first screenshot shows a copy bar running from inside a + /// quarter-width card to the window's right edge. That escape does not + /// reproduce here — `w_full` resolved against the card in every window + /// width tried, before the fix as well as after — so this is a guard on the + /// property, not the reproduction of a failure. It holds by construction + /// now that the card has a width of its own; it did not before. + #[gpui::test] + fn the_copy_bar_stays_inside_the_home_strip(cx: &mut TestAppContext) { + let full = InstallPhase::Uploading { + done: 9_227_468, + total: 9_227_468, + }; + for width in [1440.0, 900.0, 480.0] { + let mut vcx = home_strip(cx, width, Some(full), None); + let card = vcx + .debug_bounds("remote-status-card") + .expect("an install in flight draws the strip"); + let bar = vcx.debug_bounds("remote-install-bar").expect("and its bar"); + assert!( + bar.left() >= card.left() && bar.right() <= card.right(), + "at {width}px the bar {bar:?} left its card {card:?}" + ); + assert!( + card.left() >= px(0.) && card.right() <= px(width), + "and the card stays in the window: {card:?}" + ); + } + } + + /// A startup failure now carries the far end's own words, so the strip's + /// message is a paragraph rather than a phrase. Laid out in one row it + /// stretched the card until both ran off the right of the window, taking + /// the retry button with them — #774's second and third screenshots. + #[gpui::test] + fn a_long_failure_wraps_instead_of_stretching_the_card(cx: &mut TestAppContext) { + let long = "the remote tty7-server did not start: \ + /home/hw/.local/share/tty7/bin/tty7-server-c8p6 exited with status 1 before \ + it answered on the control socket; last probe said: no control server at \ + /home/hw/.config/tty7/control.sock"; + + let mut short = home_strip(cx, 1440.0, None, Some("connection refused")); + let short_card = short + .debug_bounds("remote-status-card") + .expect("strip drawn"); + let short_message = short + .debug_bounds("remote-status-message") + .expect("message drawn"); + + let mut wide = home_strip(cx, 1440.0, None, Some(long)); + let long_card = wide + .debug_bounds("remote-status-card") + .expect("strip drawn"); + let long_message = wide + .debug_bounds("remote-status-message") + .expect("message drawn"); + + assert_eq!( + short_card.size.width, long_card.size.width, + "the card's width is the card's, not the message's" + ); + assert!( + long_card.right() <= px(1440.), + "and it stays in the window: {long_card:?}" + ); + assert!( + long_message.size.height > short_message.size.height, + "a message that does not fit gets taller, not wider: \ + {long_message:?} against {short_message:?}" + ); + assert!( + long_message.right() <= long_card.right(), + "and never reaches past the card: {long_message:?} in {long_card:?}" + ); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 15502b81..7bc129c8 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1335,7 +1335,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { "No running coding agent found — start one (claude, codex, …) in a pane first." } L10nKey::SwitcherThisComputer => "This Computer", - L10nKey::SwitcherRestartingServer => "Restarting tty7's server…", + L10nKey::SwitcherStartingServer => "Starting tty7's server…", L10nKey::SwitcherDownloadingServerWithTotal => { "Downloading tty7's server… {done} / {total}" } diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index d144c560..508cb385 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1390,7 +1390,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { "実行中のコーディングエージェントが見つかりません — 先にペインでコーディングエージェントを起動してください（claude、codex など）" } L10nKey::SwitcherThisComputer => "このコンピュータ", - L10nKey::SwitcherRestartingServer => "tty7 のサーバーを再起動中…", + L10nKey::SwitcherStartingServer => "tty7 のサーバーを起動中…", L10nKey::SwitcherDownloadingServerWithTotal => { "tty7 のサーバーをダウンロード中… {done} / {total}" } diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 97a6cc2e..a77202ae 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -1078,7 +1078,7 @@ l10n_keys! { RemoteMismatchVersionFromExe, AppNoRunningCodingAgent, SwitcherThisComputer, - SwitcherRestartingServer, + SwitcherStartingServer, SwitcherDownloadingServerWithTotal, SwitcherDownloadingServerNoTotal, SwitcherCopyingServer, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index a5f2702f..e5812123 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1259,7 +1259,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { "未找到运行中的编码 agent——请先在某个窗格中启动一个（claude、codex 等）。" } L10nKey::SwitcherThisComputer => "本机", - L10nKey::SwitcherRestartingServer => "正在重启 tty7 server…", + L10nKey::SwitcherStartingServer => "正在启动 tty7 server…", L10nKey::SwitcherDownloadingServerWithTotal => "正在下载 tty7 server… {done} / {total}", L10nKey::SwitcherDownloadingServerNoTotal => "正在下载 tty7 server… {done}", L10nKey::SwitcherCopyingServer => "正在复制 tty7 server… {done} / {total}", diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 35580650..4f06d3c2 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -175,7 +175,7 @@ pub(crate) fn install_phase_caption(phase: crate::daemon::install::InstallPhase) use crate::daemon::install::InstallPhase; use crate::ui::remote_connect::human_bytes; match phase { - InstallPhase::Restarting => t(L10nKey::SwitcherRestartingServer).to_string(), + InstallPhase::Restarting => t(L10nKey::SwitcherStartingServer).to_string(), InstallPhase::Downloading { done, total } => match total { Some(total) => t_fmt( L10nKey::SwitcherDownloadingServerWithTotal, @@ -208,12 +208,17 @@ pub(crate) fn install_progress_bar( phase: crate::daemon::install::InstallPhase, cx: &gpui::App, ) -> impl gpui::IntoElement + use<> { - use gpui::{ParentElement as _, Styled as _}; + use gpui::{InteractiveElement as _, ParentElement as _, Styled as _}; use gpui_component::ActiveTheme as _; let theme = cx.theme(); gpui::div() + .debug_selector(|| "remote-install-bar".into()) .w_full() .h(gpui::px(PROGRESS_H)) + // The fill is a percentage of this track, and the track is rounded. + // Clipping to it means no fraction and no rounding can put a pixel of + // warning colour outside the groove it belongs in. + .overflow_hidden() .rounded_full() .bg(theme.border) .child( @@ -225,6 +230,69 @@ pub(crate) fn install_progress_bar( ) } +/// How wide a remote status card is when the window can spare the room. +const STATUS_CARD_W: f32 = 560.0; + +/// How much of a narrow window a status card may take. +const STATUS_CARD_MAX: f32 = 0.92; + +/// The frame both remote status strips share: home draws one under the logo, a +/// window with tabs floats one over its panes. Same machine, same sentence, and +/// — since a startup failure now carries the far end's own words — the same +/// need to survive a message that is a paragraph rather than a phrase. +/// +/// A definite width is the whole of it. Sized by its content instead, the card +/// grew with the message: at 1440px, one startup failure laid it out 1978px +/// wide, so the far half of the sentence and the retry button were off the +/// screen. It also gives the progress bar's `w_full` something bounded to be a +/// percentage of, which is what #774's first screenshot doubts — though a bar +/// escaping its card never did reproduce here, before the change or after. +pub(crate) fn status_card(cx: &gpui::App) -> gpui::Div { + use gpui::{InteractiveElement as _, Styled as _}; + use gpui_component::ActiveTheme as _; + let theme = cx.theme(); + gpui_component::v_flex() + .debug_selector(|| "remote-status-card".into()) + .w(gpui::px(STATUS_CARD_W)) + .max_w(gpui::relative(STATUS_CARD_MAX)) + .min_w_0() + .gap(gpui::px(6.)) + .px(gpui::px(12.)) + .py(gpui::px(6.)) + .rounded(gpui::px(10.)) + .bg(theme.popover) + .border_1() + .border_color(theme.border) + .text_xs() + .text_color(theme.muted_foreground) +} + +/// The row inside a status card: icon, message, and the one button. It wraps, +/// so a long message pushes the button to a second line instead of off the +/// window. +pub(crate) fn status_row() -> gpui::Div { + use gpui::Styled as _; + gpui_component::h_flex() + .w_full() + .min_w_0() + .flex_wrap() + .items_center() + .gap_2() +} + +/// The message half of that row. `flex_1` with `min_w_0` is what lets it be +/// narrower than its longest word, which is what makes wrapping possible at +/// all. +pub(crate) fn status_message(message: String) -> gpui::Div { + use gpui::{InteractiveElement as _, ParentElement as _, Styled as _}; + gpui::div() + .debug_selector(|| "remote-status-message".into()) + .flex_1() + .min_w_0() + .whitespace_normal() + .child(message) +} + /// What the remote strip's button is for, once the status has been read. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum StripAction { @@ -1832,6 +1900,14 @@ fn launch_attempt(cx: &mut gpui::App, host: HostId, target: RemoteTarget) { .collect(); RemoteLinks::mark(cx, host, |link| link.attempting = true); + // Same bookkeeping the switcher's own connect does, and for the same + // reason — see `connect_to_host`. An entry left behind here is worse than + // a stale caption: the strip and the switcher both draw an install in + // flight *instead of* the failure and its button, so a machine that needed + // Update Server sat under a frozen bar at 100% with nothing to press, + // counting attempts, for the rest of the session. The automatic path is + // the one that reaches this state, because it is the one that retries. + remote_connect::clear_install_progress(host); let for_finish = target.clone(); cx.spawn(async move |cx| { let label_for_task = label.clone(); @@ -1872,6 +1948,10 @@ fn finish_attempt( target: &RemoteTarget, outcome: Result<(remote_connect::Connected, Vec), String>, ) { + // Nothing is being installed once the attempt is back, whichever way it + // went. Retiring it here is what lets the failure — and the button that + // answers it — reach the screen at all. + remote_connect::clear_install_progress(host); let label = remote_connect::target_label(cx, target); match outcome { Ok((connected, sent)) => { @@ -2329,6 +2409,52 @@ mod tests { }); } + /// A reconnect that got as far as copying the server and then failed used + /// to leave its progress entry behind forever. That entry is not just a + /// stale caption: the strip and the switcher both draw an install in + /// flight *instead of* the failure and its button, so the machine sat + /// under a bar frozen at 100%, counting attempts, with nothing to press — + /// including the Update Server that would have fixed it. + #[gpui::test] + fn a_finished_attempt_retires_the_install_it_was_running(cx: &mut gpui::TestAppContext) { + use crate::daemon::install::InstallProgress as _; + cx.update(|cx| { + cx.set_global(crate::core::config::Config::default()); + crate::core::session::WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews::default(), + ); + let target = RemoteTarget::Alias { + alias: "build-box".into(), + }; + let host = target.host_id(); + + remote_connect::GuiInstallProgress.report( + &target.connection_key(), + crate::daemon::install::InstallPhase::Uploading { + done: 9_227_468, + total: 9_227_468, + }, + ); + assert!( + remote_connect::install_progress_for(host).is_some(), + "the copy is on screen" + ); + + finish_attempt( + cx, + host, + &target, + Err("the remote tty7-server did not start".into()), + ); + + assert!( + remote_connect::install_progress_for(host).is_none(), + "and the attempt that was running it is over, so it comes off" + ); + }); + } + #[gpui::test] fn taking_back_marks_the_workspace_for_a_whole_rebuild(cx: &mut gpui::TestAppContext) { cx.update(|cx| {