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); + }); + }), ) }