feat(sessions): per-session cleanup API + cleanup facts on read model (#2811)#3051
Draft
anuj7511 wants to merge 4 commits into
Draft
feat(sessions): per-session cleanup API + cleanup facts on read model (#2811)#3051anuj7511 wants to merge 4 commits into
anuj7511 wants to merge 4 commits into
Conversation
…rrupting
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 AgentWrapper#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.
…gentWrapper#2811) 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.
…AgentWrapper#2811) 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 AgentWrapper#2931) and reviewer-teardown (AgentWrapper#2854).
illegalcall
requested changes
Jul 24, 2026
illegalcall
left a comment
illegalcall
left a comment
Collaborator
There was a problem hiding this comment.
The endpoint/read-model shape is good, but two correctness issues need to be fixed before this leaves draft.
-
Blocking — cleanup facts are exposed across terminal generations.
Service.toSessionattaches anysession_cleanup_factsrow without checkingrec.IsTerminatedorfacts.SessionGeneration == rec.CleanupGeneration. After Restore, a live session can expose old cleanup state; after a second termination, the UI can receive the prior episode asremoveduntil reconciliation updates it. Only surface facts for the current terminal generation, and add restored/live plus second-generation tests. -
Blocking — storage failures masquerade as HTTP 200.
releaseAndPersistLockedlogs and swallows both persistence and re-read failures, then returns synthesized or stale facts. The per-session endpoint promises refreshed facts and the normal API error envelope; a database failure should propagate through the service/controller as a 500 rather than report success. Change the helper to return(record, error)and test the envelope.
Follow-up: SessionCleanupView.workspaceDisposition is an unconstrained string in OpenAPI even though this is a closed domain enum. Use the domain type or an explicit enum tag so generated clients cannot silently accept unsupported values.
Requesting changes for the two API/read-model correctness issues.
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3 of the terminal-resource reconciler redesign for #2811: the per-session cleanup API and the cleanup facts the UI renders from.
POST /api/v1/sessions/{sessionId}/cleanup— mirrors the Kill spine (route → nil-guard-501 handler →SessionServicemethod → service delegate). Reclaims one terminated session's runtime + workspace and returns the resulting facts. New manager primitiveCleanupSession: 404 for unknown, newErrNotTerminal→ 409 for a still-live session, and — unlike the automatic reconciler/bulk path — it does not idempotent-skip, so a user can retry apreserved_dirtyorfailedcleanup. Never marks a session terminated. Follows Kill's LAN precedent.session_cleanup_facts, andControllersSessionView/CleanupSessionResponsecarry acleanupobject (workspaceDisposition,runtimeReleasedAt,attemptCount,nextAttemptAt,failureCode). Derived at read time — no display status stored.cleanupSessionoperation + regeneratedopenapi.yaml+frontend/src/api/schema.ts.Stacked PR
Depends on #2931 (reconciler) and #2854 (reviewer teardown). Until those merge this diff also shows their commits — review the
reclaim-cleanup-apitop commit. Rebases to stand alone once the ancestors land.Tests
Manager (not-found / not-terminal / releases+returns facts / retries a terminal disposition without idempotent-skip); service delegation + 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.
Validation
go build/test/-race/vet, golangci-lint, httpd spec-drift/parity — all green.