fix(sessions): reclaim the worktree on merge-triggered termination (#2811)

[anuj7511]

Fixes #2811.

When a PR is merged (or closed, or a tracker issue reaches a terminal state), the lifecycle reaction terminates the session with MarkTerminated — which only sets is_terminated and tears down no external resources. The full teardown (runtime + worktree) lives on the Kill path (POST /sessions/{id}/kill), and that button is hidden the moment a session is terminated. So every merged-PR session leaves an orphaned worktree on disk that you can't reclaim from the UI. On an active project this piles up fast. The only recovery today is the project-scoped ao session cleanup, which isn't discoverable once the session drops out of the sidebar.

There's no automatic reclaim anywhere either: the reaper skips terminated sessions, and the boot-time Reconcile reap only kills a leaked runtime, never the worktree.

What this does

Bridges the fact-driven termination to the teardown the session manager already owns, without dragging runtime/workspace deps into the lifecycle layer:

• session_manager: pull the per-session teardown out of Cleanup's loop into reclaimTerminatedWorktree (no behavior change), and add ReclaimOnTerminate(id) — best-effort, skips dirty worktrees (never force-removed), no-ops for an unknown or still-live session. It runs the teardown under context.WithoutCancel so a daemon shutdown can't interrupt git worktree remove half-way (same reasoning as fix: use context.Background for spawn rollback cleanup #2731).
• lifecycle: a narrow optional terminationReclaimer seam satisfied by the session manager, wired after construction via SetTerminationReclaimer (the lcm and session manager are built over each other, so it can't be a constructor option). MarkTerminated is split so the reclaim fires only on the actual termination transition — repeat observations on an already-terminal PR or issue stay no-ops. The merge/close and tracker-terminal reaction sites now reclaim; Kill's own MarkTerminated is untouched, so nothing double-tears-down.
• daemon: one line in startSession to wire it up.

The reclaim is best-effort throughout: a dirty worktree is preserved and a teardown error is logged, never surfaced, so a filesystem hiccup can't wedge the observation pipeline or leave the row un-terminated.

Backend-only, no API or SQLite regeneration.

Tests

• lifecycle unit: merge and tracker-terminal reclaim; a merged PR with an open sibling does not reclaim; idempotent repeats don't re-tear-down; a reclaim error is swallowed; merge without a reclaimer wired still terminates.
• session_manager unit: ReclaimOnTerminate reclaims runtime + worktree, preserves a dirty worktree, and no-ops for a live or unknown session.
• integration (internal/integration/reclaim_worktree_test.go): over a real SQLite store and a real git-worktree adapter, a merged-PR observation driven through the real PR service physically removes the worktree directory from disk, and a dirty worktree is preserved.
• Existing Cleanup tests still pass over the extracted helper.

go build, go vet, go test ./..., go test -race, and golangci-lint all pass.

Known boundary

A session already terminated by another path before its PR merges (e.g. the reaper marking it terminated on runtime death) hits the no-transition path and isn't reclaimed by the later merge; reaper-death sessions are also left alone since they may hold uncommitted work. Reclaiming those uniformly needs a boot-time or periodic reconcile pass, which I've left out to keep this change scoped to the merge path.

[illegalcall]

Thanks for taking this on. The PR identifies the immediate failure correctly, and there is valuable work here that we should preserve. After tracing all of the terminal-state paths, startup wiring, reconciliation, restore behavior, and frontend visibility, I think we should adjust the core approach before treating this as the permanent fix.

Architecture we want to follow

The durable invariant should be:

When a session has is_terminated=true, it no longer wants runtime resources, and the daemon must converge its runtime and workspace toward safe release.

Terminal state is the durable cleanup intent; it does not promise that cleanup has already succeeded. Cleanup may still be pending, retrying, or intentionally blocked because a worktree is dirty.

That keeps the existing ownership boundaries intact:

• lifecycle owns durable session facts and remains independent of runtime/workspace adapters;
• session_manager owns resource teardown sequencing and safety;
• a reconciler drives terminal sessions toward the desired resource state;
• the API/UI derives cleanup presentation from durable resource facts rather than storing another display session status;
• dirty registered worktrees are never force-deleted.

The intended flow is:

SCM / tracker / reaper / activity / manual termination
                          |
                          v
              lifecycle persists terminal fact
                          |
                immediate wake + boot scan
                + bounded periodic retry
                          |
                          v
            session resource finalizer
                runtime -> safe workspace
                          |
                          v
              cleanup facts -> API/UI

The important distinction from the current PR is that cleanup must not depend on a one-time callback from a particular termination source. The callback can fail after the terminal transaction commits, the process can crash, or another path can terminate the session. The durable terminal fact must be enough for the daemon to discover and finish the work later.

This also resolves the documented ownership tension cleanly: lifecycle's MarkTerminated can continue to record facts only, while session_manager remains the only component that knows how to release resources.

Why the current callback should not be the final seam

SetTerminationReclaimer has the right goal—avoiding runtime/workspace dependencies in lifecycle—but it introduces a mutable, post-construction dependency and makes correctness depend on startup order.

Today the observers can start before startSession constructs the session manager and installs the callback. A merge observed during that window can commit termination with no reclaimer. Since SCM discovery excludes terminal sessions, that session may never be retried.

The callback is also tied only to the SCM/tracker reaction sites. Runtime observations and activity-exit signals can set is_terminated directly, so new or existing terminal paths can bypass reclamation.

Finally, the current path is effectively at-most-once: reclaim only runs on the changed=true transition, reclaim errors are swallowed, and repeat observations cannot repair the side effect. An immediate signal is useful for responsiveness, but it cannot be the correctness mechanism.

Implementation steps

1. Remove the lifecycle reclaimer dependency

• Remove terminationReclaimer, SetTerminationReclaimer, and the post-construction wiring.
• Keep lifecycle focused on atomically persisting logical session facts.
• If immediate cleanup needs a signal, either return a termination outcome to the application service or use the existing session change event to wake the finalizer.
• Do not gate eventual finalization on changed=true; repeated reconciliation must remain safe.

2. Introduce one idempotent session-manager finalization operation

Factor a per-session operation with a contract equivalent to FinalizeTerminalSession(ctx, sessionID).

It should:

1. Acquire a keyed per-session operation lock.
2. Re-read the session and confirm it is still terminal.
3. Check restore/worktree markers so it does not fight RestoreAll.
4. Destroy the runtime first.
5. If runtime destruction fails, stop and leave the workspace untouched.
6. Safely remove the normal worktree, or workspace-project worktrees child-first.
7. Treat a dirty worktree as preserved, never force-remove it.
8. Treat other workspace errors as retryable.
9. Clean auxiliary agent/prompt resources only after safe workspace handling.
10. Return a structured cleanup result.

Runtime and workspace destruction are already effectively idempotent for missing resources, so a crash between phases can be recovered by repeating this operation.

Explicit Kill and bulk Cleanup should reuse the same low-level release primitive where practical, while their user-visible contracts can remain distinct.

3. Add terminal-resource reconciliation

Add a small reconciler/finalizer runner, owned beside session_manager rather than inside lifecycle.

It should:

• make an immediate best-effort attempt after a terminal transition;
• run a boot pass over unfinished terminal sessions;
• retry transient failures periodically with capped backoff;
• use a bounded context for every attempt;
• pause automatic retries for preserved-dirty worktrees until the user explicitly retries.

Do not use change_log as a durable jobs queue. It can wake the reconciler, but current database state must remain authoritative. A generic jobs/outbox framework is unnecessary here because is_terminated already provides durable intent.

Boot ordering should be adjusted so lifecycle and session_manager are fully constructed first, adoption/restore reconciliation runs, terminal-resource reconciliation runs, and only then observers/reaper begin producing new observations.

4. Persist reliable cleanup facts

workspacePath is not cleanup state: it remains populated after successful destruction and cannot represent runtime cleanup or dirty preservation.

Persist a compact resource-finalization record. A separate table is likely clearer than overloading session_worktrees. Store raw operational facts, for example:

• the termination generation this attempt belongs to;
• runtime_released_at;
• workspace disposition: pending, removed, preserved_dirty, or not_applicable;
• attempt count, last_attempt_at, and optional next_attempt_at;
• a safe failure code such as runtime_teardown_failed or workspace_teardown_failed.

Do not persist another display session status. The API can derive Cleaning up, Worktree preserved, or Retry available from these facts.

The result must be versioned or atomically reset across Restore -> Terminate cycles. Otherwise a successful result from an earlier termination could incorrectly satisfy a later one.

Add the migration, queries, sqlc generation, and trigger-backed CDC changes required by the chosen representation.

5. Add the per-session API

Add an idempotent endpoint along the lines of:

POST /api/v1/sessions/{id}/cleanup

It should:

• operate on terminal sessions;
• invoke the same finalization primitive;
• safely no-op when resources are already finalized;
• return the refreshed cleanup facts;
• preserve the existing API error envelope/request ID behavior.

Keep the existing project-scoped bulk cleanup as the administrative path, but make it delegate to the same per-session primitive.

Regenerate OpenAPI and frontend types with npm run api.

6. Restore UI recoverability

The frontend currently removes the session from active navigation and hides Kill as soon as the session becomes merged/terminated. The Done/Terminated card or inspector should expose resource cleanup information.

Recommended behavior:

• pending/automatic retry: show a small Cleaning up or Retrying indicator;
• complete: no persistent action is necessary;
• preserved dirty: show Worktree kept — uncommitted changes and Try cleanup again;
• retryable/exhausted failure: show a safe explanation and Retry cleanup.

Keep the primary Merged/Terminated display status unchanged. Cleanup is secondary resource state.

The frontend should retain the raw terminal fact rather than infer eligibility from status === terminated, because merged sessions are also terminal.

7. Serialize cleanup and restore

Use a shared per-session keyed lock for finalization, manual cleanup, Kill, and Restore paths.

The finalizer must re-check terminal state while holding the lock and before destructive phases. This prevents a background cleanup attempt from racing a user-initiated restore.

8. Expand the tests around the invariant

Please retain the useful tests already in this PR and add coverage for:

• clean runtime and workspace teardown;
• dirty worktree preservation with an exposed result;
• runtime-destroy failure leaving the workspace untouched;
• retry after transient workspace failure;
• crash/restart equivalent: terminal fact exists but no cleanup result;
• idempotent retry after runtime has already disappeared;
• idempotent retry after workspace has already disappeared;
• repeated merge observations;
• runtime/reaper and activity-exit termination paths;
• restore racing with finalization;
• restored session terminated a second time;
• workspace-project partial/child-first cleanup;
• boot ordering and terminal reconciliation;
• bounded cancellation/shutdown behavior;
• per-session API success, validation, and daemon error envelopes;
• UI states and retry action for merged as well as terminated sessions.

What this PR has already done well

The following work is directionally correct and should be reused:

• It accurately identifies that MarkTerminated records a fact but does not release external resources.
• It keeps direct runtime/workspace adapter knowledge out of lifecycle.
• It places the teardown implementation in session_manager.
• It extracts useful per-session cleanup logic from the bulk Cleanup loop.
• It attempts runtime and worktree reclamation automatically for merge/tracker completion.
• It uses safe workspace destruction and preserves dirty worktrees.
• It adds valuable unit and real SQLite/git-worktree integration coverage.
• It tests sibling-PR completion behavior and repeat observations.
• It explicitly documents the known reaper/previously-terminal boundary rather than hiding it.

Those are good foundations. In particular, the helper extraction, dirty-worktree cases, and integration-test setup should survive the redesign.

What remains before this is the complete fix

The remaining architectural work is:

• replace the mutable lifecycle callback with state-driven reconciliation;
• cover every terminal transition, not only SCM/tracker reactions;
• make cleanup recoverable after crashes and transient failures;
• enforce runtime-failure-before-workspace safety;
• add bounded contexts rather than an unbounded context.WithoutCancel operation;
• fix construction/startup ordering;
• serialize finalization against Restore/Kill/manual cleanup;
• record reliable per-resource outcomes;
• add a per-session cleanup/retry API;
• expose dirty/failure recovery in the Done/Terminated UI;
• add the boot, retry, failure-ordering, restore-race, and frontend/API tests above.

So the requested change is not to discard this PR. It is to keep its teardown work and tests, then replace the one-shot merge callback with an idempotent terminal-resource reconciler. That gives us the automatic behavior requested by #2811 while staying consistent with the rewrite architecture and remaining maintainable as more termination sources are added.

[somewherelostt]

Thanks for contributing to Agent Orchestrator.

This PR is being picked up by the current external contributor on-call pair:

• @neversettle17-101
• @somewherelostt

If someone is already working on this, please continue as usual.
The on-call pair is added for visibility, tracking, and support, not to take over the work.
If you need help with review, direction, reproduction, or next steps, please tag @neversettle17-101 and @somewherelostt here.

For faster context or live questions, you can also join the AO Discord.

Join the session here:
https://discord.gg/H6ZDcUXmq

Come by if you want to see what is being built, ask questions, or just hang around with the community.

[anuj7511]

Thanks for the thorough review, @illegalcall. I've worked the design through and I'm confident in the direction. Here's the plan. It's 5 PRs, and this PR (#2819) becomes PR 2.

2 decisions before starting implementation:

• I reuse the existing session_updated CDC event for both the finalizer wake and the live-UI nudge, rather than adding a new event_type. Adding one would force a change_log CHECK-widen rebuild that recreates every trigger — which would collide with, and silently revert, my own open PR fix(sessions): auto-update other clients when a session is renamed #2798 (session-rename adds display_name to sessions_cdc_update). Reuse avoids the rebuild entirely: the facts-table trigger emits session_updated with a {id}-only payload, WHEN-guarded to exclude the retry-bookkeeping columns so it can't self-wake the reconciler, and the live-wake path filters on isTerminated and never immediate-retries (retries stay on next_attempt_at).
• Designing this surfaced two pre-existing bugs on main that I'm handling explicitly: the AO reviewer pane leaks today, and legacy/backlog terminal rows can reach cleanup with an unresolvable runtime handle (session-manager: runtimeHandle and tmuxName are dropped to null when lifecycle transitions to terminated, losing routing information permanently #1458).

PR 1 — Storage foundation (no behavior change). A cleanup-facts table (workspace_disposition ∈ pending/removed/preserved_dirty/failed/not_applicable, runtime_released_at, attempt_count, next_attempt_at, failure_code; no stored display status) plus a monotonic sessions.cleanup_generation column, sqlc + store. Migration 0024, coordinated with #2798 — whichever lands second rebases to 0025. The facts-table trigger reuses session_updated; no change_log rebuild.

PR 2a — Reviewer runtime teardown (pre-existing-bug fix, stacked on #2800). The AO reviewer runs in the worker's worktree as a separate pane "review-"+workerID, but Cancel only interrupts it — so a cancelled reviewer leaks the pane today (the #2715 family). Since my open #2800 (reviewer-harness-respawn) already introduces the reviewer Destroy seam, PR 2a stacks on it and adds an idempotent Launcher.Teardown + Engine.TeardownReviewer(workerID), wired into Cancel so the pane is destroyed rather than merely interrupted. (The harness-change supersede case is already handled by #2800's destroy-on-spawn.) Independent of the reclaim work; it lets PR 2 safely reclaim a worker's worktree without corrupting an in-flight reviewer's cwd.

PR 2 — Finalizer + reconciler (this PR). Removes the callback seam (lifecycle back to fact-only, which also breaks the construction cycle). Extracts one low-level release core — runtime-first (abort + retry if runtime destroy fails; don't treat a nil Destroy on an unrouted handle as success, #1458), safe child-first worktree removal, dirty preserved, and clean multi-repo children reclaimed even when a sibling is dirty (per-repo state stays in session_worktrees) — reused by Kill, bulk Cleanup, and the new FinalizeTerminalSession. The finalizer takes a per-session keyed lock, re-checks terminal + no restorable marker + generation, and tears down + awaits any live reviewer (via PR 2a) before the worktree phase. The reconciler wakes on session_updated (non-blocking subscriber), runs a sessions-driven boot scan (sessions LEFT JOIN facts, so it heals the pre-existing leaked backlog — not just rows that already have facts), and retries transient failures with capped backoff plus a capped attempt count → failed. Boot reorders so adoption/restore reconciliation completes, the CDC subscription registers, then observers start. The Restore→Terminate reset is atomic by construction: MarkSpawned bumps cleanup_generation in the same UPDATE as is_terminated=false.

PR 3 — Per-session cleanup API. POST /api/v1/sessions/{id}/cleanup (idempotent, terminal-only, returns refreshed facts, preserves the error envelope), bulk cleanup delegates to the same primitive, and the session read model exposes the facts so the UI can derive presentation at read time.

PR 4 — Frontend recoverability. Cleanup state + retry affordance on the Done/Terminated card, keyed off the raw is_terminated fact (so merged sessions get it too, not just terminated ones); the primary Merged/Terminated label stays unchanged.

Mapping to your 8 steps: (1)(2)(3)(7) → PR 2, (4) → PR 1, (5) → PR 3, (6) → PR 4, (8) tests throughout — including an end-to-end test that flips is_terminated and asserts convergence through the real session_updated → reconciler → finalizer chain, not just the primitive in isolation. All the useful teardown work and tests from this PR carry over (dirty-worktree handling, the extracted per-session helper, the real-SQLite/git-worktree integration harness — the latter moves to awaiting async finalization).

To set expectations on when the leak closes: PR 2 lands automatic reclaim of clean worktrees + live runtime for every terminal path plus capped-backoff retry of transient failures; preserved-dirty and exhausted-failure cases are safely preserved at PR 2 but only become user-recoverable at PR 3–4.

Does this staging look right before I start? I'd especially value your read on (a) reusing session_updated vs a dedicated event type given the #2798 interaction, and (b) pulling the reviewer-pane fix into its own PR (2a).

[anuj7511]

@illegalcall I've opened the first two prerequisites: #2853 (storage foundation — session_cleanup_facts table + additive cleanup_generation) and #2854 (reviewer-pane teardown on cancel). Both are self-contained, and independently reviewable. Would appreciate a review when you get a chance.

Small update for PR2a: since #2800 already introduces the reviewer Destroy, #2854 stacks on it and adds the Teardown / TeardownReviewer + cancel wiring rather than adding Destroy itself. The harness-change supersede case is already handled by #2800 destroy-on-spawn, so 2a only wires teardown into Cancel.