From 8a7302ae347338032e504eea3601703d01e8f510 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 23 Aug 2026 11:36:47 +0800 Subject: [PATCH] fix(cli): stop `pane close --orphans` reaping a session mid-restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reaper's test was "the registry is running it and no workspace holds it". A window restoring a layout spawns each pane, attaches to it, and only then files it into the tree — so for a moment every pane it is adopting answers to that description. Not theoretical. Polling `pane ls --all` through a cold start of a seven-tab window reported one, then two, then three, then six, then seven live panes as held by no workspace, and the restore afterwards was correct: all seven were wanted. `--orphans` at any point in that window takes the whole session, and a script that reaps on a timer will eventually sit in it. Attachment is the missing half, and it is the daemon's own fact rather than a guess about timing: `attach` takes the pane's seat and the connection closing calls `detach`, which clears it. A window adopting a pane is attached to it. A pane whose layout was thrown away had its view dropped, which closed the connection, which emptied the seat. Nothing else in the registry tells the two apart, which is why the GUI had to ask its windows and the CLI could not. `pane ls --all` counts the same set, because the line it prints tells the reader to run the reaper. Same race after the change: zero false positives across 300 polls, restore intact. And a stray with nobody attached is still reaped — checked by deleting the new filter, which fails both tests. --- crates/tty7-cli/src/commands.rs | 111 +++++++++++++++++++++++- crates/tty7-cli/src/output.rs | 1 + crates/tty7-core/src/daemon/pane.rs | 10 ++- crates/tty7-core/src/daemon/protocol.rs | 21 +++++ crates/tty7-core/src/host/server.rs | 1 + src/ui/switcher.rs | 2 + 6 files changed, 143 insertions(+), 3 deletions(-) diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index 7b7a6e75..a5577612 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -994,9 +994,11 @@ fn pane_ls_all(backend: &mut dyn Backend) -> Result { }) }) .collect(); + // The same test `orphan_panes` makes, so the count and the reaper cannot + // disagree — the line names `pane close --orphans` as the fix for it. let orphans = running .iter() - .filter(|info| holder(info.pane_id).is_none()) + .filter(|info| !info.attached && holder(info.pane_id).is_none()) .count(); let mut human = output::registry_table(&running, &|pane| holder(pane).map(|ws| ws.to_string())); if orphans > 0 { @@ -1135,6 +1137,24 @@ fn pane_close( /// that predates it simply omits it — but it is a wire change, and until it /// happens the honest thing is that the docs say plainly that `--orphans` will /// kill a `run` started from another shell. +/// The panes nothing holds and nobody is watching. +/// +/// "No workspace holds it" is not enough on its own, and the gap is not +/// theoretical: during a restore the window spawns a pane, attaches to it, and +/// only then files it into the tree. Polled through a cold start, seven live +/// panes of a seven-tab window each reported as held by no workspace — every +/// one of them about to be adopted. A reaper run at that moment takes the +/// session it was meant to clean up after. +/// +/// So attachment is the other half. It is the daemon's own fact rather than a +/// guess about timing: `attach` takes the seat and a pane connection closing +/// calls `detach`, which clears it. A pane a window is adopting is attached; a +/// pane whose layout was thrown away had its view dropped, which closed the +/// connection, which emptied the seat. That is the whole difference between +/// the two, and nothing else in the registry shows it. +/// +/// An older server that does not send the field reads as unattached, which is +/// what this command did before. fn orphan_panes(machine: &Machine, backend: &mut dyn Backend) -> Result> { let held: Vec = machine .workspaces @@ -1145,6 +1165,7 @@ fn orphan_panes(machine: &Machine, backend: &mut dyn Backend) -> Result Ok(backend .list_panes()? .iter() + .filter(|info| !info.attached) .map(|info| info.pane_id) .filter(|pane| !held.contains(pane)) .collect()) @@ -2237,10 +2258,23 @@ mod tests { title: "sh".into(), osc_title: None, alive: true, + attached: false, owner: owner.map(str::to_string), } } + /// The same pane with a client watching it — what a window adopting one + /// looks like from the registry. + fn attached_pane_info( + pane_id: u64, + owner: Option<&str>, + ) -> tty7_core::daemon::protocol::PaneInfo { + tty7_core::daemon::protocol::PaneInfo { + attached: true, + ..pane_info(pane_id, owner) + } + } + /// One condition, one sentence. /// /// `capture %999` used to answer with the wire request's name — "daemon @@ -2896,6 +2930,81 @@ mod tests { ); } + /// A pane a client is watching is never reaped, however unheld it looks. + /// + /// Reproduced before this existed: polling `pane ls --all` through a cold + /// start of a seven-tab window reported seven live panes as held by no + /// workspace, one after another, because the window spawns and attaches + /// before it files them. `--orphans` at that moment takes the session. + /// + /// The second half matters as much: a stray really is still reaped. It is + /// the same pane with nobody attached, which is exactly what a dropped + /// view leaves behind, and the reaper must still take it or the recovery + /// tool has been quietly turned off. + #[test] + fn pane_close_orphans_spares_a_pane_a_client_is_still_watching() { + let mut backend = mock(); + backend.registry = vec![ + pane_info(1, None), + // Unheld and attached: a window is adopting it. + attached_pane_info(77, Some("tty7-app")), + // Unheld and unwatched: a view was dropped and never came back. + pane_info(78, Some("tty7-app")), + ]; + + let json = json_of(run_cli( + &["tty7", "pane", "close", "--orphans"], + &Context::default(), + &mut backend, + )); + assert_eq!( + json["closed"], + serde_json::json!([78]), + "only the pane nobody is watching" + ); + assert_eq!(backend.killed, vec![78], "and %77 was left running"); + } + + /// The count in `pane ls --all` names the set `--orphans` would take. + /// + /// That line tells the reader to run the reaper, so a number arrived at + /// some other way sends them after panes it will not touch — or worse, + /// reads zero while the reaper is about to take something. + #[test] + fn the_orphan_count_and_the_reaper_agree() { + let mut backend = mock(); + backend.registry = vec![ + pane_info(1, None), + attached_pane_info(77, Some("tty7-app")), + pane_info(78, Some("tty7-app")), + ]; + let counted = json_of(run_cli( + &["tty7", "pane", "ls", "--all"], + &Context::default(), + &mut backend, + ))["orphans"] + .as_u64() + .expect("a count"); + + let mut backend = mock(); + backend.registry = vec![ + pane_info(1, None), + attached_pane_info(77, Some("tty7-app")), + pane_info(78, Some("tty7-app")), + ]; + let reaped = json_of(run_cli( + &["tty7", "pane", "close", "--orphans"], + &Context::default(), + &mut backend, + ))["closed"] + .as_array() + .expect("a list") + .len() as u64; + + assert_eq!(counted, reaped, "the count is the set the reaper takes"); + assert_eq!(counted, 1, "and it is the unwatched one"); + } + /// The CLI is what creates orphans, so it should be able to clear them. /// `--orphans` closes exactly the panes the registry holds and the tab /// trees do not — panes that *are* held must survive it untouched. diff --git a/crates/tty7-cli/src/output.rs b/crates/tty7-cli/src/output.rs index 8e400e87..693ca7d2 100644 --- a/crates/tty7-cli/src/output.rs +++ b/crates/tty7-cli/src/output.rs @@ -464,6 +464,7 @@ mod tests { let held = "9fd8072f-465c-4016-9a81-8143bff1240c"; let elsewhere = "76698a44-3f13-4961-8fed-90d0b3defff1"; let pane = |id: u64, owner: Option<&str>| PaneInfo { + attached: false, pane_id: id, cwd: None, title: "zsh".into(), diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 8107f869..2075203e 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -2154,9 +2154,14 @@ impl DaemonPane { } pub fn info(&self) -> PaneInfo { - let (cwd, osc_title, alive) = { + let (cwd, osc_title, alive, attached) = { let st = self.state.locked(); - (st.cwd.clone(), st.osc_title.clone(), st.alive) + ( + st.cwd.clone(), + st.osc_title.clone(), + st.alive, + st.subscriber.is_some(), + ) }; PaneInfo { pane_id: self.id, @@ -2164,6 +2169,7 @@ impl DaemonPane { title: self.foreground_title(), osc_title, alive, + attached, owner: self.owner.clone(), } } diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index 91901d1f..20c82f20 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -144,6 +144,25 @@ pub struct PaneInfo { #[serde(default, skip_serializing_if = "Option::is_none")] pub osc_title: Option, pub alive: bool, + /// Whether a client is attached to this pane right now. + /// + /// The daemon's own answer to "is anybody watching this", kept by + /// `PaneHandle::attach`/`detach` — a pane's connection closing clears the + /// seat, so a view that has been dropped stops counting immediately. + /// + /// It is what tells a pane the tree has not filed *yet* from one it will + /// never file. Both are live and unheld, and nothing else distinguishes + /// them: a window adopting a pane is attached to it, and a pane whose + /// layout was thrown away is not. `orphan_panes` spends it so that + /// `tty7 pane close --orphans` cannot reap a session mid-restore. + /// + /// `#[serde(default)]`, and deliberately not a `PROTOCOL_VERSION` bump, + /// for the same reason as `AuthPromptKind::KeyPassphrase::rejected`: an + /// older peer on either side never sets it and serde ignores fields it + /// does not know, so a peer that cannot answer reads as unattached — which + /// is what this command did before the field existed. + #[serde(default)] + pub attached: bool, #[serde(default, skip_serializing_if = "Option::is_none")] pub owner: Option, } @@ -1813,6 +1832,7 @@ mod tests { DaemonMsg::Exited { code: None }, DaemonMsg::PaneList(vec![ PaneInfo { + attached: false, pane_id: 3, cwd: Some(PathBuf::from("/x")), title: "zsh".into(), @@ -1821,6 +1841,7 @@ mod tests { owner: None, }, PaneInfo { + attached: false, pane_id: 4, cwd: None, title: String::new(), diff --git a/crates/tty7-core/src/host/server.rs b/crates/tty7-core/src/host/server.rs index 0ad785de..2007c828 100644 --- a/crates/tty7-core/src/host/server.rs +++ b/crates/tty7-core/src/host/server.rs @@ -1762,6 +1762,7 @@ mod aggregate_tests { attachments: Arc::new(AttachRegistry::default()), panes: Some(Arc::new(ThreePanesOneAgent { panes: vec![PaneInfo { + attached: false, pane_id: 7, cwd: Some(PathBuf::from("/repo/tty7")), title: "nvim".into(), diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 6d6bbd19..cda77913 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -3454,6 +3454,7 @@ mod tests { let listed: Vec = shown .iter() .map(|&pane_id| PaneInfo { + attached: false, pane_id, cwd: None, title: "zsh".to_string(), @@ -3484,6 +3485,7 @@ mod tests { fn info(pane_id: u64, alive: bool) -> PaneInfo { PaneInfo { + attached: false, pane_id, cwd: Some(std::path::PathBuf::from("/tmp/x")), title: "zsh".to_string(),