From 385d1d6536810b4e92f439e2caff0b6e0f0f61db Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 30 Jul 2026 18:50:32 +0800 Subject: [PATCH] fix(remote): offer "Restart Server" only where there is one, and keep a torn-down pane's resume record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RemoteTarget::is_ssh` answers the question the verb depends on, in one place rather than re-spelled at each call site: a WSL distribution's server is started by this client and a `LocalStdio` machine is a child process per connection, so neither has a long-lived daemon to stop and start — which is exactly why `router::restart_server` refuses them. The UI that offers the action and the router that carries it out can no longer disagree about who has one. "Restart Daemon…" now asks the window which daemon serves it. Run from a remote window it used to end every local session in every other window and leave the machine in front of the user untouched — a destructive button that did nothing its label promised. A pane torn down with its coding agent no longer publishes an observation. The kill hangs up the whole process group, so the agent dies before the PTY EOFs and a poll firing on the bytes still in flight sees nothing recognizable; publishing that cleared the record's agent and session id, and the reopened workspace came back to a bare shell instead of the conversation. The same answer about a pane nobody is tearing down still means the agent exited on its own. --- crates/tty7-core/src/core/machine.rs | 8 ++ crates/tty7-core/src/core/session.rs | 53 ++++++++++++++ crates/tty7-core/src/daemon/pane.rs | 106 ++++++++++++++++++++++++++- src/ui/app.rs | 61 +++++++++++++-- src/ui/remote_connect.rs | 15 ++++ src/ui/remote_workspace.rs | 77 +++++++++++++++---- src/ui/settings.rs | 2 +- src/ui/switcher.rs | 30 +++++++- 8 files changed, 328 insertions(+), 24 deletions(-) diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index d4f636c8..881920c6 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -1690,6 +1690,13 @@ pub(crate) fn withdraw_observations() { *OBSERVED.lock().unwrap_or_else(|e| e.into_inner()) = None; } +/// Test-only: [`OBSERVED`] is one slot for the whole process, so a test that +/// installs a store must hold this for as long as it needs its observations to +/// land there — otherwise a test elsewhere in the binary withdraws the store +/// mid-run and the observation is silently dropped. +#[cfg(test)] +pub(crate) static OBSERVE_SLOT: Mutex<()> = Mutex::new(()); + /// Copy a file we are about to stop honouring somewhere the user can find it. fn quarantine(path: &Path) { let aside = quarantine_path(path); @@ -2398,6 +2405,7 @@ mod tests { /// unconditionally. #[test] fn published_observations_land_in_the_installed_store() { + let _slot = OBSERVE_SLOT.lock().unwrap_or_else(|e| e.into_inner()); observe_pane(1, |p| p.cwd = Some("/nowhere".into())); let (store, _dir, _ws, _tab) = store_with_tab(); diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index bd3530f2..bb9e5959 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -296,6 +296,22 @@ impl RemoteTarget { } } + /// Whether this machine is reached over SSH. + /// + /// The question "Restart Server" asks. The other two variants have no + /// long-lived daemon on the far side to restart: a WSL distribution's server + /// is started by this client, and a `LocalStdio` machine is a child process + /// per connection — which is why + /// [`router::restart_server`](crate::daemon::router) refuses them. Asked + /// here rather than re-spelled at each call site, so the UI that offers the + /// verb and the router that carries it out cannot disagree about who has it. + pub fn is_ssh(&self) -> bool { + matches!( + self, + RemoteTarget::Profile { .. } | RemoteTarget::Alias { .. } | RemoteTarget::Direct { .. } + ) + } + /// The in-process id this target resolves to. /// /// This is the **only** bridge between the persisted world and the runtime @@ -713,6 +729,43 @@ mod tests { ); } + /// Which machines can be told to restart their server. The two that cannot + /// are not an omission: their server is this client's own doing, so there is + /// nothing on the far side to stop and start, and the router refuses the + /// action for exactly the same reason. A new variant has to answer this + /// question rather than inherit an answer. + #[test] + fn only_ssh_machines_have_a_server_to_restart() { + assert!( + RemoteTarget::Profile { + id: uuid::Uuid::nil() + } + .is_ssh() + ); + assert!( + RemoteTarget::Alias { + alias: "devbox".into() + } + .is_ssh() + ); + assert!(RemoteTarget::direct("me", "box.local", 22).is_ssh()); + assert!( + !RemoteTarget::Wsl { + distro: "Ubuntu".into() + } + .is_ssh(), + "a distribution's server is started by this client" + ); + assert!( + !RemoteTarget::LocalStdio { + program: "tty7-server".into(), + args: vec!["--stdio".into()], + } + .is_ssh(), + "a stdio machine is a child process per connection" + ); + } + #[test] fn direct_targets_normalize_and_reuse_the_quick_connect_parser() { // The port defaults to 22, the scheme is optional, and the host folds diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 8798335d..a5f9d112 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -1316,7 +1316,22 @@ impl DaemonPane { let alive = st.alive; let facts_after = may_change_facts.then(|| observed_facts(&st)); drop(st); - if let (Some(before), Some(after)) = (facts_before, facts_after) + // …and a third time, on teardown. From `hangup` on, + // nothing this thread still reads describes a pane + // in use — while the facts in the record are what + // the *next* open builds a successor from. The kill + // takes the whole process group down, so a poll + // landing between the coding agent's death and the + // PTY's EOF reports "nothing recognizable in the + // foreground" and would publish that as "the agent + // left", wiping the session id `--resume` needs. + // That race is why ending a workspace's sessions + // sometimes came back to a bare shell instead of + // the conversation. The last steady-state answer is + // the one worth keeping; `live` is not ours to + // write here either — `DeathReporter` owns it. + if !shutting_down.load(Ordering::SeqCst) + && let (Some(before), Some(after)) = (facts_before, facts_after) && facts_changed(&before, &after) { let (cwd, agent) = after; @@ -3838,6 +3853,95 @@ mod tests { ); } + /// Ending a workspace's sessions has to leave a record its successor can + /// resume from. The kill hangs up the whole process group, so the coding + /// agent dies before the PTY EOFs — and a poll firing on whatever bytes + /// still come out then sees nothing recognizable in the foreground. + /// Published, that answer clears the record's agent, session id and all, and + /// the reopened workspace comes back to a bare shell instead of the + /// conversation. So a teardown publishes nothing. + /// + /// The second half is the behaviour that must *not* change: the same answer + /// about a pane nobody is tearing down means the agent exited on its own. + #[test] + fn a_pane_killed_with_its_agent_keeps_the_facts_a_resume_needs() { + use crate::core::cli_agent::{AgentSessionState, CLIAgent}; + use crate::core::machine::{ + AgentFacts, MACHINE_FILE, MachineStore, OBSERVE_SLOT, PaneSeed, publish_observations, + withdraw_observations, + }; + + const PANE: u64 = 77; + let _slot = OBSERVE_SLOT.lock().unwrap_or_else(|e| e.into_inner()); + let dir = tempfile::TempDir::new().unwrap(); + let store = MachineStore::open(dir.path().join(MACHINE_FILE)); + let ws = store.workspace_create(None, None, None).unwrap(); + store + .tab_create( + ws.id, + None, + PaneSeed { + pane: PANE, + cwd: Some("/work/api".to_string()), + ssh_spec: None, + agent: Some(AgentFacts { + agent: CLIAgent::Claude, + session_id: Some("sess-1".to_string()), + launch_argv: Some(vec!["claude".to_string()]), + status: None, + }), + }, + None, + None, + ) + .unwrap(); + publish_observations(&store); + + // One read carrying a prompt mark — which is what opens the publish + // gate — while the poll answers "nothing recognizable in the + // foreground", the reading a hung-up agent produces. + let run = |shutting_down: bool| { + let mut state = test_state(true); + state.id = PANE; + state.agent = Some(CLIAgent::Claude); + state.agent_session = Some(AgentSessionState { + session_id: Some("sess-1".to_string()), + ..Default::default() + }); + DaemonPane::spawn_reader( + Arc::new(Mutex::new(state)), + Arc::new(AtomicBool::new(shutting_down)), + Arc::new(OutputGate::new()), + Box::new(std::io::Cursor::new(b"\x1b]133;D;0\x07".to_vec())), + || false, + ForegroundProbes { + remote: Box::new(|| None), + agent: Box::new(|| Some(None)), + cwd: Box::new(|| None), + }, + Arc::new(DeathReporter::new(|| {})), + ) + .join() + .unwrap(); + }; + + run(true); + let kept = store + .pane(PANE) + .expect("the record outlives the pane") + .agent + .expect("a teardown must not report the agent away"); + assert_eq!(kept.session_id.as_deref(), Some("sess-1")); + + run(false); + assert!( + store.pane(PANE).unwrap().agent.is_none(), + "an agent that left a pane still in use is a fact, and clears" + ); + + withdraw_observations(); + } + /// The full daemon-side rich-status path: sentinel OSC events sniffed out /// of the byte stream drive the pane's session state machine, identify the /// agent when argv detection hasn't, and stream every change to the diff --git a/src/ui/app.rs b/src/ui/app.rs index 21393f72..4d7b4f08 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1965,9 +1965,44 @@ impl Tty7App { .detach(); } - /// Restart the persistent background daemon: shut the running one down (which - /// stops every live shell) and bring a fresh one up, then rebuild the tabs - /// from the just-saved session so the layout returns with fresh shells. + /// The "Restart Daemon…" action: restart whichever daemon serves *this* + /// window. + /// + /// A remote window's shells live in another machine's `tty7-server`, and + /// [`restart_daemon`](Self::restart_daemon) is about this computer's. Running + /// it from a remote window ended every local session in every *other* window + /// and left the machine in front of the user untouched — a destructive button + /// that did nothing the label promised. So the action asks the window which + /// machine it is showing, and the local method keeps meaning the local daemon + /// (which is what the Settings button under "Daemon" says it does). + pub(crate) fn restart_window_daemon(&mut self, window: &mut Window, cx: &mut Context) { + let Some(remote) = WorkspaceStore::remote_ref(cx, self.workspace) else { + self.restart_daemon(window, cx); + return; + }; + let target = remote.target.clone(); + let label = crate::ui::remote_connect::label_for(&target, cx); + if !target.is_ssh() { + // Nothing to restart *over there*: this client starts the server on + // those machines itself. Said rather than silently falling back to + // restarting the local daemon, which would end sessions on a machine + // the user was not looking at. + window.push_notification( + format!( + "tty7 can only restart the server on machines it reaches over SSH. \ + {label} is served from this computer — end its sessions instead." + ), + cx, + ); + return; + } + self.confirm_restart_remote_server(target, label, window, cx); + } + + /// Restart the persistent background daemon **on this computer**: shut the + /// running one down (which stops every live shell) and bring a fresh one up, + /// then rebuild the tabs from the just-saved session so the layout returns + /// with fresh shells. /// /// A general escape hatch for the otherwise invisible, always-on daemon: /// picking up a macOS permission granted after it started (Full Disk Access @@ -4738,7 +4773,7 @@ impl Tty7App { OpenDiscord => cx.open_url(DISCORD_URL), ReportIssue => cx.open_url(ISSUES_URL), Quit => cx.quit(), - RestartDaemon => self.restart_daemon(window, cx), + RestartDaemon => self.restart_window_daemon(window, cx), ToggleSftp => self.toggle_sftp(window, cx), ShowSshForwards => self.show_ssh_forwards(window, cx), ToggleCodePanel => self.toggle_code_panel(window, cx), @@ -7077,7 +7112,7 @@ impl Render for Tty7App { this.toggle_settings(window, cx) })) .on_action(cx.listener(|this, _: &RestartDaemon, window, cx| { - this.restart_daemon(window, cx) + this.restart_window_daemon(window, cx) })) .on_action( cx.listener(|this, _: &ToggleSftp, window, cx| this.toggle_sftp(window, cx)), @@ -7239,7 +7274,21 @@ fn agent_resume_command( if !cx.global::().restore_agent_sessions { return None; } - agent.as_ref()?.resume_command(session_id?, launch_argv) + let agent = agent.as_ref()?; + // Said out loud, because it is the one step of the restore nobody can + // reconstruct afterwards: the pane comes back as a bare shell either way, + // and whether that is "no id was ever captured" (hooks not installed on + // that machine, or its record lost them) or "the agent declined to resume" + // is the whole diagnosis. A leaf that ran no agent at all is the ordinary + // case and says nothing. + let Some(session_id) = session_id else { + log::info!( + "{}'s pane had no captured session id; it comes back as a plain shell", + agent.display_name() + ); + return None; + }; + agent.resume_command(session_id, launch_argv) } /// Convert a live `Pane` tree into its serializable mirror, reading each diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index 53fa998f..5d6640cd 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -133,6 +133,21 @@ pub fn available_hosts(cx: &App) -> Vec { out } +/// The name the picker shows for `target`. +/// +/// Not `RemoteTarget`'s `Display`, which for a saved profile is its *uuid* — the +/// type deliberately cannot reach into the profile store, so anything putting a +/// machine's name in front of the user has to do this lookup. Falls back to the +/// `Display` for a machine no longer on file, which is the honest answer: that +/// is all tty7 still knows about it. +pub fn label_for(target: &RemoteTarget, cx: &App) -> String { + available_hosts(cx) + .into_iter() + .find(|host| host.target == *target) + .map(|host| host.label) + .unwrap_or_else(|| target.to_string()) +} + /// The machines matching `query`, best match first. /// /// A `~/.ssh/config` with fifty `Host` blocks is normal, and a list that long diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 03137d8f..283bde45 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -846,15 +846,73 @@ impl Tty7App { return; } let _ = this.update_in(cx, |this, window, cx| { - this.restart_remote_server(mismatch, window, cx); + this.restart_mismatched_remote_server(mismatch, window, cx); }); }) .detach(); } } - /// Carry out "Restart Server": replace the `tty7-server` on a - /// machine with this client's build. + /// [`Self::restart_remote_server`] for the machine a mismatch names: the + /// prompt knows the daemon by the record that reported it, and the record + /// has to be turned back into something addressable before anything can be + /// asked of it. + fn restart_mismatched_remote_server( + &mut self, + mismatch: crate::daemon::install::MismatchedRemoteDaemon, + window: &mut Window, + cx: &mut Context, + ) { + let label = mismatch.host.clone(); + match remote_connect::mismatch_target(&mismatch) + .ok_or_else(|| format!("tty7 no longer has a way to reach {label}")) + { + Ok(target) => self.restart_remote_server(target, label, window, cx), + Err(e) => Tty7App::report_restart_failure(&label, &e, window, cx), + } + } + + /// "Restart Server" for a machine with nothing wrong with it — the + /// switcher's machine menu, and where a remote window's "Restart Daemon…" + /// lands. + /// + /// Same outcome and same warning as the two repair paths above; the only + /// difference is that nothing is broken, so the wording claims nothing is. + /// Confirmed for the reason all three are: every session on that machine + /// ends, including the ones other windows are showing. + pub(crate) fn confirm_restart_remote_server( + &mut self, + target: RemoteTarget, + label: String, + window: &mut Window, + cx: &mut Context, + ) { + let answer = window.prompt( + PromptLevel::Warning, + &format!("Restart tty7's server on \u{201c}{label}\u{201d}?"), + Some(&format!( + "This stops every session on {label} — anything still running in them \ + will be terminated, including sessions this window is not showing. \ + Workspaces and layouts are kept and come back with fresh shells." + )), + &["Cancel", "Restart Server"], + cx, + ); + cx.spawn(async move |this, cx| { + // Index 1 is Restart Server; a dismissed prompt is Cancel. + if !matches!(answer.await, Ok(1)) { + return; + } + let _ = this.update_in(cx, |this, window, cx| { + this.restart_remote_server(target, label, window, cx); + }); + }) + .detach(); + } + + /// Carry out "Restart Server": stop the `tty7-server` on a machine and start + /// this client's build in its place. The half every entry point shares, past + /// whichever prompt asked. /// /// **This throws work away and says so.** Every pane the old server hosts /// dies with it — that is what the prompt the user just answered warns @@ -864,20 +922,11 @@ impl Tty7App { /// layout: same tabs and splits, new shells, nothing running in them. fn restart_remote_server( &mut self, - mismatch: crate::daemon::install::MismatchedRemoteDaemon, + target: RemoteTarget, + label: String, window: &mut Window, cx: &mut Context, ) { - let label = mismatch.host.clone(); - let target = match remote_connect::mismatch_target(&mismatch) - .ok_or_else(|| format!("tty7 no longer has a way to reach {label}")) - { - Ok(target) => target, - Err(e) => { - Tty7App::report_restart_failure(&label, &e, window, cx); - return; - } - }; let header = match remote_connect::control_route(&target, cx) { Ok(header) => header.restart_server(), Err(e) => { diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 5eec28c1..03805c3c 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -5528,7 +5528,7 @@ impl Tty7App { .child("Daemon"), ) .child(div().text_sm().text_color(muted_fg).child( - "Restart the daemon to pick up a newly granted macOS permission, recover if it stops responding, or start from a clean slate. This ends all running sessions; your tabs and layout reopen with fresh shells.", + "Restart the daemon on this computer to pick up a newly granted macOS permission, recover if it stops responding, or start from a clean slate. This ends all running sessions here; your tabs and layout reopen with fresh shells. A remote machine's server is restarted from its own menu in the workspace switcher.", )) .child( h_flex().child( diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 727f2e4f..24280475 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -1618,7 +1618,7 @@ fn group_menu( group: &GroupRef, app: gpui::WeakEntity, ) -> gpui_component::menu::PopupMenu { - let (a1, a2) = (app.clone(), app); + let (a1, a2, a3) = (app.clone(), app.clone(), app); let gref = group.clone(); // A remote machine can only be given a workspace once a handshake has said // where its `$HOME` is — `~` guessed from this client would be the wrong @@ -1637,12 +1637,38 @@ fn group_menu( return menu; }; let connected = group.link == Link::Connected; - menu.separator().item( + let restartable = target.is_ssh(); + let (label, for_restart) = (group.label.clone(), target.clone()); + let menu = menu.separator().item( PopupMenuItem::new("Disconnect") .disabled(!connected) .on_click(move |_, _window, cx| { let _ = a2.update(cx, |this, cx| this.switcher_disconnect(&target, cx)); }), + ); + if !restartable { + // A WSL distribution's server is started by this client and a + // `LocalStdio` one is a child process per connection, so there is no + // daemon over there to restart — the router says the same. Absent rather + // than greyed out, for the reason "Disconnect" is absent from the local + // group: a permanently disabled row only invites the question. + return menu; + } + // Deliberately not gated on `connected`. A server that has to be restarted + // is most often one this client *cannot* reach any more, and the action + // opens its own connection to do the work — requiring a live link would + // withhold the verb from exactly the machine that needs it. + // + // And deliberately *not* closing the panel, unlike the row's destructive + // items. This panel is where a restart has anything to show — the phase bar + // under the machine's header, its rows going dead and coming back — and the + // error card's identical button already leaves it open for that reason. + menu.item( + PopupMenuItem::new("Restart Server…").on_click(move |_, window, cx| { + let _ = a3.update(cx, |this, cx| { + this.confirm_restart_remote_server(for_restart.clone(), label.clone(), window, cx); + }); + }), ) }