fix(tree-sync): stop an abandoned Replace debt deleting the tabs it was pulling (#579) (#608)

`take_rehydrate` dropped an owed `Adopt::Replace` whenever the window had
any tabs, on the reading that the user had filled it in themselves. That
reading only ever fitted the one caller it was written for: Restart
Server, which empties the window before it resyncs. Every other
`Replace` — a daemon back as a new process, a restart handoff that was
refused, a remote server restarted, a layout delta that would not apply —
is ordered over a window that still holds its tabs, because those stale
tabs are the whole reason it was asked. The retry was abandoned on its
first attempt, every time, and the resync silently did nothing.

Abandoning it also left the window `informed`: still licensed to prune a
workspace whose layout it had never read. `start_prime` then refilled the
mirror from the machine, and the next `SyncScope::Full` diffed whatever
the window happened to hold against it — one tab opened over an emptied
window became `TabClose` for every tab on the machine, deleting those
panes' records while their shells kept running, with nothing tree-driven
left that could reach them. The damage #554 describes, through a door
#554 did not close, and the same on a remote window.

So the debt is now scoped to the layout it was owed over: `hydrate`
records the tabs on screen as it orders a pull, and only a tab that debt
never saw counts as the user moving on. And a `Replace` takes back the
`informed` licence up front, the way `on_preempted` does, so a window
waiting on a rebuild adds to its machine without pruning it until the
pull lands — however the debt ends.

Pre-existing since #472 (af3928d, e4bd49c); both PR #564 and #569 saw it
and left it alone because the rule is shared with the remote resyncs.

Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>

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

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<Adopt>,
+    /// 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<TabId>,
     /// 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<TabId> = 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<Adopt> {
+///
+/// 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<Adopt> {
     let state = cx
         .default_global::<TreeSync>()
         .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<TabId> {
+    if !cx.has_global::<crate::ui::windows::WindowRegistry>() {
+        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::<TreeSync>()
@@ -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::<TreeSync>()
+            let stale = [TabId::new(), TabId::new()];
+            let arm = |cx: &mut App, adopt, over: &[TabId]| {
+                let state = cx
+                    .default_global::<TreeSync>()
                     .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::<TreeSync>()
+                    .windows
+                    .entry(ws)
+                    .or_default();
+                state.informed = true;
+            }
+
+            resync_window_from_tree(cx, ws);
+            assert!(
+                !cx.default_global::<TreeSync>().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::<TreeSync>()
+                    .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::<TreeSync>().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::<TreeSync>()
+                .windows
+                .entry(ws)
+                .or_default()
+                .informed = true;
+            hydrate_window_from_tree(cx, ws);
+            assert!(cx.default_global::<TreeSync>().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);