diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index e144529128..e769baa1ab 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -1199,6 +1199,12 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu if err != nil { return CleanupResult{}, fmt.Errorf("cleanup %s: %w", project, err) } + // Workspace paths a live (non-terminated) session still occupies. A + // terminated predecessor and a live successor can share one persistent + // worktree (the orchestrator's is reused across respawn), so eligibility + // keys on the workspace path, not just the session's terminated state — + // reclaiming a path still in use would delete a live session's cwd. + liveWorkspaces := liveWorkspacePaths(recs) result := CleanupResult{Cleaned: make([]domain.SessionID, 0, len(recs)), Skipped: []CleanupSkip{}} for _, rec := range recs { if !rec.IsTerminated { @@ -1208,9 +1214,17 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu if ws.Path == "" { continue } + // Runtime teardown is keyed on the terminated session's own handle, not + // the workspace path, so it runs even when the workspace is shared with a + // live successor — otherwise a skipped session would leak its runtime + // (the lingering keep-alive shell) until cleanup reruns. if h := runtimeHandle(rec.Metadata); h.ID != "" { _ = m.runtime.Destroy(ctx, h) // best effort; usually already gone } + if liveWorkspaces[normalizeWorkspacePath(ws.Path)] { + result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: "workspace in use by a live session"}) + continue + } if err := m.workspace.Destroy(ctx, ws); err != nil { if !errors.Is(err, ports.ErrWorkspaceDirty) { // The public reason stays a fixed string (the raw error carries @@ -1225,6 +1239,30 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu return result, nil } +// liveWorkspacePaths returns the set of normalized workspace paths still +// occupied by a non-terminated session. Cleanup consults it so a terminated +// session that shares a persistent worktree with a live successor is skipped +// rather than reclaimed. +func liveWorkspacePaths(recs []domain.SessionRecord) map[string]bool { + live := make(map[string]bool) + for _, rec := range recs { + if rec.IsTerminated { + continue + } + if p := rec.Metadata.WorkspacePath; p != "" { + live[normalizeWorkspacePath(p)] = true + } + } + return live +} + +// normalizeWorkspacePath canonicalizes a workspace path for set membership so +// two records naming the same directory (a terminated predecessor and its live +// successor) compare equal despite trailing slashes or "." segments. +func normalizeWorkspacePath(p string) string { + return filepath.Clean(p) +} + // cleanupSkipReason renders a workspace teardown refusal as a short // user-facing reason for the cleanup report. Deliberately not the raw error: // it flows to the API response and CLI output, and teardown errors embed diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index 119a7946e4..09b99e337b 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -934,6 +934,97 @@ func TestCleanup_ReclaimsTerminalWorkspaces(t *testing.T) { } } +// TestCleanup_SkipsWorkspaceStillReferencedByLiveSession: a terminated +// session's workspace must NOT be reclaimed while a live (non-terminated) +// session references the same path. Persistent/shared worktrees (the +// orchestrator's) are reused across respawn, so a terminated predecessor and +// a live successor can share one path — reclaiming it deletes the live +// session's cwd out from under it. +func TestCleanup_SkipsWorkspaceStillReferencedByLiveSession(t *testing.T) { + m, st, rt, ws := newManager() + // Terminated predecessor and live successor share one persistent worktree. + // The predecessor keeps its OWN runtime handle (independent of the shared + // workspace and of the successor's handle). + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/shared", RuntimeHandleID: "mer-1-runtime"}) + live := mkLive("mer-2") + live.Metadata.WorkspacePath = "/ws/shared" + st.sessions["mer-2"] = live + + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Cleaned) != 0 { + t.Fatalf("cleaned = %v, want none (path still in use by live session)", res.Cleaned) + } + if len(res.Skipped) != 1 || res.Skipped[0].SessionID != "mer-1" { + t.Fatalf("skipped = %v, want mer-1", res.Skipped) + } + if res.Skipped[0].Reason != "workspace in use by a live session" { + t.Fatalf("reason = %q", res.Skipped[0].Reason) + } + if ws.destroyed != 0 { + t.Fatalf("destroyed = %d, want 0 — shared live workspace must not be torn down", ws.destroyed) + } + // The workspace is preserved, but the predecessor's own runtime must still be + // reclaimed — keying the skip on the workspace path must not leak its runtime. + if rt.destroyed != 1 || len(rt.destroyedIDs) != 1 || rt.destroyedIDs[0] != "mer-1-runtime" { + t.Fatalf("runtime destroyed = %d ids=%v, want the skipped session's own handle torn down", rt.destroyed, rt.destroyedIDs) + } +} + +// TestCleanup_LiveWorkspaceGuardNormalizesPaths: the shared-path guard must +// compare canonicalized paths, so a trailing slash or "." segment on one +// record doesn't let a live session's worktree slip through as "not shared". +func TestCleanup_LiveWorkspaceGuardNormalizesPaths(t *testing.T) { + m, st, _, ws := newManager() + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/shared/"}) + live := mkLive("mer-2") + live.Metadata.WorkspacePath = "/ws/./shared" + st.sessions["mer-2"] = live + + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Cleaned) != 0 || len(res.Skipped) != 1 || res.Skipped[0].SessionID != "mer-1" { + t.Fatalf("cleaned = %v, skipped = %v; want mer-1 skipped, none cleaned", res.Cleaned, res.Skipped) + } + if ws.destroyed != 0 { + t.Fatalf("destroyed = %d, want 0", ws.destroyed) + } +} + +// TestCleanup_ReclaimsUnsharedWhileSkippingShared: a shared-path skip must not +// stop cleanup from reclaiming an adjacent terminated workspace that no live +// session references. The orchestrator's persistent worktree (shared across +// respawn) is the motivating case for the skip. +func TestCleanup_ReclaimsUnsharedWhileSkippingShared(t *testing.T) { + m, st, _, ws := newManager() + // Terminated orchestrator predecessor shares its persistent worktree with + // the live orchestrator successor. + orchTerm := domain.SessionRecord{ID: "mer-orch-1", ProjectID: "mer", Kind: domain.KindOrchestrator, Metadata: domain.SessionMetadata{WorkspacePath: "/ws/orchestrator"}, IsTerminated: true, Activity: domain.Activity{State: domain.ActivityExited}} + st.sessions["mer-orch-1"] = orchTerm + orchLive := domain.SessionRecord{ID: "mer-orch-2", ProjectID: "mer", Kind: domain.KindOrchestrator, Metadata: domain.SessionMetadata{WorkspacePath: "/ws/orchestrator"}, Activity: domain.Activity{State: domain.ActivityActive}} + st.sessions["mer-orch-2"] = orchLive + // An unrelated terminated worker whose workspace nobody else uses. + seedTerminal(st, "mer-3", domain.SessionMetadata{WorkspacePath: "/ws/mer-3"}) + + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Cleaned) != 1 || res.Cleaned[0] != "mer-3" { + t.Fatalf("cleaned = %v, want [mer-3]", res.Cleaned) + } + if len(res.Skipped) != 1 || res.Skipped[0].SessionID != "mer-orch-1" { + t.Fatalf("skipped = %v, want mer-orch-1", res.Skipped) + } + if ws.destroyed != 1 { + t.Fatalf("destroyed = %d, want 1 (only the unshared worker workspace)", ws.destroyed) + } +} + // TestCleanup_ReportsSkippedWorkspaces: a refused teardown must be visible in // the result with a reason — a silent skip leaves users staring at // "Would clean N … 0 sessions cleaned" with no explanation.