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.
This commit is contained in:
l0ng-ai
2026-08-14 19:08:42 +08:00
parent 237a6b8606
commit 988c3bd7f8
4 changed files with 189 additions and 32 deletions
+57 -10
View File
@@ -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<InstallReport, InstallError> {
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/<pid>/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
@@ -47,6 +47,10 @@ struct FakeRemote {
launch_works: bool,
speaks: Mutex<HashMap<String, RemoteProtocol>>,
installed_speaks: Option<RemoteProtocol>,
/// 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::<u32>().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 {
+19 -4
View File
@@ -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();
+1 -18
View File
@@ -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<Self>,
) -> 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,