From f08d8c2764e234827c2ec2cdb5b06be55b99018b Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Fri, 14 Aug 2026 19:15:01 +0800 Subject: [PATCH] fix(remote): stop the server on machines that have no /proc, and show the install on the strip (#627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(remote): stop the server on machines that have no /proc, and show the install on the strip Restarting the remote server timed out after ten seconds on every Mac and BSD, with the old daemon still running and the new binary already sitting next to it, unlaunched. Both the probe that finds the running `tty7-server-*` and the command that terminates it walked `/proc/[0-9]*` and read each `exe` symlink. There is no `/proc` there. Two things then went wrong at once. zsh is the login shell on macOS, and it aborts the whole command line when a glob matches nothing, so even the trailing `true` never ran; and `cycle_daemon` discards the result of the terminate, so a command that killed nothing was indistinguishable from one that worked. `daemon_is_serving` then answered yes until the deadline. Guard the glob behind `[ -d /proc ]` — unreached, it is never expanded, so zsh has nothing to abort on — and fall back to `ps`, whose `comm` is the full path on the BSDs. It cannot be the only branch: Linux truncates `comm` to 15 characters, one short of `tty7-server-c7p5`, which is why `/proc` stays the first choice where it exists. `check_running_build` reads the same probe and was equally blind on those machines; it can see now. Separately, the install progress bar only ever existed inside the switcher. Pressing Update Server from a parked workspace with no switcher open froze the window for the length of the download and then produced a modal, with nothing in between. The strip draws it too now — caption and bar from the same source the switcher uses, and no button while an install is in flight, since pressing it again would start a second one on top of the first. * fix(remote): say why a stop failed, and stop a leaked install from eating the strip's button Three things the no-/proc fix left standing. `cycle_daemon` still discarded the terminate's result, which is the other half of why a Mac cost a bug report: the command ends in `true`, so anything short of success means the far end never reached the kill at all, and that is exactly what a zsh abort looks like. It is now logged, and named in the timeout error — "the running remote daemon did not stop within 10s" on its own blames a daemon for ignoring a request nobody managed to send it. The strip hides its Update Server button whenever an install is in flight, which is right, but it reads the progress registry with no link state to temper it — unlike the switcher. `finish_connect` bows out before clearing that entry whenever `connect` has moved on in the meantime, and a switcher disconnect or a move to another workspace both do that mid-install. The leftover froze a progress bar on every window pointed at the machine and took away the one button that could have fixed it. Cleared where the attempt actually ends instead, however it ended. The switcher kept its own copy of the progress bar after the caption was shared; it draws the shared one now. Tests: the probe runs for real in every shell on the machine rather than only parsing under `sh -n` — the glob that started this was valid syntax and only fell over when zsh ran it, which no `-n` can see. `ps` is checked on its own where the fallback would actually be taken, since that arm eats its own stderr and a rejected flag would otherwise cost nothing visible. And a stop that fails is asserted to reach the error. `with_shutdown_timeout` exists so that last test does not sit out ten seconds. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> --- crates/tty7-core/src/daemon/install/mod.rs | 75 ++++++++- crates/tty7-core/src/daemon/install/tests.rs | 161 +++++++++++++++++++ src/ui/app.rs | 63 +++++--- src/ui/home.rs | 57 +++++-- src/ui/remote_workspace.rs | 87 +++++++++- src/ui/switcher.rs | 39 +---- 6 files changed, 398 insertions(+), 84 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index 82b4eaa1..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); @@ -1045,9 +1081,34 @@ impl<'a> Installer<'a> { } } -const RUNNING_EXE_COMMAND: &str = r#"for p in /proc/[0-9]*; do e=$(readlink "$p/exe" 2>/dev/null) || continue; case "$e" in */tty7-server-*) printf '%s' "${e% (deleted)}"; break;; esac; done; true"#; +/// 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 — +/// 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 +/// macOS — aborts the whole command line when a glob matches nothing, so with +/// the loop at top level the trailing `true` never ran and every one of these +/// was a silent no-op on every Mac. That is what left `restart_daemon` waiting +/// out its ten seconds for a daemon nobody had asked to stop. +const RUNNING_EXE_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-*) printf '%s' "${e% (deleted)}"; break;; esac; done; else ps -xwwo pid=,comm= 2>/dev/null | while read -r pid e; do case "$e" in */tty7-server-*) printf '%s' "$e"; break;; esac; done; fi; true"#; -const TERMINATE_RUNNING_COMMAND: &str = r#"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; true"#; +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 { let bin = shell_quote(binary); diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 1f189a36..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(""); @@ -1029,6 +1046,150 @@ fn the_running_exe_probe_cannot_fail_the_command() { assert!(TERMINATE_RUNNING_COMMAND.contains("*/tty7-server-*")); } +/// Both commands have to work on a machine with no `/proc`, which is every Mac +/// and every BSD, and the `/proc` glob has to stay inside the guard: zsh is the +/// login shell over there, and a top-level glob that matches nothing takes the +/// rest of the command line with it — including the trailing `true`. +#[test] +fn finding_the_running_server_survives_a_machine_without_proc() { + for cmd in [RUNNING_EXE_COMMAND, TERMINATE_RUNNING_COMMAND] { + assert!( + cmd.starts_with("if [ -d /proc ]; then"), + "the glob has to be unreachable before the guard passes: {cmd}" + ); + let (guarded, fallback) = cmd + .split_once("; else ") + .expect("a machine with no /proc still needs an answer"); + assert!( + guarded.contains("/proc/[0-9]*") && !fallback.contains("/proc/"), + "the fallback reads ps, not a filesystem that is not there: {cmd}" + ); + assert!( + fallback.contains("ps -xwwo pid=,comm="), + "unwrapped, unabbreviated, and this user's processes only: {cmd}" + ); + } +} + +/// Whichever branch the far end takes, the command has to parse — a syntax +/// error here is invisible in production, where the output is read as "no +/// server is running" and the failure is a ten-second timeout. +/// +/// Parsed, not run: the terminate command would kill this developer's own +/// server, and this test is not the place to find that out. +#[cfg(unix)] +#[test] +fn both_branches_of_the_probe_are_valid_shell() { + for cmd in [RUNNING_EXE_COMMAND, TERMINATE_RUNNING_COMMAND] { + let out = std::process::Command::new("/bin/sh") + .arg("-n") + .arg("-c") + .arg(cmd) + .output() + .expect("every unix has /bin/sh"); + assert!( + out.status.success(), + "{cmd}\nstderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + } +} + +/// `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/app.rs b/src/ui/app.rs index 8e38723a..ddc47a06 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6165,12 +6165,25 @@ impl Tty7App { let status = self.remote_status(cx)?; let machine = self.remote_machine_label(cx); let message = status.strip_message(&machine)?; - let action = self.remote_strip_action(&status, cx); + // While an install runs, the strip is that install's progress: the + // refusal it is answering is no longer the news, and the button that + // started it would only start a second one. + let installing = self.remote_strip_progress(cx); + let action = installing + .is_none() + .then(|| self.remote_strip_action(&status, cx)) + .flatten(); let theme = cx.theme(); - let bar = gpui_component::h_flex() + let message = match installing { + Some(phase) => format!( + "{machine} — {}", + crate::ui::remote_workspace::install_phase_caption(phase) + ), + None => message, + }; + let bar = gpui_component::v_flex() .occlude() - .items_center() - .gap_2() + .gap(px(6.)) .px_3() .py_1p5() .rounded_lg() @@ -6180,25 +6193,33 @@ impl Tty7App { .shadow_md() .text_xs() .text_color(theme.muted_foreground) - .child(gpui_component::Icon::new(gpui_component::IconName::Globe)) .child( - div() - .font_weight(gpui::FontWeight::MEDIUM) - .text_color(theme.foreground) - .child(message), + gpui_component::h_flex() + .items_center() + .gap_2() + .child(gpui_component::Icon::new(gpui_component::IconName::Globe)) + .child( + div() + .font_weight(gpui::FontWeight::MEDIUM) + .text_color(theme.foreground) + .child(message), + ) + .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") + .label(label) + .primary() + .small() + .on_click(cx.listener(move |this, _, window, cx| { + this.run_strip_action(action.clone(), window, cx); + })), + ) + }), ) - .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") - .label(label) - .primary() - .small() - .on_click(cx.listener(move |this, _, window, cx| { - this.run_strip_action(action.clone(), window, cx); - })), - ) + .when_some(installing, |this, phase| { + this.child(crate::ui::remote_workspace::install_progress_bar(phase, cx)) }); Some( div() diff --git a/src/ui/home.rs b/src/ui/home.rs index 81622386..f7df385b 100644 --- a/src/ui/home.rs +++ b/src/ui/home.rs @@ -259,12 +259,25 @@ impl Tty7App { let machine = self.remote_machine_label(cx); let status = self.remote_status(cx)?; let message = status.strip_message(&machine)?; - let action = self.remote_strip_action(&status, cx); + // An install in flight replaces both halves of the strip: its own line + // instead of the complaint that is being answered, and no button, since + // pressing Update Server again would start a second one on top of it. + let installing = self.remote_strip_progress(cx); + let action = installing + .is_none() + .then(|| self.remote_strip_action(&status, cx)) + .flatten(); let theme = cx.theme(); + let message = match installing { + Some(phase) => format!( + "{machine} — {}", + crate::ui::remote_workspace::install_phase_caption(phase) + ), + None => message, + }; Some( - h_flex() - .items_center() - .gap_2() + v_flex() + .gap(px(6.)) .px(px(12.)) .py(px(6.)) .rounded(px(10.)) @@ -273,19 +286,29 @@ impl Tty7App { .border_color(theme.border) .text_xs() .text_color(theme.muted_foreground) - .child(gpui_component::Icon::new(IconName::Globe)) - .child(message) - .when_some(action, |this, (label, action)| { - this.child( - Button::new("home-remote-status-action") - .label(label) - .ghost() - .small() - .on_click(cx.listener(move |this, _, window, cx| { - this.run_strip_action(action.clone(), window, cx); - })) - .on_mouse_down(MouseButton::Left, |_, _, cx| cx.stop_propagation()), - ) + .child( + h_flex() + .items_center() + .gap_2() + .child(gpui_component::Icon::new(IconName::Globe)) + .child(message) + .when_some(action, |this, (label, action)| { + this.child( + Button::new("home-remote-status-action") + .label(label) + .ghost() + .small() + .on_click(cx.listener(move |this, _, window, cx| { + this.run_strip_action(action.clone(), window, cx); + })) + .on_mouse_down(MouseButton::Left, |_, _, cx| { + cx.stop_propagation() + }), + ) + }), + ) + .when_some(installing, |this, phase| { + this.child(crate::ui::remote_workspace::install_progress_bar(phase, cx)) }), ) } diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 387400da..798f741f 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -168,6 +168,63 @@ pub(crate) fn mismatch_action_key(refusal: &str) -> L10nKey { } } +/// One line for an install in flight — bytes moved, or the wait for the far end +/// to come back up. Shared by the switcher's progress bar and the strip's, so a +/// user watching both is not told two different things. +pub(crate) fn install_phase_caption(phase: crate::daemon::install::InstallPhase) -> String { + use crate::daemon::install::InstallPhase; + use crate::ui::remote_connect::human_bytes; + match phase { + InstallPhase::Restarting => t(L10nKey::SwitcherRestartingServer).to_string(), + InstallPhase::Downloading { done, total } => match total { + Some(total) => t_fmt( + L10nKey::SwitcherDownloadingServerWithTotal, + &[("done", &human_bytes(done)), ("total", &human_bytes(total))], + ), + None => t_fmt( + L10nKey::SwitcherDownloadingServerNoTotal, + &[("done", &human_bytes(done))], + ), + }, + InstallPhase::Uploading { done, total } => t_fmt( + L10nKey::SwitcherCopyingServer, + &[("done", &human_bytes(done)), ("total", &human_bytes(total))], + ), + } +} + +/// 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, +) -> impl gpui::IntoElement + use<> { + use gpui::{ParentElement as _, Styled as _}; + use gpui_component::ActiveTheme as _; + let theme = cx.theme(); + gpui::div() + .w_full() + .h(gpui::px(PROGRESS_H)) + .rounded_full() + .bg(theme.border) + .child( + gpui::div() + .h_full() + .w(gpui::relative(phase.fraction().unwrap_or(0.0))) + .rounded_full() + .bg(theme.warning), + ) +} + /// What the remote strip's button is for, once the status has been read. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum StripAction { @@ -401,6 +458,22 @@ impl Tty7App { }) } + /// What the install running against *this* window's machine has reached, if + /// one is running at all. + /// + /// The switcher has drawn this since installs got a progress bar, but the + /// strip never did — so pressing Update Server on a parked workspace with + /// no switcher open froze the screen for as long as the download took and + /// then produced a modal, with nothing in between. Same source, so + /// the two cannot disagree about how far along it is. + pub(crate) fn remote_strip_progress( + &self, + cx: &gpui::App, + ) -> Option { + let own = WorkspaceStore::remote_ref(cx, self.workspace)?; + remote_connect::install_progress_for(own.target.host_id()) + } + pub(crate) fn run_strip_action( &mut self, action: StripAction, @@ -450,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) }); @@ -504,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 057e53c5..b30c3ef7 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -18,7 +18,7 @@ use crate::daemon::install::InstallPhase; use crate::terminal::pane_liveness::Liveness; use crate::ui::app::Tty7App; use crate::ui::i18n::{L10nKey, t, t_fmt}; -use crate::ui::remote_connect::{self, HostChoice, RemoteWorkspaceRow, human_bytes}; +use crate::ui::remote_connect::{self, HostChoice, RemoteWorkspaceRow}; use crate::ui::remote_workspace::{ConnectFlow, MachineStatus, RemoteLinks}; const CARD_W: f32 = 840.0; @@ -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,25 +2152,7 @@ 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 = match phase { - InstallPhase::Restarting => t(L10nKey::SwitcherRestartingServer).to_string(), - InstallPhase::Downloading { done, total } => match total { - Some(total) => t_fmt( - L10nKey::SwitcherDownloadingServerWithTotal, - &[("done", &human_bytes(done)), ("total", &human_bytes(total))], - ), - None => t_fmt( - L10nKey::SwitcherDownloadingServerNoTotal, - &[("done", &human_bytes(done))], - ), - }, - InstallPhase::Uploading { done, total } => t_fmt( - L10nKey::SwitcherCopyingServer, - &[("done", &human_bytes(done)), ("total", &human_bytes(total))], - ), - }; + let caption = crate::ui::remote_workspace::install_phase_caption(phase); v_flex() .gap(px(6.)) @@ -2186,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,