From d62628be8de1f04a0e443e8943063a7c8786a66b Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Mon, 15 Jun 2026 16:46:04 +0700 Subject: [PATCH] fix(daemon): reseed id counter on restore + heal duplicate leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the multi-focus/multi-search/linked-terminal bug: the in-memory id counter resets to 0 each daemon start, but restore() never advanced it past restored ids. After a restart new_surface_id() re-minted existing ids → the same surface_id appeared twice in a layout tree (rendered as two panels sharing focus, search bar, and output channel — one ends up blank). Session-persistence made restarts routine, surfacing the latent bug. - restore() now reseeds the counter to max(restored id)+1 - ops::dedupe_leaves heals an already-corrupted persisted tree on load Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/spacesh-core/src/ops.rs | 47 +++++++++++++++++++++ crates/spaceshd/src/registry.rs | 73 +++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/crates/spacesh-core/src/ops.rs b/crates/spacesh-core/src/ops.rs index df23ff4..1e378b6 100644 --- a/crates/spacesh-core/src/ops.rs +++ b/crates/spacesh-core/src/ops.rs @@ -72,6 +72,32 @@ pub fn remove_leaf(root: LayoutNode, target: &SurfaceId) -> Option { } } +/// Drop duplicate leaves, keeping the first (left-to-right) occurrence of each +/// surface id; collapses now-single-child splits. Returns None if empty. +/// +/// Heals a tree corrupted by a duplicate surface id (e.g. an id re-minted after +/// a daemon restart before the counter fix), which otherwise renders the same +/// panel twice and confuses focus/search/output routing keyed by surface id. +pub fn dedupe_leaves(root: LayoutNode) -> Option { + let mut seen = std::collections::HashSet::new(); + dedupe(root, &mut seen) +} +fn dedupe(node: LayoutNode, seen: &mut std::collections::HashSet) -> Option { + match node { + LayoutNode::Leaf { surface_id } => { + if seen.insert(surface_id.clone()) { Some(LayoutNode::Leaf { surface_id }) } else { None } + } + LayoutNode::Split { orient, children, .. } => { + let kept: Vec = children.into_iter().filter_map(|c| dedupe(c, seen)).collect(); + match kept.len() { + 0 => None, + 1 => Some(kept.into_iter().next().unwrap()), + n => Some(LayoutNode::Split { orient, ratios: even(n), children: kept }), + } + } + } +} + /// Set ratios on the split node addressed by `path` (child indices from root). /// Normalizes to sum 1.0 and clamps each to >= MIN_RATIO. Returns false if the /// path is invalid or the length does not match the node's child count. @@ -247,6 +273,27 @@ mod tests { assert_eq!(leaves(&after), vec![sid("s_2"), sid("s_1")]); } + #[test] + fn dedupe_removes_duplicate_leaf_keeping_first() { + // s_1 appears twice (the production corruption): heal to one occurrence. + let root = LayoutNode::Split { + orient: Orient::H, ratios: vec![1.0/3.0; 3], + children: vec![LayoutNode::leaf(sid("s_0")), LayoutNode::leaf(sid("s_1")), LayoutNode::leaf(sid("s_1"))], + }; + let healed = dedupe_leaves(root).unwrap(); + assert_eq!(leaves(&healed), vec![sid("s_0"), sid("s_1")]); + } + + #[test] + fn dedupe_clean_tree_is_unchanged() { + let root = LayoutNode::Split { + orient: Orient::H, ratios: vec![0.5, 0.5], + children: vec![LayoutNode::leaf(sid("s_0")), LayoutNode::leaf(sid("s_1"))], + }; + let out = dedupe_leaves(root.clone()).unwrap(); + assert_eq!(leaves(&out), vec![sid("s_0"), sid("s_1")]); + } + #[test] fn move_onto_self_is_noop() { let root = LayoutNode::leaf(sid("s_1")); diff --git a/crates/spaceshd/src/registry.rs b/crates/spaceshd/src/registry.rs index 3bd5480..52354ec 100644 --- a/crates/spaceshd/src/registry.rs +++ b/crates/spaceshd/src/registry.rs @@ -23,6 +23,12 @@ pub struct Registry { states: HashMap, } +/// Parse the hex numeric suffix of an id (`"s_1f"` → `0x1f`). None if malformed. +/// All ids are minted as `format!("{prefix}_{n:x}")`, so the suffix is hex. +fn id_num(id: &str) -> Option { + id.rsplit_once('_').and_then(|(_, hex)| u64::from_str_radix(hex, 16).ok()) +} + impl Registry { pub fn new() -> Self { Self::default() @@ -193,8 +199,29 @@ impl Registry { self.by_path.clear(); self.live.clear(); self.states.clear(); + + // Advance the id counter past every restored id. The in-memory counter + // resets to 0 on each daemon start; without this reseed, after a restart + // `new_surface_id()` re-mints ids that already exist — producing duplicate + // leaves in a workspace tree (same panel rendered twice, focus/search/ + // output routing keyed by surface id all break) and cross-workspace id + // collisions. + let mut max_id = 0u64; + for gid in self.groups.keys() { + if let Some(n) = id_num(&gid.0) { max_id = max_id.max(n + 1); } + } + for w in &state.workspaces { + if let Some(n) = id_num(&w.id.0) { max_id = max_id.max(n + 1); } + for sid in w.surfaces.keys() { + if let Some(n) = id_num(&sid.0) { max_id = max_id.max(n + 1); } + } + } + self.counter.store(max_id, Ordering::Relaxed); + for w in state.workspaces { let mut w = w; + // Heal a tree already corrupted by a duplicate leaf (pre-fix state). + w.layout = w.layout.take().and_then(spacesh_core::ops::dedupe_leaves); if let Some(z) = &w.zoomed { if !w.surfaces.contains_key(z) { w.zoomed = None; } } @@ -252,6 +279,52 @@ mod tests { assert_eq!(w.layout, Some(LN::leaf(s1))); // split collapsed } + #[test] + fn restore_advances_counter_past_existing_ids() { + // After a daemon restart the counter must not re-mint a restored id. + let mut r = Registry::new(); + let mut surfaces = HashMap::new(); + surfaces.insert(SurfaceId("s_5".into()), spec()); + let st = PersistState { + version: 1, groups: vec![], + workspaces: vec![Workspace { + id: WorkspaceId("w_2".into()), path: "/p".into(), name: "p".into(), + group_id: None, order: 0, unread: false, pinned: false, + layout: Some(LN::leaf(SurfaceId("s_5".into()))), zoomed: None, surfaces, + }], + }; + r.restore(st); + // max restored id is s_5 (hex 5) → next minted must be s_6, no collision. + assert_eq!(r.new_surface_id(), SurfaceId("s_6".into())); + } + + #[test] + fn restore_heals_duplicate_leaf() { + // A persisted tree with s_1 twice (the production corruption) heals to one. + let mut r = Registry::new(); + let mut surfaces = HashMap::new(); + surfaces.insert(SurfaceId("s_0".into()), spec()); + surfaces.insert(SurfaceId("s_1".into()), spec()); + let tree = LN::Split { + orient: Orient::H, ratios: vec![1.0 / 3.0; 3], + children: vec![LN::leaf(SurfaceId("s_0".into())), LN::leaf(SurfaceId("s_1".into())), LN::leaf(SurfaceId("s_1".into()))], + }; + let st = PersistState { + version: 1, groups: vec![], + workspaces: vec![Workspace { + id: WorkspaceId("w_0".into()), path: "/p".into(), name: "p".into(), + group_id: None, order: 0, unread: false, pinned: false, + layout: Some(tree), zoomed: None, surfaces, + }], + }; + r.restore(st); + let w = r.workspace(&WorkspaceId("w_0".into())).unwrap(); + assert_eq!( + spacesh_core::ops::leaves(w.layout.as_ref().unwrap()), + vec![SurfaceId("s_0".into()), SurfaceId("s_1".into())] + ); + } + #[test] fn restore_round_trips_through_persist_state() { let mut r = Registry::new();