fix(desktop): close CloseTab-Snapshot/DeleteSession resurrection race

desktop/app_autosave_test.go

@@ -131,3 +131,93 @@ func TestScheduleSnapshotCoalesces(t *testing.T) {
 	waitForFile(t, path, "acknowledged")
 	waitForAutosaveIdle(t, tab)
 }
+
+// TestCloseTabNoResurrectionFromAutosave is the regression test for #4384.
+// It proves that after CloseTab returns, the per-turn autosave goroutine can no
+// longer write the session file — even when it is in flight at the moment the
+// tab is closed. Pre-fix, the loop held a raw *WorkspaceTab pointer and a
+// captured session path, so its Snapshot() call landed after DeleteSession
+// trashed the file, "resurrecting" it.
+func TestCloseTabNoResurrectionFromAutosave(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "session.jsonl")
+
+	doomed, doomedTab := appWithTab(t, path)
+	// CloseTab needs >1 tab and mutates activeTabID, so add a survivor tab.
+	survivor := &WorkspaceTab{
+		ID:          "survivor_tab",
+		Scope:       "global",
+		Ready:       true,
+		disabledMCP: map[string]ServerView{},
+	}
+	survivor.sink = &tabEventSink{tabID: survivor.ID, app: doomed}
+	doomed.tabs["survivor_tab"] = survivor
+	doomed.activeTabID = "test_tab"
+
+	// Write the session file once via the autosave loop, then wait for idle so
+	// the next TurnDone reliably kicks off a fresh loop.
+	doomedTab.sink.Emit(event.Event{Kind: event.TurnDone})
+	waitForFile(t, path, "acknowledged")
+	waitForAutosaveIdle(t, doomedTab)
+
+	// Kick the autosave loop and close the tab in close succession. The loop
+	// will be in flight when CloseTab runs — exactly the #4384 window.
+	doomedTab.sink.Emit(event.Event{Kind: event.TurnDone})
+	if err := doomed.CloseTab("test_tab"); err != nil {
+		t.Fatalf("CloseTab: %v", err)
+	}
+
+	// CloseTab must have returned only after the autosave loop finished. Remove
+	// the file the way DeleteSession would (move to trash is just a remove here
+	// since we only care that nothing rewrites the original path).
+	if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
+		t.Fatalf("remove session file: %v", err)
+	}
+
+	// Give any would-be resurrection a chance to strike. If the autosave loop
+	// were still alive (the bug), the file reappears here.
+	time.Sleep(100 * time.Millisecond)
+	if _, err := os.Stat(path); !os.IsNotExist(err) {
+		t.Fatalf("session file resurrected after CloseTab + delete (stat err=%v) — autosave loop not drained", err)
+	}
+
+	// And the controller's session path must be cleared so no future Snapshot
+	// can write either.
+	if got := doomedTab.Ctrl.SessionPath(); got != "" {
+		t.Fatalf("controller session path = %q after CloseTab, want empty so snapshots no-op", got)
+	}
+}
+
+// TestCloseTabSurvivorKeepsAutosave ensures the survivor tab is untouched: the
+// closing/drain logic is per-tab and must not leak to other tabs.
+func TestCloseTabSurvivorKeepsAutosave(t *testing.T) {
+	doomedPath := filepath.Join(t.TempDir(), "doomed.jsonl")
+	survivorPath := filepath.Join(t.TempDir(), "survivor.jsonl")
+
+	a, _ := appWithTab(t, doomedPath)
+	survivorCtrl := controllerWithContent(t, survivorPath)
+	survivor := &WorkspaceTab{
+		ID:          "survivor_tab",
+		Ctrl:        survivorCtrl,
+		Scope:       "global",
+		Ready:       true,
+		disabledMCP: map[string]ServerView{},
+	}
+	survivor.sink = &tabEventSink{tabID: survivor.ID, app: a}
+	a.tabs["survivor_tab"] = survivor
+	a.activeTabID = "test_tab"
+
+	survivor.sink.Emit(event.Event{Kind: event.TurnDone})
+	waitForFile(t, survivorPath, "acknowledged")
+	waitForAutosaveIdle(t, survivor)
+
+	if err := a.CloseTab("test_tab"); err != nil {
+		t.Fatalf("CloseTab: %v", err)
+	}
+
+	if got := survivor.Ctrl.SessionPath(); got != survivorPath {
+		t.Fatalf("survivor session path = %q, want %q", got, survivorPath)
+	}
+	if survivor.closing {
+		t.Fatal("survivor tab was marked closing — closing flag leaked across tabs")
+	}
+}

desktop/tabs.go

@@ -52,6 +52,15 @@ type WorkspaceTab struct {
 	saving    bool
 	saveAgain bool
 
+	// closing is set under saveMu when the tab is being torn down. Once set,
+	// tabSnapshotLoop stops taking new snapshot work and CloseTab waits on
+	// saveCond until any in-flight snapshot finishes - so no background
+	// snapshot can write a session file back to disk after CloseTab returns.
+	// Without this, deleting a just-closed session races that write and the
+	// session "resurrects" (#4384).
+	closing  bool
+	saveCond *sync.Cond
+
 	// readTelemetry tracks files read during this tab's session.
 	readTelemetry  []readFileRecord
 	usageTelemetry sessionUsageStats
@@ -1280,10 +1289,22 @@ func (a *App) CloseTab(tabID string) error {
 
 	// Tear down outside the lock.
 	if tab.Ctrl != nil {
+		// Final snapshot while the controller still owns its session path.
+		// a.mu stays free during the disk write; the two resurrection
+		// vectors are neutralized below before DeleteSession can see the
+		// tab as gone:
+		//   (1) clear the controller's session path so any later Snapshot
+		//       (including the autosave loop) is a no-op;
+		//   (2) drain any in-flight tabSnapshotLoop before returning, so no
+		//       background write can land after the file is trashed.
 		_ = tab.Ctrl.Snapshot()
 		if tab.hasActiveRuntimeWork() && a.detachSessionRuntime(tab) {
+			// Detached runtimes keep running and must keep saving: do not
+			// clear the path or drain for them.
 			return nil
 		}
+		tab.Ctrl.SetSessionPath("") // future snapshots become no-ops
+		a.quiesceTabAutosave(tab)   // wait for any in-flight snapshot to finish
 		tab.Ctrl.Cancel()
 		tab.Ctrl.Close()
 	}
@@ -1551,16 +1572,45 @@ func (a *App) scheduleTabSnapshot(tabID string) {
 		return
 	}
 	tab.saveMu.Lock()
+	defer tab.saveMu.Unlock()
+	if tab.closing {
+		// Tab is being torn down: don't start new snapshot work that could
+		// race DeleteSession and resurrect a trashed session file (#4384).
+		return
+	}
 	if tab.saving {
 		tab.saveAgain = true
-		tab.saveMu.Unlock()
 		return
 	}
 	tab.saving = true
-	tab.saveMu.Unlock()
 	go a.tabSnapshotLoop(tab)
 }
 
+// quiesceTabAutosave marks the tab as closing and blocks until any in-flight
+// tabSnapshotLoop has finished its current (and final) write. After it returns,
+// no background goroutine can call Snapshot on this tab's controller again, so
+// a subsequent DeleteSession cannot race a late write. Safe to call after the
+// controller's session path has been cleared: the loop's Snapshot becomes a
+// no-op and it exits on its next iteration.
+func (a *App) quiesceTabAutosave(tab *WorkspaceTab) {
+	if tab == nil {
+		return
+	}
+	tab.saveMu.Lock()
+	if tab.saveCond == nil {
+		// saveCond is lazily initialized on first snapshot; if it was never
+		// set there is no loop to wait for.
+		tab.closing = true
+		tab.saveMu.Unlock()
+		return
+	}
+	tab.closing = true
+	for tab.saving {
+		tab.saveCond.Wait()
+	}
+	tab.saveMu.Unlock()
+}
+
 func (a *App) tabSnapshotLoop(tab *WorkspaceTab) {
 	defer a.recoverToPending("tabSnapshotLoop")
 	for {
@@ -1575,12 +1625,23 @@ func (a *App) tabSnapshotLoop(tab *WorkspaceTab) {
 			}
 		}
 		tab.saveMu.Lock()
+		if tab.saveCond == nil {
+			tab.saveCond = sync.NewCond(&tab.saveMu)
+		}
+		if tab.closing {
+			// Tab is being torn down: stop without picking up saveAgain work.
+			tab.saving = false
+			tab.saveCond.Broadcast()
+			tab.saveMu.Unlock()
+			return
+		}
 		if tab.saveAgain {
 			tab.saveAgain = false
 			tab.saveMu.Unlock()
 			continue
 		}
 		tab.saving = false
+		tab.saveCond.Broadcast()
 		tab.saveMu.Unlock()
 		return
 	}