From 71253105cd767e2020f413ff5bab17de44e07aab Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari <24b4506@iitb.ac.in> Date: Sun, 3 May 2026 22:43:21 +0530 Subject: [PATCH] refactor(core): replace spawn rollback ladder with CleanupStack (#1616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(core): replace spawn rollback ladder with CleanupStack Replace the four nested try/catch + cleanupSpawnWorkspaceAndMetadata helper in _spawnInner with a single LIFO CleanupStack. Each side effect (reserved metadata, workspace, prompt files, runtime handle) pushes its undo as soon as the resource exists; on success we dismiss(), on failure we runAll(). Why: adding a new spawn step previously required extending every prior cleanup block. Easy to forget; no compiler check. The stack makes rollback structural — a new step pushes one cleanup, no risk of leaving prior resources behind. runAll() is fault-tolerant by design: a throwing cleanup never short-circuits the rest. Behavior is preserved. Adds characterization tests for the worker spawn rollback paths (none existed before — only spawnOrchestrator was covered): - workspace.create failure cleans reserved metadata - runtime.create failure destroys worktree + cleans metadata - postLaunchSetup failure destroys runtime + worktree + metadata - one cleanup throwing does not skip subsequent cleanups Refs #1603 (PR 1 of the ao spawn refactor plan). Co-Authored-By: Claude Opus 4.7 * refactor(core): make CleanupStack runAll() terminal, symmetric with dismiss() Address review feedback on PR #1616: previously `dismiss()` → `push()` was a documented no-op but `runAll()` → `push()` would silently queue cleanups that fired on a subsequent `runAll()`. Asymmetric and surprising. Set `this.dismissed = true` at the top of `runAll()` so both terminal states (success via dismiss, failure via runAll) reject further pushes identically. Add a regression test pinning the new symmetric behavior. The "idempotent runAll" test continues to pass (early-return path now fires via the dismissed flag instead of the empty-stack short-circuit). Co-Authored-By: Claude Opus 4.7 * refactor(core): surface CleanupStack errors and cover postCreate rollback Address review feedback on PR #1616: - Pass an onError callback to cleanupStack.runAll() in _spawnInner that logs cleanup failures via console.error. The previous /* best effort */ pattern silently swallowed errors during rollback; now the same errors are surfaced for debugging without changing behavior (cleanup errors still don't propagate, subsequent cleanups still run). - Add a characterization test for the workspace.postCreate failure path. This was the only rollback path without a test — the stack handled it correctly already, but pinning it down prevents regression. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Claude Opus 4.7 --- .../core/src/__tests__/cleanup-stack.test.ts | 124 +++++ .../__tests__/session-manager/spawn.test.ts | 129 ++++++ packages/core/src/cleanup-stack.ts | 56 +++ packages/core/src/session-manager.ts | 432 ++++++++---------- 4 files changed, 498 insertions(+), 243 deletions(-) create mode 100644 packages/core/src/__tests__/cleanup-stack.test.ts create mode 100644 packages/core/src/cleanup-stack.ts diff --git a/packages/core/src/__tests__/cleanup-stack.test.ts b/packages/core/src/__tests__/cleanup-stack.test.ts new file mode 100644 index 000000000..a0f6b9e16 --- /dev/null +++ b/packages/core/src/__tests__/cleanup-stack.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect, vi } from "vitest"; +import { CleanupStack } from "../cleanup-stack.js"; + +describe("CleanupStack", () => { + it("runs nothing on an empty stack", async () => { + const stack = new CleanupStack(); + await expect(stack.runAll()).resolves.toBeUndefined(); + }); + + it("runs pushed cleanups in LIFO order", async () => { + const calls: string[] = []; + const stack = new CleanupStack(); + stack.push(() => { + calls.push("a"); + }); + stack.push(() => { + calls.push("b"); + }); + stack.push(() => { + calls.push("c"); + }); + + await stack.runAll(); + + expect(calls).toEqual(["c", "b", "a"]); + }); + + it("does not run any cleanups after dismiss()", async () => { + const fn = vi.fn(); + const stack = new CleanupStack(); + stack.push(fn); + + stack.dismiss(); + await stack.runAll(); + + expect(fn).not.toHaveBeenCalled(); + }); + + it("awaits async cleanups", async () => { + const calls: string[] = []; + const stack = new CleanupStack(); + stack.push(async () => { + await new Promise((resolve) => setTimeout(resolve, 5)); + calls.push("first-pushed"); + }); + stack.push(() => { + calls.push("second-pushed"); + }); + + await stack.runAll(); + + // LIFO: second-pushed runs first (sync), then async first-pushed completes. + expect(calls).toEqual(["second-pushed", "first-pushed"]); + }); + + it("continues running subsequent cleanups when one throws", async () => { + const calls: string[] = []; + const stack = new CleanupStack(); + stack.push(() => { + calls.push("a"); + }); + stack.push(() => { + throw new Error("boom"); + }); + stack.push(() => { + calls.push("c"); + }); + + await stack.runAll(); + + // c runs first (LIFO), middle throws but is swallowed, a still runs. + expect(calls).toEqual(["c", "a"]); + }); + + it("forwards thrown errors to the onError callback when provided", async () => { + const errors: unknown[] = []; + const stack = new CleanupStack(); + stack.push(() => { + throw new Error("first"); + }); + stack.push(async () => { + throw new Error("second"); + }); + + await stack.runAll((err) => errors.push(err)); + + expect(errors).toHaveLength(2); + expect((errors[0] as Error).message).toBe("second"); // LIFO + expect((errors[1] as Error).message).toBe("first"); + }); + + it("is a no-op when runAll is called twice", async () => { + const fn = vi.fn(); + const stack = new CleanupStack(); + stack.push(fn); + + await stack.runAll(); + await stack.runAll(); + + expect(fn).toHaveBeenCalledTimes(1); + }); + + it("treats push after dismiss as a no-op (cleanup will not run)", async () => { + const fn = vi.fn(); + const stack = new CleanupStack(); + stack.dismiss(); + stack.push(fn); + + await stack.runAll(); + + expect(fn).not.toHaveBeenCalled(); + }); + + it("treats push after runAll as a no-op (symmetric with dismiss)", async () => { + const fn = vi.fn(); + const stack = new CleanupStack(); + await stack.runAll(); + stack.push(fn); + + await stack.runAll(); + + expect(fn).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/__tests__/session-manager/spawn.test.ts b/packages/core/src/__tests__/session-manager/spawn.test.ts index c536b130b..e9915c546 100644 --- a/packages/core/src/__tests__/session-manager/spawn.test.ts +++ b/packages/core/src/__tests__/session-manager/spawn.test.ts @@ -13,6 +13,7 @@ import { readMetadata, readMetadataRaw, } from "../../metadata.js"; +import { getProjectWorktreesDir } from "../../paths.js"; import type { OrchestratorConfig, PluginRegistry, @@ -1287,6 +1288,134 @@ describe("spawn", () => { vi.useRealTimers(); }, 20_000); + describe("rollback on failure", () => { + it("cleans up reserved metadata when workspace creation fails", async () => { + (mockWorkspace.create as ReturnType).mockRejectedValueOnce( + new Error("workspace creation failed"), + ); + const sm = createSessionManager({ config, registry: mockRegistry }); + + await expect(sm.spawn({ projectId: "my-app" })).rejects.toThrow( + "workspace creation failed", + ); + + expect(readMetadataRaw(sessionsDir, "app-1")).toBeNull(); + expect(mockRuntime.create).not.toHaveBeenCalled(); + }); + + it("destroys the worktree and cleans metadata when runtime creation fails", async () => { + // Workspace path must be inside the project's managed worktrees root so + // shouldDestroyWorkspacePath() permits destroy. Mock paths under tmpDir + // would be skipped as out-of-tree (correct, but not the path we want to + // characterize here). + const worktreePath = join(getProjectWorktreesDir("my-app"), "app-1"); + (mockWorkspace.create as ReturnType).mockResolvedValueOnce({ + path: worktreePath, + branch: "session/app-1", + sessionId: "app-1", + projectId: "my-app", + }); + (mockRuntime.create as ReturnType).mockRejectedValueOnce( + new Error("runtime creation failed"), + ); + const sm = createSessionManager({ config, registry: mockRegistry }); + + await expect(sm.spawn({ projectId: "my-app" })).rejects.toThrow( + "runtime creation failed", + ); + + expect(mockWorkspace.destroy).toHaveBeenCalledWith(worktreePath); + expect(readMetadataRaw(sessionsDir, "app-1")).toBeNull(); + }); + + it("destroys runtime and worktree when post-launch setup fails", async () => { + const worktreePath = join(getProjectWorktreesDir("my-app"), "app-1"); + (mockWorkspace.create as ReturnType).mockResolvedValueOnce({ + path: worktreePath, + branch: "session/app-1", + sessionId: "app-1", + projectId: "my-app", + }); + const postLaunchError = new Error("post-launch setup failed"); + const agentWithPostLaunch: typeof mockAgent = { + ...mockAgent, + postLaunchSetup: vi.fn().mockRejectedValueOnce(postLaunchError), + }; + const registryWithPostLaunch: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return agentWithPostLaunch; + if (slot === "workspace") return mockWorkspace; + return null; + }), + }; + const sm = createSessionManager({ config, registry: registryWithPostLaunch }); + + await expect(sm.spawn({ projectId: "my-app" })).rejects.toThrow("post-launch setup failed"); + + expect(mockRuntime.destroy).toHaveBeenCalled(); + expect(mockWorkspace.destroy).toHaveBeenCalledWith(worktreePath); + expect(readMetadataRaw(sessionsDir, "app-1")).toBeNull(); + }); + + it("destroys the worktree and cleans metadata when workspace.postCreate fails", async () => { + const worktreePath = join(getProjectWorktreesDir("my-app"), "app-1"); + const workspaceWithPostCreate: typeof mockWorkspace = { + ...mockWorkspace, + create: vi.fn().mockResolvedValueOnce({ + path: worktreePath, + branch: "session/app-1", + sessionId: "app-1", + projectId: "my-app", + }), + postCreate: vi.fn().mockRejectedValueOnce(new Error("postCreate hook failed")), + }; + const registryWithPostCreate: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return mockAgent; + if (slot === "workspace") return workspaceWithPostCreate; + return null; + }), + }; + const sm = createSessionManager({ config, registry: registryWithPostCreate }); + + await expect(sm.spawn({ projectId: "my-app" })).rejects.toThrow("postCreate hook failed"); + + expect(workspaceWithPostCreate.destroy).toHaveBeenCalledWith(worktreePath); + expect(readMetadataRaw(sessionsDir, "app-1")).toBeNull(); + // Runtime should not have been created since postCreate failed before runtime.create. + expect(mockRuntime.create).not.toHaveBeenCalled(); + }); + + it("still cleans subsequent resources when one cleanup step throws", async () => { + const worktreePath = join(getProjectWorktreesDir("my-app"), "app-1"); + (mockWorkspace.create as ReturnType).mockResolvedValueOnce({ + path: worktreePath, + branch: "session/app-1", + sessionId: "app-1", + projectId: "my-app", + }); + // workspace.destroy throws during rollback — metadata cleanup must still run + (mockWorkspace.destroy as ReturnType).mockRejectedValueOnce( + new Error("destroy failed"), + ); + (mockRuntime.create as ReturnType).mockRejectedValueOnce( + new Error("runtime creation failed"), + ); + const sm = createSessionManager({ config, registry: mockRegistry }); + + await expect(sm.spawn({ projectId: "my-app" })).rejects.toThrow( + "runtime creation failed", + ); + + // Even though workspace.destroy threw, metadata must have been cleaned up. + expect(readMetadataRaw(sessionsDir, "app-1")).toBeNull(); + }); + }); + describe("displayName derivation", () => { it("persists the issue title as displayName when tracker returns one", async () => { const mockTracker: Tracker = { diff --git a/packages/core/src/cleanup-stack.ts b/packages/core/src/cleanup-stack.ts new file mode 100644 index 000000000..45330bbed --- /dev/null +++ b/packages/core/src/cleanup-stack.ts @@ -0,0 +1,56 @@ +/** + * LIFO stack of cleanup callbacks for unwinding partial side effects. + * + * Use inside long initialization sequences (spawn, setup) where each successful + * step adds a side effect that must be undone if a *later* step fails. Push a + * cleanup as soon as the resource exists; call `dismiss()` once the whole + * sequence has succeeded; call `runAll()` from the catch block to unwind. + * + * `runAll()` is fault-tolerant by design: a cleanup throwing must not skip the + * remaining cleanups, otherwise the abstraction is worse than the inline ladder + * it replaces. Pass `onError` to observe errors; the default is to swallow them + * (matching the existing best-effort pattern in session-manager). + */ +export type CleanupFn = () => void | Promise; + +export class CleanupStack { + private fns: CleanupFn[] = []; + private dismissed = false; + + /** + * Register a cleanup. Cleanups added after `dismiss()` or `runAll()` will + * not run — both are terminal states for the stack. + */ + push(fn: CleanupFn): void { + if (this.dismissed) return; + this.fns.push(fn); + } + + /** + * Mark the operation as successful. Subsequent `runAll()` calls do nothing + * and subsequent `push()` calls are ignored. + */ + dismiss(): void { + this.dismissed = true; + } + + /** + * Run all pushed cleanups in LIFO order. Each cleanup is awaited; throws are + * forwarded to `onError` (default: swallowed) so one failing cleanup never + * skips the remaining ones. After running, the stack is terminal: subsequent + * `push()` calls are no-ops and subsequent `runAll()` calls do nothing — + * symmetric with `dismiss()`. + */ + async runAll(onError?: (err: unknown) => void): Promise { + if (this.dismissed) return; + this.dismissed = true; + while (this.fns.length > 0) { + const fn = this.fns.pop()!; + try { + await fn(); + } catch (err) { + if (onError) onError(err); + } + } + } +} diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index 5ecbd8bb4..5da9981cd 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -86,6 +86,7 @@ import { writeWorkspaceOpenCodeAgentsMd, } from "./opencode-agents-md.js"; import { writeOpenCodeConfig } from "./opencode-config.js"; +import { CleanupStack } from "./cleanup-stack.js"; import { getOrchestratorSessionId, normalizeOrchestratorSessionStrategy, @@ -1160,40 +1161,47 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM // Get the sessions directory for this project const sessionsDir = getProjectSessionsDir(spawnConfig.projectId); - // Determine session ID — atomically reserve to prevent concurrent collisions - const { sessionId, tmuxName } = await reserveNextSessionIdentity(project, sessionsDir); + // CleanupStack: each side effect pushes its undo as soon as the resource + // exists. On any failure below we runAll() in LIFO order; on success we + // dismiss(). Replaces the previous nested rollback ladder — adding a new + // step now requires pushing one cleanup, with no risk of forgetting prior + // ones. + const cleanupStack = new CleanupStack(); + try { + // Determine session ID — atomically reserve to prevent concurrent collisions + const { sessionId, tmuxName } = await reserveNextSessionIdentity(project, sessionsDir); + cleanupStack.push(() => deleteMetadata(sessionsDir, sessionId)); - // Determine branch name — explicit branch always takes priority - let branch: string; - if (spawnConfig.branch) { - branch = spawnConfig.branch; - } else if (spawnConfig.issueId && plugins.tracker && resolvedIssue) { - const fromIssue = resolvedIssue.branchName; - branch = - fromIssue && isGitBranchNameSafe(fromIssue) - ? fromIssue - : plugins.tracker.branchName(spawnConfig.issueId, project); - } else if (spawnConfig.issueId) { - // If the issueId is already branch-safe (e.g. "INT-9999"), use as-is. - // Otherwise sanitize free-text (e.g. "fix login bug") into a valid slug. - const id = spawnConfig.issueId; - const isBranchSafe = /^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(id) && !id.includes(".."); - const slug = isBranchSafe - ? id - : id - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .slice(0, 60) - .replace(/^-+|-+$/g, ""); - branch = `feat/${slug || sessionId}`; - } else { - branch = `session/${sessionId}`; - } + // Determine branch name — explicit branch always takes priority + let branch: string; + if (spawnConfig.branch) { + branch = spawnConfig.branch; + } else if (spawnConfig.issueId && plugins.tracker && resolvedIssue) { + const fromIssue = resolvedIssue.branchName; + branch = + fromIssue && isGitBranchNameSafe(fromIssue) + ? fromIssue + : plugins.tracker.branchName(spawnConfig.issueId, project); + } else if (spawnConfig.issueId) { + // If the issueId is already branch-safe (e.g. "INT-9999"), use as-is. + // Otherwise sanitize free-text (e.g. "fix login bug") into a valid slug. + const id = spawnConfig.issueId; + const isBranchSafe = /^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(id) && !id.includes(".."); + const slug = isBranchSafe + ? id + : id + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .slice(0, 60) + .replace(/^-+|-+$/g, ""); + branch = `feat/${slug || sessionId}`; + } else { + branch = `session/${sessionId}`; + } - // Create workspace (if workspace plugin is available) - let workspacePath = project.path; - if (plugins.workspace) { - try { + // Create workspace (if workspace plugin is available) + let workspacePath = project.path; + if (plugins.workspace) { const wsInfo = await plugins.workspace.create({ projectId: spawnConfig.projectId, project, @@ -1202,110 +1210,54 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM worktreeDir: getProjectWorktreesDir(spawnConfig.projectId), }); workspacePath = wsInfo.path; - - // Run post-create hooks — clean up workspace on failure + // Only register destroy when the path is inside a managed root — + // matches the prior shouldDestroyWorkspacePath gate so we never + // destroy a user-owned project directory. + if (shouldDestroyWorkspacePath(project, spawnConfig.projectId, workspacePath)) { + const ws = plugins.workspace; + cleanupStack.push(() => ws.destroy(workspacePath)); + } if (plugins.workspace.postCreate) { - try { - await plugins.workspace.postCreate(wsInfo, project); - } catch (err) { - if (shouldDestroyWorkspacePath(project, spawnConfig.projectId, workspacePath)) { - try { - await plugins.workspace.destroy(workspacePath); - } catch { - /* best effort */ - } - } - throw err; - } + await plugins.workspace.postCreate(wsInfo, project); } - } catch (err) { - // Clean up reserved session ID on workspace failure + } + + // Generate prompt with validated issue + let issueContext: string | undefined; + if (spawnConfig.issueId && plugins.tracker && resolvedIssue) { try { - deleteMetadata(sessionsDir, sessionId); + issueContext = await plugins.tracker.generatePrompt(spawnConfig.issueId, project); } catch { - /* best effort */ - } - throw err; - } - } - - // Generate prompt with validated issue - let issueContext: string | undefined; - if (spawnConfig.issueId && plugins.tracker && resolvedIssue) { - try { - issueContext = await plugins.tracker.generatePrompt(spawnConfig.issueId, project); - } catch { - // Non-fatal: continue without detailed issue context - // Silently ignore errors - caller can check if issueContext is undefined - } - } - - const cleanupSpawnWorkspaceAndMetadata = async ( - promptFile?: string, - opencodeConfigFile?: string, - ): Promise => { - if ( - plugins.workspace && - shouldDestroyWorkspacePath(project, spawnConfig.projectId, workspacePath) - ) { - try { - await plugins.workspace.destroy(workspacePath); - } catch { - /* best effort */ + // Non-fatal: continue without detailed issue context + // Silently ignore errors - caller can check if issueContext is undefined } } - try { - deleteMetadata(sessionsDir, sessionId); - } catch { - /* best effort */ - } - if (promptFile) { - try { - unlinkSync(promptFile); - } catch { - /* best effort */ - } - } - if (opencodeConfigFile) { - try { - unlinkSync(opencodeConfigFile); - } catch { - /* best effort */ - } - } - }; - const { systemPrompt, taskPrompt } = buildPrompt({ - project, - projectId: spawnConfig.projectId, - issueId: spawnConfig.issueId, - issueContext, - userPrompt: spawnConfig.prompt, - }); + const { systemPrompt, taskPrompt } = buildPrompt({ + project, + projectId: spawnConfig.projectId, + issueId: spawnConfig.issueId, + issueContext, + userPrompt: spawnConfig.prompt, + }); - let systemPromptFile: string | undefined; - - // need a seperate config file to pass instructions for opencode session - let opencodeConfigFile: string | undefined; - - try { const baseDir = getProjectDir(spawnConfig.projectId); mkdirSync(baseDir, { recursive: true }); - systemPromptFile = join(baseDir, `worker-prompt-${sessionId}.md`); + const systemPromptFile = join(baseDir, `worker-prompt-${sessionId}.md`); writeFileSync(systemPromptFile, systemPrompt, "utf-8"); + cleanupStack.push(() => unlinkSync(systemPromptFile)); + + // need a seperate config file to pass instructions for opencode session + let opencodeConfigFile: string | undefined; if (plugins.agent.name === "opencode") { opencodeConfigFile = writeOpenCodeConfig(baseDir, sessionId, [systemPromptFile]); + const cfg = opencodeConfigFile; + cleanupStack.push(() => unlinkSync(cfg)); } - } catch (err) { - await cleanupSpawnWorkspaceAndMetadata(systemPromptFile, opencodeConfigFile); - throw err; - } - // Get agent launch config and create runtime — clean up workspace on failure - const opencodeIssueSessionStrategy = project.opencodeIssueSessionStrategy ?? "reuse"; - let reusedOpenCodeSessionId: string | undefined; - try { - reusedOpenCodeSessionId = + // Get agent launch config and create runtime + const opencodeIssueSessionStrategy = project.opencodeIssueSessionStrategy ?? "reuse"; + const reusedOpenCodeSessionId = plugins.agent.name === "opencode" && spawnConfig.issueId ? await resolveOpenCodeSessionReuse({ sessionsDir, @@ -1313,30 +1265,25 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM strategy: opencodeIssueSessionStrategy, }) : undefined; - } catch (err) { - await cleanupSpawnWorkspaceAndMetadata(systemPromptFile, opencodeConfigFile); - throw err; - } - const agentLaunchConfig = { - sessionId, - projectConfig: { - ...project, - agentConfig: { - ...selection.agentConfig, - ...(reusedOpenCodeSessionId ? { opencodeSessionId: reusedOpenCodeSessionId } : {}), - }, - }, - workspacePath, - issueId: spawnConfig.issueId, - prompt: taskPrompt, - systemPromptFile, - permissions: selection.permissions, - model: selection.model, - subagent: spawnConfig.subagent ?? selection.subagent, - }; - let handle: RuntimeHandle; - try { + const agentLaunchConfig = { + sessionId, + projectConfig: { + ...project, + agentConfig: { + ...selection.agentConfig, + ...(reusedOpenCodeSessionId ? { opencodeSessionId: reusedOpenCodeSessionId } : {}), + }, + }, + workspacePath, + issueId: spawnConfig.issueId, + prompt: taskPrompt, + systemPromptFile, + permissions: selection.permissions, + model: selection.model, + subagent: spawnConfig.subagent ?? selection.subagent, + }; + const launchCommand = plugins.agent.getLaunchCommand(agentLaunchConfig); const environment = plugins.agent.getEnvironment(agentLaunchConfig); @@ -1344,7 +1291,7 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM await plugins.agent.preLaunchSetup(workspacePath); } - handle = await plugins.runtime.create({ + const handle = await plugins.runtime.create({ sessionId: tmuxName ?? sessionId, // Use tmux name for runtime if available workspacePath, launchCommand, @@ -1367,54 +1314,50 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM config.port !== null && { AO_PORT: String(config.port) }), }, }); - } catch (err) { - // Clean up workspace, prompt file, and reserved ID if agent config or runtime creation failed - await cleanupSpawnWorkspaceAndMetadata(systemPromptFile, opencodeConfigFile); - throw err; - } + const rt = plugins.runtime; + cleanupStack.push(() => rt.destroy(handle)); - // Derive a stable display name from task context. Unlike issue-title - // enrichment (which is a live tracker API call), this value is captured at - // spawn time and persisted, so the dashboard has a good name even when the - // tracker is unavailable or the session has no attached PR yet. - const displayName = deriveDisplayName({ - issueTitle: resolvedIssue?.title, - prompt: spawnConfig.prompt, - }); + // Derive a stable display name from task context. Unlike issue-title + // enrichment (which is a live tracker API call), this value is captured at + // spawn time and persisted, so the dashboard has a good name even when the + // tracker is unavailable or the session has no attached PR yet. + const displayName = deriveDisplayName({ + issueTitle: resolvedIssue?.title, + prompt: spawnConfig.prompt, + }); - // Write metadata and run post-launch setup — clean up on failure - const createdAt = new Date(); - const lifecycle = createInitialCanonicalLifecycle("worker", createdAt); - lifecycle.runtime.handle = handle; - lifecycle.runtime.tmuxName = tmuxName ?? null; + // Write metadata and run post-launch setup + const createdAt = new Date(); + const lifecycle = createInitialCanonicalLifecycle("worker", createdAt); + lifecycle.runtime.handle = handle; + lifecycle.runtime.tmuxName = tmuxName ?? null; - const session: Session = { - id: sessionId, - projectId: spawnConfig.projectId, - status: deriveLegacyStatus(lifecycle), - activity: "active", - activitySignal: createActivitySignal("valid", { + const session: Session = { + id: sessionId, + projectId: spawnConfig.projectId, + status: deriveLegacyStatus(lifecycle), activity: "active", - timestamp: createdAt, - source: "runtime", - }), - lifecycle, - branch, - issueId: spawnConfig.issueId ?? null, - pr: null, - workspacePath, - runtimeHandle: handle, - agentInfo: null, - createdAt, - lastActivityAt: createdAt, - metadata: { - ...(reusedOpenCodeSessionId ? { opencodeSessionId: reusedOpenCodeSessionId } : {}), - ...(spawnConfig.prompt ? { userPrompt: spawnConfig.prompt } : {}), - ...(displayName ? { displayName } : {}), - }, - }; + activitySignal: createActivitySignal("valid", { + activity: "active", + timestamp: createdAt, + source: "runtime", + }), + lifecycle, + branch, + issueId: spawnConfig.issueId ?? null, + pr: null, + workspacePath, + runtimeHandle: handle, + agentInfo: null, + createdAt, + lastActivityAt: createdAt, + metadata: { + ...(reusedOpenCodeSessionId ? { opencodeSessionId: reusedOpenCodeSessionId } : {}), + ...(spawnConfig.prompt ? { userPrompt: spawnConfig.prompt } : {}), + ...(displayName ? { displayName } : {}), + }, + }; - try { writeMetadata(sessionsDir, sessionId, { worktree: workspacePath, branch, @@ -1463,71 +1406,74 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM updateMetadata(sessionsDir, sessionId, session.metadata); } invalidateCache(); - } catch (err) { - // Clean up runtime and workspace on post-launch failure - try { - await plugins.runtime.destroy(handle); - } catch { - /* best effort */ - } - await cleanupSpawnWorkspaceAndMetadata(systemPromptFile); - throw err; - } - // Send the task-specific prompt post-launch for agents that need it - // (e.g. Claude Code exits after -p, so we send the prompt after it starts - // in interactive mode). - // This is intentionally outside the try/catch above — a prompt delivery failure - // should NOT destroy the session. The agent is running; user can retry with `ao send`. - let promptDelivered = false; - if (plugins.agent.promptDelivery === "post-launch" && agentLaunchConfig.prompt) { - const maxRetries = 3; - const baseDelayMs = 3_000; - let lastError: Error | undefined; + // Past this point every resource that needed an undo is on disk in its + // final form. Dismiss the stack so the prompt-delivery loop below (which + // is intentionally non-fatal) cannot trigger a rollback. + cleanupStack.dismiss(); - for (let attempt = 1; attempt <= maxRetries; attempt++) { - try { - // Wait for agent to start and be ready for input - // Use exponential backoff: 3s, 6s, 9s between attempts - await new Promise((resolve) => setTimeout(resolve, baseDelayMs * attempt)); - await plugins.runtime.sendMessage(handle, agentLaunchConfig.prompt); - promptDelivered = true; - break; - } catch (err) { - lastError = err instanceof Error ? err : new Error(String(err)); + // Send the task-specific prompt post-launch for agents that need it + // (e.g. Claude Code exits after -p, so we send the prompt after it starts + // in interactive mode). Prompt delivery failure must NOT destroy the + // session — the agent is running; user can retry with `ao send`. + let promptDelivered = false; + if (plugins.agent.promptDelivery === "post-launch" && agentLaunchConfig.prompt) { + const maxRetries = 3; + const baseDelayMs = 3_000; + let lastError: Error | undefined; + + for (let attempt = 1; attempt <= maxRetries; attempt++) { + try { + // Wait for agent to start and be ready for input + // Use exponential backoff: 3s, 6s, 9s between attempts + await new Promise((resolve) => setTimeout(resolve, baseDelayMs * attempt)); + await plugins.runtime.sendMessage(handle, agentLaunchConfig.prompt); + promptDelivered = true; + break; + } catch (err) { + lastError = err instanceof Error ? err : new Error(String(err)); + console.error( + `[session-manager] Prompt delivery attempt ${attempt}/${maxRetries} failed: ${lastError.message}`, + ); + } + } + + if (!promptDelivered) { console.error( - `[session-manager] Prompt delivery attempt ${attempt}/${maxRetries} failed: ${lastError.message}`, + `[session-manager] FAILED to deliver prompt to session ${sessionId} after ${maxRetries} attempts. ` + + `User must send manually with 'ao send'. Last error: ${lastError?.message}`, ); } + + session.metadata["promptDelivered"] = String(promptDelivered); + } else if (agentLaunchConfig.prompt) { + session.metadata["promptDelivered"] = "true"; } - if (!promptDelivered) { - console.error( - `[session-manager] FAILED to deliver prompt to session ${sessionId} after ${maxRetries} attempts. ` + - `User must send manually with 'ao send'. Last error: ${lastError?.message}`, - ); + if (session.metadata["promptDelivered"]) { + updateMetadata(sessionsDir, sessionId, session.metadata); + invalidateCache(); } - session.metadata["promptDelivered"] = String(promptDelivered); - } else if (agentLaunchConfig.prompt) { - session.metadata["promptDelivered"] = "true"; + recordActivityEvent({ + projectId: spawnConfig.projectId, + sessionId, + source: "session-manager", + kind: "session.spawned", + summary: `spawned: ${sessionId}`, + data: { agent: plugins.agent.name, branch: session.branch ?? undefined }, + }); + + return session; + } catch (err) { + // Log cleanup failures so they don't disappear silently. The original + // code used /* best effort */ swallows; the stack preserves that + // behavior (cleanup errors don't propagate) but surfaces them for debug. + await cleanupStack.runAll((cleanupErr) => { + console.error("[session-manager] spawn rollback step failed:", cleanupErr); + }); + throw err; } - - if (session.metadata["promptDelivered"]) { - updateMetadata(sessionsDir, sessionId, session.metadata); - invalidateCache(); - } - - recordActivityEvent({ - projectId: spawnConfig.projectId, - sessionId, - source: "session-manager", - kind: "session.spawned", - summary: `spawned: ${sessionId}`, - data: { agent: plugins.agent.name, branch: session.branch ?? undefined }, - }); - - return session; } async function spawnOrchestrator(orchestratorConfig: OrchestratorSpawnConfig): Promise {