diff --git a/CHANGELOG.md b/CHANGELOG.md index a70b1efc..d398f192 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A tree pull that has to be retried no longer ends with the window deleting + the tabs it was pulling.** A window told to rebuild itself from the machine — + a daemon back as a new process, a restart handoff that was refused, a remote + server restarted — abandoned that job the moment it had any tabs at all, and + in every one of those cases it does: the stale ones on screen are the whole + reason it was asked. So the rebuild silently did nothing, and worse, the + window went on claiming to speak for the workspace it had never read. Opening + one tab over it then diffed into "close every tab" and deleted those panes' + records while their shells were still running, with nothing left that could + reach them. The retry is now dropped only for a tab the user really did make + while the pull was out, and a window waiting on a rebuild adds to its machine + without pruning it until the pull lands (#579). - **A local daemon that dies and comes back no longer leaves a window of dead panes looking live** — from the client's side a killed daemon is indistinguishable from one whose shells all exited at once, so the window diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index 672a8b58..08849c3f 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -710,10 +710,25 @@ struct WsState { epoch: u64, /// A hydration that failed and still owes this window its layout. /// - /// The window is sitting empty because of it, so nothing may be pushed - /// from it until the pull is retried — an empty window diffs into - /// "close every tab" and would wipe the layout off the machine. + /// Whatever the window is showing is not the layout it was told to put up, + /// so nothing may be pushed from it until the pull is retried — an emptied + /// window diffs into "close every tab" and would wipe the layout off the + /// machine, and a window full of dead tabs pushes those back up as truth. rehydrate: Option, + /// The tabs the window held when the pull that is owed was ordered. + /// + /// A `Replace` retry is dropped once the user has filled the window in + /// themselves, and this is what "the user" means. A `Replace` is ordered + /// precisely *because* what the window is showing cannot be trusted — dead + /// panes left by a daemon that came back as a new process, a window emptied + /// for a handoff that was then refused — so the tabs that were already + /// there are the very thing it exists to replace, and can never be the + /// reason to abandon it. Only a tab this debt never saw says the user moved + /// on without us. + /// + /// Rewritten by every attempt, and only ever read while `rehydrate` is + /// outstanding, so it always describes the debt currently standing. + owed_over: Vec, /// How many pulls in a row this window has owed, which paces the retry. /// /// Counts consecutive failures, so it is cleared by anything that ends the @@ -757,6 +772,7 @@ impl Default for WsState { informed: false, epoch: 0, rehydrate: None, + owed_over: Vec::new(), rehydrate_attempts: 0, then_open: None, said_why_empty: false, @@ -779,7 +795,8 @@ pub(crate) fn sync_window(app: &Tty7App, cx: &mut App) { if crate::ui::remote_workspace::workspace_is_preempted(cx, client_ws) { return; } - if let Some(adopt) = take_rehydrate(cx, client_ws, app.tabs.is_empty()) { + let showing: Vec = app.tabs.iter().map(|t| t.tree_id.get()).collect(); + if let Some(adopt) = take_rehydrate(cx, client_ws, &showing) { hydrate(cx, client_ws, adopt); return; } @@ -832,17 +849,26 @@ pub(crate) fn on_link_up(cx: &mut App, host: HostId) { } /// Claims a hydration owed to `client_ws`, if one is still outstanding. +/// `showing` is what the window has on screen right now. /// -/// A `Replace` retry is dropped once the window has tabs again: the user moved -/// on without us, and replaying the machine's older layout over their work +/// A `Replace` retry is dropped once the user has filled the window in without +/// us: replaying the machine's older layout over work they did in the meantime /// would be worse than never retrying at all. -fn take_rehydrate(cx: &mut App, client_ws: WorkspaceId, window_is_empty: bool) -> Option { +/// +/// What counts as "without us" is a tab the debt never saw. Judging it by the +/// window merely *having* tabs read every resync of a window that kept its own +/// — a daemon back as a new process, a delta that would not apply, a remote +/// server restarted — as the user having moved on, when those tabs are the +/// stale ones the `Replace` was ordered to sweep away. The retry was abandoned +/// on its first attempt, every time, and the resync silently did nothing. +fn take_rehydrate(cx: &mut App, client_ws: WorkspaceId, showing: &[TabId]) -> Option { let state = cx .default_global::() .windows .get_mut(&client_ws)?; let adopt = state.rehydrate.take()?; - if !window_is_empty && adopt == Adopt::Replace { + let overtaken = showing.iter().any(|id| !state.owed_over.contains(id)); + if overtaken && adopt == Adopt::Replace { // Abandoned, not paid — but the run of failures is over either way, and // a count left standing would make the next window's first failure wait // the cap on an outage that has nothing to do with it. @@ -1324,9 +1350,24 @@ enum Adopt { Replace, } +/// The tree ids of the tabs `client_ws`'s window is showing. +/// +/// Empty for a workspace no window has — and for a test with no registry at +/// all, which is the same answer: nothing is on screen to speak for it. +fn tabs_on_screen(cx: &mut App, client_ws: WorkspaceId) -> Vec { + if !cx.has_global::() { + return Vec::new(); + } + crate::ui::windows::WindowRegistry::app_for(cx, client_ws) + .and_then(|app| app.upgrade()) + .map(|app| app.read(cx).tabs.iter().map(|t| t.tree_id.get()).collect()) + .unwrap_or_default() +} + fn hydrate(cx: &mut App, client_ws: WorkspaceId, adopt: Adopt) { let host = WorkspaceStore::host_of(cx, client_ws); let machine_ws = tree_workspace_id(cx, client_ws); + let showing = tabs_on_screen(cx, client_ws); let (epoch, failures) = { let state = cx .default_global::() @@ -1341,6 +1382,25 @@ fn hydrate(cx: &mut App, client_ws: WorkspaceId, adopt: Adopt) { state.epoch += 1; // This attempt takes over the debt; it re-records it if it fails too. state.rehydrate = None; + // What the window is showing as this attempt is ordered, so a debt it + // has to record can tell that layout apart from one the user builds + // while the pull is out. + state.owed_over = showing; + if adopt == Adopt::Replace { + // A `Replace` is the statement that this window no longer speaks + // for its workspace, so the licence to prune goes with it — the + // same move `on_preempted` makes, for the same reason. Only a pull + // that lands hands it back (`settle_hydration`). + // + // Without this, a `Replace` that failed and was then abandoned left + // a window that had never seen the layout still authorised to diff + // at `SyncScope::Full`: one tab the user opened over an emptied + // window became `TabClose` for every tab on the machine, deleting + // the records of panes whose shells were still running (#579). The + // window is left additive instead — it writes its own tabs up and + // closes nothing it cannot account for. + state.informed = false; + } (state.epoch, state.rehydrate_attempts) }; // How many times in a row this window has already failed, which is what @@ -2737,9 +2797,10 @@ mod tests { .get_mut(&ws) .unwrap(); state.rehydrate = Some(Adopt::Replace); + state.owed_over = Vec::new(); state.rehydrate_attempts = 4; } - assert!(take_rehydrate(cx, ws, false).is_none()); + assert!(take_rehydrate(cx, ws, &[TabId::new()]).is_none()); assert_eq!( attempts(cx), 0, @@ -2786,27 +2847,47 @@ mod tests { fn a_window_that_filled_up_while_owed_keeps_what_it_has(cx: &mut gpui::TestAppContext) { cx.update(|cx| { let ws = WorkspaceId::new(); - let arm = |cx: &mut App, adopt| { - cx.default_global::() + let stale = [TabId::new(), TabId::new()]; + let arm = |cx: &mut App, adopt, over: &[TabId]| { + let state = cx + .default_global::() .windows .entry(ws) - .or_default() - .rehydrate = Some(adopt); + .or_default(); + state.rehydrate = Some(adopt); + state.owed_over = over.to_vec(); }; - arm(cx, Adopt::Replace); + arm(cx, Adopt::Replace, &[]); assert!( - take_rehydrate(cx, ws, true).is_some(), + take_rehydrate(cx, ws, &[]).is_some(), "an empty window is exactly the one that still needs its layout" ); assert!( - take_rehydrate(cx, ws, true).is_none(), + take_rehydrate(cx, ws, &[]).is_none(), "the debt is claimed once" ); - arm(cx, Adopt::Replace); + // #579: the tabs a `Replace` was ordered over are the ones it exists + // to replace. Reading them as "the user moved on" abandoned the + // retry on its first attempt for every resync of a window that kept + // its tabs — a daemon back as a new process, a remote server + // restarted — and the resync silently did nothing at all. + arm(cx, Adopt::Replace, &stale); assert!( - take_rehydrate(cx, ws, false).is_none(), + take_rehydrate(cx, ws, &stale).is_some(), + "the dead tabs the resync was ordered over cannot be the reason to drop it" + ); + + arm(cx, Adopt::Replace, &stale); + assert!( + take_rehydrate(cx, ws, &stale[..1]).is_some(), + "closing some of them is not filling the window in either" + ); + + arm(cx, Adopt::Replace, &stale); + assert!( + take_rehydrate(cx, ws, &[stale[0], TabId::new()]).is_none(), "replaying an older layout over the user's new tabs is worse than not retrying" ); assert!( @@ -2816,14 +2897,89 @@ mod tests { "and the dropped retry must not linger" ); - arm(cx, Adopt::IfEmpty); + arm(cx, Adopt::IfEmpty, &[]); assert!( - take_rehydrate(cx, ws, false).is_some(), + take_rehydrate(cx, ws, &[TabId::new()]).is_some(), "IfEmpty polices that itself, and still owes the mirror a pull" ); }); } + /// #579: the other half of an abandoned `Replace`. Dropping the retry is a + /// decision about what to *put up*; it says nothing about the window having + /// learned what belongs in the workspace, and it never did learn — the pull + /// it was told to make failed. + /// + /// Left `informed`, that window went on to diff at `SyncScope::Full` + /// against a mirror `start_prime` had just refilled from the machine, so + /// the one tab the user opened over an emptied window emitted `TabClose` + /// for every other tab on it. Those panes' records went with them while + /// their shells kept running, and nothing tree-driven could reach them + /// again — the same damage as #554, through a door #554 did not close. + #[gpui::test] + fn a_resync_takes_back_the_licence_to_prune_until_a_pull_lands(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let ws = WorkspaceId::new(); + { + let state = cx + .default_global::() + .windows + .entry(ws) + .or_default(); + state.informed = true; + } + + resync_window_from_tree(cx, ws); + assert!( + !cx.default_global::().windows[&ws].informed, + "a window told its contents are not the workspace cannot still speak for it" + ); + + // The shape the damage needs: the mirror back from the machine, + // full of the tabs the window never adopted. Without the licence + // the sync is additive, which + // `an_additive_diff_never_closes_tabs_the_window_has_not_seen` + // pins as closing nothing. + { + let state = cx + .default_global::() + .windows + .get_mut(&ws) + .unwrap(); + state.rehydrate = None; + state.sync = SyncPhase::Primed(WsMirror { + tabs: vec![TreeTab::leaf(1), TreeTab::leaf(2)], + active: None, + }); + } + assert!( + !workspace_is_disposable(cx, ws), + "and it cannot delete the workspace out from under those panes either" + ); + + // A pull that lands is the only thing that hands the licence back. + mark_window_informed(cx, ws); + assert!(cx.default_global::().windows[&ws].informed); + }); + } + + /// An `IfEmpty` hydration says the opposite of a `Replace`: keep whatever + /// the window has. A window that opened onto a workspace deleted under it + /// is put back by writing its own tabs up, and needs the licence to do it. + #[gpui::test] + fn an_if_empty_pull_leaves_the_licence_where_it_found_it(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let ws = WorkspaceId::new(); + cx.default_global::() + .windows + .entry(ws) + .or_default() + .informed = true; + hydrate_window_from_tree(cx, ws); + assert!(cx.default_global::().windows[&ws].informed); + }); + } + #[test] fn a_ratio_delta_is_clamped_to_the_servers_band_not_a_narrower_one() { let mut pane = Pane::split_node(gpui::Axis::Horizontal, 0.5, Pane::Empty, Pane::Empty);