From 324e1d15438a794b45409d8d96cf5b9e91c690e4 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:49:22 +0800 Subject: [PATCH] fix(core): review hardening for the machine-tree foundation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from a correctness review of the new daemon-owned tree, applied together: - A dead pane can no longer be resurrected in the tree by its own last output. On Windows the exit monitor reports the death while the reader is still draining ConPTY's buffered bytes, and the death report is latched; the reader's 'output is proof of life' publish now asserts liveness only while the pane state still says alive. - Delta delivery is ordered. Mutations were serialized by the state lock but delivered after releasing it, so one writer's deltas could overtake another's and leave every mirroring client on the losing state with no cue to re-pull. A notify-order mutex now spans each mutation and its own fan-out; cheap, because subscriber callbacks are enqueue-only by contract. - Implicit active-tab changes broadcast. tab_create's activation and the close paths' heal now emit ActiveTabChanged, so a client applying deltas never re-implements the server's heal rule; the one inexpressible case (no tabs) needs no delta because it is a fact, not surgery. - The coarse agent status no longer drives disk writes: it flips per hook event and is display-only, so it is outside the changed-facts gate and merely rides along when a load-bearing fact changes. - control_services reports which stores it serves on stderr again — tty7-server configures no log sink, and 'no machine tree' was invisible exactly where it matters, on a headless box. - The local link's first connect attempt is immediate instead of one backoff step late; the observation-slot test withdraws its store so it cannot swallow later tests' observations; and locked()'s poison rationale now says what is actually guaranteed. --- crates/tty7-core/src/core/machine.rs | 116 ++++++++++++++++++----- crates/tty7-core/src/daemon/pane.rs | 40 +++++++- crates/tty7-core/src/daemon/server.rs | 12 ++- crates/tty7-server/tests/machine_tree.rs | 8 +- src/ui/local_link.rs | 4 + 5 files changed, 147 insertions(+), 33 deletions(-) diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index 1aec2d27..d36252bf 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -487,6 +487,12 @@ pub enum LayoutDelta { WorkspaceTouched { last_active: u64, }, + /// Which tab is active changed — by an explicit set, by a created tab + /// becoming active, or by the close paths healing a dangling active id. + /// Emitted for every *implicit* change too, so a mirroring client never + /// has to re-implement the server's heal rule; the one inexpressible case + /// (a workspace losing its last tab has no active tab) needs no delta, + /// because "no tabs → no active tab" is a fact, not surgery. ActiveTabChanged { tab: TabId, }, @@ -568,6 +574,14 @@ impl Drop for Subscription { pub struct MachineStore { path: PathBuf, state: Mutex, + /// Serializes each mutation *with its own delivery*. The state lock alone + /// orders the mutations, but deltas are delivered after it is released — + /// without this, writer B's deltas could overtake writer A's and every + /// subscriber would apply the store's history in the wrong order, ending + /// on the losing state with no error to trigger a re-pull. Cheap to hold + /// across delivery because a subscriber's callback is enqueue-only by + /// contract. Always taken before `state`, never inside it. + notify_order: Mutex<()>, subscribers: Mutex>, next_subscriber: AtomicU64, } @@ -595,6 +609,7 @@ impl MachineStore { Arc::new(MachineStore { path, state: Mutex::new(machine), + notify_order: Mutex::new(()), subscribers: Mutex::new(Vec::new()), next_subscriber: AtomicU64::new(1), }) @@ -748,9 +763,13 @@ impl MachineStore { let at = at.unwrap_or(ws.tabs.len()).min(ws.tabs.len()); ws.tabs.insert(at, tab.clone()); ws.active_tab = Some(tab.id); + let active = tab.id; Ok(( tab.clone(), - vec![(workspace, LayoutDelta::TabCreated { at, tab })], + vec![ + (workspace, LayoutDelta::TabCreated { at, tab }), + (workspace, LayoutDelta::ActiveTabChanged { tab: active }), + ], )) }) } @@ -771,10 +790,13 @@ impl MachineStore { .position(|t| t.id == tab) .ok_or_else(|| not_found(format!("workspace {workspace} has no tab {tab}")))?; ws.tabs.remove(index); - heal_active_tab(ws, index); + let mut deltas = vec![(workspace, LayoutDelta::TabClosed { tab })]; + if let Some(active) = heal_active_tab(ws, index) { + deltas.push((workspace, LayoutDelta::ActiveTabChanged { tab: active })); + } let orphans = collect_orphan_panes(m); m.panes.retain(|p| !orphans.contains(&p.id)); - Ok((orphans, vec![(workspace, LayoutDelta::TabClosed { tab })])) + Ok((orphans, deltas)) }) } @@ -888,22 +910,28 @@ impl MachineStore { .iter() .position(|t| t.root.contains(pane)) .ok_or_else(|| not_found(format!("workspace {workspace} has no pane {pane}")))?; - let delta = match ws.tabs[index].root.remove_leaf(pane) { + let mut deltas = Vec::new(); + match ws.tabs[index].root.remove_leaf(pane) { // The tab was that one leaf: the tab goes. None => { let closed = ws.tabs.remove(index); - heal_active_tab(ws, index); - LayoutDelta::TabClosed { tab: closed.id } + deltas.push((workspace, LayoutDelta::TabClosed { tab: closed.id })); + if let Some(active) = heal_active_tab(ws, index) { + deltas.push((workspace, LayoutDelta::ActiveTabChanged { tab: active })); + } } - Some(true) => LayoutDelta::TabRestructured { - tab: ws.tabs[index].clone(), - pane: None, - }, + Some(true) => deltas.push(( + workspace, + LayoutDelta::TabRestructured { + tab: ws.tabs[index].clone(), + pane: None, + }, + )), Some(false) => unreachable!("the tab was chosen because it contains the pane"), }; let orphans = collect_orphan_panes(m); m.panes.retain(|p| !orphans.contains(&p.id)); - Ok((orphans, vec![(workspace, delta)])) + Ok((orphans, deltas)) }) } @@ -971,8 +999,10 @@ impl MachineStore { return Err(refuse("a pane cannot be moved next to itself".to_string())); } let closed = ws.tabs.remove(from); - heal_active_tab(ws, from); deltas.push((workspace, LayoutDelta::TabClosed { tab: closed.id })); + if let Some(active) = heal_active_tab(ws, from) { + deltas.push((workspace, LayoutDelta::ActiveTabChanged { tab: active })); + } } Some(true) => { deltas.push(( @@ -1142,23 +1172,30 @@ impl MachineStore { // ----- internals ------------------------------------------------------- fn locked(&self) -> std::sync::MutexGuard<'_, Machine> { - // A poisoned lock means a panic mid-mutation, but every mutation is - // rolled back on failure before the lock is released, so the state is - // still a valid tree; carrying on beats taking the daemon down. + // A poisoned lock means a panic mid-mutation. Every *fallible* path + // rolls back before releasing the lock (see `mutate`); the only + // panics inside an op are `unreachable!`/`expect`s on invariants the + // same op just established, so a poisoned tree is still the pre- or + // post-images of some operation. Carrying on beats taking the daemon + // — and every pane on the machine — down with a bookkeeping panic. self.state.lock().unwrap_or_else(|e| e.into_inner()) } /// Run one operation: mutate under the lock, persist, and — only if the - /// disk said yes — deliver the deltas outside the lock. + /// disk said yes — deliver the deltas outside the state lock. /// /// A failed persist rolls the tree back to the pre-mutation clone, so the /// in-memory state never claims something the file does not, and a change /// nobody can re-read is a change nobody is told about. + /// + /// `notify_order` is held across the whole thing — see the field — so + /// subscribers receive deltas in exactly the order the mutations landed. fn mutate( &self, origin: Option, op: impl FnOnce(&mut Machine) -> io::Result<(T, Vec<(WorkspaceId, LayoutDelta)>)>, ) -> io::Result { + let _order = self.notify_order.lock().unwrap_or_else(|e| e.into_inner()); let deltas; let value; { @@ -1236,18 +1273,21 @@ fn find_tab(m: &mut Machine, workspace: WorkspaceId, tab: TabId) -> io::Result<& /// /// The replacement is the neighbour that slid into the removed tab's place /// (or the new last tab), which is what every tab strip does on close. -fn heal_active_tab(ws: &mut Workspace, removed: usize) { +/// +/// Answers the tab that became active when the heal actually re-pointed it, +/// so the caller can broadcast the change — a client mirroring by deltas must +/// not have to re-implement this rule (see [`LayoutDelta::ActiveTabChanged`]). +fn heal_active_tab(ws: &mut Workspace, removed: usize) -> Option { let named = ws .active_tab .is_some_and(|active| ws.tabs.iter().any(|t| t.id == active)); - if named { - return; + if named || ws.tabs.is_empty() { + ws.active_tab = ws.active_tab.filter(|_| named); + return None; } - ws.active_tab = if ws.tabs.is_empty() { - None - } else { - Some(ws.tabs[removed.min(ws.tabs.len() - 1)].id) - }; + let active = ws.tabs[removed.min(ws.tabs.len() - 1)].id; + ws.active_tab = Some(active); + Some(active) } /// Adopt a seed into the registry. @@ -1368,6 +1408,13 @@ pub fn observe_pane(pane: u64, f: impl FnOnce(&mut PaneRecord)) { } } +/// Test-only: clear the slot again, so one test's store cannot swallow the +/// observations of unrelated tests running later in the same binary. +#[cfg(test)] +pub(crate) fn withdraw_observations() { + *OBSERVED.lock().unwrap_or_else(|e| e.into_inner()) = None; +} + /// Copy a file we are about to stop honouring somewhere the user can find it. fn quarantine(path: &Path) { let aside = path.with_extension("json.corrupt"); @@ -1606,6 +1653,7 @@ mod tests { let second = store.tab_create(ws, None, seed(2, "/b"), None).unwrap(); store.workspace_set_active_tab(ws, second.id, None).unwrap(); + let (_sub, heard) = recorded(&store); let dropped = store.tab_close(ws, second.id, None).unwrap(); assert_eq!(dropped, vec![2]); let workspace = store.workspace(ws).unwrap(); @@ -1615,7 +1663,21 @@ mod tests { Some(first.id), "the active tab may not dangle on a closed id" ); + // The heal is broadcast, not left for clients to re-derive: after the + // `TabClosed` comes an `ActiveTabChanged` naming the survivor. + assert!( + matches!( + heard.lock().unwrap().as_slice(), + [ + (_, LayoutDelta::TabClosed { tab }), + (_, LayoutDelta::ActiveTabChanged { tab: active }) + ] if *tab == second.id && *active == first.id + ), + "heard {:?}", + heard.lock().unwrap() + ); + heard.lock().unwrap().clear(); let dropped = store.tab_close(ws, first.id, None).unwrap(); assert_eq!(dropped, vec![1]); assert_eq!( @@ -1623,6 +1685,11 @@ mod tests { None, "a workspace with no tabs has no active one — the home-page state" ); + assert_eq!( + heard.lock().unwrap().len(), + 1, + "losing the last tab needs no ActiveTabChanged: no tabs, no active tab" + ); } #[test] @@ -1907,6 +1974,7 @@ mod tests { store.pane(1).unwrap().cwd.as_deref(), Some("/observed/here") ); + withdraw_observations(); } // ── Attachment ───────────────────────────────────────────────────────── diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index a5a036bb..6b5274e9 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -1289,9 +1289,16 @@ impl DaemonPane { // change so the per-chunk cost is two clones and a // compare, not a store mutation per chunk. let pane = st.id; + // Read *with* the facts, not assumed: on Windows + // the exit monitor can report the death (flipping + // `alive`) while this thread is still draining + // ConPTY's buffered output, and the death report + // is latched — a "proof of life" published here + // after it would mark a dead pane live forever. + let alive = st.alive; let facts_after = observed_facts(&st); drop(st); - if facts_after != facts_before { + if facts_changed(&facts_before, &facts_after) { let (cwd, agent) = facts_after; crate::core::machine::observe_pane(pane, |p| { // An unknown cwd never clears a seeded one: @@ -1305,9 +1312,11 @@ impl DaemonPane { // foreground, and a revival must not // resume a session that already ended. p.agent = agent; - // Output is proof of life, whatever the - // record thought. - p.live = true; + // Output is proof of life — but only while + // the pane still is; see `alive` above. + if alive { + p.live = true; + } }); } } @@ -1952,6 +1961,29 @@ fn observed_facts(st: &PaneState) -> (Option, Option, Option), + after: &(Option, Option), +) -> bool { + let strip = |facts: &(Option, Option)| { + ( + facts.0.clone(), + facts.1.clone().map(|mut agent| { + agent.status = None; + agent + }), + ) + }; + strip(before) != strip(after) +} + /// Apply sniffed signals to the shared state and notify the subscriber of any cwd /// / prompt change. Called with the state lock held. fn apply_signals(st: &mut PaneState, signals: SniffSignals) { diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 17c61876..efefb3ce 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -157,13 +157,17 @@ pub fn run_daemon() -> anyhow::Result<()> { pub fn control_services() -> crate::host::server::Services { use crate::core::machine::MachineStore; use crate::core::workspace_store::WorkspaceStore; + // Reported on stderr as well as the log, like the socket line in + // [`run_daemon`]: on a headless box the log file is off by default, and + // "which stores does this daemon actually serve" is the first question a + // capability mismatch raises. let services = match WorkspaceStore::shared() { Ok(store) => { - log::info!("workspace store at {}", store.path().display()); + eprintln!("workspace store at {}", store.path().display()); crate::host::server::Services::with_workspaces(store) } Err(e) => { - log::warn!("no workspace store ({e}); serving files and panes only"); + eprintln!("no workspace store ({e}); serving files and panes only"); crate::host::server::Services::none() } }; @@ -173,7 +177,7 @@ pub fn control_services() -> crate::host::server::Services { // hold the other. match MachineStore::shared() { Ok(machine) => { - log::info!("machine tree at {}", machine.path().display()); + eprintln!("machine tree at {}", machine.path().display()); // From here on the pane server's own observations — OSC 7 cwds, // agent identities, deaths — land on the tree's pane records, so // what a client revives from is what the machine saw, not what @@ -182,7 +186,7 @@ pub fn control_services() -> crate::host::server::Services { services.and_machine(machine) } Err(e) => { - log::warn!("no machine tree ({e}); its verbs stay unserved"); + eprintln!("no machine tree ({e}); its verbs stay unserved"); services } } diff --git a/crates/tty7-server/tests/machine_tree.rs b/crates/tty7-server/tests/machine_tree.rs index 22401b79..3e18f4f4 100644 --- a/crates/tty7-server/tests/machine_tree.rs +++ b/crates/tty7-server/tests/machine_tree.rs @@ -372,6 +372,12 @@ fn an_operation_from_one_client_reaches_the_other_as_a_delta() { ws.id, |d| matches!(d, LayoutDelta::TabCreated { tab: t, .. } if t.id == tab.id), ); + // The created tab became active, and the *change of active tab* is its own + // delta — implicit activation must not be something a client re-derives. + watcher.expect_delta( + ws.id, + |d| matches!(d, LayoutDelta::ActiveTabChanged { tab: t } if *t == tab.id), + ); assert_eq!( writer.delta_count(), 0, @@ -391,7 +397,7 @@ fn an_operation_from_one_client_reaches_the_other_as_a_delta() { ws.id, |d| matches!(d, LayoutDelta::TabRenamed { name: Some(n), .. } if n == "build"), ); - assert_eq!(watcher.delta_count(), 2, "still only the writer's two ops"); + assert_eq!(watcher.delta_count(), 3, "still only the writer's own ops"); } /// Takeover semantics on the new tree, with **no record store served at diff --git a/src/ui/local_link.rs b/src/ui/local_link.rs index 7072d2ad..88dfd298 100644 --- a/src/ui/local_link.rs +++ b/src/ui/local_link.rs @@ -133,6 +133,10 @@ impl LocalLink { link.client = None; } match link.next_attempt { + // Never attempted at all: due now. The daemon is normally already + // up (main spawns it before the first window), so the first tick + // should connect, not start a schedule. + None if link.backoff.attempt() == 0 => {} None => { link.next_attempt = Some(now + link.backoff.delay()); return;