diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index af1d8f4c..5da98cc0 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -599,6 +599,7 @@ pub struct Installer<'a> { version: String, dialect: RemoteProtocol, startup_timeout: Duration, + shutdown_timeout: Duration, poll_interval: Duration, } @@ -618,6 +619,7 @@ impl<'a> Installer<'a> { version: client_version().to_string(), dialect: RemoteProtocol::of_this_build(), startup_timeout: REMOTE_STARTUP_TIMEOUT, + shutdown_timeout: REMOTE_SHUTDOWN_TIMEOUT, poll_interval: REMOTE_POLL_INTERVAL, } } @@ -637,6 +639,7 @@ impl<'a> Installer<'a> { version: client_version().to_string(), dialect: RemoteProtocol::of_this_build(), startup_timeout: REMOTE_STARTUP_TIMEOUT, + shutdown_timeout: REMOTE_SHUTDOWN_TIMEOUT, poll_interval: REMOTE_POLL_INTERVAL, } } @@ -705,6 +708,15 @@ impl<'a> Installer<'a> { self } + /// How long [`Installer::cycle_daemon`] waits for the old server to go + /// away. Its own knob rather than a third argument to `with_timeouts`, + /// because the only caller that shortens it is the test that watches a + /// stop fail, and every other caller wants the shipped ten seconds. + pub fn with_shutdown_timeout(mut self, shutdown: Duration) -> Self { + self.shutdown_timeout = shutdown; + self + } + pub fn run(&self) -> Result { let uname = self .ops @@ -1015,15 +1027,39 @@ impl<'a> Installer<'a> { fn cycle_daemon(&self, paths: &RemotePaths) -> Result<(), InstallError> { install_progress().report(&self.host, InstallPhase::Restarting); - let _ = self.ops.run(TERMINATE_RUNNING_COMMAND); + // Keep why the stop failed, if it did. The command ends in `true`, so + // anything short of success means the far end never reached the kill at + // all — a login shell that choked on the script, a connection that went + // away. Throwing that away is half of what made the no-`/proc` bug cost + // a report instead of one glance at a log: the only thing anyone ever + // saw was the timeout below, and it blames a daemon for not stopping + // when nothing had asked it to. + let stop_failure = match self.ops.run(TERMINATE_RUNNING_COMMAND) { + Ok(out) if out.success() => None, + Ok(out) => Some(out.failure_reason()), + Err(reason) => Some(reason), + }; + if let Some(reason) = &stop_failure { + log::warn!( + "remote {}: asking the running server to stop did not succeed: {reason}", + self.host, + ); + } - let deadline = Instant::now() + REMOTE_SHUTDOWN_TIMEOUT; + let shutdown_timeout = self.shutdown_timeout; + let deadline = Instant::now() + shutdown_timeout; while self.daemon_is_serving(paths)? { if Instant::now() >= deadline { return Err(InstallError::Launch { - reason: format!( - "the running remote daemon did not stop within {REMOTE_SHUTDOWN_TIMEOUT:?}" - ), + reason: match &stop_failure { + Some(reason) => format!( + "the running remote daemon did not stop within {shutdown_timeout:?}, \ + and the command asking it to stop failed: {reason}" + ), + None => format!( + "the running remote daemon did not stop within {shutdown_timeout:?}" + ), + }, }); } std::thread::sleep(self.poll_interval); @@ -1047,11 +1083,22 @@ impl<'a> Installer<'a> { /// Finding the running server takes two shapes because `/proc` is a Linux /// thing. On Linux, `/proc//exe` is the honest answer: a symlink to the -/// file that is actually executing, whatever anyone did to `argv[0]`. macOS and -/// the BSDs have no `/proc` at all, so fall back to `ps`, whose `comm` is the -/// full path there — on Linux it would be the name truncated to 15 characters, -/// one short of `tty7-server-c7p5`, which is exactly why the fallback is a -/// fallback and not the only branch. +/// file that is actually executing, whatever anyone did to `argv[0]`. macOS — +/// the only other machine [`asset::asset_for_uname`] will install onto — has no +/// `/proc` at all, so fall back to `ps`. +/// +/// Its `comm` is not quite `exe`'s equal: on Darwin it reports `argv[0]`, so it +/// is a path only because [`launch_command`] launches by absolute path, and a +/// server started some other way would be invisible to it. It is still the +/// better of the two answers available there. Linux's `comm` is not an answer +/// at all — the name truncated to 15 characters, one short of +/// `tty7-server-c7p5` — which is why the fallback stays a fallback and `/proc` +/// keeps first refusal. +/// +/// Neither arm reaches past the connecting user: `readlink` on another user's +/// `exe` is refused, and `ps` without `-A` lists only our own processes. The +/// pattern is anchored at a `/` so it cannot match a name that merely ends in +/// one of ours. /// /// The `/proc` glob lives *inside* the `[ -d /proc ]` arm on purpose. The far /// end runs these through the user's login shell, and zsh — the default on diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 5c2c6610..fbe348cf 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -47,6 +47,10 @@ struct FakeRemote { launch_works: bool, speaks: Mutex>, installed_speaks: Option, + /// The stop command comes back as a failure and the daemon keeps serving — + /// a login shell that could not read the script, which is the shape the + /// no-`/proc` bug took on every Mac. + stop_fails: bool, } impl FakeRemote { @@ -70,9 +74,15 @@ impl FakeRemote { launch_works: true, speaks: Mutex::new(HashMap::new()), installed_speaks: Some(ours()), + stop_fails: false, } } + fn refusing_to_stop(mut self) -> Self { + self.stop_fails = true; + self + } + fn speaking(self, exe: &str, spoken: RemoteProtocol) -> Self { self.speaks.lock().unwrap().insert(exe.to_string(), spoken); self @@ -172,6 +182,13 @@ impl RemoteOps for FakeRemote { return ok(&exe); } if cmd == TERMINATE_RUNNING_COMMAND { + if self.stop_fails { + return Ok(ExecOutput { + status: Some(127), + stdout: String::new(), + stderr: "no shell over here would read that".into(), + }); + } *self.daemon_running.lock().unwrap() = false; *self.running_exe.lock().unwrap() = None; return ok(""); @@ -1078,6 +1095,101 @@ fn both_branches_of_the_probe_are_valid_shell() { } } +/// `sh -n` cannot see the failure that started all this: a glob matching +/// nothing is perfectly good syntax and only falls over when it runs, and it +/// falls over in zsh alone — which is the login shell on the machines that had +/// no `/proc` in the first place. So run the probe for real, in every shell +/// this machine has, and hold it to an exit status: the old shape answered 1 +/// under zsh, having abandoned the command line before the trailing `true`. +/// +/// Only the probe. The terminate command differs from it by one word, and that +/// word would end whatever server the developer running this happens to have +/// up; the test above pins the two to the same shape. +#[cfg(unix)] +#[test] +fn the_probe_runs_clean_in_every_shell_this_machine_has() { + let shells: Vec<&str> = ["/bin/sh", "/bin/bash", "/bin/zsh", "/bin/dash"] + .into_iter() + .filter(|sh| std::path::Path::new(sh).exists()) + .collect(); + assert!(!shells.is_empty(), "a unix without /bin/sh is not a unix"); + + for shell in shells { + let out = std::process::Command::new(shell) + .arg("-c") + .arg(RUNNING_EXE_COMMAND) + .output() + .unwrap_or_else(|e| panic!("{shell} would not run: {e}")); + assert!( + out.status.success(), + "{shell} did not survive the probe\nstderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + } +} + +/// The `ps` arm swallows its own stderr, so a flag this machine's `ps` does not +/// accept would cost nothing visible: no output, no failure, just a probe that +/// never finds a server and a restart that waits out its timeout. Ask `ps` on +/// its own instead, and only where the guard would actually route through it — +/// on Linux this arm is unreachable and Linux's `ps` need not agree. +#[cfg(unix)] +#[test] +fn the_ps_arm_is_a_ps_this_machine_accepts() { + if std::path::Path::new("/proc").is_dir() { + return; + } + let out = std::process::Command::new("ps") + .args(["-xwwo", "pid=,comm="]) + .output() + .expect("a machine with no /proc has a ps"); + assert!( + out.status.success(), + "ps rejected the fallback's arguments: {}", + String::from_utf8_lossy(&out.stderr) + ); + let listing = String::from_utf8_lossy(&out.stdout); + assert!( + listing.lines().any(|line| { + let mut parts = line.split_whitespace(); + parts.next().is_some_and(|pid| pid.parse::().is_ok()) && parts.next().is_some() + }), + "a pid and a command per line is the whole shape the loop reads: {listing}" + ); +} + +/// A stop that never reached the far end has to say so. It used to be +/// discarded outright, so the only thing anyone saw was the wait timing out — +/// which reads as "the daemon refused to die" when the truth was that the +/// command asking it to had fallen over before the `kill`. +#[test] +fn a_stop_that_failed_is_named_in_the_timeout() { + let remote = FakeRemote::new() + .with_previous_install() + .refusing_to_stop() + .serving(&format!("{BIN_DIR}/tty7-server-26.7.4")); + remote.preinstall(BINARY, 0o755); + let release = FakeRelease::new(); + let user = FakeUser::declining(); + + let failed = installer(&remote, &release, &user, "me@stubborn-box:22") + .with_shutdown_timeout(Duration::from_millis(30)) + .restart_daemon() + .expect_err("nothing stopped, so the restart cannot claim to have worked"); + + let InstallError::Launch { reason } = &failed else { + panic!("{failed:?}"); + }; + assert!( + reason.contains("did not stop") && reason.contains("no shell over here"), + "the reason has to carry why the stop failed, not just that it did: {reason}" + ); + assert!( + *remote.daemon_running.lock().unwrap(), + "and the machine is left exactly as it was, not half stopped" + ); +} + #[test] fn exec_failures_quote_stderr_when_there_is_any() { let with_stderr = ExecOutput { diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index b57c41ce..e342cb31 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -193,10 +193,17 @@ pub(crate) fn install_phase_caption(phase: crate::daemon::install::InstallPhase) } } +/// How tall the filled bar is, wherever it is drawn. +const PROGRESS_H: f32 = 3.0; + /// The thin filled bar under an install's caption. `Restarting` has no /// fraction to show — the far end is either back or it is not — so it draws /// empty, which is what the switcher has always done and is still better than /// the bar vanishing for the last leg. +/// +/// The switcher draws this one too. It used to build its own copy of exactly +/// this shape, which is one copy too many for a bar whose whole point is that +/// both places say the same thing. pub(crate) fn install_progress_bar( phase: crate::daemon::install::InstallPhase, cx: &gpui::App, @@ -206,7 +213,7 @@ pub(crate) fn install_progress_bar( let theme = cx.theme(); gpui::div() .w_full() - .h(gpui::px(3.)) + .h(gpui::px(PROGRESS_H)) .rounded_full() .bg(theme.border) .child( @@ -516,13 +523,22 @@ impl Tty7App { }); cx.notify(); - remote_connect::clear_install_progress(choice.target.host_id()); - self.watch_for_install_consent(choice.target.host_id(), cx); + let host_id = choice.target.host_id(); + remote_connect::clear_install_progress(host_id); + self.watch_for_install_consent(host_id, cx); cx.spawn(async move |this, cx| { let result = cx .background_executor() .spawn(async move { remote_connect::connect_blocking(&target, header, &label) }) .await; + // Retired here rather than in `finish_connect`, which bows out + // early whenever `connect` has moved on — a disconnect from the + // switcher, or entering another workspace, both of which can land + // mid-install. The strip reads this entry with no link state to + // temper it, so one left behind freezes a progress bar on every + // window pointed at this machine *and* takes away the Update + // Server button, which is the one thing that could have fixed it. + remote_connect::clear_install_progress(host_id); let _ = this.update_in(cx, |this, window, cx| { this.finish_connect(result, window, cx) }); @@ -570,7 +586,6 @@ impl Tty7App { let Some(choice) = self.connect.as_ref().and_then(ConnectFlow::choice).cloned() else { return; }; - remote_connect::clear_install_progress(choice.target.host_id()); match result { Ok(connected) => { let home = connected.home.clone(); diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 55af44d3..b30c3ef7 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -52,8 +52,6 @@ const ICON: f32 = 16.0; const ROW_PAD: f32 = 8.0; -const PROGRESS_H: f32 = 3.0; - /// `Failed` stays a unit variant so `Link` can be `Copy` and travel by value in /// `GroupRef`; what went wrong rides in `Group::error` instead. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -2154,8 +2152,6 @@ impl Tty7App { cx: &mut Context, ) -> impl IntoElement + use<> { let theme = cx.theme(); - let accent = theme.warning; - let fraction = phase.fraction().unwrap_or(0.0); let caption = crate::ui::remote_workspace::install_phase_caption(phase); v_flex() @@ -2170,20 +2166,7 @@ impl Tty7App { .text_color(theme.muted_foreground) .child(format!("{label} — {caption}")), ) - .child( - div() - .w_full() - .h(px(PROGRESS_H)) - .rounded_full() - .bg(theme.border) - .child( - div() - .h_full() - .w(gpui::relative(fraction)) - .rounded_full() - .bg(accent), - ), - ) + .child(crate::ui::remote_workspace::install_progress_bar(phase, cx)) } fn render_row( &self,