feat(sessions): terminal-resource reconciler for terminated sessions (#2811)

[anuj7511]

Terminal-resource reconciler redesign for #2811, implementing state-driven design. This supersedes PR #2819 (the one-shot reclaim callback): on main that seam never existed, so this is a fresh reconciler rather than a removal — #2819 can be closed once this lands.

The shift

is_terminated=true is the durable intent that a session no longer wants runtime resources; the daemon must converge runtime + workspace toward safe release (cleanup may be pending, retrying, or blocked on a dirty worktree). A callback is the wrong seam: is_terminated is written on three paths (reaper death, activity/SessionEnd exit, MarkTerminated) but the old callback fired on only two reactions, so the two most common endings reclaimed nothing. A reconciler keyed off the durable fact covers every terminal transition and is crash/transient-failure recoverable.

What's here

• Shared release core — runtime-first (a failed runtime destroy leaves the workspace untouched); child-first worktree removal that continues past a dirty child so one dirty worktree can't strand its clean siblings; dirty = preserved, never forced; structured disposition rollup.
• FinalizeTerminalSession — idempotent, under a refcounted per-session lock; skips restore-pending sessions; captures sessions.cleanup_generation and drops a stale result if a Restore→Terminate cycle advanced it; capped-backoff retry with a capped attempt count (exhaustion → failed, stops auto-retry).
• Kill + bulk Cleanup funnel through the same core and persist facts on every terminal path (incl. no-workspace → not_applicable) so the sessions-driven boot scan can't re-enqueue an ever-killed session forever. Kill keeps its contract (fail-closed on runtime / non-dirty-workspace failure; preserved-dirty still terminates with freed=false).
• Reconciler runner (internal/observe/reconciler, modelled on the reaper) — live CDC session_updated wake (non-blocking send, filtered on isTerminated so a facts-write can't self-wake it), a sessions-driven boot scan that drains the pre-existing leaked backlog async, and a periodic capped-backoff retry sweep. Attempts run on a detached-but-bounded context; shutdown waits a short grace and unsubscribes before the CDC pipe stops.
• Reviewer gate — never reclaim a worktree with a live reviewer: tear the pane down (idempotent) and gate removal on its actual IsAlive.
• MarkSpawned bumps sessions.cleanup_generation in the same UPDATE that un-terminates, so the reset is atomic-with the un-terminate.
• reconcileReap deleted — its boot runtime-Destroy is a strict subset of the finalizer's runtime-first release. Boot reorder: run Reconcile/RestoreAll to completion, then start the reconciler (subscribing before its boot snapshot to close the boot-window race).

Behavior change to call out

Crash/reaper-terminated and activity-exit-terminated sessions — which leak today — now have their clean worktrees + live runtime auto-reclaimed. Uncommitted work is protected solely by the dirty→preserve rule (never force-removed).

Stacked PR

Depends on #2853 (storage foundation) and #2854 (reviewer teardown, itself on #2800). Until those merge, this diff also shows their commits — review the reclaim-terminal-reconciler top commit. Will rebase once the prerequisites land.

Tests

Finalizer clean / dirty / runtime-fail-retry / exhaustion / idempotent / generation-guard / not-applicable / reviewer-gate; workspace-project mixed clean+dirty; Kill writes facts; reconciler live-wake + boot-scan + facts-only-ignore; and a wake-path e2e through the real CDC chain (store trigger → poller → broadcaster → reconciler → finalizer).

Validation

go build ./..., go test ./..., go test -race ./..., go vet ./..., golangci-lint (v2.12.2) — all green; no sqlc drift.

[illegalcall]

Really nice design overall — state-driven off the durable is_terminated fact (with the 30s ticker as a capped-backoff retry backstop rather than terminal detection), runtime-first teardown ordering, idempotent generation-stamped facts, dirty-worktree preservation, and the reviewer-liveness gate. The keyedMutex refcounting and the shutdown close-queue choreography (unsubscribe blocks the in-flight Publish before close(queue), so no send-on-closed) are correct. 4 of the 5 core invariants hold cleanly.

One item I'd treat as gating before this leaves draft, plus two smaller notes.

1. (gating) The per-session lock doesn't actually cover Restore/Spawn — "single disk writer" is only half-enforced.

sessionLocks.lock is taken in FinalizeTerminalSession (finalize.go:288), Kill (manager.go:655) and finalizeOne (manager.go:1711) — but not in Spawn (manager.go:287), RestoreWithMode (manager.go:797) or RestoreAll (manager.go:1082). The doc comment at manager.go:165-167 states the lock serialises finalize "so a reconciler finalize can't race a concurrent Kill or Restore over the same runtime/workspace" — the Restore half isn't implemented.

Race: a terminated session whose session_worktrees are still active isn't skipped by finalize; live-wake enqueues it and it begins releaseTerminalResources → runtime.Destroy → git worktree remove. Concurrently the user clicks Restore → RestoreWithMode runs workspace.Restore + runtime.Create + un-terminate/gen-bump. The generation guard (re-read CleanupGeneration != generation → drop persist) only protects the facts write, not the physical Destroy already in flight — so the finalizer can pull the worktree out from under a freshly relaunched live session.

Fix: take sessionLocks.lock(id) in RestoreWithMode/Spawn (and per-session inside RestoreAll); the generation re-check then aborts the release correctly once the lock is re-acquired. A concurrent restore-vs-finalize test (and a Kill-vs-finalize serialization test) would lock this down. At minimum the doc comment should stop claiming Restore is covered.

2. (note) Shutdown grace (5s) < attempt timeout (2m).

run waits DefaultShutdownGrace for workerDone then returns, but a detached finalize attempt runs on a 2m context.Background() timeout and isn't cancelled on shutdown by design. A slow git op can therefore keep calling GetSession/UpsertSessionCleanupFacts after store.Close() (the deferred cleanup in daemon startup). The idempotent boot-scan covers correctness on the next start, but the write-after-close window is real — consider joining workerDone before close, or giving the attempt a shutdown-aware deadline that precedes close.

3. (nit) finalizeOne lacks the early idempotent-skip that FinalizeTerminalSession has.

FinalizeTerminalSession short-circuits when facts already record a terminal disposition for the current generation; finalizeOne (manager.go:1711) doesn't, so a bulk Cleanup over an already-removed session re-runs releaseTerminalResources and re-persists. Harmless (idempotent) but redundant, and it's an inconsistency between the two entry points — consider funneling both through the same guard.

Happy to talk any of these through. The bones are good; #1 is the one thing I'd want closed before undraft.

[anuj7511]

Thanks for the thorough review. Addressed all three (latest push):

• (1) Took the per-session lock in Spawn / RestoreWithMode / RestoreAll (extracted a locked restoreSavedSession), so a restore can no longer race a finalize into removing its worktree — the generation re-check now aborts the release once the finalizer re-acquires the lock. Added restore-vs-finalize and Kill-vs-finalize serialization tests (confirmed they fail without the lock, clean under -race). Deadlock-safe: Spawn's rollback helpers never re-enter the lock, and RestoreAll inlines its restore rather than calling the now-locking RestoreWithMode.
• (2) Finalize attempts now run on a detached base context that run force-cancels after the shutdown grace and then joins the worker before returning — bounded shutdown (no waiting the full per-attempt timeout) and no write-after-store.Close.
• (3) Both finalize paths now share the idempotent-skip guard (isTerminalDisposition). I left bulk Cleanup deliberately not adopting finalize's restore-pending skip — it's an explicit user reclaim, unlike the automatic reconciler, so it should still clean an unresumable shutdown-saved leftover. Happy to make them identical if you'd prefer.

Now that #2853 and #2800 have merged, I've rebased this onto current main (it now stacks only on #2854) and resolved the conflicts — the PR is mergeable again.

[anuj7511]

Marking this ready for review — the gating lock item and the two smaller notes are addressed, and CI is green across the board (build-test, lint, api-drift, native on macOS/Ubuntu/Windows, container, scan).

One merge-ordering note: this still stacks on #2854 (reviewer teardown), so #2854 should go in first — I'll then rebase this to drop it and it merges on its own. Happy to reorder if you'd prefer otherwise.

[anuj7511]

@illegalcall — the full #2811 redesign is now up as 5 stacked PRs:

1. feat(storage): add session cleanup-facts table and cleanup_generation (#2811) #2853 — storage foundation (cleanup-facts table + cleanup_generation) — merged
2. fix(review): destroy the reviewer pane on cancel instead of only interrupting #2854 — reviewer-pane teardown — ready, green
3. feat(sessions): terminal-resource reconciler for terminated sessions (#2811) #2931 — terminal-resource reconciler (this PR) — ready, green
4. feat(sessions): per-session cleanup API + cleanup facts on read model (#2811) #3051 — per-session cleanup API + cleanup facts on the read model — draft
5. feat(frontend): surface terminal-resource cleanup recovery on the board (#2811) #3052 — frontend recoverability (render cleanup state + retry on the Done board) — draft

Merge order is top-down: #2854 → #2931 → #3051 → #3052. Each downstream is stacked on the one above, so once an upstream lands I rebase the rest to drop it (git auto-drops the already-merged commits). #3051/#3052 are drafts only because they show their ancestors' commits until those merge — happy to un-draft whenever you'd like to look. One concern per PR throughout.