diff --git a/CHANGELOG.md b/CHANGELOG.md index c4b199c1..6d703919 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A local daemon that dies and comes back no longer leaves a window of dead + panes looking live** — from the client's side a killed daemon is + indistinguishable from one whose shells all exited at once, so the window + kept showing every pane with its last title, and the reconnect then pushed + that dead layout back up as the new daemon's truth. The control handshake's + instance id is now compared on every local reconnect — the same check the + remote path already made — and a changed instance rebuilds each local + window from the machine tree instead: tabs come back, and each pane is + restored from its scrollback snapshot with the "this is a new shell" banner + rather than left frozen mid-lie. The restart-server action uses the same + rebuild path (#553). - **A file link in a remote pane no longer opens this machine's copy.** An absolute path was checked against the local filesystem whatever the pane was connected to, so `/etc/nginx/nginx.conf` on a server opened the one on your diff --git a/src/ui/app.rs b/src/ui/app.rs index 538ce3d5..4d7574a7 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1519,11 +1519,13 @@ impl Tty7App { let _ = this.update_in(cx, |this, window, cx| { match &restarted { Ok(()) => { - // The link we held pointed at the server we just killed. Drop it - // before asking for the tree, or the pull goes out on a dead - // socket and the window is left empty on the home page. - crate::ui::local_link::LocalLink::invalidate(cx); - crate::ui::tree_sync::resync_window_from_tree(cx, this.workspace); + // The link we held pointed at the server we just killed; + // the reconnect finds a new process whose registry knows + // nothing about these panes. The helper drops the dead + // link first — a pull sent down it dies on a dead socket + // before the reader notices — and rebuilds every local + // window from the tree. + crate::ui::tree_sync::resync_after_local_daemon_change(cx); } Err(e) => { // The user asked for this and lands on an empty home diff --git a/src/ui/local_link.rs b/src/ui/local_link.rs index 1e8f5bda..a483218a 100644 --- a/src/ui/local_link.rs +++ b/src/ui/local_link.rs @@ -8,6 +8,12 @@ use crate::ui::remote_workspace::Backoff; #[derive(Default)] pub struct LocalLink { client: Option>, + /// The daemon process the current link is talking to, by its hello + /// instance — empty until the first connect records one. Survives + /// `invalidate` on purpose: forgetting it there would make every + /// reconnect a first sighting, and a daemon that came back as a + /// *different* process would never be noticed (#553). + instance: String, backoff: Backoff, next_attempt: Option, attempting: bool, @@ -49,6 +55,11 @@ impl LocalLink { /// for a moment after we kill the daemon ourselves the dead link still /// hands itself out and every call on it fails. Callers that know the far /// end is gone say so here, and the next tick reconnects. + /// + /// The remembered hello `instance` deliberately survives: the reconnect + /// compares against it to tell "same daemon, link hiccuped" from "new + /// daemon process" (#553), and forgetting it here — the restart path's + /// own first move — would blind exactly that comparison. pub fn invalidate(cx: &mut App) { let link = cx.default_global::(); if link.client.take().is_some() { @@ -95,6 +106,26 @@ impl LocalLink { match connected { Ok(client) => { log::info!("control link to the local daemon is up"); + // A daemon that died and came back is a *new process* + // whose registry knows nothing about the panes this + // window is showing — and from the client's side a + // killed daemon is indistinguishable from one whose + // shells all exited at once (its DeathReporter says + // nothing while it shuts down, and a kill says nothing + // ever), so the panes on screen are probably lying + // about being alive. The instance id in the hello is + // the only way to tell "same daemon, link hiccuped" + // from "new daemon": compare before syncing, or + // `on_link_up` would push the window of dead panes up + // as the new daemon's truth (#553). + let restarted = { + let link = cx.default_global::(); + crate::ui::tree_sync::note_instance( + &mut link.instance, + &client.hello().instance, + ) + }; + let link = cx.default_global::(); link.client = Some(client); link.backoff.reset(); link.next_attempt = None; @@ -102,7 +133,19 @@ impl LocalLink { cx, tty7_core::host::HostId::LOCAL, ); - crate::ui::tree_sync::on_link_up(cx, tty7_core::host::HostId::LOCAL); + if restarted { + // The link installed just above is this new + // daemon's own and answers right now, so the pull + // goes out on it. Dropping it first — which is what + // a caller that killed the daemon itself has to do — + // would leave every window waiting out another + // connect, and a pull that runs out its fifteen + // seconds waiting owes a `Replace` a window with + // tabs on screen never claims back. + crate::ui::tree_sync::resync_local_windows_from_tree(cx); + } else { + crate::ui::tree_sync::on_link_up(cx, tty7_core::host::HostId::LOCAL); + } } Err(e) => match dialect_refusal(&e) { // Retrying will not talk this server round, and the diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 7a3626d7..6a366435 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -1580,27 +1580,7 @@ fn relink_panes(cx: &mut gpui::App, workspace: WorkspaceId) { fn server_restarted(cx: &mut gpui::App, host: HostId, peer: &RemoteHost) -> bool { let instance = peer.peer().instance.clone(); let seen = &mut cx.default_global::().instances; - note_instance(seen, host, &instance) -} - -fn note_instance( - seen: &mut std::collections::HashMap, - host: HostId, - instance: &str, -) -> bool { - if instance.is_empty() { - return false; - } - match seen.insert(host, instance.to_string()) { - Some(before) if before != instance => { - log::info!( - "the tty7-server on this machine is a new process ({before} → {instance}); \ - its panes are gone" - ); - true - } - _ => false, - } + crate::ui::tree_sync::note_instance(seen.entry(host).or_default(), &instance) } fn refresh_window_shells(cx: &mut gpui::App, workspace: WorkspaceId) { @@ -1893,41 +1873,30 @@ mod tests { assert_eq!(flow.choice(), Some(&choice)); } - #[test] - fn only_a_changed_instance_counts_as_a_restart() { - let mut seen = std::collections::HashMap::new(); - let host = HostId::from_connection_key("ssh:build-box"); - - assert!(!note_instance(&mut seen, host, "abc")); - assert!(!note_instance(&mut seen, host, "abc")); - assert!(note_instance(&mut seen, host, "def")); - assert!(!note_instance(&mut seen, host, "def")); - } - - #[test] - fn an_unknown_instance_is_not_a_restart_and_is_not_remembered() { - let mut seen = std::collections::HashMap::new(); - let host = HostId::from_connection_key("ssh:build-box"); - - assert!(!note_instance(&mut seen, host, "")); - assert!(seen.is_empty(), "an unknown instance must not be recorded"); - assert!( - !note_instance(&mut seen, host, "abc"), - "the first real instance is a first sighting, not a restart" - ); - } - + /// What the comparison itself answers is pinned where it now lives, in + /// `tree_sync`. What is this module's own is the slot it reads: one per + /// host, keyed the way `server_restarted` keys it, so a machine that + /// restarted says nothing about the one next to it. #[test] fn instances_are_per_machine() { let mut seen = std::collections::HashMap::new(); let a = HostId::from_connection_key("ssh:box-a"); let b = HostId::from_connection_key("ssh:box-b"); - assert!(!note_instance(&mut seen, a, "a1")); - assert!(!note_instance(&mut seen, b, "b1")); - assert!(note_instance(&mut seen, a, "a2")); + assert!(!crate::ui::tree_sync::note_instance( + seen.entry(a).or_default(), + "a1" + )); + assert!(!crate::ui::tree_sync::note_instance( + seen.entry(b).or_default(), + "b1" + )); + assert!(crate::ui::tree_sync::note_instance( + seen.entry(a).or_default(), + "a2" + )); assert!( - !note_instance(&mut seen, b, "b1"), + !crate::ui::tree_sync::note_instance(seen.entry(b).or_default(), "b1"), "box-b never changed; box-a restarting is not its business" ); } diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index 425421cf..ac0082e2 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -1906,6 +1906,68 @@ pub(crate) fn resync_window_from_tree(cx: &mut App, client_ws: WorkspaceId) { hydrate(cx, client_ws, Adopt::Replace); } +/// Records the daemon process a link is talking to, answering "did the server +/// behind this link just become a *different* process?". +/// +/// `seen` is the instance last recorded for this link, empty when unknown. A +/// first sighting is not a restart (there is nothing on screen to be wrong +/// about yet), and an empty instance means the server predates the field — +/// it neither reports nor overwrites what was seen before. Shared by the +/// remote path (`remote_workspace::server_restarted`, keyed per host) and the +/// local link (single daemon), which needs the same comparison to notice the +/// daemon it lost came back as another process (#553). +pub(crate) fn note_instance(seen: &mut String, instance: &str) -> bool { + if instance.is_empty() { + return false; + } + let before = std::mem::replace(seen, instance.to_string()); + if !before.is_empty() && before != instance { + log::info!( + "the tty7-server on this machine is a new process ({before} → {instance}); \ + its panes are gone" + ); + return true; + } + false +} + +/// Drops the link to the local daemon and rebuilds every local window from the +/// machine tree, for a caller that killed that daemon itself (#553). +/// +/// The invalidate half comes first on purpose: the link still holds the client +/// that pointed at the server that is now gone, and a pull sent down it dies on +/// a dead socket before the reader notices. With it dropped, `hydrate` waits for +/// the reconnect `LocalLink::tick` is already driving and pulls the layout the +/// daemon actually has. +/// +/// Only for the caller that has no live link left. One that just handshaked a +/// *new* daemon calls [`resync_local_windows_from_tree`] with that link in hand: +/// dropping it there would throw away a working link and make every window wait +/// out another connect for no reason. +pub(crate) fn resync_after_local_daemon_change(cx: &mut App) { + crate::ui::local_link::LocalLink::invalidate(cx); + resync_local_windows_from_tree(cx); +} + +/// Rebuilds every local window from the machine tree, because the daemon behind +/// the local link became a different process — it died and the reconnect found a +/// new one, whose registry knows nothing about the panes on screen (#553). +/// +/// Remote windows are left alone: their own link says when their machine's +/// server changed, and this one speaks for this computer only. +/// +/// When no daemon answers the pull, the windows owe a rehydration instead +/// (`owe_rehydration`) — which is also the guard that keeps a window emptied by +/// a failed pull from being pushed back up as "close every tab". +pub(crate) fn resync_local_windows_from_tree(cx: &mut App) { + for (workspace, _) in crate::ui::windows::WindowRegistry::open_windows(cx) { + if WorkspaceStore::host_of(cx, workspace) != HostId::LOCAL { + continue; + } + resync_window_from_tree(cx, workspace); + } +} + impl Tty7App { pub(crate) fn apply_layout_delta( &mut self, @@ -2136,6 +2198,40 @@ fn set_gui_ratio(pane: &mut Pane, path: &[Side], ratio: f32) -> bool { mod tests { use super::*; + #[test] + fn note_instance_reports_only_a_real_change() { + let mut seen = String::new(); + + assert!( + !note_instance(&mut seen, "abc"), + "a first sighting is not a restart" + ); + assert!( + !note_instance(&mut seen, "abc"), + "the same process is not a restart" + ); + assert!(note_instance(&mut seen, "def"), "a new process is"); + assert!(!note_instance(&mut seen, "def")); + } + + #[test] + fn note_instance_ignores_a_server_that_predates_the_field() { + let mut seen = String::from("abc"); + + assert!( + !note_instance(&mut seen, ""), + "an unknown instance is never a restart" + ); + assert_eq!(seen, "abc", "and it must not overwrite what was seen"); + + let mut fresh = String::new(); + assert!(!note_instance(&mut fresh, "")); + assert!( + !note_instance(&mut fresh, "abc"), + "the first real instance after an unknown one is a first sighting" + ); + } + #[cfg(unix)] #[test] fn a_peer_without_the_machine_tree_bit_classifies_as_unserved() {