diff --git a/.changeset/auto-cleanup-on-merge.md b/.changeset/auto-cleanup-on-merge.md new file mode 100644 index 000000000..3cc639bc1 --- /dev/null +++ b/.changeset/auto-cleanup-on-merge.md @@ -0,0 +1,19 @@ +--- +"@aoagents/ao-core": minor +--- + +Sessions whose PRs are detected as merged now auto-terminate (tmux kill + worktree remove + metadata archive) instead of lingering in the active `sessions/` directory with a `merged` status. `ao status` and `ao session ls` stay clean without an external watchdog. + +Enabled by default. Guarded by an idleness check so in-flight agents are not killed mid-task; deferred cleanups retry on each lifecycle poll until the agent idles or a 5-minute grace window elapses. + +Opt out or tune via the new top-level `lifecycle` config in `agent-orchestrator.yaml`: + +```yaml +lifecycle: + autoCleanupOnMerge: false # preserve merged worktrees for inspection + mergeCleanupIdleGraceMs: 300000 # grace window before forcing cleanup +``` + +`sessionManager.kill()` now takes an optional `reason` (`"manually_killed" | "pr_merged" | "auto_cleanup"`) and returns `KillResult` (`{ cleaned, alreadyTerminated }`) instead of `void`. All existing call sites ignore the return value so this is backward-compatible in practice. + +Closes #1309. Part of #536. diff --git a/agent-orchestrator.yaml.example b/agent-orchestrator.yaml.example index 4ca87ceb8..dd9f385ea 100644 --- a/agent-orchestrator.yaml.example +++ b/agent-orchestrator.yaml.example @@ -20,6 +20,15 @@ port: 3000 # # Uses caffeinate -i -w — auto-releases when AO exits # # Note: lid-close sleep is enforced by hardware and cannot be prevented +# Lifecycle — controls how AO cleans up sessions after their PRs merge +# lifecycle: +# autoCleanupOnMerge: true # Default. When a PR is detected as merged, tear down +# # the tmux session, remove the worktree, and archive +# # metadata so `ao status` stays clean. Set false if +# # you want merged worktrees preserved for inspection. +# mergeCleanupIdleGraceMs: 300000 # Grace window (ms) before forcing cleanup on an agent +# # that is still active at merge time. Default 5 min. + # Default plugins (these are the defaults — you can omit this section) defaults: runtime: tmux # tmux | process diff --git a/docs/observability.md b/docs/observability.md index ab7da76e8..1849f055a 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -35,6 +35,9 @@ Counters are emitted per project and operation: - `cleanup` (`session.cleanup`) - `send` (`session.send`) - `lifecycle_poll` (`lifecycle.poll`, `lifecycle.transition`) +- `lifecycle_poll` (`lifecycle.merge_cleanup.completed`) — auto-cleanup ran after a PR was detected as merged; session runtime + worktree + metadata were torn down +- `lifecycle_poll` (`lifecycle.merge_cleanup.deferred`) — auto-cleanup is waiting for the agent to idle (or for the `mergeCleanupIdleGraceMs` window to elapse) before tearing down +- `lifecycle_poll` (`lifecycle.merge_cleanup.failed`) — auto-cleanup threw during `sessionManager.kill()`; the session stays in `merged` so the next poll retries - `api_request` (web API routes) - `sse_connect`, `sse_snapshot`, `sse_disconnect` - `websocket_connect`, `websocket_disconnect`, `websocket_error` (websocket servers) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index a630693d9..ebd31c40d 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -2686,3 +2686,143 @@ describe("summary pinning", () => { await expect(lm.check("app-1")).resolves.not.toThrow(); }); }); + +describe("auto-cleanup on merge (#1309)", () => { + function mergedScm() { + return createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + } + + function configWithLifecycle( + overrides: Partial<{ autoCleanupOnMerge: boolean; mergeCleanupIdleGraceMs: number }>, + ): OrchestratorConfig { + return { + ...config, + lifecycle: { + autoCleanupOnMerge: overrides.autoCleanupOnMerge ?? true, + mergeCleanupIdleGraceMs: overrides.mergeCleanupIdleGraceMs ?? 300_000, + }, + }; + } + + it("kills session with reason=pr_merged when PR merges and agent is idle", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mergedScm(), + }); + const lm = setupCheck("app-1", { + session: makeSession({ status: "approved", pr: makePR(), activity: "idle" }), + registry, + configOverride: configWithLifecycle({}), + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).toHaveBeenCalledWith("app-1", { + purgeOpenCode: true, + reason: "pr_merged", + }); + }); + + it("defers cleanup when agent is still active and records pending marker", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mergedScm(), + }); + const lm = setupCheck("app-1", { + session: makeSession({ status: "approved", pr: makePR(), activity: "active" }), + registry, + configOverride: configWithLifecycle({}), + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).not.toHaveBeenCalled(); + const meta = readMetadataRaw(env.sessionsDir, "app-1"); + expect(meta?.["mergedPendingCleanupSince"]).toMatch(/\d{4}-\d{2}-\d{2}T/); + expect(meta?.["status"]).toBe("merged"); + }); + + it("forces cleanup after grace window elapses even if agent is still active", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mergedScm(), + }); + const pendingSince = new Date(Date.now() - 10 * 60_000).toISOString(); // 10min ago + const lm = setupCheck("app-1", { + session: makeSession({ + status: "approved", + pr: makePR(), + activity: "active", + metadata: { mergedPendingCleanupSince: pendingSince }, + }), + registry, + configOverride: configWithLifecycle({ mergeCleanupIdleGraceMs: 300_000 }), + metaOverrides: { mergedPendingCleanupSince: pendingSince }, + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).toHaveBeenCalledWith("app-1", { + purgeOpenCode: true, + reason: "pr_merged", + }); + }); + + it("does not trigger cleanup when autoCleanupOnMerge is disabled", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mergedScm(), + }); + const lm = setupCheck("app-1", { + session: makeSession({ status: "approved", pr: makePR(), activity: "idle" }), + registry, + configOverride: configWithLifecycle({ autoCleanupOnMerge: false }), + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).not.toHaveBeenCalled(); + expect(lm.getStates().get("app-1")).toBe("merged"); + }); + + it("does not trigger cleanup for terminated/killed sessions (no self-recursion)", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + }); + const lm = setupCheck("app-1", { + session: makeSession({ status: "killed", activity: "exited" }), + registry, + configOverride: configWithLifecycle({}), + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).not.toHaveBeenCalled(); + }); + + it("retains merged status when kill() fails so the next poll retries", async () => { + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mergedScm(), + }); + vi.mocked(mockSessionManager.kill).mockRejectedValueOnce(new Error("tmux busy")); + const lm = setupCheck("app-1", { + session: makeSession({ status: "approved", pr: makePR(), activity: "idle" }), + registry, + configOverride: configWithLifecycle({}), + }); + + await lm.check("app-1"); + + expect(mockSessionManager.kill).toHaveBeenCalledTimes(1); + const meta = readMetadataRaw(env.sessionsDir, "app-1"); + expect(meta?.["status"]).toBe("merged"); + expect(meta?.["mergedPendingCleanupSince"]).toMatch(/\d{4}-\d{2}-\d{2}T/); + }); +}); diff --git a/packages/core/src/__tests__/session-manager/lifecycle.test.ts b/packages/core/src/__tests__/session-manager/lifecycle.test.ts index 6a656cf5f..a1019b9dd 100644 --- a/packages/core/src/__tests__/session-manager/lifecycle.test.ts +++ b/packages/core/src/__tests__/session-manager/lifecycle.test.ts @@ -160,7 +160,10 @@ describe("kill", () => { const sm = createSessionManager({ config, registry: registryWithFail }); // Should not throw even though runtime.destroy fails - await expect(sm.kill("app-1")).resolves.toBeUndefined(); + await expect(sm.kill("app-1")).resolves.toEqual({ + cleaned: true, + alreadyTerminated: false, + }); }); it("does not purge mapped OpenCode session on default kill", async () => { diff --git a/packages/core/src/__tests__/test-utils.ts b/packages/core/src/__tests__/test-utils.ts index 8fa24cc72..70d2785d0 100644 --- a/packages/core/src/__tests__/test-utils.ts +++ b/packages/core/src/__tests__/test-utils.ts @@ -417,7 +417,7 @@ export function createMockSessionManager(): SessionManager { listCached: vi.fn().mockResolvedValue([]), invalidateCache: vi.fn(), get: vi.fn().mockResolvedValue(null), - kill: vi.fn().mockResolvedValue(undefined), + kill: vi.fn().mockResolvedValue({ cleaned: true, alreadyTerminated: false }), cleanup: vi.fn().mockResolvedValue({ killed: [], skipped: [], errors: [] }), send: vi.fn().mockResolvedValue(undefined), claimPR: vi.fn().mockResolvedValue({ diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 220790e52..bf18a1973 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -243,12 +243,39 @@ const DashboardConfigSchema = z.object({ attentionZones: z.enum(["simple", "detailed"]).default("simple"), }); +const LifecycleConfigSchema = z + .object({ + /** + * When a session's PR is detected as merged, automatically tear down the + * tmux runtime, remove the worktree, and archive the session metadata. + * Defaults to true so `ao status` does not retain stale merged entries. + */ + autoCleanupOnMerge: z.boolean().default(true), + /** + * Maximum time (ms) to wait after a session enters `merged` before forcing + * cleanup regardless of agent activity. Defaults to 5 minutes. Use `0` to + * disable the grace window (cleanup runs immediately even if the agent is + * still active). Values between 1 and 9999 are rejected to catch the common + * mistake of writing seconds (e.g. `5`) when milliseconds are expected. + */ + mergeCleanupIdleGraceMs: z + .number() + .nonnegative() + .refine((v) => v === 0 || v >= 10_000, { + message: + "mergeCleanupIdleGraceMs is in milliseconds; values between 1 and 9999 are likely a units mistake (use 0 to disable the gate, or e.g. 10000 for 10s, 300000 for 5min)", + }) + .default(300_000), + }) + .default({}); + const OrchestratorConfigSchema = z.object({ port: z.number().default(3000), terminalPort: z.number().optional(), directTerminalPort: z.number().optional(), readyThresholdMs: z.number().nonnegative().default(300_000), power: PowerConfigSchema, + lifecycle: LifecycleConfigSchema, defaults: DefaultPluginsSchema.default({}), plugins: z.array(InstalledPluginConfigSchema).default([]), dashboard: DashboardConfigSchema.optional(), diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index f00fc66e6..42e0633ea 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -13,6 +13,7 @@ import { randomUUID } from "node:crypto"; import { SESSION_STATUS, + ACTIVITY_STATE, PR_STATE, CI_STATUS, TERMINAL_STATUSES, @@ -1555,6 +1556,98 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } + /** + * When a session's PR is merged, tear down its tmux runtime, remove its + * worktree, and archive its metadata. Guarded by an idleness check so we + * don't kill an agent mid-task; deferred cases set `mergedPendingCleanupSince` + * in metadata and retry on subsequent polls until the agent idles or the + * grace window elapses. + */ + async function maybeAutoCleanupOnMerge(session: Session): Promise { + if (session.status !== SESSION_STATUS.MERGED) return; + + // config.lifecycle is typed optional to support hand-constructed + // configs in tests. When loaded from YAML via Zod, the schema's + // .default({}) always populates it. The destructure below handles + // both paths uniformly. + const { autoCleanupOnMerge = true, mergeCleanupIdleGraceMs: graceMs = 300_000 } = + config.lifecycle ?? {}; + if (!autoCleanupOnMerge) return; + + // Check for idleness: if the agent is still working, defer cleanup. + const nowIso = new Date().toISOString(); + const pendingSince = session.metadata["mergedPendingCleanupSince"] || nowIso; + const pendingSinceMs = Date.parse(pendingSince); + const graceElapsed = Number.isFinite(pendingSinceMs) + ? Date.now() - pendingSinceMs >= graceMs + : false; + + const activity = session.activity; + const agentIsBusy = + activity === ACTIVITY_STATE.ACTIVE || + activity === ACTIVITY_STATE.WAITING_INPUT || + activity === ACTIVITY_STATE.BLOCKED; + + if (agentIsBusy && !graceElapsed) { + if (!session.metadata["mergedPendingCleanupSince"]) { + updateSessionMetadata(session, { mergedPendingCleanupSince: nowIso }); + } + observer.recordOperation({ + metric: "lifecycle_poll", + operation: "lifecycle.merge_cleanup.deferred", + outcome: "success", + correlationId: createCorrelationId("lifecycle-merge-cleanup"), + projectId: session.projectId, + sessionId: session.id, + reason: primaryLifecycleReason(session.lifecycle), + data: { activity, pendingSince, graceMs }, + level: "info", + }); + return; + } + + const correlationId = createCorrelationId("lifecycle-merge-cleanup"); + try { + const result = await sessionManager.kill(session.id, { + purgeOpenCode: true, + reason: "pr_merged", + }); + observer.recordOperation({ + metric: "lifecycle_poll", + operation: "lifecycle.merge_cleanup.completed", + outcome: "success", + correlationId, + projectId: session.projectId, + sessionId: session.id, + reason: primaryLifecycleReason(session.lifecycle), + data: { + cleaned: result.cleaned, + alreadyTerminated: result.alreadyTerminated, + graceElapsed, + activity, + }, + level: "info", + }); + states.delete(session.id); + } catch (err) { + // Leave `merged` status in place so the next poll retries. Preserve the + // deferral marker so idempotent retries don't restart the grace clock. + if (!session.metadata["mergedPendingCleanupSince"]) { + updateSessionMetadata(session, { mergedPendingCleanupSince: nowIso }); + } + observer.recordOperation({ + metric: "lifecycle_poll", + operation: "lifecycle.merge_cleanup.failed", + outcome: "failure", + correlationId, + projectId: session.projectId, + sessionId: session.id, + reason: err instanceof Error ? err.message : String(err), + level: "warn", + }); + } + } + /** Poll a single session and handle state transitions. */ async function checkSession(session: Session): Promise { // Use tracked state if available; otherwise use the persisted metadata status @@ -1787,6 +1880,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // Report watcher: audit agent reports for issues (#140) await auditAndReactToReports(session); + + // PR-merge auto-cleanup: tear down runtime + worktree + archive metadata + // once the agent is idle (or grace window elapses). Runs last so reactions + // and notifications observe the live session before it is destroyed. + await maybeAutoCleanupOnMerge(session); } /** diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index 0ded119ff..43c021331 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -30,6 +30,9 @@ import { type CleanupResult, type ClaimPROptions, type ClaimPRResult, + type KillOptions, + type KillResult, + type LifecycleKillReason, type OrchestratorConfig, type ProjectConfig, type Runtime, @@ -1878,9 +1881,37 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM return null; } - async function kill(sessionId: SessionId, options?: { purgeOpenCode?: boolean }): Promise { - const { raw, sessionsDir, project, projectId } = requireSessionRecord(sessionId); + async function kill(sessionId: SessionId, options?: KillOptions): Promise { + const located = findSessionRecord(sessionId); + if (!located) { + // Session already archived or never existed. If it's in the archive, + // treat as a no-op so auto-cleanup retries don't throw. + for (const project of Object.values(config.projects)) { + if (!project) continue; + const sessionsDir = getProjectSessionsDir(project); + if (readArchivedMetadataRaw(sessionsDir, sessionId)) { + return { cleaned: false, alreadyTerminated: true }; + } + } + throw new SessionNotFoundError(sessionId); + } + const { raw, sessionsDir, project, projectId } = located; + // Idempotency: if lifecycle already says terminated, don't re-run destroys + // (which could double-purge opencode or race with concurrent archives). + const existingLifecycle = parseCanonicalLifecycle(raw); + if (existingLifecycle?.session.state === "terminated") { + // Lifecycle says terminated but metadata is still in active dir — finish + // the archive and return alreadyTerminated so the caller logs a no-op. + try { + deleteMetadata(sessionsDir, sessionId, true); + } catch { + // Already archived by a racing caller. + } + return { cleaned: false, alreadyTerminated: true }; + } + + const killReason: LifecycleKillReason = options?.reason ?? "manually_killed"; const cleanupAgent = resolveSelectionForSession(project, sessionId, raw).agentName; // Destroy runtime — prefer handle.runtimeName to find the correct plugin @@ -1935,13 +1966,19 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM } } + const runtimeReason = + killReason === "pr_merged" + ? "pr_merged_cleanup" + : killReason === "auto_cleanup" + ? "auto_cleanup" + : "manual_kill_requested"; const terminatedLifecycle = buildUpdatedLifecycle(sessionId, raw, (next) => { next.session.state = "terminated"; - next.session.reason = "manually_killed"; + next.session.reason = killReason; next.session.terminatedAt = new Date().toISOString(); next.session.lastTransitionAt = next.session.terminatedAt; next.runtime.state = raw["runtimeHandle"] || raw["tmuxName"] ? "missing" : "exited"; - next.runtime.reason = "manual_kill_requested"; + next.runtime.reason = runtimeReason; next.runtime.lastObservedAt = new Date().toISOString(); }); updateMetadata(sessionsDir, sessionId, { @@ -1954,6 +1991,7 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM markArchivedOpenCodeCleanup(sessionsDir, sessionId); } invalidateCache(); + return { cleaned: true, alreadyTerminated: false }; } async function cleanup( diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 0e109fa3f..54de8c894 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -49,6 +49,8 @@ export type CanonicalSessionReason = | "research_complete" | "merged_waiting_decision" | "manually_killed" + | "pr_merged" + | "auto_cleanup" | "runtime_lost" | "agent_process_exited" | "probe_failure" @@ -75,6 +77,8 @@ export type CanonicalRuntimeReason = | "process_missing" | "tmux_missing" | "manual_kill_requested" + | "pr_merged_cleanup" + | "auto_cleanup" | "probe_error"; export interface SessionStateRecord { @@ -1156,6 +1160,22 @@ export interface PowerConfig { preventIdleSleep: boolean; } +/** Lifecycle-level orchestration configuration. */ +export interface LifecycleConfig { + /** + * When a session's PR is detected as merged, automatically tear down the + * tmux runtime, remove the worktree, and archive the session metadata. + * Defaults to true so `ao status` does not retain stale merged entries. + */ + autoCleanupOnMerge: boolean; + /** + * Maximum time (ms) to wait after a session enters `merged` before forcing + * cleanup regardless of agent activity. If the agent becomes idle sooner, + * cleanup happens then. Defaults to 5 minutes. + */ + mergeCleanupIdleGraceMs: number; +} + /** Top-level orchestrator configuration (from agent-orchestrator.yaml) */ export interface OrchestratorConfig { /** @@ -1180,6 +1200,14 @@ export interface OrchestratorConfig { /** Power management settings (idle sleep prevention, etc.). Populated with defaults post-validation. */ power?: PowerConfig; + /** + * Lifecycle-level orchestration settings. Populated with defaults by Zod + * when loaded from YAML, but typed as optional so hand-constructed test + * configs remain valid. Consumers should destructure with defaults rather + * than dereferencing directly. Mirrors the `power?` pattern above. + */ + lifecycle?: LifecycleConfig; + /** Default plugin selections */ defaults: DefaultPlugins; @@ -1561,6 +1589,27 @@ export interface SessionMetadata { // SERVICE INTERFACES (core, not pluggable) // ============================================================================= +/** + * Why a session was killed. Recorded as the lifecycle reason so observability + * can distinguish human action from automated teardown (e.g. PR merge cleanup). + */ +export type LifecycleKillReason = "manually_killed" | "pr_merged" | "auto_cleanup"; + +/** + * Outcome of a kill() call. `cleaned` means resources were torn down this + * invocation; `alreadyTerminated` means the session was already archived and + * kill() was a no-op. Callers can use this to avoid double-notifying. + */ +export interface KillResult { + cleaned: boolean; + alreadyTerminated: boolean; +} + +export interface KillOptions { + purgeOpenCode?: boolean; + reason?: LifecycleKillReason; +} + /** Session manager — CRUD for sessions */ export interface SessionManager { spawn(config: SessionSpawnConfig): Promise; @@ -1576,7 +1625,7 @@ export interface SessionManager { */ invalidateCache(): void; get(sessionId: SessionId): Promise; - kill(sessionId: SessionId, options?: { purgeOpenCode?: boolean }): Promise; + kill(sessionId: SessionId, options?: KillOptions): Promise; cleanup( projectId?: string, options?: { dryRun?: boolean; purgeOpenCode?: boolean }, diff --git a/packages/web/src/lib/__tests__/types.test.ts b/packages/web/src/lib/__tests__/types.test.ts index a7f5dca55..ceee22169 100644 --- a/packages/web/src/lib/__tests__/types.test.ts +++ b/packages/web/src/lib/__tests__/types.test.ts @@ -104,7 +104,7 @@ describe("getAttentionLevel", () => { evidence: null, detectingAttempts: 0, detectingEscalatedAt: null, - summary: "PR merged; worker is still available for a keep-or-kill decision", + summary: "PR merged; worker session will be cleaned up automatically", guidance: null, }, pr: { @@ -291,7 +291,7 @@ describe("getAttentionLevel", () => { evidence: null, detectingAttempts: 0, detectingEscalatedAt: null, - summary: "PR merged; worker is still available for a keep-or-kill decision", + summary: "PR merged; worker session will be cleaned up automatically", guidance: null, }, }); diff --git a/packages/web/src/lib/serialize.ts b/packages/web/src/lib/serialize.ts index 9b316ed29..4a4b53719 100644 --- a/packages/web/src/lib/serialize.ts +++ b/packages/web/src/lib/serialize.ts @@ -74,7 +74,9 @@ function buildLifecycleSummary(session: Session): string { return `Detecting runtime truth (${humanizeLifecycleToken(lifecycle.session.reason)})`; } if (lifecycle.pr.state === "merged") { - return "PR merged; worker is still available for a keep-or-kill decision"; + return session.metadata["mergedPendingCleanupSince"] + ? "PR merged; worker session will be cleaned up automatically" + : "PR merged"; } if (lifecycle.pr.state === "closed") { return "PR closed without merge";