diff --git a/CHANGELOG.md b/CHANGELOG.md index cef5e28e..0deb0bcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 shows through the whole workspace, and the settings panel stays opaque. macOS and Linux keep the existing blur toggle. +- **Panes come back showing what was on them after the background service dies + unexpectedly** — a crash, a `kill -9` or a reboot takes the shells with it + either way, but the screens no longer go with them. A capped tail of each + pane's output is kept at `/scrollback/*.bin` (0600 on unix, behind + the config directory's ACL on Windows; 256 KiB per pane, written at most + every 30s and only for panes whose output moved), and a pane that reopens on + a dead predecessor's id is handed it. A planned restart already carried the + live ptys across untouched; this covers the deaths nothing gets to prepare + for. There is no switch: the moment anyone learns they wanted this is the + moment a service has already died, so it is on for everyone. The bytes are + dropped as soon as nothing can ask for them — closing a pane deletes its + file at once, a restore consumes it, and a periodic pass collects the rest. + A pane's shell is recorded alongside, so a git bash pane no longer comes + back as PowerShell. + ### Fixed - **An SFTP upload no longer sits in the browser under its temporary name** — diff --git a/crates/tty7-core/src/daemon/scrollback.rs b/crates/tty7-core/src/daemon/scrollback.rs index dff605eb..5c545b6d 100644 --- a/crates/tty7-core/src/daemon/scrollback.rs +++ b/crates/tty7-core/src/daemon/scrollback.rs @@ -22,10 +22,15 @@ //! screen or two, not the ring's whole 8 MiB. The value of scrollback decays //! steeply with distance from the bottom, and every byte here is a byte of //! someone's terminal sitting on disk. -//! - **It is off unless asked for.** These bytes include whatever was echoed -//! into the pane: tokens, `env` output, an agent's transcript. In memory -//! they die with the daemon. On disk they outlive it, which is the whole -//! point and also the whole risk, so the choice is the user's. +//! - **It is not asked for.** These bytes include whatever was echoed into the +//! pane: tokens, `env` output, an agent's transcript. In memory they die +//! with the daemon; on disk they outlive it, which is the whole point and +//! also the whole risk. It was a setting once, defaulting to off. That was +//! wrong about *when* the choice gets made: the moment anyone learns they +//! wanted this is the moment a daemon has already died, and by then the +//! switch could only be flipped for next time. A feature that exists to +//! survive an unscheduled event cannot be opt-in. So the cost is paid for +//! everyone, and the two bullets below are what keep it small. //! - **It is dropped as soon as it is meaningless.** A pane that is closed, or //! that no workspace refers to any more, has its file removed. Retention is //! by relevance, not by calendar: a snapshot of a pane nobody will reopen is diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 40a7f919..e1450f02 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -178,12 +178,19 @@ fn restorable_pane_ids(registry: &Registry) -> std::collections::HashSet { ids } -/// Keep each pane's stored screen roughly current. +/// Keep each pane's stored screen roughly current, and collect what no pane +/// can be asked about any more. /// /// Only panes whose ring has moved are written, so an idle machine does no IO /// at all, and the busy pane that most needs a fresh copy is the one that gets /// it. -fn spawn_scrollback_writer(registry: Arc) { +/// +/// The two sweeps ride along here rather than at startup because this is where +/// the question they ask can be answered: by now the registry holds this +/// daemon's panes and the windows have had time to put their trees back. Both +/// take the same set, because a pane whose screen is still worth restoring is +/// exactly a pane whose commands are still worth carrying. +fn spawn_snapshot_keeper(registry: Arc) { let spawned = std::thread::Builder::new() .name("tty7-scrollback".into()) .spawn(move || { @@ -198,7 +205,9 @@ fn spawn_scrollback_writer(registry: Arc) { crate::daemon::scrollback::save(pane.id, &segments); marks.insert(pane.id, mark); } - crate::daemon::scrollback::sweep(&restorable_pane_ids(®istry)); + let restorable = restorable_pane_ids(®istry); + crate::daemon::scrollback::sweep(&restorable); + crate::daemon::history::sweep(&restorable); marks.retain(|id, _| registry.get(*id).is_some()); } }); @@ -230,13 +239,15 @@ fn restored_screen( request: crate::daemon::protocol::RestoreFrom, ) -> Option { let segments = crate::daemon::scrollback::load(request.pane_id)?; + // Dropped either way — this is the one request that will ever be made about + // this pane, so nothing is served by keeping the file past it. What the + // emptiness check decides is whether a *restore* happened, not whether the + // file stays: a snapshot holding nothing is not a screen to hand over, but + // it is still a file nobody will read again. + crate::daemon::scrollback::forget(request.pane_id); if segments.is_empty() { return None; } - // After the emptiness check, not before it: dropping the file is how a - // screen that *was* handed out stops being handed out twice, and a - // snapshot that turned out to hold nothing was never handed out at all. - crate::daemon::scrollback::forget(request.pane_id); log::info!( "pane {} is gone; its last screen is restored into a fresh pane", request.pane_id @@ -549,25 +560,26 @@ fn run_with(registry: Arc) -> anyhow::Result<()> { } spawn_orphan_sweep(registry.clone()); - let restorable = restorable_pane_ids(®istry); - // No scrollback sweep here, deliberately. Startup is the one moment this - // process knows least: it owns no panes yet, and the windows that know - // which screens are still wanted cannot say so until the endpoint below is - // listening. Answering "is anyone going to ask for this?" here answers it - // when nobody can — and the answer deletes. A tree that failed to parse - // makes it worse, because `read_machine` quarantines it and hands back an - // empty `Machine`, so one bad file would take every pane's screen with it. + // No sweeping here, deliberately — of either kind. Startup is the one + // moment this process knows least: it owns no panes yet, and the windows + // that know which of a dead daemon's files are still wanted cannot say so + // until the endpoint below is listening. Answering "is anyone going to ask + // for this?" here answers it when nobody can — and the answer deletes. A + // tree that failed to parse makes it worse, because `read_machine` + // quarantines it and hands back an empty `Machine`, so one bad file would + // take every pane's screen and every pane's history with it. // - // The periodic sweep asks the same question a tick later, with the registry - // filled in and the tree caught up, and that is soon enough: nothing here - // is serving a request in the meantime. + // History was swept here until it was noticed that a restore carries the + // dead pane's commands to its successor (`history::carry`, at the top of + // the `Spawn` handler): sweeping before the window can ask deletes the very + // file the request is about. Same shape as the scrollback bug, one file + // over. // - // A daemon that was killed outright never retired anything, so the files of - // panes that died with it are still here. Their commands cannot be - // recovered — the mark saying which were new belongs to a shell that is - // gone — so what is left is not to hoard them. - crate::daemon::history::sweep(&restorable); - spawn_scrollback_writer(registry.clone()); + // Both sweeps run on the writer's tick instead, with the registry filled in + // and the tree caught up. That is soon enough: nothing here is serving a + // request in the meantime, and a daemon killed outright left its files for + // exactly this pass to collect. + spawn_snapshot_keeper(registry.clone()); for stream in listener.incoming() { match stream { diff --git a/crates/tty7-server/tests/scrollback_restore.rs b/crates/tty7-server/tests/scrollback_restore.rs index 15b660cb..04436e72 100644 --- a/crates/tty7-server/tests/scrollback_restore.rs +++ b/crates/tty7-server/tests/scrollback_restore.rs @@ -332,9 +332,11 @@ fn the_tree_records_the_shell_a_pane_is_running() { // A pane reaches the tree by being put in a tab, which is what the window // does right after it spawns one — carrying the same seed it spawned with. - // Until then the daemon has nothing to record its facts against, so the - // seed is the pane's first and only chance to say what it is running. - let mut control = ControlClient::connect_at( + // Until then the daemon has nothing to record its facts against, so this is + // the pane's first chance to say what it is running, and for a pane that + // then sits at a prompt it is the only one: the daemon's own observation + // rides on a fact *changing*, and a pane's shell never does. + let control = ControlClient::connect_at( &instance.control_endpoint(), &ControlHello::host_rpc("probe", "probe"), ) diff --git a/src/ui/app.rs b/src/ui/app.rs index 2dfc35ce..f3336c97 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6633,16 +6633,6 @@ pub(crate) fn alive_panes_on( } } -fn pane_attachable( - alive: Option<&std::collections::HashMap>>, - id: u64, - owner: crate::core::session::WorkspaceId, -) -> bool { - // Listed at all, and then whose it is. A pane missing from the listing is - // one there is nothing to attach to. - alive.is_none_or(|listed| listed.contains_key(&id)) && pane_free_for(alive, id, owner) -} - /// Whether this window may stand on `id` — as the pane it attaches to, or as /// the dead predecessor whose screen a fresh pane opens showing. /// @@ -6652,8 +6642,13 @@ fn pane_attachable( /// Ruling the id out there threw away the only thing that could ask: the /// window spawned a pane that had never heard of a predecessor, so no attach /// was tried, no restore was requested, and the screen the daemon still had on -/// disk was swept a tick later, unread. Attaching is still tried first and -/// still fails harmlessly when the pane really is gone. +/// disk was swept a tick later, unread. +/// +/// There used to be a second predicate here that also required the id to be +/// listed, and the attach site consulted it. Nothing does now: the attach is +/// simply tried, and a pane that really is gone fails it and falls through to +/// the fresh spawn — the same outcome the listing was consulted to predict, +/// reached by asking the daemon instead of guessing ahead of it. /// /// Ownership does come into it. Another workspace's pane is not this window's /// to attach to, and its screen is not this window's to show. @@ -7389,8 +7384,8 @@ mod window_drag_tests { mod tests { use super::{ CloseReason, TabAgentSession, clear_window_override_values, close_prompt, - leaf_shares_the_window_daemon, mru_order, pane_attachable, pane_free_for, - parse_ssh_connect_input, parse_ssh_option_words, + leaf_shares_the_window_daemon, mru_order, pane_free_for, parse_ssh_connect_input, + parse_ssh_option_words, }; #[test] @@ -7502,28 +7497,24 @@ mod tests { .collect(); assert!( - pane_attachable(Some(&alive), 1, ours), + pane_free_for(Some(&alive), 1, ours), "our own pane attaches" ); assert!( - !pane_attachable(Some(&alive), 2, ours), + !pane_free_for(Some(&alive), 2, ours), "another workspace's pane must spawn fresh instead" ); assert!( - pane_attachable(Some(&alive), 3, ours), + pane_free_for(Some(&alive), 3, ours), "an unowned pane is legacy" ); assert!( - pane_attachable(Some(&alive), 5, ours), + pane_free_for(Some(&alive), 5, ours), "an owner that names no workspace is not a rival's claim: older CLIs \ wrote their own name there, and respawning strands the live pane" ); assert!( - !pane_attachable(Some(&alive), 4, ours), - "a dead id never attaches" - ); - assert!( - pane_attachable(None, 4, ours), + pane_free_for(None, 4, ours), "a failed List says nothing about pane 4; the attach itself must decide, \ because respawning on a transient RPC error destroys a live session" ); @@ -7546,10 +7537,6 @@ mod tests { pane_free_for(Some(&alive), 4, ours), "a dead pane's id has to survive; the restore is keyed on it" ); - assert!( - !pane_attachable(Some(&alive), 4, ours), - "attaching to it is still hopeless, and that stays true" - ); // What being free does not mean: helping yourself to a pane that is // alive and belongs to another workspace, whose screen is not this