From 627b4aee98ce86e40bd1138292ab62069fc38f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 25 Apr 2026 11:31:42 +0100 Subject: [PATCH] fix: auto-resume failed sessions with silent workspace-restore fallback (#696) --- .../internal/orchestrator/task_operations.go | 18 +- .../task_operations_resume_test.go | 162 ++++++++++++++++++ .../session/use-session-resumption.test.ts | 158 +++++++++++++++++ .../domains/session/use-session-resumption.ts | 81 ++++++++- 4 files changed, 416 insertions(+), 3 deletions(-) create mode 100644 apps/web/hooks/domains/session/use-session-resumption.test.ts diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 2c4d20d4a4..e014f5f224 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -32,6 +32,11 @@ type PromptResult struct { // error-recovery state (WAITING_FOR_INPUT with a non-empty ErrorMessage). const resumeReasonErrorRecovery = "error_recovery" +// resumeReasonFailedSessionResumable is the resume reason returned when a +// FAILED session is auto-resumed because its runtime is Resumable. Distinct +// from "agent_not_running" so log filtering can isolate FAILED auto-resumes. +const resumeReasonFailedSessionResumable = "failed_session_resumable" + var ErrAgentPromptInProgress = errors.New("agent is currently processing a prompt") var ErrSessionResetInProgress = errors.New("session reset in progress") @@ -1037,7 +1042,18 @@ func (s *Service) GetTaskSessionStatus(ctx context.Context, taskID, sessionID st // 3. Session can be resumed if it has a resume token if resumeToken != "" { - // Don't auto-resume terminal sessions — they failed/completed for a reason. + // Auto-resume FAILED sessions when the runtime is resumable: PR #670 made + // terminal-state resume safe (cleanup + retry), so recover transparently + // before surfacing the error. Frontend falls back to restore_workspace if + // the resume itself fails. + if session.State == models.TaskSessionStateFailed && running != nil && running.Resumable { + out := s.validateResumeEligibility(session, resp) + if out.NeedsResume { + out.ResumeReason = resumeReasonFailedSessionResumable + } + return out, nil + } + // Don't auto-resume other terminal sessions (CANCELLED stays stopped, COMPLETED is done). if !isActiveSessionState(session.State) { resp.IsAgentRunning = false resp.IsResumable = false diff --git a/apps/backend/internal/orchestrator/task_operations_resume_test.go b/apps/backend/internal/orchestrator/task_operations_resume_test.go index f65aaa1f72..048d7fbedc 100644 --- a/apps/backend/internal/orchestrator/task_operations_resume_test.go +++ b/apps/backend/internal/orchestrator/task_operations_resume_test.go @@ -109,6 +109,168 @@ func TestGetTaskSessionStatus_AutoResumesNormalWaitingSession(t *testing.T) { } } +// TestGetTaskSessionStatus_AutoResumesFailedSessionWithResumeToken verifies the +// failed-but-recoverable path: a FAILED session that still has a resumable +// runtime + resume token reports NeedsResume=true so the frontend retries +// transparently instead of showing the historical error to the user. +func TestGetTaskSessionStatus_AutoResumesFailedSessionWithResumeToken(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateFailed) + + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("failed to load session: %v", err) + } + session.AgentProfileID = "profile1" + session.ErrorMessage = "execution already running" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("failed to update session: %v", err) + } + + now := time.Now().UTC() + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "er1", + SessionID: "session1", + TaskID: "task1", + Status: "ready", + Resumable: true, + ResumeToken: "acp-session-123", + CreatedAt: now, + UpdatedAt: now, + }); err != nil { + t.Fatalf("failed to upsert executor running: %v", err) + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateReview} + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + resp, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("GetTaskSessionStatus returned error: %v", err) + } + if !resp.NeedsResume { + t.Fatal("expected NeedsResume=true so frontend auto-resumes a failed session") + } + if !resp.IsResumable { + t.Fatal("expected IsResumable=true for failed session with resumable runtime") + } + if resp.NeedsWorkspaceRestore { + t.Fatal("expected NeedsWorkspaceRestore=false when auto-resuming") + } + if resp.ResumeReason != resumeReasonFailedSessionResumable { + t.Fatalf("expected ResumeReason=%q for FAILED auto-resume, got %q", + resumeReasonFailedSessionResumable, resp.ResumeReason) + } +} + +// TestGetTaskSessionStatus_FailedSessionWithoutResumableFlagFallsBack verifies +// that a FAILED session whose runtime is NOT resumable still goes through the +// workspace-restore path (no auto-resume attempt to avoid certain failure). +func TestGetTaskSessionStatus_FailedSessionWithoutResumableFlagFallsBack(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateFailed) + + // Seed a worktree so canRestoreWorkspace returns true. + now := time.Now().UTC() + if err := repo.CreateTaskSessionWorktree(ctx, &models.TaskSessionWorktree{ + ID: "wt1", + SessionID: "session1", + WorktreeID: "wid1", + RepositoryID: "repo1", + WorktreePath: "/tmp/worktrees/session1", + WorktreeBranch: "feature/test", + CreatedAt: now, + }); err != nil { + t.Fatalf("failed to create worktree: %v", err) + } + + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "er1", + SessionID: "session1", + TaskID: "task1", + Status: "ready", + Resumable: false, + ResumeToken: "acp-session-123", + CreatedAt: now, + UpdatedAt: now, + }); err != nil { + t.Fatalf("failed to upsert executor running: %v", err) + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateReview} + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + resp, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("GetTaskSessionStatus returned error: %v", err) + } + if resp.NeedsResume { + t.Fatal("expected NeedsResume=false when runtime is not Resumable") + } + if !resp.NeedsWorkspaceRestore { + t.Fatal("expected NeedsWorkspaceRestore=true as fallback") + } +} + +// TestGetTaskSessionStatus_CancelledSessionStaysWorkspaceRestore verifies that +// CANCELLED sessions are NOT auto-resumed (the user explicitly stopped them). +func TestGetTaskSessionStatus_CancelledSessionStaysWorkspaceRestore(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCancelled) + + now := time.Now().UTC() + if err := repo.CreateTaskSessionWorktree(ctx, &models.TaskSessionWorktree{ + ID: "wt1", + SessionID: "session1", + WorktreeID: "wid1", + RepositoryID: "repo1", + WorktreePath: "/tmp/worktrees/session1", + WorktreeBranch: "feature/test", + CreatedAt: now, + }); err != nil { + t.Fatalf("failed to create worktree: %v", err) + } + + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "er1", + SessionID: "session1", + TaskID: "task1", + Status: "ready", + Resumable: true, + ResumeToken: "acp-session-123", + CreatedAt: now, + UpdatedAt: now, + }); err != nil { + t.Fatalf("failed to upsert executor running: %v", err) + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateReview} + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + resp, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("GetTaskSessionStatus returned error: %v", err) + } + if resp.NeedsResume { + t.Fatal("expected NeedsResume=false for CANCELLED — user stopped intentionally") + } + if !resp.NeedsWorkspaceRestore { + t.Fatal("expected NeedsWorkspaceRestore=true for CANCELLED with worktree") + } +} + func TestGetTaskSessionStatus_NoAutoResumeWithResumeTokenOnError(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/apps/web/hooks/domains/session/use-session-resumption.test.ts b/apps/web/hooks/domains/session/use-session-resumption.test.ts new file mode 100644 index 0000000000..b991e499b2 --- /dev/null +++ b/apps/web/hooks/domains/session/use-session-resumption.test.ts @@ -0,0 +1,158 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockRequest = vi.fn(); + +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => ({ request: mockRequest }), +})); + +import { + resumeWithSilentFallback, + type ResumeStateSetter, + type ResumptionState, +} from "./use-session-resumption"; + +type SetterCalls = { + resumptionStates: ResumptionState[]; + errors: (string | null)[]; + worktreePaths: (string | null)[]; + worktreeBranches: (string | null)[]; + taskSessionStates: string[]; +}; + +function createSetters(): { setters: ResumeStateSetter; calls: SetterCalls } { + const calls: SetterCalls = { + resumptionStates: [], + errors: [], + worktreePaths: [], + worktreeBranches: [], + taskSessionStates: [], + }; + const setters: ResumeStateSetter = { + setResumptionState: (s: ResumptionState) => { + calls.resumptionStates.push(s); + }, + setError: (e: string | null) => { + calls.errors.push(e); + }, + setWorktreePath: (p: string | null) => { + calls.worktreePaths.push(p); + }, + setWorktreeBranch: (b: string | null) => { + calls.worktreeBranches.push(b); + }, + setTaskSession: (s: { state: string }) => { + calls.taskSessionStates.push(s.state); + }, + }; + return { setters, calls }; +} + +describe("resumeWithSilentFallback", () => { + beforeEach(() => { + vi.clearAllMocks(); + // tryLaunch logs caught errors via console.error; silence in tests so the + // expected error paths don't pollute the test output. + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + it("uses resume on first try when it succeeds, never calling restore_workspace", async () => { + mockRequest.mockResolvedValueOnce({ + success: true, + task_id: "t1", + session_id: "s1", + state: "STARTING", + worktree_path: "/wt/foo", + worktree_branch: "feature/foo", + }); + const { setters, calls } = createSetters(); + + await resumeWithSilentFallback("t1", "s1", null, setters); + + expect(mockRequest).toHaveBeenCalledTimes(1); + expect(mockRequest).toHaveBeenCalledWith( + "session.launch", + expect.objectContaining({ intent: "resume", session_id: "s1" }), + expect.any(Number), + ); + expect(calls.resumptionStates).toContain("resumed"); + expect(calls.errors).not.toContain(expect.any(String)); + expect(calls.worktreePaths).toContain("/wt/foo"); + }); + + it("falls back to restore_workspace silently when resume returns success=false", async () => { + // 1st call: resume fails. 2nd call: restore_workspace succeeds. + mockRequest + .mockResolvedValueOnce({ success: false, task_id: "t1", state: "FAILED" }) + .mockResolvedValueOnce({ + success: true, + task_id: "t1", + session_id: "s1", + state: "FAILED", + worktree_path: "/wt/foo", + }); + const { setters, calls } = createSetters(); + + await resumeWithSilentFallback("t1", "s1", null, setters); + + expect(mockRequest).toHaveBeenCalledTimes(2); + expect(mockRequest).toHaveBeenNthCalledWith( + 1, + "session.launch", + expect.objectContaining({ intent: "resume" }), + expect.any(Number), + ); + expect(mockRequest).toHaveBeenNthCalledWith( + 2, + "session.launch", + expect.objectContaining({ intent: "restore_workspace" }), + expect.any(Number), + ); + // Final state is "resumed" (from successful restore), no error surfaced. + expect(calls.resumptionStates.at(-1)).toBe("resumed"); + expect(calls.errors.filter((e) => typeof e === "string")).toHaveLength(0); + }); + + it("falls back to restore_workspace silently when resume throws", async () => { + mockRequest + .mockRejectedValueOnce(new Error("ws timeout")) + .mockResolvedValueOnce({ success: true, task_id: "t1", session_id: "s1", state: "FAILED" }); + const { setters, calls } = createSetters(); + + await resumeWithSilentFallback("t1", "s1", null, setters); + + expect(mockRequest).toHaveBeenCalledTimes(2); + expect(calls.resumptionStates.at(-1)).toBe("resumed"); + expect(calls.errors.filter((e) => typeof e === "string")).toHaveLength(0); + }); + + it("surfaces an error only when BOTH resume and restore_workspace fail", async () => { + mockRequest + .mockResolvedValueOnce({ success: false, task_id: "t1", state: "FAILED" }) + .mockResolvedValueOnce({ success: false, task_id: "t1", state: "FAILED" }); + const { setters, calls } = createSetters(); + + await resumeWithSilentFallback("t1", "s1", null, setters); + + expect(mockRequest).toHaveBeenCalledTimes(2); + expect(calls.resumptionStates.at(-1)).toBe("error"); + expect(calls.errors.at(-1)).toBe( + "Failed to resume session — workspace restore also unavailable", + ); + }); + + it("surfaces an error when both resume and restore_workspace throw", async () => { + mockRequest + .mockRejectedValueOnce(new Error("ws closed")) + .mockRejectedValueOnce(new Error("still closed")); + const { setters, calls } = createSetters(); + + await resumeWithSilentFallback("t1", "s1", null, setters); + + expect(mockRequest).toHaveBeenCalledTimes(2); + expect(calls.resumptionStates.at(-1)).toBe("error"); + expect(calls.errors.at(-1)).toBe( + "Failed to resume session — workspace restore also unavailable", + ); + }); +}); diff --git a/apps/web/hooks/domains/session/use-session-resumption.ts b/apps/web/hooks/domains/session/use-session-resumption.ts index 8fab85589f..e373a23ed2 100644 --- a/apps/web/hooks/domains/session/use-session-resumption.ts +++ b/apps/web/hooks/domains/session/use-session-resumption.ts @@ -44,7 +44,7 @@ type ResumeResponse = { error?: string; }; -type ResumeStateSetter = { +export type ResumeStateSetter = { setResumptionState: (s: ResumptionState) => void; setError: (e: string | null) => void; setWorktreePath: (p: string | null) => void; @@ -117,6 +117,83 @@ async function resumeViaLaunch( ); } +/** Attempt resume, silently falling back to restore_workspace on any failure. + * Used for sessions where the backend reports needs_resume=true — typically + * WAITING_FOR_INPUT after restart, or FAILED with a resumable token. The user + * only sees an error banner if BOTH attempts fail; otherwise they just see + * the session reload (resumed) or the workspace come back read-only. + * Exported for unit tests. */ +export async function resumeWithSilentFallback( + taskId: string, + sessionId: string, + session: SessionLike, + setters: ResumeStateSetter, +): Promise { + setters.setResumptionState("resuming"); + if ( + await tryLaunch( + buildResumeRequest(taskId, sessionId).request, + taskId, + sessionId, + session, + setters, + ) + ) { + return; + } + // Resume failed (returned success=false OR threw). Fall back to read-only + // workspace restore so the user keeps file/terminal/git access. + if ( + await tryLaunch( + buildRestoreWorkspaceRequest(taskId, sessionId).request, + taskId, + sessionId, + session, + setters, + ) + ) { + return; + } + setters.setResumptionState("error"); + setters.setError("Failed to resume session — workspace restore also unavailable"); +} + +/** Run a single launch attempt; returns true on success, false on any failure. + * Logs caught errors to the console so silent fallback paths remain debuggable + * (errors otherwise vanish into the implicit `false` return). */ +async function tryLaunch( + request: import("@/lib/services/session-launch-service").LaunchSessionRequest, + taskId: string, + sessionId: string, + session: SessionLike, + setters: ResumeStateSetter, +): Promise { + try { + const resp = await launchSession(request); + if (!resp.success) return false; + applyResumeResponse( + { + success: true, + state: resp.state, + worktree_path: resp.worktree_path, + worktree_branch: resp.worktree_branch, + }, + taskId, + sessionId, + session, + setters, + ); + return true; + } catch (err) { + console.error("[tryLaunch] session launch failed", { + intent: request.intent, + sessionId, + err, + }); + return false; + } +} + type CheckAndResumeParams = { taskId: string; sessionId: string; @@ -177,7 +254,7 @@ async function checkAndResume({ if (status.is_agent_running) { setters.setResumptionState("running"); } else if (status.needs_resume && status.is_resumable) { - await resumeViaLaunch(taskId, sessionId, session, setters, buildResumeRequest); + await resumeWithSilentFallback(taskId, sessionId, session, setters); } else if (status.needs_workspace_restore) { await resumeViaLaunch(taskId, sessionId, session, setters, buildRestoreWorkspaceRequest); } else {