diff --git a/src/app/actions.rs b/src/app/actions.rs index 5330408a110..8a42480d8ff 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -644,6 +644,36 @@ impl AppState { self.terminal_runtime_shutdowns.push(terminal_id); } } + self.prune_runtime_binding_owners(); + } + + pub(crate) fn runtime_binding_owner_is_live(&self, public_pane_id: &str) -> bool { + let Some((workspace_id, _)) = public_pane_id.split_once(':') else { + return false; + }; + self.workspaces + .iter() + .filter(|ws| ws.id == workspace_id) + .any(|ws| { + ws.public_pane_numbers.iter().any(|(&pane_id, &number)| { + ws.pane_state(pane_id).is_some() + && crate::workspace::public_pane_id_for_number(&ws.id, number) + == public_pane_id + }) + }) + } + + /// Drops binding owners, bound or unbound, whose public pane no longer exists. + /// Owners of open panes are kept so an unbind followed by a rebind of the same + /// binding on the same pane continues its sequence guard. + pub(crate) fn prune_runtime_binding_owners(&mut self) -> bool { + let before = self.runtime_binding_owners.len(); + let owners = std::mem::take(&mut self.runtime_binding_owners); + self.runtime_binding_owners = owners + .into_iter() + .filter(|(_, owner)| self.runtime_binding_owner_is_live(&owner.pane_id)) + .collect(); + self.runtime_binding_owners.len() != before } pub(crate) fn remove_plugin_pane_records( diff --git a/src/app/api/runtime_attachment.rs b/src/app/api/runtime_attachment.rs index af89c539572..acef238d9fb 100644 --- a/src/app/api/runtime_attachment.rs +++ b/src/app/api/runtime_attachment.rs @@ -252,6 +252,7 @@ impl App { }, ); } + let pruned = self.state.prune_runtime_binding_owners(); if exact { if let Some(update) = self.state.update_terminal_state(pane_id, |terminal| { Some(terminal.mutate_runtime_binding(None, None)) @@ -259,9 +260,11 @@ impl App { self.emit_pane_state_update(&update); } self.sync_agent_metadata_deadline(); - self.schedule_session_save(); self.emit_pane_updated(ws_idx, pane_id); } + if exact || pruned { + self.schedule_session_save(); + } encode_success( id, ResponseResult::PaneRuntime { @@ -394,6 +397,110 @@ mod tests { app.state.assert_invariants_for_test(); } + #[test] + fn runtime_attachment_owner_is_pruned_when_owning_pane_closes() { + let mut app = app(); + let pane_id = |app: &App, tab: usize| { + app.public_pane_id(0, app.state.workspaces[0].tabs[tab].root_pane) + .unwrap() + }; + let bound_pane = pane_id(&app, 0); + let unbound_pane = pane_id(&app, 1); + let survivor_pane = pane_id(&app, 2); + let bind = |pane_id: &str, binding_id: &str| PaneBindRuntimeParams { + pane_id: pane_id.into(), + binding_id: binding_id.into(), + attachment: attachment(), + }; + for (pane, binding_id) in [(&bound_pane, "bound"), (&unbound_pane, "unbound")] { + assert!(app + .handle_pane_bind_runtime("bind".into(), bind(pane, binding_id)) + .contains("\"applied\":true")); + } + assert!(app + .handle_pane_unbind_runtime( + "unbind".into(), + PaneUnbindRuntimeParams { + pane_id: unbound_pane.clone(), + binding_id: "unbound".into(), + } + ) + .contains("\"applied\":true")); + assert!(app.state.runtime_binding_owners.contains_key("unbound")); + + app.handle_pane_close( + "close-bound".into(), + PaneTarget { + pane_id: bound_pane, + }, + ); + assert!(!app.state.runtime_binding_owners.contains_key("bound")); + assert!(app.state.runtime_binding_owners.contains_key("unbound")); + app.state.assert_invariants_for_test(); + + app.handle_pane_close( + "close-unbound".into(), + PaneTarget { + pane_id: unbound_pane, + }, + ); + assert!(app.state.runtime_binding_owners.is_empty()); + app.state.assert_invariants_for_test(); + + let rebound: SuccessResponse = serde_json::from_str( + &app.handle_pane_bind_runtime("rebind".into(), bind(&survivor_pane, "bound")), + ) + .unwrap(); + let ResponseResult::PaneRuntime { + binding: Some(binding), + .. + } = rebound.result + else { + panic!("released binding should bind to a live pane") + }; + assert_eq!(binding.pane_id, survivor_pane); + assert_eq!(binding.seq, None); + app.state.assert_invariants_for_test(); + } + + #[test] + fn runtime_attachment_unbind_prunes_owners_of_missing_panes() { + let mut app = app(); + let pane_id = app + .public_pane_id(0, app.state.workspaces[0].tabs[0].root_pane) + .unwrap(); + let bind = PaneBindRuntimeParams { + pane_id: pane_id.clone(), + binding_id: "binding-a".into(), + attachment: attachment(), + }; + app.handle_pane_bind_runtime("bind".into(), bind.clone()); + let mut stale = app.state.runtime_binding_owners["binding-a"].clone(); + stale.binding_id = "stale".into(); + stale.pane_id = format!("{}:p{}", app.state.workspaces[0].id, "ZZZ"); + app.state + .runtime_binding_owners + .insert("stale".into(), stale); + assert!(app + .handle_pane_unbind_runtime( + "unbind".into(), + PaneUnbindRuntimeParams { + pane_id: pane_id.clone(), + binding_id: "binding-a".into(), + } + ) + .contains("\"applied\":true")); + assert_eq!( + app.state.runtime_binding_owners.keys().collect::>(), + vec!["binding-a"] + ); + assert_eq!( + app.state.runtime_binding_owners["binding-a"].pane_id, + pane_id + ); + app.state.assert_invariants_for_test(); + } + #[test] fn runtime_attachment_observation_survives_tui_navigation_and_expires() { let mut app = app(); diff --git a/src/app/mod.rs b/src/app/mod.rs index ad1f9e3fb76..73b0cb5feab 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -535,6 +535,9 @@ impl App { }; state.terminals = restored_terminals; + // Owners of panes that did not survive restore, or tombstones from older + // snapshots, are dropped and written out with the next session save. + state.prune_runtime_binding_owners(); for ws_idx in 0..state.workspaces.len() { let cwd = state.workspaces[ws_idx] @@ -673,6 +676,7 @@ impl App { app.terminal_runtimes = runtimes.into(); app.state.runtime_binding_owners = snapshot.runtime_binding_owners.clone(); app.sync_agent_metadata_deadline(); + app.state.prune_runtime_binding_owners(); app.state.active = snapshot .active .filter(|&idx| idx < app.state.workspaces.len()); diff --git a/src/app/session.rs b/src/app/session.rs index bac22adea6e..604f6f38bf5 100644 --- a/src/app/session.rs +++ b/src/app/session.rs @@ -149,3 +149,61 @@ fn run_session_save_job( SessionSaveJob::Save { snapshot, history } => writer.save(&snapshot, history.as_ref()), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn session_save_clears_after_last_runtime_binding_owner_pane_closes() { + let (_, rx) = tokio::sync::mpsc::unbounded_channel(); + let mut app = App::new( + &crate::config::Config::default(), + crate::app::AppPolicy::TEST, + None, + rx, + crate::api::EventHub::default(), + ); + app.state = crate::app::AppState::test_with_adversarial_identity_state(); + app.state.ensure_test_terminals(); + let pane_id = app + .public_pane_id(0, app.state.workspaces[0].tabs[0].root_pane) + .expect("root pane should have a public ID"); + app.state.runtime_binding_owners.insert( + "binding-a".into(), + crate::api::schema::RuntimeBinding { + pane_id, + binding_id: "binding-a".into(), + attachment: crate::api::schema::RuntimeAttachment { + provider: "shuvcode".into(), + home_id: "home-a".into(), + session_id: "ses_a".into(), + location: "/".into(), + host_id: "local".into(), + attach_argv: vec!["shuvcode".into()], + }, + seq: Some(3), + state: crate::api::schema::RuntimeState::Unknown, + label: None, + ttl_ms: crate::runtime_attachment::DEFAULT_TTL_MS, + fresh: false, + }, + ); + app.state.assert_invariants_for_test(); + assert!(matches!( + app.capture_session_save_job(), + SessionSaveJob::Save { .. } + )); + + while !app.state.workspaces.is_empty() { + app.state.close_selected_workspace(); + } + + assert!(app.state.runtime_binding_owners.is_empty()); + assert!(matches!( + app.capture_session_save_job(), + SessionSaveJob::Clear + )); + app.state.assert_invariants_for_test(); + } +} diff --git a/src/app/state.rs b/src/app/state.rs index f85e88e4546..51660d325fa 100644 --- a/src/app/state.rs +++ b/src/app/state.rs @@ -1168,6 +1168,13 @@ impl AppState { } pub fn assert_invariants_for_test(&self) { + for (binding_id, owner) in &self.runtime_binding_owners { + assert!( + self.runtime_binding_owner_is_live(&owner.pane_id), + "runtime binding owner {binding_id} references closed pane {}", + owner.pane_id + ); + } if self.workspaces.is_empty() { assert!( self.active.is_none(), diff --git a/src/persist/restore.rs b/src/persist/restore.rs index a2e536420bd..e8f7038d659 100644 --- a/src/persist/restore.rs +++ b/src/persist/restore.rs @@ -269,7 +269,8 @@ fn restore_with_imports_and_failures( render_notify: Arc, render_dirty: Arc, ) -> RestoreFailures { - // Closed attachment owners still reserve public identity after an empty restart. + // Saved attachment owners reserve their public identity even when their pane fails + // to restore, so a later pane never reuses an ID a pre-restore client may hold. crate::workspace::reserve_public_workspace_ids( snapshot .runtime_binding_owners @@ -1563,6 +1564,114 @@ mod tests { assert_eq!(workspace.next_public_tab_number, 6); } + #[tokio::test] + async fn restore_keeps_live_runtime_binding_owners_and_reserves_their_identity() { + let cwd = std::env::current_dir().unwrap(); + let owner = |binding_id: &str, pane_id: &str| { + ( + binding_id.to_string(), + crate::api::schema::RuntimeBinding { + pane_id: pane_id.into(), + binding_id: binding_id.into(), + attachment: crate::api::schema::RuntimeAttachment { + provider: "shuvcode".into(), + home_id: "home-a".into(), + session_id: binding_id.into(), + location: cwd.display().to_string(), + host_id: "local".into(), + attach_argv: vec!["shuvcode".into()], + }, + seq: Some(7), + state: crate::api::schema::RuntimeState::Unknown, + label: None, + ttl_ms: crate::runtime_attachment::DEFAULT_TTL_MS, + fresh: false, + }, + ) + }; + let live_workspace = format!("w{}", crate::workspace::encode_public_number(40)); + let closed_workspace = format!("w{}", crate::workspace::encode_public_number(41)); + let live_pane = crate::workspace::public_pane_id_for_number(&live_workspace, 1); + let snapshot = SessionSnapshot { + version: super::super::snapshot::SNAPSHOT_VERSION, + workspaces: vec![WorkspaceSnapshot { + id: Some(live_workspace.clone()), + custom_name: None, + identity_cwd: cwd.clone(), + worktree_space: None, + public_pane_numbers: HashMap::from([(0, 1)]), + next_public_pane_number: 2, + public_tab_numbers: vec![1], + next_public_tab_number: 2, + tabs: vec![TabSnapshot { + custom_name: None, + layout: LayoutSnapshot::Pane(0), + panes: HashMap::from([( + 0, + super::super::snapshot::PaneSnapshot { + cwd: cwd.clone(), + label: None, + agent_name: None, + managed_agent_kind: None, + agent_session: None, + runtime_binding: None, + launch_argv: None, + }, + )]), + zoomed: false, + focused: Some(0), + root_pane: Some(0), + }], + active_tab: 0, + }], + active: Some(0), + selected: 0, + sidebar_width: None, + sidebar_section_split: None, + collapsed_space_keys: Default::default(), + runtime_binding_owners: HashMap::from([ + owner("live-binding", &live_pane), + owner( + "legacy-tombstone", + &crate::workspace::public_pane_id_for_number(&closed_workspace, 1), + ), + ]), + }; + let (events, _event_rx) = mpsc::channel(4); + + let (workspaces, terminals, _runtimes) = restore( + &snapshot, + None, + 24, + 80, + 0, + test_restore_shell(), + crate::config::ShellModeConfig::NonLogin, + false, + events, + Arc::new(Notify::new()), + Arc::new(RenderSignal::new()), + ); + + let next = crate::workspace::generate_workspace_id(); + assert!( + crate::workspace::public_workspace_number(&next).is_some_and(|number| number > 41), + "restored owner identities must stay reserved, got {next}" + ); + let mut state = crate::app::AppState::test_new(); + state.workspaces = workspaces; + state.terminals = terminals; + state.active = Some(0); + state.runtime_binding_owners = snapshot.runtime_binding_owners.clone(); + assert!(state.prune_runtime_binding_owners()); + assert_eq!( + state.runtime_binding_owners.keys().collect::>(), + vec!["live-binding"] + ); + assert_eq!(state.runtime_binding_owners["live-binding"].seq, Some(7)); + state.assert_invariants_for_test(); + } + #[tokio::test] async fn cold_restore_with_gapped_public_tab_numbers_drops_unmanaged_agent_name() { let cwd = std::env::current_dir().unwrap(); diff --git a/tests/api_ping.rs b/tests/api_ping.rs index c38e2c7dac5..a0d43e6ae5a 100644 --- a/tests/api_ping.rs +++ b/tests/api_ping.rs @@ -2791,7 +2791,7 @@ fn native_runtime_attachment_public_json_persists_identity_and_rejects_stale_rep } #[test] -fn native_runtime_attachment_closed_owner_reserves_identity_after_empty_restart() { +fn native_runtime_attachment_closed_owner_is_released_after_empty_restart() { let _lock = test_lock(); let base = unique_test_dir(); fs::create_dir_all(&base).unwrap(); @@ -2830,12 +2830,11 @@ fn native_runtime_attachment_closed_owner_reserves_identity_after_empty_restart( serde_json::json!({"cwd":base,"focus":false}), ); let replacement_pane = &replacement["result"]["root_pane"]["pane_id"]; - assert_ne!(replacement_pane, pane); bind["pane_id"] = replacement_pane.clone(); - assert_eq!( - request(&socket, "pane.bind_runtime", bind)["error"]["code"], - "runtime_binding_conflict" - ); + let rebound = request(&socket, "pane.bind_runtime", bind); + assert_eq!(rebound["result"]["applied"], true, "{rebound}"); + assert_eq!(rebound["result"]["binding"]["pane_id"], *replacement_pane); + assert!(rebound["result"]["binding"]["seq"].is_null()); request(&socket, "server.stop", serde_json::json!({})); restarted.child.wait().unwrap(); cleanup_spawned_herdr(restarted, base);