From 5e6bc1246fb8a1bf8ca4b705ba2b2062c8730ca4 Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Sat, 8 Aug 2026 16:22:19 +0800 Subject: [PATCH] fix(workspace): stop switching workspaces from destroying live sessions (#410) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workspace): stop switching workspaces from destroying live sessions Switching workspaces rebuilt every pane it was asked to restore, and a window that rebuilt nothing then deleted the workspace outright — tree and store both. Three separate guesses, each one authorizing an irreversible act: - `session_from_tree` erased a pane's id when the tree said `live: false`. That flag is a cached observation from another process, reloaded as false on every server start, so a quiet pane read as dead while its shell was running. The restore had nothing to attach to and spawned a fresh shell over it. - Two servers could start against one config dir. `run_with` decided another server was dead by failing to connect once, then unlinked its socket and bound its own. The loser kept `control.sock` with an empty pane registry, so `MachineGet` reported every pane dead and nothing logged an error. - `finish_hydration` marked a window informed before the rebuild and without looking at the result. `tabs_from_session` drops any tab whose panes all fail to start, which is every tab when the pane socket is unreachable — leaving a window that was empty and authoritative at once, and the next switch deleted a workspace with ten live tabs. Each is now settled by whoever holds the truth: attaching decides whether a pane is there, an advisory lock decides which process is the server, and a deletion needs the machine's own mirror to agree that the workspace is empty. * fix(state): quarantine a corrupt views.json instead of silently discarding it machine.json already sets a corrupt file aside before falling back to defaults; views.json just logged and returned None, and the next save overwrote whatever the file held. Move the quarantine helpers to config so both loaders share them. Also make the no-lock-primitive fallback in the daemon singleton report Unavailable rather than Taken, so a platform without flock still gets a server instead of one that refuses to start. * fix(restore): a failed List no longer reads as every pane being dead Review follow-ups on #410, all three the same shape the PR exists to stamp out: - alive_panes_on flattened a failed List RPC into an empty alive-set, which made pane_attachable respawn every pane in the batch over its running session. The failure now surfaces as None and the attach itself decides, the way session_from_tree already leaves it to. - pub fn run() bypassed the singleton lock entirely; it had no callers, so a future one would have silently reintroduced the split-brain. Removed. - Singleton::path() and the field behind it were unused. Removed. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> --- crates/tty7-core/src/core/config.rs | 33 ++++ crates/tty7-core/src/core/machine.rs | 33 +--- crates/tty7-core/src/core/session.rs | 33 +++- crates/tty7-core/src/daemon/mod.rs | 1 + crates/tty7-core/src/daemon/server.rs | 25 ++- crates/tty7-core/src/daemon/singleton.rs | 174 +++++++++++++++++++++ src/ui/app.rs | 74 ++++++--- src/ui/tree_sync.rs | 190 +++++++++++++++++++++-- 8 files changed, 496 insertions(+), 67 deletions(-) create mode 100644 crates/tty7-core/src/daemon/singleton.rs diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index 8c2f2156..37d7cb17 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -622,6 +622,39 @@ pub fn strip_bom(text: &str) -> &str { text.strip_prefix('\u{FEFF}').unwrap_or(text) } +/// Sets a corrupt state file aside (copied, the original left in place) so the +/// caller can fall back to defaults without silently destroying what was there. +pub(crate) fn quarantine(path: &std::path::Path) { + let aside = quarantine_path(path); + match std::fs::copy(path, &aside) { + Ok(_) => log::warn!("the previous contents were kept at {}", aside.display()), + Err(e) => log::warn!("could not keep a copy at {}: {e}", aside.display()), + } +} + +/// Like [`quarantine`], but moves the file out of the way — for files that +/// cannot even be read, where copying would fail too. +pub(crate) fn quarantine_by_rename(path: &std::path::Path) { + let aside = quarantine_path(path); + match std::fs::rename(path, &aside) { + Ok(()) => log::warn!("the previous contents were moved to {}", aside.display()), + Err(e) => log::warn!("could not move the file to {}: {e}", aside.display()), + } +} + +fn quarantine_path(path: &std::path::Path) -> PathBuf { + const MAX_QUARANTINED: u32 = 8; + + let base = path.with_extension("json.corrupt"); + if !base.exists() { + return base; + } + (1..MAX_QUARANTINED) + .map(|n| path.with_extension(format!("json.corrupt.{n}"))) + .find(|candidate| !candidate.exists()) + .unwrap_or(base) +} + pub fn write_atomic(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> { write_atomic_mode(path, bytes, false) } diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index 721aabab..ae00fae6 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -1158,7 +1158,7 @@ fn load_machine(path: &Path) -> Machine { Err(e) if e.kind() == io::ErrorKind::NotFound => return Machine::default(), Err(e) => { log::warn!("could not read {}; quarantining it: {e}", path.display()); - quarantine_by_rename(path); + crate::core::config::quarantine_by_rename(path); return Machine::default(); } }; @@ -1171,7 +1171,7 @@ fn load_machine(path: &Path) -> Machine { } Err(e) => { log::warn!("{} does not parse ({e}); quarantining it", path.display()); - quarantine(path); + crate::core::config::quarantine(path); Machine::default() } } @@ -1202,35 +1202,6 @@ pub(crate) fn withdraw_observations() { #[cfg(test)] pub(crate) static OBSERVE_SLOT: Mutex<()> = Mutex::new(()); -fn quarantine(path: &Path) { - let aside = quarantine_path(path); - match std::fs::copy(path, &aside) { - Ok(_) => log::warn!("the previous contents were kept at {}", aside.display()), - Err(e) => log::warn!("could not keep a copy at {}: {e}", aside.display()), - } -} - -fn quarantine_by_rename(path: &Path) { - let aside = quarantine_path(path); - match std::fs::rename(path, &aside) { - Ok(()) => log::warn!("the previous contents were moved to {}", aside.display()), - Err(e) => log::warn!("could not move the file to {}: {e}", aside.display()), - } -} - -fn quarantine_path(path: &Path) -> PathBuf { - const MAX_QUARANTINED: u32 = 8; - - let base = path.with_extension("json.corrupt"); - if !base.exists() { - return base; - } - (1..MAX_QUARANTINED) - .map(|n| path.with_extension(format!("json.corrupt.{n}"))) - .find(|candidate| !candidate.exists()) - .unwrap_or(base) -} - pub fn default_machine_path() -> io::Result { Ok(data_dir()?.join(MACHINE_FILE)) } diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index ed3f01f3..1409b9c5 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -285,7 +285,14 @@ impl WindowViews { match serde_json::from_str(crate::core::config::strip_bom(&text)) { Ok(loaded) => Some(loaded), Err(e) => { - log::warn!("failed to parse views at {}: {e}; ignoring", path.display()); + // The next `save` overwrites this file wholesale, so ignoring a + // corrupt one quietly discards whatever it held. Keep a copy + // aside first, the way `load_machine` does. + log::warn!( + "failed to parse views at {}: {e}; quarantining it", + path.display() + ); + crate::core::config::quarantine(&path); None } } @@ -418,6 +425,30 @@ mod tests { assert_eq!(loaded.active, Some(id)); } + #[test] + fn a_corrupt_views_file_is_kept_aside_before_being_ignored() { + let _file = lock_session_file(); + let dir = pin_config_dir(); + let path = dir.join("views.json"); + let aside = dir.join("views.json.corrupt"); + std::fs::remove_file(&aside).ok(); + std::fs::write(&path, "{ not json").unwrap(); + + assert!( + WindowViews::load().is_none(), + "a corrupt file yields nothing rather than a guess" + ); + assert_eq!( + std::fs::read_to_string(&aside).as_deref().ok(), + Some("{ not json"), + "the next save overwrites views.json wholesale, so the old contents \ + must already be parked beside it" + ); + + std::fs::remove_file(&path).ok(); + std::fs::remove_file(&aside).ok(); + } + #[test] fn an_empty_or_partial_file_decodes_to_defaults() { let empty: WindowViews = serde_json::from_str("{}").unwrap(); diff --git a/crates/tty7-core/src/daemon/mod.rs b/crates/tty7-core/src/daemon/mod.rs index b89d5a39..72f72d99 100644 --- a/crates/tty7-core/src/daemon/mod.rs +++ b/crates/tty7-core/src/daemon/mod.rs @@ -9,6 +9,7 @@ pub(crate) mod remote; pub mod remote_link; pub mod router; pub mod server; +pub mod singleton; pub mod spawn; pub mod ssh; pub mod transport; diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 6f930461..0b1cb2ed 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -165,6 +165,27 @@ macro_rules! startup_note { } pub fn run_daemon() -> anyhow::Result<()> { + // Before either endpoint, and before the machine tree is opened. Whoever + // holds this is the server; everyone else stands down while it lives. + // + // The endpoints cannot decide this between themselves. They are bound in + // sequence, so a second server can take one of them in the window before + // the first takes the other, and the two then serve half a machine each — + // panes on one socket, an empty pane registry answering the machine tree on + // the other. That is how a workspace switch came to rebuild live sessions: + // no error anywhere, just every pane reading as dead. See `singleton`. + let _seat = match crate::daemon::singleton::claim() { + crate::daemon::singleton::Claim::Held(seat) => Some(seat), + crate::daemon::singleton::Claim::Taken => { + startup_note!("tty7-server: another server already serves this config dir; exiting"); + return Ok(()); + } + crate::daemon::singleton::Claim::Unavailable(why) => { + startup_note!("tty7-server: starting without the single-server lock ({why})"); + None + } + }; + let registry = Arc::new(Registry::new()); #[cfg(any(unix, windows))] @@ -200,10 +221,6 @@ pub fn control_services() -> crate::host::server::Services { } } -pub fn run() -> anyhow::Result<()> { - run_with(Arc::new(Registry::new())) -} - /// Say which pseudoconsole this daemon's panes will run on. /// /// `portable-pty` loads a sideloaded `conpty.dll` if one sits beside the diff --git a/crates/tty7-core/src/daemon/singleton.rs b/crates/tty7-core/src/daemon/singleton.rs new file mode 100644 index 00000000..a41c0fb9 --- /dev/null +++ b/crates/tty7-core/src/daemon/singleton.rs @@ -0,0 +1,174 @@ +//! One server per config directory, decided by the kernel. +//! +//! Two servers against one config dir is not a degraded mode, it is a silent +//! one. They do not fight over both endpoints — they split them. The one that +//! keeps `control.sock` answers `MachineGet` out of its own pane registry, +//! which is empty because the panes are all in the *other* process, so every +//! pane in the machine tree reads as dead. Nothing logs an error; the window +//! just quietly rebuilds every session it was asked to restore. +//! +//! That split was reachable because "is a server already running?" was answered +//! by connecting to its socket and treating one failed connect as proof of +//! death — after which the newcomer unlinked the endpoint and bound its own. +//! A connect can fail for a server that is merely slow to reach `accept`, and +//! the loser of that race never learns it was replaced: it holds a listener on +//! an unlinked path and serves nobody, forever. +//! +//! An advisory lock has no such gap. Holding it is the definition of being the +//! server, the kernel drops it when the holder dies however it dies, and there +//! is no stale state to interpret — which is the property the socket probe +//! could never have. +//! +//! Take it *before* either endpoint. Then a stale endpoint can only ever be +//! removed by a process that already knows it is alone, and the removal is safe +//! by construction rather than by timing. + +use std::fs::File; +use std::path::PathBuf; + +/// Proof that this process is the server for its config directory. +/// +/// Released when dropped, and by the kernel if the process never gets to drop +/// it. Hold it for as long as the server serves. +#[derive(Debug)] +pub struct Singleton { + _file: File, +} + +/// The outcome of asking to be the server. +#[derive(Debug)] +pub enum Claim { + /// This process holds it. Serve. + Held(Singleton), + /// Another live server holds it. Stand down and talk to that one. + Taken, + /// The lock could not be evaluated (no config dir, unwritable path). The + /// caller carries on unprotected rather than refusing to start: a server + /// that will not run is worse than one that might race. + Unavailable(String), +} + +fn lock_path() -> Option { + crate::core::config::config_path("daemon.lock") +} + +/// Claims the right to be this machine's server. +pub fn claim() -> Claim { + let Some(path) = lock_path() else { + return Claim::Unavailable("no config directory".into()); + }; + if let Some(parent) = path.parent() { + let _ = std::fs::create_dir_all(parent); + } + match open_exclusive(&path) { + Ok(Some(file)) => Claim::Held(Singleton { _file: file }), + Ok(None) => Claim::Taken, + Err(e) => Claim::Unavailable(format!("{} could not be locked: {e}", path.display())), + } +} + +/// `Ok(Some(file))` when the lock is ours, `Ok(None)` when someone else holds +/// it, `Err` when the question could not be put to the kernel at all. +#[cfg(unix)] +fn open_exclusive(path: &std::path::Path) -> std::io::Result> { + use std::os::unix::io::AsRawFd as _; + + let file = File::options() + .create(true) + .read(true) + .write(true) + .truncate(false) + .open(path)?; + // LOCK_NB so a running server answers "taken" instead of parking this + // process on a lock it will hold until it exits. + let locked = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + if locked == 0 { + return Ok(Some(file)); + } + let e = std::io::Error::last_os_error(); + match e.raw_os_error() { + Some(libc::EWOULDBLOCK) => Ok(None), + _ => Err(e), + } +} + +#[cfg(windows)] +fn open_exclusive(path: &std::path::Path) -> std::io::Result> { + use std::os::windows::fs::OpenOptionsExt as _; + + // share_mode(0) is the whole mechanism: the file stays open for as long as + // the server runs, and every other open of it fails until this handle is + // closed — by `drop`, or by Windows when the process ends. + match File::options() + .create(true) + .read(true) + .write(true) + .share_mode(0) + .open(path) + { + Ok(file) => Ok(Some(file)), + Err(e) if e.raw_os_error() == Some(32) => Ok(None), // ERROR_SHARING_VIOLATION + Err(e) => Err(e), + } +} + +#[cfg(not(any(unix, windows)))] +fn open_exclusive(_path: &std::path::Path) -> std::io::Result> { + // No lock primitive here. `Ok(None)` would read as "taken" and stop the + // server from ever starting; not being able to ask is `Unavailable`. + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "no single-server lock on this platform", + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// `set_config_dir` is process-wide, so these cases cannot run beside each + /// other: one would be claiming the very seat the other is asserting is + /// free, and which failed would depend on the scheduler. + static CONFIG_DIR: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn pin_dir(name: &str) -> (PathBuf, std::sync::MutexGuard<'static, ()>) { + let guard = CONFIG_DIR.lock().unwrap_or_else(|e| e.into_inner()); + let dir = + std::env::temp_dir().join(format!("tty7-singleton-{}-{name}", std::process::id())); + std::fs::create_dir_all(&dir).ok(); + crate::core::config::set_config_dir(dir.clone()); + (dir, guard) + } + + #[test] + fn the_second_claim_is_refused_while_the_first_is_held() { + let _pinned = pin_dir("basic"); + let first = match claim() { + Claim::Held(s) => s, + other => panic!("the first claim must be granted, got {other:?}"), + }; + assert!( + matches!(claim(), Claim::Taken), + "a second server must be told the seat is taken, not race for the endpoints" + ); + drop(first); + assert!( + matches!(claim(), Claim::Held(_)), + "releasing it hands the seat to the next server — no stale file to interpret" + ); + } + + #[test] + fn a_lock_left_behind_by_a_dead_holder_is_claimable() { + let (dir, _guard) = pin_dir("stale"); + let path = dir.join("daemon.lock"); + // The file surviving is exactly what a killed server leaves. It must + // not read as "taken" — that was the old socket probe's failure mode, + // in reverse. + std::fs::write(&path, b"").unwrap(); + assert!( + matches!(claim(), Claim::Held(_)), + "an unlocked file is not a holder, however it came to exist" + ); + } +} diff --git a/src/ui/app.rs b/src/ui/app.rs index bf5c4880..3a50e86f 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -838,7 +838,7 @@ impl Tty7App { let answered = WorkspaceStore::machine_is_connected(cx, self.workspace); if self.tabs.is_empty() && answered - && crate::ui::tree_sync::window_is_informed(cx, self.workspace) + && crate::ui::tree_sync::workspace_is_disposable(cx, self.workspace) { crate::ui::tree_sync::fire_workspace_op(cx, self.workspace, |ws| { tty7_core::daemon::control::ControlRequest::WorkspaceRemove { workspace: ws } @@ -939,7 +939,7 @@ impl Tty7App { } // Anything parked for the workspace we are leaving is now meaningless. self.pending_tab = None; - if self.tabs.is_empty() && crate::ui::tree_sync::window_is_informed(cx, previous) { + if self.tabs.is_empty() && crate::ui::tree_sync::workspace_is_disposable(cx, previous) { crate::ui::tree_sync::fire_workspace_op(cx, previous, |ws| { tty7_core::daemon::control::ControlRequest::WorkspaceRemove { workspace: ws } }); @@ -999,7 +999,7 @@ impl Tty7App { pane_ws.as_ref(), self.workspace, &st.pane, - &alive, + alive.as_ref(), self.font_size, window, cx, @@ -5697,24 +5697,47 @@ fn pane_to_session(pane: &Pane, cx: &App) -> SessionPane { } } +/// The daemon's account of which panes are alive, or `None` when it could not +/// be asked at all. +/// +/// The distinction is the point: a pane absent from a *successful* listing is +/// genuinely gone and may be respawned, while a failed `List` says nothing +/// about any pane. Flattening the failure into an empty map made one transient +/// RPC error read as "every pane is dead", and the restore then spawned fresh +/// shells over all of them — the same destruction-by-inference this file's +/// restore path is built to avoid. pub(crate) fn alive_panes_on( route: &crate::terminal::PaneRoute, -) -> std::collections::HashMap> { +) -> Option>> { if !matches!(route, crate::terminal::PaneRoute::Local) { - return std::collections::HashMap::new(); + return Some(std::collections::HashMap::new()); + } + match crate::terminal::RemoteTerminal::try_list_panes_on(route) { + Ok(list) => Some( + list.into_iter() + .filter(|p| p.alive) + .map(|p| (p.pane_id, p.owner)) + .collect(), + ), + Err(e) => { + log::warn!("could not list panes ({e}); leaving each attach to decide"); + None + } } - crate::terminal::RemoteTerminal::list_panes_on(route) - .into_iter() - .filter(|p| p.alive) - .map(|p| (p.pane_id, p.owner)) - .collect() } fn pane_attachable( - alive: &std::collections::HashMap>, + alive: Option<&std::collections::HashMap>>, id: u64, owner: crate::core::session::WorkspaceId, ) -> bool { + let Some(alive) = alive else { + // No listing to consult. Attaching is the safe guess in both + // directions: if the daemon is really unreachable the attach fails and + // the pane falls to the fresh-spawn path anyway, while spawning fresh + // on a hunch destroys a session that was merely hard to reach. + return true; + }; match alive.get(&id) { None => false, Some(None) => true, @@ -5745,8 +5768,15 @@ fn tabs_from_session( let alive = alive_panes_on(&crate::terminal::PaneRoute::for_workspace(workspace)); let mut tabs: Vec = Vec::with_capacity(session.tabs.len()); for st in &session.tabs { - let Some(pane) = session_to_pane(workspace, owner, &st.pane, &alive, font_size, window, cx) - else { + let Some(pane) = session_to_pane( + workspace, + owner, + &st.pane, + alive.as_ref(), + font_size, + window, + cx, + ) else { log::error!("dropping a restored tab: no pane in it could be started"); continue; }; @@ -5777,7 +5807,7 @@ fn session_to_pane( workspace: Option<&crate::terminal::PaneWorkspace>, owner: WorkspaceId, sp: &SessionPane, - alive: &std::collections::HashMap>, + alive: Option<&std::collections::HashMap>>, font_size: f32, window: &mut Window, cx: &mut Context, @@ -6448,19 +6478,27 @@ mod tests { .into_iter() .collect(); - assert!(pane_attachable(&alive, 1, ours), "our own pane attaches"); assert!( - !pane_attachable(&alive, 2, ours), + pane_attachable(Some(&alive), 1, ours), + "our own pane attaches" + ); + assert!( + !pane_attachable(Some(&alive), 2, ours), "another workspace's pane must spawn fresh instead" ); assert!( - pane_attachable(&alive, 3, ours), + pane_attachable(Some(&alive), 3, ours), "an unowned pane is legacy" ); assert!( - !pane_attachable(&alive, 4, ours), + !pane_attachable(Some(&alive), 4, ours), "a dead id never attaches" ); + assert!( + pane_attachable(None, 4, ours), + "a failed List says nothing about pane 4; the attach itself must decide, \ + because respawning on a transient RPC error destroys a live session" + ); } #[test] diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index da4e5927..1c3021a6 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -770,10 +770,27 @@ fn take_rehydrate(cx: &mut App, client_ws: WorkspaceId, window_is_empty: bool) - (window_is_empty || adopt == Adopt::IfEmpty).then_some(adopt) } -pub(crate) fn window_is_informed(cx: &App, client_ws: WorkspaceId) -> bool { - cx.try_global::() +/// Whether a window with no tabs may delete `client_ws` outright — from the +/// machine's tree and from the store both. +/// +/// Two independent things have to agree, because the window's own emptiness +/// cannot tell them apart: a workspace is empty when it genuinely holds +/// nothing, and equally when its layout failed to rebuild. Only the first is a +/// reason to delete anything, and the second has already cost a workspace with +/// ten live tabs in it. +/// +/// So the window must be informed (it pulled a layout and put it up), *and* the +/// mirror — the machine's own account, which no local failure can empty — must +/// agree there is nothing there. An unprimed mirror knows nothing and answers +/// no: "I don't know" may never authorize a deletion. +pub(crate) fn workspace_is_disposable(cx: &App, client_ws: WorkspaceId) -> bool { + let Some(state) = cx + .try_global::() .and_then(|t| t.windows.get(&client_ws)) - .is_some_and(|s| s.informed) + else { + return false; + }; + state.informed && matches!(&state.sync, SyncPhase::Primed(mirror) if mirror.tabs.is_empty()) } pub(crate) fn mark_window_informed(cx: &mut App, client_ws: WorkspaceId) { @@ -1122,7 +1139,6 @@ fn session_pane_from_node(node: &PaneNode, panes: &[PaneRecord]) -> SessionPane match node { PaneNode::Leaf { pane } => { let record = panes.iter().find(|p| p.id == *pane); - let live = record.is_some_and(|r| r.live); let (cwd, ssh_spec, agent) = match record { Some(r) => ( r.cwd.clone().map(std::path::PathBuf::from), @@ -1133,7 +1149,21 @@ fn session_pane_from_node(node: &PaneNode, panes: &[PaneRecord]) -> SessionPane }; SessionPane::Leaf { cwd, - pane_id: live.then_some(*pane), + // The id goes down whatever `live` says. That flag is a cached + // fact about another process, written by whoever last observed + // the pane and reloaded from disk as `false` on every server + // start — so a quiet pane that nobody has observed since reads + // as dead while its shell is very much alive. Believing it here + // is what threw away live sessions on a workspace switch: the + // id was erased, and the restore below had nothing to attach + // to, so it spawned a fresh shell over a running one. + // + // Attaching is the thing that actually knows. `spawn_shell_ + // terminal_in` attaches when the pane is there and spawns fresh + // when it is not, which is the same answer this filter was + // trying to guess — except it is right. `live` stays a hint for + // what to show, never the judge of what to destroy. + pane_id: Some(*pane), ssh_spec, agent: agent.as_ref().map(|a| a.agent), agent_session_id: agent.as_ref().and_then(|a| a.session_id.clone()), @@ -1370,16 +1400,39 @@ fn finish_hydration( let Some(handle) = crate::ui::windows::WindowRegistry::window_for(cx, client_ws) else { return; }; - log::info!( - "rebuilding {} tab(s) of workspace {client_ws} from its machine's tree", - session.tabs.len() - ); - mark_window_informed(cx, client_ws); + let wanted = session.tabs.len(); + log::info!("rebuilding {wanted} tab(s) of workspace {client_ws} from its machine's tree"); let _ = handle.update(cx, move |_, window, cx| { app.update(cx, |app, cx| { app.adopt_workspace(client_ws, session, window, cx) }); }); + + // Informed *after* the rebuild, and only if the rebuild produced something. + // + // The licence means "this window knows what belongs in this workspace", and + // `switch_workspace` / `detach_workspace` read it as permission to delete a + // workspace that has no tabs — from the machine tree and from the store + // both. Granting it before the rebuild handed that permission to a window + // whose rebuild had not happened yet, and a rebuild can produce nothing: + // `tabs_from_session` drops any tab whose panes all fail to start, which is + // what every tab does when the pane socket is unreachable. The window then + // sat there, empty and authoritative, and the next switch deleted a + // workspace with ten live tabs in it. + // + // Emptiness that came from a failure has to stay indistinguishable from not + // knowing, because that is what it is. + let rebuilt = crate::ui::windows::WindowRegistry::app_for(cx, client_ws) + .and_then(|app| app.upgrade()) + .is_some_and(|app| !app.read(cx).tabs.is_empty()); + if rebuilt || wanted == 0 { + mark_window_informed(cx, client_ws); + } else { + log::warn!( + "workspace {client_ws}: none of its {wanted} tab(s) could be rebuilt; leaving the \ + window uninformed so the layout is not mistaken for an empty workspace" + ); + } } pub(crate) fn on_layout_delta(cx: &mut App, host: HostId, key: &str, delta: LayoutDelta) { @@ -1824,6 +1877,72 @@ mod tests { }); } + /// The rule that stops a failed rebuild from being read as "empty". + /// + /// A window with no tabs may delete its workspace outright — tree and store + /// both — so the two ways of having no tabs must not look alike. Genuinely + /// empty is a reason; "the panes would not start" is not, and it is what + /// every tab looks like when the pane socket has gone away. + #[gpui::test] + fn only_a_mirror_that_agrees_lets_an_empty_window_delete_its_workspace( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| { + let ws = WorkspaceId::new(); + let set = |cx: &mut App, informed: bool, sync: SyncPhase| { + let state = cx + .default_global::() + .windows + .entry(ws) + .or_default(); + state.informed = informed; + state.sync = sync; + }; + let unprimed = || SyncPhase::Unprimed { + dirty: false, + priming: false, + }; + let primed_with = + |tabs: Vec| SyncPhase::Primed(WsMirror { tabs, active: None }); + let a_tab = || TreeTab { + id: TabId::new(), + name: None, + sidebar_group: None, + root: PaneNode::Leaf { pane: 1 }, + }; + + assert!( + !workspace_is_disposable(cx, WorkspaceId::new()), + "a workspace nothing is tracking is not a workspace to delete" + ); + + set(cx, true, unprimed()); + assert!( + !workspace_is_disposable(cx, ws), + "an unpulled mirror knows nothing, and not knowing must never authorize this" + ); + + set(cx, true, primed_with(vec![a_tab()])); + assert!( + !workspace_is_disposable(cx, ws), + "this is the regression: the window came up empty because the rebuild failed, \ + while the machine still held the tabs. Deleting here destroyed them." + ); + + set(cx, false, primed_with(vec![])); + assert!( + !workspace_is_disposable(cx, ws), + "a window that never put up a layout does not get to say what belongs here" + ); + + set(cx, true, primed_with(vec![])); + assert!( + workspace_is_disposable(cx, ws), + "informed, and the machine agrees it holds nothing — the one case that is" + ); + }); + } + #[gpui::test] fn a_hydration_that_died_on_a_stale_link_is_owed_back(cx: &mut gpui::TestAppContext) { cx.update(|cx| { @@ -2550,7 +2669,7 @@ mod tests { } #[test] - fn a_live_leaf_keeps_its_pane_id_and_a_dead_one_lowers_to_a_revival_leaf() { + fn a_lowered_leaf_carries_its_pane_id_and_its_agent_whatever_live_says() { use tty7_core::core::cli_agent::CLIAgent; let tab_id = TabId::new(); let ws = tty7_core::core::machine::Workspace { @@ -2619,8 +2738,12 @@ mod tests { .. } => { assert_eq!( - *pane_id, None, - "a dead pane's leaf takes the fresh-spawn path — that is the revival" + *pane_id, + Some(2), + "a pane the tree calls dead still goes down by its id: the flag is a \ + cached observation from another process — reloaded as false on every \ + server start — and attaching is what settles it. Believing the flag \ + here spawned fresh shells over running sessions." ); assert_eq!(cwd.as_deref(), Some(std::path::Path::new("/work/api"))); assert_eq!(*agent, Some(CLIAgent::Claude)); @@ -2630,6 +2753,47 @@ mod tests { } } + /// The regression behind "switching workspaces threw away every session". + /// + /// Two servers had started against one config dir — one holding the control + /// socket with an empty pane registry, the other holding the panes — so + /// `MachineGet` answered with every `live` still `false`, the value + /// `load_machine` stamps on a cold read. Erasing the id on that made the + /// restore spawn a fresh shell over each running one, and nineteen live + /// agent sessions went out with it. + /// + /// The id has to survive a `live: false`, because nothing here is entitled + /// to declare a pane dead. Attaching is. + #[test] + fn a_pane_the_tree_calls_dead_still_goes_down_by_its_id() { + let tab_id = TabId::new(); + let ws = tty7_core::core::machine::Workspace { + tabs: vec![TreeTab { + id: tab_id, + name: None, + sidebar_group: None, + root: PaneNode::Leaf { pane: 7 }, + }], + active_tab: Some(tab_id), + ..Default::default() + }; + let panes = vec![PaneRecord { + id: 7, + cwd: Some("/work".into()), + live: false, + ..PaneRecord::new(7) + }]; + + match &session_from_tree(&ws, &panes).tabs[0].pane { + SessionPane::Leaf { pane_id, .. } => assert_eq!( + *pane_id, + Some(7), + "the attach decides whether pane 7 is still there; this must not pre-empt it" + ), + _ => panic!("leaf"), + } + } + #[test] fn a_dangling_active_tab_in_the_pulled_tree_falls_back_to_the_first() { let ws = tty7_core::core::machine::Workspace {