diff --git a/.changeset/reap-terminated-session-worktrees.md b/.changeset/reap-terminated-session-worktrees.md new file mode 100644 index 0000000000..b38d5ce678 --- /dev/null +++ b/.changeset/reap-terminated-session-worktrees.md @@ -0,0 +1,5 @@ +--- +"@aoagents/ao-core": patch +--- + +Reap managed worktrees left behind by sessions whose lifecycle was already marked terminated before cleanup ran. diff --git a/packages/core/src/__tests__/session-manager/lifecycle.test.ts b/packages/core/src/__tests__/session-manager/lifecycle.test.ts index 37b469930f..9c7798b377 100644 --- a/packages/core/src/__tests__/session-manager/lifecycle.test.ts +++ b/packages/core/src/__tests__/session-manager/lifecycle.test.ts @@ -3,6 +3,7 @@ import { mkdirSync, readFileSync, existsSync, + rmSync, } from "node:fs"; import { join } from "node:path"; import { homedir } from "node:os"; @@ -137,6 +138,31 @@ describe("kill", () => { await expect(sm.kill("nonexistent")).rejects.toThrow("not found"); }); + it("reaps a managed workspace for an already terminated session", async () => { + const managedWorktree = join(getProjectWorktreesDir("my-app"), "app-terminated"); + mkdirSync(managedWorktree, { recursive: true }); + + writeMetadata(sessionsDir, "app-terminated", { + worktree: managedWorktree, + branch: "main", + status: "killed", + project: "my-app", + runtimeHandle: makeHandle("rt-terminated"), + }); + updateMetadata(sessionsDir, "app-terminated", { + lifecycle: JSON.stringify({ + session: { state: "terminated", terminatedAt: new Date().toISOString() }, + }), + }); + + const sm = createSessionManager({ config, registry: mockRegistry }); + const result = await sm.kill("app-terminated"); + + expect(result).toEqual({ cleaned: true, alreadyTerminated: true }); + expect(mockRuntime.destroy).not.toHaveBeenCalled(); + expect(mockWorkspace.destroy).toHaveBeenCalledWith(managedWorktree); + }); + it("tolerates runtime destroy failure", async () => { const failRuntime: Runtime = { ...mockRuntime, @@ -397,6 +423,63 @@ describe("cleanup", () => { expect(deleteLog).toContain("session delete ses_archived"); }, 15_000); + it("reaps worktrees left behind by already terminated sessions", async () => { + const managedWorktree = join(getProjectWorktreesDir("my-app"), "app-terminated-cleanup"); + mkdirSync(managedWorktree, { recursive: true }); + + writeMetadata(sessionsDir, "app-terminated-cleanup", { + worktree: managedWorktree, + branch: "main", + status: "killed", + project: "my-app", + runtimeHandle: makeHandle("rt-terminated-cleanup"), + }); + updateMetadata(sessionsDir, "app-terminated-cleanup", { + lifecycle: JSON.stringify({ + session: { state: "terminated", terminatedAt: new Date().toISOString() }, + }), + }); + + const sm = createSessionManager({ config, registry: mockRegistry }); + const result = await sm.cleanup(); + + expect(result.killed).toContain("app-terminated-cleanup"); + expect(result.skipped).not.toContain("app-terminated-cleanup"); + expect(mockRuntime.destroy).not.toHaveBeenCalled(); + expect(mockWorkspace.destroy).toHaveBeenCalledWith(managedWorktree); + }); + + it("skips already terminated sessions after orphan worktree cleanup is idempotent", async () => { + const managedWorktree = join(getProjectWorktreesDir("my-app"), "app-terminated-idempotent"); + mkdirSync(managedWorktree, { recursive: true }); + vi.mocked(mockWorkspace.destroy).mockImplementation(async (workspacePath: string) => { + rmSync(workspacePath, { recursive: true, force: true }); + }); + + writeMetadata(sessionsDir, "app-terminated-idempotent", { + worktree: managedWorktree, + branch: "main", + status: "killed", + project: "my-app", + runtimeHandle: makeHandle("rt-terminated-idempotent"), + }); + updateMetadata(sessionsDir, "app-terminated-idempotent", { + lifecycle: JSON.stringify({ + session: { state: "terminated", terminatedAt: new Date().toISOString() }, + }), + }); + + const sm = createSessionManager({ config, registry: mockRegistry }); + const first = await sm.cleanup(); + const second = await sm.cleanup(); + + expect(first.killed).toContain("app-terminated-idempotent"); + expect(second.killed).not.toContain("app-terminated-idempotent"); + expect(second.skipped).toContain("app-terminated-idempotent"); + expect(mockWorkspace.destroy).toHaveBeenCalledTimes(1); + expect(existsSync(managedWorktree)).toBe(false); + }); + it("does not skip terminated cleanup for matching session IDs in other projects", async () => { const deleteLogPath = join(tmpDir, "opencode-delete-archived-cross-project.log"); const mockBin = installMockOpencode(tmpDir, "[]", deleteLogPath); diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index 6eb6435944..ff2050fd77 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -101,6 +101,7 @@ import { setupPathWrapperWorkspace, PREFERRED_GH_PATH, } from "./agent-workspace-hooks.js"; +import { isWindows } from "./platform.js"; const execFileAsync = promisify(execFile); const OPENCODE_DISCOVERY_TIMEOUT_MS = 10_000; @@ -109,7 +110,7 @@ const INDEXED_PR_METADATA_KEY_REGEX = /^(prEnrichment|prReviewComments)_\d+$/; // On Windows, execFile cannot resolve .cmd shim extensions without invoking the shell. // windowsHide:true suppresses the conhost popup that the shell would otherwise flash. const EXEC_SHELL_OPTION = - process.platform === "win32" ? ({ shell: true, windowsHide: true } as const) : ({} as const); + isWindows() ? ({ shell: true, windowsHide: true } as const) : ({} as const); function errorIncludesSessionNotFound(err: unknown): boolean { @@ -431,6 +432,63 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM return roots.some((root) => isPathInside(workspacePath, root)); } + function hasReapableManagedWorkspace( + project: ProjectConfig | undefined, + projectId: string | undefined, + workspacePath: string | undefined, + ): boolean { + return Boolean( + workspacePath && + shouldDestroyWorkspacePath(project, projectId, workspacePath) && + existsSync(workspacePath), + ); + } + + async function destroyManagedWorkspace( + project: ProjectConfig | undefined, + projectId: string | undefined, + workspacePath: string | undefined, + options: { requireExisting?: boolean; sessionId?: SessionId } = {}, + ): Promise { + if (!workspacePath || !shouldDestroyWorkspacePath(project, projectId, workspacePath)) { + return false; + } + + if (options.requireExisting && !existsSync(workspacePath)) { + return false; + } + + const workspacePlugin = project + ? resolvePlugins(project).workspace + : registry.get("workspace", config.defaults.workspace); + if (!workspacePlugin) { + return false; + } + + try { + await workspacePlugin.destroy(workspacePath); + return true; + } catch (err) { + // Workspace might already be gone — emit AE when we know the session so + // abandoned worktrees surface for cleanup tooling. + if (projectId && options.sessionId) { + recordActivityEvent({ + projectId, + sessionId: options.sessionId, + source: "session-manager", + kind: "workspace.destroy_failed", + level: "warn", + summary: `workspace.destroy failed during kill: ${options.sessionId}`, + data: { + workspace: workspacePlugin.name, + reason: err instanceof Error ? err.message : String(err), + }, + }); + } + return false; + } + } + function isOrchestratorSessionRecord( sessionId: string, raw: Record | null | undefined, @@ -2516,11 +2574,22 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM } 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 kills). + // Idempotency: terminal lifecycle means runtime/agent teardown has already + // been recorded, but it does not prove the workspace was reaped. A crashed + // kill or runtime_lost transition can leave the worktree on disk, so allow + // a safe workspace-only cleanup retry before returning. const existingLifecycle = parseCanonicalLifecycle(raw); if (existingLifecycle?.session.state === "terminated") { - return { cleaned: false, alreadyTerminated: true }; + const cleanedWorkspace = await destroyManagedWorkspace( + project, + projectId, + raw["worktree"], + { requireExisting: true, sessionId }, + ); + if (cleanedWorkspace) { + invalidateCache(); + } + return { cleaned: cleanedWorkspace, alreadyTerminated: true }; } const killReason: LifecycleKillReason = options?.reason ?? "manually_killed"; @@ -2568,32 +2637,7 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM } } - const worktree = raw["worktree"]; - if (worktree && shouldDestroyWorkspacePath(project, projectId, worktree)) { - const workspacePlugin = project - ? resolvePlugins(project).workspace - : registry.get("workspace", config.defaults.workspace); - if (workspacePlugin) { - try { - await workspacePlugin.destroy(worktree); - } catch (err) { - // Workspace might already be gone — emit AE so abandoned worktrees - // surface for cleanup tooling. - recordActivityEvent({ - projectId, - sessionId, - source: "session-manager", - kind: "workspace.destroy_failed", - level: "warn", - summary: `workspace.destroy failed during kill: ${sessionId}`, - data: { - workspace: workspacePlugin.name, - reason: err instanceof Error ? err.message : String(err), - }, - }); - } - } - } + await destroyManagedWorkspace(project, projectId, raw["worktree"], { sessionId }); let didPurgeOpenCodeSession = false; if (options?.purgeOpenCode === true && cleanupAgent === "opencode") { @@ -2798,10 +2842,22 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM continue; } + const cleanedTerminatedWorkspace = options?.dryRun + ? hasReapableManagedWorkspace( + project, + projectKey, + terminatedRaw["worktree"], + ) + : (await kill(terminatedId, { purgeOpenCode: false })).cleaned; + const cleanupAgent = resolveSelectionForSession(project, terminatedId, terminatedRaw).agentName; const mappedOpenCodeSessionId = asValidOpenCodeSessionId(terminatedRaw["opencodeSessionId"]); if (cleanupAgent === "opencode" && terminatedRaw["opencodeCleanedAt"]) { - pushSkipped(projectKey, terminatedId); + if (cleanedTerminatedWorkspace) { + pushKilled(projectKey, terminatedId); + } else { + pushSkipped(projectKey, terminatedId); + } continue; } if (cleanupAgent === "opencode" && mappedOpenCodeSessionId && shouldPurgeOpenCode) { @@ -2835,6 +2891,8 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM } } pushKilled(projectKey, terminatedId); + } else if (cleanedTerminatedWorkspace) { + pushKilled(projectKey, terminatedId); } else { pushSkipped(projectKey, terminatedId); } diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 86850228e6..5f162451ea 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1857,8 +1857,9 @@ export type LifecycleKillReason = "manually_killed" | "pr_merged" | "auto_cleanu /** * 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. + * invocation; `alreadyTerminated` means terminal lifecycle metadata was already + * recorded before this call. Both may be true when kill() reaps a workspace + * left behind by a previously terminated session. */ export interface KillResult { cleaned: boolean;