diff --git a/desktop/app_autosave_test.go b/desktop/app_autosave_test.go index 181d54d7b..6cccbc892 100644 --- a/desktop/app_autosave_test.go +++ b/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") + } +} diff --git a/desktop/tabs.go b/desktop/tabs.go index 79169d727..11ed07c7b 100644 --- a/desktop/tabs.go +++ b/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 }