From 504c6eae87346e0816a614a0ff8b4c59d7870095 Mon Sep 17 00:00:00 2001 From: Anuj Date: Mon, 20 Jul 2026 15:08:49 +0530 Subject: [PATCH 1/4] fix(review): destroy the reviewer pane on cancel instead of only interrupting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AO reviewer runs as a stable pane ("review-"+workerID) reused across review passes, but Cancel only interrupted it — so a cancelled reviewer left a live pane and agent process running (the #2715 leak family). Add a real teardown: extend the launcher with Teardown(handleID) over the runtime's Destroy, expose Engine.TeardownReviewer(workerID) (idempotent, serialised via lockWorker so it can't race a concurrent respawn), and call it from Cancel so the pane is destroyed rather than merely interrupted. Reuses the reviewer-pane Destroy seam added for harness-respawn. --- backend/internal/review/launcher.go | 19 +++++++++++ backend/internal/review/launcher_test.go | 21 +++++++++++++ backend/internal/review/review.go | 26 ++++++++++++++- backend/internal/review/review_test.go | 40 ++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go index 65f783eaa9..f4446e3972 100644 --- a/backend/internal/review/launcher.go +++ b/backend/internal/review/launcher.go @@ -37,6 +37,10 @@ type Launcher interface { Alive(ctx context.Context, handleID string) (bool, error) // Cancel interrupts a running reviewer pane while keeping the terminal alive. Cancel(ctx context.Context, handleID string, harness domain.ReviewerHarness) error + // Teardown destroys a reviewer pane so a cancelled or ended reviewer does not + // leak a live pane. Idempotent: an empty handle or an already-gone pane is a + // no-op. + Teardown(ctx context.Context, handleID string) error } // LaunchSpec is the engine's request to (re)launch a reviewer for one pass. @@ -301,3 +305,18 @@ func (l *agentLauncher) Cancel(ctx context.Context, handleID string, harness dom return fmt.Errorf("reviewer adapter %q returned unsupported cancel mode %q", harness, spec.Mode) } } + +// Teardown destroys the reviewer pane for handleID. Unlike Cancel (which only +// interrupts and keeps the pane alive), this removes the pane entirely so a +// cancelled or ended reviewer does not leak a live pane + agent process. It is +// idempotent: an empty handle is a no-op, and runtime.Destroy returns nil for an +// already-gone pane. +func (l *agentLauncher) Teardown(ctx context.Context, handleID string) error { + if handleID == "" { + return nil + } + if err := l.runtime.Destroy(ctx, ports.RuntimeHandle{ID: handleID}); err != nil { + return fmt.Errorf("teardown reviewer pane %q: %w", handleID, err) + } + return nil +} diff --git a/backend/internal/review/launcher_test.go b/backend/internal/review/launcher_test.go index d344363b9c..5fb597e791 100644 --- a/backend/internal/review/launcher_test.go +++ b/backend/internal/review/launcher_test.go @@ -287,6 +287,27 @@ func TestLauncherAlive(t *testing.T) { } } +func TestLauncherTeardownDestroysPane(t *testing.T) { + rt := &fakeRuntime{} + l := NewLauncher(fakeReviewerResolver{ok: true}, rt) + if err := l.Teardown(context.Background(), "review-mer-1"); err != nil { + t.Fatalf("Teardown: %v", err) + } + if rt.destroyed != "review-mer-1" { + t.Fatalf("destroyed handle = %q, want review-mer-1", rt.destroyed) + } + + // An empty handle is a no-op (nothing to destroy). + rtEmpty := &fakeRuntime{} + lEmpty := NewLauncher(fakeReviewerResolver{ok: true}, rtEmpty) + if err := lEmpty.Teardown(context.Background(), ""); err != nil { + t.Fatalf("empty-handle Teardown: %v", err) + } + if rtEmpty.destroyed != "" { + t.Fatalf("empty handle should not Destroy, got %q", rtEmpty.destroyed) + } +} + func TestLauncherCancelUsesReviewerCancelMode(t *testing.T) { reviewer := &fakeCancellableReviewer{interrupts: 2} rt := &fakeRuntime{} diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go index 4596de4b71..692a6c76c8 100644 --- a/backend/internal/review/review.go +++ b/backend/internal/review/review.go @@ -396,7 +396,8 @@ func (e *Engine) List(ctx stdctx.Context, workerID domain.SessionID) (SessionRev return SessionReviews{ReviewerHandleID: handle, Runs: runs, Reviews: Plan(prs, runs)}, nil } -// Cancel interrupts the live reviewer pane for a worker and marks running +// Cancel stops the live reviewer for a worker: it interrupts the pane, tears it +// down so a cancelled reviewer does not leak a live pane, and marks running // review runs as cancelled so they no longer block a fresh trigger. func (e *Engine) Cancel(ctx stdctx.Context, workerID domain.SessionID) (CancelResult, error) { if workerID == "" { @@ -422,6 +423,13 @@ func (e *Engine) Cancel(ctx stdctx.Context, workerID domain.SessionID) (CancelRe return CancelResult{}, err } } + // Destroy the pane, not just interrupt it: a cancelled reviewer should not + // leave "review-"+workerID (and its agent process) running. Idempotent and + // serialised against a concurrent Trigger respawn via lockWorker inside + // TeardownReviewer. + if err := e.TeardownReviewer(ctx, workerID); err != nil { + return CancelResult{}, err + } if _, err := e.store.CancelRunningReviewRunsBySession(ctx, workerID, "cancelled by user"); err != nil { return CancelResult{}, err } @@ -444,6 +452,22 @@ func (e *Engine) Cancel(ctx stdctx.Context, workerID domain.SessionID) (CancelRe return CancelResult{ReviewerHandleID: review.ReviewerHandleID, Reviews: Plan(prs, runs), CancelledRuns: cancelled}, nil } +// TeardownReviewer destroys a worker's reviewer pane so a cancelled or ended +// reviewer does not leak a live pane. The reviewer pane ("review-"+workerID) is +// stable per worker and reused across passes, so nothing else tears it down; +// without this, an explicit cancel (or, wired later, a terminated worker) leaves +// the pane and its agent process running. It is idempotent — no reviewer / an +// already-gone pane is a no-op — and serialises against Trigger via lockWorker so +// it cannot race a concurrent (re)spawn on the same deterministic handle. +func (e *Engine) TeardownReviewer(ctx stdctx.Context, workerID domain.SessionID) error { + if workerID == "" { + return fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + unlock := e.lockWorker(workerID) + defer unlock() + return e.launcher.Teardown(ctx, reviewerHandleID(workerID)) +} + // reviewerHarness resolves which harness reviews the worker's PR: a configured // reviewer wins, otherwise worker's own harness is reused when it is a // supported reviewer, otherwise fallback to claude-code. diff --git a/backend/internal/review/review_test.go b/backend/internal/review/review_test.go index b28c46fe4d..dcb2e282f5 100644 --- a/backend/internal/review/review_test.go +++ b/backend/internal/review/review_test.go @@ -158,6 +158,9 @@ type fakeLauncher struct { cancelled bool cancelErr error aliveErr error + tornDown bool + tornDownHandle string + teardownErr error gotSpec LaunchSpec gotHandle string cancelledHandle string @@ -199,6 +202,11 @@ func (f *fakeLauncher) Preflight(_ context.Context, _ domain.ReviewerHarness, _ f.preflighted = true return f.preflightErr } +func (f *fakeLauncher) Teardown(_ context.Context, handleID string) error { + f.tornDown = true + f.tornDownHandle = handleID + return f.teardownErr +} func liveWorker() domain.SessionRecord { return domain.SessionRecord{ @@ -275,6 +283,11 @@ func TestCancelInterruptsReviewerAndCancelsRunningRuns(t *testing.T) { if launcher.cancelledHarness != domain.ReviewerCodex { t.Fatalf("cancel harness = %q, want codex", launcher.cancelledHarness) } + // The pane is torn down, not merely interrupted — a cancelled reviewer must + // not leak a live pane. + if !launcher.tornDown || launcher.tornDownHandle != "review-mer-1" { + t.Fatalf("reviewer pane not torn down: tornDown=%v handle=%q", launcher.tornDown, launcher.tornDownHandle) + } if len(res.CancelledRuns) != 1 || res.CancelledRuns[0].ID != "run-1" { t.Fatalf("cancelled runs = %+v", res.CancelledRuns) } @@ -337,6 +350,33 @@ func TestCancelKeepsRunsRunningWhenReviewerCancelFailsAndHandleIsAlive(t *testin } } +func TestTeardownReviewerDestroysPaneIdempotently(t *testing.T) { + launcher := &fakeLauncher{} + eng := newEngineForTest(&fakeStore{}, fakeSessions{}, fakePRs{}, fakeProjects{}, launcher) + + // Tears down the deterministic "review-"+workerID pane. + if err := eng.TeardownReviewer(context.Background(), "mer-1"); err != nil { + t.Fatalf("TeardownReviewer: %v", err) + } + if !launcher.tornDown || launcher.tornDownHandle != "review-mer-1" { + t.Fatalf("pane not torn down: tornDown=%v handle=%q", launcher.tornDown, launcher.tornDownHandle) + } + + // Idempotent: a second teardown of an already-gone pane still succeeds. + launcher.tornDown = false + if err := eng.TeardownReviewer(context.Background(), "mer-1"); err != nil { + t.Fatalf("second TeardownReviewer: %v", err) + } + if !launcher.tornDown { + t.Fatal("second teardown should still reach the launcher") + } + + // A missing worker id is rejected as invalid input. + if err := eng.TeardownReviewer(context.Background(), ""); !errors.Is(err, ErrInvalid) { + t.Fatalf("empty worker id err = %v, want ErrInvalid", err) + } +} + func TestTriggerConcurrentSameWorkerSpawnsOnce(t *testing.T) { store := &fakeStore{} launcher := &fakeLauncher{handle: "review-mer-1"} From 2f646e81dc9c6a384455b7e2ae0618c967c89a6e Mon Sep 17 00:00:00 2001 From: Anuj Date: Fri, 24 Jul 2026 00:22:20 +0530 Subject: [PATCH 2/4] feat(sessions): terminal-resource reconciler for terminated sessions (#2811) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit State-driven, idempotent reconciler that converges every terminated session's runtime + workspace toward safe release (supersedes the one-shot callback in workspace, or records why it can't (dirty worktree, transient retry, exhausted). - Shared release core (runtime-first; continue-past-dirty so a dirty child can't strand clean siblings; structured disposition rollup). - FinalizeTerminalSession: idempotent, under a refcounted per-session lock; skips restore-pending sessions; generation guard drops a stale result across a Restore→Terminate cycle; capped-backoff retry + attempt cap (→ failed). - The per-session lock is also taken in Spawn, RestoreWithMode and per session in RestoreAll (restoreSavedSession), so a finalize can't git-worktree-remove out from under a concurrent restore/kill — the generation re-check then aborts the release under the lock. - Kill + bulk Cleanup funnel through the core and persist facts on every terminal path (incl. no-workspace → not_applicable); both share the idempotent-skip. Cleanup keeps reclaiming unresumable restore-pending leftovers (explicit user intent), unlike the automatic reconciler. - Reconciler runner (internal/observe/reconciler): live CDC session_updated wake (non-blocking, isTerminated-filtered so a facts-write can't self-wake it), sessions-driven boot scan for the leaked backlog, periodic retry sweep. Attempts run on a detached base context force-cancelled after the shutdown grace and joined before exit — bounded shutdown, no write-after-store.Close. - Never reclaim a worktree with a live reviewer: tear the pane down + gate on its IsAlive (review engine gains ReviewerAlive; manager takes an optional reviewer). - MarkSpawned bumps sessions.cleanup_generation in the same UPDATE that un-terminates. reconcileReap deleted (subsumed by the finalizer boot pass). - Boot: run Reconcile/RestoreAll to completion, then start the reconciler (subscribes before its boot snapshot); its done-channel drains before cdcPipe. Tests: finalizer clean/dirty/runtime-fail-retry/exhaustion/idempotent/ generation-guard/not-applicable/reviewer-gate; workspace-project mixed clean+dirty; Kill writes facts; restore-vs-finalize and Kill-vs-finalize serialization (verified they fail without the lock); reconciler live-wake + boot-scan + facts-only-ignore; and a wake-path e2e through the real CDC chain. --- backend/internal/daemon/daemon.go | 18 +- backend/internal/daemon/lifecycle_wiring.go | 44 +- backend/internal/daemon/wiring_test.go | 4 + .../integration/lifecycle_sqlite_test.go | 19 +- backend/internal/lifecycle/manager.go | 8 + .../internal/observe/reconciler/reconciler.go | 256 +++++++++ .../observe/reconciler/reconciler_test.go | 189 +++++++ backend/internal/review/launcher_test.go | 4 +- backend/internal/review/review.go | 15 + backend/internal/session_manager/finalize.go | 373 +++++++++++++ .../internal/session_manager/finalize_test.go | 388 +++++++++++++ backend/internal/session_manager/locks.go | 53 ++ backend/internal/session_manager/manager.go | 523 +++++++++--------- .../internal/session_manager/manager_test.go | 176 +++--- 14 files changed, 1699 insertions(+), 371 deletions(-) create mode 100644 backend/internal/observe/reconciler/reconciler.go create mode 100644 backend/internal/observe/reconciler/reconciler_test.go create mode 100644 backend/internal/session_manager/finalize.go create mode 100644 backend/internal/session_manager/finalize_test.go create mode 100644 backend/internal/session_manager/locks.go diff --git a/backend/internal/daemon/daemon.go b/backend/internal/daemon/daemon.go index 19294ae510..d173d1f92a 100644 --- a/backend/internal/daemon/daemon.go +++ b/backend/internal/daemon/daemon.go @@ -22,6 +22,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/httpd/controllers" "github.com/aoagents/agent-orchestrator/backend/internal/mobilebridge" "github.com/aoagents/agent-orchestrator/backend/internal/notify" + "github.com/aoagents/agent-orchestrator/backend/internal/observe/reconciler" "github.com/aoagents/agent-orchestrator/backend/internal/ports" "github.com/aoagents/agent-orchestrator/backend/internal/preview" "github.com/aoagents/agent-orchestrator/backend/internal/push" @@ -265,9 +266,10 @@ func Run() error { } // Reconcile sessions on boot: adopt crash-surviving runtimes, capture and - // terminate dead ones, reap leaked tmux, then restore shutdown-saved - // sessions. Best-effort: a failure is logged but never blocks boot. Placed - // before srv.Run so sessions are consistent before the server serves. + // terminate dead ones, then restore shutdown-saved sessions. Run to + // completion (synchronous, as before) so restore-pending sessions are settled + // before the terminal-resource reconciler starts and before the server + // serves. Best-effort: a failure is logged but never blocks boot. if reconcileErr := sessMgr.Reconcile(ctx); reconcileErr != nil { log.Error("reconcile sessions on boot failed", "err", reconcileErr) } @@ -281,6 +283,16 @@ func Run() error { // the recovery sweep is the backstop if it does not finish before shutdown. go lcStack.LCM.DispatchAllPendingWorkerIdleEvents(ctx) + // Start the terminal-resource reconciler AFTER Reconcile/RestoreAll has + // settled: it subscribes to the CDC session_updated wake, then drains the + // sessions-driven candidate backlog (the pre-existing leaked worktrees #2811 + // is about) onto its async worker, and retries capped-backoff pending + // cleanups on a sweep. Subscribing before its own boot snapshot closes the + // boot-window race; draining is async so a large backlog never delays serving. + // Its done-channel joins lcStack.Stop so it unsubscribes before cdcPipe.Stop. + termReconciler := reconciler.New(sessMgr, store, cdcPipe.Broadcaster, reconciler.Config{Logger: log}) + lcStack.reconcilerDone = termReconciler.Start(ctx) + // ponytail: 5s tolerates a brief frontend restart; tune if dev hot-reload trips it. const supervisorGrace = 5 * time.Second diff --git a/backend/internal/daemon/lifecycle_wiring.go b/backend/internal/daemon/lifecycle_wiring.go index c7d508a9d3..54ef90a7eb 100644 --- a/backend/internal/daemon/lifecycle_wiring.go +++ b/backend/internal/daemon/lifecycle_wiring.go @@ -43,6 +43,11 @@ type lifecycleStack struct { scmDone <-chan struct{} trackerDone <-chan struct{} sweepDone <-chan struct{} + // reconcilerDone is the terminal-resource reconciler's drain channel. Assigned + // after boot Reconcile completes (daemon.go); nil until then, so Stop must + // nil-guard it. It is drained BEFORE the CDC pipe stops so the reconciler + // unsubscribes before a late Publish could send on its closed queue. + reconcilerDone <-chan struct{} } // workerIdleSweepInterval is the low-frequency recovery cadence that redelivers @@ -127,6 +132,11 @@ func (l *lifecycleStack) Stop() { if l.trackerDone != nil { <-l.trackerDone } + // Drained before the CDC pipe stops (daemon shutdown order) so the reconciler + // unsubscribes before a late Publish could send on its closed queue. + if l.reconcilerDone != nil { + <-l.reconcilerDone + } } // sessionLifecycle is the narrow surface of sessionmanager.Manager used for @@ -141,6 +151,10 @@ type sessionLifecycle interface { Reconcile(ctx context.Context) error RestoreAll(ctx context.Context) error Kill(ctx context.Context, id domain.SessionID) (bool, error) + // FinalizeTerminalSession is the terminal-resource reconciler's per-session + // unit of work (release a terminated session's runtime + workspace). Exposed + // here so the daemon can hand the manager to the reconciler runner. + FinalizeTerminalSession(ctx context.Context, id domain.SessionID) error } // startSession builds the controller-facing session service: a session manager @@ -172,6 +186,21 @@ func startSession(cfg config.Config, runtime runtimeselect.Runtime, store *sqlit Scratch: scratchWS, Projects: store, }) + // Build the review engine before the session manager so the manager can gate + // worktree reclaim on a worker's reviewer pane (destroy it + confirm it's gone + // before removing the worktree). The engine depends only on the store/launcher, + // not on the manager, so there is no construction cycle. + reviewers, err := reviewer.NewResolver() + if err != nil { + return nil, nil, nil, fmt.Errorf("reviewer resolver: %w", err) + } + reviewEngine := reviewcore.New(reviewcore.Deps{ + Store: store, + Sessions: store, + PRs: store, + Projects: store, + Launcher: reviewcore.NewLauncher(reviewers, runtime, cfg.DataDir), + }) mgr := sessionmanager.New(sessionmanager.Deps{ Runtime: runtime, Agents: agents, @@ -179,6 +208,7 @@ func startSession(cfg config.Config, runtime runtimeselect.Runtime, store *sqlit Store: store, Messenger: messenger, Lifecycle: lcm, + Reviewer: reviewEngine, DataDir: cfg.DataDir, Logger: log, }) @@ -213,18 +243,8 @@ func startSession(cfg config.Config, runtime runtimeselect.Runtime, store *sqlit // Triggering a review spawns a reviewer over the worker's worktree, resolved // from the reviewer registry (distinct from the worker agent set). The // reviewer posts its review to the PR itself, so the service needs no SCM - // writer. - reviewers, err := reviewer.NewResolver() - if err != nil { - return nil, nil, nil, fmt.Errorf("reviewer resolver: %w", err) - } - reviewEngine := reviewcore.New(reviewcore.Deps{ - Store: store, - Sessions: store, - PRs: store, - Projects: store, - Launcher: reviewcore.NewLauncher(reviewers, runtime, cfg.DataDir), - }) + // writer. The engine itself was built above (the manager reuses it for the + // reviewer-pane teardown gate). reviewSvc := reviewsvc.New(reviewEngine, store, reviewsvc.WithLifecycleReducer(lcm)) return sessionSvc, reviewSvc, mgr, nil } diff --git a/backend/internal/daemon/wiring_test.go b/backend/internal/daemon/wiring_test.go index af2e05197b..5638f02f12 100644 --- a/backend/internal/daemon/wiring_test.go +++ b/backend/internal/daemon/wiring_test.go @@ -633,6 +633,10 @@ func (f *fakeSessionLifecycle) RestoreAll(_ context.Context) error { return f.restoreErr } +func (f *fakeSessionLifecycle) FinalizeTerminalSession(_ context.Context, _ domain.SessionID) error { + return nil +} + // TestWiring_SessionLifecycleInterfaceInvokedByDaemon asserts the // sessionLifecycle interface is satisfied by *sessionmanager.Manager (compile // check) and that Reconcile and RestoreAll dispatch correctly through the diff --git a/backend/internal/integration/lifecycle_sqlite_test.go b/backend/internal/integration/lifecycle_sqlite_test.go index 5a742758c7..73aad6eab0 100644 --- a/backend/internal/integration/lifecycle_sqlite_test.go +++ b/backend/internal/integration/lifecycle_sqlite_test.go @@ -260,7 +260,7 @@ func TestRestoreRoundTripPreservesMetadata(t *testing.T) { // is_terminated=true, runtime.Create count stays 0. // - Session B: is_terminated=1 but its runtime is still ALIVE (leaked teardown) // => Reconcile must call Destroy on its handle. -func TestReconcile_TerminatesDeadLiveSessionAndReapsLeakedTmux(t *testing.T) { +func TestReconcile_TerminatesDeadLiveSession_FinalizerReapsLeakedRuntime(t *testing.T) { ctx := context.Background() st := newStack(t) @@ -341,9 +341,22 @@ func TestReconcile_TerminatesDeadLiveSessionAndReapsLeakedTmux(t *testing.T) { t.Fatalf("want 0 runtime Creates (promptless worker must not relaunch), got %d", st.rt.created) } - // Session B's leaked runtime must have been destroyed. + // Reconcile no longer reaps a terminated session's leaked runtime: the old + // reconcileReap pass was deleted because it is a strict subset of the + // terminal-resource reconciler's runtime-first release. So session B's leaked + // runtime is still alive right after Reconcile. + if st.rt.wasDestroyed("hdl-B") { + t.Fatalf("session B: Reconcile must NOT reap the leaked runtime (that is now the finalizer's job); destroyed handles: %v", st.rt.destroyedHandles) + } + + // FinalizeTerminalSession (the reconciler's per-session unit of work, driven at + // boot by the sessions-driven candidate scan) now owns reclaiming the leaked + // runtime — runtime-first, so hdl-B is destroyed before any workspace teardown. + if err := st.mgr.FinalizeTerminalSession(ctx, recB.ID); err != nil { + t.Fatalf("FinalizeTerminalSession(B): %v", err) + } if !st.rt.wasDestroyed("hdl-B") { - t.Fatalf("session B: want Destroy called for handle hdl-B; destroyed handles: %v", st.rt.destroyedHandles) + t.Fatalf("session B: want the finalizer to Destroy leaked handle hdl-B; destroyed handles: %v", st.rt.destroyedHandles) } } diff --git a/backend/internal/lifecycle/manager.go b/backend/internal/lifecycle/manager.go index cde6b897ff..4b7d213bd9 100644 --- a/backend/internal/lifecycle/manager.go +++ b/backend/internal/lifecycle/manager.go @@ -801,6 +801,14 @@ func (m *Manager) MarkSpawned(ctx context.Context, id domain.SessionID, metadata } now := m.clock() rec.IsTerminated = false + // Bump the cleanup generation in the same UPDATE that un-terminates the + // session. The terminal-resource reconciler stamps each cleanup-facts row + // with the generation it observed; comparing that against this authoritative + // counter is what stops a finalize begun under an earlier terminal episode + // from satisfying a later Restore→Terminate cycle. Keeping the bump inline + // here (rather than in a shared UpdateSession) keeps it atomic-with the + // un-terminate and ensures only spawn/restore advances it. + rec.CleanupGeneration++ rec.Activity = domain.Activity{State: domain.ActivityIdle, LastActivityAt: now} // Each spawn/restore must re-prove its hook pipeline: clear the receipt so // a relaunch with broken hooks degrades to no_signal instead of inheriting diff --git a/backend/internal/observe/reconciler/reconciler.go b/backend/internal/observe/reconciler/reconciler.go new file mode 100644 index 0000000000..b13621e110 --- /dev/null +++ b/backend/internal/observe/reconciler/reconciler.go @@ -0,0 +1,256 @@ +// Package reconciler drives the terminal-resource reconciler: a state-driven, +// idempotent loop that converges every terminated session's runtime + workspace +// toward safe release. It is the mirror image of observe/reaper — the reaper +// skips terminated sessions, the reconciler processes ONLY them. +// +// The durable invariant it maintains: is_terminated=true means the session no +// longer wants runtime resources, so the daemon must release them (or record why +// it can't: a dirty worktree, a transient failure being retried, or an exhausted +// failure). Terminal state is the durable intent; it does not promise cleanup +// already succeeded. +// +// Three drivers feed one async worker: +// - Live wake: a CDC session_updated subscription enqueues a session the moment +// its is_terminated flips true (the immediate first attempt). +// - Boot scan: a sessions-driven candidate scan at start drains the pre-existing +// leaked backlog (terminated sessions with no facts row). +// - Periodic sweep: the same candidate scan on a ticker retries capped-backoff +// pending cleanups whose next_attempt_at is due. +package reconciler + +import ( + "context" + "encoding/json" + "log/slog" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/cdc" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// Default tunables. +const ( + // DefaultSweepInterval is how often the periodic candidate scan runs to pick + // up capped-backoff retries and any session missed by a dropped live wake. + DefaultSweepInterval = 30 * time.Second + // DefaultQueueSize bounds the enqueue channel. A live wake that finds it full + // is dropped (the scan is the durable backstop), so this only needs to absorb + // bursts, not the whole backlog. + DefaultQueueSize = 256 + // DefaultAttemptTimeout bounds one finalize attempt. It runs on a detached + // context so a slow `git worktree remove` is not interrupted by daemon + // shutdown, yet cannot hang forever. + DefaultAttemptTimeout = 2 * time.Minute + // DefaultShutdownGrace is how long Stop waits for an in-flight attempt to + // finish before returning. Independent of AttemptTimeout so a generous + // per-attempt bound can't stall daemon exit. + DefaultShutdownGrace = 5 * time.Second +) + +// finalizer releases one terminated session's resources. Implemented by +// *sessionmanager.Manager.FinalizeTerminalSession. +type finalizer interface { + FinalizeTerminalSession(ctx context.Context, id domain.SessionID) error +} + +// candidateSource is the sessions-driven candidate scan. Implemented by the +// sqlite store's ListTerminalCleanupCandidates. +type candidateSource interface { + ListTerminalCleanupCandidates(ctx context.Context, now time.Time) ([]domain.SessionID, error) +} + +// subscriber is the CDC broadcaster surface used for the live wake. Implemented +// by *cdc.Broadcaster. +type subscriber interface { + Subscribe(fn func(cdc.Event)) (unsubscribe func()) +} + +// Config holds the reconciler's tunables; the zero value is filled with defaults. +type Config struct { + SweepInterval time.Duration + QueueSize int + AttemptTimeout time.Duration + ShutdownGrace time.Duration + Clock func() time.Time + Logger *slog.Logger +} + +// Reconciler runs the terminal-resource reconciliation loop. +type Reconciler struct { + finalizer finalizer + candidates candidateSource + events subscriber + + queue chan domain.SessionID + unsub func() + // attemptBase is the detached base context for finalize attempts: it survives + // parent (daemon) cancellation so a git op isn't interrupted the instant + // shutdown begins, but is force-cancelled after the shutdown grace so an + // in-flight attempt can't keep writing to a store the daemon is about to close. + attemptBase context.Context + attemptCancel context.CancelFunc + + sweep time.Duration + attemptTimeout time.Duration + grace time.Duration + clock func() time.Time + logger *slog.Logger +} + +// New builds a Reconciler, defaulting any unset Config field. +func New(f finalizer, candidates candidateSource, events subscriber, cfg Config) *Reconciler { + r := &Reconciler{ + finalizer: f, + candidates: candidates, + events: events, + sweep: cfg.SweepInterval, + attemptTimeout: cfg.AttemptTimeout, + grace: cfg.ShutdownGrace, + clock: cfg.Clock, + logger: cfg.Logger, + } + if r.sweep <= 0 { + r.sweep = DefaultSweepInterval + } + if r.attemptTimeout <= 0 { + r.attemptTimeout = DefaultAttemptTimeout + } + if r.grace <= 0 { + r.grace = DefaultShutdownGrace + } + if r.clock == nil { + r.clock = time.Now + } + if r.logger == nil { + r.logger = slog.Default() + } + qs := cfg.QueueSize + if qs <= 0 { + qs = DefaultQueueSize + } + r.queue = make(chan domain.SessionID, qs) + return r +} + +// Start registers the live-wake subscription, then launches the run loop and +// returns a done-channel closed once the loop has drained on ctx cancellation. +// Subscribing here — BEFORE run takes the boot-scan snapshot — closes the boot +// window: a restore-then-exit session can't slip between the snapshot and the +// live wake (an overlap is harmless because finalize is idempotent). +func (r *Reconciler) Start(ctx context.Context) <-chan struct{} { + r.unsub = r.events.Subscribe(r.onEvent) + // Detached from ctx on purpose (see field doc); cancelled by run after the + // shutdown grace, and always released via defer in run. + r.attemptBase, r.attemptCancel = context.WithCancel(context.Background()) //nolint:gosec // G118: detached attempt context is intentional + done := make(chan struct{}) + go r.run(ctx, done) + return done +} + +func (r *Reconciler) run(ctx context.Context, done chan<- struct{}) { + defer close(done) + // Always release the detached base so it can't leak past shutdown. + defer r.attemptCancel() + + workerDone := make(chan struct{}) + go r.worker(workerDone) + + // Boot scan: drain the backlog onto the worker (idempotent with any live wake + // that also fires for the same session). + r.scan(ctx) + + t := time.NewTicker(r.sweep) + defer t.Stop() + for { + select { + case <-ctx.Done(): + // Stop new live wakes BEFORE closing the queue, else a late Publish + // would send on a closed channel and panic. Unsubscribe blocks until any + // in-flight Publish callback returns, so no onEvent can race the close. + r.unsub() + close(r.queue) + // Wait a short bounded grace for the worker to finish cleanly. If it + // doesn't (a slow git op on the detached context), force-cancel the + // attempt base and JOIN the worker before returning: this both bounds + // shutdown (we don't wait the full per-attempt timeout) AND guarantees + // the worker has stopped touching the store before the daemon closes it, + // closing the write-after-close window. + select { + case <-workerDone: + case <-time.After(r.grace): + r.logger.Warn("reconciler: shutdown grace elapsed, cancelling in-flight cleanup") + r.attemptCancel() + <-workerDone + } + return + case <-t.C: + r.scan(ctx) + } + } +} + +// worker drains the queue, finalizing one session at a time. Serial processing +// keeps concurrent finalizes off the same session (the manager also holds a +// per-session lock) and bounds resource use during a large boot drain. +func (r *Reconciler) worker(done chan<- struct{}) { + defer close(done) + for id := range r.queue { + // A fresh bounded context per attempt, derived from the detached + // attemptBase: it survives daemon-shutdown cancellation (so a git op isn't + // interrupted mid-write) yet cannot hang forever, and run force-cancels + // attemptBase after the shutdown grace so this can't outlive store.Close. + attemptCtx, cancel := context.WithTimeout(r.attemptBase, r.attemptTimeout) + if err := r.finalizer.FinalizeTerminalSession(attemptCtx, id); err != nil { + r.logger.Error("reconciler: finalize failed", "sessionID", id, "err", err) + } + cancel() + } +} + +// scan enqueues every current cleanup candidate. It blocks on a full queue +// (draining as the worker makes room) but unblocks promptly on ctx cancellation, +// so a large backlog neither drops work nor stalls shutdown. +func (r *Reconciler) scan(ctx context.Context) { + ids, err := r.candidates.ListTerminalCleanupCandidates(ctx, r.clock()) + if err != nil { + r.logger.Error("reconciler: candidate scan failed", "err", err) + return + } + for _, id := range ids { + select { + case r.queue <- id: + case <-ctx.Done(): + return + } + } +} + +// onEvent is the live-wake callback. It runs inline on the CDC poller goroutine +// under the broadcaster's RLock, so it MUST NOT block: it does a non-blocking +// send and drops on a full queue (the scan is the backstop). It filters on the +// payload's isTerminated so it only wakes on a genuine terminal transition and +// never does a DB read on the hot path. Facts-trigger events carry {id} only (no +// isTerminated), so they are ignored here and serve purely as the frontend +// refetch nudge — which is also why a failed/pending retry write can't self-wake +// the reconciler and hot-loop. +func (r *Reconciler) onEvent(e cdc.Event) { + if e.Type != cdc.EventSessionUpdated { + return + } + var p struct { + ID string `json:"id"` + IsTerminated bool `json:"isTerminated"` + } + if err := json.Unmarshal(e.Payload, &p); err != nil { + return + } + if p.ID == "" || !p.IsTerminated { + return + } + select { + case r.queue <- domain.SessionID(p.ID): + default: + // Never block the broadcaster (it stalls SSE + terminal fanout too). + r.logger.Warn("reconciler: enqueue full, dropping live wake; scan will retry", "sessionID", p.ID) + } +} diff --git a/backend/internal/observe/reconciler/reconciler_test.go b/backend/internal/observe/reconciler/reconciler_test.go new file mode 100644 index 0000000000..90d64f19d1 --- /dev/null +++ b/backend/internal/observe/reconciler/reconciler_test.go @@ -0,0 +1,189 @@ +package reconciler_test + +import ( + "context" + "encoding/json" + "sync" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/cdc" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/observe/reconciler" + "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite" +) + +// --- fakes --- + +type fakeFinalizer struct { + called chan domain.SessionID +} + +func (f *fakeFinalizer) FinalizeTerminalSession(_ context.Context, id domain.SessionID) error { + f.called <- id + return nil +} + +type fakeCandidates struct { + mu sync.Mutex + ids []domain.SessionID +} + +func (f *fakeCandidates) ListTerminalCleanupCandidates(_ context.Context, _ time.Time) ([]domain.SessionID, error) { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]domain.SessionID, len(f.ids)) + copy(out, f.ids) + return out, nil +} + +type fakeBroadcaster struct { + mu sync.Mutex + fn func(cdc.Event) +} + +func (b *fakeBroadcaster) Subscribe(fn func(cdc.Event)) func() { + b.mu.Lock() + b.fn = fn + b.mu.Unlock() + return func() { b.mu.Lock(); b.fn = nil; b.mu.Unlock() } +} + +func (b *fakeBroadcaster) publish(e cdc.Event) { + b.mu.Lock() + fn := b.fn + b.mu.Unlock() + if fn != nil { + fn(e) + } +} + +func sessionUpdated(id string, terminated bool) cdc.Event { + payload, _ := json.Marshal(map[string]any{"id": id, "isTerminated": terminated}) + return cdc.Event{Type: cdc.EventSessionUpdated, SessionID: id, Payload: payload} +} + +func awaitCall(t *testing.T, ch chan domain.SessionID, want domain.SessionID) { + t.Helper() + select { + case got := <-ch: + if got != want { + t.Fatalf("finalize called for %q, want %q", got, want) + } + case <-time.After(2 * time.Second): + t.Fatalf("timed out waiting for finalize(%q)", want) + } +} + +func expectNoCall(t *testing.T, ch chan domain.SessionID) { + t.Helper() + select { + case got := <-ch: + t.Fatalf("unexpected finalize for %q", got) + case <-time.After(150 * time.Millisecond): + } +} + +// --- unit tests (fakes) --- + +func TestLiveWake_EnqueuesTerminatedSession(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + fin := &fakeFinalizer{called: make(chan domain.SessionID, 4)} + bcast := &fakeBroadcaster{} + r := reconciler.New(fin, &fakeCandidates{}, bcast, reconciler.Config{}) + done := r.Start(ctx) + + bcast.publish(sessionUpdated("mer-1", true)) + awaitCall(t, fin.called, "mer-1") + + cancel() + <-done +} + +func TestLiveWake_IgnoresNonTerminalAndFactsOnlyEvents(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + fin := &fakeFinalizer{called: make(chan domain.SessionID, 4)} + bcast := &fakeBroadcaster{} + r := reconciler.New(fin, &fakeCandidates{}, bcast, reconciler.Config{}) + done := r.Start(ctx) + + // A live (non-terminal) session_updated must not wake the reconciler. + bcast.publish(sessionUpdated("mer-1", false)) + // A facts-trigger event carries {id} only (no isTerminated): it is the + // frontend refetch nudge and must NOT self-wake the reconciler (critique #14). + bcast.publish(cdc.Event{Type: cdc.EventSessionUpdated, SessionID: "mer-2", Payload: json.RawMessage(`{"id":"mer-2"}`)}) + expectNoCall(t, fin.called) + + cancel() + <-done +} + +func TestBootScan_EnqueuesBacklogCandidates(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + fin := &fakeFinalizer{called: make(chan domain.SessionID, 4)} + cands := &fakeCandidates{ids: []domain.SessionID{"mer-9"}} + r := reconciler.New(fin, cands, &fakeBroadcaster{}, reconciler.Config{}) + done := r.Start(ctx) + + awaitCall(t, fin.called, "mer-9") + + cancel() + <-done +} + +// --- wake-path e2e (real store + real CDC poller/broadcaster + reconciler) --- + +// TestWakePath_E2E drives convergence through the REAL session_updated CDC chain +// (store trigger → change_log → poller → broadcaster → reconciler subscription → +// finalizer), which unit-testing the finalizer in isolation cannot cover +// (critique #6). +func TestWakePath_E2E(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + store, err := sqlite.Open(t.TempDir()) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + + if err := store.UpsertProject(ctx, domain.ProjectRecord{ID: "mer", Path: "/tmp/mer", RegisteredAt: time.Now().UTC()}); err != nil { + t.Fatalf("seed project: %v", err) + } + rec, err := store.CreateSession(ctx, domain.SessionRecord{ + ProjectID: "mer", Kind: domain.KindWorker, Harness: domain.HarnessClaudeCode, + Activity: domain.Activity{State: domain.ActivityActive, LastActivityAt: time.Now().UTC()}, + CreatedAt: time.Now().UTC(), UpdatedAt: time.Now().UTC(), + }) + if err != nil { + t.Fatalf("create session: %v", err) + } + + // Real CDC pipe seeked to head, so only the future is_terminated flip is seen. + bcast := cdc.NewBroadcaster() + poller := cdc.NewPoller(store, bcast, cdc.PollerConfig{}) + if err := poller.SeekToHead(ctx); err != nil { + t.Fatalf("seek: %v", err) + } + pollerDone := poller.Start(ctx) + + fin := &fakeFinalizer{called: make(chan domain.SessionID, 4)} + r := reconciler.New(fin, store, bcast, reconciler.Config{}) + reconcilerDone := r.Start(ctx) + + // Flip is_terminated: the sessions_cdc_update trigger fans out a + // session_updated event carrying isTerminated=true. + rec.IsTerminated = true + if err := store.UpdateSession(ctx, rec); err != nil { + t.Fatalf("mark terminated: %v", err) + } + + awaitCall(t, fin.called, rec.ID) + + cancel() + <-reconcilerDone + <-pollerDone +} diff --git a/backend/internal/review/launcher_test.go b/backend/internal/review/launcher_test.go index 5fb597e791..9c6f7c9a9f 100644 --- a/backend/internal/review/launcher_test.go +++ b/backend/internal/review/launcher_test.go @@ -289,7 +289,7 @@ func TestLauncherAlive(t *testing.T) { func TestLauncherTeardownDestroysPane(t *testing.T) { rt := &fakeRuntime{} - l := NewLauncher(fakeReviewerResolver{ok: true}, rt) + l := NewLauncher(fakeReviewerResolver{ok: true}, rt, t.TempDir()) if err := l.Teardown(context.Background(), "review-mer-1"); err != nil { t.Fatalf("Teardown: %v", err) } @@ -299,7 +299,7 @@ func TestLauncherTeardownDestroysPane(t *testing.T) { // An empty handle is a no-op (nothing to destroy). rtEmpty := &fakeRuntime{} - lEmpty := NewLauncher(fakeReviewerResolver{ok: true}, rtEmpty) + lEmpty := NewLauncher(fakeReviewerResolver{ok: true}, rtEmpty, t.TempDir()) if err := lEmpty.Teardown(context.Background(), ""); err != nil { t.Fatalf("empty-handle Teardown: %v", err) } diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go index 692a6c76c8..7b895d286b 100644 --- a/backend/internal/review/review.go +++ b/backend/internal/review/review.go @@ -468,6 +468,21 @@ func (e *Engine) TeardownReviewer(ctx stdctx.Context, workerID domain.SessionID) return e.launcher.Teardown(ctx, reviewerHandleID(workerID)) } +// ReviewerAlive reports whether the worker's reviewer pane ("review-"+workerID) +// is still running. The terminal-resource reconciler calls this after +// TeardownReviewer to gate worktree removal on the pane's actual liveness rather +// than on a cancel/teardown return, so an in-flight reviewer in the worker's +// worktree is never corrupted by a reclaim. It serialises against Trigger via +// lockWorker, and reports not-alive for an unknown worker id. +func (e *Engine) ReviewerAlive(ctx stdctx.Context, workerID domain.SessionID) (bool, error) { + if workerID == "" { + return false, nil + } + unlock := e.lockWorker(workerID) + defer unlock() + return e.launcher.Alive(ctx, reviewerHandleID(workerID)) +} + // reviewerHarness resolves which harness reviews the worker's PR: a configured // reviewer wins, otherwise worker's own harness is reused when it is a // supported reviewer, otherwise fallback to claude-code. diff --git a/backend/internal/session_manager/finalize.go b/backend/internal/session_manager/finalize.go new file mode 100644 index 0000000000..40de722148 --- /dev/null +++ b/backend/internal/session_manager/finalize.go @@ -0,0 +1,373 @@ +package sessionmanager + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// reviewerTeardown is the narrow slice of the review engine the finalizer needs +// to keep an in-flight reviewer from being corrupted by a worktree reclaim. The +// AO reviewer runs as a separate pane ("review-"+workerID) inside the worker's +// own worktree, so removing that worktree under a live reviewer would break it. +// It is optional: nil disables the gate (e.g. in unit tests with no reviewer). +type reviewerTeardown interface { + // TeardownReviewer destroys the worker's reviewer pane; idempotent. + TeardownReviewer(ctx context.Context, workerID domain.SessionID) error + // ReviewerAlive reports whether the worker's reviewer pane is still running. + ReviewerAlive(ctx context.Context, workerID domain.SessionID) (bool, error) +} + +// Retry bounds for a transient teardown failure. The backoff caps the interval; +// a capped attempt COUNT is also enforced so a permanently-failing teardown +// (locked file, corrupt worktree registration) transitions to a terminal +// `failed` disposition instead of retrying forever with no end state. +const maxCleanupAttempts = 5 + +const ( + cleanupBackoffBase = 30 * time.Second + cleanupBackoffCap = 15 * time.Minute +) + +// Machine failure codes recorded on session_cleanup_facts.failure_code. +const ( + failRuntimeDestroy = "runtime_destroy" + failWorkspaceRows = "workspace_rows" + failWorkspaceDestroy = "workspace_destroy" + failReviewerAlive = "reviewer_alive" + failProjectUnresolvable = "project_unresolvable" + failTeardown = "teardown" +) + +// cleanupResult is the structured outcome of a single terminal-resource release +// attempt. It records durable facts only — the derived display status is never +// stored (hard rule). A "" disposition means the attempt aborted before it could +// classify the workspace (the accompanying error carries why). +type cleanupResult struct { + // disposition is the session-level rollup: removed / preserved_dirty / + // not_applicable, or pending when a transient failure needs a retry. + disposition domain.WorkspaceDisposition + // runtimeReleased is true only on a genuine runtime release (not a no-op skip + // on an empty/unrouted handle). + runtimeReleased bool + // workspaceProject is true for a multi-repo workspace-project session. + workspaceProject bool + // failureCode is a machine code for the last failure; "" when none. + failureCode string +} + +// releaseTerminalResources is the shared, idempotent release core that Kill, +// bulk Cleanup, and the reconciler's FinalizeTerminalSession all funnel through. +// Ordering is runtime-first: if the runtime cannot be destroyed the workspace is +// left untouched and the error is returned, so a worktree is never removed out +// from under a live process. Workspace teardown continues past a dirty child so +// one dirty worktree cannot strand its clean siblings. It returns a structured +// result plus a hard error for the two fail-closed cases (runtime destroy failed; +// non-dirty workspace teardown failed) — the caller decides whether to surface +// that (Kill) or fold it into a retry fact (the finalizer / Cleanup). +func (m *Manager) releaseTerminalResources(ctx context.Context, rec domain.SessionRecord) (cleanupResult, error) { + var res cleanupResult + + handle := runtimeHandle(rec.Metadata) + if handle.ID != "" { + if err := m.runtime.Destroy(ctx, handle); err != nil { + res.failureCode = failRuntimeDestroy + return res, fmt.Errorf("runtime: %w", err) + } + res.runtimeReleased = true + } + // handle.ID == "": an unrouted / legacy-backlog row. Destroy returns nil for + // an empty handle too, so treating that as proof of release would silently + // leave a live-but-unrouted runtime leaked. We do NOT claim release + // (runtimeReleased stays false) and still proceed to workspace cleanup; + // actively reconstructing the adapter's deterministic session name to probe + // such a runtime is a cross-adapter seam left as follow-up (ties to #1458). + + rows, workspaceProject, err := m.workspaceProjectRows(ctx, rec) + if err != nil { + // Fail closed on unresolved historical rows (mirrors Kill): a workspace + // whose repo layout can't be reconciled must not be torn down. An archived + // or unregistered project gets a distinct code so the UI can tell the user + // to remove the worktree manually. + res.failureCode = failWorkspaceRows + if errors.Is(err, ErrProjectNotResolvable) { + res.failureCode = failProjectUnresolvable + } + return res, fmt.Errorf("workspace rows: %w", err) + } + res.workspaceProject = workspaceProject + ws := workspaceInfo(rec) + if !workspaceProject && ws.Path == "" { + // No workspace to release (no-worktree / spawn-failed / orchestrator). + res.disposition = domain.DispositionNotApplicable + m.cleanupSystemPromptDir(rec.ID) + return res, nil + } + + // Never reclaim a worktree with a live reviewer. Tear the pane down + // (idempotent) and gate removal on its ACTUAL liveness, not the teardown + // return, so a #2715-style fail-open can't corrupt an in-flight review. + if m.reviewer != nil { + if err := m.reviewer.TeardownReviewer(ctx, rec.ID); err != nil { + m.logger.Warn("finalize: reviewer teardown failed", "sessionID", rec.ID, "error", err) + } + alive, err := m.reviewer.ReviewerAlive(ctx, rec.ID) + if err != nil || alive { + res.failureCode = failReviewerAlive + return res, fmt.Errorf("reviewer pane still live for %s; deferring worktree removal", rec.ID) + } + } + + if workspaceProject { + out := m.releaseWorkspaceProjectRows(ctx, rows) + if out.dirty == 0 && out.firstErr != nil { + // Purely non-dirty failure(s): surface so Kill fails closed and the + // finalizer/Cleanup schedule a retry. The failed rows are already + // marked retry_remove. + res.failureCode = workspaceFailureCode(out.firstErr) + return res, fmt.Errorf("workspace: %w", out.firstErr) + } + if out.removed > 0 { + m.cleanupAgentWorkspace(ctx, rec, rec.Metadata.WorkspacePath) + } + if out.dirty > 0 { + res.disposition = domain.DispositionPreservedDirty + } else { + res.disposition = domain.DispositionRemoved + } + m.cleanupSystemPromptDir(rec.ID) + return res, nil + } + + // Single worktree. + if err := m.workspace.Destroy(ctx, ws); err != nil { + if errors.Is(err, ports.ErrWorkspaceDirty) { + res.disposition = domain.DispositionPreservedDirty + // Drop the restore marker so RestoreAll can't relaunch a terminated + // session over the preserved worktree; the worktree is left on disk. + m.deleteRestoreMarker(ctx, rec.ID) + m.cleanupSystemPromptDir(rec.ID) + return res, nil + } + res.failureCode = workspaceFailureCode(err) + return res, fmt.Errorf("workspace: %w", err) + } + res.disposition = domain.DispositionRemoved + m.cleanupAgentWorkspace(ctx, rec, ws.Path) + m.deleteRestoreMarker(ctx, rec.ID) + m.cleanupSystemPromptDir(rec.ID) + return res, nil +} + +// workspaceFailureCode classifies a non-dirty workspace teardown error: an +// archived/unregistered project (its repo no longer resolves) gets a distinct +// code so the UI can tell the user to remove the worktree by hand; everything +// else is a generic teardown failure. +func workspaceFailureCode(err error) string { + if errors.Is(err, ErrProjectNotResolvable) { + return failProjectUnresolvable + } + return failWorkspaceDestroy +} + +// isTerminalDisposition reports whether a stored disposition is settled for the +// current generation — i.e. auto-retry is done and a finalize can no-op. Pending +// is the only non-terminal state (it is still auto-retrying). +func isTerminalDisposition(d domain.WorkspaceDisposition) bool { + switch d { + case domain.DispositionRemoved, domain.DispositionPreservedDirty, + domain.DispositionFailed, domain.DispositionNotApplicable: + return true + default: + return false + } +} + +// wpOutcome tallies a workspace-project teardown that continues past dirty +// children (#16). Per-repo state lives in session_worktrees.state; the +// facts-table disposition is only the rollup. +type wpOutcome struct { + removed int + dirty int + failed int + firstErr error // first non-dirty teardown error, for the failure code +} + +// releaseWorkspaceProjectRows removes a workspace project's worktrees child-first +// but, unlike the old abort-on-first-dirty destroyWorkspaceProjectRows, continues +// past a dirty child: clean children are removed (state → unavailable), dirty +// children are preserved untouched, and non-dirty failures are marked +// retry_remove. This stops one dirty worktree from permanently stranding its +// clean siblings. +func (m *Manager) releaseWorkspaceProjectRows(ctx context.Context, rows []ports.WorkspaceRepoInfo) wpOutcome { + var out wpOutcome + for i := len(rows) - 1; i >= 0; i-- { + if rows[i].Path == "" { + continue + } + info := workspaceInfoFromRepoInfo(rows[i]) + if err := m.workspace.Destroy(ctx, info); err != nil { + if errors.Is(err, ports.ErrWorkspaceDirty) { + // Preserve the dirty child as-is (never force); its row stays + // non-restorable spawn state, so RestoreAll won't resurrect it. + out.dirty++ + continue + } + if stateErr := m.upsertWorkspaceProjectRowState(ctx, rows[i], "retry_remove"); stateErr != nil && out.firstErr == nil { + out.firstErr = stateErr + } + if out.firstErr == nil { + out.firstErr = err + } + out.failed++ + continue + } + if err := m.upsertWorkspaceProjectRowState(ctx, rows[i], "unavailable"); err != nil && out.firstErr == nil { + out.firstErr = err + } + out.removed++ + } + return out +} + +// deleteRestoreMarker best-effort clears a session's shutdown-restore markers. +func (m *Manager) deleteRestoreMarker(ctx context.Context, id domain.SessionID) { + if err := m.store.DeleteSessionWorktrees(ctx, id); err != nil { + m.logger.Warn("finalize: delete restore marker failed", "sessionID", id, "error", err) + } +} + +// cleanupBackoff returns the capped-exponential retry interval for the given +// (1-based) attempt number. +func cleanupBackoff(attempt int64) time.Duration { + if attempt < 1 { + attempt = 1 + } + d := cleanupBackoffBase + for i := int64(1); i < attempt; i++ { + d *= 2 + if d >= cleanupBackoffCap { + return cleanupBackoffCap + } + } + return d +} + +// persistCleanupFacts writes the durable teardown facts for a terminal session, +// stamped with the cleanup generation the release ran under. Retry bookkeeping +// (attempt count, next_attempt_at) is only carried forward within the same +// generation episode: a Restore→Terminate cycle that advanced the counter starts +// a fresh episode. A pending (transient-failure) result schedules a capped +// backoff retry, or transitions to a terminal `failed` disposition once the +// attempt cap is reached. +func (m *Manager) persistCleanupFacts(ctx context.Context, rec domain.SessionRecord, generation int64, res cleanupResult) error { + now := m.clock() + prior, hadPrior, err := m.store.GetSessionCleanupFacts(ctx, rec.ID) + if err != nil { + return err + } + sameEpisode := hadPrior && prior.SessionGeneration == generation + + facts := domain.SessionCleanupRecord{ + SessionID: rec.ID, + SessionGeneration: generation, + FailureCode: res.failureCode, + LastAttemptAt: now, + } + switch { + case res.runtimeReleased: + facts.RuntimeReleasedAt = now + case sameEpisode && !prior.RuntimeReleasedAt.IsZero(): + // Once genuinely released this episode, the stamp survives later retries. + facts.RuntimeReleasedAt = prior.RuntimeReleasedAt + } + + priorAttempts := int64(0) + if sameEpisode { + priorAttempts = prior.AttemptCount + } + attempts := priorAttempts + 1 + facts.AttemptCount = attempts + + if res.disposition == domain.DispositionPending { + if attempts >= maxCleanupAttempts { + // Exhausted: stop auto-retry. Only a user-triggered retry (PR 3 API) + // clears this, distinct from a still-auto-retrying pending state. + facts.WorkspaceDisposition = domain.DispositionFailed + } else { + facts.WorkspaceDisposition = domain.DispositionPending + facts.NextAttemptAt = now.Add(cleanupBackoff(attempts)) + } + } else { + facts.WorkspaceDisposition = res.disposition + } + return m.store.UpsertSessionCleanupFacts(ctx, facts) +} + +// FinalizeTerminalSession converges a terminated session's runtime + workspace +// toward safe release. It is idempotent and safe to call repeatedly: it is the +// reconciler's per-session unit of work and the primitive the per-session +// cleanup API (PR 3) delegates to. It never marks a session terminated — that +// durable intent is written by Kill / the lifecycle reactions — and it never +// releases a restore-pending session (RestoreAll owns those). +func (m *Manager) FinalizeTerminalSession(ctx context.Context, id domain.SessionID) error { + unlock := m.sessionLocks.lock(id) + defer unlock() + + rec, ok, err := m.store.GetSession(ctx, id) + if err != nil { + return fmt.Errorf("finalize %s: %w", id, err) + } + if !ok || !rec.IsTerminated { + // Gone or no longer terminal (a Restore un-terminated it): nothing to do. + return nil + } + + // A restorable session_worktrees row means the session is shutdown-saved and + // owned by RestoreAll; releasing under it would race a restore. Merge/kill + // terminated sessions keep their spawn-time rows in state "active" (not + // restorable), so those remain finalize-eligible. + rows, err := m.store.ListSessionWorktrees(ctx, id) + if err != nil { + return fmt.Errorf("finalize %s: %w", id, err) + } + if len(restorableWorktreeRows(rows)) > 0 { + return nil + } + + generation := rec.CleanupGeneration + + // Idempotent no-op if already finalized for this generation episode. Shared + // with finalizeOne (bulk Cleanup) so both entry points skip a re-release. + if facts, ok, err := m.store.GetSessionCleanupFacts(ctx, id); err != nil { + return fmt.Errorf("finalize %s: %w", id, err) + } else if ok && facts.SessionGeneration == generation && isTerminalDisposition(facts.WorkspaceDisposition) { + return nil + } + + res, releaseErr := m.releaseTerminalResources(ctx, rec) + if releaseErr != nil { + // A terminated session's teardown failure is not surfaced to the caller; + // it becomes a retry fact the periodic sweep re-attempts. + m.logger.Warn("finalize: release failed; scheduling retry", "sessionID", id, "error", releaseErr) + res.disposition = domain.DispositionPending + if res.failureCode == "" { + res.failureCode = failTeardown + } + } + + // Generation guard: if a Restore→Terminate cycle advanced the counter while we + // released, this result is stale and must not satisfy the newer episode. + cur, ok, err := m.store.GetSession(ctx, id) + if err != nil { + return fmt.Errorf("finalize %s: %w", id, err) + } + if !ok || !cur.IsTerminated || cur.CleanupGeneration != generation { + return nil + } + return m.persistCleanupFacts(ctx, rec, generation, res) +} diff --git a/backend/internal/session_manager/finalize_test.go b/backend/internal/session_manager/finalize_test.go new file mode 100644 index 0000000000..d2cb2cf889 --- /dev/null +++ b/backend/internal/session_manager/finalize_test.go @@ -0,0 +1,388 @@ +package sessionmanager + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// fakeReviewer stands in for the review engine's reviewer-teardown gate. +type fakeReviewer struct { + teardownCalls int + alive bool + aliveErr error + teardownErr error +} + +func (f *fakeReviewer) TeardownReviewer(context.Context, domain.SessionID) error { + f.teardownCalls++ + return f.teardownErr +} +func (f *fakeReviewer) ReviewerAlive(context.Context, domain.SessionID) (bool, error) { + return f.alive, f.aliveErr +} + +// finalizeManager builds a Manager wired for finalizer tests, with a fixed clock +// (so backoff/next_attempt_at are deterministic) and an optional reviewer. +func finalizeManager(rv reviewerTeardown) (*Manager, *fakeStore, *fakeRuntime, *fakeWorkspace) { + st := newFakeStore() + rt := &fakeRuntime{} + ws := &fakeWorkspace{} + m := New(Deps{ + Runtime: rt, + Agents: fakeAgents{}, + Workspace: ws, + Store: st, + Messenger: &fakeMessenger{}, + Lifecycle: &fakeLCM{store: st}, + Reviewer: rv, + Clock: func() time.Time { return time.Date(2026, 7, 21, 0, 0, 0, 0, time.UTC) }, + LookPath: func(string) (string, error) { return "/bin/true", nil }, + }) + return m, st, rt, ws +} + +func TestFinalize_CleanTeardownReleasesRuntimeAndWorkspace(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if rt.destroyed != 1 || ws.destroyed != 1 { + t.Fatalf("runtime destroyed=%d workspace destroyed=%d, want 1/1", rt.destroyed, ws.destroyed) + } + facts, ok := st.cleanup["mer-1"] + if !ok { + t.Fatal("no cleanup facts written") + } + if facts.WorkspaceDisposition != domain.DispositionRemoved { + t.Fatalf("disposition = %q, want removed", facts.WorkspaceDisposition) + } + if facts.RuntimeReleasedAt.IsZero() { + t.Fatal("runtime_released_at should be stamped on a genuine release") + } +} + +func TestFinalize_DirtyWorkspaceIsPreservedDirty(t *testing.T) { + m, st, _, ws := finalizeManager(nil) + ws.destroyErr = fmt.Errorf("gitworktree: refusing to remove: %w", ports.ErrWorkspaceDirty) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + facts := st.cleanup["mer-1"] + if facts.WorkspaceDisposition != domain.DispositionPreservedDirty { + t.Fatalf("disposition = %q, want preserved_dirty", facts.WorkspaceDisposition) + } + // A preserved-dirty session is terminal for auto-retry: no next_attempt_at. + if !facts.NextAttemptAt.IsZero() { + t.Fatalf("next_attempt_at = %v, want none for preserved_dirty", facts.NextAttemptAt) + } +} + +func TestFinalize_RuntimeFailLeavesWorkspaceAndSchedulesRetry(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + rt.destroyErr = errors.New("tmux transient") + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize should not surface a terminated session's teardown error: %v", err) + } + if ws.destroyed != 0 { + t.Fatal("runtime-first: workspace must be left untouched when runtime destroy fails") + } + facts := st.cleanup["mer-1"] + if facts.WorkspaceDisposition != domain.DispositionPending { + t.Fatalf("disposition = %q, want pending (retry scheduled)", facts.WorkspaceDisposition) + } + if facts.AttemptCount != 1 || facts.NextAttemptAt.IsZero() { + t.Fatalf("attempt=%d next=%v, want attempt 1 with a scheduled retry", facts.AttemptCount, facts.NextAttemptAt) + } + if facts.FailureCode != failRuntimeDestroy { + t.Fatalf("failure code = %q, want %q", facts.FailureCode, failRuntimeDestroy) + } + if !facts.RuntimeReleasedAt.IsZero() { + t.Fatal("runtime must NOT be recorded released when its destroy failed") + } +} + +func TestFinalize_RetryExhaustionMarksFailed(t *testing.T) { + m, st, rt, _ := finalizeManager(nil) + rt.destroyErr = errors.New("permanently wedged") + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + // Each attempt fails transiently and increments the count; the cap flips it to + // a terminal `failed` disposition that stops auto-retry. + for i := 0; i < maxCleanupAttempts; i++ { + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize attempt %d: %v", i, err) + } + } + facts := st.cleanup["mer-1"] + if facts.WorkspaceDisposition != domain.DispositionFailed { + t.Fatalf("disposition = %q, want failed after %d attempts", facts.WorkspaceDisposition, maxCleanupAttempts) + } + if facts.AttemptCount != int64(maxCleanupAttempts) { + t.Fatalf("attempt count = %d, want %d", facts.AttemptCount, maxCleanupAttempts) + } + if !facts.NextAttemptAt.IsZero() { + t.Fatal("a failed (exhausted) cleanup must not schedule further auto-retry") + } +} + +func TestFinalize_IdempotentReRunAfterRemoved(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize 1: %v", err) + } + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize 2: %v", err) + } + // Second run is a no-op: facts already record `removed` for this generation. + if rt.destroyed != 1 || ws.destroyed != 1 { + t.Fatalf("re-run should be a no-op: runtime=%d workspace=%d, want 1/1", rt.destroyed, ws.destroyed) + } +} + +func TestFinalize_NotTerminalIsNoop(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + st.sessions["mer-1"] = mkLive("mer-1") // not terminated + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if rt.destroyed != 0 || ws.destroyed != 0 || len(st.cleanup) != 0 { + t.Fatal("finalize must not touch a live (non-terminal) session") + } +} + +func TestFinalize_RestorePendingSessionIsSkipped(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + // A "removed"-state row is a shutdown-restore marker: RestoreAll owns it. + st.worktrees["mer-1"] = []domain.SessionWorktreeRecord{ + {SessionID: "mer-1", RepoName: domain.RootWorkspaceRepoName, State: "removed", WorktreePath: "/ws/mer-1"}, + } + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if rt.destroyed != 0 || ws.destroyed != 0 || len(st.cleanup) != 0 { + t.Fatal("finalize must never release a restore-pending session") + } +} + +func TestFinalize_NoWorkspaceIsNotApplicable(t *testing.T) { + m, st, _, _ := finalizeManager(nil) + // Terminal session with no workspace and no handle (spawn-failed / orchestrator). + st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", IsTerminated: true} + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + facts, ok := st.cleanup["mer-1"] + if !ok || facts.WorkspaceDisposition != domain.DispositionNotApplicable { + t.Fatalf("facts = %+v, want a not_applicable row so the boot scan won't re-enqueue it forever", facts) + } +} + +// TestFinalize_GenerationGuardDropsStaleResult pins critique #17: a finalize +// begun under generation N must not persist after a Restore→Terminate cycle has +// advanced the counter to N+1. +func TestFinalize_GenerationGuardDropsStaleResult(t *testing.T) { + m, st, rt, _ := finalizeManager(nil) + st.sessions["mer-1"] = domain.SessionRecord{ + ID: "mer-1", ProjectID: "mer", IsTerminated: true, CleanupGeneration: 1, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}, + } + // Simulate a Restore→Terminate cycle advancing the generation while the + // release core runs (during runtime Destroy). + rt.destroyHook = func() { + rec := st.sessions["mer-1"] + rec.CleanupGeneration = 2 + st.sessions["mer-1"] = rec + } + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if _, ok := st.cleanup["mer-1"]; ok { + t.Fatal("a stale-generation finalize result must not be persisted") + } +} + +// TestFinalize_ReviewerAliveDefersRemoval pins critique #24 / M1: a worktree with +// a live reviewer pane is never removed; the workspace becomes retryable. +func TestFinalize_ReviewerAliveDefersRemoval(t *testing.T) { + rv := &fakeReviewer{alive: true} + m, st, _, ws := finalizeManager(rv) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if rv.teardownCalls != 1 { + t.Fatalf("reviewer teardown calls = %d, want 1", rv.teardownCalls) + } + if ws.destroyed != 0 { + t.Fatal("worktree must not be removed while the reviewer pane is still alive") + } + facts := st.cleanup["mer-1"] + if facts.WorkspaceDisposition != domain.DispositionPending || facts.FailureCode != failReviewerAlive { + t.Fatalf("facts = %+v, want pending/reviewer_alive so it retries after the reviewer ends", facts) + } +} + +func TestFinalize_ReviewerTornDownThenWorktreeRemoved(t *testing.T) { + rv := &fakeReviewer{alive: false} // teardown confirmed the pane is gone + m, st, _, ws := finalizeManager(rv) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + if rv.teardownCalls != 1 || ws.destroyed != 1 { + t.Fatalf("teardown=%d workspace destroyed=%d, want 1/1", rv.teardownCalls, ws.destroyed) + } + if st.cleanup["mer-1"].WorkspaceDisposition != domain.DispositionRemoved { + t.Fatalf("disposition = %q, want removed", st.cleanup["mer-1"].WorkspaceDisposition) + } +} + +// TestFinalize_WorkspaceProjectMixedCleanDirty pins critique #16 / M2: a dirty +// child no longer strands its clean siblings — clean children are removed, the +// dirty one is preserved, and the session rolls up to preserved_dirty. +func TestFinalize_WorkspaceProjectMixedCleanDirty(t *testing.T) { + m, st, _, ws := finalizeManager(nil) + ws.destroyErrByRepo = map[string]error{ + "web": fmt.Errorf("dirty: %w", ports.ErrWorkspaceDirty), + } + st.projects["mer"] = domain.ProjectRecord{ID: "mer", Path: "/repo/mer", Kind: domain.ProjectKindWorkspace, Config: testRoleAgents()} + st.workspaceRepo["mer"] = []domain.WorkspaceRepoRecord{ + {Name: "api", RelativePath: "api"}, + {Name: "web", RelativePath: "web"}, + } + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "ao/mer-1", RuntimeHandleID: "h1"}) + st.worktrees["mer-1"] = []domain.SessionWorktreeRecord{ + {SessionID: "mer-1", RepoName: domain.RootWorkspaceRepoName, Branch: "ao/mer-1", WorktreePath: "/ws/mer-1", State: "active"}, + {SessionID: "mer-1", RepoName: "api", Branch: "ao/mer-1", WorktreePath: "/ws/mer-1/api", State: "active"}, + {SessionID: "mer-1", RepoName: "web", Branch: "ao/mer-1", WorktreePath: "/ws/mer-1/web", State: "active"}, + } + + if err := m.FinalizeTerminalSession(ctx, "mer-1"); err != nil { + t.Fatalf("finalize: %v", err) + } + states := map[string]string{} + for _, row := range st.worktrees["mer-1"] { + states[row.RepoName] = row.State + } + if states["api"] != "unavailable" || states[domain.RootWorkspaceRepoName] != "unavailable" { + t.Fatalf("clean children not reclaimed: states = %v", states) + } + if states["web"] != "active" { + t.Fatalf("dirty child web state = %q, want preserved (unchanged)", states["web"]) + } + if st.cleanup["mer-1"].WorkspaceDisposition != domain.DispositionPreservedDirty { + t.Fatalf("rollup = %q, want preserved_dirty", st.cleanup["mer-1"].WorkspaceDisposition) + } +} + +// overlapDetector returns an opHook that flags if two disk-touching workspace +// ops are ever in flight at once for the same session, plus a reader for the flag. +func overlapDetector() (hook func(string), overlapped func() bool) { + var mu sync.Mutex + inCS := 0 + over := false + hook = func(string) { + mu.Lock() + inCS++ + if inCS > 1 { + over = true + } + mu.Unlock() + time.Sleep(10 * time.Millisecond) // widen the window so a missing lock overlaps + mu.Lock() + inCS-- + mu.Unlock() + } + overlapped = func() bool { mu.Lock(); defer mu.Unlock(); return over } + return hook, overlapped +} + +// TestFinalize_SerializedWithRestore pins the gating review item: the per-session +// lock must make a reconciler finalize and a concurrent RestoreWithMode mutually +// exclusive, so a finalize can't remove the worktree of a session being +// relaunched. Without the lock in RestoreWithMode, the workspace Destroy and +// Restore overlap; -race would also flag the resulting store races. +func TestFinalize_SerializedWithRestore(t *testing.T) { + for i := 0; i < 20; i++ { + m, st, _, ws := finalizeManager(nil) + st.projects["mer"] = domain.ProjectRecord{ID: "mer", Path: "/repo/mer", Config: testRoleAgents()} + st.sessions["mer-1"] = domain.SessionRecord{ + ID: "mer-1", ProjectID: "mer", IsTerminated: true, CleanupGeneration: 1, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "ao/mer-1", RuntimeHandleID: "h1"}, + } + hook, overlapped := overlapDetector() + ws.opHook = hook + + var wg sync.WaitGroup + wg.Add(2) + go func() { defer wg.Done(); _ = m.FinalizeTerminalSession(ctx, "mer-1") }() + go func() { defer wg.Done(); _, _ = m.RestoreWithMode(ctx, "mer-1") }() + wg.Wait() + + if overlapped() { + t.Fatalf("iteration %d: finalize and restore overlapped — per-session lock not serializing them", i) + } + } +} + +// TestKill_SerializedWithFinalize pins that a user Kill and a reconciler finalize +// over the same session never tear down its workspace concurrently. +func TestKill_SerializedWithFinalize(t *testing.T) { + for i := 0; i < 20; i++ { + m, st, _, ws := finalizeManager(nil) + st.sessions["mer-1"] = domain.SessionRecord{ + ID: "mer-1", ProjectID: "mer", IsTerminated: true, CleanupGeneration: 1, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "ao/mer-1", RuntimeHandleID: "h1"}, + } + hook, overlapped := overlapDetector() + ws.opHook = hook + + var wg sync.WaitGroup + wg.Add(2) + go func() { defer wg.Done(); _, _ = m.Kill(ctx, "mer-1") }() + go func() { defer wg.Done(); _ = m.FinalizeTerminalSession(ctx, "mer-1") }() + wg.Wait() + + if overlapped() { + t.Fatalf("iteration %d: kill and finalize overlapped — per-session lock not serializing them", i) + } + } +} + +// TestKill_WritesCleanupFacts pins critique #18: every terminal path (here Kill) +// persists a facts row so the sessions-driven boot scan doesn't re-enqueue it +// forever. +func TestKill_WritesCleanupFacts(t *testing.T) { + m, st, _, _ := finalizeManager(nil) + st.sessions["mer-1"] = mkLive("mer-1") + + if _, err := m.Kill(ctx, "mer-1"); err != nil { + t.Fatalf("kill: %v", err) + } + facts, ok := st.cleanup["mer-1"] + if !ok || facts.WorkspaceDisposition != domain.DispositionRemoved { + t.Fatalf("kill facts = %+v (ok=%v), want a removed row", facts, ok) + } +} diff --git a/backend/internal/session_manager/locks.go b/backend/internal/session_manager/locks.go new file mode 100644 index 0000000000..73389a60b9 --- /dev/null +++ b/backend/internal/session_manager/locks.go @@ -0,0 +1,53 @@ +package sessionmanager + +import ( + "sync" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// keyedMutex serialises operations per session id. It mirrors the house idiom +// (review.go:lockWorker / service.go:lockOrchestratorProject: an outer mutex +// guarding a map of per-key mutexes) but adds refcounted eviction: the finalizer +// runs for every terminated session, so a map that never deletes entries would +// grow unbounded by total sessions ever seen. Each entry is dropped when its +// last holder unlocks. +type keyedMutex struct { + mu sync.Mutex + locks map[domain.SessionID]*refLock +} + +type refLock struct { + mu sync.Mutex + refs int +} + +func newKeyedMutex() *keyedMutex { + return &keyedMutex{locks: map[domain.SessionID]*refLock{}} +} + +// lock acquires the per-id mutex and returns an unlock func. The mutex is +// non-reentrant, so a holder must never call lock for the same id again before +// unlocking (the finalizer must therefore never run synchronously inside the CDC +// callback, which is why the reconciler enqueues-and-returns instead). +func (k *keyedMutex) lock(id domain.SessionID) func() { + k.mu.Lock() + rl, ok := k.locks[id] + if !ok { + rl = &refLock{} + k.locks[id] = rl + } + rl.refs++ + k.mu.Unlock() + + rl.mu.Lock() + return func() { + rl.mu.Unlock() + k.mu.Lock() + rl.refs-- + if rl.refs == 0 { + delete(k.locks, id) + } + k.mu.Unlock() + } +} diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 990d67d1c7..e24cfaa28f 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -151,6 +151,13 @@ type Store interface { // Kill and successful RestoreAll must remove these rows to prevent // resurrecting sessions the user intentionally terminated. DeleteSessionWorktrees(ctx context.Context, id domain.SessionID) error + // UpsertSessionCleanupFacts records the durable runtime/workspace teardown + // facts for a terminated session (disposition rollup + retry bookkeeping). + // The reconciler and the per-session cleanup API render from these. + UpsertSessionCleanupFacts(ctx context.Context, rec domain.SessionCleanupRecord) error + // GetSessionCleanupFacts returns a session's cleanup facts, ok=false when it + // has none yet. + GetSessionCleanupFacts(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, bool, error) } // Manager coordinates internal session spawn, restore, kill, and cleanup over @@ -167,8 +174,20 @@ type Manager struct { // initial-prompt delivery, where a blocked session is impossible. messenger *sessionguard.Guard lcm lifecycleRecorder - dataDir string - clock func() time.Time + // reviewer, when set, lets the release core tear down and gate on a worker's + // reviewer pane before reclaiming its worktree. Optional (nil disables it). + reviewer reviewerTeardown + // sessionLocks serialises every disk-touching lifecycle op for a session — + // Spawn, RestoreWithMode, RestoreAll (per session), Kill, bulk Cleanup, and + // FinalizeTerminalSession — so a reconciler finalize can't git-worktree-remove + // out from under a concurrent Kill or Restore over the same runtime/workspace. + // The mutex is non-reentrant: the finalizer must never run synchronously inside + // the CDC callback (the reconciler enqueues-and-returns), and locked ops must + // not call each other (Spawn's rollback helpers never re-enter; RollbackSpawn→ + // Kill is a separate top-level call). Refcounted so entries don't accumulate. + sessionLocks *keyedMutex + dataDir string + clock func() time.Time // lookPath is exec.LookPath in production; tests substitute a stub so // they don't need real binaries on PATH. Returns ports.ErrAgentBinaryNotFound // when the binary is missing so the sentinel propagates through toAPIError. @@ -220,6 +239,10 @@ type Deps struct { Store Store Messenger ports.AgentMessenger Lifecycle lifecycleRecorder + // Reviewer, when set, is the review engine the release core uses to tear down + // and gate on a worker's reviewer pane before reclaiming its worktree. + // Optional: production wires the review engine; unit tests leave it nil. + Reviewer reviewerTeardown // DataDir is exported to spawned agents as AO_DATA_DIR so their hook // commands can open the same store. DataDir string @@ -243,17 +266,19 @@ type Deps struct { // time.Now when Deps.Clock is nil. func New(d Deps) *Manager { m := &Manager{ - runtime: d.Runtime, - agents: d.Agents, - workspace: d.Workspace, - store: d.Store, - lcm: d.Lifecycle, - dataDir: d.DataDir, - clock: d.Clock, - lookPath: d.LookPath, - executable: d.Executable, - newLaunchID: d.NewLaunchID, - resuming: make(map[domain.SessionID]struct{}), + runtime: d.Runtime, + agents: d.Agents, + workspace: d.Workspace, + store: d.Store, + lcm: d.Lifecycle, + reviewer: d.Reviewer, + sessionLocks: newKeyedMutex(), + dataDir: d.DataDir, + clock: d.Clock, + lookPath: d.LookPath, + executable: d.Executable, + newLaunchID: d.NewLaunchID, + resuming: make(map[domain.SessionID]struct{}), sendConfirm: sendConfirmConfig{ pollInterval: sendConfirmPollInterval, attemptDeadline: sendConfirmAttemptDeadline, @@ -328,6 +353,14 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess return domain.SessionRecord{}, 0, 0, fmt.Errorf("spawn: create: %w", err) } id := rec.ID + // Serialise this session's disk-touching lifecycle against a concurrent Kill + // or reconciler finalize (shared per-session lock). A brand-new id is never + // terminal so nothing targets it yet, but holding the lock keeps the + // one-writer-per-session invariant uniform across Spawn/Restore/Kill/finalize. + // Deadlock-safe: Spawn's rollback helpers never re-enter the lock (the public + // RollbackSpawn→Kill path is a separate top-level call). + unlock := m.sessionLocks.lock(id) + defer unlock() systemPromptFile, err := m.prepareSystemPromptFile(id, cfg.Harness, systemPrompt) if err != nil { m.rollbackSpawnSeedRow(ctx, id) @@ -711,6 +744,9 @@ func (m *Manager) RollbackSpawn(ctx context.Context, id domain.SessionID) (delet // available destroy steps are skipped so it can be cleaned up from the // dashboard. func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { + unlock := m.sessionLocks.lock(id) + defer unlock() + rec, ok, err := m.store.GetSession(ctx, id) if err != nil { return false, fmt.Errorf("kill %s: %w", id, err) @@ -718,69 +754,30 @@ func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { if !ok { return false, nil // already gone: benign race } - handle := runtimeHandle(rec.Metadata) - ws := workspaceInfo(rec) + generation := rec.CleanupGeneration - var workspaceProjectRows []ports.WorkspaceRepoInfo - workspaceProject := false - if rows, ok, rowErr := m.workspaceProjectRows(ctx, rec); rowErr != nil { - return false, fmt.Errorf("kill %s: workspace rows: %w", id, rowErr) - } else if ok { - workspaceProjectRows = rows - workspaceProject = true + res, releaseErr := m.releaseTerminalResources(ctx, rec) + if releaseErr != nil { + // Fail closed: a runtime or non-dirty workspace teardown failure leaves the + // session live (never marked terminated) so the user can retry — the same + // contract Kill has always had. A dirty worktree is NOT an error: the core + // reports it as preserved_dirty below. + return false, fmt.Errorf("kill %s: %w", id, releaseErr) } - if handle.ID != "" { - if err := m.runtime.Destroy(ctx, handle); err != nil { - return false, fmt.Errorf("kill %s: runtime: %w", id, err) - } - } - freed := false - if workspaceProject { - cleaned, err := m.destroyWorkspaceProjectRows(ctx, workspaceProjectRows) - if err != nil { - if errors.Is(err, ports.ErrWorkspaceDirty) { - if err := m.lcm.MarkTerminated(ctx, id); err != nil { - return false, fmt.Errorf("kill %s: %w", id, err) - } - m.cleanupSystemPromptDir(id) - return false, nil - } - return false, fmt.Errorf("kill %s: workspace: %w", id, err) - } - freed = cleaned - if cleaned { - m.cleanupAgentWorkspace(ctx, rec, ws.Path) - } - } else if ws.Path != "" { - if err := m.workspace.Destroy(ctx, ws); err != nil { - if errors.Is(err, ports.ErrWorkspaceDirty) { - if err := m.store.DeleteSessionWorktrees(ctx, id); err != nil { - m.logger.Warn("kill: delete restore marker failed", "sessionID", id, "error", err) - } - if err := m.lcm.MarkTerminated(ctx, id); err != nil { - return false, fmt.Errorf("kill %s: %w", id, err) - } - m.cleanupSystemPromptDir(id) - return false, nil - } - return false, fmt.Errorf("kill %s: workspace: %w", id, err) - } - freed = true - m.cleanupAgentWorkspace(ctx, rec, ws.Path) - } - // Clear the restore marker so the next boot's RestoreAll cannot resurrect a - // killed session (#2319). For workspace projects this must happen after - // teardown reads the rows; dirty-preserved rows return above and are left as - // non-restorable inventory. - if err := m.store.DeleteSessionWorktrees(ctx, id); err != nil { - m.logger.Warn("kill: delete restore marker failed", "sessionID", id, "error", err) - } if err := m.lcm.MarkTerminated(ctx, id); err != nil { return false, fmt.Errorf("kill %s: %w", id, err) } - m.cleanupSystemPromptDir(id) - return freed, nil + // Persist facts so a user-Kill exits the reconciler's boot scan and the UI can + // render its disposition. Best-effort: a facts-write hiccup must not fail the + // Kill (the session is already torn down and terminated). + if err := m.persistCleanupFacts(ctx, rec, generation, res); err != nil { + m.logger.Warn("kill: persist cleanup facts failed", "sessionID", id, "error", err) + } + // freed reports that the workspace was actually released. A preserved-dirty or + // not_applicable outcome frees nothing (any preserved/failed child on a + // workspace project → not freed). + return res.disposition == domain.DispositionRemoved, nil } // RetireForReplacement terminates a live orchestrator and releases its branch @@ -890,6 +887,14 @@ func (m *Manager) retireWorkspaceProjectForReplacement(ctx context.Context, rec // runs before any durable session write, so a failure never resurrects the row // or destroys the worktree (it may hold the agent's prior work). func (m *Manager) RestoreWithMode(ctx context.Context, id domain.SessionID) (RestoreResult, error) { + // Held across the re-read, workspace.Restore, runtime.Create and the + // un-terminate/generation-bump so a concurrent reconciler finalize can't + // git-worktree-remove out from under the session being relaunched. The + // finalizer re-checks cleanup_generation under this same lock and aborts once + // the un-terminate has advanced it. + unlock := m.sessionLocks.lock(id) + defer unlock() + rec, ok, err := m.store.GetSession(ctx, id) if err != nil { return RestoreResult{}, fmt.Errorf("restore %s: %w", id, err) @@ -1245,38 +1250,18 @@ func (m *Manager) reconcileLive(ctx context.Context, rec domain.SessionRecord) e return nil } -// reconcileReap kills the leaked tmux session of a session the DB already marks -// terminated. This covers the teardown that marked the row terminated but failed -// to kill the runtime (e.g. ForceDestroy/Destroy errored after MarkTerminated). -// Destroy is idempotent, so an already-gone session is a no-op. -func (m *Manager) reconcileReap(ctx context.Context, rec domain.SessionRecord) error { - handle := runtimeHandle(rec.Metadata) - if handle.ID == "" { - return nil - } - alive, err := m.runtime.IsAlive(ctx, handle) - if err != nil { - return fmt.Errorf("reconcile reap %s: probe: %w", rec.ID, err) - } - if !alive { - return nil - } - if err := m.runtime.Destroy(ctx, handle); err != nil { - return fmt.Errorf("reconcile reap %s: destroy: %w", rec.ID, err) - } - return nil -} - // Reconcile is the boot-time consistency pass. It replaces the bare RestoreAll // call so that however the previous daemon died (clean shutdown, SIGKILL, or // crash), live reality matches the DB: // // 1. Live pass: for each non-terminated session, adopt it if its runtime // survived, else capture work and mark terminated (reconcileLive). -// 2. Reap pass: for each terminated session whose runtime leaked, kill it -// (reconcileReap). Runs before restore so a restored session does not -// collide with a leaked tmux of the same name. -// 3. Restore pass: relaunch shutdown-saved sessions (existing RestoreAll). +// 2. Restore pass: relaunch shutdown-saved sessions (existing RestoreAll). +// +// The old reap pass (runtime-Destroy every terminated-but-alive session) is +// gone: it is a strict subset of the terminal-resource reconciler's runtime-first +// release, which now owns reclaiming leaked runtimes for every terminal session +// via its boot scan. Keeping both would double-destroy and drift. // // Best-effort throughout: a per-session failure is logged and never aborts the // pass or blocks boot. @@ -1293,14 +1278,6 @@ func (m *Manager) Reconcile(ctx context.Context) error { m.logger.Error("reconcile: live pass failed, skipping", "sessionID", rec.ID, "error", err) } } - for _, rec := range recs { - if !rec.IsTerminated { - continue - } - if err := m.reconcileReap(ctx, rec); err != nil { - m.logger.Error("reconcile: reap pass failed, skipping", "sessionID", rec.ID, "error", err) - } - } return m.RestoreAll(ctx) } @@ -1325,123 +1302,133 @@ func (m *Manager) RestoreAll(ctx context.Context) error { if !rec.IsTerminated { continue } - // Check the shutdown-saved marker: is there a session_worktrees row? - rows, err := m.store.ListSessionWorktrees(ctx, rec.ID) - if err != nil { - m.logger.Error("restore-all: list worktrees failed", "sessionID", rec.ID, "error", err) - continue - } - if len(rows) == 0 { - // No marker: this session was killed by the user before shutdown. - continue - } - rows = restorableWorktreeRows(rows) - if len(rows) == 0 { - continue - } + m.restoreSavedSession(ctx, rec) + } + return nil +} - // Step 1: ensure the worktree exists. workspace.Restore re-creates it - // if it was removed by SaveAndTeardownAll. - project, err := m.loadProject(ctx, rec.ProjectID) - if err != nil { - m.logger.Error("restore-all: load project failed", "sessionID", rec.ID, "error", err) - continue +// restoreSavedSession relaunches one shutdown-saved terminated session under the +// shared per-session lock, so a concurrent reconciler finalize can't remove the +// worktree it is restoring. Per-session failures are logged, never fatal. +func (m *Manager) restoreSavedSession(ctx context.Context, rec domain.SessionRecord) { + unlock := m.sessionLocks.lock(rec.ID) + defer unlock() + + // Check the shutdown-saved marker: is there a session_worktrees row? + rows, err := m.store.ListSessionWorktrees(ctx, rec.ID) + if err != nil { + m.logger.Error("restore-all: list worktrees failed", "sessionID", rec.ID, "error", err) + return + } + if len(rows) == 0 { + // No marker: this session was killed by the user before shutdown. + return + } + rows = restorableWorktreeRows(rows) + if len(rows) == 0 { + return + } + + // Step 1: ensure the worktree exists. workspace.Restore re-creates it + // if it was removed by SaveAndTeardownAll. + project, err := m.loadProject(ctx, rec.ProjectID) + if err != nil { + m.logger.Error("restore-all: load project failed", "sessionID", rec.ID, "error", err) + return + } + var ws ports.WorkspaceInfo + restoredWorkspaceProject := project.Kind.WithDefault() == domain.ProjectKindWorkspace + var projectRows []ports.WorkspaceRepoInfo + if restoredWorkspaceProject { + var rowErr error + projectRows, rowErr = m.workspaceProjectRestoreRowsFromMarkers(ctx, project, rec, rows) + if rowErr != nil { + m.logger.Error("restore-all: workspace rows failed", "sessionID", rec.ID, "error", rowErr) + return } - var ws ports.WorkspaceInfo - restoredWorkspaceProject := project.Kind.WithDefault() == domain.ProjectKindWorkspace - var projectRows []ports.WorkspaceRepoInfo - if restoredWorkspaceProject { - var rowErr error - projectRows, rowErr = m.workspaceProjectRestoreRowsFromMarkers(ctx, project, rec, rows) - if rowErr != nil { - m.logger.Error("restore-all: workspace rows failed", "sessionID", rec.ID, "error", rowErr) - continue - } - root, restoreErr := m.restoreWorkspaceProjectRows(ctx, projectRows) - if restoreErr != nil { - m.logger.Error("restore-all: workspace project restore failed", "sessionID", rec.ID, "error", restoreErr) - continue - } - ws = workspaceInfoFromRepoInfo(root) - } else { - var restoreErr error - ws, restoreErr = m.workspace.Restore(ctx, ports.WorkspaceConfig{ - ProjectID: rec.ProjectID, - SessionID: rec.ID, - Kind: rec.Kind, - SessionPrefix: sessionPrefix(project), - Branch: rec.Metadata.Branch, - Path: rec.Metadata.WorkspacePath, - }) - if restoreErr != nil { - m.logger.Error("restore-all: workspace restore failed", "sessionID", rec.ID, "error", restoreErr) - continue - } + root, restoreErr := m.restoreWorkspaceProjectRows(ctx, projectRows) + if restoreErr != nil { + m.logger.Error("restore-all: workspace project restore failed", "sessionID", rec.ID, "error", restoreErr) + return } - if ws.Path == "" { - m.logger.Error("restore-all: workspace restore failed", "sessionID", rec.ID, "error", "empty restored root path") - continue + ws = workspaceInfoFromRepoInfo(root) + } else { + var restoreErr error + ws, restoreErr = m.workspace.Restore(ctx, ports.WorkspaceConfig{ + ProjectID: rec.ProjectID, + SessionID: rec.ID, + Kind: rec.Kind, + SessionPrefix: sessionPrefix(project), + Branch: rec.Metadata.Branch, + Path: rec.Metadata.WorkspacePath, + }) + if restoreErr != nil { + m.logger.Error("restore-all: workspace restore failed", "sessionID", rec.ID, "error", restoreErr) + return } + } + if ws.Path == "" { + m.logger.Error("restore-all: workspace restore failed", "sessionID", rec.ID, "error", "empty restored root path") + return + } - // Step 2: replay preserve ref when one was recorded. - if restoredWorkspaceProject { - m.applyWorkspaceProjectPreserved(ctx, projectRows) - } else { - var preserveRef string - for _, r := range rows { - if r.PreservedRef != "" { - preserveRef = r.PreservedRef - break - } + // Step 2: replay preserve ref when one was recorded. + if restoredWorkspaceProject { + m.applyWorkspaceProjectPreserved(ctx, projectRows) + } else { + var preserveRef string + for _, r := range rows { + if r.PreservedRef != "" { + preserveRef = r.PreservedRef + break } - if preserveRef != "" { - if applyErr := m.workspace.ApplyPreserved(ctx, ws, preserveRef); applyErr != nil { - if errors.Is(applyErr, ports.ErrPreservedConflict) { - m.logger.Warn("restore-all: apply preserved produced conflicts; agent relaunched with conflict markers in place", - "sessionID", rec.ID, "ref", preserveRef, "error", applyErr) - } else { - m.logger.Error("restore-all: apply preserved failed", "sessionID", rec.ID, "error", applyErr) - } - // Continue: always relaunch even on conflict (never delete the ref here). + } + if preserveRef != "" { + if applyErr := m.workspace.ApplyPreserved(ctx, ws, preserveRef); applyErr != nil { + if errors.Is(applyErr, ports.ErrPreservedConflict) { + m.logger.Warn("restore-all: apply preserved produced conflicts; agent relaunched with conflict markers in place", + "sessionID", rec.ID, "ref", preserveRef, "error", applyErr) + } else { + m.logger.Error("restore-all: apply preserved failed", "sessionID", rec.ID, "error", applyErr) } + // Continue: always relaunch even on conflict (never delete the ref here). } } + } - // Step 3: relaunch the agent in the restored workspace. - if _, err := m.relaunchRestoredSession(ctx, rec, project, ws); err != nil { - switch { - case errors.Is(err, ErrNotResumable): - // A promptless, unresumable worker is intentionally left terminated: - // expected, not an operational failure, so log it quietly. - m.logger.Warn("restore-all: session left terminated (nothing to resume)", "sessionID", rec.ID) - case errors.Is(err, ErrNotFound): - // The row was reaped between listing and relaunch (a stale id during - // reconciliation): skip it and keep restoring the rest. - m.logger.Warn("restore-all: session vanished before relaunch, skipping", "sessionID", rec.ID) - default: - m.logger.Error("restore-all: relaunch failed", "sessionID", rec.ID, "error", err) - } - continue + // Step 3: relaunch the agent in the restored workspace. + if _, err := m.relaunchRestoredSession(ctx, rec, project, ws); err != nil { + switch { + case errors.Is(err, ErrNotResumable): + // A promptless, unresumable worker is intentionally left terminated: + // expected, not an operational failure, so log it quietly. + m.logger.Warn("restore-all: session left terminated (nothing to resume)", "sessionID", rec.ID) + case errors.Is(err, ErrNotFound): + // The row was reaped between listing and relaunch (a stale id during + // reconciliation): skip it and keep restoring the rest. + m.logger.Warn("restore-all: session vanished before relaunch, skipping", "sessionID", rec.ID) + default: + m.logger.Error("restore-all: relaunch failed", "sessionID", rec.ID, "error", err) } + return + } - // One-shot: drop the consumed marker so it never outlives one restart - // (#2319). A still-live session re-acquires it at the next quit. - if restoredWorkspaceProject { - for _, row := range projectRows { - if err := m.upsertWorkspaceProjectRowState(ctx, row, "active"); err != nil { - m.logger.Warn("restore-all: marking workspace repo active failed", "sessionID", rec.ID, "repo", row.RepoName, "error", err) - } - } - } else { - if err := m.markSessionWorktreesActive(ctx, rows); err != nil { - m.logger.Warn("restore-all: marking worktrees active failed", "sessionID", rec.ID, "error", err) - } - if err := m.store.DeleteSessionWorktrees(ctx, rec.ID); err != nil { - m.logger.Warn("restore-all: delete restore marker failed", "sessionID", rec.ID, "error", err) + // One-shot: drop the consumed marker so it never outlives one restart + // (#2319). A still-live session re-acquires it at the next quit. + if restoredWorkspaceProject { + for _, row := range projectRows { + if err := m.upsertWorkspaceProjectRowState(ctx, row, "active"); err != nil { + m.logger.Warn("restore-all: marking workspace repo active failed", "sessionID", rec.ID, "repo", row.RepoName, "error", err) } } + } else { + if err := m.markSessionWorktreesActive(ctx, rows); err != nil { + m.logger.Warn("restore-all: marking worktrees active failed", "sessionID", rec.ID, "error", err) + } + if err := m.store.DeleteSessionWorktrees(ctx, rec.ID); err != nil { + m.logger.Warn("restore-all: delete restore marker failed", "sessionID", rec.ID, "error", err) + } } - return nil } func restorableWorktreeRows(rows []domain.SessionWorktreeRecord) []domain.SessionWorktreeRecord { @@ -1638,34 +1625,6 @@ func (m *Manager) saveAndTeardownWorkspaceProject(ctx context.Context, rec domai return nil } -func (m *Manager) destroyWorkspaceProjectRows(ctx context.Context, rows []ports.WorkspaceRepoInfo) (bool, error) { - cleaned := false - var firstErr error - for i := len(rows) - 1; i >= 0; i-- { - if rows[i].Path == "" { - continue - } - info := workspaceInfoFromRepoInfo(rows[i]) - if err := m.workspace.Destroy(ctx, info); err != nil { - if errors.Is(err, ports.ErrWorkspaceDirty) { - return cleaned, err - } - if stateErr := m.upsertWorkspaceProjectRowState(ctx, rows[i], "retry_remove"); stateErr != nil && firstErr == nil { - firstErr = stateErr - } - if firstErr == nil { - firstErr = err - } - continue - } - if err := m.upsertWorkspaceProjectRowState(ctx, rows[i], "unavailable"); err != nil && firstErr == nil { - firstErr = err - } - cleaned = true - } - return cleaned, firstErr -} - func (m *Manager) upsertWorkspaceProjectRowState(ctx context.Context, row ports.WorkspaceRepoInfo, state string) error { return m.store.UpsertSessionWorktree(ctx, domain.SessionWorktreeRecord{ SessionID: row.SessionID, @@ -1961,53 +1920,69 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu if !rec.IsTerminated { continue } - ws := workspaceInfo(rec) - if ws.Path == "" { - m.cleanupSystemPromptDir(rec.ID) - continue + facts := m.finalizeOne(ctx, rec) + switch facts.WorkspaceDisposition { + case domain.DispositionRemoved: + result.Cleaned = append(result.Cleaned, rec.ID) + case domain.DispositionNotApplicable: + // Nothing to reclaim (no workspace): neither cleaned nor skipped, + // matching the historical silent-skip for handle-less sessions. + default: + result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: cleanupSkipReason(facts)}) } - if h := runtimeHandle(rec.Metadata); h.ID != "" { - _ = m.runtime.Destroy(ctx, h) // best effort; usually already gone - } - if rows, ok, rowErr := m.workspaceProjectRows(ctx, rec); rowErr != nil { - m.logger.Warn("cleanup: workspace rows failed", "sessionID", rec.ID, "error", rowErr) - result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: "workspace teardown failed"}) - continue - } else if ok { - if _, err := m.destroyWorkspaceProjectRows(ctx, rows); err != nil { - if !errors.Is(err, ports.ErrWorkspaceDirty) { - m.logger.Warn("cleanup: workspace teardown failed", "sessionID", rec.ID, "path", ws.Path, "error", err) - } - result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: cleanupSkipReason(err)}) - continue - } - m.cleanupAgentWorkspace(ctx, rec, ws.Path) - } else 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 - // internal filesystem paths); the full cause lands here. - m.logger.Warn("cleanup: workspace teardown failed", "sessionID", rec.ID, "path", ws.Path, "error", err) - } - result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: cleanupSkipReason(err)}) - continue - } else { - m.cleanupAgentWorkspace(ctx, rec, ws.Path) - } - m.cleanupSystemPromptDir(rec.ID) - result.Cleaned = append(result.Cleaned, rec.ID) } return result, nil } -// 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 -// internal filesystem paths. -func cleanupSkipReason(err error) string { - if errors.Is(err, ports.ErrWorkspaceDirty) { +// finalizeOne runs the shared release core for one already-terminated session +// under the per-session lock and returns the resulting disposition. Bulk Cleanup +// uses it rather than FinalizeTerminalSession because Cleanup is an explicit user +// request to reclaim — it deliberately does NOT skip restore-pending sessions +// (an unresumable shutdown-saved leftover should still be cleanable). It shares +// the same idempotent-skip guard (isTerminalDisposition) so an already-finalized +// session isn't needlessly re-released. +func (m *Manager) finalizeOne(ctx context.Context, rec domain.SessionRecord) domain.SessionCleanupRecord { + unlock := m.sessionLocks.lock(rec.ID) + defer unlock() + + generation := rec.CleanupGeneration + if facts, ok, err := m.store.GetSessionCleanupFacts(ctx, rec.ID); err == nil && ok && + facts.SessionGeneration == generation && isTerminalDisposition(facts.WorkspaceDisposition) { + return facts + } + + res, releaseErr := m.releaseTerminalResources(ctx, rec) + if releaseErr != nil { + res.disposition = domain.DispositionPending + if res.failureCode == "" { + res.failureCode = failTeardown + } + } + if err := m.persistCleanupFacts(ctx, rec, generation, res); err != nil { + m.logger.Warn("cleanup: persist facts failed", "sessionID", rec.ID, "error", err) + } + // Re-read so a transient failure that just exhausted its attempt cap reports + // its terminal `failed` disposition rather than the in-memory `pending`. + if stored, ok, err := m.store.GetSessionCleanupFacts(ctx, rec.ID); err == nil && ok && stored.SessionGeneration == generation { + return stored + } + return domain.SessionCleanupRecord{ + SessionID: rec.ID, + SessionGeneration: generation, + WorkspaceDisposition: res.disposition, + FailureCode: res.failureCode, + } +} + +// cleanupSkipReason renders a non-removed disposition 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 internal filesystem +// paths. +func cleanupSkipReason(facts domain.SessionCleanupRecord) string { + if facts.WorkspaceDisposition == domain.DispositionPreservedDirty { return "workspace has uncommitted changes" } - if errors.Is(err, ErrProjectNotResolvable) { + if facts.FailureCode == failProjectUnresolvable { return "project is archived or unregistered — remove worktree manually" } return "workspace teardown failed" diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index aba772f841..0eadbfa13d 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -31,6 +31,8 @@ type fakeStore struct { upsertWTErr error // worktrees maps session ID to its saved worktree rows (shutdown-saved marker). worktrees map[domain.SessionID][]domain.SessionWorktreeRecord + // cleanup maps session ID to its persisted terminal-resource cleanup facts. + cleanup map[domain.SessionID]domain.SessionCleanupRecord // sharedLog, when non-nil, receives an ordered call entry for each // UpsertSessionWorktree invocation so ordering tests can compare across fakes. sharedLog *[]string @@ -43,6 +45,7 @@ func newFakeStore() *fakeStore { projects: map[string]domain.ProjectRecord{}, workspaceRepo: map[string][]domain.WorkspaceRepoRecord{}, worktrees: map[domain.SessionID][]domain.SessionWorktreeRecord{}, + cleanup: map[domain.SessionID]domain.SessionCleanupRecord{}, } } func (f *fakeStore) GetProject(_ context.Context, id string) (domain.ProjectRecord, bool, error) { @@ -131,6 +134,17 @@ func (f *fakeStore) DeleteSessionWorktrees(_ context.Context, id domain.SessionI delete(f.worktrees, id) return nil } +func (f *fakeStore) UpsertSessionCleanupFacts(_ context.Context, rec domain.SessionCleanupRecord) error { + if f.cleanup == nil { + f.cleanup = map[domain.SessionID]domain.SessionCleanupRecord{} + } + f.cleanup[rec.SessionID] = rec + return nil +} +func (f *fakeStore) GetSessionCleanupFacts(_ context.Context, id domain.SessionID) (domain.SessionCleanupRecord, bool, error) { + rec, ok := f.cleanup[id] + return rec, ok, nil +} type fakeLCM struct { store *fakeStore @@ -181,6 +195,10 @@ type fakeRuntime struct { aliveByHandle map[string]bool aliveErr error destroyedIDs []string + // destroyHook, when set, runs at the start of Destroy. Tests use it to + // simulate a concurrent mutation (e.g. a Restore→Terminate cycle advancing + // cleanup_generation) racing an in-flight finalize. + destroyHook func() } type fakeRestartRuntime struct { @@ -226,6 +244,9 @@ func (r *fakeRuntime) Create(_ context.Context, cfg ports.RuntimeConfig) (ports. return ports.RuntimeHandle{ID: "h1"}, nil } func (r *fakeRuntime) Destroy(_ context.Context, handle ports.RuntimeHandle) error { + if r.destroyHook != nil { + r.destroyHook() + } r.destroyed++ r.destroyedIDs = append(r.destroyedIDs, handle.ID) return r.destroyErr @@ -428,7 +449,11 @@ func (missingAgents) Agent(domain.AgentHarness) (ports.Agent, bool) { return nil type fakeWorkspace struct { createErr error destroyErr error - destroyed int + // destroyErrByRepo overrides destroyErr for a specific repo name (keyed by + // fakeWorkspaceRepoName), so a mixed clean+dirty workspace-project teardown is + // representable — the single destroyErr applies one error to every Destroy. + destroyErrByRepo map[string]error + destroyed int // createRepoPath, when set, is returned as the RepoPath of a single-repo // Create so tests can assert it survives the spawn->teardown metadata round // trip (production Create resolves this path; the zero default keeps every @@ -456,6 +481,10 @@ type fakeWorkspace struct { // set, is returned so best-effort handling can be exercised. excludePatterns []string addExcludeErr error + // opHook, when set, runs at the start of Destroy and Restore with the op name. + // Serialization tests use it to detect whether two disk-touching ops overlap + // for the same session (they must not, under the per-session lock). + opHook func(op string) // calls records the sequence of workspace method calls for ordering assertions. calls []string // sharedLog, when non-nil, receives entries alongside calls so ordering @@ -516,6 +545,9 @@ func (w *fakeWorkspace) CreateWorkspaceProject(_ context.Context, cfg ports.Work } func (w *fakeWorkspace) Destroy(_ context.Context, info ports.WorkspaceInfo) error { w.lastDestroyInfo = info + if w.opHook != nil { + w.opHook("Destroy") + } if info.RepoPath != "" { entry := "Destroy:" + fakeWorkspaceRepoName(info) w.calls = append(w.calls, entry) @@ -524,6 +556,11 @@ func (w *fakeWorkspace) Destroy(_ context.Context, info ports.WorkspaceInfo) err } } w.destroyed++ + if w.destroyErrByRepo != nil { + if err, ok := w.destroyErrByRepo[fakeWorkspaceRepoName(info)]; ok { + return err + } + } return w.destroyErr } func (w *fakeWorkspace) DestroyWorkspaceProject(context.Context, ports.WorkspaceProjectInfo) error { @@ -531,6 +568,9 @@ func (w *fakeWorkspace) DestroyWorkspaceProject(context.Context, ports.Workspace return w.destroyErr } func (w *fakeWorkspace) Restore(ctx context.Context, cfg ports.WorkspaceConfig) (ports.WorkspaceInfo, error) { + if w.opHook != nil { + w.opHook("Restore") + } if cfg.RepoPath != "" { entry := "Restore:" + fakeWorkspaceRepoName(ports.WorkspaceInfo{ Path: cfg.Path, @@ -1823,7 +1863,11 @@ func TestKill_WorkspaceProjectDirtyRowRefusesRemoval(t *testing.T) { if err != nil || freed { t.Fatalf("freed=%v err=%v, want dirty row to preserve workspace", freed, err) } - want := []string{"Destroy:api"} + // Continue-past-dirty (#16): a dirty child no longer aborts teardown of its + // siblings — every child is attempted child-first. Here both children are + // dirty (the fake applies one dirty error to every Destroy), so both are + // attempted and both preserved; the session is still terminated, freed=false. + want := []string{"Destroy:api", "Destroy:__root__"} if got := ws.calls; strings.Join(got, ",") != strings.Join(want, ",") { t.Fatalf("calls = %v, want %v", got, want) } @@ -2005,51 +2049,64 @@ func TestCleanup_ReclaimsTerminalWorkspaces(t *testing.T) { // the result with a reason — a silent skip leaves users staring at // "Would clean N … 0 sessions cleaned" with no explanation. func TestCleanup_ReportsSkippedWorkspaces(t *testing.T) { - m, st, _, ws := newManager() - seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) - ws.destroyErr = fmt.Errorf("gitworktree: refusing to remove: %w", ports.ErrWorkspaceDirty) - res, err := m.Cleanup(ctx, "mer") - if err != nil { - t.Fatal(err) - } - if len(res.Cleaned) != 0 { - t.Fatalf("cleaned = %v, want none", 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 has uncommitted changes" { - t.Fatalf("reason = %q", res.Skipped[0].Reason) + // A dirty worktree is preserved and reported with the uncommitted-changes reason. + // (Independent setups per case: re-running Cleanup on the same already-finalized + // session is now correctly an idempotent no-op, so each reason needs its own.) + { + m, st, _, ws := newManager() + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) + ws.destroyErr = fmt.Errorf("gitworktree: refusing to remove: %w", ports.ErrWorkspaceDirty) + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Cleaned) != 0 { + t.Fatalf("cleaned = %v, want none", 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 has uncommitted changes" { + t.Fatalf("reason = %q", res.Skipped[0].Reason) + } } // A non-dirty teardown failure is reported too — but with a fixed public // reason: the raw cause carries internal filesystem paths and belongs in // the server log, not the API response. - ws.destroyErr = errors.New("disk on fire") - res, err = m.Cleanup(ctx, "mer") - if err != nil { - t.Fatal(err) - } - if len(res.Skipped) != 1 || res.Skipped[0].Reason != "workspace teardown failed" { - t.Fatalf("skipped = %v, want fixed teardown-failed reason", res.Skipped) - } - if strings.Contains(res.Skipped[0].Reason, "disk on fire") { - t.Fatalf("raw internal error leaked into public reason: %q", res.Skipped[0].Reason) + { + m, st, _, ws := newManager() + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) + ws.destroyErr = errors.New("disk on fire") + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Skipped) != 1 || res.Skipped[0].Reason != "workspace teardown failed" { + t.Fatalf("skipped = %v, want fixed teardown-failed reason", res.Skipped) + } + if strings.Contains(res.Skipped[0].Reason, "disk on fire") { + t.Fatalf("raw internal error leaked into public reason: %q", res.Skipped[0].Reason) + } } // A teardown that fails because the session's project is archived or // unregistered (its repo can no longer be resolved) is reported with a // distinct reason telling the user the worktree must be removed by hand. - ws.destroyErr = fmt.Errorf("resolve project repo: %w", ErrProjectNotResolvable) - res, err = m.Cleanup(ctx, "mer") - if err != nil { - t.Fatal(err) - } - 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 != "project is archived or unregistered — remove worktree manually" { - t.Fatalf("reason = %q, want archived-project reason", res.Skipped[0].Reason) + { + m, st, _, ws := newManager() + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) + ws.destroyErr = fmt.Errorf("resolve project repo: %w", ErrProjectNotResolvable) + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + 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 != "project is archived or unregistered — remove worktree manually" { + t.Fatalf("reason = %q, want archived-project reason", res.Skipped[0].Reason) + } } } @@ -5166,46 +5223,11 @@ func TestReconcile_AdoptAcrossDaemonRestart(t *testing.T) { } } -func TestReconcileReap_TerminatedButAliveTmuxDestroyed(t *testing.T) { - st := newFakeStore() - rt := &fakeRuntime{aliveByHandle: map[string]bool{"t1": true}} - ws := &fakeWorkspace{} - lcm := &fakeLCM{store: st} - lookPath := func(string) (string, error) { return "/bin/true", nil } - m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: lcm, LookPath: lookPath}) - - rec := domain.SessionRecord{ - ID: "t1", ProjectID: "p1", IsTerminated: true, - Metadata: domain.SessionMetadata{RuntimeHandleID: "t1"}, - } - - if err := m.reconcileReap(context.Background(), rec); err != nil { - t.Fatalf("reconcileReap: %v", err) - } - if len(rt.destroyedIDs) != 1 || rt.destroyedIDs[0] != "t1" { - t.Fatalf("destroyedIDs = %v, want [t1]", rt.destroyedIDs) - } -} - -func TestReconcileReap_TerminatedAndDeadTmuxLeftAlone(t *testing.T) { - st := newFakeStore() - rt := &fakeRuntime{aliveByHandle: map[string]bool{}} // t2 not alive - ws := &fakeWorkspace{} - lcm := &fakeLCM{store: st} - lookPath := func(string) (string, error) { return "/bin/true", nil } - m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: lcm, LookPath: lookPath}) - - rec := domain.SessionRecord{ - ID: "t2", ProjectID: "p1", IsTerminated: true, - Metadata: domain.SessionMetadata{RuntimeHandleID: "t2"}, - } - if err := m.reconcileReap(context.Background(), rec); err != nil { - t.Fatalf("reconcileReap: %v", err) - } - if rt.destroyed != 0 { - t.Fatalf("Destroy calls = %d, want 0", rt.destroyed) - } -} +// The former TestReconcileReap_* tests were removed with reconcileReap: the +// boot reap pass (runtime-Destroy every terminated-but-alive session) is now a +// strict subset of FinalizeTerminalSession's runtime-first release, which the +// terminal-resource reconciler owns. Runtime reclaim of a terminated session is +// covered by TestFinalize_* below. // --- Send activity-confirmation tests (issue #2342) --- From 86e0a1cdaf55846344e7e2928e3fc8f6034e5808 Mon Sep 17 00:00:00 2001 From: Anuj Date: Fri, 24 Jul 2026 02:16:50 +0530 Subject: [PATCH 3/4] feat(sessions): per-session cleanup API + cleanup facts on read model (#2811) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the user-facing surface for the terminal-resource reconciler: an on-demand per-session cleanup endpoint, and the cleanup facts the UI renders from. - POST /api/v1/sessions/{sessionId}/cleanup — mirrors the Kill spine (route, nil-guard 501 handler, SessionService method, service delegate). It reclaims a single terminated session's runtime + workspace and returns the resulting facts. New manager primitive CleanupSession: 404 for an unknown session, a new ErrNotTerminal -> 409 for a still-live one, and — unlike the reconciler's FinalizeTerminalSession / bulk Cleanup — it does NOT idempotent-skip a session already in a terminal disposition, so a user can retry a preserved_dirty or failed cleanup. It never marks a session terminated. Follows Kill's LAN precedent (allowed behind bearer auth, not in lanControlBlockedPrefixes). - Expose cleanup facts on the session read model (required for the live UI): the service read assembly (toSession) joins session_cleanup_facts, and ControllersSessionView / CleanupSessionResponse carry a `cleanup` object with workspaceDisposition, runtimeReleasedAt, attemptCount, nextAttemptAt, failureCode. Derived at read time — no display status is stored. isTerminated is already on the wire via the embedded session record. - specgen: cleanupSession operation + CleanupSessionResponse / SessionCleanupView schemaNames entries; regenerated openapi.yaml + frontend schema.ts. Tests: manager CleanupSession (not-found / not-terminal / releases+returns facts / retries a terminal disposition without idempotent-skip); service delegation + ErrNotTerminal->409 mapping; read model surfaces facts for a terminated session and omits them for a live one; controller happy-path + 409 envelope + nil-Svc 501. Stacked on the reconciler (PR #2931) and reviewer-teardown (#2854). --- backend/internal/cli/dto_drift_e2e_test.go | 4 + backend/internal/domain/session.go | 4 + backend/internal/httpd/apispec/openapi.yaml | 78 +++++++++++++++ .../internal/httpd/apispec/specgen/build.go | 13 +++ backend/internal/httpd/controllers/dto.go | 36 +++++++ .../internal/httpd/controllers/sessions.go | 48 ++++++++- .../httpd/controllers/sessions_test.go | 77 ++++++++++++--- backend/internal/service/session/service.go | 32 +++++- .../internal/service/session/service_test.go | 97 ++++++++++++++++--- .../internal/session_manager/finalize_test.go | 56 +++++++++++ backend/internal/session_manager/manager.go | 38 +++++++- frontend/src/api/schema.ts | 84 ++++++++++++++++ 12 files changed, 535 insertions(+), 32 deletions(-) diff --git a/backend/internal/cli/dto_drift_e2e_test.go b/backend/internal/cli/dto_drift_e2e_test.go index 7866bad3cd..067490e2b8 100644 --- a/backend/internal/cli/dto_drift_e2e_test.go +++ b/backend/internal/cli/dto_drift_e2e_test.go @@ -92,6 +92,10 @@ func (f *fakeSessionService) Cleanup(context.Context, domain.ProjectID) (session return sessionsvc.CleanupOutcome{}, nil } +func (f *fakeSessionService) CleanupSession(context.Context, domain.SessionID) (domain.SessionCleanupRecord, error) { + return domain.SessionCleanupRecord{}, nil +} + func (f *fakeSessionService) Rename(context.Context, domain.SessionID, string) error { return nil } diff --git a/backend/internal/domain/session.go b/backend/internal/domain/session.go index 319ed268b4..b7341b70be 100644 --- a/backend/internal/domain/session.go +++ b/backend/internal/domain/session.go @@ -85,4 +85,8 @@ type Session struct { // They feed status derivation and are surfaced on the API read model. Not // serialized here: the HTTP boundary maps them to the curated wire shape. PRs []PRFacts `json:"-"` + // Cleanup holds the terminal-resource cleanup facts for a terminated session, + // nil when none exist yet (a live session, or one not yet finalized). Joined + // at read time; the HTTP boundary maps it to the curated wire shape. + Cleanup *SessionCleanupRecord `json:"-"` } diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index 5ca4ad27a1..fed03f4b3d 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -1225,6 +1225,45 @@ paths: summary: Report an agent activity-state signal for a session tags: - sessions + /api/v1/sessions/{sessionId}/cleanup: + post: + operationId: cleanupSession + parameters: + - description: Session identifier, e.g. project-1. + in: path + name: sessionId + required: true + schema: + description: Session identifier, e.g. project-1. + type: string + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/CleanupSessionResponse' + description: OK + "404": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Found + "409": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Conflict + "500": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Internal Server Error + summary: Reclaim a single terminated session's runtime + workspace resources + tags: + - sessions /api/v1/sessions/{sessionId}/kill: post: operationId: killSession @@ -2319,6 +2358,21 @@ components: - branchChanged - takenOverFrom type: object + CleanupSessionResponse: + properties: + cleanup: + $ref: '#/components/schemas/SessionCleanupView' + isTerminated: + type: boolean + ok: + type: boolean + sessionId: + type: string + required: + - ok + - sessionId + - isTerminated + type: object CleanupSessionsResponse: properties: cleaned: @@ -2352,6 +2406,8 @@ components: $ref: '#/components/schemas/DomainActivity' branch: type: string + cleanup: + $ref: '#/components/schemas/SessionCleanupView' createdAt: format: date-time type: string @@ -3240,6 +3296,28 @@ components: - sessionId - message type: object + SessionCleanupView: + properties: + attemptCount: + format: int64 + type: integer + failureCode: + type: string + nextAttemptAt: + format: date-time + type: + - "null" + - string + runtimeReleasedAt: + format: date-time + type: + - "null" + - string + workspaceDisposition: + type: string + required: + - workspaceDisposition + type: object SessionPRCISummary: properties: failingChecks: diff --git a/backend/internal/httpd/apispec/specgen/build.go b/backend/internal/httpd/apispec/specgen/build.go index 66c1fca14d..dd2b618eaf 100644 --- a/backend/internal/httpd/apispec/specgen/build.go +++ b/backend/internal/httpd/apispec/specgen/build.go @@ -167,6 +167,8 @@ var schemaNames = map[string]string{ "ControllersWorkspaceFileSummary": "WorkspaceFileSummary", "ControllersWorkspaceFileResponse": "WorkspaceFileResponse", "ControllersKillSessionResponse": "KillSessionResponse", + "ControllersCleanupSessionResponse": "CleanupSessionResponse", + "ControllersSessionCleanupView": "SessionCleanupView", "ControllersRollbackSessionResponse": "RollbackSessionResponse", "ControllersSendSessionMessageRequest": "SendSessionMessageRequest", "ControllersSendSessionMessageResponse": "SendSessionMessageResponse", @@ -947,6 +949,17 @@ func sessionOperations() []operation { {http.StatusInternalServerError, envelope.APIError{}}, }, }, + { + method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/cleanup", id: "cleanupSession", tag: "sessions", + summary: "Reclaim a single terminated session's runtime + workspace resources", + pathParams: []any{controllers.SessionIDParam{}}, + resps: []respUnit{ + {http.StatusOK, controllers.CleanupSessionResponse{}}, + {http.StatusNotFound, envelope.APIError{}}, + {http.StatusConflict, envelope.APIError{}}, + {http.StatusInternalServerError, envelope.APIError{}}, + }, + }, { method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/rollback", id: "rollbackSession", tag: "sessions", summary: "Undo a partially-completed spawn (delete seed row, or kill if spawn output exists)", diff --git a/backend/internal/httpd/controllers/dto.go b/backend/internal/httpd/controllers/dto.go index 836454a006..a50b5a0426 100644 --- a/backend/internal/httpd/controllers/dto.go +++ b/backend/internal/httpd/controllers/dto.go @@ -143,6 +143,31 @@ type SessionView struct { // Metadata. PreviewRevision int64 `json:"previewRevision,omitempty"` PRs []SessionPRFacts `json:"prs"` + // Cleanup is the terminal-resource cleanup state for a terminated session + // (runtime + workspace release progress). Omitted for a live session or one + // with no facts row yet. Derived at read time — no display status is stored. + Cleanup *SessionCleanupView `json:"cleanup,omitempty"` +} + +// SessionCleanupView is the read-model view of a terminated session's +// terminal-resource cleanup facts. Every field is a durable fact, never a +// derived display status: the client renders cleanup state (cleaning up / +// worktree kept — uncommitted changes / cleanup failed / done) from +// workspaceDisposition. +type SessionCleanupView struct { + // WorkspaceDisposition is the rollup: pending, removed, preserved_dirty, + // failed, or not_applicable. + WorkspaceDisposition string `json:"workspaceDisposition"` + // RuntimeReleasedAt is when the runtime handle was genuinely released; omitted + // when the runtime has not been confirmed released. + RuntimeReleasedAt *time.Time `json:"runtimeReleasedAt,omitempty"` + // AttemptCount is how many teardown attempts have run so far. + AttemptCount int64 `json:"attemptCount,omitempty"` + // NextAttemptAt is when the next capped-backoff retry is due; omitted when + // none is scheduled (a terminal disposition, or preserved-dirty pause). + NextAttemptAt *time.Time `json:"nextAttemptAt,omitempty"` + // FailureCode is a machine code for the last teardown failure; omitted when none. + FailureCode string `json:"failureCode,omitempty"` } // ListSessionsResponse is the body of GET /api/v1/sessions. @@ -287,6 +312,17 @@ type KillSessionResponse struct { Freed bool `json:"freed,omitempty"` } +// CleanupSessionResponse is the body of POST /api/v1/sessions/{sessionId}/cleanup: +// the resulting cleanup facts after reclaiming one terminated session's runtime + +// workspace. isTerminated is always true here (the endpoint 409s for a live +// session); it is surfaced for parity with the session read model. +type CleanupSessionResponse struct { + OK bool `json:"ok"` + SessionID domain.SessionID `json:"sessionId"` + IsTerminated bool `json:"isTerminated"` + Cleanup *SessionCleanupView `json:"cleanup,omitempty"` +} + // RollbackSessionResponse is the body of POST /api/v1/sessions/{sessionId}/rollback. // Exactly one of Deleted/Killed is true on a successful rollback; both are // false when the session was already absent or already terminated (benign). diff --git a/backend/internal/httpd/controllers/sessions.go b/backend/internal/httpd/controllers/sessions.go index 691489fea9..bb223672e0 100644 --- a/backend/internal/httpd/controllers/sessions.go +++ b/backend/internal/httpd/controllers/sessions.go @@ -70,6 +70,7 @@ type SessionService interface { Kill(ctx context.Context, id domain.SessionID) (bool, error) RollbackSpawn(ctx context.Context, id domain.SessionID) (sessionsvc.RollbackOutcome, error) Cleanup(ctx context.Context, project domain.ProjectID) (sessionsvc.CleanupOutcome, error) + CleanupSession(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) Rename(ctx context.Context, id domain.SessionID, displayName string) error SetPreview(ctx context.Context, id domain.SessionID, previewURL string) (domain.Session, error) SetTerminateOnPRMerge(ctx context.Context, id domain.SessionID, terminate bool) (domain.Session, error) @@ -115,6 +116,7 @@ func (c *SessionsController) Register(r chi.Router) { r.Post("/sessions/{sessionId}/restore", c.restore) r.Post("/sessions/{sessionId}/resume-agent", c.resumeAgent) r.Post("/sessions/{sessionId}/kill", c.kill) + r.Post("/sessions/{sessionId}/cleanup", c.cleanupSession) r.Post("/sessions/{sessionId}/rollback", c.rollback) r.Post("/sessions/{sessionId}/send", c.send) r.Post("/sessions/{sessionId}/activity", c.activity) @@ -637,6 +639,28 @@ func (c *SessionsController) rollback(w http.ResponseWriter, r *http.Request) { envelope.WriteJSON(w, http.StatusOK, RollbackSessionResponse{OK: true, SessionID: sessionID(r), Deleted: out.Deleted, Killed: out.Killed}) } +// cleanupSession reclaims a single terminated session's runtime + workspace +// (the per-session counterpart to the bulk /sessions/cleanup). It returns 409 +// for a still-live session and, for a preserved-dirty or failed one, re-attempts +// the release — the user-triggered retry the UI offers. +func (c *SessionsController) cleanupSession(w http.ResponseWriter, r *http.Request) { + if c.Svc == nil { + apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/cleanup") + return + } + facts, err := c.Svc.CleanupSession(r.Context(), sessionID(r)) + if err != nil { + envelope.WriteError(w, r, err) + return + } + envelope.WriteJSON(w, http.StatusOK, CleanupSessionResponse{ + OK: true, + SessionID: sessionID(r), + IsTerminated: true, // CleanupSession 409s a live session, so we only reach here for a terminal one. + Cleanup: cleanupFactsView(&facts), + }) +} + func (c *SessionsController) cleanup(w http.ResponseWriter, r *http.Request) { if c.Svc == nil { apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/cleanup") @@ -949,7 +973,29 @@ func previewFileURL(r *http.Request, id domain.SessionID, entry string) (string, } func sessionView(s domain.Session) SessionView { - return SessionView{Session: s, Branch: s.Metadata.Branch, PreviewURL: s.Metadata.PreviewURL, PreviewRevision: s.Metadata.PreviewRevision, PRs: sessionPRFacts(s.PRs)} + return SessionView{Session: s, Branch: s.Metadata.Branch, PreviewURL: s.Metadata.PreviewURL, PreviewRevision: s.Metadata.PreviewRevision, PRs: sessionPRFacts(s.PRs), Cleanup: cleanupFactsView(s.Cleanup)} +} + +// cleanupFactsView maps the domain cleanup record to its wire view, dropping the +// internal generation counter and rendering zero timestamps as omitted. +func cleanupFactsView(rec *domain.SessionCleanupRecord) *SessionCleanupView { + if rec == nil { + return nil + } + v := &SessionCleanupView{ + WorkspaceDisposition: string(rec.WorkspaceDisposition), + AttemptCount: rec.AttemptCount, + FailureCode: rec.FailureCode, + } + if !rec.RuntimeReleasedAt.IsZero() { + t := rec.RuntimeReleasedAt + v.RuntimeReleasedAt = &t + } + if !rec.NextAttemptAt.IsZero() { + t := rec.NextAttemptAt + v.NextAttemptAt = &t + } + return v } func sessionViews(sessions []domain.Session) []SessionView { diff --git a/backend/internal/httpd/controllers/sessions_test.go b/backend/internal/httpd/controllers/sessions_test.go index 933490278a..fa320e2a96 100644 --- a/backend/internal/httpd/controllers/sessions_test.go +++ b/backend/internal/httpd/controllers/sessions_test.go @@ -26,17 +26,20 @@ import ( ) type fakeSessionService struct { - sessions map[domain.SessionID]domain.Session - sent string - cleanupProjects []domain.ProjectID - cleanupResult []domain.SessionID - cleanupSkipped []sessionsvc.CleanupSkipped - workspaceFiles sessionsvc.WorkspaceFiles - workspaceFile sessionsvc.WorkspaceFileDetail - spawnErr error - claimErr error - listPRErr error - workspaceErr error + sessions map[domain.SessionID]domain.Session + sent string + cleanupProjects []domain.ProjectID + cleanupResult []domain.SessionID + cleanupSkipped []sessionsvc.CleanupSkipped + cleanedSessions []domain.SessionID + cleanupSessionRec domain.SessionCleanupRecord + cleanupSessionErr error + workspaceFiles sessionsvc.WorkspaceFiles + workspaceFile sessionsvc.WorkspaceFileDetail + spawnErr error + claimErr error + listPRErr error + workspaceErr error } func newFakeSessionService() *fakeSessionService { @@ -161,6 +164,14 @@ func (f *fakeSessionService) Cleanup(_ context.Context, project domain.ProjectID return sessionsvc.CleanupOutcome{Cleaned: cleaned, Skipped: f.cleanupSkipped}, nil } +func (f *fakeSessionService) CleanupSession(_ context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) { + if f.cleanupSessionErr != nil { + return domain.SessionCleanupRecord{}, f.cleanupSessionErr + } + f.cleanedSessions = append(f.cleanedSessions, id) + return f.cleanupSessionRec, nil +} + func (f *fakeSessionService) Rename(_ context.Context, id domain.SessionID, displayName string) error { s, ok := f.sessions[id] if !ok { @@ -305,6 +316,50 @@ func doPreviewOriginMethod(t *testing.T, srv *httptest.Server, method, previewUR return body, resp.StatusCode, resp.Header } +func TestCleanupSessionEndpoint(t *testing.T) { + // Happy path: 200 with the resulting cleanup facts. + svc := newFakeSessionService() + svc.cleanupSessionRec = domain.SessionCleanupRecord{SessionID: "ao-1", WorkspaceDisposition: domain.DispositionPreservedDirty, AttemptCount: 1} + srv := newSessionTestServer(t, svc) + body, status, _ := doRequest(t, srv, "POST", "/api/v1/sessions/ao-1/cleanup", "") + if status != http.StatusOK { + t.Fatalf("cleanup = %d, want 200; body=%s", status, body) + } + var resp struct { + OK bool `json:"ok"` + SessionID domain.SessionID `json:"sessionId"` + IsTerminated bool `json:"isTerminated"` + Cleanup *struct { + WorkspaceDisposition string `json:"workspaceDisposition"` + AttemptCount int64 `json:"attemptCount"` + } `json:"cleanup"` + } + mustJSON(t, body, &resp) + if !resp.OK || resp.SessionID != "ao-1" || !resp.IsTerminated { + t.Fatalf("resp = %+v", resp) + } + if resp.Cleanup == nil || resp.Cleanup.WorkspaceDisposition != "preserved_dirty" || resp.Cleanup.AttemptCount != 1 { + t.Fatalf("cleanup = %+v, want preserved_dirty/attempt 1", resp.Cleanup) + } + if len(svc.cleanedSessions) != 1 || svc.cleanedSessions[0] != "ao-1" { + t.Fatalf("cleaned = %v, want [ao-1]", svc.cleanedSessions) + } + + // Daemon-error envelope: a still-live session surfaces the service's 409. + svc2 := newFakeSessionService() + svc2.cleanupSessionErr = apierr.Conflict("SESSION_NOT_TERMINAL", "Session is still live", nil) + srv2 := newSessionTestServer(t, svc2) + body, status, _ = doRequest(t, srv2, "POST", "/api/v1/sessions/ao-1/cleanup", "") + assertErrorCode(t, body, status, http.StatusConflict, "SESSION_NOT_TERMINAL") + + // Nil service: the route stays registered and returns a 501. + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + srv3 := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{}, httpd.ControlDeps{})) + t.Cleanup(srv3.Close) + body, status, _ = doRequest(t, srv3, "POST", "/api/v1/sessions/ao-1/cleanup", "") + assertErrorCode(t, body, status, http.StatusNotImplemented, "NOT_IMPLEMENTED") +} + func TestSessionsRoutes_DefaultToStubsWithoutService(t *testing.T) { log := slog.New(slog.NewTextHandler(io.Discard, nil)) srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{}, httpd.ControlDeps{})) diff --git a/backend/internal/service/session/service.go b/backend/internal/service/session/service.go index 94fbb4d761..a449ee5f17 100644 --- a/backend/internal/service/session/service.go +++ b/backend/internal/service/session/service.go @@ -31,6 +31,9 @@ type Store interface { ListPRReviewThreads(ctx context.Context, prURL string) ([]domain.PullRequestReviewThread, error) ListPRComments(ctx context.Context, prURL string) ([]domain.PullRequestComment, error) GetProject(ctx context.Context, id string) (domain.ProjectRecord, bool, error) + // GetSessionCleanupFacts joins the terminal-resource cleanup facts onto the + // session read model; ok=false when the session has no facts row yet. + GetSessionCleanupFacts(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, bool, error) } // ListFilter captures API-facing session list query filters. @@ -51,6 +54,7 @@ type commander interface { RetireForReplacement(ctx context.Context, id domain.SessionID) error Send(ctx context.Context, id domain.SessionID, message string) error Cleanup(ctx context.Context, project domain.ProjectID) (sessionmanager.CleanupResult, error) + CleanupSession(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) RollbackSpawn(ctx context.Context, id domain.SessionID) (deleted, killed bool, err error) } @@ -461,6 +465,18 @@ func (s *Service) Kill(ctx context.Context, id domain.SessionID) (bool, error) { return freed, toAPIError(err) } +// CleanupSession reclaims a single terminated session's runtime + workspace and +// returns its resulting cleanup facts. It is the per-session counterpart to bulk +// Cleanup: a 404 for an unknown session, a 409 for a still-live one, and (for a +// preserved_dirty/failed session) a user-triggered retry of the release. +func (s *Service) CleanupSession(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) { + facts, err := s.manager.CleanupSession(ctx, id) + if err != nil { + return domain.SessionCleanupRecord{}, toAPIError(err) + } + return facts, nil +} + // RollbackSpawn deletes a seed-state session row, or falls back to a Kill if // the session has spawn output. Used by the CLI to undo a `spawn --claim-pr` // when the claim step fails, avoiding the orphan terminated row that a plain @@ -643,6 +659,8 @@ func toAPIError(err error) error { case errors.Is(err, sessionmanager.ErrResumeInProgress): return apierr.Conflict("AGENT_RESUME_IN_PROGRESS", "The agent is already being resumed", nil) + case errors.Is(err, sessionmanager.ErrNotTerminal): + return apierr.Conflict("SESSION_NOT_TERMINAL", "Session is still live; only a terminated session can be cleaned up", nil) case errors.Is(err, sessionmanager.ErrAwaitingDecision): return apierr.Conflict("SESSION_AWAITING_DECISION", "Session is paused on a permission decision; answer it in the session terminal first", nil) @@ -679,13 +697,23 @@ func (s *Service) toSession(ctx context.Context, rec domain.SessionRecord) (doma if err != nil { return domain.Session{}, fmt.Errorf("pr facts %s: %w", rec.ID, err) } - return domain.Session{ + sess := domain.Session{ SessionRecord: rec, Status: deriveStatus(rec, prs, s.now(), s.harnessSignals(rec.Harness)), SCMStatus: deriveSCMStatus(prs), TerminalHandleID: rec.Metadata.RuntimeHandleID, PRs: prs, - }, nil + } + // Join terminal-resource cleanup facts so the UI can render a terminated + // session's release progress (pending / preserved_dirty / failed / removed). + // Derived at read time — no display status is stored. + if facts, ok, err := s.store.GetSessionCleanupFacts(ctx, rec.ID); err != nil { + return domain.Session{}, fmt.Errorf("cleanup facts %s: %w", rec.ID, err) + } else if ok { + f := facts + sess.Cleanup = &f + } + return sess, nil } // now tolerates a zero-value Service (tests construct the struct literally diff --git a/backend/internal/service/session/service_test.go b/backend/internal/service/session/service_test.go index 30cbc9e03e..ab9b44d6f4 100644 --- a/backend/internal/service/session/service_test.go +++ b/backend/internal/service/session/service_test.go @@ -33,6 +33,7 @@ type fakeStore struct { reviews map[string][]domain.PullRequestReview threads map[string][]domain.PullRequestReviewThread comments map[string][]domain.PullRequestComment + cleanup map[domain.SessionID]domain.SessionCleanupRecord num int } @@ -45,9 +46,15 @@ func newFakeStore() *fakeStore { reviews: map[string][]domain.PullRequestReview{}, threads: map[string][]domain.PullRequestReviewThread{}, comments: map[string][]domain.PullRequestComment{}, + cleanup: map[domain.SessionID]domain.SessionCleanupRecord{}, } } +func (f *fakeStore) GetSessionCleanupFacts(_ context.Context, id domain.SessionID) (domain.SessionCleanupRecord, bool, error) { + rec, ok := f.cleanup[id] + return rec, ok, nil +} + func newWorkspaceRepo(t *testing.T) string { t.Helper() dir := t.TempDir() @@ -523,22 +530,25 @@ func TestSessionRenameMissingSessionReturnsNotFound(t *testing.T) { // fakeCommander records Kill/Spawn calls so a test can assert the // clean-orchestrator ordering without wiring a real session engine. type fakeCommander struct { - killed []domain.SessionID - retired []domain.SessionID - sent []domain.SessionID - sentMessages []string - cleanupProjects []domain.ProjectID - killErr error - retireErr error - sendErr error - cleanupErr error - spawnErr error - spawnRecord domain.SessionRecord - spawned bool - spawnedCfg ports.SpawnConfig - killsAtSpawn int - restoreErr error - restoreResult sessionmanager.RestoreResult + killed []domain.SessionID + retired []domain.SessionID + sent []domain.SessionID + sentMessages []string + cleanupProjects []domain.ProjectID + cleanedSessions []domain.SessionID + cleanupSessionRec domain.SessionCleanupRecord + cleanupSessionErr error + killErr error + retireErr error + sendErr error + cleanupErr error + spawnErr error + spawnRecord domain.SessionRecord + spawned bool + spawnedCfg ports.SpawnConfig + killsAtSpawn int + restoreErr error + restoreResult sessionmanager.RestoreResult } func (f *fakeCommander) Spawn(_ context.Context, cfg ports.SpawnConfig) (domain.SessionRecord, int, int, error) { @@ -597,6 +607,13 @@ func (f *fakeCommander) Cleanup(_ context.Context, project domain.ProjectID) (se Skipped: []sessionmanager.CleanupSkip{{SessionID: "mer-2", Reason: "workspace has uncommitted changes"}}, }, nil } +func (f *fakeCommander) CleanupSession(_ context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) { + if f.cleanupSessionErr != nil { + return domain.SessionCleanupRecord{}, f.cleanupSessionErr + } + f.cleanedSessions = append(f.cleanedSessions, id) + return f.cleanupSessionRec, nil +} func (f *fakeCommander) RollbackSpawn(context.Context, domain.SessionID) (bool, bool, error) { return false, false, nil } @@ -617,6 +634,54 @@ func TestCleanupMapsManagerResult(t *testing.T) { } } +// TestCleanupSessionMapsNotTerminalToConflict: the per-session cleanup API maps +// the manager's ErrNotTerminal sentinel to a 409 apierr (a live session cannot +// be cleaned up). +func TestCleanupSessionMapsNotTerminalToConflict(t *testing.T) { + svc := &Service{manager: &fakeCommander{cleanupSessionErr: sessionmanager.ErrNotTerminal}} + _, err := svc.CleanupSession(context.Background(), "mer-1") + var e *apierr.Error + if !errors.As(err, &e) || e.Kind != apierr.KindConflict || e.Code != "SESSION_NOT_TERMINAL" { + t.Fatalf("err = %v, want SESSION_NOT_TERMINAL conflict", err) + } +} + +func TestCleanupSessionReturnsFacts(t *testing.T) { + svc := &Service{manager: &fakeCommander{cleanupSessionRec: domain.SessionCleanupRecord{SessionID: "mer-1", WorkspaceDisposition: domain.DispositionPreservedDirty}}} + facts, err := svc.CleanupSession(context.Background(), "mer-1") + if err != nil { + t.Fatalf("CleanupSession: %v", err) + } + if facts.WorkspaceDisposition != domain.DispositionPreservedDirty { + t.Fatalf("disposition = %q, want preserved_dirty", facts.WorkspaceDisposition) + } +} + +// TestReadModelSurfacesCleanupFacts: the session read model joins cleanup facts +// for a terminated session and omits them for one with no facts row. +func TestReadModelSurfacesCleanupFacts(t *testing.T) { + st := newFakeStore() + st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", IsTerminated: true} + st.sessions["mer-2"] = domain.SessionRecord{ID: "mer-2", ProjectID: "mer", IsTerminated: true} + st.cleanup["mer-1"] = domain.SessionCleanupRecord{SessionID: "mer-1", WorkspaceDisposition: domain.DispositionPreservedDirty, AttemptCount: 1} + svc := NewWithDeps(Deps{Store: st}) + + withFacts, err := svc.Get(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Get mer-1: %v", err) + } + if withFacts.Cleanup == nil || withFacts.Cleanup.WorkspaceDisposition != domain.DispositionPreservedDirty { + t.Fatalf("mer-1 cleanup facts = %+v, want preserved_dirty", withFacts.Cleanup) + } + noFacts, err := svc.Get(context.Background(), "mer-2") + if err != nil { + t.Fatalf("Get mer-2: %v", err) + } + if noFacts.Cleanup != nil { + t.Fatalf("mer-2 cleanup = %+v, want nil (no facts row)", noFacts.Cleanup) + } +} + func TestTeardownProjectKillsActiveSessionsThenCleansProject(t *testing.T) { st := newFakeStore() st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer"} diff --git a/backend/internal/session_manager/finalize_test.go b/backend/internal/session_manager/finalize_test.go index d2cb2cf889..95f6dab6cf 100644 --- a/backend/internal/session_manager/finalize_test.go +++ b/backend/internal/session_manager/finalize_test.go @@ -371,6 +371,62 @@ func TestKill_SerializedWithFinalize(t *testing.T) { } } +// --- CleanupSession (per-session cleanup API primitive, PR 3) --- + +func TestCleanupSession_UnknownSessionIsNotFound(t *testing.T) { + m, _, _, _ := finalizeManager(nil) + if _, err := m.CleanupSession(ctx, "nope"); !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestCleanupSession_LiveSessionIsNotTerminal(t *testing.T) { + m, st, rt, ws := finalizeManager(nil) + st.sessions["mer-1"] = mkLive("mer-1") + _, err := m.CleanupSession(ctx, "mer-1") + if !errors.Is(err, ErrNotTerminal) { + t.Fatalf("err = %v, want ErrNotTerminal", err) + } + if rt.destroyed != 0 || ws.destroyed != 0 { + t.Fatal("a live session must not be torn down by cleanup") + } +} + +func TestCleanupSession_TerminalReleasesAndReturnsFacts(t *testing.T) { + m, st, _, ws := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + facts, err := m.CleanupSession(ctx, "mer-1") + if err != nil { + t.Fatalf("cleanup: %v", err) + } + if ws.destroyed != 1 { + t.Fatalf("workspace destroyed = %d, want 1", ws.destroyed) + } + if facts.WorkspaceDisposition != domain.DispositionRemoved { + t.Fatalf("disposition = %q, want removed", facts.WorkspaceDisposition) + } +} + +// TestCleanupSession_RetriesTerminalDisposition pins the user-retry contract: a +// session already recorded failed (or preserved_dirty) is re-attempted, NOT +// idempotent-skipped like the bulk/reconciler path. Here the retry now succeeds +// (workspace clean) so the disposition flips to removed. +func TestCleanupSession_RetriesTerminalDisposition(t *testing.T) { + m, st, _, _ := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + st.cleanup["mer-1"] = domain.SessionCleanupRecord{ + SessionID: "mer-1", SessionGeneration: 0, + WorkspaceDisposition: domain.DispositionFailed, AttemptCount: maxCleanupAttempts, FailureCode: failWorkspaceDestroy, + } + facts, err := m.CleanupSession(ctx, "mer-1") + if err != nil { + t.Fatalf("cleanup: %v", err) + } + if facts.WorkspaceDisposition != domain.DispositionRemoved { + t.Fatalf("disposition = %q, want removed (retry must not idempotent-skip a failed session)", facts.WorkspaceDisposition) + } +} + // TestKill_WritesCleanupFacts pins critique #18: every terminal path (here Kill) // persists a facts row so the sessions-driven boot scan doesn't re-enqueue it // forever. diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index e24cfaa28f..c7a9d334da 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -35,6 +35,10 @@ var ( ErrAgentExited = errors.New("session: agent exited") ErrAgentNotExited = errors.New("session: agent has not exited") ErrIncompleteHandle = errors.New("session: incomplete teardown handle") + // ErrNotTerminal means a per-session cleanup was requested for a session that + // is still live. Only a terminated session's resources may be reclaimed; the + // API maps this to a 409. + ErrNotTerminal = errors.New("session: not terminal") // ErrProjectNotResolvable means the spawn's project has no usable repo // (unregistered, archived, or missing a path). The API maps it to a 400. ErrProjectNotResolvable = errors.New("session: project repo not resolvable") @@ -1945,12 +1949,42 @@ func (m *Manager) finalizeOne(ctx context.Context, rec domain.SessionRecord) dom unlock := m.sessionLocks.lock(rec.ID) defer unlock() - generation := rec.CleanupGeneration if facts, ok, err := m.store.GetSessionCleanupFacts(ctx, rec.ID); err == nil && ok && - facts.SessionGeneration == generation && isTerminalDisposition(facts.WorkspaceDisposition) { + facts.SessionGeneration == rec.CleanupGeneration && isTerminalDisposition(facts.WorkspaceDisposition) { return facts } + return m.releaseAndPersistLocked(ctx, rec) +} + +// CleanupSession is the per-session cleanup API primitive (PR 3). Unlike the +// reconciler's FinalizeTerminalSession it surfaces typed errors to the caller +// (ErrNotFound / ErrNotTerminal) and, unlike finalizeOne, it does NOT +// idempotent-skip a session already in a terminal disposition — a user +// triggering cleanup on a preserved_dirty or failed session is explicitly asking +// to re-attempt the release. It never marks a session terminated. +func (m *Manager) CleanupSession(ctx context.Context, id domain.SessionID) (domain.SessionCleanupRecord, error) { + unlock := m.sessionLocks.lock(id) + defer unlock() + rec, ok, err := m.store.GetSession(ctx, id) + if err != nil { + return domain.SessionCleanupRecord{}, fmt.Errorf("cleanup %s: %w", id, err) + } + if !ok { + return domain.SessionCleanupRecord{}, ErrNotFound + } + if !rec.IsTerminated { + return domain.SessionCleanupRecord{}, ErrNotTerminal + } + return m.releaseAndPersistLocked(ctx, rec), nil +} + +// releaseAndPersistLocked runs the shared release core for a terminal session +// and persists the resulting facts, returning the stored record. The caller must +// hold the per-session lock. A transient failure is recorded as a pending retry +// (or exhausted `failed`) rather than surfaced. +func (m *Manager) releaseAndPersistLocked(ctx context.Context, rec domain.SessionRecord) domain.SessionCleanupRecord { + generation := rec.CleanupGeneration res, releaseErr := m.releaseTerminalResources(ctx, rec) if releaseErr != nil { res.disposition = domain.DispositionPending diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index 1cb09f9517..5eb5ff23a9 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -470,6 +470,23 @@ export interface paths { patch?: never; trace?: never; }; + "/api/v1/sessions/{sessionId}/cleanup": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** Reclaim a single terminated session's runtime + workspace resources */ + post: operations["cleanupSession"]; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/v1/sessions/{sessionId}/kill": { parameters: { query?: never; @@ -845,6 +862,12 @@ export interface components { sessionId: string; takenOverFrom: string[]; }; + CleanupSessionResponse: { + cleanup?: components["schemas"]["SessionCleanupView"]; + isTerminated: boolean; + ok: boolean; + sessionId: string; + }; CleanupSessionsResponse: { cleaned: string[]; ok: boolean; @@ -857,6 +880,7 @@ export interface components { ControllersSessionView: { activity: components["schemas"]["DomainActivity"]; branch?: string; + cleanup?: components["schemas"]["SessionCleanupView"]; /** Format: date-time */ createdAt: string; displayName?: string; @@ -1200,6 +1224,16 @@ export interface components { ok: boolean; sessionId: string; }; + SessionCleanupView: { + /** Format: int64 */ + attemptCount?: number; + failureCode?: string; + /** Format: date-time */ + nextAttemptAt?: null | string; + /** Format: date-time */ + runtimeReleasedAt?: null | string; + workspaceDisposition: string; + }; SessionPRCISummary: { failingChecks: components["schemas"]["SessionPRFailingCheck"][]; /** @enum {string} */ @@ -3055,6 +3089,56 @@ export interface operations { }; }; }; + cleanupSession: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Session identifier, e.g. project-1. */ + sessionId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["CleanupSessionResponse"]; + }; + }; + /** @description Not Found */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Conflict */ + 409: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Internal Server Error */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + }; + }; killSession: { parameters: { query?: never; From 9a9c5970a1f20c22e9c7ff74ad76e1741f154baf Mon Sep 17 00:00:00 2001 From: Anuj Date: Sat, 25 Jul 2026 01:10:45 +0530 Subject: [PATCH 4/4] fix(review): scope cleanup facts to the current terminal episode and propagate storage failures toSession attached any session_cleanup_facts row regardless of whether the session was still terminated or the facts belonged to a prior generation, so a restored session or a second termination could surface stale disposition. releaseAndPersistLocked also swallowed persist/re-read errors and returned a synthesized record, so a storage failure on the per-session cleanup endpoint looked like a 200. Also tag workspaceDisposition as a closed enum in the DTO and regenerate the OpenAPI spec and frontend schema. --- backend/internal/httpd/apispec/openapi.yaml | 6 +++ backend/internal/httpd/controllers/dto.go | 2 +- .../internal/httpd/controllers/sessions.go | 2 +- backend/internal/service/session/service.go | 15 ++++--- .../internal/service/session/service_test.go | 33 ++++++++++++++ .../internal/session_manager/finalize_test.go | 41 ++++++++++++++++++ backend/internal/session_manager/manager.go | 43 +++++++++++++++---- .../internal/session_manager/manager_test.go | 22 +++++++--- frontend/src/api/schema.ts | 3 +- 9 files changed, 143 insertions(+), 24 deletions(-) diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index fed03f4b3d..afe078a2ad 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -3314,6 +3314,12 @@ components: - "null" - string workspaceDisposition: + enum: + - pending + - removed + - preserved_dirty + - failed + - not_applicable type: string required: - workspaceDisposition diff --git a/backend/internal/httpd/controllers/dto.go b/backend/internal/httpd/controllers/dto.go index a50b5a0426..da80c48299 100644 --- a/backend/internal/httpd/controllers/dto.go +++ b/backend/internal/httpd/controllers/dto.go @@ -157,7 +157,7 @@ type SessionView struct { type SessionCleanupView struct { // WorkspaceDisposition is the rollup: pending, removed, preserved_dirty, // failed, or not_applicable. - WorkspaceDisposition string `json:"workspaceDisposition"` + WorkspaceDisposition domain.WorkspaceDisposition `json:"workspaceDisposition" enum:"pending,removed,preserved_dirty,failed,not_applicable"` // RuntimeReleasedAt is when the runtime handle was genuinely released; omitted // when the runtime has not been confirmed released. RuntimeReleasedAt *time.Time `json:"runtimeReleasedAt,omitempty"` diff --git a/backend/internal/httpd/controllers/sessions.go b/backend/internal/httpd/controllers/sessions.go index bb223672e0..b5dbe63222 100644 --- a/backend/internal/httpd/controllers/sessions.go +++ b/backend/internal/httpd/controllers/sessions.go @@ -983,7 +983,7 @@ func cleanupFactsView(rec *domain.SessionCleanupRecord) *SessionCleanupView { return nil } v := &SessionCleanupView{ - WorkspaceDisposition: string(rec.WorkspaceDisposition), + WorkspaceDisposition: rec.WorkspaceDisposition, AttemptCount: rec.AttemptCount, FailureCode: rec.FailureCode, } diff --git a/backend/internal/service/session/service.go b/backend/internal/service/session/service.go index a449ee5f17..afcf896881 100644 --- a/backend/internal/service/session/service.go +++ b/backend/internal/service/session/service.go @@ -706,12 +706,17 @@ func (s *Service) toSession(ctx context.Context, rec domain.SessionRecord) (doma } // Join terminal-resource cleanup facts so the UI can render a terminated // session's release progress (pending / preserved_dirty / failed / removed). + // Only the current terminal episode's facts qualify: a live session (Restore + // un-terminated it) or a stale generation (a second termination since these + // facts were written) must not leak the prior episode's disposition. // Derived at read time — no display status is stored. - if facts, ok, err := s.store.GetSessionCleanupFacts(ctx, rec.ID); err != nil { - return domain.Session{}, fmt.Errorf("cleanup facts %s: %w", rec.ID, err) - } else if ok { - f := facts - sess.Cleanup = &f + if rec.IsTerminated { + if facts, ok, err := s.store.GetSessionCleanupFacts(ctx, rec.ID); err != nil { + return domain.Session{}, fmt.Errorf("cleanup facts %s: %w", rec.ID, err) + } else if ok && facts.SessionGeneration == rec.CleanupGeneration { + f := facts + sess.Cleanup = &f + } } return sess, nil } diff --git a/backend/internal/service/session/service_test.go b/backend/internal/service/session/service_test.go index ab9b44d6f4..e7d7bfb7f9 100644 --- a/backend/internal/service/session/service_test.go +++ b/backend/internal/service/session/service_test.go @@ -682,6 +682,39 @@ func TestReadModelSurfacesCleanupFacts(t *testing.T) { } } +// TestReadModelHidesCleanupFactsForLiveOrStaleGeneration: a facts row from a +// prior terminal episode must not leak onto a live (Restore un-terminated) or +// newly re-terminated session — the read model only surfaces facts stamped +// with the session's current CleanupGeneration, matching the write-side guard +// finalizeOne and FinalizeTerminalSession already enforce. +func TestReadModelHidesCleanupFactsForLiveOrStaleGeneration(t *testing.T) { + st := newFakeStore() + // Restored: no longer terminated, but the prior episode's facts row lingers. + st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", IsTerminated: false, CleanupGeneration: 1} + st.cleanup["mer-1"] = domain.SessionCleanupRecord{SessionID: "mer-1", SessionGeneration: 0, WorkspaceDisposition: domain.DispositionRemoved} + // Re-terminated (generation bumped) before the reconciler wrote fresh facts + // for the new episode: the stale generation-0 row must not surface either. + st.sessions["mer-2"] = domain.SessionRecord{ID: "mer-2", ProjectID: "mer", IsTerminated: true, CleanupGeneration: 1} + st.cleanup["mer-2"] = domain.SessionCleanupRecord{SessionID: "mer-2", SessionGeneration: 0, WorkspaceDisposition: domain.DispositionRemoved} + svc := NewWithDeps(Deps{Store: st}) + + restored, err := svc.Get(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Get mer-1: %v", err) + } + if restored.Cleanup != nil { + t.Fatalf("mer-1 (restored, live) cleanup = %+v, want nil", restored.Cleanup) + } + + staleGen, err := svc.Get(context.Background(), "mer-2") + if err != nil { + t.Fatalf("Get mer-2: %v", err) + } + if staleGen.Cleanup != nil { + t.Fatalf("mer-2 (stale generation) cleanup = %+v, want nil", staleGen.Cleanup) + } +} + func TestTeardownProjectKillsActiveSessionsThenCleansProject(t *testing.T) { st := newFakeStore() st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer"} diff --git a/backend/internal/session_manager/finalize_test.go b/backend/internal/session_manager/finalize_test.go index 95f6dab6cf..842dfb6eeb 100644 --- a/backend/internal/session_manager/finalize_test.go +++ b/backend/internal/session_manager/finalize_test.go @@ -427,6 +427,47 @@ func TestCleanupSession_RetriesTerminalDisposition(t *testing.T) { } } +// TestCleanupSession_PersistFailurePropagatesError pins the correctness fix: a +// storage failure while persisting cleanup facts must surface as an error +// (the service maps it to a 500) rather than a synthesized-success record — +// the caller asked for refreshed, durable facts and got neither. +func TestCleanupSession_PersistFailurePropagatesError(t *testing.T) { + m, st, _, ws := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + boom := errors.New("db locked") + st.upsertCleanupErr = boom + + facts, err := m.CleanupSession(ctx, "mer-1") + if err == nil { + t.Fatalf("cleanup: want error on persist failure, got facts %+v", facts) + } + if !errors.Is(err, boom) { + t.Fatalf("err = %v, want wrapping %v", err, boom) + } + // The release itself must still have run (teardown is not gated on storage). + if ws.destroyed != 1 { + t.Fatalf("workspace destroyed = %d, want 1", ws.destroyed) + } +} + +// TestCleanupSession_ReloadFailurePropagatesError: same contract for the +// post-persist re-read — a storage error there must not be swallowed into a +// synthesized record either. +func TestCleanupSession_ReloadFailurePropagatesError(t *testing.T) { + m, st, _, _ := finalizeManager(nil) + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", RuntimeHandleID: "h1"}) + boom := errors.New("db unreachable") + st.getCleanupErr = boom + + _, err := m.CleanupSession(ctx, "mer-1") + if err == nil { + t.Fatal("cleanup: want error on re-read failure") + } + if !errors.Is(err, boom) { + t.Fatalf("err = %v, want wrapping %v", err, boom) + } +} + // TestKill_WritesCleanupFacts pins critique #18: every terminal path (here Kill) // persists a facts row so the sessions-driven boot scan doesn't re-enqueue it // forever. diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index c7a9d334da..befe3d667e 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -1953,7 +1953,20 @@ func (m *Manager) finalizeOne(ctx context.Context, rec domain.SessionRecord) dom facts.SessionGeneration == rec.CleanupGeneration && isTerminalDisposition(facts.WorkspaceDisposition) { return facts } - return m.releaseAndPersistLocked(ctx, rec) + facts, err := m.releaseAndPersistLocked(ctx, rec) + if err != nil { + // Bulk Cleanup has no per-session error channel (CleanupResult is + // Cleaned/Skipped only); a storage hiccup here becomes a pending retry the + // periodic sweep re-attempts, same as a release failure. + m.logger.Warn("cleanup: releaseAndPersistLocked failed", "sessionID", rec.ID, "error", err) + return domain.SessionCleanupRecord{ + SessionID: rec.ID, + SessionGeneration: rec.CleanupGeneration, + WorkspaceDisposition: domain.DispositionPending, + FailureCode: failTeardown, + } + } + return facts } // CleanupSession is the per-session cleanup API primitive (PR 3). Unlike the @@ -1976,14 +1989,22 @@ func (m *Manager) CleanupSession(ctx context.Context, id domain.SessionID) (doma if !rec.IsTerminated { return domain.SessionCleanupRecord{}, ErrNotTerminal } - return m.releaseAndPersistLocked(ctx, rec), nil + facts, err := m.releaseAndPersistLocked(ctx, rec) + if err != nil { + return domain.SessionCleanupRecord{}, fmt.Errorf("cleanup %s: %w", id, err) + } + return facts, nil } // releaseAndPersistLocked runs the shared release core for a terminal session // and persists the resulting facts, returning the stored record. The caller must -// hold the per-session lock. A transient failure is recorded as a pending retry -// (or exhausted `failed`) rather than surfaced. -func (m *Manager) releaseAndPersistLocked(ctx context.Context, rec domain.SessionRecord) domain.SessionCleanupRecord { +// hold the per-session lock. A *release* failure (runtime/workspace teardown) is +// recorded as a pending retry (or exhausted `failed`) rather than surfaced — that +// is expected, retryable steady-state. A *storage* failure (persisting or +// re-reading the facts) is returned as an error instead: the caller asked for +// refreshed facts and none were durably recorded, so reporting synthesized +// success would be a lie. +func (m *Manager) releaseAndPersistLocked(ctx context.Context, rec domain.SessionRecord) (domain.SessionCleanupRecord, error) { generation := rec.CleanupGeneration res, releaseErr := m.releaseTerminalResources(ctx, rec) if releaseErr != nil { @@ -1993,19 +2014,23 @@ func (m *Manager) releaseAndPersistLocked(ctx context.Context, rec domain.Sessio } } if err := m.persistCleanupFacts(ctx, rec, generation, res); err != nil { - m.logger.Warn("cleanup: persist facts failed", "sessionID", rec.ID, "error", err) + return domain.SessionCleanupRecord{}, fmt.Errorf("persist cleanup facts %s: %w", rec.ID, err) } // Re-read so a transient failure that just exhausted its attempt cap reports // its terminal `failed` disposition rather than the in-memory `pending`. - if stored, ok, err := m.store.GetSessionCleanupFacts(ctx, rec.ID); err == nil && ok && stored.SessionGeneration == generation { - return stored + stored, ok, err := m.store.GetSessionCleanupFacts(ctx, rec.ID) + if err != nil { + return domain.SessionCleanupRecord{}, fmt.Errorf("reload cleanup facts %s: %w", rec.ID, err) + } + if ok && stored.SessionGeneration == generation { + return stored, nil } return domain.SessionCleanupRecord{ SessionID: rec.ID, SessionGeneration: generation, WorkspaceDisposition: res.disposition, FailureCode: res.failureCode, - } + }, nil } // cleanupSkipReason renders a non-removed disposition as a short user-facing diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index 0eadbfa13d..ced104d0d8 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -22,13 +22,15 @@ import ( var ctx = context.Background() type fakeStore struct { - sessions map[domain.SessionID]domain.SessionRecord - pr map[domain.SessionID]domain.PRFacts - projects map[string]domain.ProjectRecord - workspaceRepo map[string][]domain.WorkspaceRepoRecord - num int - deleteErr error - upsertWTErr error + sessions map[domain.SessionID]domain.SessionRecord + pr map[domain.SessionID]domain.PRFacts + projects map[string]domain.ProjectRecord + workspaceRepo map[string][]domain.WorkspaceRepoRecord + num int + deleteErr error + upsertWTErr error + upsertCleanupErr error + getCleanupErr error // worktrees maps session ID to its saved worktree rows (shutdown-saved marker). worktrees map[domain.SessionID][]domain.SessionWorktreeRecord // cleanup maps session ID to its persisted terminal-resource cleanup facts. @@ -135,6 +137,9 @@ func (f *fakeStore) DeleteSessionWorktrees(_ context.Context, id domain.SessionI return nil } func (f *fakeStore) UpsertSessionCleanupFacts(_ context.Context, rec domain.SessionCleanupRecord) error { + if f.upsertCleanupErr != nil { + return f.upsertCleanupErr + } if f.cleanup == nil { f.cleanup = map[domain.SessionID]domain.SessionCleanupRecord{} } @@ -142,6 +147,9 @@ func (f *fakeStore) UpsertSessionCleanupFacts(_ context.Context, rec domain.Sess return nil } func (f *fakeStore) GetSessionCleanupFacts(_ context.Context, id domain.SessionID) (domain.SessionCleanupRecord, bool, error) { + if f.getCleanupErr != nil { + return domain.SessionCleanupRecord{}, false, f.getCleanupErr + } rec, ok := f.cleanup[id] return rec, ok, nil } diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index 5eb5ff23a9..6025c9747f 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -1232,7 +1232,8 @@ export interface components { nextAttemptAt?: null | string; /** Format: date-time */ runtimeReleasedAt?: null | string; - workspaceDisposition: string; + /** @enum {string} */ + workspaceDisposition: "pending" | "removed" | "preserved_dirty" | "failed" | "not_applicable"; }; SessionPRCISummary: { failingChecks: components["schemas"]["SessionPRFailingCheck"][];