diff --git a/src/core/session.rs b/src/core/session.rs index 63f9196d..09661384 100644 --- a/src/core/session.rs +++ b/src/core/session.rs @@ -79,6 +79,11 @@ impl WorkspaceStore { /// when `id` is `None` / no longer on file (the "New Workspace" path). Marks it /// open and returns its id plus the tabs the window should rebuild. pub fn claim(cx: &mut gpui::App, id: Option) -> (WorkspaceId, Session) { + // Read before the store is borrowed: whether the layout may be rebuilt + // depends on another global (the connection table), and a remote + // workspace whose machine is unreachable must open empty. See + // [`claimable_session`]. + let reachable = id.is_none_or(|id| Self::machine_is_connected(cx, id)); let Some(store) = Self::try_store(cx) else { // No store (tests): hand back a detached identity so the window // still builds, but nothing is persisted. @@ -94,7 +99,7 @@ impl WorkspaceStore { }; workspace.open = true; workspace.touch(); - let claimed = (workspace.id, claimable_session(workspace)); + let claimed = (workspace.id, claimable_session(workspace, reachable)); store.workspaces.active = Some(claimed.0); store.workspaces.save(); claimed @@ -108,6 +113,11 @@ impl WorkspaceStore { session: Session, window: Option, ) { + // Same reason as in [`claim`]: read the connection table before the + // store is borrowed. A window whose machine is unreachable is not + // describing that machine's layout, so it does not get to overwrite the + // copy we have of it — see [`record_session`]. + let reachable = Self::machine_is_connected(cx, id); let Some(store) = Self::try_store(cx) else { return; }; @@ -116,7 +126,7 @@ impl WorkspaceStore { // tearing down); nothing to record. return; }; - record_session(workspace, session); + record_session(workspace, session, reachable); if let Some(window) = window { workspace.window = Some(window); } @@ -189,6 +199,20 @@ impl WorkspaceStore { Self::all(cx).get(id).and_then(|w| w.host.clone()) } + /// Whether this client can reach the machine `id`'s panes are on *right + /// now* — the predicate both halves of the layout cache turn on + /// ([`claimable_session`], [`record_session`]). + /// + /// A local workspace is always reachable: its daemon is this machine's, and + /// a gate that could answer otherwise for a local window would stop it + /// saving its own tabs. + pub fn machine_is_connected(cx: &mut gpui::App, id: WorkspaceId) -> bool { + let Some(host) = Self::remote_ref(cx, id) else { + return true; + }; + crate::ui::remote_connect::RemoteConnections::get(cx, host.host_id()).is_some() + } + /// The client-side entry for `host` — the existing one if this machine has /// seen that workspace before, a fresh one otherwise. /// @@ -268,15 +292,6 @@ impl WorkspaceStore { } } -/// Write a window's layout onto its workspace entry, honouring design §10's -/// storage split. -/// -/// **A remote workspace's entry never holds a layout on this client.** The -/// machine's own `workspaces.json` is the authority for it, and the client entry -/// is a pointer plus this machine's view state. That is not just tidiness: a -/// remote entry carrying local `SessionPane`s is exactly the shape "one window, -/// two hosts" would take on disk, and clearing it here is what makes the -/// invariant survive a restart rather than only holding while the app runs. /// The machine a window showing `id` is bound to. /// /// The whole of "one window, one machine" reduces to this being a *function*: a @@ -305,30 +320,42 @@ pub(crate) fn crosses_machines(previous: HostId, current: HostId) -> bool { /// The layout a window opening on `workspace` may rebuild — the read-side twin /// of [`record_session`]. /// -/// Both halves are needed, and it took a real launch to notice: the write guard -/// stops this client *creating* a remote entry with a local layout, but it says -/// nothing about one that arrived some other way — a hand-edited `session.json`, -/// a file written before the split existed, a sync tool. Restoring such an entry -/// would rebuild local shells inside a window bound to another machine, which is -/// design §3's "never do this" arriving through the back door. +/// A remote entry's `session` is this client's copy of a record the machine +/// owns: pulled on connect, refreshed on every `WorkspaceChanged`, pushed back +/// on every structural change. Rebuilding from it is the whole of "reconnecting +/// gets my tabs back", and it is safe to do because every pane it names is +/// routed by [`crate::ui::remote_workspace::pane_workspace_for`] — a leaf in a +/// remote workspace attaches or spawns *over there*, and a machine that cannot +/// be reached fails the spawn rather than falling back to a local shell. /// -/// So a remote workspace always opens empty here, and the entry is scrubbed on -/// the way past so the bad layout does not survive to be tried again. The real -/// layout is the remote's `workspaces.json`, pulled on connect. -fn claimable_session(workspace: &mut Workspace) -> Session { - if workspace.is_remote() { - workspace.session = Session::default(); +/// `reachable` is what keeps that guarantee from being theoretical. With the +/// link down, `List` answers nothing, so every leaf would miss its live pane and +/// try to spawn a fresh one — either failing (an empty window, having thrown the +/// layout away) or, worse, landing a second shell next to the one still running +/// over there. So an unreachable remote workspace opens empty **without +/// touching the cached layout**, and +/// [`crate::ui::remote_workspace`]'s connect path rebuilds the window the moment +/// the machine answers. +fn claimable_session(workspace: &mut Workspace, reachable: bool) -> Session { + if workspace.is_remote() && !reachable { return Session::default(); } workspace.session.clone() } -fn record_session(workspace: &mut Workspace, session: Session) { - workspace.session = if workspace.is_remote() { - Session::default() - } else { - session - }; +/// Write a window's layout onto its entry — the write-side twin of +/// [`claimable_session`]. +/// +/// A window that cannot reach its machine is not describing that machine's +/// layout (its panes failed to restore, or are sitting there disconnected), so +/// it records nothing rather than replacing the copy we have with the wreckage. +/// The remote's own `workspaces.json` is still the authority; this entry is the +/// cache the next launch opens from. +fn record_session(workspace: &mut Workspace, session: Session, reachable: bool) { + if workspace.is_remote() && !reachable { + return; + } + workspace.session = session; } #[cfg(test)] @@ -370,22 +397,25 @@ mod tests { #[test] fn a_local_workspace_stores_its_own_layout() { let mut workspace = Workspace::default(); - record_session(&mut workspace, local_layout()); + record_session(&mut workspace, local_layout(), true); assert_eq!(workspace.session.tabs.len(), 1); assert_eq!(workspace.pane_ids(), vec![7]); } - /// The one that matters: a remote entry must never end up holding panes - /// from this machine. This is the on-disk half of "a window is one machine" - /// — if a local layout could be written onto a remote entry, the next launch - /// would restore local shells into a window bound to a remote host, which is - /// design §3's "never do this". + /// The point of the whole cache: a connected remote window's layout is + /// kept, so the next launch has something to open from and + /// `remote_payload` has something to push. Without this, reconnecting to a + /// machine gives an empty window every time. #[test] - fn a_remote_workspace_never_stores_a_local_layout() { + fn a_connected_remote_workspace_stores_its_layout() { let mut workspace = Workspace::on_remote(remote_ref()); - record_session(&mut workspace, local_layout()); - assert!(workspace.session.tabs.is_empty()); - assert!(workspace.pane_ids().is_empty()); + record_session(&mut workspace, local_layout(), true); + assert_eq!(workspace.session.tabs.len(), 1); + assert_eq!( + workspace.pane_ids(), + vec![7], + "the pane ids are the remote daemon's, and are what a reconnect re-attaches" + ); } /// A local workspace opens on the layout it saved. @@ -395,42 +425,101 @@ mod tests { session: local_layout(), ..Workspace::default() }; - let claimed = claimable_session(&mut workspace); + let claimed = claimable_session(&mut workspace, true); assert_eq!(claimed.tabs.len(), 1); // And the entry is left alone. assert_eq!(workspace.session.tabs.len(), 1); } - /// The regression a real launch caught: a remote entry that arrived holding - /// a local layout — a hand-edited `session.json`, or a file written before - /// the storage split — would otherwise rebuild local shells inside a window - /// bound to another machine on the next start. - /// - /// It must open empty *and* be scrubbed, so a layout that got in somehow - /// cannot sit there being retried on every launch. + /// A connected remote workspace reopens on the layout its machine last + /// reported — the read half of "reconnecting gets my tabs back". #[test] - fn a_remote_workspace_never_reopens_a_local_layout() { + fn a_connected_remote_workspace_reopens_its_layout() { + let mut workspace = Workspace::on_remote(remote_ref()); + workspace.session = local_layout(); + let claimed = claimable_session(&mut workspace, true); + assert_eq!(claimed.tabs.len(), 1); + assert_eq!(workspace.session.tabs.len(), 1); + } + + /// With the machine unreachable, `List` answers nothing, so every leaf + /// would miss its live pane and try to spawn a fresh one beside it. The + /// window opens empty instead — and, the half that took a real launch to + /// get right, **the cached layout survives**: it is what the connect path + /// rebuilds the window from a moment later. + #[test] + fn an_unreachable_remote_workspace_opens_empty_but_keeps_its_layout() { let mut workspace = Workspace::on_remote(remote_ref()); workspace.session = local_layout(); - let claimed = claimable_session(&mut workspace); + let claimed = claimable_session(&mut workspace, false); assert!(claimed.tabs.is_empty(), "the window must open with no tabs"); - assert!( - workspace.session.tabs.is_empty(), - "and the bad layout must not survive to be tried again" + assert_eq!( + workspace.session.tabs.len(), + 1, + "and the layout must still be there for the connect to rebuild from" ); } - /// And a remote entry that somehow *arrived* holding a layout (a - /// hand-edited `session.json`, a record from a build that predates the - /// split) is cleaned out the first time the window records itself, rather - /// than being left to restore later. + /// The write-side twin: a window that could not restore its panes is not + /// describing the machine's layout, so its empty tab list must not replace + /// the copy we have of it. #[test] - fn recording_clears_a_layout_a_remote_entry_should_never_have_had() { + fn an_unreachable_remote_window_does_not_overwrite_the_cached_layout() { let mut workspace = Workspace::on_remote(remote_ref()); workspace.session = local_layout(); - record_session(&mut workspace, Session::default()); - assert!(workspace.session.tabs.is_empty()); + record_session(&mut workspace, Session::default(), false); + assert_eq!(workspace.session.tabs.len(), 1); + } + + /// The launch path end to end, with the store's own reachability lookup + /// rather than a hand-passed flag: nothing has ever connected to that + /// machine in this process, so the window opens empty and the layout it + /// will be rebuilt from is still on file afterwards. + /// + /// The config dir is pinned first because `claim` and `record` both persist + /// — without it this test would rewrite the developer's real + /// `session.json`. + #[gpui::test] + fn an_unconnected_machine_keeps_its_workspace_layout_across_a_claim( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| { + // The same path every other config-pinning test in this process + // uses: `set_config_dir` is first-call-wins, so a test that pinned + // a *different* scratch would silently redirect whichever tests + // lost the race away from the directory they then read back. + let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id())); + std::fs::create_dir_all(&dir).ok(); + crate::core::config::set_config_dir(dir); + + let mut entry = Workspace::on_remote(remote_ref()); + entry.session = local_layout(); + let id = entry.id; + WorkspaceStore::install_for_test( + cx, + Workspaces { + workspaces: vec![entry], + active: None, + }, + ); + + let (claimed, session) = WorkspaceStore::claim(cx, Some(id)); + assert_eq!(claimed, id); + assert!( + session.tabs.is_empty(), + "an unreachable machine's window opens empty" + ); + + // …and the window recording that emptiness does not erase what the + // machine still has. + WorkspaceStore::record(cx, id, Session::default(), None); + assert_eq!( + WorkspaceStore::all(cx).get(id).unwrap().session.tabs.len(), + 1, + "the cached layout must survive for the connect to rebuild from" + ); + }); } /// The remote-bound payload travels under the *remote's* id, so a record diff --git a/src/ui/app.rs b/src/ui/app.rs index eb942124..3f93eddc 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1239,6 +1239,13 @@ impl Tty7App { session, Some(WindowState::from_bounds(self.window_bounds)), ); + // …and for a remote workspace the machine that owns the layout has to + // hear about it, or `session.json` is the only place it exists and any + // other client (or a fresh install) opens the workspace empty. No-ops + // for a local workspace and for a machine we are not connected to — + // the latter is also what keeps a window that failed to restore from + // pushing its emptiness over a good record. + self.push_remote_layout(self.workspace, cx); } /// This window is going away: capture its final state (a plain `cd` may @@ -6349,6 +6356,23 @@ fn tabs_from_session( (tabs, active) } +/// Whether a restored leaf's saved `pane_id` names a pane in the same daemon +/// the caller read its `alive` set from — the window's daemon. +/// +/// Pane ids are unique only *within* a daemon, so the question is not academic: +/// looking one up in the wrong set is how a saved id silently matches somebody +/// else's live pane and the restore attaches to it. +/// +/// A native-SSH leaf is the one case where a pane does not live in its window's +/// daemon. Its russh session is spawned by **this client's** daemon however the +/// window is bound, so in a remote workspace it belongs to a different machine +/// than every other leaf around it — and its id must not be matched against the +/// remote's pane list. It reconnects from its saved spec instead, which is what +/// it does for any id that is no longer live. +fn leaf_shares_the_window_daemon(window_is_remote: bool, leaf_is_native_ssh: bool) -> bool { + !(window_is_remote && leaf_is_native_ssh) +} + /// Rebuild a live `Pane` tree from a saved `SessionPane`. A leaf whose saved /// `pane_id` is still alive in the daemon re-`attach`es (process + scrollback /// intact); otherwise it spawns a fresh shell in the saved cwd. `alive` is the @@ -6376,7 +6400,12 @@ fn session_to_pane( } => { // Only restore the pane id when the daemon confirms it's still live; // a stale id (daemon restarted, pane killed) falls back to a spawn. - let restore = (*pane_id).filter(|id| alive.contains(id)); + // + // …and `alive` is *one* daemon's pane set, so a leaf whose pane + // lives in a different one must not be looked up in it. + let same_daemon = + leaf_shares_the_window_daemon(workspace.is_some(), ssh_spec.is_some()); + let restore = (*pane_id).filter(|id| same_daemon && alive.contains(id)); // A *dead* native-SSH leaf (spec persisted, pane no longer alive) // reconnects rather than dropping back to a local shell (FR-C2/E4): // re-resolve secrets from the profile when it names one, else reuse @@ -6721,7 +6750,24 @@ fn apply_ssh_o_option( #[cfg(test)] mod tests { - use super::{parse_ssh_connect_input, parse_ssh_option_words}; + use super::{leaf_shares_the_window_daemon, parse_ssh_connect_input, parse_ssh_option_words}; + + /// A remote window's saved layout can hold a native-SSH pane, whose russh + /// session runs in *this* client's daemon rather than the machine's. Its + /// saved id must not be matched against the remote's pane list: the two + /// daemons number panes independently, so `1` over there is a different + /// pane, and restoring it would swap the user's SSH tab for whatever the + /// remote happens to be running. + #[test] + fn a_native_ssh_leaf_in_a_remote_window_is_not_looked_up_in_the_remote_daemon() { + assert!(!leaf_shares_the_window_daemon(true, true)); + // Everything else is the window's own daemon: a shell in a remote + // window is a pane over there, and in a local window both kinds are + // panes here. + assert!(leaf_shares_the_window_daemon(true, false)); + assert!(leaf_shares_the_window_daemon(false, true)); + assert!(leaf_shares_the_window_daemon(false, false)); + } #[test] fn parses_ssh_option_words_with_quotes() { diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index b29e32fe..a48220b2 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -1513,6 +1513,10 @@ fn finish_attempt( } cx.default_global::().preempted.remove(&id); relink_panes(cx, id); + // A window that came up before its machine did has no panes to + // relink — it opened empty because there was nothing to route + // to. Now there is. + hydrate_window(cx, id); } RemoteLinks::mark(cx, host, |link| { link.state = LinkState::Attached; @@ -1603,6 +1607,55 @@ fn relink_panes(cx: &mut gpui::App, workspace: WorkspaceId) { } } +/// Build the tabs of a window that opened before its machine was reachable. +/// +/// This is the other end of [`crate::core::session::WorkspaceStore::claim`]'s +/// reachability rule. A remote workspace reopened at launch has nowhere to route +/// to yet — the link is still being built — so it opens empty rather than +/// spawning a second set of shells beside the ones still running over there. +/// The layout it *would* have opened from is the entry's cached session, which +/// [`finish_attempt`] has just refreshed from the machine itself, so by the time +/// this runs the window is rebuilding from the authority. +/// +/// # What it will not do +/// +/// **Only an empty window is touched.** A window with tabs is one the user is +/// working in; rearranging it because a link came back is the same fight +/// [`refresh_remote_workspace`] refuses to pick. That also makes this safe to +/// call on every reconnect — the second one through finds tabs and leaves. +fn hydrate_window(cx: &mut gpui::App, workspace: WorkspaceId) { + let session = match WorkspaceStore::all(cx).get(workspace) { + Some(entry) if entry.is_remote() => entry.session.clone(), + // Local, or an entry that went away while the connect was in flight. + _ => return, + }; + if session.tabs.is_empty() { + // Nothing to restore: a workspace that was quit from the home page, or + // a brand-new one. Its window is right as it is. + return; + } + let Some(handle) = crate::ui::windows::WindowRegistry::window_for(cx, workspace) else { + return; + }; + let Some(app) = + crate::ui::windows::WindowRegistry::app_for(cx, workspace).and_then(|app| app.upgrade()) + else { + return; + }; + if !app.read(cx).tabs.is_empty() { + return; + } + log::info!( + "rebuilding {} tab(s) of workspace {workspace} now its machine is reachable", + session.tabs.len() + ); + let _ = handle.update(cx, move |_, window, cx| { + app.update(cx, |app, cx| { + app.adopt_workspace(workspace, session, window, cx) + }); + }); +} + /// Design §10's takeover, on this client's side: stop holding a stream to a /// workspace somebody else is now typing in. ///