From 2fe91cbab8d809a3c989f778c86a7f169cdcb9c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 15 Jul 2026 19:32:32 +0100 Subject: [PATCH] fix: handle parked task sessions (#1711) --- .../task/handlers/process_handlers.go | 36 +++++- .../task/handlers/process_handlers_test.go | 105 ++++++++++++++++++ .../task/task-page-content-helpers.test.ts | 37 +++++- .../task/task-page-content-helpers.ts | 13 +++ .../web/components/task/task-page-content.tsx | 20 +--- .../state/slices/kanban/kanban-slice.test.ts | 14 +++ .../lib/state/slices/kanban/kanban-slice.ts | 3 + .../ws/handlers/agent-session-pure.test.ts | 31 ++++++ apps/web/lib/ws/handlers/agent-session.ts | 14 ++- docs/specs/tasks/model-unification.md | 11 ++ 10 files changed, 256 insertions(+), 28 deletions(-) diff --git a/apps/backend/internal/task/handlers/process_handlers.go b/apps/backend/internal/task/handlers/process_handlers.go index 11041bcbd1..0095895936 100644 --- a/apps/backend/internal/task/handlers/process_handlers.go +++ b/apps/backend/internal/task/handlers/process_handlers.go @@ -479,7 +479,25 @@ func resolveScriptCommand( } } -// httpSetSessionMode sets the session mode for a running agent. +func (h *ProcessHandlers) applyIfSessionReady(sessionID string, allowPassthrough bool, apply func() error) error { + execution, running := h.lifecycleMgr.GetExecutionBySessionID(sessionID) + if !running { + return nil + } + if !h.lifecycleMgr.WasSessionInitialized(execution.ID) && + (!allowPassthrough || execution.PassthroughProcessID == "") { + return nil + } + if err := apply(); err != nil { + if _, stillRunning := h.lifecycleMgr.GetExecutionBySessionID(sessionID); !stillRunning { + return nil + } + return err + } + return nil +} + +// httpSetSessionMode applies the session mode now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionMode(c *gin.Context) { sessionID := c.Param("id") var req struct { @@ -489,7 +507,9 @@ func (h *ProcessHandlers) httpSetSessionMode(c *gin.Context) { c.JSON(400, gin.H{"error": "invalid request: " + err.Error()}) return } - if err := h.lifecycleMgr.SetSessionModeBySessionID(c.Request.Context(), sessionID, req.ModeID); err != nil { + if err := h.applyIfSessionReady(sessionID, false, func() error { + return h.lifecycleMgr.SetSessionModeBySessionID(c.Request.Context(), sessionID, req.ModeID) + }); err != nil { h.logger.Error("failed to set session mode", zap.String("session_id", sessionID), zap.String("mode_id", req.ModeID), @@ -509,7 +529,7 @@ func (h *ProcessHandlers) httpSetSessionMode(c *gin.Context) { c.JSON(200, gin.H{"ok": true}) } -// httpSetSessionModel sets the session model for a running agent. +// httpSetSessionModel applies the session model now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionModel(c *gin.Context) { sessionID := c.Param("id") var req struct { @@ -519,7 +539,9 @@ func (h *ProcessHandlers) httpSetSessionModel(c *gin.Context) { c.JSON(400, gin.H{"error": "invalid request: " + err.Error()}) return } - if err := h.lifecycleMgr.SetSessionModelBySessionID(c.Request.Context(), sessionID, req.ModelID); err != nil { + if err := h.applyIfSessionReady(sessionID, true, func() error { + return h.lifecycleMgr.SetSessionModelBySessionID(c.Request.Context(), sessionID, req.ModelID) + }); err != nil { h.logger.Error("failed to set session model", zap.String("session_id", sessionID), zap.String("model_id", req.ModelID), @@ -539,7 +561,7 @@ func (h *ProcessHandlers) httpSetSessionModel(c *gin.Context) { c.JSON(200, gin.H{"ok": true}) } -// httpSetSessionConfigOption sets a session config option for a running agent. +// httpSetSessionConfigOption applies an option now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionConfigOption(c *gin.Context) { sessionID := c.Param("id") var req struct { @@ -550,7 +572,9 @@ func (h *ProcessHandlers) httpSetSessionConfigOption(c *gin.Context) { c.JSON(400, gin.H{"error": "invalid request: " + err.Error()}) return } - if err := h.lifecycleMgr.SetSessionConfigOptionBySessionID(c.Request.Context(), sessionID, req.ConfigID, req.Value); err != nil { + if err := h.applyIfSessionReady(sessionID, false, func() error { + return h.lifecycleMgr.SetSessionConfigOptionBySessionID(c.Request.Context(), sessionID, req.ConfigID, req.Value) + }); err != nil { h.logger.Error("failed to set session config option", zap.String("session_id", sessionID), zap.String("config_id", req.ConfigID), diff --git a/apps/backend/internal/task/handlers/process_handlers_test.go b/apps/backend/internal/task/handlers/process_handlers_test.go index ac760fe69c..1283267cea 100644 --- a/apps/backend/internal/task/handlers/process_handlers_test.go +++ b/apps/backend/internal/task/handlers/process_handlers_test.go @@ -523,6 +523,16 @@ func (m *mockRepository) UpdateSessionMetadata(ctx context.Context, sessionID st return nil } func (m *mockRepository) SetSessionMetadataKey(ctx context.Context, sessionID, key string, value interface{}) error { + if session := m.sessions[sessionID]; session != nil { + if session.Metadata == nil { + session.Metadata = make(map[string]interface{}) + } + if value == nil { + delete(session.Metadata, key) + } else { + session.Metadata[key] = value + } + } return nil } func (m *mockRepository) DismissLastAgentError(_ context.Context, _ string, _ models.LastAgentError, _ time.Time) (bool, error) { @@ -705,6 +715,101 @@ func TestResolveScriptCommandErrors(t *testing.T) { } } +func TestSetSessionRuntimeConfigPersistsWithoutRunningAgent(t *testing.T) { + tests := []struct { + name string + path string + body string + assertions func(*testing.T, models.SessionRuntimeConfig, map[string]interface{}) + }{ + { + name: "model", + path: "/api/v1/task-sessions/session-idle/set-model", + body: `{"model_id":"claude-opus-4-8"}`, + assertions: func(t *testing.T, cfg models.SessionRuntimeConfig, _ map[string]interface{}) { + t.Helper() + if cfg.Model != "claude-opus-4-8" { + t.Fatalf("expected persisted model, got %q", cfg.Model) + } + }, + }, + { + name: "dynamic model option", + path: "/api/v1/task-sessions/session-idle/set-config-option", + body: `{"config_id":"model","value":"claude-opus-4-8"}`, + assertions: func(t *testing.T, cfg models.SessionRuntimeConfig, _ map[string]interface{}) { + t.Helper() + if cfg.Model != "claude-opus-4-8" || cfg.ConfigOptions["model"] != "claude-opus-4-8" { + t.Fatalf("expected persisted dynamic model option, got %+v", cfg) + } + }, + }, + { + name: "mode", + path: "/api/v1/task-sessions/session-idle/set-mode", + body: `{"mode_id":"acceptEdits"}`, + assertions: func(t *testing.T, cfg models.SessionRuntimeConfig, metadata map[string]interface{}) { + t.Helper() + if cfg.Mode != "acceptEdits" || metadata[models.SessionMetaKeySessionMode] != "acceptEdits" { + t.Fatalf("expected persisted mode, got config=%+v metadata=%+v", cfg, metadata) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("failed to create logger: %v", err) + } + session := &models.TaskSession{ + ID: "session-idle", + TaskID: "task-1", + State: models.TaskSessionStateIdle, + Metadata: make(map[string]interface{}), + } + repo := &mockRepository{sessions: map[string]*models.TaskSession{session.ID: session}} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + lifecycleMgr := newLifecycleManager(t, log) + workspaceOnlyExecution := (&lifecycle.ExecutorInstance{ + InstanceID: "execution-workspace-only", + TaskID: session.TaskID, + SessionID: session.ID, + }).ToAgentExecution(&lifecycle.ExecutorCreateRequest{ + InstanceID: "execution-workspace-only", + TaskID: session.TaskID, + SessionID: session.ID, + AgentProfileID: "profile-1", + WorkspacePath: "/tmp", + }) + addExecution(t, lifecycleMgr, workspaceOnlyExecution) + + router := newProcessStopRouter(t, svc, lifecycleMgr, log) + req := httptest.NewRequest(http.MethodPost, tt.path, strings.NewReader(tt.body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + + if resp.Code != http.StatusOK { + t.Fatalf("expected idle session update to succeed, got %d: %s", resp.Code, resp.Body.String()) + } + cfg, ok := models.LoadSessionRuntimeConfig(session.Metadata) + if !ok { + t.Fatalf("expected persisted runtime config, got %+v", session.Metadata) + } + tt.assertions(t, cfg, session.Metadata) + }) + } +} + func TestStopProcessRejectsDifferentSession(t *testing.T) { log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) if err != nil { diff --git a/apps/web/components/task/task-page-content-helpers.test.ts b/apps/web/components/task/task-page-content-helpers.test.ts index 80d5ab14f4..ad0ef00ace 100644 --- a/apps/web/components/task/task-page-content-helpers.test.ts +++ b/apps/web/components/task/task-page-content-helpers.test.ts @@ -1,10 +1,11 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import type { Task } from "@/lib/types/http"; import { buildDebugEntries, hasResolvedTaskDetails, resolveTaskContentState, resolveTaskProps, + syncActiveTaskSession, } from "./task-page-content-helpers"; function baseParams(overrides: Partial[0]> = {}) { @@ -150,3 +151,37 @@ describe("hasResolvedTaskDetails", () => { ).toBe(false); }); }); + +describe("syncActiveTaskSession", () => { + it("restores the initial session without creating a user pin", () => { + const setActiveSessionAuto = vi.fn(); + const setActiveTask = vi.fn(); + + syncActiveTaskSession({ + initialTaskId: "task-1", + fallbackTaskId: null, + initialSessionId: "session-1", + setActiveSessionAuto, + setActiveTask, + }); + + expect(setActiveSessionAuto).toHaveBeenCalledWith("task-1", "session-1"); + expect(setActiveTask).not.toHaveBeenCalled(); + }); + + it("falls back to selecting the task when there is no initial session", () => { + const setActiveSessionAuto = vi.fn(); + const setActiveTask = vi.fn(); + + syncActiveTaskSession({ + initialTaskId: "task-1", + fallbackTaskId: null, + initialSessionId: null, + setActiveSessionAuto, + setActiveTask, + }); + + expect(setActiveTask).toHaveBeenCalledWith("task-1"); + expect(setActiveSessionAuto).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/components/task/task-page-content-helpers.ts b/apps/web/components/task/task-page-content-helpers.ts index 1a0c4b3611..7faabfbdc9 100644 --- a/apps/web/components/task/task-page-content-helpers.ts +++ b/apps/web/components/task/task-page-content-helpers.ts @@ -127,6 +127,19 @@ export function hasResolvedTaskDetails(params: { ); } +export function syncActiveTaskSession(params: { + initialTaskId: string | undefined; + fallbackTaskId: string | null | undefined; + initialSessionId: string | null; + setActiveSessionAuto: (taskId: string, sessionId: string) => void; + setActiveTask: (taskId: string) => void; +}) { + const taskId = params.initialTaskId ?? params.fallbackTaskId; + if (!taskId) return; + if (params.initialSessionId) params.setActiveSessionAuto(taskId, params.initialSessionId); + else params.setActiveTask(taskId); +} + export function resolveTaskIds(task: Task | null) { return { taskId: task?.id ?? null, diff --git a/apps/web/components/task/task-page-content.tsx b/apps/web/components/task/task-page-content.tsx index 8530680829..d3209c0199 100644 --- a/apps/web/components/task/task-page-content.tsx +++ b/apps/web/components/task/task-page-content.tsx @@ -28,6 +28,7 @@ import { buildArchivedValue, hasResolvedTaskDetails, resolveTaskContentState, + syncActiveTaskSession, } from "@/components/task/task-page-content-helpers"; import { TaskPageInner } from "@/components/task/task-page-inner"; import { GridSpinner } from "@/components/grid-spinner"; @@ -193,19 +194,6 @@ export function useMergedAgentState( return { isResuming, isResumed, taskSessionState, worktreePath, worktreeBranch, isAgentWorking }; } -function syncActiveTaskSession(params: { - initialTaskId: string | undefined; - fallbackTaskId: string | null | undefined; - initialSessionId: string | null; - setActiveSession: (taskId: string, sessionId: string) => void; - setActiveTask: (taskId: string) => void; -}) { - const taskId = params.initialTaskId ?? params.fallbackTaskId; - if (!taskId) return; - if (params.initialSessionId) params.setActiveSession(taskId, params.initialSessionId); - else params.setActiveTask(taskId); -} - function TaskLoadingState() { return (
state.tasks.activeTaskId); - const setActiveSession = useAppStore((state) => state.setActiveSession); + const setActiveSessionAuto = useAppStore((state) => state.setActiveSessionAuto); const setActiveTask = useAppStore((state) => state.setActiveTask); // Validate that activeSessionId belongs to activeTaskId to prevent showing @@ -325,10 +313,10 @@ function useTaskPageData( initialTaskId: initialTask?.id, fallbackTaskId, initialSessionId, - setActiveSession, + setActiveSessionAuto, setActiveTask, }); - }, [initialTask?.id, fallbackTaskId, initialSessionId, setActiveSession, setActiveTask]); + }, [initialTask?.id, fallbackTaskId, initialSessionId, setActiveSessionAuto, setActiveTask]); const { repositories } = useRepositories(task?.workspace_id ?? null, Boolean(task?.workspace_id)); const effectiveRepositories = repositories.length ? repositories : initialRepositories; diff --git a/apps/web/lib/state/slices/kanban/kanban-slice.test.ts b/apps/web/lib/state/slices/kanban/kanban-slice.test.ts index 4a45884b1f..2ef121bf5f 100644 --- a/apps/web/lib/state/slices/kanban/kanban-slice.test.ts +++ b/apps/web/lib/state/slices/kanban/kanban-slice.test.ts @@ -53,4 +53,18 @@ describe("kanban slice active session selection", () => { lastSessionByTaskId: { [TASK_ID]: AUTO_SESSION_ID }, }); }); + + it("clears a pin when auto-selecting a session for a different task", () => { + const store = makeStore(); + + store.getState().setActiveSession(TASK_ID, PINNED_SESSION_ID); + store.getState().setActiveSessionAuto("task-2", AUTO_SESSION_ID); + + expect(store.getState().tasks).toMatchObject({ + activeTaskId: "task-2", + activeSessionId: AUTO_SESSION_ID, + pinnedSessionId: null, + lastSessionByTaskId: { [TASK_ID]: PINNED_SESSION_ID, "task-2": AUTO_SESSION_ID }, + }); + }); }); diff --git a/apps/web/lib/state/slices/kanban/kanban-slice.ts b/apps/web/lib/state/slices/kanban/kanban-slice.ts index 2b738f805c..f2602e6482 100644 --- a/apps/web/lib/state/slices/kanban/kanban-slice.ts +++ b/apps/web/lib/state/slices/kanban/kanban-slice.ts @@ -58,6 +58,9 @@ export const createKanbanSlice: StateCreator< }), setActiveSessionAuto: (taskId, sessionId) => set((draft) => { + if (draft.tasks.activeTaskId !== taskId) { + draft.tasks.pinnedSessionId = null; + } draft.tasks.activeTaskId = taskId; draft.tasks.activeSessionId = sessionId; draft.tasks.lastSessionByTaskId[taskId] = sessionId; diff --git a/apps/web/lib/ws/handlers/agent-session-pure.test.ts b/apps/web/lib/ws/handlers/agent-session-pure.test.ts index 8c4900afb8..f08c31c7c1 100644 --- a/apps/web/lib/ws/handlers/agent-session-pure.test.ts +++ b/apps/web/lib/ws/handlers/agent-session-pure.test.ts @@ -111,6 +111,21 @@ describe("shouldAdoptNewSession", () => { expect(shouldAdoptNewSession(state, "t-1", "STARTING")).toBe(true); }); + it("adopts when the active session is parked idle", () => { + const state = makeAppState({ + tasks: { + activeTaskId: "t-1", + activeSessionId: "s-old", + pinnedSessionId: null, + lastSessionByTaskId: {}, + }, + taskSessions: { + items: { "s-old": { id: "s-old", task_id: "t-1", state: "IDLE" } }, + } as unknown as AppState["taskSessions"], + }); + expect(shouldAdoptNewSession(state, "t-1", "STARTING")).toBe(true); + }); + it("does NOT adopt while the current active session is still running", () => { const state = makeAppState({ tasks: { @@ -187,6 +202,22 @@ describe("pickReplacementSessionId", () => { }); describe("shouldPreservePinnedSessionForTask", () => { + it("preserves a pinned idle session for the active task", () => { + const state = makeAppState({ + tasks: { + activeTaskId: "t-1", + activeSessionId: "s-pinned", + pinnedSessionId: "s-pinned", + lastSessionByTaskId: {}, + }, + taskSessions: { + items: { "s-pinned": { id: "s-pinned", task_id: "t-1", state: "IDLE" } }, + } as unknown as AppState["taskSessions"], + }); + + expect(shouldPreservePinnedSessionForTask(state, "t-1")).toBe(true); + }); + it("preserves a missing pinned session while the per-task list is hydrating", () => { const state = makeAppState({ tasks: { diff --git a/apps/web/lib/ws/handlers/agent-session.ts b/apps/web/lib/ws/handlers/agent-session.ts index e4b5ad8c8d..b3ade0fd30 100644 --- a/apps/web/lib/ws/handlers/agent-session.ts +++ b/apps/web/lib/ws/handlers/agent-session.ts @@ -112,10 +112,10 @@ function maybePromoteAgentctlReady( * follow the switch. Returns true when the caller should adopt the new session * as the task's active session. * - * Adopts only when the current active session is missing, cross-task, or - * already terminal — not while a live session for the same task is still - * running (the backend only creates sessions during workflow step transitions - * after stopping the previous one, but WS events may arrive out of order). + * Adopts only when the current active session is missing, cross-task, parked + * IDLE, or already terminal — not while a live session for the same task is + * still running. A parked IDLE session has no live process, so a newly started + * workflow session should replace it just like a terminal session. */ export function shouldAdoptNewSession( state: AppState, @@ -127,7 +127,11 @@ export function shouldAdoptNewSession( const activeSessionId = state.tasks.activeSessionId; if (activeSessionId) { const activeSession = state.taskSessions.items[activeSessionId]; - if (activeSession?.task_id === taskId && !isTerminalSessionState(activeSession.state)) { + if ( + activeSession?.task_id === taskId && + activeSession.state !== "IDLE" && + !isTerminalSessionState(activeSession.state) + ) { return false; } } diff --git a/docs/specs/tasks/model-unification.md b/docs/specs/tasks/model-unification.md index 842e3b1788..992615ba10 100644 --- a/docs/specs/tasks/model-unification.md +++ b/docs/specs/tasks/model-unification.md @@ -365,6 +365,17 @@ preserved. binds to `task_sessions.current_execution_id` and shows live state. When the execution ends, the dockview renders dormant. +- **GIVEN** advanced mode is showing an unpinned `IDLE` session, + **WHEN** a workflow transition starts a different session for the + same task, **THEN** the dockview selects the newly started session. + An explicitly selected live session remains selected. + +- **GIVEN** advanced mode is showing a parked `IDLE` session, **WHEN** + the user changes its model, mode, or dynamic runtime option before + sending the next message, **THEN** the selection is persisted and + applied when the agent process resumes. A running session applies the + same selection immediately and persists it for future recovery. + ## Out of scope - Per-workspace strategy lock UI. The unified model serves mixed