diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index 37a5b224..486d50fc 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -408,32 +408,6 @@ pub struct Workspace { /// workspace from the laptop at home. #[serde(default, skip_serializing_if = "Option::is_none")] pub host: Option, - /// Identity of the daemon *process* the pane ids in `session` refer to - /// (see `daemon::protocol::DaemonVersion::instance`). One field for the - /// whole workspace, not one per leaf, because a workspace's panes all live - /// in one daemon (one window, one machine). - /// - /// This is what makes a saved pane id safe to trust: daemon ids restart - /// from 1, so after a reboot every saved id points at whatever unrelated - /// shell happens to hold the number now — and restore's aliveness check - /// cannot tell a survivor from a squatter. A claim whose instance differs - /// from the daemon now serving blanks its ids instead - /// ([`Workspace::forget_stale_pane_ids`]) and takes the fresh-spawn path, - /// agent resume included, which is the correct reading of "the daemon - /// those panes lived in is gone". - /// - /// A remote workspace records its machine's `tty7-server` instance here, - /// for exactly the same reason and read by exactly the same check. The live - /// per-connection tracking on the client (`note_instance`) does not replace - /// this: that map is in memory, so it is empty on the launch where it would - /// matter most — the one after a client restart that spanned a server - /// replacement. - /// - /// `None` for records written before the field, and whenever the serving - /// process cannot be named (an older peer, a machine not connected). `None` - /// disables the check, never fails it. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub daemon_instance: Option, } impl Default for Workspace { @@ -446,7 +420,6 @@ impl Default for Workspace { open: true, last_active: now_secs(), host: None, - daemon_instance: None, } } } @@ -552,29 +525,6 @@ impl Workspace { forgotten } - /// Blank every saved pane id if it was recorded against a *different* - /// daemon process than `current` — see [`Workspace::daemon_instance`] for - /// the id-reuse failure this closes. Answers how many ids were dropped. - /// - /// Only a **known, differing** instance pair trips it. `None` on either - /// side means "cannot tell" (an old record, an old daemon), and treating - /// that as stale would respawn every pane on the first launch after an - /// upgrade — exactly the sessions persistence exists to keep. - /// - /// The agent fields stay, deliberately: unlike a *duplicate* claim (see - /// `drop_duplicate_pane_ids`), a stale-instance claim means the pane is - /// genuinely gone with its daemon, nothing else is running the - /// conversation, and the fresh shell resuming it is the feature. - pub fn forget_stale_pane_ids(&mut self, current: Option<&str>) -> usize { - let (Some(recorded), Some(current)) = (self.daemon_instance.as_deref(), current) else { - return 0; - }; - if recorded == current { - return 0; - } - self.forget_pane_ids() - } - /// Stamp this workspace as just-focused. pub fn touch(&mut self) { self.last_active = now_secs(); @@ -614,68 +564,6 @@ impl Workspace { None => crate::host::HostId::LOCAL, } } - - /// The record the **remote** owns, as the JSON that crosses the wire in a - /// [`WorkspacePut`](crate::daemon::control::ControlRequest::WorkspacePut). - /// - /// The storage split, executable rather than aspirational: what - /// stays here is `window`, `open` and `host` — this client's view state — - /// and what goes over there is everything that is a fact about the machine. - /// [`REMOTE_OWNED_FIELDS`] pins the split, and a test fails if a new field - /// is added without a decision about which side it belongs to. - pub fn to_remote_json(&self) -> serde_json::Value { - let mut value = serde_json::to_value(self).unwrap_or(serde_json::Value::Null); - if let Some(obj) = value.as_object_mut() { - obj.retain(|k, _| REMOTE_OWNED_FIELDS.contains(&k.as_str())); - } - value - } - - /// Merge an authoritative record pulled from a remote store into this entry. - /// - /// Touches only the remote-owned fields. `id`, `host`, `window` and `open` - /// are left exactly as they were — the first two because the client's entry - /// is the thing being *pointed* by them, the last two because they are this - /// machine's view state and the remote has no opinion about them. - pub fn apply_remote_json(&mut self, value: &serde_json::Value) -> serde_json::Result<()> { - let record: RemoteRecord = serde_json::from_value(value.clone())?; - self.name = record.name; - self.session = record.session; - self.last_active = record.last_active; - Ok(()) - } -} - -/// The `Workspace` fields the **remote** is the authority for. -/// Everything else is client-side view state and never leaves this machine. -/// -/// A `Workspace` field that is in neither list is a bug: it would be dropped by -/// [`Workspace::to_remote_json`] and silently lost on the next pull. The test -/// `the_storage_split_covers_every_workspace_field` is what makes that a red -/// build rather than a data-loss report. -pub const REMOTE_OWNED_FIELDS: &[&str] = &["id", "name", "session", "last_active"]; - -/// The client-side view state, which stays in this machine's `session.json`. -/// `daemon_instance` is client-owned because it records **which serving process -/// this client last saw** — an observation, not a property of the workspace. Two -/// clients open on one remote workspace each keep their own, and neither may -/// overwrite the other's; a remote record that carried it would do exactly that. -pub const CLIENT_OWNED_FIELDS: &[&str] = &["window", "open", "host", "daemon_instance"]; - -/// The remote-owned half of a [`Workspace`], for reading a record back. -/// -/// Every field defaults: a record written by a *newer* client carries fields -/// this build has never heard of (serde ignores them), and one written by an -/// older client is missing fields this build expects. Neither may fail the pull -/// — a workspace that will not decode is a workspace the user cannot open. -#[derive(Deserialize)] -struct RemoteRecord { - #[serde(default)] - name: Option, - #[serde(default)] - session: Session, - #[serde(default)] - last_active: u64, } /// The whole `session.json`: every workspace tty7 knows about, plus which one @@ -780,48 +668,6 @@ impl Workspaces { closed } - /// Drop pane ids that appear in more than one workspace *on the same - /// machine*, keeping the claim of whichever workspace was active most - /// recently. A duplicate would have two windows attach the same daemon - /// pane, and the daemon's single subscriber means the loser's terminal goes - /// silently dead — so this runs on every load, before any window is built. - /// - /// **Scoped per machine, because a pane id only means anything within one - /// daemon.** Every daemon hands out 1, 2, 3…, so a laptop and a build box - /// both having a pane 1 is the normal case, not a conflict. Deduping - /// globally would make the remote workspace forfeit a claim on a pane that - /// is alive and well on its own machine — orphaning a live session over a - /// collision that never existed. - /// - /// Returns the number of claims dropped (0 in the healthy case). - pub fn dedupe_pane_ids(&mut self) -> usize { - let mut order: Vec<(usize, u64)> = self - .workspaces - .iter() - .enumerate() - .map(|(i, w)| (i, w.last_active)) - .collect(); - // Most recently active first: it keeps its claim, earlier ones yield. - order.sort_by(|a, b| b.1.cmp(&a.1)); - - // One `seen` set per machine. `HostId` is process-local, but this only - // has to be self-consistent within the single pass below. - let mut seen: std::collections::HashMap< - crate::host::HostId, - std::collections::HashSet, - > = std::collections::HashMap::new(); - let mut dropped = 0; - for (index, _) in order { - let workspace = &mut self.workspaces[index]; - let host = workspace.host_id(); - let seen_here = seen.entry(host).or_default(); - for tab in &mut workspace.session.tabs { - dropped += drop_duplicate_pane_ids(&mut tab.pane, seen_here); - } - } - dropped - } - /// Persist as JSON, creating the parent directory if needed. Any /// IO/serialization error is logged and swallowed — the app must never /// crash or stall over session bookkeeping. @@ -904,46 +750,6 @@ pub fn blank_pane_ids(pane: &mut SessionPane) -> usize { } } -/// Blank any `pane_id` already claimed by an earlier-visited workspace. A -/// blanked leaf still restores — it just spawns a fresh shell in its saved cwd, -/// the same path a session from before the daemon existed takes. -/// -/// The agent resume fields go with it. A blanked leaf takes restore's -/// spawn-fresh path, and that path auto-types the agent's resume command — -/// but the pane this claim duplicated is still running that very agent under -/// its winning workspace, so "recovering" the loser would start a second -/// process on the same agent session id. The duplicate claim is the evidence -/// of a corrupted record, not of a lost conversation; the conversation lives -/// with the winner. -fn drop_duplicate_pane_ids( - pane: &mut SessionPane, - seen: &mut std::collections::HashSet, -) -> usize { - match pane { - SessionPane::Leaf { - pane_id, - agent_session_id, - agent_launch_argv, - .. - } => match *pane_id { - Some(id) if !seen.insert(id) => { - log::warn!( - "workspace claims pane {id} twice; dropping the duplicate claim \ - (and its agent resume, which the winning claim still owns)" - ); - *pane_id = None; - *agent_session_id = None; - *agent_launch_argv = None; - 1 - } - _ => 0, - }, - SessionPane::Split { a, b, .. } => { - drop_duplicate_pane_ids(a, seen) + drop_duplicate_pane_ids(b, seen) - } - } -} - /// Helpers for every test that touches the on-disk `session.json`. The /// config-dir pin is process-wide (`set_config_dir` is first-call-wins), so /// the file is process-wide too — any test that reads or writes it must hold @@ -1276,72 +1082,6 @@ mod tests { ); } - /// The stale-instance check: ids recorded against a *different* daemon - /// process are blanked (they now name unrelated shells at best), ids - /// recorded against the *same* one are kept, and an unknown on either side - /// changes nothing — treating "cannot tell" as stale would respawn every - /// pane on the first launch after an upgrade. - #[test] - fn stale_instance_blanks_pane_ids_and_matching_or_unknown_keeps_them() { - let fresh = |instance: Option<&str>| { - let mut ws = workspace(vec![tab(leaf(Some("/work"), Some(7)), None)]); - ws.daemon_instance = instance.map(str::to_string); - ws - }; - - let mut ws = fresh(Some("daemon-a")); - assert_eq!(ws.forget_stale_pane_ids(Some("daemon-b")), 1); - assert!(ws.pane_ids().is_empty()); - assert_eq!( - ws.first_cwd(), - Some(PathBuf::from("/work")), - "the layout survives; only the claims go" - ); - - let mut ws = fresh(Some("daemon-a")); - assert_eq!(ws.forget_stale_pane_ids(Some("daemon-a")), 0); - assert_eq!(ws.pane_ids(), vec![7], "same process, ids stay attachable"); - - let mut ws = fresh(None); - assert_eq!(ws.forget_stale_pane_ids(Some("daemon-b")), 0); - assert_eq!(ws.pane_ids(), vec![7], "an old record is not judged"); - - let mut ws = fresh(Some("daemon-a")); - assert_eq!(ws.forget_stale_pane_ids(None), 0); - assert_eq!(ws.pane_ids(), vec![7], "an unknown daemon is not judged"); - } - - /// Unlike a duplicate claim, a stale-instance claim keeps its agent resume: - /// the daemon those panes lived in is gone, nothing else runs the - /// conversation, and the fresh shell resuming it is the feature working. - #[test] - fn stale_instance_keeps_the_agent_resume() { - let mut ws = workspace(vec![tab( - SessionPane::Leaf { - cwd: Some(PathBuf::from("/work")), - pane_id: Some(7), - ssh_spec: None, - agent: Some(crate::core::cli_agent::CLIAgent::Claude), - agent_session_id: Some("sid".into()), - agent_launch_argv: None, - }, - None, - )]); - ws.daemon_instance = Some("daemon-a".into()); - assert_eq!(ws.forget_stale_pane_ids(Some("daemon-b")), 1); - match &ws.session.tabs[0].pane { - SessionPane::Leaf { - pane_id, - agent_session_id, - .. - } => { - assert!(pane_id.is_none()); - assert_eq!(agent_session_id.as_deref(), Some("sid")); - } - SessionPane::Split { .. } => panic!("leaf stays a leaf"), - } - } - #[test] fn display_name_prefers_user_name_then_repo_then_cwd() { // No name, no repo group: fall back to the first leaf's directory. @@ -1398,181 +1138,6 @@ mod tests { assert_eq!(ws.first_cwd(), Some(PathBuf::from("/a"))); } - #[test] - fn dedupe_pane_ids_keeps_the_most_recently_active_claim() { - // Two workspaces both claim pane 5 — the crash/hand-edit case. The - // stale one must yield, or its window silently steals the live one's - // stream when both attach (the daemon has a single subscriber). - let mut stale = workspace(vec![tab(leaf(Some("/old"), Some(5)), None)]); - stale.last_active = 100; - let mut fresh = workspace(vec![tab(leaf(Some("/new"), Some(5)), None)]); - fresh.last_active = 200; - let (stale_id, fresh_id) = (stale.id, fresh.id); - - let mut all = Workspaces { - active: Some(fresh_id), - workspaces: vec![stale, fresh], - }; - assert_eq!(all.dedupe_pane_ids(), 1); - - // The recent one keeps pane 5; the stale one drops to a fresh spawn in - // its saved cwd (cwd is preserved — only the id is cleared). - assert_eq!(all.get(fresh_id).unwrap().pane_ids(), vec![5]); - assert!(all.get(stale_id).unwrap().pane_ids().is_empty()); - assert_eq!( - all.get(stale_id).unwrap().first_cwd(), - Some(PathBuf::from("/old")) - ); - } - - /// The duplicate claim loses its agent resume along with its pane id. - /// Restore's spawn-fresh path auto-types the agent's resume command, and - /// the winning workspace's pane is still *running* that agent — a loser - /// that kept `agent_session_id` would come back as a second process on - /// the same conversation (double `claude --resume `, both live). - #[test] - fn dedupe_pane_ids_disarms_the_duplicate_claims_agent_resume() { - let agent_leaf = |pane_id| SessionPane::Leaf { - cwd: Some(PathBuf::from("/work")), - pane_id: Some(pane_id), - ssh_spec: None, - agent: Some(crate::core::cli_agent::CLIAgent::Claude), - agent_session_id: Some("362f9261".into()), - agent_launch_argv: Some(vec!["claude".into(), "--continue".into()]), - }; - let mut stale = workspace(vec![tab(agent_leaf(5), None)]); - stale.last_active = 100; - let mut fresh = workspace(vec![tab(agent_leaf(5), None)]); - fresh.last_active = 200; - let (stale_id, fresh_id) = (stale.id, fresh.id); - - let mut all = Workspaces { - active: Some(fresh_id), - workspaces: vec![stale, fresh], - }; - assert_eq!(all.dedupe_pane_ids(), 1); - - let loser = &all.get(stale_id).unwrap().session.tabs[0].pane; - match loser { - SessionPane::Leaf { - pane_id, - cwd, - agent_session_id, - agent_launch_argv, - .. - } => { - assert!(pane_id.is_none()); - assert_eq!( - cwd.as_deref(), - Some(std::path::Path::new("/work")), - "the layout survives — only the claim and its resume go" - ); - assert!( - agent_session_id.is_none(), - "no second resume of one conversation" - ); - assert!(agent_launch_argv.is_none()); - } - SessionPane::Split { .. } => panic!("the leaf must survive as a leaf"), - } - - // The winner is untouched: its pane is the one actually running the agent. - match &all.get(fresh_id).unwrap().session.tabs[0].pane { - SessionPane::Leaf { - pane_id, - agent_session_id, - .. - } => { - assert_eq!(*pane_id, Some(5)); - assert_eq!(agent_session_id.as_deref(), Some("362f9261")); - } - SessionPane::Split { .. } => panic!("the leaf must survive as a leaf"), - } - } - - /// A pane id is only unique within one daemon, so the same number on two - /// machines is not a collision. Deduping globally would make the remote - /// workspace forfeit a claim on a pane that is alive on its own box — - /// orphaning a live session over a conflict that never existed. - #[test] - fn dedupe_pane_ids_is_scoped_to_one_machine() { - let mut local = workspace(vec![tab(leaf(Some("/local"), Some(1)), None)]); - local.last_active = 200; - let mut remote = workspace(vec![tab(leaf(Some("/remote"), Some(1)), None)]); - remote.last_active = 100; // older, so a global dedupe would drop *this* one - remote.host = Some(RemoteRef { - target: RemoteTarget::Alias { - alias: "build-box".into(), - }, - workspace: WorkspaceId::new(), - }); - let (local_id, remote_id) = (local.id, remote.id); - - let mut all = Workspaces { - active: Some(local_id), - workspaces: vec![local, remote], - }; - assert_eq!(all.dedupe_pane_ids(), 0, "different machines never collide"); - assert_eq!(all.get(local_id).unwrap().pane_ids(), vec![1]); - assert_eq!( - all.get(remote_id).unwrap().pane_ids(), - vec![1], - "the remote keeps its claim on its own daemon's pane 1" - ); - - // …and two workspaces on the *same* remote machine still dedupe. - let host = RemoteRef { - target: RemoteTarget::Alias { - alias: "build-box".into(), - }, - workspace: WorkspaceId::new(), - }; - let mut older = workspace(vec![tab(leaf(Some("/a"), Some(7)), None)]); - older.last_active = 100; - older.host = Some(host.clone()); - let mut newer = workspace(vec![tab(leaf(Some("/b"), Some(7)), None)]); - newer.last_active = 200; - newer.host = Some(host); - let (older_id, newer_id) = (older.id, newer.id); - - let mut same_box = Workspaces { - active: Some(newer_id), - workspaces: vec![older, newer], - }; - assert_eq!(same_box.dedupe_pane_ids(), 1); - assert_eq!(same_box.get(newer_id).unwrap().pane_ids(), vec![7]); - assert!(same_box.get(older_id).unwrap().pane_ids().is_empty()); - } - - #[test] - fn dedupe_pane_ids_is_a_noop_on_healthy_sessions() { - let mut all = Workspaces { - active: None, - workspaces: vec![ - workspace(vec![tab(leaf(Some("/a"), Some(1)), None)]), - workspace(vec![tab(leaf(Some("/b"), Some(2)), None)]), - ], - }; - assert_eq!(all.dedupe_pane_ids(), 0); - assert_eq!(all.workspaces[0].pane_ids(), vec![1]); - assert_eq!(all.workspaces[1].pane_ids(), vec![2]); - } - - #[test] - fn dedupe_pane_ids_catches_a_duplicate_within_one_workspace() { - // Same guarantee inside a single workspace: a split that somehow ended - // up with the same pane in both halves would deadlock the same way. - let mut all = Workspaces { - active: None, - workspaces: vec![workspace(vec![ - tab(leaf(Some("/a"), Some(1)), None), - tab(leaf(Some("/b"), Some(1)), None), - ])], - }; - assert_eq!(all.dedupe_pane_ids(), 1); - assert_eq!(all.workspaces[0].pane_ids(), vec![1]); - } - // ── Remote workspaces (M5) ────────────────────────────────────────────── /// A real-shaped `session.json` from before `host` existed, written by the @@ -1808,160 +1373,6 @@ mod tests { ); } - #[test] - fn a_remote_workspace_survives_a_restart() { - let remote_id = WorkspaceId::new(); - let mut ws = Workspace::on_remote(RemoteRef::new( - RemoteTarget::direct("me", "box.local", 2222), - remote_id, - )); - ws.name = Some("api".into()); - ws.open = false; - let all = Workspaces { - active: None, - workspaces: vec![ws], - }; - let text = serde_json::to_string(&all).unwrap(); - let back = Workspaces::decode(&text).unwrap(); - let only = &back.workspaces[0]; - assert!(only.is_remote()); - let host = only.host.as_ref().unwrap(); - assert_eq!(host.workspace, remote_id); - assert_eq!(host.target, RemoteTarget::direct("me", "box.local", 2222)); - assert_eq!(host.target.connection_key(), "ssh-direct:me@box.local:2222"); - } - - /// Every `Workspace` field belongs to exactly one side of the storage - /// split. A new field that is in neither list would be silently dropped by - /// `to_remote_json` and lost on the next pull, which is data loss that no - /// other test would notice. - #[test] - fn the_storage_split_covers_every_workspace_field() { - let mut ws = workspace(vec![tab(leaf(Some("/w"), Some(1)), Some("/w"))]); - ws.name = Some("named".into()); - ws.window = Some(crate::core::window_state::WindowState { - x: 0.0, - y: 0.0, - width: 800.0, - height: 600.0, - }); - ws.host = Some(RemoteRef::new( - RemoteTarget::Alias { - alias: "devbox".into(), - }, - WorkspaceId::new(), - )); - // Every skip-when-`None` field must be populated here, or it never - // serializes and this census can't see it. - ws.daemon_instance = Some("daemon-uuid".into()); - - let value = serde_json::to_value(&ws).unwrap(); - let mut present: Vec = value - .as_object() - .unwrap() - .keys() - .map(String::from) - .collect(); - present.sort(); - let mut expected: Vec = REMOTE_OWNED_FIELDS - .iter() - .chain(CLIENT_OWNED_FIELDS) - .map(|s| (*s).to_string()) - .collect(); - expected.sort(); - assert_eq!( - present, expected, - "a Workspace field is on neither side of the storage split; decide which \ - machine owns it and add it to REMOTE_OWNED_FIELDS or CLIENT_OWNED_FIELDS" - ); - } - - #[test] - fn the_remote_record_carries_the_layout_and_nothing_local() { - let mut ws = workspace(vec![tab(leaf(Some("/srv/app"), Some(7)), Some("/srv/app"))]); - ws.name = Some("app".into()); - ws.last_active = 1_753_600_000; - ws.open = true; - ws.window = Some(crate::core::window_state::WindowState { - x: 1.0, - y: 2.0, - width: 800.0, - height: 600.0, - }); - ws.host = Some(RemoteRef::new( - RemoteTarget::Alias { - alias: "devbox".into(), - }, - WorkspaceId::new(), - )); - - let record = ws.to_remote_json(); - let obj = record.as_object().unwrap(); - // The machine's facts go over. - assert!(obj.contains_key("session")); - assert_eq!(obj["name"], "app"); - assert_eq!(obj["last_active"], 1_753_600_000u64); - assert_eq!(obj["id"], ws.id.to_string()); - // This client's view state does not — the point of the split. - for k in CLIENT_OWNED_FIELDS { - assert!(!obj.contains_key(*k), "`{k}` must not leave this machine"); - } - - // Pulling it back onto a *different* client's entry updates the layout - // and leaves that client's own view state alone. - let mut mine = Workspace::on_remote(RemoteRef::new( - RemoteTarget::Alias { - alias: "devbox".into(), - }, - ws.id, - )); - mine.open = false; - mine.window = None; - let my_id = mine.id; - mine.apply_remote_json(&record).unwrap(); - assert_eq!(mine.session.tabs.len(), 1); - assert_eq!(mine.name.as_deref(), Some("app")); - assert_eq!(mine.last_active, 1_753_600_000); - assert_eq!( - mine.id, my_id, - "the client's own entry id is not overwritten" - ); - assert!( - !mine.open, - "the remote has no opinion about my open windows" - ); - assert!(mine.window.is_none()); - assert!(mine.is_remote(), "and it is still a remote workspace"); - } - - /// A record from a newer client carries fields this build has never seen, - /// and one from an older client is missing fields it expects. Neither may - /// fail the pull. - #[test] - fn applying_a_record_tolerates_version_skew() { - let mut ws = Workspace::default(); - ws.apply_remote_json(&serde_json::json!({ - "id": "6a8f2a1e-1c1b-4f7a-9d3e-2b5c8e4a7f01", - "session": {"active": 0, "tabs": []}, - "last_active": 5, - "something_from_2027": {"nested": true} - })) - .expect("unknown fields are ignored, not fatal"); - assert_eq!(ws.last_active, 5); - - let mut ws = Workspace { - name: Some("stale".into()), - ..Workspace::default() - }; - ws.apply_remote_json(&serde_json::json!({})) - .expect("a record missing every optional field still applies"); - assert_eq!( - ws.name, None, - "the remote's answer wins, including 'no name'" - ); - assert!(ws.session.tabs.is_empty()); - } - #[test] fn open_and_closed_partition_by_flag_and_recency() { let mut open_one = workspace(vec![]); diff --git a/crates/tty7-core/src/core/workspace_store.rs b/crates/tty7-core/src/core/workspace_store.rs index c4ad6f11..211681a3 100644 --- a/crates/tty7-core/src/core/workspace_store.rs +++ b/crates/tty7-core/src/core/workspace_store.rs @@ -870,7 +870,15 @@ mod tests { ws.name = Some("api".into()); ws.last_active = 1_753_600_000; let id = ws.id.to_string(); - store.put(&id, ws.to_remote_json(), None).unwrap(); + // The remote-owned half of a record, spelled inline: the helper that + // used to derive it is retired with the whole-record write path. + let record = serde_json::json!({ + "id": id, + "name": "api", + "session": { "active": 0, "tabs": [] }, + "last_active": 1_753_600_000u64, + }); + store.put(&id, record, None).unwrap(); let text = std::fs::read_to_string(dir.path().join(STORE_FILE)).unwrap(); let parsed = Workspaces::decode(&text).expect("the store's file is a Workspaces document"); diff --git a/src/core/session.rs b/src/core/session.rs index 663a3b21..91f0bcb2 100644 --- a/src/core/session.rs +++ b/src/core/session.rs @@ -30,14 +30,7 @@ impl WorkspaceStore { /// duplicate pane claims, and install the result as the app global. Call /// once, before the first window is built. pub fn init(cx: &mut gpui::App) { - let mut workspaces = Workspaces::load().unwrap_or_default(); - let dropped = workspaces.dedupe_pane_ids(); - if dropped > 0 { - log::warn!( - "session.json claimed {dropped} pane(s) from more than one workspace; \ - the stale claims will spawn fresh shells instead" - ); - } + let workspaces = Workspaces::load().unwrap_or_default(); cx.set_global(Self { workspaces }); } @@ -84,7 +77,6 @@ impl WorkspaceStore { // 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 instance = Self::serving_instance(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. @@ -100,10 +92,7 @@ impl WorkspaceStore { }; workspace.open = true; workspace.touch(); - let claimed = ( - workspace.id, - claimable_session(workspace, reachable, instance.as_deref()), - ); + let claimed = (workspace.id, claimable_session(workspace, reachable)); store.workspaces.active = Some(claimed.0); store.workspaces.save(); claimed @@ -122,7 +111,6 @@ impl WorkspaceStore { // 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 instance = Self::serving_instance(cx, Some(id)); let Some(store) = Self::try_store(cx) else { return; }; @@ -131,7 +119,7 @@ impl WorkspaceStore { // tearing down); nothing to record. return; }; - record_session(workspace, session, reachable, instance); + record_session(workspace, session, reachable); if let Some(window) = window { workspace.window = Some(window); } @@ -297,60 +285,6 @@ impl WorkspaceStore { crate::ui::remote_connect::RemoteConnections::get(cx, host.host_id()).is_some() } - /// The process whose pane ids this workspace's record is about: this - /// machine's daemon for a local workspace, the far machine's `tty7-server` - /// for a remote one. `None` when it cannot be named — an older peer, a - /// machine not connected right now, or a brand-new workspace with no host - /// yet — which every reader treats as "no instance check possible". - /// - /// One function for both because [`Workspace::daemon_instance`] means the - /// same thing on both sides. It used to be local-only, on the reasoning - /// that a remote server's identity is tracked live per connection instead - /// — but that live map lives in memory, so it is empty on the launch that - /// matters most: the one where the client was closed while the remote - /// server was replaced. - pub fn serving_instance(cx: &mut gpui::App, id: Option) -> Option { - match id.and_then(|id| Self::remote_ref(cx, id)) { - Some(host) => crate::ui::remote_connect::RemoteConnections::get(cx, host.host_id()) - .map(|h| h.peer().instance.clone()) - .filter(|instance| !instance.is_empty()), - None => crate::daemon::spawn::local_daemon_instance(), - } - } - - /// Blank `id`'s saved pane ids when they were recorded against a different - /// server process than `instance`, and persist that. Answers whether any - /// were dropped. - /// - /// The remote counterpart of the check [`claimable_session`] runs for a - /// local workspace at claim time. It cannot run there for a remote one: at - /// claim time the machine is usually not connected yet, so there is no - /// instance to compare against. The reconnect is the first moment the - /// answer exists, which is where this is called from. - pub fn forget_stale_pane_ids(cx: &mut gpui::App, id: WorkspaceId, instance: &str) -> bool { - let current = (!instance.is_empty()).then_some(instance); - let Some(store) = Self::try_store(cx) else { - return false; - }; - let Some(workspace) = store.workspaces.get_mut(id) else { - return false; - }; - let dropped = workspace.forget_stale_pane_ids(current); - if dropped == 0 { - return false; - } - // Stamped now rather than left for the next save: the record has just - // been made to describe *this* server, and a crash before the window - // saves must not leave it claiming the old process again. - workspace.daemon_instance = current.map(str::to_string); - store.workspaces.save(); - log::info!( - "workspace {id}: {dropped} saved pane id(s) belong to a previous \ - tty7-server process; rebuilding from the layout" - ); - true - } - /// The client-side entry for `host` — the existing one if this machine has /// seen that workspace before, a fresh one otherwise. /// @@ -387,47 +321,6 @@ impl WorkspaceStore { store.workspaces.save(); id } - - /// Merge an authoritative record pulled from the remote into the client's - /// entry. Only the remote-owned fields move; `open`, `window` and `host` - /// stay as this machine left them (see [`Workspace::apply_remote_json`]). - /// - /// A record that will not decode is dropped with a log line rather than - /// failing the open: the layout is recoverable on the next push, an - /// unopenable workspace is not. - pub fn apply_remote(cx: &mut gpui::App, id: WorkspaceId, record: &serde_json::Value) { - let Some(store) = Self::try_store(cx) else { - return; - }; - let Some(workspace) = store.workspaces.get_mut(id) else { - return; - }; - if let Err(e) = workspace.apply_remote_json(record) { - log::warn!("remote workspace {id} sent a record this build cannot read: {e}"); - return; - } - store.workspaces.save(); - } - - /// What to send the remote for `id`: its store key and the remote-owned half - /// of the record. `None` for a local workspace — there is nobody to send to. - pub fn remote_payload( - cx: &gpui::App, - id: WorkspaceId, - ) -> Option<(RemoteRef, String, serde_json::Value)> { - let workspace = Self::all(cx).get(id)?; - let host = workspace.host.clone()?; - let key = host.store_key(); - // The record travels under the *remote's* id, not the client entry's: - // the remote store is keyed by its own ids, and a record whose `id` - // disagreed with its key would be a workspace that renames itself on - // every round trip. - let mut record = workspace.to_remote_json(); - if let Some(obj) = record.as_object_mut() { - obj.insert("id".to_string(), serde_json::json!(key)); - } - Some((host, key, record)) - } } /// The machine a window showing `id` is bound to. @@ -456,57 +349,21 @@ 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`]. +/// of [`record_session`], and by now only the *fallback* source: the machine's +/// tree is the layout authority, and the hydration that follows a claim is +/// what actually fills the window. What this still decides is the shape the +/// window opens in. /// -/// 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. -/// -/// `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. -/// `current_instance` is the identity of the process serving this workspace's -/// panes (see [`WorkspaceStore::serving_instance`]). A workspace whose saved ids -/// were recorded against a different one blanks them first — after a restart the -/// numbers begin again at 1, so a stale id would otherwise pass the aliveness -/// check by landing on whatever unrelated pane holds it now. Blanked in the -/// stored entry too, not just the returned copy, so the record stops claiming -/// panes that no longer exist even if the window never saves again. -/// -/// A remote workspace usually reaches the early return above instead: at claim -/// time its machine is not connected yet, so there is no instance to compare and -/// no layout to hand back. `remote_workspace::finish_attempt` runs the same -/// check the moment the connect answers, which is the first point it can. -fn claimable_session( - workspace: &mut Workspace, - reachable: bool, - current_instance: Option<&str>, -) -> Session { - // A remote workspace *always* opens empty now, reachable or not: its - // machine's tree is the layout authority, and the hydration that follows - // the claim (`tree_sync::hydrate_window_from_tree`) is what fills the - // window — from the tree, not from this client's cache. +/// A remote workspace opens empty unconditionally — its layout lives on +/// another machine, and this client's cached copy is an import fallback, not +/// something to build panes from. A local one hands back the cached layout for +/// the paths that deliberately skip hydration (restore off, and the hydration +/// import itself). +fn claimable_session(workspace: &mut Workspace, reachable: bool) -> Session { let _ = reachable; if workspace.is_remote() { return Session::default(); } - let dropped = workspace.forget_stale_pane_ids(current_instance); - if dropped > 0 { - log::info!( - "workspace {}: {dropped} saved pane id(s) belong to a previous serving \ - process; restoring with fresh shells (and agent resume where recorded)", - workspace.id - ); - } workspace.session.clone() } @@ -515,29 +372,14 @@ fn 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. -/// The record is stamped with the process its pane ids came from (`instance`): -/// this machine's daemon for a local workspace, the far machine's -/// `tty7-server` for a remote one. That is what lets the next launch tell a -/// surviving process from a replaced one — see [`claimable_session`] and -/// [`WorkspaceStore::forget_stale_pane_ids`]. -/// -/// The unreachable early return doubles as the guard on that stamp: with the -/// machine down there is no instance to record, and writing `None` over a good -/// one would throw away the very comparison the next connect needs. -fn record_session( - workspace: &mut Workspace, - session: Session, - reachable: bool, - instance: Option, -) { +/// it records nothing rather than replacing the copy we have with the +/// wreckage. The machine's tree is the authority; this entry is the cache the +/// hydration import falls back to. +fn record_session(workspace: &mut Workspace, session: Session, reachable: bool) { if workspace.is_remote() && !reachable { return; } workspace.session = session; - workspace.daemon_instance = instance; } #[cfg(test)] @@ -580,7 +422,7 @@ mod tests { #[test] fn a_local_workspace_stores_its_own_layout() { let mut workspace = Workspace::default(); - record_session(&mut workspace, local_layout(), true, None); + record_session(&mut workspace, local_layout(), true); assert_eq!(workspace.session.tabs.len(), 1); assert_eq!(workspace.pane_ids(), vec![7]); } @@ -592,7 +434,7 @@ mod tests { #[test] fn a_connected_remote_workspace_stores_its_layout() { let mut workspace = Workspace::on_remote(remote_ref()); - record_session(&mut workspace, local_layout(), true, None); + record_session(&mut workspace, local_layout(), true); assert_eq!(workspace.session.tabs.len(), 1); assert_eq!( workspace.pane_ids(), @@ -601,35 +443,6 @@ mod tests { ); } - /// A remote workspace's record is stamped with the **server's** instance, - /// not left blank. That stamp is the only part of "which process minted - /// these ids" that survives the client being closed, and it is what the - /// next connect compares against before re-attaching anything. - #[test] - fn a_connected_remote_workspace_records_the_serving_instance() { - let mut workspace = Workspace::on_remote(remote_ref()); - record_session( - &mut workspace, - local_layout(), - true, - Some("server-a".to_string()), - ); - assert_eq!(workspace.daemon_instance.as_deref(), Some("server-a")); - } - - /// …and an unreachable machine does not un-stamp it. `None` there means - /// "nobody to ask", and writing it over a good value would disarm the very - /// check the next connect needs — the ids would look current again. - #[test] - fn an_unreachable_remote_window_does_not_erase_the_recorded_instance() { - let mut workspace = Workspace::on_remote(remote_ref()); - workspace.session = local_layout(); - workspace.daemon_instance = Some("server-a".to_string()); - record_session(&mut workspace, Session::default(), false, None); - assert_eq!(workspace.daemon_instance.as_deref(), Some("server-a")); - assert_eq!(workspace.session.tabs.len(), 1, "and the layout stays too"); - } - /// A local workspace opens on the layout it saved. #[test] fn a_local_workspace_reopens_its_saved_layout() { @@ -637,48 +450,12 @@ mod tests { session: local_layout(), ..Workspace::default() }; - let claimed = claimable_session(&mut workspace, true, None); + 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); } - /// Claiming a local workspace whose ids were recorded against a *different* - /// daemon process blanks them — in the returned session **and** in the - /// stored entry. After a reboot the numbers restart from 1, so a stale id - /// passes the aliveness check by landing on whatever unrelated pane holds - /// it now; blanking is what turns that into an honest fresh spawn (with - /// the agent resume the leaf recorded). - #[test] - fn claiming_a_local_workspace_from_another_daemon_process_blanks_its_ids() { - let mut workspace = Workspace { - session: local_layout(), - daemon_instance: Some("previous-boot".into()), - ..Workspace::default() - }; - let leaf_id = |session: &Session| match &session.tabs[0].pane { - SessionPane::Leaf { pane_id, .. } => *pane_id, - SessionPane::Split { .. } => panic!("the fixture is a single leaf"), - }; - let claimed = claimable_session(&mut workspace, true, Some("current-boot")); - assert_eq!(claimed.tabs.len(), 1, "the layout still restores"); - assert_eq!( - leaf_id(&claimed), - None, - "but no leaf may attach by a number from a dead daemon" - ); - assert!(workspace.pane_ids().is_empty(), "the entry agrees"); - - // Same process → the ids stay attachable. - let mut workspace = Workspace { - session: local_layout(), - daemon_instance: Some("current-boot".into()), - ..Workspace::default() - }; - let claimed = claimable_session(&mut workspace, true, Some("current-boot")); - assert_eq!(leaf_id(&claimed), Some(7)); - } - /// A remote workspace opens empty even when its machine is connected: the /// machine's tree is the layout authority now, and the hydration that /// follows the claim fills the window from it. The cached copy stays — @@ -687,7 +464,7 @@ mod tests { fn a_connected_remote_workspace_still_opens_empty_for_the_tree_to_fill() { let mut workspace = Workspace::on_remote(remote_ref()); workspace.session = local_layout(); - let claimed = claimable_session(&mut workspace, true, None); + let claimed = claimable_session(&mut workspace, true); assert!(claimed.tabs.is_empty()); assert_eq!(workspace.session.tabs.len(), 1, "the cache is kept"); } @@ -702,7 +479,7 @@ mod tests { let mut workspace = Workspace::on_remote(remote_ref()); workspace.session = local_layout(); - let claimed = claimable_session(&mut workspace, false, None); + let claimed = claimable_session(&mut workspace, false); assert!(claimed.tabs.is_empty(), "the window must open with no tabs"); assert_eq!( workspace.session.tabs.len(), @@ -718,7 +495,7 @@ mod tests { 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(), false, None); + record_session(&mut workspace, Session::default(), false); assert_eq!(workspace.session.tabs.len(), 1); } @@ -806,108 +583,6 @@ mod tests { }); } - /// **The cold-launch half of the restart check.** `RemoteLinks::instances` - /// is in memory, so on the first connect after the client starts every - /// machine is a first sighting and nothing is judged a restart. A server - /// replaced while the client was closed would therefore sail through, and - /// its recycled ids — daemons number panes from 1 — would attach to - /// whatever unrelated shells hold those numbers now. The stamp on the - /// record is what closes that, so this is the test that has to hold. - #[gpui::test] - fn a_remote_workspace_drops_pane_ids_minted_by_a_previous_server( - cx: &mut gpui::TestAppContext, - ) { - cx.update(|cx| { - crate::core::config::pin_test_config_dir(); - - let mut entry = Workspace::on_remote(remote_ref()); - entry.session = local_layout(); - entry.daemon_instance = Some("server-a".to_string()); - let id = entry.id; - WorkspaceStore::install_for_test( - cx, - Workspaces { - workspaces: vec![entry], - active: None, - }, - ); - - // Same process: these ids still name the panes they always did. - assert!(!WorkspaceStore::forget_stale_pane_ids(cx, id, "server-a")); - assert_eq!(WorkspaceStore::all(cx).get(id).unwrap().pane_ids(), vec![7]); - - // An unknown instance is never judged — a peer too old to report - // one must not cost the user every pane on the machine. - assert!(!WorkspaceStore::forget_stale_pane_ids(cx, id, "")); - assert_eq!(WorkspaceStore::all(cx).get(id).unwrap().pane_ids(), vec![7]); - - // Replaced: the claims go, the layout stays, and the stamp moves on. - assert!(WorkspaceStore::forget_stale_pane_ids(cx, id, "server-b")); - let after = WorkspaceStore::all(cx).get(id).unwrap(); - assert!(after.pane_ids().is_empty()); - assert_eq!( - after.session.tabs.len(), - 1, - "the layout is exactly what the rebuild draws from" - ); - assert_eq!( - after.daemon_instance.as_deref(), - Some("server-b"), - "stamped now, so a crash before the next save cannot re-arm the old claim" - ); - - // And the same server is not a restart twice over. - assert!(!WorkspaceStore::forget_stale_pane_ids(cx, id, "server-b")); - }); - } - - /// **Why clearing the ids locally is not enough.** The remote owns the - /// record, so reopening pulls its copy over the client's — and - /// a copy that still claims the killed panes puts them straight back. This - /// is the constraint `windows::forget_killed_panes` pushes to satisfy; if - /// this assertion ever flips, that push is dead weight. - #[test] - fn a_remote_record_reinstates_pane_ids_a_client_only_clear_dropped() { - let mut theirs = Workspace::on_remote(remote_ref()); - theirs.session = local_layout(); - let record = theirs.to_remote_json(); - - let mut ours = Workspace::on_remote(remote_ref()); - ours.session = local_layout(); - ours.forget_pane_ids(); - assert!(ours.pane_ids().is_empty()); - - ours.apply_remote_json(&record).unwrap(); - assert_eq!( - ours.pane_ids(), - vec![7], - "the machine's copy wins, so the clear has to reach it" - ); - } - - /// The remote-bound payload travels under the *remote's* id, so a record - /// pushed and pulled back names the same workspace both times. - #[test] - fn the_remote_payload_is_keyed_by_the_remote_id_not_the_client_entry() { - let host = remote_ref(); - let workspace = Workspace::on_remote(host.clone()); - let mut record = workspace.to_remote_json(); - record - .as_object_mut() - .unwrap() - .insert("id".into(), serde_json::json!(host.store_key())); - assert_eq!(host.store_key(), host.workspace.to_string()); - assert_ne!(host.store_key(), workspace.id.to_string()); - assert_eq!(record["id"], serde_json::json!(host.store_key())); - // The client-owned half never crosses. - for client_only in tty7_core::core::session::CLIENT_OWNED_FIELDS { - assert!( - record.get(*client_only).is_none(), - "{client_only} must not be sent to the remote" - ); - } - } - /// **The window/host invariant, as a test.** /// /// A window is one machine. The inverse is listed under diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index 7e6585dc..0316f0d2 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -1260,7 +1260,7 @@ mod tests { /// name derived the way a local workspace's would be when none is set. #[test] fn rows_from_the_tree_sort_newest_first_and_derive_names() { - use tty7_core::core::machine::{Machine, PaneNode, PaneRecord, Tab, Workspace}; + use tty7_core::core::machine::{Machine, PaneRecord, Tab, Workspace}; let older = WorkspaceId::new(); let newer = WorkspaceId::new(); let machine = Machine {