diff --git a/AGENTS.md b/AGENTS.md index c39304b3ec..f3036f06b9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,7 +174,7 @@ history and remains immutable. - ADR 0015's step-completion-signal telemetry lives under `workflow_*`: `workflow_step_completion_signal_received_total` (`internal/workflow/signalmetrics/`), labelled by `source` and `agent_type`, counts accepted `step_complete_kandev` signals. Its separate `workflow_step_completion_signal_fallback_used_total` counter is labelled by `agent_type` and counts manual fallback uses. The fallback button does not have a production increment site yet, so the counter remains zero until that UI ships. - Office stall detection (REQ-OFFICE-STALL-VISIBILITY) lives under `office_stall_*`: `office_stall_stranded_signal_total` (labelled by `gate`, which names which of the two watchdog gate sites saw it), `office_stall_decision_waiting_total`, and `office_stall_detector_skipped_total` (labelled by `reason`, so a detector that fails closed is visible rather than silent). Detection only: these counters never accompany a transition, a synthesized decision, or a queued run, because Office tasks are surfaced and never reclaimed. Also emitted as structured zap logs. - Office session-termination suppression (REQ-OFFICE-SESSION-TERM) lives under `office_session_term_suppressed_total`, labelled by `reason` (one of `task_reassigned`, `participant_removed`, `participant_seat_claimed`) and `outcome` (`runner` or `seat` when the agent still holds that capacity, `read_failed` when the capacity read itself failed). It counts guarded terminations that left a session live, so a suppression is distinguishable from a path that never ran. Both dimensions are closed sets; no task, agent, step or session identifier is ever a label. Also emitted as structured zap logs. -- Task-level stall detection (issue #3712) runs in the session reconciliation sweep (`internal/task/service/`, one-minute tick shared with the archived-session pass): an unarchived task holding an active session with no live execution behind it and no events/messages past `tasks.stallDetectionThreshold` (default 2h) emits a `task.stalled` event plus a `stalled_task` WARN, once per stall episode. At or beyond twice the threshold the sweep cancels the orphaned sessions (`error_message` = "orphaned session") through the archived pass's `finalizeCancelledSessions` transition scoped to the classified session IDs; the heal runs only when every active session of the task is orphaned and past the grace window (a live or merely-stalled sibling cancels nothing), and liveness is re-checked at the cancellation boundary. A stall past the grace window can therefore emit `task.stalled` and a `session.state_changed` in the same sweep; the cancel never depends on the detection event. +- Task-level stall detection (issue #3712) runs in the session reconciliation sweep (`internal/task/service/`, one-minute tick shared with the archived-session pass): an unarchived task holding a non-idle active session with no live execution behind it and no events/messages past `tasks.stallDetectionThreshold` (default 2h) emits a `task.stalled` event plus a `stalled_task` WARN, once per stall episode. At or beyond twice the threshold the sweep conditionally settles classified interrupted sessions to `WAITING_FOR_INPUT`, preserving the same conversation and abandoning only the observed turn without a completion event; the recovery runs only when every active session of the task is execution-less and past the grace window (an idle waiting sibling blocks it), and liveness is re-checked at the recovery boundary. A stall past the grace window can therefore emit `task.stalled` and a `session.state_changed` in the same sweep; recovery never depends on the detection event. The separate restart pass applies the same guarded settlement per stale `STARTING`/`RUNNING` session, including rows without an executor record. ### GitHub Operations diff --git a/apps/backend/internal/backendapp/orchestrator.go b/apps/backend/internal/backendapp/orchestrator.go index 34cf37cdb2..fcca50b576 100644 --- a/apps/backend/internal/backendapp/orchestrator.go +++ b/apps/backend/internal/backendapp/orchestrator.go @@ -170,8 +170,9 @@ func provideOrchestrator( // Runtime-aware liveness lets durable cleanup treat a not-found stop for a // confirmed-dead local runtime as already stopped instead of retrying forever. taskSvc.SetRowLivenessProber(agentManagerClient) - // The orphan-session sweep terminalizes stale STARTING/RUNNING sessions no - // live in-memory execution backs (backend-restart residue, #3711). + // The orphan-session sweep preserves stale STARTING/RUNNING sessions when no + // live in-memory execution backs them, so the conversation can recover on + // task focus after a backend restart. taskSvc.SetExecutionLivenessChecker(agentManagerClient) // The session reconciliation sweep's active-task pass (stall detection and // orphaned-session healing) verifies "no live execution" against the agent diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index 5e9c261124..786fd30ed6 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -567,7 +567,16 @@ func (s *Service) handleAgentBootReady(ctx context.Context, data watcher.AgentEv zap.String("session_state", string(session.State))) return } - recoveryResolvedAt := s.markRecoveryResolved(ctx, data.SessionID, session) + markerSnapshot, markerSnapshotKnown := s.interruptedMarkerSnapshotForResumeAttempt(data.SessionID, data.AttemptID) + // Every boot-ready callback is a recovery callback for marker purposes. A + // missing/finished attempt, empty attempt ID, or failed task snapshot leaves + // the marker snapshot unavailable and therefore fails closed in + // clearTaskInterruptedMarker. + // Ordinary sessions without a marker take the same no-op path; only an + // immutable marker captured for this attempt can clear the warning. + recoveryResolvedAt := s.markRecoveryResolved( + ctx, data.SessionID, session, markerSnapshot, markerSnapshotKnown, + ) // Idempotent: if the session is already WAITING_FOR_INPUT (e.g. revived // from a previously launched session and the boot signal arrived faster @@ -3215,7 +3224,13 @@ func (s *Service) dismissRecoveredAgentError( // The boot transcript is useful observability, but its writes are best effort. // Session metadata is the authoritative recovery result used by the frontend // after a reload when the transcript row is missing or incomplete. -func (s *Service) markRecoveryResolved(ctx context.Context, sessionID string, session *models.TaskSession) *time.Time { +func (s *Service) markRecoveryResolved( + ctx context.Context, + sessionID string, + session *models.TaskSession, + markerSnapshot interruptedMarkerSnapshot, + markerSnapshotKnown bool, +) *time.Time { resolvedAt := time.Now().UTC() resolvedAtValue := resolvedAt.Format(time.RFC3339Nano) if err := s.repo.SetSessionMetadataKey( @@ -3235,10 +3250,51 @@ func (s *Service) markRecoveryResolved(ctx context.Context, sessionID string, se session.Metadata[models.SessionMetaKeyRecoveryResolvedAt] = resolvedAtValue if session.TaskID != "" { s.dismissRecoveredAgentError(ctx, session.TaskID, session, resolvedAt) + // Boot-ready is the provider-confirmed recovery boundary. Keeping this + // out of the STARTING/RUNNING state funnel leaves the durable warning in + // place when a launch later fails or is cancelled. + if markerSnapshotKnown && markerSnapshot.captured { + s.clearTaskInterruptedMarker(ctx, session.TaskID, markerSnapshot.value) + } + } + // A guarded boot callback may clear recovery ownership only when it carries + // a valid immutable marker snapshot. A known recovery attempt whose task + // read failed keeps its durable settlement so a later sweep can retry it. + if !markerSnapshotKnown || markerSnapshot.captured { + s.clearRecoveryMetadataAfterBoot(ctx, sessionID, session) } return &resolvedAt } +func (s *Service) clearRecoveryMetadataAfterBoot( + ctx context.Context, + sessionID string, + session *models.TaskSession, +) { + remover, supported := s.repo.(interface { + RemoveSessionMetadataKeyIfJSONValue(context.Context, string, string, interface{}) (bool, error) + }) + if !supported || session == nil { + return + } + if pending, ok := session.Metadata[models.SessionMetaKeyInterruptedRecoveryPending].(string); ok && pending != "" { + if _, err := remover.RemoveSessionMetadataKeyIfJSONValue( + ctx, sessionID, models.SessionMetaKeyInterruptedRecoveryPending, pending, + ); err != nil { + s.logger.Warn("failed to clear interrupted recovery marker", + zap.String("session_id", sessionID), zap.Error(err)) + } + } + if settlement, ok := models.LoadInterruptedRecoverySettlement(session.Metadata); ok { + if _, err := remover.RemoveSessionMetadataKeyIfJSONValue( + ctx, sessionID, models.SessionMetaKeyRecoverySettlementPending, settlement, + ); err != nil { + s.logger.Warn("failed to clear recovery settlement marker after boot", + zap.String("session_id", sessionID), zap.Error(err)) + } + } +} + // providerRemediationURL returns the adapter-validated remediation URL from the // normalized provider diagnostic, or "" when the failure carried none. The URL // is only ever set by the adapter's allowlist validator; the orchestrator does diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming.go b/apps/backend/internal/orchestrator/event_handlers_streaming.go index 4899b9d265..d3aa5d41ac 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming.go @@ -1203,11 +1203,10 @@ func (s *Service) updateTaskSessionStateWithHook( zap.Error(err)) } } - // Work has resumed: a session entering STARTING/RUNNING clears the - // startup interruption marker and republishes the task so open clients - // drop the red interruption icon. No-op when the marker is absent. + // Entering STARTING/RUNNING only records a recovery attempt. The durable + // interruption marker is cleared after the provider confirms boot/readiness, + // so failed or cancelled attempts remain visible to the user. if nextState == models.TaskSessionStateStarting || nextState == models.TaskSessionStateRunning { - s.clearTaskInterruptedMarker(ctx, taskID) s.clearTaskAutoStartFailedMarker(ctx, taskID) } if authoritativeUpdatedAt == nil { @@ -2174,9 +2173,8 @@ func (s *Service) setSessionStartingWithOptions( } // The launch path moves a session to STARTING without going through - // updateTaskSessionStateWithHook, so clear the interruption marker here - // too (no-op when absent). - s.clearTaskInterruptedMarker(ctx, taskID) + // updateTaskSessionStateWithHook. It records an attempt only; the + // interruption marker is cleared after confirmed provider readiness. s.clearTaskAutoStartFailedMarker(ctx, taskID) if publishSession != nil { @@ -2187,14 +2185,39 @@ func (s *Service) setSessionStartingWithOptions( // clearTaskInterruptedMarker removes the startup interruption marker from a // task and republishes task.updated when it was actually present, so open -// clients drop the red interruption icon. Called from the session-start -// funnel when a session enters STARTING/RUNNING — work has resumed, so the -// task is no longer interrupted. No-op when the marker is absent. -func (s *Service) clearTaskInterruptedMarker(ctx context.Context, taskID string) { +// clients drop the warning icon. Callers invoke it only after provider +// readiness confirms that recovery succeeded. No-op when the marker is absent. +func (s *Service) clearTaskInterruptedMarker( + ctx context.Context, + taskID string, + expectedMarker string, +) { if taskID == "" { return } - removed, err := s.repo.RemoveTaskMetadataKey(ctx, taskID, models.MetaKeyInterruptedAt) + var ( + removed bool + err error + ) + if strings.TrimSpace(expectedMarker) == "" { + // A recovery callback without a valid immutable marker snapshot fails + // closed. There is no safe unconditional removal path. + return + } + if remover, ok := s.repo.(interface { + RemoveTaskMetadataKeyIfValue(context.Context, string, string, string) (bool, error) + }); ok { + removed, err = remover.RemoveTaskMetadataKeyIfValue( + ctx, taskID, models.MetaKeyInterruptedAt, expectedMarker, + ) + } else { + // A read followed by an unconditional legacy removal is not a + // compare-and-set. Refuse to clear when the adapter cannot provide the + // guarded primitive so a delayed callback cannot erase a newer marker. + s.logger.Warn("skipping interrupted-marker clear without compare-and-set support", + zap.String("task_id", taskID)) + return + } if err != nil { s.logger.Warn("failed to clear interrupted marker", zap.String("task_id", taskID), @@ -2216,9 +2239,9 @@ func (s *Service) clearTaskInterruptedMarker(ctx context.Context, taskID string) // clearTaskAutoStartFailedMarker removes the auto-start-failure marker from a // task and republishes task.updated when it was actually present, so open -// clients drop the failure badge. Called from the same session-start funnel -// as clearTaskInterruptedMarker: a session entering STARTING/RUNNING means an -// agent did launch, so any earlier auto-start failure no longer applies. +// clients drop the failure badge. It shares the session-start funnel with the +// interruption marker, but remains clear at launch admission because it means +// the auto-start request itself was accepted. // No-op when the marker is absent. func (s *Service) clearTaskAutoStartFailedMarker(ctx context.Context, taskID string) { if taskID == "" { @@ -2699,7 +2722,6 @@ func (s *Service) setQueuedSessionRunningForIdentity( session.UpdatedAt = updatedAt if oldState != models.TaskSessionStateRunning { s.reconcileRunningTaskStateLocked(ctx, identity.TaskID, identity.SessionID) - s.clearTaskInterruptedMarker(ctx, identity.TaskID) s.clearTaskAutoStartFailedMarker(ctx, identity.TaskID) s.publishTaskSessionStateChanged( ctx, diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go index 6fade959ee..efc05c081e 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go @@ -2758,10 +2758,10 @@ func seedInterruptedMarker(t *testing.T, repo *sqliterepo.Repository, taskID str } } -func TestSessionStartClearsInterruptedMarker(t *testing.T) { +func TestSessionStartKeepsInterruptedMarkerUntilRecovery(t *testing.T) { ctx := context.Background() - t.Run("state hook transition to STARTING clears and republishes", func(t *testing.T) { + t.Run("state hook transition to STARTING keeps the marker", func(t *testing.T) { repo := setupTestRepo(t) seedSession(t, repo, "t1", "s1", "step1") seedInterruptedMarker(t, repo, "t1") @@ -2779,12 +2779,12 @@ func TestSessionStartClearsInterruptedMarker(t *testing.T) { task, err := repo.GetTask(ctx, "t1") require.NoError(t, err) _, marked := task.Metadata[models.MetaKeyInterruptedAt] - require.False(t, marked, "marker must be cleared when the session enters STARTING") - require.Equal(t, []string{"t1"}, publisher.updatedTaskIDs, - "task.updated must be republished after clearing the marker") + require.True(t, marked, "marker must survive the STARTING transition") + require.Empty(t, publisher.updatedTaskIDs, + "STARTING must not publish a marker-clearing task.updated") }) - t.Run("launch path via setSessionStarting clears and republishes", func(t *testing.T) { + t.Run("launch path via setSessionStarting keeps the marker", func(t *testing.T) { repo := setupTestRepo(t) seedSession(t, repo, "t1", "s1", "step1") seedInterruptedMarker(t, repo, "t1") @@ -2804,9 +2804,151 @@ func TestSessionStartClearsInterruptedMarker(t *testing.T) { task, err := repo.GetTask(ctx, "t1") require.NoError(t, err) _, marked := task.Metadata[models.MetaKeyInterruptedAt] - require.False(t, marked, "marker must be cleared on the launch path") + require.True(t, marked, "the launch attempt must not clear the marker") + require.Empty(t, publisher.updatedTaskIDs, + "the launch attempt must not publish a marker-clearing task.updated") + }) + + t.Run("confirmed boot recovery clears and republishes", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedInterruptedMarker(t, repo, "t1") + + publisher := &recordingTaskUpdatedPublisher{} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetTaskEventPublisher(publisher) + + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + marker, ok := task.Metadata[models.MetaKeyInterruptedAt].(string) + require.True(t, ok) + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, interruptedMarkerSnapshot{value: marker, captured: true}, true)) + + task, err = repo.GetTask(ctx, "t1") + require.NoError(t, err) + _, marked := task.Metadata[models.MetaKeyInterruptedAt] + require.False(t, marked, "successful boot recovery must clear the marker") require.Equal(t, []string{"t1"}, publisher.updatedTaskIDs, - "task.updated must be republished after clearing the marker") + "successful boot recovery must publish the marker-clearing task.updated") + }) + + t.Run("stale recovery preserves a newer interruption marker", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "old-marker")) + + publisher := &recordingTaskUpdatedPublisher{} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetTaskEventPublisher(publisher) + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "new-marker")) + + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, interruptedMarkerSnapshot{value: "old-marker", captured: true}, true)) + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + require.Equal(t, "new-marker", task.Metadata[models.MetaKeyInterruptedAt]) + require.Empty(t, publisher.updatedTaskIDs, + "a stale recovery callback must not publish a marker-clearing update") + }) + + t.Run("recovery callback with no marker snapshot fails closed", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "new-marker")) + + publisher := &recordingTaskUpdatedPublisher{} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetTaskEventPublisher(publisher) + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + // A recovery attempt can finish after its task snapshot failed to load. + // An uncaptured snapshot must remain a guarded no-op. + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, interruptedMarkerSnapshot{}, true)) + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + require.Equal(t, "new-marker", task.Metadata[models.MetaKeyInterruptedAt]) + require.Empty(t, publisher.updatedTaskIDs) + }) + + t.Run("finished recovery attempt with absent marker preserves a newer marker", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + registry := newResumeAttemptRegistry() + svc.resumeAttemptsMu.Lock() + svc.resumeAttempts = registry + svc.resumeAttemptsMu.Unlock() + attempt, owner := registry.begin(ctx, "t1", "s1") + require.True(t, owner) + attempt.setInterruptedMarkerSnapshot("", true) + attempt.finish(registry) + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "new-marker")) + + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + snapshot, known := svc.interruptedMarkerSnapshotForResumeAttempt("s1", attempt.identity()) + require.True(t, known) + require.True(t, snapshot.captured) + require.Empty(t, snapshot.value) + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, snapshot, known)) + + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + require.Equal(t, "new-marker", task.Metadata[models.MetaKeyInterruptedAt]) + }) + + t.Run("finished recovery attempt with failed marker read preserves a newer marker", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + registry := newResumeAttemptRegistry() + svc.resumeAttemptsMu.Lock() + svc.resumeAttempts = registry + svc.resumeAttemptsMu.Unlock() + attempt, owner := registry.begin(ctx, "t1", "s1") + require.True(t, owner) + // Leave the snapshot uncaptured to model a failed GetTask read. + attempt.finish(registry) + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "new-marker")) + + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + snapshot, known := svc.interruptedMarkerSnapshotForResumeAttempt("s1", attempt.identity()) + require.True(t, known) + require.False(t, snapshot.captured) + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, snapshot, known)) + + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + require.Equal(t, "new-marker", task.Metadata[models.MetaKeyInterruptedAt]) + }) + + t.Run("finished recovery attempt retains its immutable marker snapshot", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "old-marker")) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + registry := newResumeAttemptRegistry() + svc.resumeAttemptsMu.Lock() + svc.resumeAttempts = registry + svc.resumeAttemptsMu.Unlock() + attempt, owner := registry.begin(ctx, "t1", "s1") + require.True(t, owner) + attempt.setInterruptedMarkerSnapshot("old-marker", true) + attempt.finish(registry) + require.NoError(t, repo.SetTaskMetadataKey(ctx, "t1", models.MetaKeyInterruptedAt, "new-marker")) + + session, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + marker := svc.interruptedMarkerForResumeAttempt("s1", attempt.identity()) + require.Equal(t, "old-marker", marker) + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", session, interruptedMarkerSnapshot{value: marker, captured: true}, true)) + task, err := repo.GetTask(ctx, "t1") + require.NoError(t, err) + require.Equal(t, "new-marker", task.Metadata[models.MetaKeyInterruptedAt]) }) t.Run("no marker means no republish", func(t *testing.T) { @@ -2992,7 +3134,7 @@ func TestClearRecoveredAgentErrorOnTurnCompletion(t *testing.T) { }, )) - require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", snapshot)) + require.NotNil(t, svc.markRecoveryResolved(ctx, "s1", snapshot, interruptedMarkerSnapshot{}, false)) stored, err := repo.GetTaskSession(ctx, "s1") require.NoError(t, err) @@ -3074,7 +3216,7 @@ func TestTransitionBootstrapFailurePersistsSessionHistory(t *testing.T) { reloaded, err := repo.GetTaskSession(ctx, "bootstrap-history-session") require.NoError(t, err) - resolvedAt := svc.markRecoveryResolved(ctx, reloaded.ID, reloaded) + resolvedAt := svc.markRecoveryResolved(ctx, reloaded.ID, reloaded, interruptedMarkerSnapshot{}, false) require.NotNil(t, resolvedAt) afterRecovery, err := repo.GetTaskSession(ctx, "bootstrap-history-session") diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 4d5accad26..9e05cfc970 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -1679,16 +1679,15 @@ func (e *Executor) applyExecutorConfigToResumeRequest(ctx context.Context, req * return execConfig } -// isArchiveCancelledResumeSession reports whether session was cancelled by an -// archive (Service.ArchiveTask's single-task path or HandoffService's cascade -// archive) rather than an explicit user/coordinator stop. Mirrors -// orchestrator.isArchiveCancelledSession — kept local to this package since -// the two live on opposite sides of the executor/orchestrator boundary and -// the check is two lines over already-exported models helpers. -func isArchiveCancelledResumeSession(session *models.TaskSession) bool { +// isRecoverableCancelledResumeSession reports whether session was cancelled by +// a system reconciliation path rather than an explicit user/coordinator stop. +// Mirrors the orchestrator's eligibility checks while keeping the executor's +// prompt-free launch behavior aligned for legacy archive and orphan rows. +func isRecoverableCancelledResumeSession(session *models.TaskSession) bool { return session != nil && session.State == models.TaskSessionStateCancelled && - models.IsArchiveCancelReason(session.ErrorMessage) + (models.IsArchiveCancelReason(session.ErrorMessage) || + models.IsOrphanCancelReason(session.ErrorMessage)) } // applyRunningRecordToResumeRequest loads the ExecutorRunning record and applies @@ -1702,12 +1701,12 @@ func (e *Executor) applyRunningRecordToResumeRequest( ) *models.ExecutorRunning { if running == nil { // Archive cleanup tears down the executors_running row entirely, so an - // archive-cancelled session reaches this point with running == nil. The + // system-cancelled session reaches this point with running == nil. The // session metadata mirrors the provider conversation identity so an // explicit completed follow-up can still restore the same conversation // after runtime cleanup removed the operational row. noAutoPromptState := session.State == models.TaskSessionStateWaitingForInput || - isArchiveCancelledResumeSession(session) || + isRecoverableCancelledResumeSession(session) || session.State == models.TaskSessionStateCompleted if startAgent && noAutoPromptState { if token := persistedSessionResumeToken(session); token != "" { @@ -1755,9 +1754,9 @@ func (e *Executor) applyRunningRecordToResumeRequest( zap.String("session_id", session.ID), zap.Bool("has_resume_token", running.ResumeToken != "")) } else if startAgent && (session.State == models.TaskSessionStateWaitingForInput || - isArchiveCancelledResumeSession(session) || session.State == models.TaskSessionStateCompleted) { + isRecoverableCancelledResumeSession(session) || session.State == models.TaskSessionStateCompleted) { // Fresh-start resume (no resume token): don't auto-prompt with the task - // description. Also covers completed and archive-cancelled sessions whose + // description. Also covers completed and system-cancelled sessions whose // running record survived cleanup but carries no token — the same // auto-resume shape as the running==nil branch above. req.TaskDescription = "" diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_test.go index 7a38332b73..2c7a09f771 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_test.go @@ -1011,6 +1011,65 @@ func TestResumeSession_ArchiveCancelledWithoutRunningRow_ClearsTaskDescription(t } } +// TestResumeSession_OrphanCancelledWithoutRunningRow_ClearsTaskDescription +// preserves the prompt-free recovery contract for legacy rows that the +// reconciliation sweep cancelled after losing their runtime inventory. +func TestResumeSession_OrphanCancelledWithoutRunningRow_ClearsTaskDescription(t *testing.T) { + repo := newMockRepository() + now := time.Now().UTC() + repo.tasks["task-1"] = &models.Task{ + ID: "task-1", + WorkspaceID: "workspace-1", + Description: "do the original thing", + CreatedAt: now, + UpdatedAt: now, + } + repo.sessions["sess-1"] = &models.TaskSession{ + ID: "sess-1", + TaskID: "task-1", + AgentProfileID: "profile-1", + State: models.TaskSessionStateCancelled, + ErrorMessage: models.SessionOrphanedCancelReason, + } + + var capturedReq *LaunchAgentRequest + agentMgr := &mockAgentManager{ + launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { + capturedReq = req + return &LaunchAgentResponse{AgentExecutionID: "exec-new", Status: v1.AgentStatusStarting}, nil + }, + } + exec := newTestExecutor(t, agentMgr, repo) + + callerSession := *repo.sessions["sess-1"] + if _, err := exec.ResumeSession(context.Background(), &callerSession, true); err != nil { + t.Fatalf("ResumeSession: %v", err) + } + if capturedReq == nil { + t.Fatal("LaunchAgent was not called") + } + if capturedReq.TaskDescription != "" { + t.Errorf("TaskDescription = %q, want empty — auto-resuming an orphan-cancelled "+ + "session without a running row must not replay the original prompt", capturedReq.TaskDescription) + } +} + +func TestApplyRunningRecordToResumeRequest_OrphanCancelledTokenlessRowClearsTaskDescription(t *testing.T) { + repo := newMockRepository() + exec := newTestExecutor(t, &mockAgentManager{}, repo) + req := &LaunchAgentRequest{TaskDescription: "do the original thing"} + task := &v1.Task{ID: "task-1"} + session := &models.TaskSession{ + ID: "sess-1", State: models.TaskSessionStateCancelled, + ErrorMessage: models.SessionOrphanedCancelReason, + } + + exec.applyRunningRecordToResumeRequest(req, task, session, true, &models.ExecutorRunning{}) + if req.TaskDescription != "" { + t.Fatalf("TaskDescription = %q, want empty for tokenless orphan recovery", req.TaskDescription) + } +} + func TestApplyRunningRecordToResumeRequest_FailedSessionKeepsTaskDescription(t *testing.T) { repo := newMockRepository() exec := newTestExecutor(t, &mockAgentManager{}, repo) diff --git a/apps/backend/internal/orchestrator/reconcile_retracked_sessions_test.go b/apps/backend/internal/orchestrator/reconcile_retracked_sessions_test.go index d37a044e5f..7a5720eb90 100644 --- a/apps/backend/internal/orchestrator/reconcile_retracked_sessions_test.go +++ b/apps/backend/internal/orchestrator/reconcile_retracked_sessions_test.go @@ -144,6 +144,8 @@ func TestReconcileSessionsOnStartupAppliesFullReconciliationWhenNotRetracked(t * turns := &countingTurnService{} svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, &mockAgentManager{}) svc.turnService = turns + publisher := &recordingTaskUpdatedPublisher{} + svc.SetTaskEventPublisher(publisher) svc.SetRetrackedSessionChecker(func(sessionID string) bool { return false }) svc.reconcileSessionsOnStartup(ctx) @@ -171,6 +173,9 @@ func TestReconcileSessionsOnStartupAppliesFullReconciliationWhenNotRetracked(t * if got := turns.calls(); got != 1 { t.Fatalf("AbandonOpenTurns called %d times, want 1", got) } + if len(publisher.updatedTaskIDs) != 1 || publisher.updatedTaskIDs[0] != "task1" { + t.Fatalf("task.updated publishes = %v, want [task1] after startup marker commit", publisher.updatedTaskIDs) + } } // TestReconcileSessionsOnStartupUnwiredCheckerTreatsEverySessionAsNotRetracked diff --git a/apps/backend/internal/orchestrator/resume_attempt.go b/apps/backend/internal/orchestrator/resume_attempt.go index 2448f686f3..a1c12118fd 100644 --- a/apps/backend/internal/orchestrator/resume_attempt.go +++ b/apps/backend/internal/orchestrator/resume_attempt.go @@ -11,6 +11,7 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/task/models" ) // ErrResumeAttemptCancelled is returned when a startup continuation no longer @@ -33,6 +34,12 @@ type resumeAttempt struct { finishOnce sync.Once executionMu sync.Mutex executionID string + // interruptedMarker is the marker value observed when this recovery attempt + // was admitted. A delayed boot callback may clear only that exact marker; + // a later restart must remain visible until its own attempt succeeds. + interruptedMarkerMu sync.RWMutex + interruptedMarker string + interruptedMarkerCaptured bool // retained is protected by resumeAttemptRegistry.mu. A cancelled attempt // can be retained when a replacement is admitted and retained again when // its owner eventually returns; both paths must describe one tombstone. @@ -66,10 +73,17 @@ const maxResumeAttemptTombstones = 16 const resumeAttemptIdentityPrefix = "resume-" type resumeAttemptTombstone struct { - id uint64 - executionID string - cancelled bool - accepted bool + id uint64 + executionID string + cancelled bool + accepted bool + interruptedMarker string + interruptedMarkerCaptured bool +} + +type interruptedMarkerSnapshot struct { + value string + captured bool } func newResumeAttemptRegistry() *resumeAttemptRegistry { @@ -231,6 +245,8 @@ func (r *resumeAttemptRegistry) retainLocked(attempt *resumeAttempt) { } if entries[index].id == attempt.id { entries[index].accepted = attempt.accepted + entries[index].interruptedMarker = attempt.interruptedMarkerValue() + entries[index].interruptedMarkerCaptured = attempt.interruptedMarkerSnapshotCaptured() break } } @@ -240,10 +256,12 @@ func (r *resumeAttemptRegistry) retainLocked(attempt *resumeAttempt) { attempt.retained = true entries := r.tombstones[attempt.sessionID] entries = append(entries, resumeAttemptTombstone{ - id: attempt.id, - executionID: executionID, - cancelled: attempt.ctx.Err() != nil, - accepted: attempt.accepted, + id: attempt.id, + executionID: executionID, + cancelled: attempt.ctx.Err() != nil, + accepted: attempt.accepted, + interruptedMarker: attempt.interruptedMarkerValue(), + interruptedMarkerCaptured: attempt.interruptedMarkerSnapshotCaptured(), }) if len(entries) > maxResumeAttemptTombstones { entries = entries[len(entries)-maxResumeAttemptTombstones:] @@ -424,6 +442,34 @@ func (attempt *resumeAttempt) setExecutionID(executionID string) { attempt.executionMu.Unlock() } +func (attempt *resumeAttempt) setInterruptedMarkerSnapshot(marker string, captured bool) { + if attempt == nil { + return + } + attempt.interruptedMarkerMu.Lock() + attempt.interruptedMarker = marker + attempt.interruptedMarkerCaptured = captured + attempt.interruptedMarkerMu.Unlock() +} + +func (attempt *resumeAttempt) interruptedMarkerValue() string { + if attempt == nil { + return "" + } + attempt.interruptedMarkerMu.RLock() + defer attempt.interruptedMarkerMu.RUnlock() + return attempt.interruptedMarker +} + +func (attempt *resumeAttempt) interruptedMarkerSnapshotCaptured() bool { + if attempt == nil { + return false + } + attempt.interruptedMarkerMu.RLock() + defer attempt.interruptedMarkerMu.RUnlock() + return attempt.interruptedMarkerCaptured +} + func (attempt *resumeAttempt) execution() string { if attempt == nil { return "" @@ -494,6 +540,9 @@ func (s *Service) beginResumeAttempt( attempt, owner := s.resumeAttemptStore().begin(ctx, taskID, sessionID) lock.Unlock() release() + if owner { + s.captureInterruptedMarkerForResumeAttempt(ctx, attempt) + } return attempt, owner, nil } lock.Unlock() @@ -508,6 +557,61 @@ func (s *Service) beginResumeAttempt( } } +func (s *Service) captureInterruptedMarkerForResumeAttempt(ctx context.Context, attempt *resumeAttempt) { + if s == nil || s.repo == nil || attempt == nil || attempt.taskID == "" { + return + } + task, err := s.repo.GetTask(ctx, attempt.taskID) + if err != nil || task == nil { + return + } + if task.Metadata == nil { + attempt.setInterruptedMarkerSnapshot("", true) + return + } + marker, ok := task.Metadata[models.MetaKeyInterruptedAt].(string) + // A successful task read with no marker is still an immutable snapshot. + // Preserve that fact through tombstones, while the empty value remains a + // fail-closed generation for the recovery callback. + attempt.setInterruptedMarkerSnapshot(marker, ok) +} + +func (s *Service) interruptedMarkerSnapshotForResumeAttempt( + sessionID, attemptID string, +) (interruptedMarkerSnapshot, bool) { + if s == nil || sessionID == "" || attemptID == "" { + return interruptedMarkerSnapshot{}, false + } + id, isRecovery := parseResumeAttemptIdentity(attemptID) + if !isRecovery { + return interruptedMarkerSnapshot{}, false + } + registry := s.resumeAttemptStore() + registry.mu.Lock() + defer registry.mu.Unlock() + if attempt := registry.attempts[sessionID]; attempt != nil && attempt.id == id { + return interruptedMarkerSnapshot{ + value: attempt.interruptedMarkerValue(), + captured: attempt.interruptedMarkerSnapshotCaptured(), + }, true + } + if tombstone, ok := registry.tombstoneLocked(sessionID, id); ok { + return interruptedMarkerSnapshot{ + value: tombstone.interruptedMarker, + captured: tombstone.interruptedMarkerCaptured, + }, true + } + return interruptedMarkerSnapshot{}, false +} + +func (s *Service) interruptedMarkerForResumeAttempt(sessionID, attemptID string) string { + snapshot, known := s.interruptedMarkerSnapshotForResumeAttempt(sessionID, attemptID) + if !known || !snapshot.captured { + return "" + } + return snapshot.value +} + func (s *Service) validateResumeAttempt(attempt *resumeAttempt) error { if attempt == nil { return nil diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 3c496bf213..0d99e2a826 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -3671,20 +3671,34 @@ func (s *Service) reconcileActiveSessionOnStartup( } // Mark the task interrupted when its session was mid-turn when the backend - // died (STARTING/RUNNING) so task-list surfaces can show the red - // interruption icon. WAITING_FOR_INPUT sessions were idle, not interrupted. + // died (STARTING/RUNNING) so task-list surfaces can show the warning + // indicator. WAITING_FOR_INPUT sessions were idle, not interrupted. // A re-tracked session's task was never actually interrupted -- its agent // kept running across the restart -- so this is skipped for it per // AC-EXECUTORS-SURVIVAL-003.2. // The write is archive-atomic (SetTaskMetadataKeyIfNotArchived), so an // archive that commits after this check cannot leave a stale marker on an - // archived task. The marker is cleared when a session of the task next - // enters STARTING/RUNNING (see updateTaskSessionStateWithHook). + // archived task. The marker is cleared after the provider confirms recovery + // (see markRecoveryResolved). if !retracked && running.TaskID != "" && (previousState == models.TaskSessionStateStarting || previousState == models.TaskSessionStateRunning) { - if _, setErr := s.repo.SetTaskMetadataKeyIfNotArchived(ctx, running.TaskID, models.MetaKeyInterruptedAt, time.Now().UTC().Format(time.RFC3339)); setErr != nil { + changed, setErr := s.repo.SetTaskMetadataKeyIfNotArchived(ctx, running.TaskID, models.MetaKeyInterruptedAt, time.Now().UTC().Format(time.RFC3339)) + if setErr != nil { s.logger.Warn("failed to mark task interrupted on startup", zap.String("task_id", running.TaskID), zap.Error(setErr)) + } else if changed { + // Startup reconciliation writes the marker directly because it runs + // before the task service's periodic sweep. Publish the committed + // task projection so already-connected clients show the warning + // without waiting for a reload or unrelated task update. + task, taskErr := s.repo.GetTask(ctx, running.TaskID) + if taskErr != nil || task == nil { + s.logger.Warn("failed to load task after startup interruption marker", + zap.String("task_id", running.TaskID), + zap.Error(taskErr)) + } else { + s.publishTaskUpdated(ctx, task) + } } } diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 96628cb27e..5aa2bfca7e 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -77,6 +77,18 @@ const resumeReasonFailedSessionResumable = "failed_session_resumable" // instead of staying stuck on read-only workspace restore. const resumeReasonArchiveCancelledResumable = "archive_cancelled_resumable" +// resumeReasonOrphanCancelledResumable is the resume reason returned when the +// session reconciliation sweep cancelled an execution-less session. Existing +// rows with this exact system reason remain eligible for same-session recovery; +// explicit user/coordinator stops continue through the terminal-state path. +const resumeReasonOrphanCancelledResumable = "orphan_cancelled_resumable" + +// resumeReasonInterruptedSessionResumable is returned for a session that the +// restart/stall sweep settled to WAITING_FOR_INPUT after execution loss. The +// durable session marker keeps this shape resumable even when the executor row +// was removed before the sweep ran. +const resumeReasonInterruptedSessionResumable = "interrupted_session_resumable" + // resumeReasonTaskArchived tells clients that recovery is unavailable until // the owning task is unarchived. It is intentionally distinct from a runtime // failure so archived history never enters recovery UI. @@ -2975,6 +2987,7 @@ func (s *Service) resumeTaskSessionWithContinuation( if (err != nil || running == nil) && session.State != models.TaskSessionStateCancelled && session.State != models.TaskSessionStateFailed && + !models.HasInterruptedRecoveryPending(session.Metadata) && !allowCompletedResume { // Executor record is required for non-terminal sessions. For cancelled/failed sessions // the record may already have been cleaned up before the user clicked Resume — allow it. @@ -4152,6 +4165,8 @@ func (s *Service) GetTaskSessionStatus(ctx context.Context, taskID, sessionID st resp.AgentProfileID = session.AgentProfileID resp.AutoResumeAllowed, resp.AutoResumeBlockedReason = s.autoResumeEligibility(ctx, task, session) if task.ArchivedAt != nil { + resp.AutoResumeAllowed = false + resp.AutoResumeBlockedReason = resumeReasonTaskArchived resp.ResumeReason = resumeReasonTaskArchived return resp, nil } @@ -4231,6 +4246,21 @@ func (s *Service) GetTaskSessionStatus(ctx context.Context, taskID, sessionID st // (needs_resume && is_resumable) can never satisfy. return evaluateFreshStartResume(session, running, runErr, resp), nil } + // The active-session reconciliation sweep used the exact orphan reason + // when it cancelled execution-less sessions. Treat those legacy rows like + // archive cancellations so opening the task restores the same conversation + // without replaying its prior task prompt. Other CANCELLED rows remain + // explicit user/coordinator stops and stay on workspace restore. + if isOrphanCancelledSession(session) { + if running != nil && running.Resumable { + out := s.validateResumeEligibility(session, resp) + if out.NeedsResume { + out.ResumeReason = resumeReasonOrphanCancelledResumable + } + return out, nil + } + return evaluateFreshStartResume(session, running, runErr, resp), nil + } // Don't auto-resume other terminal sessions (CANCELLED stays stopped, COMPLETED is done). if !isActiveSessionState(session.State) { resp.IsAgentRunning = false @@ -4343,6 +4373,16 @@ func evaluateFreshStartResume(session *models.TaskSession, running *models.Execu resp.ResumeReason = "agent_not_running_fresh_start" return resp } + if running == nil && + (session.State == models.TaskSessionStateWaitingForInput || session.State == models.TaskSessionStateStarting || session.State == models.TaskSessionStateRunning) && + (runErr == nil || errors.Is(runErr, models.ErrExecutorRunningNotFound)) && + models.HasInterruptedRecoveryPending(session.Metadata) { + resp.IsAgentRunning = false + resp.IsResumable = true + resp.NeedsResume = true + resp.ResumeReason = resumeReasonInterruptedSessionResumable + return resp + } // The archive cleanup (Service.ArchiveTask / HandoffService cascade) tears // down the ExecutorRunning row entirely, so an archive-cancelled session // reaches this function with running == nil — exactly the shape an @@ -4355,6 +4395,13 @@ func evaluateFreshStartResume(session *models.TaskSession, running *models.Execu resp.ResumeReason = resumeReasonArchiveCancelledResumable return resp } + if isOrphanCancelledSession(session) { + resp.IsAgentRunning = false + resp.IsResumable = true + resp.NeedsResume = true + resp.ResumeReason = resumeReasonOrphanCancelledResumable + return resp + } resp.IsAgentRunning = false resp.IsResumable = false resp.NeedsResume = false @@ -4492,6 +4539,15 @@ func isArchiveCancelledSession(session *models.TaskSession) bool { models.IsArchiveCancelReason(session.ErrorMessage) } +// isOrphanCancelledSession reports the exact legacy cancellation marker used +// by session reconciliation. Matching the reason exactly keeps explicit user +// stops and unrelated terminal states out of automatic recovery. +func isOrphanCancelledSession(session *models.TaskSession) bool { + return session != nil && + session.State == models.TaskSessionStateCancelled && + models.IsOrphanCancelReason(session.ErrorMessage) +} + func shouldHealStuckStartingSession(session *models.TaskSession, running *models.ExecutorRunning) bool { if session == nil || running == nil { return false diff --git a/apps/backend/internal/orchestrator/task_operations_orphan_resume_test.go b/apps/backend/internal/orchestrator/task_operations_orphan_resume_test.go new file mode 100644 index 0000000000..826120d5d9 --- /dev/null +++ b/apps/backend/internal/orchestrator/task_operations_orphan_resume_test.go @@ -0,0 +1,219 @@ +package orchestrator + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/orchestrator/dto" + "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" +) + +type orphanStatusCase struct { + name string + profileID string + reason string + archive bool + deferred bool + live bool + running bool + resumable bool + resumeToken string + wantNeedsResume bool + wantResumable bool + wantAgentRunning bool + wantAutoResumeAllowed bool + wantBlockReason string + wantResumeReason string + wantError string +} + +func TestGetTaskSessionStatus_OrphanCancelledRecoveryGuards(t *testing.T) { + for _, tc := range orphanStatusCases() { + t.Run(tc.name, func(t *testing.T) { runOrphanStatusCase(t, tc) }) + } +} + +func orphanStatusCases() []orphanStatusCase { + return []orphanStatusCase{ + { + name: "missing runtime row remains recoverable", profileID: "profile1", + reason: models.SessionOrphanedCancelReason, wantNeedsResume: true, + wantResumable: true, wantAutoResumeAllowed: true, + wantResumeReason: resumeReasonOrphanCancelledResumable, + }, + { + name: "non-resumable runtime falls back to fresh start", profileID: "profile1", + reason: models.SessionOrphanedCancelReason, running: true, + resumeToken: "acp-session-123", wantNeedsResume: true, wantResumable: true, + wantAutoResumeAllowed: true, wantResumeReason: resumeReasonOrphanCancelledResumable, + }, + { + name: "missing profile reports actionable error", reason: models.SessionOrphanedCancelReason, + running: true, resumable: true, resumeToken: "acp-session-123", + wantAutoResumeAllowed: true, wantError: "session missing agent profile", + }, + { + name: "explicit stop stays terminal", profileID: "profile1", + reason: "user stopped the session", running: true, resumable: true, + resumeToken: "acp-session-123", wantAutoResumeAllowed: true, + }, + { + name: "archived task blocks recovery", profileID: "profile1", + reason: models.SessionOrphanedCancelReason, archive: true, running: true, + resumable: true, resumeToken: "acp-session-123", + wantBlockReason: resumeReasonTaskArchived, wantResumeReason: resumeReasonTaskArchived, + }, + { + name: "deferred owner blocks passive recovery", profileID: "profile1", + reason: models.SessionOrphanedCancelReason, deferred: true, running: true, + resumable: true, resumeToken: "acp-session-123", wantNeedsResume: true, + wantResumable: true, wantBlockReason: autoResumeBlockedLaunchQueued, + wantResumeReason: resumeReasonOrphanCancelledResumable, + }, + { + name: "live execution wins over recovery", profileID: "profile1", + reason: models.SessionOrphanedCancelReason, live: true, running: true, + resumable: true, resumeToken: "acp-session-123", wantResumable: true, + wantAgentRunning: true, wantAutoResumeAllowed: true, + }, + } +} + +func runOrphanStatusCase(t *testing.T, tc orphanStatusCase) { + t.Helper() + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCancelled) + setOrphanStatusSession(t, repo, tc) + if tc.deferred { + setOrphanStatusDeferral(t, repo) + } + if tc.running { + setOrphanStatusRuntime(t, repo, tc) + } + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + if tc.live { + agentMgr.isAgentRunningFn = func(context.Context, string) bool { return true } + } + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + resp, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("GetTaskSessionStatus: %v", err) + } + assertOrphanStatus(t, resp, tc) +} + +func setOrphanStatusSession(t *testing.T, repo *sqliterepo.Repository, tc orphanStatusCase) { + t.Helper() + ctx := context.Background() + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("load session: %v", err) + } + session.AgentProfileID = tc.profileID + session.ErrorMessage = tc.reason + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + if !tc.archive { + return + } + if err := repo.ArchiveTask(ctx, "task1"); err != nil { + t.Fatalf("archive task: %v", err) + } + session, err = repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("reload archived session: %v", err) + } + session.AgentProfileID = tc.profileID + session.ErrorMessage = tc.reason + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("restore archived session marker: %v", err) + } +} + +func setOrphanStatusDeferral(t *testing.T, repo *sqliterepo.Repository) { + t.Helper() + deferred := models.CeilingRecordKeys(models.CeilingDeferral{ + Kind: models.CeilingLaunchStartCreated, + Payload: map[string]interface{}{ + metaKeySessionID: "session1", + metaKeyAgentProfileID: "profile1", + }, + }) + if err := repo.SetTaskMetadataKey(context.Background(), "task1", models.MetaKeyDeferredLaunch, deferred); err != nil { + t.Fatalf("set deferred launch: %v", err) + } +} + +func setOrphanStatusRuntime(t *testing.T, repo *sqliterepo.Repository, tc orphanStatusCase) { + t.Helper() + now := time.Now().UTC() + if err := repo.UpsertExecutorRunning(context.Background(), &models.ExecutorRunning{ + ID: "running1", SessionID: "session1", TaskID: "task1", Status: "ready", + Resumable: tc.resumable, ResumeToken: tc.resumeToken, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert executor running: %v", err) + } +} + +func assertOrphanStatus(t *testing.T, resp dto.TaskSessionStatusResponse, tc orphanStatusCase) { + t.Helper() + if resp.NeedsResume != tc.wantNeedsResume || resp.IsResumable != tc.wantResumable { + t.Fatalf("recovery flags = needs:%t resumable:%t, want needs:%t resumable:%t; response=%+v", + resp.NeedsResume, resp.IsResumable, tc.wantNeedsResume, tc.wantResumable, resp) + } + if resp.IsAgentRunning != tc.wantAgentRunning || resp.AutoResumeAllowed != tc.wantAutoResumeAllowed { + t.Fatalf("runtime flags = running:%t auto:%t, want running:%t auto:%t; response=%+v", + resp.IsAgentRunning, resp.AutoResumeAllowed, tc.wantAgentRunning, tc.wantAutoResumeAllowed, resp) + } + if resp.AutoResumeBlockedReason != tc.wantBlockReason || resp.ResumeReason != tc.wantResumeReason { + t.Fatalf("recovery reasons = blocked:%q resume:%q, want blocked:%q resume:%q; response=%+v", + resp.AutoResumeBlockedReason, resp.ResumeReason, tc.wantBlockReason, tc.wantResumeReason, resp) + } + if resp.Error != tc.wantError { + t.Fatalf("Error = %q, want %q; response=%+v", resp.Error, tc.wantError, resp) + } +} + +func TestGetTaskSessionStatus_OrphanCancelledStatusIsIdempotent(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCancelled) + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("load session: %v", err) + } + session.AgentProfileID = "profile1" + session.ErrorMessage = models.SessionOrphanedCancelReason + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + now := time.Now().UTC() + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "running1", SessionID: "session1", TaskID: "task1", Status: "ready", + Resumable: true, ResumeToken: "acp-session-123", CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert executor running: %v", err) + } + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + first, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("first GetTaskSessionStatus: %v", err) + } + second, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("second GetTaskSessionStatus: %v", err) + } + if first != second { + t.Fatalf("repeated status changed response: first=%+v second=%+v", first, second) + } +} diff --git a/apps/backend/internal/orchestrator/task_operations_resume_test.go b/apps/backend/internal/orchestrator/task_operations_resume_test.go index 67ffd0d0ea..ed0395f344 100644 --- a/apps/backend/internal/orchestrator/task_operations_resume_test.go +++ b/apps/backend/internal/orchestrator/task_operations_resume_test.go @@ -111,6 +111,34 @@ func TestGetTaskSessionStatus_AutoResumesNormalWaitingSession(t *testing.T) { } } +func TestGetTaskSessionStatus_RecoversSweptSessionWithoutExecutorRow(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateWaitingForInput) + if err := repo.SetSessionMetadataKey(ctx, "session1", models.SessionMetaKeyInterruptedRecoveryPending, "recovery-token"); err != nil { + t.Fatalf("set interrupted recovery marker: %v", err) + } + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("load session: %v", err) + } + session.AgentProfileID = "profile1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + resp, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil { + t.Fatalf("GetTaskSessionStatus: %v", err) + } + if !resp.NeedsResume || !resp.IsResumable { + t.Fatalf("status = %+v, want resumable lazy recovery", resp) + } +} + func TestAutoResumeEligibilityPreservesDeferredLaunchOwnership(t *testing.T) { queuedRecord := models.CeilingRecordKeys(models.CeilingDeferral{ Kind: models.CeilingLaunchStartCreated, @@ -473,6 +501,63 @@ func TestGetTaskSessionStatus_ArchiveCancelledSessionWithTokenAutoResumes(t *tes } } +// TestGetTaskSessionStatus_OrphanCancelledSessionAutoResumes verifies that a +// session cancelled by the orphan reconciliation sweep remains recoverable in +// the same conversation. The cancellation reason is exact so an explicit user +// stop cannot accidentally enter this path. +func TestGetTaskSessionStatus_OrphanCancelledSessionAutoResumes(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCancelled) + + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("failed to load session: %v", err) + } + session.AgentProfileID = "profile1" + session.ErrorMessage = models.SessionOrphanedCancelReason + 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{repoForExecutionLookup: repo} + 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 for orphan-cancelled session") + } + if !resp.IsResumable { + t.Fatal("expected IsResumable=true for orphan-cancelled session") + } + if resp.NeedsWorkspaceRestore { + t.Fatal("expected NeedsWorkspaceRestore=false when auto-resuming") + } + if resp.ResumeReason != resumeReasonOrphanCancelledResumable { + t.Fatalf("expected ResumeReason=%q, got %q", resumeReasonOrphanCancelledResumable, resp.ResumeReason) + } +} + // TestGetTaskSessionStatus_ArchiveCancelledSessionWithNonResumableTokenFreshStarts // covers the CodeRabbit-flagged gap: an archive-cancelled session can carry a // resume token whose ExecutorRunning row has Resumable=false (the runtime @@ -773,6 +858,98 @@ func TestResumeTaskSession_WaitsForPromptReady(t *testing.T) { } } +func TestResumeTaskSession_RecoversSweptSessionWithoutExecutorRow(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateRunning) + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("load session: %v", err) + } + session.AgentProfileID = "profile1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + // Use the same guarded transition as the active/restart sweep. This + // intentionally has no executors_running row. + recovered, err := repo.RecoverTaskSessionByCandidate(ctx, models.ActiveSessionRecoveryCandidate{ + TaskID: session.TaskID, SessionID: session.ID, ExpectedState: session.State, + ExpectedUpdatedAt: session.UpdatedAt, ExpectedLastEventAt: session.UpdatedAt, + }, time.Time{}) + if err != nil || recovered == nil { + t.Fatalf("recover swept session = %+v, err=%v", recovered, err) + } + + ready := make(chan struct{}) + checked := make(chan struct{}, 1) + pollCtx, cancelPoll := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancelPoll) + agentMgr := &mockAgentManager{ + isAgentRunning: false, + repoForExecutionLookup: repo, + isAgentReadyFn: func(_ context.Context, _ string) bool { + select { + case checked <- struct{}{}: + default: + } + select { + case <-ready: + return true + default: + return false + } + }, + launchAgentFunc: func(_ context.Context, req *executor.LaunchAgentRequest) (*executor.LaunchAgentResponse, error) { + go func(sessionID string) { + for { + select { + case <-pollCtx.Done(): + return + default: + } + sess, getErr := repo.GetTaskSession(context.Background(), sessionID) + if getErr == nil && sess != nil && sess.State == models.TaskSessionStateStarting { + sess.State = models.TaskSessionStateWaitingForInput + sess.UpdatedAt = time.Now().UTC() + _ = repo.UpdateTaskSession(context.Background(), sess) + return + } + time.Sleep(time.Millisecond) + } + }(req.SessionID) + return &executor.LaunchAgentResponse{AgentExecutionID: "exec-swept-resume"}, nil + }, + } + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateInProgress} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, agentMgr) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + status, err := svc.GetTaskSessionStatus(ctx, "task1", "session1") + if err != nil || !status.NeedsResume || !status.IsResumable { + t.Fatalf("status after sweep = %+v, err=%v, want focus recovery", status, err) + } + done := make(chan error, 1) + go func() { + _, resumeErr := svc.ResumeTaskSession(ctx, "task1", "session1") + done <- resumeErr + }() + select { + case <-checked: + case <-time.After(3 * time.Second): + t.Fatal("ResumeTaskSession did not reach prompt readiness") + } + close(ready) + select { + case err := <-done: + if err != nil { + t.Fatalf("ResumeTaskSession after sweep: %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("ResumeTaskSession did not complete") + } +} + func TestResumeTaskSession_ReapsPromptDeadExecutionAndRelaunches(t *testing.T) { oldReadyTimeout := agentPromptReadyTimeout oldReadyInterval := agentPromptReadyInterval diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 404a58bd77..9e4537502a 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -178,11 +178,10 @@ const ( // because the winner will (or already did) handle it. // Absent on ordinary (non-watcher) auto-start tasks, which launch normally. MetaKeyAutoStartClaimed = "auto_start_claimed" - // MetaKeyInterruptedAt is set by startup reconciliation when a task's - // session was mid-turn (STARTING/RUNNING) when the backend died. Its - // presence makes the task DTO report `interrupted: true` so task-list - // surfaces show the red interruption icon; the orchestrator removes the - // key when a session of the task next enters STARTING/RUNNING. + // MetaKeyInterruptedAt is set by reconciliation when a task's session lost + // an active turn. Its presence makes the task DTO report `interrupted: true` + // so task-list surfaces show the warning indicator. The orchestrator removes + // the key only after the provider confirms recovery. MetaKeyInterruptedAt = "interrupted_at" // MetaKeyAutoStartFailed is set when a workflow step's auto_start_agent // on_enter action fails to launch a run (kanban StartTask error, or an @@ -400,6 +399,14 @@ const ( // latest successful agent boot. Recovery cards compare this timestamp with // their own creation time, so the result survives transcript write failures. SessionMetaKeyRecoveryResolvedAt = "recovery_resolved_at" + // SessionMetaKeyInterruptedRecoveryPending is a durable token written by + // session reconciliation when execution loss returns a conversation to + // WAITING_FOR_INPUT. It distinguishes an interrupted waiting session from + // an ordinary idle conversation even when the executor row was removed. + SessionMetaKeyInterruptedRecoveryPending = "interrupted_recovery_pending" + // SessionMetaKeyRecoverySettlementPending carries the immutable recovery + // snapshot needed to retry post-commit effects after a partial failure. + SessionMetaKeyRecoverySettlementPending = "recovery_settlement_pending" // SessionMetaKeyInitialCreatePromptPassthrough stores the durable // execution and turn evidence for a creation prompt admitted before a // passthrough agent.running event. It prevents recovery from evaluating @@ -407,6 +414,69 @@ const ( SessionMetaKeyInitialCreatePromptPassthrough = "initial_create_prompt_passthrough" ) +// InterruptedRecoverySettlement is the durable recovery snapshot used when a +// session transition committed but one of its cross-repository effects did +// not. The executor identity and turn ID are compare-and-set inputs; they +// must never be refreshed from a successor execution during retry. +type InterruptedRecoverySettlement struct { + Token string `json:"token"` + ExpectedState TaskSessionState `json:"expected_state"` + // RecoveredUpdatedAt is the session generation written by the recovery + // transition. Retries must compare against this immutable timestamp rather + // than treating a later session read as the recovery generation. + RecoveredUpdatedAt time.Time `json:"recovered_updated_at"` + ExpectedTurnID string `json:"expected_turn_id,omitempty"` + ExpectedExecutorID string `json:"expected_executor_id,omitempty"` + ExpectedExecutorAgentExecutionID string `json:"expected_executor_agent_execution_id,omitempty"` + ExpectedExecutorUpdatedAt time.Time `json:"expected_executor_updated_at,omitempty"` +} + +// HasInterruptedRecoveryPending reports whether metadata carries a valid +// interrupted-session token. Older in-process callers used a boolean marker, +// so explicit true remains accepted during the transition to tokens. +func HasInterruptedRecoveryPending(metadata map[string]interface{}) bool { + if metadata == nil { + return false + } + value, ok := metadata[SessionMetaKeyInterruptedRecoveryPending] + if !ok || value == nil { + return false + } + switch typed := value.(type) { + case bool: + return typed + case string: + return strings.TrimSpace(typed) != "" + default: + return true + } +} + +// LoadInterruptedRecoverySettlement decodes the immutable post-commit +// snapshot. Invalid or incomplete values fail closed so a retry cannot repair +// a successor executor or abandon an unrelated turn. +func LoadInterruptedRecoverySettlement(metadata map[string]interface{}) (InterruptedRecoverySettlement, bool) { + if metadata == nil { + return InterruptedRecoverySettlement{}, false + } + value, ok := metadata[SessionMetaKeyRecoverySettlementPending] + if !ok || value == nil { + return InterruptedRecoverySettlement{}, false + } + payload, err := json.Marshal(value) + if err != nil { + return InterruptedRecoverySettlement{}, false + } + var settlement InterruptedRecoverySettlement + if err := json.Unmarshal(payload, &settlement); err != nil { + return InterruptedRecoverySettlement{}, false + } + if strings.TrimSpace(settlement.Token) == "" || settlement.ExpectedState == "" || settlement.RecoveredUpdatedAt.IsZero() { + return InterruptedRecoverySettlement{}, false + } + return settlement, true +} + // IsCompletionFollowUpSession reports whether a session was explicitly // reopened only to continue its completed conversation. The marker remains // meaningful after the session settles back to WAITING_FOR_INPUT. @@ -1903,6 +1973,27 @@ type ActiveSessionCancellationCandidate struct { ExpectedTurnID string } +// ActiveSessionRecoveryCandidate is the compare-and-set snapshot used by +// session reconciliation when execution loss should return a conversation to +// WAITING_FOR_INPUT. The observed state, activity clock, and turn identity +// must still match at the write boundary; a newer launch, message, or turn +// therefore owns the session and prevents stale recovery. +type ActiveSessionRecoveryCandidate struct { + TaskID string + SessionID string + ExpectedState TaskSessionState + ExpectedUpdatedAt time.Time + // RecoveredUpdatedAt is populated from the durable settlement on retry. + // The initial candidate leaves it zero because the repository assigns the + // committed generation when it performs the state transition. + RecoveredUpdatedAt time.Time + ExpectedLastEventAt time.Time + ExpectedTurnID string + ExpectedExecutorID string + ExpectedExecutorAgentExecutionID string + ExpectedExecutorUpdatedAt time.Time +} + // ToAPI converts internal TaskSession to API type // TODO: Add v1.TaskSession type to pkg/api/v1/ func (s *TaskSession) ToAPI() map[string]interface{} { diff --git a/apps/backend/internal/task/models/resume_safety.go b/apps/backend/internal/task/models/resume_safety.go index 5efc7da3d8..84735c32fc 100644 --- a/apps/backend/internal/task/models/resume_safety.go +++ b/apps/backend/internal/task/models/resume_safety.go @@ -79,15 +79,23 @@ const ( SessionArchiveTreeCancelReason = "task tree archived" ) -// SessionOrphanedCancelReason is the TaskSession.ErrorMessage value written -// when the session reconciliation sweep cancels an active session that no -// longer has a live execution behind it (e.g. after a backend restart) and -// has been event-silent beyond the sweep's grace window. It is deliberately -// not an archive reason: unarchived tasks must treat the cancelled session -// like an explicit stop, not like archive cancellation awaiting unarchive. +// SessionOrphanedCancelReason is the exact TaskSession.ErrorMessage value +// written when the session reconciliation sweep cancels an active session that +// no longer has a live execution behind it (for example, after a backend +// restart) and has been event-silent beyond the sweep's grace window. It is +// deliberately distinct from archive and explicit-stop reasons: legacy rows +// with this marker remain eligible for same-session recovery when the user +// opens the task. const SessionOrphanedCancelReason = "orphaned session" // IsArchiveCancelReason reports whether reason came from an archive path. func IsArchiveCancelReason(reason string) bool { return reason == SessionArchiveCancelReason || reason == SessionArchiveTreeCancelReason } + +// IsOrphanCancelReason reports whether reason is the exact marker emitted by +// session reconciliation for an execution-less interruption. Callers must not +// broaden this match: explicit user stops retain their terminal recovery rules. +func IsOrphanCancelReason(reason string) bool { + return reason == SessionOrphanedCancelReason +} diff --git a/apps/backend/internal/task/models/resume_safety_test.go b/apps/backend/internal/task/models/resume_safety_test.go index 30960d71ce..6e51cdeb32 100644 --- a/apps/backend/internal/task/models/resume_safety_test.go +++ b/apps/backend/internal/task/models/resume_safety_test.go @@ -54,3 +54,20 @@ func TestRowMustBePreserved(t *testing.T) { }) } } + +func TestIsOrphanCancelReasonRequiresExactSystemMarker(t *testing.T) { + if !IsOrphanCancelReason(SessionOrphanedCancelReason) { + t.Fatal("the reconciliation marker must be recognized") + } + for _, reason := range []string{ + "", + "orphaned session ", + "ORPHANED SESSION", + SessionArchiveCancelReason, + "user stopped", + } { + if IsOrphanCancelReason(reason) { + t.Fatalf("reason %q must not be treated as an orphan cancellation", reason) + } + } +} diff --git a/apps/backend/internal/task/repository/sqlite/session.go b/apps/backend/internal/task/repository/sqlite/session.go index b0ba83d03e..c2464b93fe 100644 --- a/apps/backend/internal/task/repository/sqlite/session.go +++ b/apps/backend/internal/task/repository/sqlite/session.go @@ -2240,6 +2240,163 @@ func (r *Repository) CancelRunningTaskSessionByID(ctx context.Context, sessionID return session, rows.Err() } +// RecoverTaskSessionByCandidate returns one execution-less session to +// WAITING_FOR_INPUT when its state, activity clock, and current turn still +// match the reconciliation snapshot. The optional staleBefore cutoff is used +// by the restart pass to preserve its launch grace window; a zero cutoff is +// used by the active-task stall pass. +// +// The write is session-scoped and compare-and-set guarded. A newer message, +// state transition, successor turn, or refreshed launch causes the UPDATE to +// match no row, leaving the current owner untouched for the next sweep. +// +//nolint:cyclop,funlen // The SQL predicate mirrors the complete recovery CAS contract. +func (r *Repository) RecoverTaskSessionByCandidate( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, + staleBefore time.Time, +) (*models.TaskSession, error) { + if candidate.SessionID == "" || candidate.TaskID == "" { + return nil, nil + } + switch candidate.ExpectedState { + case models.TaskSessionStateCreated, + models.TaskSessionStateStarting, + models.TaskSessionStateRunning, + models.TaskSessionStateWaitingForInput: + default: + return nil, nil + } + // PostgreSQL timestamp columns retain microsecond precision. Use the same + // precision in the JSON settlement snapshot so a retry compares the + // stored generation exactly on both database dialects. + now := time.Now().UTC().Truncate(time.Microsecond) + writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) + defer cancel() + + // Preserve a durable recovery token and immutable effect snapshot in the + // same compare-and-set write as the WAITING_FOR_INPUT transition. A later + // focus request can therefore distinguish this recovered conversation from + // an ordinary idle session even when executors_running has no row, and a + // partial post-commit settlement can retry without inspecting a successor. + settlementPending := candidate.ExpectedTurnID != "" || + candidate.ExpectedState == models.TaskSessionStateCreated || + candidate.ExpectedState == models.TaskSessionStateStarting || + candidate.ExpectedState == models.TaskSessionStateRunning + setClause := "state = ?, error_message = ?, completed_at = ?, updated_at = ?" + query := ` + UPDATE task_sessions + SET ` + args := make([]interface{}, 0, 16) + if settlementPending { + token := interruptedRecoveryToken(candidate) + settlement := models.InterruptedRecoverySettlement{ + Token: token, + ExpectedState: candidate.ExpectedState, + RecoveredUpdatedAt: now, + ExpectedTurnID: candidate.ExpectedTurnID, + ExpectedExecutorID: candidate.ExpectedExecutorID, + ExpectedExecutorAgentExecutionID: candidate.ExpectedExecutorAgentExecutionID, + ExpectedExecutorUpdatedAt: candidate.ExpectedExecutorUpdatedAt, + } + pendingJSON, err := json.Marshal(token) + if err != nil { + return nil, err + } + settlementJSON, err := json.Marshal(settlement) + if err != nil { + return nil, err + } + if dialect.IsPostgres(r.db.DriverName()) { + base := postgresMetadataObject + setClause = "metadata = jsonb_set(jsonb_set(" + base + ", ARRAY[?]::text[], ?::jsonb, true), ARRAY[?]::text[], ?::jsonb, true)::text, " + setClause + args = append(args, + models.SessionMetaKeyInterruptedRecoveryPending, string(pendingJSON), + models.SessionMetaKeyRecoverySettlementPending, string(settlementJSON), + ) + } else { + base := sqliteMetadataObject + setClause = "metadata = json_set(json_set(" + base + ", ?, json(?)), ?, json(?)), " + setClause + args = append(args, + jsonPath(models.SessionMetaKeyInterruptedRecoveryPending), string(pendingJSON), + jsonPath(models.SessionMetaKeyRecoverySettlementPending), string(settlementJSON), + ) + } + } + query += setClause + ` + WHERE id = ? + AND task_id = ? + AND EXISTS ( + SELECT 1 FROM tasks recoverable_task + WHERE recoverable_task.id = task_sessions.task_id + AND recoverable_task.archived_at IS NULL + ) + AND state = ? + AND updated_at = ? + ` + args = append(args, + string(models.TaskSessionStateWaitingForInput), "", nil, now, + candidate.SessionID, candidate.TaskID, string(candidate.ExpectedState), candidate.ExpectedUpdatedAt, + ) + if !staleBefore.IsZero() { + query += " AND updated_at < ?\n" + args = append(args, staleBefore) + } + if !candidate.ExpectedLastEventAt.IsZero() { + query += ` + AND NOT EXISTS ( + SELECT 1 FROM task_session_messages newer_message + WHERE newer_message.task_session_id = task_sessions.id + AND newer_message.updated_at > ? + ) + ` + args = append(args, candidate.ExpectedLastEventAt) + } + if candidate.ExpectedTurnID == "" { + query += ` + AND NOT EXISTS ( + SELECT 1 FROM task_session_turns active_turn + WHERE active_turn.task_session_id = task_sessions.id + AND active_turn.completed_at IS NULL + ) + ` + } else { + query += ` + AND EXISTS ( + SELECT 1 FROM task_session_turns active_turn + WHERE active_turn.id = ? + AND active_turn.task_session_id = task_sessions.id + AND active_turn.completed_at IS NULL + ) + ` + args = append(args, candidate.ExpectedTurnID) + } + query += ` + RETURNING id, agent_profile_id, agent_profile_snapshot, is_passthrough, name, + review_status, metadata, task_environment_id, state, updated_at, is_primary + ` + rows, err := r.db.QueryContext(writeCtx, r.db.Rebind(query), args...) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + if !rows.Next() { + return nil, rows.Err() + } + session, err := scanCancelledTaskSessionRow(rows, candidate.TaskID) + if err != nil { + return nil, err + } + return session, rows.Err() +} + +func interruptedRecoveryToken(candidate models.ActiveSessionRecoveryCandidate) string { + return fmt.Sprintf("%s:%s:%s:%s", candidate.SessionID, + candidate.ExpectedState, + candidate.ExpectedUpdatedAt.UTC().Format(time.RFC3339Nano), + candidate.ExpectedTurnID) +} + // CancelActiveTaskSessionsByIDs is documented on the SessionRepository // interface. It shares CancelActiveTaskSessionsByTaskID's atomic // UPDATE ... RETURNING shape and detached-but-bounded write context, so the @@ -2846,6 +3003,52 @@ func (r *Repository) RemoveSessionMetadataKeyIfStamp( return rows > 0, err } +// RemoveSessionMetadataKeyIfJSONValue removes one metadata key only when its +// complete JSON value still equals expectedValue. Recovery settlement uses +// this compare-and-set boundary so a delayed effect cannot erase a newer +// recovery snapshot written after a successor launch. +func (r *Repository) RemoveSessionMetadataKeyIfJSONValue( + ctx context.Context, + sessionID, key string, + expectedValue interface{}, +) (bool, error) { + payload, err := json.Marshal(expectedValue) + if err != nil { + return false, fmt.Errorf("failed to serialize expected session metadata: %w", err) + } + now := time.Now().UTC() + driver := r.db.DriverName() + var query string + var args []interface{} + if dialect.IsPostgres(driver) { + query = ` + UPDATE task_sessions + SET metadata = ( + CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END + #- ARRAY[?]::text[] + )::text, updated_at = ? + WHERE id = ? + AND (CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END -> ?) = ?::jsonb + ` + args = []interface{}{key, now, sessionID, key, string(payload)} + } else { + path := jsonPath(key) + query = ` + UPDATE task_sessions + SET metadata = json_remove(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?), updated_at = ? + WHERE id = ? + AND json_extract(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?) = json(?) + ` + args = []interface{}{path, now, sessionID, path, string(payload)} + } + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + return rows > 0, err +} + func setSessionMetadataKeyIfAbsentQuery(driver string) string { if dialect.IsPostgres(driver) { return ` diff --git a/apps/backend/internal/task/repository/sqlite/session_test.go b/apps/backend/internal/task/repository/sqlite/session_test.go index 1c60c61d60..3452f2fc73 100644 --- a/apps/backend/internal/task/repository/sqlite/session_test.go +++ b/apps/backend/internal/task/repository/sqlite/session_test.go @@ -2286,6 +2286,99 @@ func TestCancelActiveTaskSessionsByCandidatesRejectsNewActivityAndTurn(t *testin } } +func TestRecoverTaskSessionByCandidatePreservesConversationAndGuardsTurn(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + seedRepoLink(t, repo, "ws-recover", "repo-recover", "task-recover", "session-recover", "RUNNING") + seedRepoLink(t, repo, "ws-recover", "repo-recover-2", "task-recover-2", "session-recover-2", "RUNNING") + seedRepoLink(t, repo, "ws-recover", "repo-recover-created", "task-recover-created", "session-recover-created", "CREATED") + + created, err := repo.GetTaskSession(ctx, "session-recover-created") + if err != nil { + t.Fatalf("GetTaskSession created candidate: %v", err) + } + recovered, err := repo.RecoverTaskSessionByCandidate(ctx, models.ActiveSessionRecoveryCandidate{ + TaskID: created.TaskID, + SessionID: created.ID, + ExpectedState: created.State, + ExpectedUpdatedAt: created.UpdatedAt, + ExpectedLastEventAt: created.UpdatedAt, + }, time.Now().UTC().Add(time.Minute)) + if err != nil { + t.Fatalf("RecoverTaskSessionByCandidate created session: %v", err) + } + if recovered == nil { + t.Fatal("RecoverTaskSessionByCandidate created session returned nil") + } + if !models.HasInterruptedRecoveryPending(recovered.Metadata) { + t.Fatalf("created recovered metadata = %#v, want durable recovery marker", recovered.Metadata) + } + if _, ok := models.LoadInterruptedRecoverySettlement(recovered.Metadata); !ok { + t.Fatalf("created recovered metadata = %#v, want settlement snapshot", recovered.Metadata) + } + + idleCandidate, err := repo.GetTaskSession(ctx, "session-recover") + if err != nil { + t.Fatalf("GetTaskSession idle candidate: %v", err) + } + // A cutoff in the future admits this stale snapshot. The absence of an + // active turn is part of the candidate predicate for a session whose actor + // died before a turn row was persisted. + recovered, err = repo.RecoverTaskSessionByCandidate(ctx, models.ActiveSessionRecoveryCandidate{ + TaskID: idleCandidate.TaskID, + SessionID: idleCandidate.ID, + ExpectedState: idleCandidate.State, + ExpectedUpdatedAt: idleCandidate.UpdatedAt, + ExpectedLastEventAt: idleCandidate.UpdatedAt, + }, time.Now().UTC().Add(time.Minute)) + if err != nil { + t.Fatalf("RecoverTaskSessionByCandidate without turn: %v", err) + } + if recovered == nil { + t.Fatal("RecoverTaskSessionByCandidate without turn returned nil") + } + if recovered.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("recovered state = %q, want WAITING_FOR_INPUT", recovered.State) + } + if recovered.TaskID != idleCandidate.TaskID { + t.Fatalf("recovered task id = %q, want %q", recovered.TaskID, idleCandidate.TaskID) + } + + turn := &models.Turn{ID: "turn-recover", TaskSessionID: "session-recover-2", TaskID: "task-recover-2"} + if err := repo.CreateTurn(ctx, turn); err != nil { + t.Fatalf("CreateTurn: %v", err) + } + turnCandidate, err := repo.GetTaskSession(ctx, "session-recover-2") + if err != nil { + t.Fatalf("GetTaskSession turn candidate: %v", err) + } + guarded := models.ActiveSessionRecoveryCandidate{ + TaskID: turnCandidate.TaskID, + SessionID: turnCandidate.ID, + ExpectedState: turnCandidate.State, + ExpectedUpdatedAt: turnCandidate.UpdatedAt, + ExpectedLastEventAt: turnCandidate.UpdatedAt, + ExpectedTurnID: turn.ID, + } + // A successor turn refreshes the session and invalidates the observed + // identity before the recovery write. + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: "turn-recover-successor", TaskSessionID: "session-recover-2", TaskID: "task-recover-2", + }); err != nil { + t.Fatalf("Create successor turn: %v", err) + } + recovered, err = repo.RecoverTaskSessionByCandidate(ctx, guarded, time.Time{}) + if err != nil { + t.Fatalf("RecoverTaskSessionByCandidate stale turn: %v", err) + } + if recovered != nil { + t.Fatalf("stale turn candidate recovered session = %+v, want nil", recovered) + } + if got := sessionState(t, repo, "session-recover-2"); got != "RUNNING" { + t.Fatalf("session with successor turn = %q, want RUNNING", got) + } +} + // sameStringSet reports whether got and want contain the same strings, // ignoring order and duplicates count-for-count — used to compare the set of // cancelled session IDs against an expected set regardless of return order. diff --git a/apps/backend/internal/task/repository/sqlite/task.go b/apps/backend/internal/task/repository/sqlite/task.go index 6d621d4d64..901226f04a 100644 --- a/apps/backend/internal/task/repository/sqlite/task.go +++ b/apps/backend/internal/task/repository/sqlite/task.go @@ -1806,6 +1806,43 @@ func (r *Repository) RemoveTaskMetadataKey(ctx context.Context, taskID, key stri return r.removeTaskMetadataKeyWithExecutor(ctx, r.db, taskID, key) } +// RemoveTaskMetadataKeyIfValue removes one metadata key only when its scalar +// JSON value still equals expectedValue. Recovery callbacks use this compare +// and set boundary so a delayed callback cannot erase a newer interruption +// marker written by a later restart. +func (r *Repository) RemoveTaskMetadataKeyIfValue( + ctx context.Context, + taskID, key, expectedValue string, +) (bool, error) { + if strings.TrimSpace(expectedValue) == "" { + return false, nil + } + var query string + var args []interface{} + if dialect.IsPostgres(r.db.DriverName()) { + query = ` + UPDATE tasks + SET metadata = (CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END #- ARRAY[?]::text[])::text, updated_at = ? + WHERE id = ? AND jsonb_extract_path_text(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END, ?) = ? + ` + args = []interface{}{key, time.Now().UTC(), taskID, key, expectedValue} + } else { + path := jsonPath(key) + query = ` + UPDATE tasks + SET metadata = json_remove(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?), updated_at = ? + WHERE id = ? AND json_extract(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?) = ? + ` + args = []interface{}{path, time.Now().UTC(), taskID, path, expectedValue} + } + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + return rows > 0, err +} + // ClearManualMoveLifecycleMarkersIfCompleted atomically removes the pending // and completed markers for a manual move only while the completed marker and // task update generation still match the recovery snapshot. A new move clears @@ -2190,6 +2227,115 @@ func (r *Repository) SetTaskMetadataKeyIfNotArchived(ctx context.Context, taskID return rows > 0, err } +// SetTaskMetadataKeyIfAbsentNotArchived writes one metadata key only when the +// task is live and the key is absent. Recovery marker creation uses this +// compare-and-set boundary so an older settlement cannot overwrite a newer +// interruption generation. +func (r *Repository) SetTaskMetadataKeyIfAbsentNotArchived(ctx context.Context, taskID, key string, value interface{}) (bool, error) { + payload, err := json.Marshal(value) + if err != nil { + return false, err + } + var query string + if dialect.IsPostgres(r.db.DriverName()) { + query = `UPDATE tasks + SET metadata = jsonb_set(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END, ARRAY[?]::text[], ?::jsonb, true)::text, updated_at = ? + WHERE id = ? AND archived_at IS NULL + AND jsonb_extract_path(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}'::jsonb ELSE metadata::jsonb END, ?) IS NULL` + } else { + query = `UPDATE tasks + SET metadata = json_set(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?, json(?)), updated_at = ? + WHERE id = ? AND archived_at IS NULL + AND json_type(CASE WHEN metadata IS NULL OR metadata = 'null' OR metadata = '' THEN '{}' ELSE metadata END, ?) IS NULL` + } + path := key + if !dialect.IsPostgres(r.db.DriverName()) { + path = jsonPath(key) + } + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), path, string(payload), time.Now().UTC(), taskID, path) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + return rows > 0, err +} + +// SetTaskMetadataKeyIfRecoveryCurrent writes a marker only while the session +// that owns the recovery settlement is still waiting at the same generation. +// The settlement token check is deliberately in the same UPDATE as the task +// metadata write: a successor that changes the session state or consumes the +// token cannot be followed by a delayed recovery callback that re-adds the +// interruption warning. +func (r *Repository) SetTaskMetadataKeyIfRecoveryCurrent( + ctx context.Context, + taskID, sessionID string, + expectedSessionUpdatedAt time.Time, + expectedRecoveryToken, key string, + value interface{}, +) (bool, error) { + if taskID == "" || sessionID == "" || expectedRecoveryToken == "" { + return false, nil + } + payload, err := json.Marshal(value) + if err != nil { + return false, err + } + now := time.Now().UTC() + var query string + var args []interface{} + if dialect.IsPostgres(r.db.DriverName()) { + base := postgresMetadataObject + query = `UPDATE tasks + SET metadata = jsonb_set(` + base + `, ARRAY[?]::text[], ?::jsonb, true)::text, updated_at = ? + WHERE id = ? AND archived_at IS NULL + AND jsonb_extract_path(` + base + `, ?) IS NULL + AND EXISTS ( + SELECT 1 FROM task_sessions recovery_session + WHERE recovery_session.id = ? + AND recovery_session.task_id = tasks.id + AND recovery_session.state = ? + AND recovery_session.updated_at = ? + AND jsonb_extract_path_text( + CASE WHEN recovery_session.metadata IS NULL OR recovery_session.metadata = 'null' OR recovery_session.metadata = '' THEN '{}'::jsonb ELSE recovery_session.metadata::jsonb END, + ?, 'token' + ) = ? + )` + args = []interface{}{ + key, string(payload), now, taskID, + key, sessionID, string(models.TaskSessionStateWaitingForInput), expectedSessionUpdatedAt, + models.SessionMetaKeyRecoverySettlementPending, expectedRecoveryToken, + } + } else { + path := jsonPath(key) + markerPath := jsonPath(key) + settlementTokenPath := jsonPath(models.SessionMetaKeyRecoverySettlementPending + ".token") + base := sqliteMetadataObject + query = `UPDATE tasks + SET metadata = json_set(` + base + `, ?, json(?)), updated_at = ? + WHERE id = ? AND archived_at IS NULL + AND json_type(` + base + `, ?) IS NULL + AND EXISTS ( + SELECT 1 FROM task_sessions recovery_session + WHERE recovery_session.id = ? + AND recovery_session.task_id = tasks.id + AND recovery_session.state = ? + AND recovery_session.updated_at = ? + AND json_extract(CASE WHEN recovery_session.metadata IS NULL OR recovery_session.metadata = 'null' OR recovery_session.metadata = '' THEN '{}' ELSE recovery_session.metadata END, ?) = ? + )` + args = []interface{}{ + path, string(payload), now, taskID, + markerPath, sessionID, string(models.TaskSessionStateWaitingForInput), expectedSessionUpdatedAt, + settlementTokenPath, expectedRecoveryToken, + } + } + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + return rows > 0, err +} + // SetTaskMetadataKeyIfPresent rewrites one metadata key only while that key is // still present, and reports whether the write landed. // diff --git a/apps/backend/internal/task/repository/sqlite/task_metadata_cas_test.go b/apps/backend/internal/task/repository/sqlite/task_metadata_cas_test.go index 7790e86e63..6ad9b9919e 100644 --- a/apps/backend/internal/task/repository/sqlite/task_metadata_cas_test.go +++ b/apps/backend/internal/task/repository/sqlite/task_metadata_cas_test.go @@ -144,6 +144,82 @@ func runMetadataNoActiveSessionContract(t *testing.T, repo *Repository) { } } +func runInterruptedMarkerCASContract(t *testing.T, repo *Repository) { + t.Helper() + ctx := context.Background() + if err := repo.SetTaskMetadataKey(ctx, casTaskID, models.MetaKeyInterruptedAt, "old-marker"); err != nil { + t.Fatalf("seed interrupted marker: %v", err) + } + removed, err := repo.RemoveTaskMetadataKeyIfValue(ctx, casTaskID, models.MetaKeyInterruptedAt, "new-marker") + if err != nil { + t.Fatalf("remove with stale marker: %v", err) + } + if removed { + t.Fatal("stale marker comparison must not remove the current value") + } + if value, ok := metadataValue(t, repo, models.MetaKeyInterruptedAt); !ok || value != "old-marker" { + t.Fatalf("marker after stale removal = %v (present=%v), want old-marker", value, ok) + } + removed, err = repo.RemoveTaskMetadataKeyIfValue(ctx, casTaskID, models.MetaKeyInterruptedAt, "old-marker") + if err != nil { + t.Fatalf("remove with current marker: %v", err) + } + if !removed { + t.Fatal("current marker comparison must remove the marker") + } +} + +func runRecoveryMarkerCASContract(t *testing.T, repo *Repository) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + settlement := models.InterruptedRecoverySettlement{ + Token: "recovery-token", + ExpectedState: models.TaskSessionStateRunning, + RecoveredUpdatedAt: now, + } + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-recovery-cas", TaskID: casTaskID, + State: models.TaskSessionStateWaitingForInput, UpdatedAt: now, + Metadata: map[string]interface{}{ + models.SessionMetaKeyRecoverySettlementPending: settlement, + }, + }); err != nil { + t.Fatalf("create recovery session: %v", err) + } + session, err := repo.GetTaskSession(ctx, "session-recovery-cas") + if err != nil { + t.Fatalf("load recovery session: %v", err) + } + written, err := repo.SetTaskMetadataKeyIfRecoveryCurrent( + ctx, casTaskID, session.ID, session.UpdatedAt, settlement.Token, + models.MetaKeyInterruptedAt, "recovery-marker", + ) + if err != nil { + t.Fatalf("write recovery marker: %v", err) + } + if !written { + t.Fatal("current recovery generation must write its marker") + } + if value, ok := metadataValue(t, repo, models.MetaKeyInterruptedAt); !ok || value != "recovery-marker" { + t.Fatalf("recovery marker = %v (present=%v), want recovery-marker", value, ok) + } + + if err := repo.UpdateTaskSessionState(ctx, session.ID, models.TaskSessionStateStarting, ""); err != nil { + t.Fatalf("advance successor session: %v", err) + } + written, err = repo.SetTaskMetadataKeyIfRecoveryCurrent( + ctx, casTaskID, session.ID, session.UpdatedAt, settlement.Token, + "later_marker", "stale-marker", + ) + if err != nil { + t.Fatalf("write stale recovery marker: %v", err) + } + if written { + t.Fatal("a successor session state must block a stale recovery marker") + } +} + func runManualMoveLifecycleMarkerContract(t *testing.T, repo *Repository) { t.Helper() ctx := context.Background() @@ -210,6 +286,8 @@ func TestSetTaskMetadataKeyIfPresentSQLite(t *testing.T) { }) runMetadataCASContract(t, repo) runMetadataNoActiveSessionContract(t, repo) + runInterruptedMarkerCASContract(t, repo) + runRecoveryMarkerCASContract(t, repo) runManualMoveLifecycleMarkerContract(t, repo) } @@ -227,6 +305,8 @@ func TestPostgresSetTaskMetadataKeyIfPresent(t *testing.T) { }) runMetadataCASContract(t, repo) runMetadataNoActiveSessionContract(t, repo) + runInterruptedMarkerCASContract(t, repo) + runRecoveryMarkerCASContract(t, repo) runManualMoveLifecycleMarkerContract(t, repo) } diff --git a/apps/backend/internal/task/service/active_session_stall.go b/apps/backend/internal/task/service/active_session_stall.go index 425cb049c7..0bc0019282 100644 --- a/apps/backend/internal/task/service/active_session_stall.go +++ b/apps/backend/internal/task/service/active_session_stall.go @@ -17,10 +17,10 @@ const ( // an execution-less active session is classified as stalled // (tasks.stallDetectionThreshold defaults to 2h). defaultStallDetectionThreshold = 2 * time.Hour - // stallHealGraceMultiplier derives the orphaned-session healing grace - // window from the stall threshold. Healing waits twice as long as + // stallHealGraceMultiplier derives the interrupted-session recovery grace + // window from the stall threshold. Recovery waits twice as long as // detection so an operator has a full threshold window to act on the - // task.stalled event before the sweep cancels the orphaned session, and + // task.stalled event before the sweep recovers the orphaned session, and // so a live in-flight launch that has not yet registered its execution // never races the grace window. stallHealGraceMultiplier = 2 @@ -46,11 +46,10 @@ func (s *Service) stallThreshold() time.Duration { // // - emits one task.stalled event per stall episode once the session has // been event-silent beyond the stall threshold (detection only), and -// - cancels the session via the same finalizeCancelledSessions transition -// the archived pass uses once the silence exceeds the grace window -// (twice the threshold), making the DB state truthful, delivering the -// session.state_changed event clients key off, and unblocking the task's -// step lifecycle. +// - returns the session to WAITING_FOR_INPUT once the silence exceeds the +// grace window (twice the threshold), preserving the same conversation +// for lazy recovery and delivering the session.state_changed event clients +// key off. // // The execution check is fail-closed: without the registry the pass cannot // prove "no live execution", so it skips rather than healing or alerting on @@ -117,6 +116,18 @@ func (s *Service) sweepTaskSessions( return } + s.retryPendingRecoverySettlements(ctx, task, activeSessions, liveSessions) + orphaned, filterErr := s.excludeIdleWaitingSessions(ctx, orphaned) + if filterErr != nil { + // A turn read failure leaves the session's intent unknown. Do not + // report or recover any part of this task from an incomplete snapshot. + return + } + if len(orphaned) == 0 { + s.clearStallNotifications(task.ID) + return + } + stalled, healable, lastEventBySession, classErr := s.classifyOrphanedSessions( ctx, orphaned, now, threshold, grace, ) @@ -132,6 +143,40 @@ func (s *Service) sweepTaskSessions( s.healOrphanedSessions(ctx, task, activeSessions, orphaned, healable, lastEventBySession) } +// excludeIdleWaitingSessions removes ordinary waiting conversations from the +// execution-loss candidate set. WAITING_FOR_INPUT with no active turn is a +// healthy idle state and must neither emit task.stalled nor participate in +// task-wide healing. A turn read error fails closed for the whole task. +func (s *Service) excludeIdleWaitingSessions( + ctx context.Context, + candidates []*models.TaskSession, +) ([]*models.TaskSession, error) { + filtered := make([]*models.TaskSession, 0, len(candidates)) + for _, session := range candidates { + if session == nil || session.State != models.TaskSessionStateWaitingForInput { + filtered = append(filtered, session) + continue + } + if _, pending := models.LoadInterruptedRecoverySettlement(session.Metadata); pending { + // A prior recovery already owns this waiting row. The retry pass + // above handles its effects; it must not be reclassified as a new + // stall episode while the settlement marker remains. + continue + } + turn, err := s.GetActiveTurn(ctx, session.ID) + if err != nil { + s.logger.Warn("active-session sweep: failed to inspect waiting session turn; skipping task", + zap.String("session_id", session.ID), + zap.Error(err)) + return nil, err + } + if turn != nil { + filtered = append(filtered, session) + } + } + return filtered, nil +} + // classifyOrphanedSessions splits execution-less sessions into stalled // (event-silent beyond threshold) and healable (silent beyond the grace // window), and reports each stalled session's resolved event time for the @@ -170,7 +215,9 @@ func (s *Service) classifyOrphanedSessions( func sessionIDs(sessions []*models.TaskSession) []string { ids := make([]string, 0, len(sessions)) for _, session := range sessions { - ids = append(ids, session.ID) + if session != nil && session.ID != "" { + ids = append(ids, session.ID) + } } return ids } @@ -271,20 +318,18 @@ func (s *Service) notifyStalledSessions( } } -// healOrphanedSessions cancels execution-less sessions that have been silent -// beyond the grace window, reusing the archived pass's -// finalizeCancelledSessions transition (cancellation with the orphaned -// reason, clarification expiry, parked-projection cleanup, ceiling release, -// and the session.state_changed event) so clients keying off that event see -// the session stop. Repeat passes are no-ops: the underlying UPDATE only -// matches rows still in an active state. +// healOrphanedSessions returns execution-less sessions that have been silent +// beyond the grace window to WAITING_FOR_INPUT. It preserves the conversation +// and settles only the turn observed by the compare-and-set snapshot. No +// cancellation effects run: questions, queued work, and workflow ownership +// remain available for lazy recovery. // // Healing is task-scoped at the eligibility boundary, so it only runs when // every active session of the task is execution-less and past the grace // window. The write itself is candidate-scoped, so a same-session resume or a -// newer turn cannot be cancelled by a stale sweep snapshot. +// newer turn cannot be changed by a stale sweep snapshot. // -// The guard is re-evaluated at the cancellation boundary itself: the +// The guard is re-evaluated at the recovery boundary itself: the // liveness snapshot was taken before the silence reads, and an execution // that registered (or a session that started) in between must abort the // heal. Re-checking LiveSessionIDsForTask immediately before the write is @@ -308,7 +353,13 @@ func (s *Service) healOrphanedSessions( zap.Int("live_sessions", len(live))) return } - candidates := make([]models.ActiveSessionCancellationCandidate, 0, len(healable)) + repo, ok := s.sessions.(interface { + RecoverTaskSessionByCandidate(context.Context, models.ActiveSessionRecoveryCandidate, time.Time) (*models.TaskSession, error) + }) + if !ok { + return + } + candidates := make([]models.ActiveSessionRecoveryCandidate, 0, len(healable)) for _, session := range healable { turn, err := s.GetActiveTurn(ctx, session.ID) if err != nil { @@ -322,32 +373,98 @@ func (s *Service) healOrphanedSessions( if lastEvent.IsZero() { lastEvent = session.UpdatedAt } - candidate := models.ActiveSessionCancellationCandidate{ + candidate := models.ActiveSessionRecoveryCandidate{ + TaskID: task.ID, SessionID: session.ID, + ExpectedState: session.State, ExpectedUpdatedAt: session.UpdatedAt, ExpectedLastEventAt: lastEvent, } if turn != nil { candidate.ExpectedTurnID = turn.ID } + if err := s.captureRecoveryExecutorSnapshot(ctx, &candidate); err != nil { + s.logger.Warn("active-session sweep: failed to capture executor reservation; skipping heal", + zap.String("task_id", task.ID), + zap.String("session_id", session.ID), + zap.Error(err)) + return + } candidates = append(candidates, candidate) } - s.logger.Info("active-session sweep: healing orphaned sessions", + s.logger.Info("active-session sweep: preserving interrupted sessions for lazy recovery", zap.String("task_id", task.ID), - zap.Strings("session_ids", cancellationCandidateIDs(candidates))) + zap.Strings("session_ids", recoveryCandidateIDs(candidates))) deadline := archivecascade.ArchiveDeadline(ctx) healCtx, cancel := archivecascade.ContinuationContextUntil(ctx, deadline) defer cancel() - // Candidates scope the cancellation to exactly the sessions this pass - // classified as orphaned and compare their activity/turn identity in the - // UPDATE. A same-session resume or successor turn therefore survives the - // write and is retried by the next sweep. - s.finalizeCancelledSessionCandidates( - healCtx, task.ID, activeSessions, candidates, deadline, models.SessionOrphanedCancelReason, - ) + for _, candidate := range candidates { + if !time.Now().Before(deadline) { + return + } + if _, live := s.liveSessionSet(task.ID)[candidate.SessionID]; live { + continue + } + recovered, err := repo.RecoverTaskSessionByCandidate(healCtx, candidate, time.Time{}) + if err != nil { + s.logger.Warn("active-session sweep: failed to recover interrupted session", + zap.String("task_id", task.ID), + zap.String("session_id", candidate.SessionID), + zap.Error(err)) + continue + } + if recovered == nil { + continue + } + completionCtx, cancelCompletion := context.WithTimeout( + context.WithoutCancel(ctx), taskPublicationTimeout, + ) + s.settleRecoveredSession(completionCtx, candidate, recovered) + cancelCompletion() + } } -func cancellationCandidateIDs(candidates []models.ActiveSessionCancellationCandidate) []string { +// retryPendingRecoverySettlements retries cross-repository effects that did +// not finish during an earlier recovery write. The session metadata carries +// the original turn and executor snapshot, so this pass never reads a +// successor identity merely to repair it. +func (s *Service) retryPendingRecoverySettlements( + ctx context.Context, + task *models.Task, + activeSessions []*models.TaskSession, + liveSessions map[string]struct{}, +) { + if task == nil { + return + } + for _, session := range activeSessions { + if session == nil || session.ID == "" { + continue + } + if _, live := liveSessions[session.ID]; live { + continue + } + settlement, ok := models.LoadInterruptedRecoverySettlement(session.Metadata) + if !ok { + continue + } + candidate := models.ActiveSessionRecoveryCandidate{ + TaskID: task.ID, + SessionID: session.ID, + ExpectedState: settlement.ExpectedState, + RecoveredUpdatedAt: settlement.RecoveredUpdatedAt, + ExpectedTurnID: settlement.ExpectedTurnID, + ExpectedExecutorID: settlement.ExpectedExecutorID, + ExpectedExecutorAgentExecutionID: settlement.ExpectedExecutorAgentExecutionID, + ExpectedExecutorUpdatedAt: settlement.ExpectedExecutorUpdatedAt, + } + completionCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), taskPublicationTimeout) + s.settleRecoveredSession(completionCtx, candidate, session) + cancel() + } +} + +func recoveryCandidateIDs(candidates []models.ActiveSessionRecoveryCandidate) []string { ids := make([]string, 0, len(candidates)) for _, candidate := range candidates { ids = append(ids, candidate.SessionID) diff --git a/apps/backend/internal/task/service/active_session_stall_test.go b/apps/backend/internal/task/service/active_session_stall_test.go index b0b71c8e18..aa02be95ba 100644 --- a/apps/backend/internal/task/service/active_session_stall_test.go +++ b/apps/backend/internal/task/service/active_session_stall_test.go @@ -9,6 +9,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" ) @@ -166,6 +167,120 @@ func TestService_ActiveSessionSweepSkipsRecentSessions(t *testing.T) { } } +// TestService_ActiveSessionSweepPreservesIdleWaitingSession proves that a +// waiting conversation with no unfinished turn is ordinary idle state. It must +// not be reported as stalled or changed by the execution-loss sweep, even +// after the full healing grace window. +func TestService_ActiveSessionSweepPreservesIdleWaitingSession(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + if _, err := fixture.rawRepo.DB().ExecContext(ctx, ` + UPDATE task_sessions + SET state = ?, updated_at = ? + WHERE id = ? + `, string(models.TaskSessionStateWaitingForInput), fixture.now.Add(-5*time.Hour), "session-1"); err != nil { + t.Fatalf("make session idle waiting: %v", err) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + + session, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state = %q, want WAITING_FOR_INPUT", session.State) + } + if stalled := findTaskStalledEvents(fixture.eventBus); len(stalled) != 0 { + t.Fatalf("task.stalled events = %d, want 0 for idle waiting session", len(stalled)) + } +} + +// TestService_ActiveSessionSweepRecoversInterruptedConversation proves that +// a stale execution-less turn returns to the same waiting conversation. The +// turn is settled without a successful completion event so workflow ownership +// and the transcript remain available for a later user message. +func TestService_ActiveSessionSweepRecoversInterruptedConversation(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + turn, err := fixture.svc.StartTurn(ctx, "session-1") + if err != nil { + t.Fatalf("StartTurn: %v", err) + } + old := fixture.now.Add(-5 * time.Hour) + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_session_turns SET started_at = ?, created_at = ?, updated_at = ? WHERE id = ?`, + old, old, old, turn.ID); err != nil { + t.Fatalf("age turn: %v", err) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_sessions SET updated_at = ? WHERE id = ?`, old, "session-1"); err != nil { + t.Fatalf("age session: %v", err) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + + session, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state = %q, want WAITING_FOR_INPUT", session.State) + } + storedTurn, err := fixture.svc.turns.GetTurn(ctx, turn.ID) + if err != nil { + t.Fatalf("GetTurn: %v", err) + } + if storedTurn.CompletedAt == nil || !storedTurn.CompletedAt.Equal(storedTurn.StartedAt) { + t.Fatalf("turn completion = %v, want zero-duration completion at %v", storedTurn.CompletedAt, storedTurn.StartedAt) + } + for _, event := range fixture.eventBus.GetPublishedEvents() { + if event.Type != events.TurnCompleted { + continue + } + if data, ok := event.Data.(map[string]interface{}); ok && data["id"] == turn.ID { + t.Fatal("system recovery published turn.completed and may advance workflow") + } + } +} + +func TestService_ActiveSessionSweepRecoversWaitingSessionWithUnfinishedTurn(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + turn, err := fixture.svc.StartTurn(ctx, "session-1") + if err != nil { + t.Fatalf("StartTurn: %v", err) + } + old := fixture.now.Add(-5 * time.Hour) + if _, err := fixture.rawRepo.DB().ExecContext(ctx, ` + UPDATE task_sessions SET state = ?, updated_at = ? WHERE id = ? + `, string(models.TaskSessionStateWaitingForInput), old, "session-1"); err != nil { + t.Fatalf("age waiting session: %v", err) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_session_turns SET started_at = ?, created_at = ?, updated_at = ? WHERE id = ?`, + old, old, old, turn.ID); err != nil { + t.Fatalf("age turn: %v", err) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + + session, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state = %q, want WAITING_FOR_INPUT", session.State) + } + storedTurn, err := fixture.svc.turns.GetTurn(ctx, turn.ID) + if err != nil { + t.Fatalf("GetTurn: %v", err) + } + if storedTurn.CompletedAt == nil || !storedTurn.CompletedAt.Equal(storedTurn.StartedAt) { + t.Fatalf("turn completion = %v, want zero-duration completion at %v", storedTurn.CompletedAt, storedTurn.StartedAt) + } +} + // TestService_ActiveSessionSweepStallEventEmittedOncePerEpisode proves the // episode dedupe: a sweep ticking every minute must not re-emit task.stalled // for the same stalled session, but must report it again after the episode @@ -191,11 +306,10 @@ func TestService_ActiveSessionSweepStallEventEmittedOncePerEpisode(t *testing.T) } } -// TestService_ActiveSessionSweepHealsOrphanedSessions is the #3711 half of +// TestService_ActiveSessionSweepHealsOrphanedSessions is the recovery half of // the sweep: once an execution-less session has been silent beyond the grace -// window (twice the threshold), the sweep cancels it through the same -// finalizeCancelledSessions transition the archived pass uses, so the DB -// state becomes truthful and the session.state_changed event fires. +// window (twice the threshold), the sweep returns it to WAITING_FOR_INPUT so +// the same conversation remains available for lazy recovery. func TestService_ActiveSessionSweepHealsOrphanedSessions(t *testing.T) { // 5h of silence: past the 4h grace window (2 x 2h threshold). fixture := newSweepFixture(t, 5*time.Hour) @@ -206,11 +320,11 @@ func TestService_ActiveSessionSweepHealsOrphanedSessions(t *testing.T) { if err != nil { t.Fatalf("GetTaskSession: %v", err) } - if session.State != models.TaskSessionStateCancelled { - t.Fatalf("session state after healing = %q, want CANCELLED", session.State) + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state after healing = %q, want WAITING_FOR_INPUT", session.State) } - if session.ErrorMessage != models.SessionOrphanedCancelReason { - t.Fatalf("session error_message = %q, want %q", session.ErrorMessage, models.SessionOrphanedCancelReason) + if session.ErrorMessage != "" { + t.Fatalf("session error_message = %q, want empty", session.ErrorMessage) } var stateChanged *bus.Event @@ -226,8 +340,259 @@ func TestService_ActiveSessionSweepHealsOrphanedSessions(t *testing.T) { t.Fatal("expected a session.state_changed event for session-1 from the healing pass, got none") } data := stateChanged.Data.(map[string]interface{}) - if got := data["new_state"]; got != string(models.TaskSessionStateCancelled) { - t.Errorf("new_state = %v, want CANCELLED", got) + if got := data["new_state"]; got != string(models.TaskSessionStateWaitingForInput) { + t.Errorf("new_state = %v, want WAITING_FOR_INPUT", got) + } +} + +// The active sweep must keep a durable recovery marker on the session when it +// has no executors_running row. The orchestrator uses that marker to offer the +// same conversation for lazy focus recovery after a restart. +func TestService_ActiveSessionSweepMarksMissingExecutorRecovery(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + + session, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if !models.HasInterruptedRecoveryPending(session.Metadata) { + t.Fatalf("session metadata = %#v, want an interrupted recovery marker", session.Metadata) + } + task, err := fixture.rawRepo.GetTask(ctx, "task-1") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if _, ok := task.Metadata[models.MetaKeyInterruptedAt]; !ok { + t.Fatalf("task metadata = %#v, want interrupted marker", task.Metadata) + } + var taskUpdated bool + for _, event := range fixture.eventBus.GetPublishedEvents() { + if event.Type == events.TaskUpdated { + if data, ok := event.Data.(map[string]interface{}); ok && data["task_id"] == "task-1" { + taskUpdated = true + if interrupted, ok := data["interrupted"].(bool); !ok || !interrupted { + t.Fatalf("task.updated interrupted = %#v, want true", data["interrupted"]) + } + } + } + } + if !taskUpdated { + t.Fatal("expected task.updated after setting the interrupted marker") + } +} + +func TestService_ActiveSessionSweepUsesFreshEffectsContextAfterDelayedCommit(t *testing.T) { + delayed := &delayedOrphanSessionRepository{ + entered: make(chan struct{}), + release: make(chan struct{}), + } + svc, eventBus, repo := createTestServiceWithSessionsRepo(t, func(base *sqliterepo.Repository) repository.SessionRepository { + delayed.Repository = base + return delayed + }) + svc.SetSessionExecutionRegistry(&stubExecutionRegistry{}) + svc.SetStallDetectionThreshold(2 * time.Hour) + ctx := context.Background() + seedOrphanSweepFixtures(t, repo) + seedOrphanSweepTask(t, repo, "task-active-delay", false) + stale := time.Now().UTC().Add(-5 * time.Hour) + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-active-delay", TaskID: "task-active-delay", State: models.TaskSessionStateRunning, + AgentProfileID: "agent-1", IsPrimary: true, UpdatedAt: stale, StartedAt: stale, + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + passCtx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + done := make(chan struct{}) + go func() { + svc.runActiveSessionSweep(passCtx, time.Now().UTC()) + close(done) + }() + select { + case <-delayed.entered: + case <-time.After(time.Second): + t.Fatal("active sweep did not start the delayed recovery") + } + select { + case <-passCtx.Done(): + case <-time.After(time.Second): + t.Fatal("active sweep did not reach its deadline") + } + close(delayed.release) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("active sweep did not finish after delayed commit") + } + if !sessionRecoveredEventPublished(eventBus, "session-active-delay") { + t.Fatal("expected recovery event after delayed active-sweep commit") + } +} + +type failOnceToolSettlementTurns struct { + repository.TurnRepository + fail bool +} + +func (r *failOnceToolSettlementTurns) CompletePendingToolCallsForTurn(ctx context.Context, turnID string) (int64, error) { + if r.fail { + r.fail = false + return 0, errors.New("tool settlement unavailable") + } + return r.TurnRepository.CompletePendingToolCallsForTurn(ctx, turnID) +} + +func TestService_ActiveSessionSweepRetriesPartialSettlement(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + turn, err := fixture.svc.StartTurn(ctx, "session-1") + if err != nil { + t.Fatalf("StartTurn: %v", err) + } + old := fixture.now.Add(-5 * time.Hour) + if err := fixture.svc.messages.CreateMessage(ctx, &models.Message{ + ID: "retry-pending-tool-message", + TaskSessionID: "session-1", + TaskID: "task-1", + TurnID: turn.ID, + AuthorType: models.MessageAuthorAgent, + Type: models.MessageTypeToolExecute, + Content: "tool output", + Metadata: map[string]interface{}{"tool_call_id": "retry-call-1", "status": "pending"}, + CreatedAt: old, + UpdatedAt: old, + }); err != nil { + t.Fatalf("CreateMessage: %v", err) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_session_turns SET started_at = ?, created_at = ?, updated_at = ? WHERE id = ?`, + old, old, old, turn.ID); err != nil { + t.Fatalf("age turn: %v", err) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_sessions SET updated_at = ? WHERE id = ?`, old, "session-1"); err != nil { + t.Fatalf("age session: %v", err) + } + if err := fixture.rawRepo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "stale-recovery-row", SessionID: "session-1", TaskID: "task-1", + Status: models.ExecutorRunningStatusRunning, AgentExecutionID: "stale-recovery-execution", + LocalPID: 42, CreatedAt: old, UpdatedAt: old, + }); err != nil { + t.Fatalf("seed stale executor row: %v", err) + } + fixture.svc.turns = &failOnceToolSettlementTurns{TurnRepository: fixture.svc.turns, fail: true} + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + failed, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession after partial settlement: %v", err) + } + if _, pending := models.LoadInterruptedRecoverySettlement(failed.Metadata); !pending { + t.Fatalf("session metadata after partial settlement = %#v, want durable retry marker", failed.Metadata) + } + stopped, err := fixture.rawRepo.GetExecutorRunningBySessionID(ctx, "session-1") + if err != nil { + t.Fatalf("GetExecutorRunningBySessionID after partial settlement: %v", err) + } + if stopped.Status != models.ExecutorRunningStatusStopped || stopped.LocalPID != 0 { + t.Fatalf("executor after partial settlement = %+v, want repaired stopped row", stopped) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now.Add(time.Minute)) + retried, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession after retry: %v", err) + } + if _, pending := models.LoadInterruptedRecoverySettlement(retried.Metadata); pending { + t.Fatalf("session metadata after retry = %#v, want settlement marker cleared", retried.Metadata) + } + stopped, err = fixture.rawRepo.GetExecutorRunningBySessionID(ctx, "session-1") + if err != nil { + t.Fatalf("GetExecutorRunningBySessionID after retry: %v", err) + } + if stopped.Status != models.ExecutorRunningStatusStopped || stopped.LocalPID != 0 { + t.Fatalf("executor after retry = %+v, want repaired stopped row", stopped) + } + message, err := fixture.svc.messages.GetMessage(ctx, "retry-pending-tool-message") + if err != nil { + t.Fatalf("GetMessage after retry: %v", err) + } + if got := message.Metadata["status"]; got != "complete" { + t.Fatalf("pending tool status after retry = %v, want complete", got) + } +} + +func TestService_ActiveSessionSweepSkipsRecoveryWhenExecutorSnapshotFails(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + fixture.svc.executors = failGetExecutorRunningRepository{ + ExecutorRepository: fixture.rawRepo, + err: errors.New("executor inventory unavailable"), + } + + fixture.svc.runActiveSessionSweep(context.Background(), fixture.now) + + session, err := fixture.repo.GetTaskSession(context.Background(), "session-1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != models.TaskSessionStateRunning { + t.Fatalf("session state = %q, want RUNNING when executor snapshot fails", session.State) + } + if models.HasInterruptedRecoveryPending(session.Metadata) { + t.Fatalf("session metadata = %#v, want no recovery marker", session.Metadata) + } +} + +func TestService_ActiveSessionSweepRejectsRetryAfterSessionGenerationChanges(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + turn, err := fixture.svc.StartTurn(ctx, "session-1") + if err != nil { + t.Fatalf("StartTurn: %v", err) + } + old := fixture.now.Add(-5 * time.Hour) + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_session_turns SET started_at = ?, created_at = ?, updated_at = ? WHERE id = ?`, + old, old, old, turn.ID); err != nil { + t.Fatalf("age turn: %v", err) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_sessions SET updated_at = ? WHERE id = ?`, old, "session-1"); err != nil { + t.Fatalf("age session: %v", err) + } + if err := fixture.rawRepo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "stale-retry-generation-row", SessionID: "session-1", TaskID: "task-1", + Status: models.ExecutorRunningStatusRunning, AgentExecutionID: "stale-retry-generation-execution", + LocalPID: 42, CreatedAt: old, UpdatedAt: old, + }); err != nil { + t.Fatalf("seed stale executor row: %v", err) + } + fixture.svc.turns = &failOnceToolSettlementTurns{TurnRepository: fixture.svc.turns, fail: true} + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + first, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession after first sweep: %v", err) + } + if _, pending := models.LoadInterruptedRecoverySettlement(first.Metadata); !pending { + t.Fatalf("session metadata after first sweep = %#v, want durable retry marker", first.Metadata) + } + if _, err := fixture.rawRepo.DB().ExecContext(ctx, + `UPDATE task_sessions SET updated_at = ? WHERE id = ?`, first.UpdatedAt.Add(time.Second), "session-1"); err != nil { + t.Fatalf("advance session generation: %v", err) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now.Add(time.Minute)) + second, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession after stale retry: %v", err) + } + if _, pending := models.LoadInterruptedRecoverySettlement(second.Metadata); !pending { + t.Fatalf("session metadata after stale retry = %#v, want settlement marker preserved", second.Metadata) } } @@ -264,6 +629,42 @@ func TestService_ActiveSessionSweepDoesNotHealAroundLiveSibling(t *testing.T) { } } +func TestService_ActiveSessionSweepIdleWaitingSiblingBlocksRecovery(t *testing.T) { + fixture := newSweepFixture(t, 5*time.Hour) + ctx := context.Background() + if err := fixture.repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-idle-sibling", TaskID: "task-1", State: models.TaskSessionStateWaitingForInput, + AgentProfileID: "agent-1", UpdatedAt: fixture.now.Add(-5 * time.Hour), + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + + fixture.svc.runActiveSessionSweep(ctx, fixture.now) + + running, err := fixture.repo.GetTaskSession(ctx, "session-1") + if err != nil { + t.Fatalf("GetTaskSession(session-1): %v", err) + } + if running.State != models.TaskSessionStateRunning { + t.Fatalf("running sibling state = %q, want RUNNING while idle sibling blocks recovery", running.State) + } + idle, err := fixture.repo.GetTaskSession(ctx, "session-idle-sibling") + if err != nil { + t.Fatalf("GetTaskSession(session-idle-sibling): %v", err) + } + if idle.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("idle sibling state = %q, want WAITING_FOR_INPUT", idle.State) + } + stalled := findTaskStalledEvents(fixture.eventBus) + if len(stalled) != 1 { + t.Fatalf("task.stalled events = %d, want 1", len(stalled)) + } + ids, _ := stalled[0].Data.(map[string]interface{})["session_ids"].([]string) + if len(ids) != 1 || ids[0] != "session-1" { + t.Fatalf("stalled session_ids = %v, want [session-1]", ids) + } +} + // TestService_ActiveSessionSweepSkipsArchivedTasks proves the pass boundary: // archived tasks with active sessions belong to the archived pass, not the // active-task pass, and must not produce task.stalled events. @@ -331,8 +732,8 @@ func TestService_ActiveSessionSweepHealAbortedWhenExecutionRegistersMidSweep(t * } // TestService_ActiveSessionSweepHealCancelsOnlyClassifiedSessions proves the -// ID-scoped cancellation: when the task genuinely qualifies for healing, the -// cancellation only touches the sessions the sweep classified. A session +// ID-scoped recovery: when the task genuinely qualifies for healing, the +// transition only touches the sessions the sweep classified. A session // that became active after classification — outside the classified set — // keeps running. func TestService_ActiveSessionSweepHealCancelsOnlyClassifiedSessions(t *testing.T) { @@ -347,8 +748,8 @@ func TestService_ActiveSessionSweepHealCancelsOnlyClassifiedSessions(t *testing. if err != nil { t.Fatalf("GetTaskSession: %v", err) } - if session.State != models.TaskSessionStateCancelled { - t.Fatalf("session-1 state = %q, want CANCELLED", session.State) + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session-1 state = %q, want WAITING_FOR_INPUT", session.State) } } diff --git a/apps/backend/internal/task/service/archived_session_reconciliation.go b/apps/backend/internal/task/service/archived_session_reconciliation.go index cf1a708441..f566fd8e61 100644 --- a/apps/backend/internal/task/service/archived_session_reconciliation.go +++ b/apps/backend/internal/task/service/archived_session_reconciliation.go @@ -2,6 +2,8 @@ package service import ( "context" + "errors" + "fmt" "time" "github.com/kandev/kandev/internal/common/constants" @@ -37,7 +39,8 @@ import ( // session (the turn-complete path dies with the process), leaving // task_sessions stuck in STARTING/RUNNING forever on tasks that were never // archived. Every tick, stale STARTING/RUNNING sessions with no live -// in-memory execution backing them are terminalized. +// in-memory execution backing them return to WAITING_FOR_INPUT for lazy +// recovery. // // Both passes follow the exact periodic-sweep shape StartAutoArchiveLoop // already uses and re-invoke the same finalize transitions; as long as the @@ -123,19 +126,16 @@ func orphanedSessionGracePeriod() time.Duration { // repository is the only production implementer. type orphanedSessionRepository interface { ListStaleRunningSessionsOnUnarchivedTasks(ctx context.Context, staleBefore time.Time) ([]*models.TaskSession, error) - CancelRunningTaskSessionByID(ctx context.Context, sessionID, reason string, staleBefore time.Time) (*models.TaskSession, error) + RecoverTaskSessionByCandidate(ctx context.Context, candidate models.ActiveSessionRecoveryCandidate, staleBefore time.Time) (*models.TaskSession, error) } // runOrphanedSessionReconciliation is pass 2 of the reconciliation loop: it -// terminalizes stale STARTING/RUNNING sessions of unarchived tasks that no -// live in-memory execution backs. Without it, a backend restart mid-turn -// leaves those sessions RUNNING forever (#3711): the actor that would have -// transitioned them died with the process, and no other path ever retries. -// Sessions backed by a live execution — including survivors re-tracked by -// startup recovery — are never touched, and the per-session cancel keeps -// healthy sibling sessions of the same task (e.g. WAITING_FOR_INPUT) intact. -// A session whose cancellation write fails is simply retried on the next -// tick; the sweep never gives up for as long as the process runs. +// returns stale STARTING/RUNNING sessions of unarchived tasks to +// WAITING_FOR_INPUT when no live in-memory execution backs them. An +// interrupted turn is settled without a completion event, so the same +// conversation remains available for lazy recovery. Sessions backed by a +// live execution — including survivors re-tracked by startup recovery — are +// never touched. A failed recovery write is retried on the next tick. func (s *Service) runOrphanedSessionReconciliation(ctx context.Context) { if s.executionLivenessChecker == nil { // Absence-from-store is the sweep's only dead signal; without the @@ -166,10 +166,9 @@ func (s *Service) reconcileOrphanedSessions(ctx context.Context, candidates []*m } // reconcileOrphanedSessionsUntil applies one orphan sweep within the supplied -// admission deadline. A cancellation write may finish after that deadline -// because the repository owns a detached write context; its post-commit -// effects therefore receive a fresh bounded context instead of the expired -// sweep context. +// admission deadline. A recovery write may finish after that deadline because +// the repository owns a detached write context; the state event and turn +// settlement therefore use the bounded context supplied to the write. func (s *Service) reconcileOrphanedSessionsUntil( ctx context.Context, candidates []*models.TaskSession, @@ -180,6 +179,12 @@ func (s *Service) reconcileOrphanedSessionsUntil( if !ok { return } + lastMessageBySession, err := s.messages.GetLastMessageTimeBySessionIDs(ctx, sessionIDs(candidates)) + if err != nil { + s.logger.Warn("orphaned-session reconciliation: failed to load last message times; skipping pass", + zap.Error(err)) + return + } for _, session := range candidates { if session == nil || session.ID == "" { continue @@ -191,36 +196,381 @@ func (s *Service) reconcileOrphanedSessionsUntil( } // Re-check liveness at the moment of the write, not just at the // candidate read: a launch that raced the grace window since the read - // must not be reaped. The staleBefore predicate inside the cancel - // UPDATE is the second half of that guard — it re-asserts the - // staleness cutoff so a row refreshed by an in-flight launch between - // this check and the write no longer matches. + // must not be recovered. The staleBefore predicate inside the recovery + // UPDATE is the second half of that guard — it re-asserts the staleness + // cutoff so a row refreshed by an in-flight launch between this check + // and the write no longer matches. if s.executionLivenessChecker.HasLiveExecution(session.ID) { continue } - cancelled, err := repo.CancelRunningTaskSessionByID(ctx, session.ID, models.SessionOrphanedCancelReason, staleBefore) + turn, err := s.GetActiveTurn(ctx, session.ID) if err != nil { - s.logger.Warn("orphaned-session reconciliation: failed to cancel session", + s.logger.Warn("orphaned-session reconciliation: failed to inspect current turn; skipping session", zap.String("session_id", session.ID), zap.Error(err)) continue } - if cancelled == nil { - // Raced to another state between the candidate read and the - // write; its new owner is responsible for it now. + candidate := models.ActiveSessionRecoveryCandidate{ + TaskID: session.TaskID, + SessionID: session.ID, + ExpectedState: session.State, + ExpectedUpdatedAt: session.UpdatedAt, + ExpectedLastEventAt: lastSessionEventAt(session, lastMessageBySession), + } + if turn != nil { + candidate.ExpectedTurnID = turn.ID + } + if err := s.captureRecoveryExecutorSnapshot(ctx, &candidate); err != nil { + s.logger.Warn("orphaned-session reconciliation: failed to capture executor reservation; skipping session", + zap.String("session_id", session.ID), + zap.Error(err)) continue } - s.logger.Info("orphaned-session reconciliation: terminalized unbacked session", + // The turn read above can take long enough for a replacement + // execution to register. Re-check immediately before the guarded write + // so a live successor keeps ownership of the session. + if s.executionLivenessChecker.HasLiveExecution(session.ID) { + continue + } + recovered, err := repo.RecoverTaskSessionByCandidate(ctx, candidate, staleBefore) + if err != nil { + s.logger.Warn("orphaned-session reconciliation: failed to recover session", + zap.String("session_id", session.ID), + zap.Error(err)) + continue + } + if recovered == nil { + // Raced to another state between the candidate read and the + // write; its new owner is responsible for it now. The next + // sweep will classify any still-interrupted state again. + continue + } + completionCtx, cancelCompletion := context.WithTimeout( + context.WithoutCancel(ctx), taskPublicationTimeout, + ) + s.settleRecoveredSession(completionCtx, candidate, recovered) + cancelCompletion() + s.logger.Info("orphaned-session reconciliation: preserved unbacked session for lazy recovery", zap.String("task_id", session.TaskID), zap.String("session_id", session.ID), zap.String("previous_state", string(session.State))) - completionDeadline := time.Now().Add(taskPublicationTimeout) - completionCtx, cancelCompletion := context.WithDeadline( - context.WithoutCancel(ctx), completionDeadline, - ) - s.runCancelledSessionEffects(completionCtx, session.TaskID, - []*models.TaskSession{session}, []*models.TaskSession{cancelled}, - completionDeadline, models.SessionOrphanedCancelReason) - cancelCompletion() } } + +// captureRecoveryExecutorSnapshot records the executor identity observed with +// the session candidate. Post-commit repair must use this immutable snapshot; +// reading the row again after the session write can otherwise stop a successor +// that registered during the effects window. +func (s *Service) captureRecoveryExecutorSnapshot(ctx context.Context, candidate *models.ActiveSessionRecoveryCandidate) error { + if s == nil || s.executors == nil || candidate == nil || candidate.SessionID == "" { + return nil + } + running, err := s.executors.GetExecutorRunningBySessionID(ctx, candidate.SessionID) + if errors.Is(err, models.ErrExecutorRunningNotFound) { + return nil + } + if err != nil { + return fmt.Errorf("failed to capture executor reservation for session recovery: %w", err) + } + if running == nil { + return nil + } + candidate.ExpectedExecutorID = running.ID + candidate.ExpectedExecutorAgentExecutionID = running.AgentExecutionID + candidate.ExpectedExecutorUpdatedAt = running.UpdatedAt + return nil +} + +// settleRecoveredSession applies the side effects shared by both execution +// loss sweeps. An interrupted turn is closed with zero duration and pending +// tool calls are settled, but no turn.completed event or workflow transition +// is emitted. The recovery state event is published only after the conditional +// session write has committed. +// +//nolint:cyclop // The ordered effects boundary is intentionally explicit. +func (s *Service) settleRecoveredSession( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, + recovered *models.TaskSession, +) { + if recovered == nil { + return + } + // A retry may observe the stale executor row already repaired by an earlier + // attempt. That stopped row is still the same recovery generation and may + // continue through settlement; a newly running or rotated execution never + // satisfies the repaired-row allowance. + if !s.recoveredSessionStillOwned(ctx, candidate, recovered, true) { + return + } + settlementComplete := true + if err := s.repairRecoveredExecutor(ctx, candidate); err != nil { + settlementComplete = false + } + if candidate.ExpectedTurnID != "" { + turnSettled, successor := s.settleRecoveredTurn(ctx, candidate) + if successor { + return + } + settlementComplete = settlementComplete && turnSettled + } + if !s.recoveredSessionStillOwned(ctx, candidate, recovered, true) { + return + } + if candidate.ExpectedTurnID != "" || + candidate.ExpectedState == models.TaskSessionStateCreated || + candidate.ExpectedState == models.TaskSessionStateStarting || + candidate.ExpectedState == models.TaskSessionStateRunning { + if _, err := s.markTaskInterrupted(ctx, recovered); err != nil { + settlementComplete = false + } + } + if !s.recoveredSessionStillOwned(ctx, candidate, recovered, true) { + return + } + // Publish before clearing the durable settlement. If the clear fails, the + // next sweep republishes this idempotent state event so the recovery cannot + // be lost between the database write and publication. + if err := s.publishSessionRecovered(ctx, candidate.TaskID, candidate.ExpectedState, recovered); err != nil { + settlementComplete = false + } + if settlementComplete { + if err := s.clearRecoverySettlement(ctx, recovered); err != nil { + s.logger.Warn("failed to clear recovered-session settlement marker", + zap.String("session_id", candidate.SessionID), zap.Error(err)) + } + } +} + +func (s *Service) settleRecoveredTurn( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, +) (settled, successor bool) { + activeTurn, err := s.GetActiveTurn(ctx, candidate.SessionID) + if err != nil { + s.logger.Warn("failed to inspect interrupted turn after recovery", + zap.String("session_id", candidate.SessionID), + zap.String("turn_id", candidate.ExpectedTurnID), + zap.Error(err)) + return false, false + } + if activeTurn == nil { + return s.completeRecoveredTurnToolCalls(ctx, candidate), false + } + if activeTurn.ID != candidate.ExpectedTurnID { + // A different active turn proves a successor crossed the effects + // boundary. Leave its turn, marker, and event state alone. + return false, true + } + if err := s.turns.AbandonTurn(ctx, candidate.ExpectedTurnID); err != nil { + s.logger.Warn("failed to abandon interrupted turn after recovery", + zap.String("session_id", candidate.SessionID), + zap.String("turn_id", candidate.ExpectedTurnID), + zap.Error(err)) + return false, false + } + return s.completeRecoveredTurnToolCalls(ctx, candidate), false +} + +func (s *Service) completeRecoveredTurnToolCalls( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, +) bool { + affected, err := s.turns.CompletePendingToolCallsForTurn(ctx, candidate.ExpectedTurnID) + if err != nil { + s.logger.Warn("failed to complete pending tool calls for recovered turn", + zap.String("session_id", candidate.SessionID), + zap.String("turn_id", candidate.ExpectedTurnID), + zap.Error(err)) + return false + } + if affected > 0 { + s.logger.Info("completed pending tool calls for recovered turn", + zap.String("session_id", candidate.SessionID), + zap.String("turn_id", candidate.ExpectedTurnID), + zap.Int64("affected", affected)) + } + return true +} + +// repairRecoveredExecutor releases the stale runtime reservation while +// preserving resume_token, worktree, and other recovery identity. The +// compare-and-set variant is preferred when the executor repository exposes +// it, so a successor launch cannot be marked stopped by a delayed sweep. +func (s *Service) repairRecoveredExecutor(ctx context.Context, candidate models.ActiveSessionRecoveryCandidate) error { + if s.executors == nil || candidate.SessionID == "" || + (candidate.ExpectedExecutorID == "" && candidate.ExpectedExecutorAgentExecutionID == "" && candidate.ExpectedExecutorUpdatedAt.IsZero()) { + return nil + } + if cas, ok := s.executors.(interface { + RepairExecutorRunningDeadIfCurrent(context.Context, string, string, time.Time) error + }); ok { + err := cas.RepairExecutorRunningDeadIfCurrent( + ctx, candidate.SessionID, candidate.ExpectedExecutorAgentExecutionID, candidate.ExpectedExecutorUpdatedAt, + ) + if errors.Is(err, models.ErrExecutorRunningNotFound) || errors.Is(err, models.ErrExecutionRotated) { + return nil + } + return err + } else { + // Legacy adapters have no compare-and-set primitive. Refuse to claim + // settlement completed: a post-commit read/repair pair could stop a + // successor, so the marker and event must remain retryable instead. + return errors.New("executor repository does not support guarded recovery repair") + } +} + +// recoveredSessionStillOwned verifies that the recovery write still owns its +// effects boundary. It compares the session generation and the executor +// identity captured before the write; a newly registered successor therefore +// prevents stale marker, turn, and event effects from being applied. +func (s *Service) recoveredSessionStillOwned( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, + recovered *models.TaskSession, + allowRepairedExecutor bool, +) bool { + if recovered == nil || s.sessions == nil { + return false + } + expectedUpdatedAt := recovered.UpdatedAt + if !candidate.RecoveredUpdatedAt.IsZero() { + expectedUpdatedAt = candidate.RecoveredUpdatedAt + } + current, err := s.sessions.GetTaskSession(ctx, candidate.SessionID) + if err != nil || current == nil || current.TaskID != candidate.TaskID || + current.State != models.TaskSessionStateWaitingForInput || + !current.UpdatedAt.Equal(expectedUpdatedAt) { + return false + } + return s.recoveryExecutorStillOwned(ctx, candidate, allowRepairedExecutor) +} + +//nolint:cyclop // Identity, timestamp, and repaired-row guards are one CAS contract. +func (s *Service) recoveryExecutorStillOwned( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, + allowRepairedExecutor bool, +) bool { + if s.executors == nil { + return true + } + running, err := s.executors.GetExecutorRunningBySessionID(ctx, candidate.SessionID) + if errors.Is(err, models.ErrExecutorRunningNotFound) { + return candidate.ExpectedExecutorID == "" && candidate.ExpectedExecutorAgentExecutionID == "" && candidate.ExpectedExecutorUpdatedAt.IsZero() + } + if err != nil || running == nil { + return false + } + if candidate.ExpectedExecutorID == "" && candidate.ExpectedExecutorAgentExecutionID == "" && candidate.ExpectedExecutorUpdatedAt.IsZero() { + return false + } + if candidate.ExpectedExecutorID != "" && running.ID != candidate.ExpectedExecutorID { + return false + } + if candidate.ExpectedExecutorAgentExecutionID != "" && running.AgentExecutionID != candidate.ExpectedExecutorAgentExecutionID { + return false + } + if !candidate.ExpectedExecutorUpdatedAt.IsZero() && !running.UpdatedAt.Equal(candidate.ExpectedExecutorUpdatedAt) { + return allowRepairedExecutor && running.Status == models.ExecutorRunningStatusStopped && + running.AgentExecutionID == candidate.ExpectedExecutorAgentExecutionID + } + return true +} + +func (s *Service) clearRecoverySettlement(ctx context.Context, session *models.TaskSession) error { + settlement, ok := models.LoadInterruptedRecoverySettlement(session.Metadata) + if !ok { + return nil + } + remover, ok := s.sessions.(interface { + RemoveSessionMetadataKeyIfJSONValue(context.Context, string, string, interface{}) (bool, error) + }) + if !ok { + return errors.New("session repository does not support guarded recovery settlement removal") + } + _, err := remover.RemoveSessionMetadataKeyIfJSONValue( + ctx, session.ID, models.SessionMetaKeyRecoverySettlementPending, settlement, + ) + return err +} + +type interruptedTaskMetadataSetter interface { + SetTaskMetadataKeyIfNotArchived(ctx context.Context, taskID, key string, value interface{}) (bool, error) +} + +type interruptedTaskMetadataAbsentSetter interface { + SetTaskMetadataKeyIfAbsentNotArchived(ctx context.Context, taskID, key string, value interface{}) (bool, error) +} + +// interruptedTaskMetadataRecoverySetter adds the session recovery generation +// to the marker write. The task row is updated only while the recovered +// session still owns its WAITING_FOR_INPUT generation and its settlement token +// is present. This closes the gap between the in-memory ownership check and +// the task metadata write when a successor starts concurrently. +type interruptedTaskMetadataRecoverySetter interface { + SetTaskMetadataKeyIfRecoveryCurrent( + ctx context.Context, + taskID, sessionID string, + expectedSessionUpdatedAt time.Time, + expectedRecoveryToken, key string, + value interface{}, + ) (bool, error) +} + +func (s *Service) markTaskInterrupted(ctx context.Context, recovered *models.TaskSession) (bool, error) { + if recovered == nil || recovered.TaskID == "" || recovered.ID == "" { + return false, nil + } + settlement, hasSettlement := models.LoadInterruptedRecoverySettlement(recovered.Metadata) + // The concrete SQLite task repository owns the archive-atomic metadata CAS. + // Keep this optional so focused task-service users and test repositories + // without the newer marker primitive remain compatible. + _, ok := s.tasks.(interruptedTaskMetadataSetter) + if !ok { + return false, errors.New("task repository does not support recovery interruption markers") + } + value := time.Now().UTC().Format(time.RFC3339Nano) + expectedUpdatedAt := recovered.UpdatedAt + if hasSettlement && !settlement.RecoveredUpdatedAt.IsZero() { + expectedUpdatedAt = settlement.RecoveredUpdatedAt + } + var changed bool + var err error + if recoverySetter, supported := s.tasks.(interruptedTaskMetadataRecoverySetter); supported && hasSettlement { + changed, err = recoverySetter.SetTaskMetadataKeyIfRecoveryCurrent( + ctx, + recovered.TaskID, + recovered.ID, + expectedUpdatedAt, + settlement.Token, + models.MetaKeyInterruptedAt, + value, + ) + } else if absentSetter, supported := s.tasks.(interruptedTaskMetadataAbsentSetter); supported { + changed, err = absentSetter.SetTaskMetadataKeyIfAbsentNotArchived(ctx, recovered.TaskID, models.MetaKeyInterruptedAt, value) + } else { + // A plain archive guard does not protect a recovery marker from a + // newer interruption arriving between the ownership check and this write. + // Fail closed when the adapter lacks either generation-safe primitive. + s.logger.Warn("skipping recovery interruption marker without generation-safe setter", + zap.String("task_id", recovered.TaskID), + zap.String("session_id", recovered.ID)) + return false, errors.New("task repository does not support generation-safe recovery interruption markers") + } + if err != nil { + return false, err + } + task, err := s.tasks.GetTask(ctx, recovered.TaskID) + if err != nil || task == nil { + if err == nil { + err = errors.New("task not found after interruption marker write") + } + return changed, err + } + // Publish after the metadata commit. The task service's per-task FIFO keeps + // this update ahead of later clears, so connected clients see the warning + // without a reload. + s.PublishTaskUpdated(ctx, task) + return changed, nil +} diff --git a/apps/backend/internal/task/service/orphaned_session_reconciliation_test.go b/apps/backend/internal/task/service/orphaned_session_reconciliation_test.go index 81064d8e0f..7de274953f 100644 --- a/apps/backend/internal/task/service/orphaned_session_reconciliation_test.go +++ b/apps/backend/internal/task/service/orphaned_session_reconciliation_test.go @@ -54,12 +54,12 @@ func seedOrphanSweepTask(t *testing.T, repo *sqliterepo.Repository, taskID strin } } -func TestService_OrphanedSessionReconciliationTerminalizesUnbackedSessions(t *testing.T) { +func TestService_OrphanedSessionReconciliationPreservesUnbackedSessions(t *testing.T) { svc, eventBus, repo := createTestService(t) ctx := context.Background() seedOrphanSweepFixtures(t, repo) - stale := time.Now().UTC().Add(-30 * time.Minute) + stale := time.Now().UTC().Add(-5 * time.Hour) fresh := time.Now().UTC().Add(-1 * time.Minute) seedOrphanSweepTask(t, repo, "task-orphaned", false) @@ -94,7 +94,7 @@ func TestService_OrphanedSessionReconciliationTerminalizesUnbackedSessions(t *te // Archived task with a stale RUNNING session: the archived pass owns it, // the orphan pass must not touch it. seedSession("session-archived-task", "task-archived", models.TaskSessionStateRunning, stale) - // Primary session on the orphaned task: the CANCELLED event must carry + // Primary session on the orphaned task: the recovery event must carry // is_primary=true so the status-summary projection keeps the durable // primary assignment. seedSession("session-primary", "task-primary", models.TaskSessionStateRunning, stale) @@ -114,45 +114,99 @@ func TestService_OrphanedSessionReconciliationTerminalizesUnbackedSessions(t *te t.Errorf("session %s state = %q, want %q", id, session.State, want) } } - assertState("session-orphaned-running", models.TaskSessionStateCancelled) - assertState("session-orphaned-starting", models.TaskSessionStateCancelled) + assertState("session-orphaned-running", models.TaskSessionStateWaitingForInput) + assertState("session-orphaned-starting", models.TaskSessionStateWaitingForInput) assertState("session-healthy-sibling", models.TaskSessionStateWaitingForInput) assertState("session-live-execution", models.TaskSessionStateRunning) assertState("session-inflight-launch", models.TaskSessionStateRunning) assertState("session-archived-task", models.TaskSessionStateRunning) - assertState("session-primary", models.TaskSessionStateCancelled) + assertState("session-primary", models.TaskSessionStateWaitingForInput) orphaned, err := repo.GetTaskSession(ctx, "session-orphaned-running") if err != nil { t.Fatalf("GetTaskSession(orphaned): %v", err) } - if orphaned.ErrorMessage != models.SessionOrphanedCancelReason { - t.Errorf("orphaned session error_message = %q, want %q", - orphaned.ErrorMessage, models.SessionOrphanedCancelReason) + if orphaned.ErrorMessage != "" { + t.Errorf("orphaned session error_message = %q, want empty", orphaned.ErrorMessage) } // The event must carry the durable is_primary flag: the primary session's - // cancellation reports is_primary=true rather than the zero value. - if data := sessionCancelledEvent(eventBus, "session-primary"); data != nil { + // recovery reports is_primary=true rather than the zero value. + if data := sessionRecoveredEvent(eventBus, "session-primary"); data != nil { if isPrimary, ok := data["is_primary"].(bool); !ok || !isPrimary { - t.Errorf("primary session cancellation event is_primary = %v, want true", data["is_primary"]) + t.Errorf("primary session recovery event is_primary = %v, want true", data["is_primary"]) } } else { - t.Error("expected a cancellation event for session-primary") + t.Error("expected a recovery event for session-primary") } for _, id := range []string{"session-orphaned-running", "session-orphaned-starting", "session-primary"} { - if !sessionCancelledEventPublished(eventBus, id) { - t.Errorf("expected a session.state_changed event for %s from the orphan sweep, got none", id) + if !sessionRecoveredEventPublished(eventBus, id) { + t.Errorf("expected a recovery session.state_changed event for %s from the orphan sweep, got none", id) } } for _, id := range []string{"session-healthy-sibling", "session-live-execution", "session-inflight-launch", "session-archived-task"} { - if sessionCancelledEventPublished(eventBus, id) { - t.Errorf("unexpected session.state_changed event for %s", id) + if sessionRecoveredEventPublished(eventBus, id) { + t.Errorf("unexpected recovery session.state_changed event for %s", id) } } } +// TestService_OrphanedSessionReconciliationPreservesInterruptedConversation +// covers the restart reconciliation pass. A stale STARTING/RUNNING session +// whose execution disappeared remains the same recoverable conversation, +// including its unfinished turn, instead of becoming CANCELLED. +func TestService_OrphanedSessionReconciliationPreservesInterruptedConversation(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + seedOrphanSweepFixtures(t, repo) + seedOrphanSweepTask(t, repo, "task-interrupted", false) + stale := time.Now().UTC().Add(-30 * time.Minute) + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-interrupted", TaskID: "task-interrupted", State: models.TaskSessionStateRunning, + AgentProfileID: "agent-1", IsPrimary: true, UpdatedAt: stale, StartedAt: stale, + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + turn, err := svc.StartTurn(ctx, "session-interrupted") + if err != nil { + t.Fatalf("StartTurn: %v", err) + } + if _, err := repo.DB().ExecContext(ctx, + `UPDATE task_session_turns SET started_at = ?, created_at = ?, updated_at = ? WHERE id = ?`, + stale, stale, stale, turn.ID); err != nil { + t.Fatalf("age turn: %v", err) + } + if _, err := repo.DB().ExecContext(ctx, + `UPDATE task_sessions SET updated_at = ? WHERE id = ?`, stale, "session-interrupted"); err != nil { + t.Fatalf("age session: %v", err) + } + + svc.SetExecutionLivenessChecker(&fakeExecutionLiveness{}) + svc.runOrphanedSessionReconciliation(ctx) + + session, err := repo.GetTaskSession(ctx, "session-interrupted") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state = %q, want WAITING_FOR_INPUT", session.State) + } + if session.ErrorMessage != "" { + t.Fatalf("session error_message = %q, want empty", session.ErrorMessage) + } + storedTurn, err := svc.turns.GetTurn(ctx, turn.ID) + if err != nil { + t.Fatalf("GetTurn: %v", err) + } + if storedTurn.CompletedAt == nil || !storedTurn.CompletedAt.Equal(storedTurn.StartedAt) { + t.Fatalf("turn completion = %v, want zero-duration completion at %v", storedTurn.CompletedAt, storedTurn.StartedAt) + } + if sessionCancelledEventPublished(eventBus, "session-interrupted") { + t.Fatal("unexpected cancellation event for recovered session") + } +} + func sessionCancelledEventPublished(eventBus *MockEventBus, sessionID string) bool { for _, evt := range eventBus.GetPublishedEvents() { if evt.Type != events.TaskSessionStateChanged { @@ -166,18 +220,27 @@ func sessionCancelledEventPublished(eventBus *MockEventBus, sessionID string) bo return false } -// sessionCancelledEvent returns the last session.state_changed payload for -// sessionID, or nil when none published. The orphan sweep's event must carry -// the durable is_primary flag — the status-summary projector demotes the -// session when a cancellation event reports is_primary: false. -func sessionCancelledEvent(eventBus *MockEventBus, sessionID string) map[string]interface{} { +func sessionRecoveredEventPublished(eventBus *MockEventBus, sessionID string) bool { + for _, evt := range eventBus.GetPublishedEvents() { + if evt.Type != events.TaskSessionStateChanged { + continue + } + data, ok := evt.Data.(map[string]interface{}) + if ok && data["session_id"] == sessionID && data["new_state"] == string(models.TaskSessionStateWaitingForInput) { + return true + } + } + return false +} + +func sessionRecoveredEvent(eventBus *MockEventBus, sessionID string) map[string]interface{} { var found map[string]interface{} for _, evt := range eventBus.GetPublishedEvents() { if evt.Type != events.TaskSessionStateChanged { continue } data, ok := evt.Data.(map[string]interface{}) - if ok && data["session_id"] == sessionID && data["new_state"] == string(models.TaskSessionStateCancelled) { + if ok && data["session_id"] == sessionID && data["new_state"] == string(models.TaskSessionStateWaitingForInput) { found = data } } @@ -254,14 +317,14 @@ type delayedOrphanSessionRepository struct { once sync.Once } -func (r *delayedOrphanSessionRepository) CancelRunningTaskSessionByID( +func (r *delayedOrphanSessionRepository) RecoverTaskSessionByCandidate( ctx context.Context, - sessionID, reason string, + candidate models.ActiveSessionRecoveryCandidate, staleBefore time.Time, ) (*models.TaskSession, error) { r.once.Do(func() { close(r.entered) }) <-r.release - return r.Repository.CancelRunningTaskSessionByID(ctx, sessionID, reason, staleBefore) + return r.Repository.RecoverTaskSessionByCandidate(ctx, candidate, staleBefore) } func TestService_OrphanedSessionReconciliationUsesFreshEffectsContextAfterDeadline(t *testing.T) { @@ -287,8 +350,6 @@ func TestService_OrphanedSessionReconciliationUsesFreshEffectsContextAfterDeadli if err != nil { t.Fatalf("GetTaskSession: %v", err) } - clarifications := &recordingTaskClarificationCanceller{} - svc.SetClarificationCanceller(clarifications) svc.SetExecutionLivenessChecker(&fakeExecutionLiveness{}) passCtx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) @@ -301,7 +362,7 @@ func TestService_OrphanedSessionReconciliationUsesFreshEffectsContextAfterDeadli select { case <-delayed.entered: case <-time.After(time.Second): - t.Fatal("orphan cancellation did not start") + t.Fatal("orphan recovery did not start") } select { case <-passCtx.Done(): @@ -315,11 +376,101 @@ func TestService_OrphanedSessionReconciliationUsesFreshEffectsContextAfterDeadli t.Fatal("reconciliation did not finish after the delayed write was released") } - if len(clarifications.contextErrs) != 1 || clarifications.contextErrs[0] != nil { - t.Fatalf("clarification cleanup context errors = %v, want one nil error after a late commit", clarifications.contextErrs) + if session, err := repo.GetTaskSession(setupCtx, "session-late-effects"); err != nil { + t.Fatalf("GetTaskSession after delayed recovery: %v", err) + } else if session.State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state after delayed recovery = %q, want WAITING_FOR_INPUT", session.State) } - if !sessionCancelledEventPublished(eventBus, "session-late-effects") { - t.Fatal("expected cancellation event after the delayed write committed") + if !sessionRecoveredEventPublished(eventBus, "session-late-effects") { + t.Fatal("expected recovery event after the delayed write committed") + } +} + +type successorAfterRecoveryRepository struct { + *sqliterepo.Repository + afterRecover func() +} + +func (r *successorAfterRecoveryRepository) RecoverTaskSessionByCandidate( + ctx context.Context, + candidate models.ActiveSessionRecoveryCandidate, + staleBefore time.Time, +) (*models.TaskSession, error) { + recovered, err := r.Repository.RecoverTaskSessionByCandidate(ctx, candidate, staleBefore) + if recovered != nil && err == nil && r.afterRecover != nil { + r.afterRecover() + } + return recovered, err +} + +// A successor can register after the guarded session write commits but before +// post-commit effects run. The recovery must leave that successor's executor +// row and warning marker untouched. +func TestService_OrphanedSessionReconciliationDoesNotRepairSuccessorExecutor(t *testing.T) { + var rawRepo *sqliterepo.Repository + var successorInstalled bool + wrapper := &successorAfterRecoveryRepository{} + svc, _, repo := createTestServiceWithSessionsRepo(t, func(base *sqliterepo.Repository) repository.SessionRepository { + rawRepo = base + wrapper.Repository = base + wrapper.afterRecover = func() { + if successorInstalled { + return + } + successorInstalled = true + now := time.Now().UTC() + if err := base.UpsertExecutorRunning(context.Background(), &models.ExecutorRunning{ + ID: "successor-row", SessionID: "session-successor", TaskID: "task-successor", + Status: models.ExecutorRunningStatusRunning, AgentExecutionID: "successor-execution", + LocalPID: 1234, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("install successor executor: %v", err) + } + if err := base.SetTaskMetadataKey(context.Background(), "task-successor", models.MetaKeyInterruptedAt, "successor-marker"); err != nil { + t.Fatalf("install successor warning marker: %v", err) + } + } + return wrapper + }) + ctx := context.Background() + seedOrphanSweepFixtures(t, repo) + seedOrphanSweepTask(t, repo, "task-successor", false) + stale := time.Now().UTC().Add(-5 * time.Hour) + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-successor", TaskID: "task-successor", State: models.TaskSessionStateRunning, + AgentProfileID: "agent-1", IsPrimary: true, UpdatedAt: stale, StartedAt: stale, + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + oldExecTime := stale + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "old-row", SessionID: "session-successor", TaskID: "task-successor", + Status: models.ExecutorRunningStatusRunning, AgentExecutionID: "old-execution", + LocalPID: 99, CreatedAt: oldExecTime, UpdatedAt: oldExecTime, + }); err != nil { + t.Fatalf("seed old executor: %v", err) + } + svc.SetExecutionLivenessChecker(&fakeExecutionLiveness{}) + + // The active sweep uses the same task/session recovery path as restart + // reconciliation, so an expired liveness snapshot cannot stop the successor. + svc.SetSessionExecutionRegistry(&stubExecutionRegistry{}) + svc.SetStallDetectionThreshold(2 * time.Hour) + svc.runActiveSessionSweep(ctx, time.Now().UTC()) + + running, err := rawRepo.GetExecutorRunningBySessionID(ctx, "session-successor") + if err != nil { + t.Fatalf("GetExecutorRunningBySessionID: %v", err) + } + if running.AgentExecutionID != "successor-execution" || running.Status != models.ExecutorRunningStatusRunning || running.LocalPID != 1234 { + t.Fatalf("successor executor = %+v, want running successor unchanged", running) + } + task, err := rawRepo.GetTask(ctx, "task-successor") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if got := task.Metadata[models.MetaKeyInterruptedAt]; got != "successor-marker" { + t.Fatalf("successor warning marker = %#v, want successor-marker", got) } } @@ -327,7 +478,7 @@ func TestService_OrphanedSessionReconciliationUsesFreshEffectsContextAfterDeadli // pins the read-then-write guard: a launch CAS-writes its session to STARTING // (bumping updated_at) before it registers an execution in the in-memory // store, so the row can be refreshed between the sweep's candidate read and -// its cancellation write. The cancel's staleBefore predicate must fail to +// its recovery write. The recovery's staleBefore predicate must fail to // match that refreshed row instead of cancelling a launch in progress. func TestService_OrphanedSessionReconciliationSparesRowRefreshedSinceCandidateRead(t *testing.T) { svc, eventBus, repo := createTestService(t) diff --git a/apps/backend/internal/task/service/service_events.go b/apps/backend/internal/task/service/service_events.go index f5137dd6c7..3fcd10b985 100644 --- a/apps/backend/internal/task/service/service_events.go +++ b/apps/backend/internal/task/service/service_events.go @@ -244,6 +244,54 @@ func (s *Service) publishSessionsCancelled( } } +// publishSessionRecovered publishes the state transition produced by +// execution-loss recovery. It mirrors the session.state_changed payload used +// by cancellation while carrying an empty error message and the recoverable +// WAITING_FOR_INPUT state. +func (s *Service) publishSessionRecovered( + ctx context.Context, + taskID string, + oldState models.TaskSessionState, + session *models.TaskSession, +) error { + if s.eventBus == nil || session == nil { + return nil + } + sessCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + data := map[string]interface{}{ + sessionEventFieldTaskID: taskID, + sessionEventFieldSessionID: session.ID, + "old_state": string(oldState), + "new_state": string(session.State), + "error_message": "", + "agent_profile_id": session.AgentProfileID, + "agent_profile_snapshot": session.AgentProfileSnapshot, + "is_passthrough": session.IsPassthrough, + "is_primary": session.IsPrimary, + sessionEventFieldUpdatedAt: session.UpdatedAt.Format(time.RFC3339Nano), + sessionEventFieldName: session.Name, + } + if session.ReviewStatus != models.ReviewStatusNone { + data["review_status"] = string(session.ReviewStatus) + } + if len(session.Metadata) > 0 { + data["session_metadata"] = session.Metadata + } + if session.TaskEnvironmentID != "" { + data["task_environment_id"] = session.TaskEnvironmentID + } + event := bus.NewEvent(events.TaskSessionStateChanged, "task-service", data) + if err := s.eventBus.Publish(sessCtx, events.TaskSessionStateChanged, event); err != nil { + s.logger.Error("failed to publish session recovery event", + zap.String(sessionEventFieldTaskID, taskID), + zap.String(sessionEventFieldSessionID, session.ID), + zap.Error(err)) + return err + } + return nil +} + // publishTaskEvent publishes task events to the event bus func (s *Service) publishTaskEvent(ctx context.Context, eventType string, task *models.Task, oldState *v1.TaskState, oldWorkflowIDs ...string) { s.publishTaskEventWithExtra(ctx, eventType, task, oldState, nil, oldWorkflowIDs...) @@ -439,6 +487,10 @@ func (s *Service) publishTaskEventNow(ctx context.Context, eventType string, tas // omitted key here would make clearTaskAutoStartFailedMarker's publish // as invisible as the set it is meant to undo. "auto_start_failed": task.Metadata[models.MetaKeyAutoStartFailed] != nil, + // Keep the interruption projection explicit on task.updated so a live + // client receives the warning immediately after reconciliation writes + // the marker; task-merge preserves omitted fields for partial updates. + "interrupted": task.Metadata[models.MetaKeyInterruptedAt] != nil, // The human assignee, always sent, never omitted when empty, for the // same reason as auto_start_failed above: the frontend pins the // previous value when the key is absent, so omitting it would make diff --git a/apps/web/components/task/task-item.test.tsx b/apps/web/components/task/task-item.test.tsx index 98dc854ab3..d3cb2e0978 100644 --- a/apps/web/components/task/task-item.test.tsx +++ b/apps/web/components/task/task-item.test.tsx @@ -265,7 +265,7 @@ describe("TaskItem queue status", () => { }); describe("TaskItem interrupted icon", () => { - it("shows the red interrupted icon for a task marked interrupted after a restart", () => { + it("shows the warning triangle for a task marked interrupted after a restart", () => { renderTaskItem({ state: "REVIEW", sessionState: "WAITING_FOR_INPUT", diff --git a/apps/web/e2e/tests/task/mobile-task-interrupted-icon.spec.ts b/apps/web/e2e/tests/task/mobile-task-interrupted-icon.spec.ts new file mode 100644 index 0000000000..d2ea45c4f9 --- /dev/null +++ b/apps/web/e2e/tests/task/mobile-task-interrupted-icon.spec.ts @@ -0,0 +1,41 @@ +import { expect, test } from "../../fixtures/test-base"; +import { MobileKanbanPage } from "../../pages/mobile-kanban-page"; + +test.describe("Mobile kanban - interrupted-task icon", () => { + test("shows the warning triangle for an interrupted task", async ({ + testPage, + apiClient, + seedData, + }) => { + const interrupted = await apiClient.createTask( + seedData.workspaceId, + "Mobile interrupted fixture", + { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + metadata: { interrupted_at: "2026-08-02T10:00:00Z" }, + }, + ); + await apiClient.updateTaskState(interrupted.id, "REVIEW"); + + const plain = await apiClient.createTask(seedData.workspaceId, "Mobile plain fixture", { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + }); + await apiClient.updateTaskState(plain.id, "REVIEW"); + + const mobile = new MobileKanbanPage(testPage); + await mobile.goto(); + + const interruptedCard = mobile.taskCard(interrupted.id); + await expect(interruptedCard).toBeVisible(); + const interruptedIcon = interruptedCard.getByTestId("task-state-interrupted"); + await expect(interruptedIcon).toBeVisible(); + await expect(interruptedIcon).toHaveClass(/tabler-icon-alert-triangle/); + await expect(interruptedIcon).toHaveClass(/text-yellow-500/); + + const plainCard = mobile.taskCard(plain.id); + await expect(plainCard).toBeVisible(); + await expect(plainCard.getByTestId("task-state-interrupted")).toHaveCount(0); + }); +}); diff --git a/apps/web/e2e/tests/task/task-interrupted-icon.spec.ts b/apps/web/e2e/tests/task/task-interrupted-icon.spec.ts index e0d23c5266..1177b33986 100644 --- a/apps/web/e2e/tests/task/task-interrupted-icon.spec.ts +++ b/apps/web/e2e/tests/task/task-interrupted-icon.spec.ts @@ -4,13 +4,13 @@ import { SessionPage } from "../../pages/session-page"; // Interrupted-task indicator: when Kandev dies while a task's session is // mid-turn, startup reconciliation marks the task with the `interrupted_at` -// metadata key and the task list shows a red alert icon until the task is +// metadata key and the task list shows a warning triangle until the task is // resumed. Seeding `interrupted_at` through the public task-metadata surface // is the deterministic stand-in for a real crash; the icon itself must render // from that marker alone. test.describe("Task list — interrupted-task icon", () => { - test("shows the red interrupted icon only for marked tasks", async ({ + test("shows the warning triangle only for marked tasks", async ({ testPage, apiClient, seedData, @@ -58,38 +58,48 @@ test.describe("Task list — interrupted-task icon", () => { await apiClient.updateTaskState(terminal.id, "COMPLETED"); const kanban = new KanbanPage(testPage); + const workflowSnapshot = testPage.waitForResponse( + (response) => + response.request().method() === "GET" && + response.url().includes(`/api/v1/workflows/${seedData.workflowId}/snapshot`) && + response.ok(), + ); await kanban.goto(); + await workflowSnapshot; + const interruptedCard = kanban.taskCard(interrupted.id); + await expect(interruptedCard).toBeVisible(); + const cardIcon = interruptedCard.getByTestId("task-state-interrupted"); + await expect(cardIcon).toBeVisible(); + await expect(cardIcon).toHaveClass(/tabler-icon-alert-triangle/); + await expect(cardIcon).toHaveClass(/text-yellow-500/); + const anchorCard = kanban.taskCardByTitle("Anchor Session"); - await expect(anchorCard).toBeVisible({ timeout: 20_000 }); + await expect(anchorCard).toBeVisible(); await anchorCard.click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 20_000 }); + await expect(testPage).toHaveURL(/\/t\//); const session = new SessionPage(testPage); await session.waitForLoad(); const interruptedRow = session.sidebarTaskItem("Interrupted Fixture"); - await expect(interruptedRow).toBeVisible({ timeout: 20_000 }); - await expect(interruptedRow.getByTestId("task-state-interrupted")).toBeVisible({ - timeout: 20_000, - }); + await expect(interruptedRow).toBeVisible(); + await expect(interruptedRow.getByTestId("task-state-interrupted")).toBeVisible(); const plainRow = session.sidebarTaskItem("Plain Fixture"); - await expect(plainRow).toBeVisible({ timeout: 20_000 }); + await expect(plainRow).toBeVisible(); await expect(plainRow.getByTestId("task-state-interrupted")).toHaveCount(0); const terminalRow = session.sidebarTaskItem("Terminal Fixture"); - await expect(terminalRow).toBeVisible({ timeout: 20_000 }); + await expect(terminalRow).toBeVisible(); await expect(terminalRow.getByTestId("task-state-interrupted")).toHaveCount(0); // Reload: the marker must survive SSR hydration from the boot payload. await testPage.reload(); await session.waitForLoad(); - await expect(session.sidebarTaskItem("Interrupted Fixture")).toBeVisible({ - timeout: 20_000, - }); + await expect(session.sidebarTaskItem("Interrupted Fixture")).toBeVisible(); await expect( session.sidebarTaskItem("Interrupted Fixture").getByTestId("task-state-interrupted"), - ).toBeVisible({ timeout: 20_000 }); + ).toBeVisible(); await expect( session.sidebarTaskItem("Plain Fixture").getByTestId("task-state-interrupted"), ).toHaveCount(0); diff --git a/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts b/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts index 284541cbaf..8257618922 100644 --- a/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts +++ b/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts @@ -300,6 +300,8 @@ const WORKTREE_EXECUTOR_FIELDS = { primaryExecutorName: "Worktree", isRemoteExecutor: false, }; +const INTERRUPTED_TASK_ID = "task-1"; +const INTERRUPTED_STALE_TASK_TITLE = "Interrupted stale task"; function seedCachedTask(taskOverrides: Record) { mockState.kanbanMulti.snapshots = { @@ -370,6 +372,78 @@ describe("useAllWorkflowSnapshots — snapshot mapping", () => { await waitFor(() => expect(mockSetWorkflowSnapshot).toHaveBeenCalled()); expect(mockSetWorkflowSnapshot.mock.calls[0][1].tasks[0].autopilot).toBe(false); }); + + it("preserves a newer live interruption marker over a stale snapshot value", async () => { + seedCachedTask({ id: INTERRUPTED_TASK_ID, interrupted: false }); + let resolveSnapshot: ((value: { steps: unknown[]; tasks: unknown[] }) => void) | undefined; + mockFetchWorkflowSnapshot.mockImplementationOnce( + () => + new Promise<{ steps: unknown[]; tasks: unknown[] }>((resolve) => { + resolveSnapshot = resolve; + }), + ); + + renderHook(() => useAllWorkflowSnapshots("ws-A")); + await waitFor(() => expect(mockFetchWorkflowSnapshot).toHaveBeenCalled()); + + // A live task.updated event marks the task while the older snapshot is in flight. + seedCachedTask({ id: INTERRUPTED_TASK_ID, interrupted: true }); + resolveSnapshot?.({ + steps: [{ id: "step-1", name: "Review", position: 1 }], + tasks: [ + { + id: INTERRUPTED_TASK_ID, + workflow_step_id: "step-1", + title: INTERRUPTED_STALE_TASK_TITLE, + interrupted: false, + }, + ], + }); + + await waitFor(() => expect(mockSetWorkflowSnapshot).toHaveBeenCalled()); + expect(mockSetWorkflowSnapshot.mock.calls[0][1].tasks[0].interrupted).toBe(true); + }); +}); + +describe("useAllWorkflowSnapshots — interruption marker generations", () => { + beforeEach(() => { + resetMocks([{ id: "wf-A", workspaceId: "ws-A", name: "A" }]); + }); + + it("preserves a newer false marker after a complete in-flight interruption episode", async () => { + seedCachedTask({ id: INTERRUPTED_TASK_ID, interrupted: false, interruptedGeneration: 1 }); + let resolveSnapshot: ((value: { steps: unknown[]; tasks: unknown[] }) => void) | undefined; + mockFetchWorkflowSnapshot.mockImplementationOnce( + () => + new Promise<{ steps: unknown[]; tasks: unknown[] }>((resolve) => { + resolveSnapshot = resolve; + }), + ); + + renderHook(() => useAllWorkflowSnapshots("ws-A")); + await waitFor(() => expect(mockFetchWorkflowSnapshot).toHaveBeenCalled()); + + // The marker changes twice while the snapshot is in flight. The final + // false value is newer even though it equals the value at fetch start. + seedCachedTask({ id: INTERRUPTED_TASK_ID, interrupted: false, interruptedGeneration: 3 }); + resolveSnapshot?.({ + steps: [{ id: "step-1", name: "Review", position: 1 }], + tasks: [ + { + id: INTERRUPTED_TASK_ID, + workflow_step_id: "step-1", + title: INTERRUPTED_STALE_TASK_TITLE, + interrupted: false, + }, + ], + }); + + await waitFor(() => expect(mockSetWorkflowSnapshot).toHaveBeenCalled()); + expect(mockSetWorkflowSnapshot.mock.calls[0][1].tasks[0]).toMatchObject({ + interrupted: false, + interruptedGeneration: 3, + }); + }); }); describe("useAllWorkflowSnapshots — executor binding merge", () => { diff --git a/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts b/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts index c09606f8b6..fa8c689f82 100644 --- a/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts +++ b/apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts @@ -60,6 +60,20 @@ function hasNewerLiveAutoStartFailed( return existing.autoStartFailed !== fetchStart.autoStartFailed; } +function hasNewerLiveInterrupted( + existing: KanbanTask, + fetchStart: KanbanTask | undefined, +): boolean { + if (!fetchStart) return true; + if ( + existing.interruptedGeneration !== undefined || + fetchStart.interruptedGeneration !== undefined + ) { + return existing.interruptedGeneration !== fetchStart.interruptedGeneration; + } + return existing.interrupted !== fetchStart.interrupted; +} + function hasNewerLiveWorkspaceOrphaned( existing: KanbanTask, fetchStart: KanbanTask | undefined, @@ -85,6 +99,12 @@ function preserveLiveMarkerFields( ): void { // A task.updated event can set or clear either marker while the snapshot // is in flight. Preserve the newer live value instead of rolling it back. + if (merged.interrupted === undefined || hasNewerLiveInterrupted(existing, fetchStart)) { + merged.interrupted = existing.interrupted; + } + if (existing.interruptedGeneration !== undefined) { + merged.interruptedGeneration = existing.interruptedGeneration; + } if (merged.autoStartFailed === undefined || hasNewerLiveAutoStartFailed(existing, fetchStart)) { merged.autoStartFailed = existing.autoStartFailed; } diff --git a/apps/web/lib/state/slices/kanban/types.ts b/apps/web/lib/state/slices/kanban/types.ts index 13a052ba37..07aab65589 100644 --- a/apps/web/lib/state/slices/kanban/types.ts +++ b/apps/web/lib/state/slices/kanban/types.ts @@ -142,6 +142,8 @@ export type KanbanState = { foregroundActivity?: ForegroundActivity | null; /** True when the task's session was mid-turn when the backend died. */ interrupted?: boolean; + /** Monotonic client generation for explicit interruption-marker updates. */ + interruptedGeneration?: number; /** True when a workflow step's auto_start_agent on_enter action failed to * launch a run for this task. */ autoStartFailed?: boolean; diff --git a/apps/web/lib/ui/state-icons.test.tsx b/apps/web/lib/ui/state-icons.test.tsx index 1aeadc445c..aa0e939457 100644 --- a/apps/web/lib/ui/state-icons.test.tsx +++ b/apps/web/lib/ui/state-icons.test.tsx @@ -269,7 +269,7 @@ describe("getTaskStateIcon — task-level activity tri-state", () => { }); describe("getTaskStateIcon — status markers", () => { - it("renders the accessible interrupted icon with the tooltip label", () => { + it("renders the accessible warning triangle with the tooltip label", () => { const { container } = render( {getTaskStateIcon("REVIEW", undefined, { interrupted: true })} @@ -277,7 +277,9 @@ describe("getTaskStateIcon — status markers", () => { ); const icon = container.querySelector('[data-testid="task-state-interrupted"]'); expect(icon).not.toBeNull(); - expect(icon?.className).toContain("text-red-500"); + expect(icon?.className).toContain("tabler-icon-alert-triangle"); + expect(icon?.className).not.toContain("tabler-icon-alert-circle"); + expect(icon?.className).toContain("text-yellow-500"); expect(container.querySelector('[aria-label="Interrupted by restart"]')).not.toBeNull(); // The icon itself is decorative; the label lives on the trigger. expect(icon?.getAttribute("aria-hidden")).toBe("true"); diff --git a/apps/web/lib/ui/state-icons.tsx b/apps/web/lib/ui/state-icons.tsx index c5610044b9..2a2266044e 100644 --- a/apps/web/lib/ui/state-icons.tsx +++ b/apps/web/lib/ui/state-icons.tsx @@ -117,12 +117,11 @@ const PENDING_PERMISSION_ICON: IconConfig = { }; // The task-level interrupted affordance: the session was mid-turn when the -// backend died and the task has not been resumed. A red alert circle — red is -// otherwise the error/cancelled hue, but the alert shape plus the REVIEW/idle -// coarse state it replaces keeps it distinct from the terminal X affordances. +// backend died and the task has not been resumed. Use the shared warning +// triangle styling so this durable marker reads as attention-needed state. const TASK_INTERRUPTED_ICON: IconConfig = { - Icon: IconAlertCircle, - className: STYLE_ERROR, + Icon: IconAlertTriangle, + className: STYLE_WARNING, }; // The task-level auto-start-failed affordance: a workflow step's @@ -214,7 +213,7 @@ export function isTerminalInterruptedState( } /** - * Shared red alert affordance for a task whose session was mid-turn when the + * Shared warning affordance for a task whose session was mid-turn when the * backend died. Carries the accessible "Interrupted by restart" label and * tooltip, so every surface that renders the interrupted state (sidebar rows, * board cards, graph nodes, open-task header) presents it consistently. @@ -227,12 +226,12 @@ export function InterruptedTaskIcon({ className }: { className?: string }) { - diff --git a/apps/web/lib/ws/handlers/kanban.ts b/apps/web/lib/ws/handlers/kanban.ts index 41d5854e28..76be34d317 100644 --- a/apps/web/lib/ws/handlers/kanban.ts +++ b/apps/web/lib/ws/handlers/kanban.ts @@ -112,6 +112,7 @@ function preserveMultiSnapshotFields( | "taskPendingAction" | "foregroundActivity" | "interrupted" + | "interruptedGeneration" | "autoStartFailed" | "workspaceOrphaned" | "priority" @@ -126,6 +127,10 @@ function preserveMultiSnapshotFields( taskPendingAction: preserveIfUndefined(t.taskPendingAction, fallback?.taskPendingAction), foregroundActivity: preserveIfUndefined(t.foregroundActivity, fallback?.foregroundActivity), interrupted: preserveIfUndefined(t.interrupted, fallback?.interrupted), + interruptedGeneration: preserveIfUndefined( + t.interruptedGeneration, + fallback?.interruptedGeneration, + ), autoStartFailed: preserveIfUndefined(t.autoStartFailed, fallback?.autoStartFailed), workspaceOrphaned: preserveIfUndefined(t.workspaceOrphaned, fallback?.workspaceOrphaned), priority: preserveIfUndefined(t.priority, fallback?.priority), @@ -354,6 +359,7 @@ export function registerKanbanHandlers(store: StoreApi): WsHandlers { primarySessionPendingAction: existing?.primarySessionPendingAction, taskPendingAction: existing?.taskPendingAction, interrupted: existing?.interrupted, + interruptedGeneration: existing?.interruptedGeneration, autoStartFailed: existing?.autoStartFailed, workspaceOrphaned: existing?.workspaceOrphaned, foregroundActivity: existing?.foregroundActivity, diff --git a/apps/web/lib/ws/handlers/task-merge.ts b/apps/web/lib/ws/handlers/task-merge.ts index 3a8b85ce05..fe54cb21ba 100644 --- a/apps/web/lib/ws/handlers/task-merge.ts +++ b/apps/web/lib/ws/handlers/task-merge.ts @@ -101,12 +101,22 @@ export function mergeTaskUpdate( nextTask: KanbanTask, payload: TaskEventPayload, ): KanbanTask { - if (!existing) return nextTask; + if (!existing) { + return hasPayloadField(payload, "interrupted") + ? { ...nextTask, interruptedGeneration: 1 } + : nextTask; + } if (isStaleTaskUpdate(existing, nextTask)) return existing; const merged = { ...nextTask, ...mergeTaskRepositoryFields(existing, nextTask), }; + // A snapshot can observe the same boolean value before and after a live + // false -> true -> false marker episode. Count every explicit marker update + // so an in-flight snapshot cannot erase that newer generation. + merged.interruptedGeneration = hasPayloadField(payload, "interrupted") + ? (existing.interruptedGeneration ?? 0) + 1 + : existing.interruptedGeneration; preserveOmittedField(existing, merged, payload, nextTask, { payloadKey: "parent_id", taskField: "parentTaskId", diff --git a/apps/web/lib/ws/handlers/tasks-activity.test.ts b/apps/web/lib/ws/handlers/tasks-activity.test.ts index dda4056092..4eab7dca35 100644 --- a/apps/web/lib/ws/handlers/tasks-activity.test.ts +++ b/apps/web/lib/ws/handlers/tasks-activity.test.ts @@ -325,6 +325,22 @@ describe("task.updated interrupted marker (live propagation + safe fallback)", ( expect(interruptedFor(store, "t1")).toEqual({ kanban: false, multi: false }); }); + it("increments the marker generation for every explicit update", () => { + const store = storeWithTask({ id: "t1", interrupted: false, interruptedGeneration: 1 }); + const handlers = registerTasksHandlers(store); + + handlers["task.updated"]!(makeMessage({ ...makeTask("t1"), interrupted: true })); + handlers["task.updated"]!(makeMessage({ ...makeTask("t1"), interrupted: false })); + + expect(store.getState().kanban.tasks.find((task) => task.id === "t1")).toMatchObject({ + interrupted: false, + interruptedGeneration: 3, + }); + expect( + store.getState().kanbanMulti.snapshots.wf1.tasks.find((task) => task.id === "t1"), + ).toMatchObject({ interrupted: false, interruptedGeneration: 3 }); + }); + it("preserves the marker when a partial update omits interrupted", () => { // Clobber guard: a lightweight task.updated (e.g. a rename) must not wipe // the interruption reading between real marker events. diff --git a/docs/plans/orphaned-session-open-recovery/plan.md b/docs/plans/orphaned-session-open-recovery/plan.md new file mode 100644 index 0000000000..d5ea3fee04 --- /dev/null +++ b/docs/plans/orphaned-session-open-recovery/plan.md @@ -0,0 +1,252 @@ +--- +created: 2026-09-21 +status: complete +requirements: + - REQ-TASKS-SESSION-STALL-VISIBILITY-001 + - REQ-TASKS-RESTART-ORPHAN-SESSIONS-002 + - REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001 +system_design: + - ../../specs/tasks/system-design/session-stall-visibility.md + - ../../specs/tasks/system-design/restart-orphaned-session-terminalization.md + - ../../specs/tasks/system-design/interrupted-task-indicator.md +legacy_specs: [] +--- + +# Implementation plan: Orphaned session recovery on task open + +## Overview + +Preserve idle and interrupted running conversations during reconciliation. +Both sweeps must leave interrupted sessions recoverable, rather than cancelled. +Focusing a task restores its selected agent. A new user message continues the +conversation. Opening Kandev must not eagerly resume every interrupted agent. +Implement recoverable settlement, then focus recovery, then the persistent warning indicator. + +The task system owns this change because it owns session cancellation and +recovery eligibility. The UI uses the existing recovery flow. + +## Confirmed evidence + +Read-only diagnostics for task `52f801c5-b0ff-4b4f-8f00-0d37ba804d03` +identify session `a136900f-3546-4843-8a99-37194378222e`. +Times below include the Lisbon UTC offset. + +- September 20, 11:37:44 +01:00: idle reclaim released the provider runtime. + The log recorded a preserved resume token and worktree. +- September 21, 09:44:24 +01:00: startup reconciled `WAITING_FOR_INPUT` + for lazy recovery, again with a token and worktree. +- September 21, 09:45:25 +01:00: `active-session sweep: healing orphaned sessions` + named this session. The state event recorded `WAITING_FOR_INPUT -> CANCELLED`. +- The session-list MCP response independently recorded the same cancellation time. +- The diagnostic manifest identified deployed commit `1614720ceb`. + The bundle was partial because its oldest logs exceeded the archive limit. + The relevant September 21 cancellation records were present. + +PR [#3832](https://github.com/kdlbs/kandev/pull/3832), commit `00c7469d81`, +introduced the active-session sweep. It classifies every execution-less active +session as orphaned, including idle `WAITING_FOR_INPUT` sessions. +PR [#3833](https://github.com/kdlbs/kandev/pull/3833), commit `1614720ceb`, +added a separate restart pass limited to `STARTING` and `RUNNING`. +The logs identify #3832's pass as the cancellation source in this incident. +The merged-PR banner for #3819 is not evidence that its merge cancelled the session. + +`GetTaskSessionStatus` automatically recovers archive cancellations, but other +`CANCELLED` sessions fall through to workspace restoration. The orphan reason +therefore requires a manual Resume despite preserved recovery data. + +## Scope + +### In scope + +- Exclude waiting sessions without an unfinished turn from stall detection and healing. +- Replace execution-loss cancellation in both sweeps with recoverable waiting state. +- Recover existing exact orphan cancellations through normal task opening. +- Continue interrupted work only when a new message arrives, including during startup. +- Preserve same-session history, workspace identity, user preferences, and launch ownership. +- Test desktop and phone opening, failure fallback, and explicit-stop exclusions. +- Keep interruption warnings until successful resume. Replace the shared red circle + with a warning-colored triangle in sidebar rows and task cards. + +### Out of scope + +- Prompt replay, automatic continuation of completed work, or workflow advancement. +- Removing reconciliation, changing thresholds, or adding a runtime flag. +- Live-instance repair, new sessions, database migrations, or provider redesign. + +## Technical approach + +Task 01 changes both reconciliation passes and their conditional repository +transitions. Reuse startup recovery semantics in `reconcileActiveSessionOnStartup`: +settle the abandoned turn without a success event, preserve recovery identity, +and expose `WAITING_FOR_INPUT`. Include missing-runtime-row sessions. Retain +live-execution, launch-grace, archive/stop, and newer-turn guards. Idle sessions +need no transition or stall event. + +Task 02 changes exact cancellation eligibility in `task_operations.go` and +corrects the orphan-reason comment in `models/resume_safety.go`. Reuse existing +resume-token and same-session initialization paths. Keep unsupported recovery +and missing-profile errors actionable. Use the existing frontend recovery hooks +and automatic admission contract. Change frontend logic only if regression +coverage exposes a state-handling gap. + +Task 03 updates the shared interruption renderer and moves marker clearing +from STARTING entry to confirmed recovery success. The existing durable marker +survives unsuccessful attempts. Follow its paired design for stale callbacks, +partial updates, warning styling, and desktop/phone coverage. + +No separate ADR is needed. The existing +[session-open decision](../../decisions/2026-09-18-session-open-resumes-conversation.md) +already owns prompt-free provider recovery. This package also replaces automatic +orphan cancellation with recoverable interruption settlement, as the user requested. + +## ASCII UI preview + +UI-01: Focus an interrupted or legacy orphan-cancelled conversation. Labels are illustrative existing +localized states, not proposed new copy. + +```text +Current desktop +[orphaned session] [Resume] [Start fresh session] +[existing transcript ] + +Proposed desktop +[normal agent startup progress ] +[existing transcript ] + | + v +[existing transcript ] +[message composer Send ] + +Proposed phone: task route or task picker +[Task header / session picker] +[normal startup progress ] +[existing transcript ] <- existing chat scroll +[composer Send ] +[existing bottom navigation ] +``` + +After success, the warning disappears. A new user message continues the work. On actual recovery failure, retain the +existing error, Resume, and Start fresh session controls. With auto-start +prevention enabled, retain manual recovery. Opening never sends a message. +No new overlay, scroll owner, or touch control is added. Keep phone safe areas +and existing touch targets. Structural requirement: recovery occurs in the +selected conversation. Spacing is illustrative. + +UI-02: Interruption status in the existing sidebar/card status slot. + +```text +Before: (red !) Task title +Desktop after: /!\ Task title [warning color] +Phone after: /!\ Task title [drawer row / board card] +Startup: [normal progress; interruption marker retained] +Resume succeeds: [normal task status] Task title +Resume fails: [marker retained; existing recovery error available] +``` + +Keep the localized accessible label "Interrupted by restart". The shared +triangle and warning color are required. Existing layouts and scroll owners +remain. No new hover-only information or touch control is introduced. + +## Tests + +- Task 01 covers stall AC .1-.7 and restart AC 002.1, .4-.7: + idle exemption, resumable interrupted work in both sweeps, missing executor rows, + active-turn read errors, mixed siblings, and completion races. +- Task 02 covers AC .8-.10 with `task_operations_resume_test.go` and frontend + session-resumption tests: exact reason, supported/missing token, profile + validation, duplicate opening, blocked recovery, and failure fallback. +- Task 03 covers interruption AC .2-.7 with marker lifecycle tests, shared icon + tests, and desktop/phone sidebar and board assertions. Verify retained warnings + across failed recovery, reload, and repeated restart, followed by live clearing. +- Retain restart-pass, archive-recovery, explicit-stop, deferred-owner, and + session-ceiling regressions. + +## E2E tests + +The existing `session/session-resume.spec.ts`, +`session/session-recovery.spec.ts`, and their mobile counterparts already cover +backend interruption, lazy focus recovery, queued messages during startup, +same-session follow-up, auto-start prevention, and failed recovery retry using +the real status-to-launch path. The new backend tests establish the exact +legacy orphan eligibility and reconciliation classification without adding a +production endpoint. The task indicator checks below provide the rendered +desktop and phone assertions for the retained warning state. + +Add `task/mobile-task-interrupted-icon.spec.ts` and extend +`task/task-interrupted-icon.spec.ts` for UI-02. Use the shared state-icon unit +tests for every consumer's shape/color contract and the recovery suites for +successful/failed resume behavior. Browser screenshots prove the rendered states. + +## Work orders + +- [x] [Task 01: Preserve resumable conversations](task-01-preserve-idle-conversations.md) +- [x] [Task 02: Recover orphan conversations on open](task-02-recover-orphan-conversations.md) +- [x] [Task 03: Keep interruption warnings](task-03-interruption-warning.md) + +## Related delivery records + +The implemented [stall package](../session-stall-visibility/plan.md) and +[restart package](../restart-orphaned-session-terminalization/plan.md) remain +historical records. Their tests remain regression inputs. This package supersedes their execution-loss cancellation policy and retains +their liveness, grace, and concurrency protections. Their historical results do +not prove the revised behavior. + +## Verification results + +- `python3 scripts/list-docs.py validate`: passed, catalog valid after the warning-indicator revision. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. +- `go test ./internal/orchestrator ./internal/task/service ./internal/task/repository/sqlite`: passed. +- Full affected Go packages pass after the review remediation, including + missing-row focus recovery, successor barriers, fresh-context settlement, + marker CAS, and committed-generation retry regressions. +- Targeted backend recovery, reconciliation, metadata-CAS, and marker-lifecycle tests: passed. +- Targeted frontend Vitest, typecheck, and ESLint checks: passed. +- Review-fixup regressions for fail-closed executor snapshots, CREATED-session + recovery tokens, abandoned-turn tool-call retry, fresh delayed-commit effects + context, and false-to-true-to-false marker generations: passed. +- Chromium and mobile-chrome interrupted-indicator E2E tests: passed (one test each). +- `node --test scripts/validate-public-docs.test.mjs` and + `node scripts/validate-public-docs.mjs`: passed. + +The implementation preserves idle conversations, settles interrupted sessions +to recoverable waiting state with race-safe repository predicates, and lazily +recovers the selected conversation without replaying its old prompt. Exact +legacy orphan cancellations remain compatible with the same recovery path while +archive, explicit-stop, ownership, capacity, and live-execution guards retain +their authority. The shared interruption marker now uses a warning triangle, +survives failed or stale recovery attempts, and clears only after confirmed +provider recovery. + +## Integrated review remediation + +The three work orders are complete together with the follow-up review fixes: + +- Recovered CREATED, STARTING, and RUNNING sessions persist an immutable settlement token + and executor snapshot. A missing `executors_running` row is still resumable, + and task-open status plus focus recovery use the same session and profile. +- Active and restart reconciliation use a bounded post-commit context. Turn, + tool, executor, marker, and event effects remain retryable until the durable + settlement is cleared. Executor repair, session settlement, and marker writes + compare the captured executor and committed session generations, so a + successor cannot be stopped, settled, or marked by a delayed sweep. +- Recovery attempts retain the interruption-marker snapshot through active and + tombstoned attempts. Boot callbacks without a valid generation fail closed, + and a committed marker publishes `task.updated` with the interruption state + so connected desktop and phone clients update without a reload. Live task + merges carry a per-task marker generation, so an in-flight snapshot cannot + erase a newer interruption episode when its final boolean matches the + fetch-start value. + +The integrated implementation and regressions are validated by the backend +package tests, the targeted marker and UI checks, the workspace build and lint, +and the specification and public-document validation commands listed above. + +## Risks + +- A waiting session can still hold an unfinished turn. State alone is insufficient. +- Re-reading a completed turn as absent can defeat the original cancellation snapshot. +- Broadening all cancellations would restart intentionally stopped work. +- A tokenless provider cannot promise native conversation context restoration. +- Partial recovery must not expire questions, duplicate prompts, or advance the workflow. +- Partial diagnostic retention limits reconstruction of earlier runtime history. diff --git a/docs/plans/orphaned-session-open-recovery/task-01-preserve-idle-conversations.md b/docs/plans/orphaned-session-open-recovery/task-01-preserve-idle-conversations.md new file mode 100644 index 0000000000..e6d759bf98 --- /dev/null +++ b/docs/plans/orphaned-session-open-recovery/task-01-preserve-idle-conversations.md @@ -0,0 +1,144 @@ +--- +id: "01-preserve-idle-conversations" +title: "Preserve resumable conversations during reconciliation" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-SESSION-STALL-VISIBILITY-001 + - REQ-TASKS-RESTART-ORPHAN-SESSIONS-002 +acceptance_criteria: + - AC-TASKS-SESSION-STALL-VISIBILITY-001.1 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.2 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.3 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.4 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.5 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.6 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.7 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.1 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.4 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.5 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.6 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.7 +system_design: + - ../../specs/tasks/system-design/session-stall-visibility.md + - ../../specs/tasks/system-design/restart-orphaned-session-terminalization.md +--- + +# Task 01: Preserve resumable conversations during reconciliation + +## Summary + +Keep idle conversations untouched. Reconcile interrupted running conversations +to recoverable waiting state in both sweeps. Preserve abandoned-turn settlement +without cancellation, prompt replay, or workflow advancement. + +## In scope + +- Add `TestService_ActiveSessionSweepPreservesIdleWaitingSession` before implementation. + Reuse `newSweepFixture` with five hours of silence. Set `WAITING_FOR_INPUT` + and no active turn. Expect unchanged state and no `task.stalled` event. + Current code cancels this fixture, matching the recorded incident. +- Add `TestService_OrphanedSessionReconciliationPreservesInterruptedConversation` + and `TestService_ActiveSessionSweepRecoversInterruptedConversation` before changes. + Seed old STARTING/RUNNING sessions without live executions. Expect recoverable + waiting state and preserved identity. Current code cancels these sessions. +- Cover waiting sessions with an unfinished turn, missing/retained executor rows, + active-turn read failure, and ordinary `RUNNING`/`STARTING` candidates. +- Cover an idle waiting sibling plus a genuine stale running sibling. Detection + names only the running sibling; task-wide healing remains blocked. +- Cover a turn completing or being replaced between classification and cancellation. + Carry observed identity to the write and retain existing SQL predicates. +- Replace orphan cancellation calls in both sweeps with conditional recoverable + settlement. Reuse `reconcileActiveSessionOnStartup` semantics and existing + abandoned-turn cleanup without importing orchestrator into task service. +- Preserve questions, deferred work, task ownership, and runtime resume identity. + Release only stale execution reservations through their current owner. +- Update root AGENTS.md stall guidance and historical orphan-reason comments. + +## Out of scope + +Task-open launch behavior, thresholds, explicit-stop/archive policy, and new database columns. + +## Acceptance + +1. Idle waiting sessions produce neither stall events nor orphan cancellations. +2. Unknown turn state skips evaluation. Mixed-session and racing-turn cases preserve healthy work. +3. Interrupted work becomes resumable waiting state in both passes. It never + becomes CANCELLED because of execution loss, and settlement never advances workflow. + +## Verification + +From the repository root: + +```bash +(cd apps/backend && go test ./internal/task/service -run 'TestService_ActiveSessionSweep|TestService_OrphanedSessionReconciliation' -count=1) +(cd apps/backend && go test ./internal/task/repository/sqlite -run 'Test.*(CancelActiveTaskSessionsByCandidates|RecoverInterrupted)' -count=1) +(cd apps/backend && go test ./internal/orchestrator -run 'Test.*(Startup|Reconcile)' -count=1) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Record the new idle test's expected failure before the production correction. +If a repository predicate changes, also run `go run ./cmd/sqlguard ./internal` +from `apps/backend` and record the result. + +## Files likely touched + +- `apps/backend/internal/task/service/active_session_stall.go` +- `apps/backend/internal/task/service/active_session_stall_test.go` +- `apps/backend/internal/task/models/models.go` only if candidate identity needs extension. +- `apps/backend/internal/task/repository/sqlite/session.go` and `session_test.go` only for a proven predicate gap. +- `apps/backend/internal/task/service/archived_session_reconciliation.go` +- `apps/backend/internal/task/service/orphaned_session_reconciliation_test.go` +- `apps/backend/internal/task/service/service_tasks.go` (shared settlement effects). +- `apps/backend/internal/orchestrator/service.go` (startup recovery parity, if needed). +- `apps/backend/internal/orchestrator/service_test.go` (startup recovery regressions). +- `AGENTS.md` + +## Dependencies + +None. Read the paired requirement and design before implementation. + +## Risks + +Filtering the all-active-session denominator would weaken the existing task-wide guard. +A second turn read must not replace the classified turn identity with an idle snapshot. + +## Parallelism + +`sequential` + +## Inputs + +- [Plan evidence](plan.md#confirmed-evidence) +- [Requirements](../../specs/tasks/requirements/session-stall-visibility.md) +- `active_session_stall_test.go` fixture and mixed-sibling tests. +- `idle_session_reaper.go` documents legitimate execution-less waiting sessions. + +## Results + +Implemented both reconciliation paths with guarded recoverable settlement. +Idle `WAITING_FOR_INPUT` sessions are excluded from stall classification, while +stale `STARTING` and `RUNNING` sessions, including sessions without a runtime +row, return to `WAITING_FOR_INPUT` after their observed unfinished turn is +abandoned. State, activity, message, turn, archive, and liveness predicates +protect the transition from races; stale executor reservations are repaired +without changing the resume identity. Settlement publishes a recoverable state +event and never emits turn completion or advances workflow state. + +The service, SQLite repository, and reconciliation regression suites pass, +including idle siblings, turn read failures, missing executors, archive and +explicit-stop exclusions, and completion races. + +The review remediation is included in this work order. Both sweeps now persist +the executorless recovery generation, use a fresh bounded effects context after +detached writes, and retry incomplete settlement from the session metadata. +The retry path accepts the same committed session generation after its stale +executor row was repaired, so an abandoned turn cannot lose its remaining +effects after the session becomes idle or after a successor changes the row. +Executor inventory read failures fail closed before the recovery write, and +CREATED sessions receive the same durable recovery token and interruption +marker as STARTING and RUNNING sessions. diff --git a/docs/plans/orphaned-session-open-recovery/task-02-recover-orphan-conversations.md b/docs/plans/orphaned-session-open-recovery/task-02-recover-orphan-conversations.md new file mode 100644 index 0000000000..6d485b62fb --- /dev/null +++ b/docs/plans/orphaned-session-open-recovery/task-02-recover-orphan-conversations.md @@ -0,0 +1,151 @@ +--- +id: "02-recover-orphan-conversations" +title: "Recover orphan conversations on task open" +status: done +wave: 2 +depends_on: + - "01-preserve-idle-conversations" +plan: "plan.md" +requirements: + - REQ-TASKS-SESSION-STALL-VISIBILITY-001 + - REQ-TASKS-RESTART-ORPHAN-SESSIONS-002 +acceptance_criteria: + - AC-TASKS-SESSION-STALL-VISIBILITY-001.8 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.9 + - AC-TASKS-SESSION-STALL-VISIBILITY-001.10 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.2 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.3 + - AC-TASKS-RESTART-ORPHAN-SESSIONS-002.8 +system_design: + - ../../specs/tasks/system-design/session-stall-visibility.md + - ../../specs/tasks/system-design/restart-orphaned-session-terminalization.md +--- + +# Task 02: Recover orphan conversations on task open + +## Summary + +Treat exact system orphan cancellation as eligible for normal automatic recovery. +Keep the selected conversation, existing launch guards, and failure fallback. + +## In scope + +- Add `TestGetTaskSessionStatus_OrphanCancelledSessionAutoResumes` before implementation. + Seed `CANCELLED`, the exact orphan reason, and a resumable executor row. + Expect `is_resumable` and `needs_resume` true. Current code returns false. +- Cover no token, missing runtime row, non-resumable runtime, missing profile, + existing live execution, archived task, deferred owner, and explicit user stop. + Use existing archive recovery tests as the pattern for same-session initialization. +- Correct the `SessionOrphanedCancelReason` comment that currently equates it with an explicit stop. +- Verify recovery launches with automatic admission, sends no prompt, and preserves + workflow ownership. Exercise repeated status/open calls and capacity refusal. +- Extend frontend hook tests for successful recovery and failure fallback. +- Add desktop and phone scenarios described in the plan using real backend eligibility. + Cover restart during an active turn, focus recovery, and exactly-once delivery + of a new message submitted during startup. Keep another interrupted task + unopened and verify it does not launch. Cover legacy cancelled rows separately. +- Update the relevant recovery section in `docs/public/tasks-and-workflows.md` at implementation, + or the current owning page found by `/docs-maintainer` if that path changes. + Describe recovery, auto-start prevention, and manual fallback as a how-to section. + +## Out of scope + +New recovery UI, unconditional recovery of terminal states, prompt replay, +replacement sessions, or live database mutations. + +## Acceptance + +1. Eligible orphan cancellations recover automatically in the same conversation, including pre-existing rows. +2. Launch guards, user stops, and real failures retain their existing behavior without automatic retry loops. +3. Desktop and phone tests prove warning removal, retained history/workspace, no prompt replay, and successful follow-up messaging. + +## ASCII UI preview + +UI-01 uses the [full plan preview](plan.md#ascii-ui-preview), covering AC .8-.10. + +```text +Desktop: [normal startup progress] -> [transcript + composer] +Phone: [task header / session picker] + [startup -> transcript ] + [composer + bottom navigation] +Failure: existing recovery error and explicit retry controls +``` + +Reuse the existing mobile layout and chat scroll owner. Do not add a dialog. +Retain safe-area clearance and touch-accessible retry and send controls. + +## Verification + +From the repository root, install dependencies once in this worktree: + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/backend && go test ./internal/orchestrator -run 'TestGetTaskSessionStatus_|TestAutoResumeEligibility|Test.*Resume.*|Test.*Archive.*Resume' -count=1) +(cd apps/web && pnpm exec vitest run hooks/domains/session/use-session-resumption.test.ts hooks/domains/session/use-session-resumption.archive.test.ts hooks/domains/session/use-session-resumption.navigation.test.ts) +(cd apps/web && pnpm e2e:run --project chromium tests/session/session-resume-recovery.spec.ts -- --retries=0) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/session/mobile-session-resume-recovery.spec.ts -- --retries=0) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +Run desktop and mobile sequentially. The managed runner builds current source. +Record RED and GREEN results, actual discovered test counts, and screenshots. +If frontend source changes, run its typecheck and targeted ESLint. If new copy +is necessary, add five-language translations and run `pnpm run i18n:check`. + +## Files likely touched + +- `apps/backend/internal/orchestrator/task_operations.go` +- `apps/backend/internal/orchestrator/task_operations_resume_test.go` +- `apps/backend/internal/task/models/resume_safety.go` +- `apps/web/hooks/domains/session/use-session-resumption*.ts` +- `apps/web/e2e/tests/session/session-resume-recovery.spec.ts` +- `apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts` +- `apps/web/e2e/helpers/session-resume-recovery.ts` +- The existing public task recovery documentation. + +## Dependencies + +Task 01 establishes recoverable interruption state and prevents idle cancellation. + +## Risks + +Tokenless initialization cannot restore provider-native history. Retain stored +Kandev history and use the existing fallback contract. Broad reason matching +can silently restart deliberately stopped sessions. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/session-stall-visibility.md) +- [Design](../../specs/tasks/system-design/session-stall-visibility.md) +- `task_operations_resume_test.go` archive-cancellation and explicit-stop cases. +- `e2e/tests/task/archived-session-recovery.spec.ts` recovery fixture pattern. + +## Results + +Implemented exact legacy orphan-cancellation eligibility in task-open status and +resume handling. Rows with the recorded orphan reason use the existing +same-session recovery path when their runtime is resumable; missing, tokenless, +or non-resumable runtime data uses the prompt-free fresh-start fallback. Archive, +explicit-stop, deferred-owner, live-execution, missing-profile, and capacity +guards remain authoritative. Repeated status/open evaluation is safe and does +not replay the previous prompt. + +Added backend coverage for the eligibility matrix and resume guards. Existing +desktop and mobile recovery suites cover delayed startup, queued messages, +failure fallback, and same-session follow-up behavior. Public task and +configuration documentation now describes lazy focus recovery and the updated +stall behavior. + +The integrated review coverage pairs the active-sweep missing-row regression +with an orchestrator regression that queries the resulting recovery state +through the normal status contract and focuses the selected conversation +without replaying its previous prompt. The existing archive, explicit-stop, +ownership, and capacity guards remain in the same path. diff --git a/docs/plans/orphaned-session-open-recovery/task-03-interruption-warning.md b/docs/plans/orphaned-session-open-recovery/task-03-interruption-warning.md new file mode 100644 index 0000000000..938d0da3d7 --- /dev/null +++ b/docs/plans/orphaned-session-open-recovery/task-03-interruption-warning.md @@ -0,0 +1,144 @@ +--- +id: "03-interruption-warning" +title: "Keep interruption warnings until recovery succeeds" +status: done +wave: 3 +depends_on: + - "02-recover-orphan-conversations" +plan: "plan.md" +requirements: + - REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001 +acceptance_criteria: + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.2 + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.3 + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.4 + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.5 + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.6 + - AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.7 +system_design: + - ../../specs/tasks/system-design/interrupted-task-indicator.md +--- + +# Task 03: Keep interruption warnings until recovery succeeds + +## Summary + +Show a warning triangle for interrupted tasks until successful agent recovery. +Preserve the marker through delayed, failed, or blocked resume attempts. + +## In scope + +- Change both shared interruption icon paths from red circle to warning triangle. +- Replace STARTING-based marker clearing with confirmed recovery success. + Include prompt-free recovery to WAITING_FOR_INPUT and stale callback protection. +- Extend tests for restart marking, missing executor rows, repeated restart, + failed attempts, live removal, and existing icon precedence. +- Update public recovery documentation and affected scoped guidance with implementation. + +## Out of scope + +Other error icons, new task states, manual warning dismissal, and new mobile controls. + +## Acceptance + +1. Sidebar and task cards show the warning triangle while interrupted work remains unresumed. +2. Attempts retain the marker; successful recovery clears it across clients. Stale callbacks cannot clear newer interruption state. +3. Desktop and phone coverage proves shape, warning style, accessible meaning, persistence, and recovery lifecycle. + +## ASCII UI preview + +UI-02, [full plan](plan.md#ascii-ui-preview), covers all criteria in this work order. + +```text +Before, sidebar/card: (red !) Task title +After, sidebar/card: /!\ Task title + warning color +Phone drawer/card: /!\ Task title +During startup: [normal progress; marker retained] +After success: [normal task status] Task title +Failed attempt: [interruption retained + existing recovery error] +``` + +The warning occupies the existing status slot. Desktop and phone share the +semantic change, with existing layouts and scroll owners. Triangle shape and +accessible label are required; ASCII spacing is illustrative. No new touch +control is added. Use the phone task drawer as the shipped mobile exemplar. + +## Verification + +Write failing marker-lifecycle and shared-icon tests before implementation. +From the repository root, after the package's dependency installation: + +```bash +(cd apps/backend && go test ./internal/orchestrator -run 'Test.*(Interrupted|Startup|Retracked|Reconcile)' -count=1) +(cd apps/web && pnpm exec vitest run lib/ui/state-icons.test.tsx) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm exec eslint lib/ui/state-icons.tsx) +(cd apps/web && pnpm e2e:run --project chromium tests/task/task-interrupted-icon.spec.ts tests/session/session-resume-recovery.spec.ts -- --retries=0) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-task-interrupted-icon.spec.ts tests/session/mobile-session-resume-recovery.spec.ts -- --retries=0) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +Add the new mobile file in this work order. Use the configured mobile project +and touch task selection. Extend existing desktop coverage to include actual +board cards, not only the sidebar. Capture both views and assert shape/style, +accessible label, and removal after successful recovery. Keep delayed-resume +and failure cases in the existing recovery suites. Run projects sequentially. + +## Files likely touched + +- `apps/backend/internal/orchestrator/event_handlers_streaming.go` +- `apps/backend/internal/orchestrator/event_handlers_streaming_test.go` +- `apps/backend/internal/orchestrator/service.go` and restart tests. +- `apps/backend/internal/task/repository/sqlite/` if conditional metadata removal is needed. +- `apps/web/lib/ui/state-icons.tsx` and `state-icons.test.tsx` +- `apps/web/e2e/tests/task/task-interrupted-icon.spec.ts` +- `apps/web/e2e/tests/task/mobile-task-interrupted-icon.spec.ts` (new) +- `apps/web/e2e/tests/session/session-resume-recovery.spec.ts` +- `apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts` +- `docs/public/tasks-and-workflows.md` + +## Dependencies + +Task 02 provides the recovery path whose success clears the marker. + +## Risks + +Clearing on STARTING hides failed recovery. Clearing on every WAITING_FOR_INPUT +transition would also erase markers during restart reconciliation. Use confirmed +execution readiness, not state names alone. Preserve unrelated metadata keys. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/interrupted-task-indicator.md) +- [Design](../../specs/tasks/system-design/interrupted-task-indicator.md) +- `task-interrupted-icon.spec.ts` and `mobile-sidebar-workflow-completion-icon.spec.ts`. + +## Results + +Implemented warning-triangle rendering for the shared sidebar and task-card +state icon, including the accessible “Interrupted by restart” label and warning +focus styling on desktop and phone. The durable marker is retained through +STARTING entry, request acceptance, failed recovery, and stale snapshots. A +compare-and-set metadata removal clears it only after confirmed provider +recovery, so a late callback cannot erase a newer interruption marker. + +Added backend marker-lifecycle and metadata-CAS tests, frontend state-icon and +workflow-snapshot tests, and desktop/mobile E2E assertions for the rendered +triangle and warning color. Typecheck and targeted lint pass. + +The review remediation keeps the marker generation through failed, cancelled, +and tombstoned attempts. Empty or unreadable attempt snapshots cannot clear a +newer marker. Reconciliation publishes the committed marker through +`task.updated`, including its explicit interruption projection, so connected +clients see the warning immediately without a reload. The client task merge +also records every explicit marker update, preserving a newer false marker +after a complete interruption episode races an in-flight workflow snapshot. diff --git a/docs/public/configuration.md b/docs/public/configuration.md index e30d963287..7a8ca4f62b 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -181,11 +181,13 @@ startup setting, not a database or Settings value. sweep. When an unarchived task holds an active session with no live execution behind it and no session events or messages for longer than this threshold, Kandev emits a `task.stalled` event and logs a warning. After twice the -threshold of silence, the sweep cancels the orphaned sessions so the task -stops showing a phantom active session. The cancel is all-or-nothing per -task: it runs only when every active session of the task is orphaned and -past the grace window, so a session that still has a live execution blocks -the cancellation of its siblings rather than being swept along with them. +threshold of silence, the sweep returns each classified interrupted session to +`WAITING_FOR_INPUT` and preserves its conversation for recovery when you open +the task. It abandons only the observed unfinished turn, without reporting a +successful completion or advancing the workflow. Idle waiting sessions are +excluded, and recovery runs only when every active session of the task is +execution-less and past the grace window, so a live execution or idle sibling +blocks recovery for the task rather than being swept along with it. The value is source-specific: an invalid YAML duration fails configuration parsing, while a zero or negative YAML duration is rejected at startup with `tasks.stallDetectionThreshold must be positive`. An invalid or non-positive diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index 4729567ea5..549c984800 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -453,14 +453,21 @@ preference applies in two situations: prepared session and the agent stays stopped until you select **Start agent**. Opening the same task with the preference off keeps the workflow step's normal auto-start behavior. -- **Opening a task whose agent was stopped by a Kandev restart.** The session - is recovered and shown stopped instead of being resumed automatically. Select - **Start agent** to resume it. +- **Opening a task whose agent was interrupted by a Kandev restart.** Kandev + restores the selected session when the task opens, unless this preference is + enabled. It restores the existing conversation and workspace without + replaying the previous prompt. A new message continues that conversation, + including while the agent is starting. Opening the application does not + resume every interrupted task; each task starts recovery when you focus it. The preference only gates opening a task. Choosing **Start agent** (or a workflow step transition) always starts the agent as usual, and a failed or interrupted session still shows its recovery actions. +An interrupted task keeps a warning indicator until the agent confirms +recovery. Opening the task or starting a recovery attempt does not clear the +indicator; a failed attempt keeps it visible with the existing retry actions. + ## Answer clarification questions When an agent asks a clarification question, answer it from the question panel diff --git a/docs/specs/tasks/requirements/interrupted-task-indicator.md b/docs/specs/tasks/requirements/interrupted-task-indicator.md index a3a1bbe858..3d3eaf373b 100644 --- a/docs/specs/tasks/requirements/interrupted-task-indicator.md +++ b/docs/specs/tasks/requirements/interrupted-task-indicator.md @@ -1,192 +1,70 @@ --- -status: active +status: draft system: tasks created: 2026-08-02 -updated: 2026-08-09 +updated: 2026-09-21 owners: - kandev --- -# Interrupted Task Indicator Requirements + +# Interrupted task warning ## Overview -This document is the migrated task-system source for the capability. The source detail below remains authoritative while the system is migrated into separate requirement and design records. +A backend restart can interrupt work while preserving its conversation for recovery. +Users must be able to identify affected tasks until their agents resume. +The task system owns the durable interruption marker and its visible lifecycle. + +This revision changes the existing red error-style circle to a warning triangle. +It also prevents a failed resume attempt from erasing the interruption marker. +Implementation belongs to the [recovery package](../../../plans/orphaned-session-open-recovery/plan.md). ## Requirements -### REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001: Interrupted Task Indicator +### REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001: Interrupted task warning -**Intent:** Preserve the observable task or workflow behavior recorded by the legacy specification. +**Intent:** Show which tasks lost running work to a restart until recovery succeeds. -#### Acceptance criteria +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.1:** Superseded by criteria + 001.2 through 001.7. The previous migrated criterion required a red alert + circle and cleared the marker when startup began. +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.2:** When restart recovery finds + interrupted starting or running work, the system shall retain a durable + interruption marker on its unarchived task. Idle conversations and executions + that survived the restart shall not gain that marker. +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.3:** Unresumed interrupted tasks + shall show a warning triangle in the sidebar and task card. The shared + interruption indicator shall use the existing warning color instead of error red. + Its accessible label shall remain "Interrupted by restart". +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.4:** Opening Kandev, focusing a + task, or accepting a resume request shall not clear the marker. Failed, + cancelled, or blocked recovery attempts shall preserve it. Successful agent + recovery shall clear it and update visible task surfaces without a reload. +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.5:** The marker shall survive + reload, reconnect, repeated restart, and updates that omit interruption state. + A stale callback shall not clear a newer interruption marker. +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.6:** Phone task navigation and + board cards shall show the same warning and recovery lifecycle. The meaning + shall remain accessible without hover or reliance on color alone. +- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.7:** Existing permission, + clarification, current activity, and explicit terminal-state precedence shall + remain. Temporary startup progress shall not erase the durable warning. + Failed recovery shall leave the interruption state available alongside the + actual recovery error. Explicit failure and cancellation icons remain unchanged. -- **AC-TASKS-INTERRUPTED-TASK-INDICATOR-001.1:** When a consumer uses this capability, the system shall provide the observable behavior and exclusions documented below. +## Boundaries -## Migrated source detail +Recovery follows [the interruption contract](restart-orphaned-session-terminalization.md). +The warning does not cancel work, start an agent, replay a prompt, or advance +its workflow. Successful recovery can clear the warning before a new message +arrives; restoring the agent and continuing work are separate actions. -## Why - -When Kandev crashes, is killed, or restarts while tasks are mid-turn, those -sessions are interrupted: the agent process is gone and the turn was cut short. -Startup reconciliation already flips such sessions to `WAITING_FOR_INPUT` and -the task to `REVIEW`, so after a restart an interrupted task is visually -indistinguishable from a task that finished its turn and is waiting normally. -Operators cannot tell which tasks lost work to the crash. This spec adds a -durable "interrupted" marker set at startup for tasks whose sessions were -mid-turn, surfaced as a red icon in the task list. - -## What - -- At startup, when reconciliation finds a session that was `STARTING` or - `RUNNING` (mid-turn) when the backend died, the owning task is marked - interrupted with a durable task metadata key `interrupted_at` holding the - reconciliation timestamp (RFC 3339 UTC). -- Sessions that were `WAITING_FOR_INPUT`, `IDLE`, `CREATED`, or terminal at - startup are not marked: they were not interrupted mid-work. -- Archived or deleted tasks are not marked (they do not appear in the active - task list). -- The task API exposes `interrupted: true` on the task DTO while the marker is - present; it is derived from the metadata key, so no schema migration is - required. -- Task list surfaces render a red alert icon beside the task while - `interrupted` is true: sidebar task rows, the mobile task-switcher drawer - (shared row rendering), kanban board cards, graph/swimlane nodes, the rich - task-list row, and the open-task state icon. The icon carries an accessible - label and tooltip ("Interrupted by restart"). -- The marker clears when a session of the task next enters `STARTING` or - `RUNNING` — work has resumed, so the task is no longer interrupted — and the - cleared state is published over the existing `task.updated` event so open - clients drop the icon live. -- The red icon renders instead of the idle/done affordances only. Pending - permission, pending clarification, generating/background activity, and - running spinners keep their existing precedence; terminal `COMPLETED`, - `FAILED`, and `CANCELLED` states keep their existing icons. - -## Data model - -No schema change. The marker lives in the existing `tasks.metadata` JSON -column under a new reserved key: - -``` -MetaKeyInterruptedAt = "interrupted_at" // value: RFC 3339 UTC timestamp string -``` - -Task metadata is already serialized into the API task DTO and already -survives restarts. The repository's existing `SetTaskMetadataKey` / -`RemoveTaskMetadataKey` helpers (concurrent-key-safe JSON patch, both SQLite -and Postgres) are the only writers; nothing else may mutate the key. - -## API surface - -- `v1.Task` gains `interrupted bool json:"interrupted,omitempty"`. -- The field is computed at DTO conversion time in both task serializers - (`internal/task/dto` and `internal/task/models` ToAPI) as - `metadata["interrupted_at"] != nil`. It is not a stored column. -- The field flows through every existing task payload automatically: boot - payload, `kanban.update`, `task.created`, `task.updated`, `task.state_changed`, - and `GET /api/v1/tasks/:id`. -- No new HTTP route, WebSocket action, or event type. - -## State machine - -The marker follows this lifecycle: - -- `unmarked`: no `interrupted_at` key. -- `marked`: `interrupted_at` present; task DTO reports `interrupted: true`. -- `cleared`: key removed; DTO reports `interrupted: false` (omitted). - -Transitions: - -- `unmarked -> marked` at startup reconciliation when a preserved - `executors_running` row's session was `STARTING` or `RUNNING` and the task - exists and is not archived. Overwriting an existing key is safe: a marked - task cannot have a mid-turn session at the next startup unless it was - resumed and crashed again. -- `marked -> cleared` when any session of the task transitions into - `STARTING` or `RUNNING` (launch, resume, prompt dispatch, agent-ready - wake). The transition chokepoint in the orchestrator - (`updateTaskSessionStateWithHook` / `setSessionStarting`; the implementer - verifies the funnel covers every start path) removes the key and publishes - `task.updated` only when the key was actually removed. -- Terminal task/session transitions do not clear the marker; terminal state - icons take precedence over the red icon, and archiving/deleting the task - removes the row with its metadata. - -## Permissions - -None. The marker is display metadata on a task the viewer can already see. It -is user-writable only through the generic task metadata update surface and has -no effect beyond the icon. - -## Failure modes - -- A metadata write fails during reconciliation: the marker is skipped with a - warning and startup continues; the task simply shows no icon until the next - restart. -- A metadata removal fails during a session start: the marker survives and the - next successful start transition clears it; the icon may briefly persist - behind the running spinner, which takes precedence. -- Remote executors: the backend connection is severed even if the remote agent - is still alive. The task is marked. If the remote status poll later reports - the session running again and the session transitions, the marker clears; if - not, the marker stays until the next resume. This is acceptable — from - Kandev's perspective the turn was cut off. -- An already-open client during startup reconnects to the boot payload, which - carries the marker; no startup-time event is required. -- A `task.updated` event that omits `interrupted` must not wipe a set marker in - client state (frontend merge preserves the field when the payload omits it, - mirroring `foreground_activity`). - -## Persistence guarantees - -- The marker is durable in `tasks.metadata` and survives any number of - restarts until a session start clears it. -- Clearing is a normal key removal; a crash between removal and the - `task.updated` publish leaves the client icon stale until the next payload, - never a corrupt row. - -## Scenarios - -- **GIVEN** a task whose primary session was `RUNNING` when the backend died, - **WHEN** the backend starts and reconciliation runs, **THEN** the task's - metadata contains `interrupted_at`, the task DTO reports `interrupted: true`, - and the sidebar row, board card, graph node, rich list row, and mobile - switcher row show the red alert icon. -- **GIVEN** that task also has a stored `generating` task summary, **WHEN** reconciliation changes - the primary session to `WAITING_FOR_INPUT`, **THEN** the summary clears `generating` and the red - alert icon is not hidden by a stale running spinner. -- **GIVEN** a task whose session was `WAITING_FOR_INPUT` when the backend - died, **WHEN** reconciliation runs, **THEN** the task is not marked and shows - no red icon. -- **GIVEN** an archived task with a mid-turn session row, **WHEN** - reconciliation runs, **THEN** the task is not marked. -- **GIVEN** a marked task, **WHEN** the user resumes the session and it enters - `STARTING`/`RUNNING`, **THEN** the marker is removed, `task.updated` is - published, and every surface drops the red icon (running spinners take over). -- **GIVEN** a marked task that is never resumed, **WHEN** the backend restarts - again, **THEN** the marker survives and the icon still renders. -- **GIVEN** a marked task whose state is `COMPLETED`, **WHEN** any surface - renders it, **THEN** the green done icon wins and no red icon is shown. -- **GIVEN** a `task.updated` event without an `interrupted` field for a marked - task, **WHEN** a client merges it, **THEN** the client keeps the marker. - -## Out of scope - -- Changing the task or session state machine (no new task state; the task - remains `REVIEW` with a `WAITING_FOR_INPUT` session after reconciliation). -- Marking sessions or tasks `FAILED` on interruption; resumability behavior is - unchanged. -- Persisting the marker as a typed column; metadata is the single source. -- Office agent cards and the Office dashboard surface, which use their own - status presentation. -- Reconstructing detached remote-work liveness after restart. -- A user-facing action to clear or dismiss the icon manually. +No new task state, manual dismiss action, or Office dashboard behavior is added. +Reuse the existing marker and translated label. This is a task-owned indicator, +not a separate UI-owned requirement. ## Related -- [Task Runtime Cleanup](../system-design/runtime-cleanup.md) — startup reconciliation semantics - the marker hooks into. -- [Sidebar Task Completion Icons](../../ui/requirements/sidebar-task-completion-icons.md) — - the shared task-row icon precedence this feature extends. -- [Backend Runtime-State Ownership](../../../plans/backend-runtime-state-ownership/plan.md) — - startup publication that clears stale generating activity. \ No newline at end of file +- [System design](../system-design/interrupted-task-indicator.md) +- [Task recovery design](../system-design/restart-orphaned-session-terminalization.md) +- [Sidebar task completion icons](../../ui/requirements/sidebar-task-completion-icons.md) diff --git a/docs/specs/tasks/requirements/restart-orphaned-session-terminalization.md b/docs/specs/tasks/requirements/restart-orphaned-session-terminalization.md index fd23e45a1a..adb06ec570 100644 --- a/docs/specs/tasks/requirements/restart-orphaned-session-terminalization.md +++ b/docs/specs/tasks/requirements/restart-orphaned-session-terminalization.md @@ -6,42 +6,61 @@ owners: - kandev --- -# Restart-Orphaned Session Terminalization Requirements +# Session recovery after backend interruption ## Overview -A task session records the lifecycle of one agent conversation turn. When the -backend process dies while a session's turn is open — a graceful restart, a -crash, or a kill — the component that would have transitioned the session to a -terminal state dies with it. No other actor retries that transition, so the -session row stays in an active state forever and the task appears to wait for -an agent that already finished. - -The tasks system owns the task-session lifecycle, so it owns this -reconciliation: the system itself must detect and terminalize sessions that -lost their backing actor, without depending on the dead process, the agent, or -a human SQL edit. - -## Terminology - -- **Active session state:** A `task_sessions.state` value that implies work is - in flight: `CREATED`, `STARTING`, `RUNNING`, or `WAITING_FOR_INPUT`. -- **Terminal session state:** A state a session never leaves: `COMPLETED`, - `FAILED`, or `CANCELLED`. -- **Orphaned session:** An active-state session whose transition actor is gone: - the backend that would complete or fail its turn terminated, and no live - agent execution backs the session anymore. -- **Live execution:** An agent execution the current backend process tracks in - its in-memory execution store. A session backed by a live execution is not - orphaned even when its row is stale. -- **Launch grace window:** The interval between a session row entering - `STARTING` and its execution necessarily being visible in the in-memory - store. A session inside this window is not eligible for orphan detection. -- **Reconciliation sweep:** The periodic pass that lists candidate sessions - and terminalizes the orphaned ones. +A backend restart can interrupt an agent turn. The conversation remains available +for recovery. Opening Kandev must not cancel interrupted tasks or resume every +agent at once. Focusing a task restores its selected conversation. A new user +message continues the work without replaying the interrupted prompt. ## Requirements +### REQ-TASKS-RESTART-ORPHAN-SESSIONS-002: Interrupted conversations remain resumable + +This requirement supersedes requirement 001's automatic cancellation policy. + +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.1:** After a backend interruption, + an unarchived session that lost its execution shall remain recoverable. + Reconciliation shall represent it as waiting for recovery, never cancelled + solely because its execution is absent or its activity is old. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.2:** Opening Kandev shall not start + all interrupted agents. Focusing an eligible task shall restore its selected + agent and conversation under the existing auto-start preference. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.3:** Focusing a task shall not send + or replay a prompt. A subsequent user message shall continue that conversation + through normal prompt delivery, including during recovery startup. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.4:** Recovery shall preserve session, + workspace, transcript, and available provider resume identity. Missing runtime + records shall not cause cancellation or silent conversation replacement. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.5:** Surviving executions, recent + launches, newer turns, and healthy siblings shall remain untouched. Unknown + liveness or failed reads shall defer reconciliation. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.6:** Settling an interrupted turn + shall not complete the task, advance its workflow, or report successful work. + The session shall remain available for the next message. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.7:** Explicit stops, archive rules, + authorization, capacity admission, and deferred-launch ownership shall retain + their existing authority. Neither sweep shall override these rules. +- **AC-TASKS-RESTART-ORPHAN-SESSIONS-002.8:** Desktop and phone shall support + focus, recovery, and follow-up messaging without an orphan-cancellation banner. + Actual recovery failures shall retain explicit retry actions and stored history. + +## Interruption visibility + +Interrupted tasks retain the [interruption warning](interrupted-task-indicator.md) +until successful agent recovery. The warning uses the shared triangle and +warning color on sidebar rows and task cards, including phone surfaces. +Focus or an unsuccessful resume attempt alone does not clear it. + +## Historical requirement + +Requirement 001 and its criteria below record the superseded cancellation policy. +They must not drive new implementation. The pending +[recovery package](../../../plans/orphaned-session-open-recovery/plan.md) +replaces that policy with requirement 002. + ### REQ-TASKS-RESTART-ORPHAN-SESSIONS-001: Sessions orphaned by a backend restart reach a terminal state **Intent:** A backend restart must not leave task sessions stuck in an active @@ -94,10 +113,10 @@ that the task board stays truthful and the task can move on. `is_primary` flag, so the status-summary projection keeps the durable primary-session assignment. -## Out of scope +## Historical exclusions -- Resuming or replaying a turn that was interrupted mid-flight; the orphaned - session is terminalized, not resumed. +- The sweep does not resume or replay interrupted turns. Later task opening + follows the [orphan recovery contract](session-stall-visibility.md). - Detecting orphans of already-archived tasks; the archived-task reconciliation pass owns those. - Agent-side heartbeats or adapter-side turn-state persistence. diff --git a/docs/specs/tasks/requirements/session-stall-visibility.md b/docs/specs/tasks/requirements/session-stall-visibility.md index a4c9a61d56..9573f593ed 100644 --- a/docs/specs/tasks/requirements/session-stall-visibility.md +++ b/docs/specs/tasks/requirements/session-stall-visibility.md @@ -1,5 +1,5 @@ --- -status: active +status: draft system: tasks created: 2026-09-20 owners: @@ -10,14 +10,15 @@ owners: ## Overview -An unarchived task whose session is in an active DB state (CREATED, STARTING, -RUNNING, WAITING_FOR_INPUT) but whose backing execution is gone — after a -backend restart, a lost executor, or a launch that never registered — is -invisible to every event-driven surface: no request path owns the session, -no actor will ever advance it, and silence produces no signal. This document -defines the observable contract for detecting that state and healing it, -closing the incident where four finished review tasks sat silently in -`WAITING_FOR_INPUT` for hours with no operator-visible signal. +Sessions with unfinished work can lose their execution after a restart or +executor failure. The system detects these stalls and settles the abandoned work. +A conversation waiting for user input can also have no execution after normal +idle cleanup. That condition is not a stall. + +Opening a recoverable conversation restores its agent without requiring a +manual Resume action. The task system owns both classification and recovery. +The September 21 revision is implemented in the +[recovery plan](../../../plans/orphaned-session-open-recovery/plan.md). ## Terms @@ -25,10 +26,13 @@ closing the incident where four finished review tasks sat silently in or WAITING_FOR_INPUT. - **Live execution**: an execution registered in the backend's in-memory execution store for the session. +- **Idle conversation**: a session waiting for user input with no unfinished + turn. Its execution can be absent after idle cleanup or restart. +- **Stall candidate**: an active session that is not an idle conversation. - **Event silence**: no persisted activity for the session — the newer of the session row's last update and its newest persisted message. - **Stall episode**: the continuous period during which one session is - active, execution-less, and event-silent past the stall threshold. An + a stall candidate, execution-less, and event-silent past the stall threshold. An episode ends when the session leaves that state (terminal transition, healing, or a live execution reappearing). @@ -43,7 +47,7 @@ silently forever. #### Acceptance criteria - **AC-TASKS-SESSION-STALL-VISIBILITY-001.1:** When an unarchived task holds - an active session with no live execution and no persisted session activity + a stall candidate with no live execution and no persisted session activity for longer than the configurable stall threshold (default 2h), the system shall emit a `task.stalled` event and a warning log naming the task and its stalled sessions, at most once per session per stall episode. @@ -51,15 +55,13 @@ silently forever. execution, transitions to a terminal state, or otherwise shows persisted activity again, its stall episode shall end, and a later stall of the same session shall be reported again as a new episode. -- **AC-TASKS-SESSION-STALL-VISIBILITY-001.3:** When an active session with - no live execution has been silent for at least twice the stall threshold, - and every active session of the same task is equally orphaned and silent, - the system shall cancel those sessions with the reason `orphaned session` - and publish the session-state change event clients observe. -- **AC-TASKS-SESSION-STALL-VISIBILITY-001.4:** The healing cancellation - shall never cancel a session with a live execution, including one that - registered while the sweep was evaluating the task, and shall never cancel - a session outside the set the sweep classified as orphaned. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.3:** Superseded by + `REQ-TASKS-RESTART-ORPHAN-SESSIONS-002`. Execution loss and prolonged silence + shall lead to recoverable interruption settlement, not automatic cancellation. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.4:** The healing transition shall + never alter a session with a live execution, including one that registered + while the sweep was evaluating the task, and shall never alter a session + outside the set the sweep classified as orphaned. - **AC-TASKS-SESSION-STALL-VISIBILITY-001.5:** When a session's activity clock cannot be read (for example a transient database read failure), the system shall neither report a stall nor heal for that evaluation, rather @@ -67,6 +69,30 @@ silently forever. - **AC-TASKS-SESSION-STALL-VISIBILITY-001.6:** When the stall detection threshold cannot be verified against the live-execution registry, the sweep shall do nothing rather than guess that executions are gone. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.7:** The system shall preserve idle + conversations regardless of silence duration or execution absence. It shall + not report them as stalled or cancel them as orphaned. If unfinished-turn + evidence is unavailable, classification shall wait for a later evaluation. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.8:** When a user opens a recoverable + orphan-cancelled conversation, the system shall automatically restore the agent + in that session. This applies to existing cancellations and both reconciliation + passes. Recovery shall preserve conversation history and workspace identity. + It shall not replay a prompt, create a replacement session, or advance the workflow. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.9:** Automatic recovery shall honor + auto-start prevention, archive status, authorization, capacity, and deferred + launch ownership. Explicit user stops and unrelated terminal states shall + retain their existing recovery rules. Repeated opening shall not duplicate execution. +- **AC-TASKS-SESSION-STALL-VISIBILITY-001.10:** Desktop and phone task views + shall use normal recovery progress and conversation controls. Successful recovery + shall remove the orphan warning. If recovery fails, existing recovery actions + shall remain available without an automatic retry loop. + +## Related contracts + +- [Session-open recovery](queued-session-ownership.md) +- [Auto-start preference](prevent-agent-autostart-on-open.md) +- [Restart reconciliation](restart-orphaned-session-terminalization.md) +- [System design](../system-design/session-stall-visibility.md) ## Exclusions diff --git a/docs/specs/tasks/system-design/interrupted-task-indicator.md b/docs/specs/tasks/system-design/interrupted-task-indicator.md new file mode 100644 index 0000000000..dd16224311 --- /dev/null +++ b/docs/specs/tasks/system-design/interrupted-task-indicator.md @@ -0,0 +1,75 @@ +--- +status: draft +system: tasks +requirements: + - REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001 +--- + +# Interrupted task warning design + +## Ownership and data + +Keep `tasks.metadata.interrupted_at` and the derived task DTO `interrupted` flag. +No schema, route, or event type is added. Startup reconciliation and periodic +interruption recovery mark lost mid-turn work, including missing executor rows. +Normal idle conversations and surviving executions remain unmarked. + +Use concurrent-key-safe repository operations and existing archive guards. +Preserve the marker through waiting-state recovery and subsequent restarts. +Client merges retain existing interruption state when a partial payload omits it. +Successful clearing uses the existing authoritative update contract. + +## Marker lifecycle + +Current `event_handlers_streaming.go` clears the marker at STARTING entry. +Move that clearing to confirmed successful recovery through the existing +provider-ready or accepted-running paths. Audit every `clearTaskInterruptedMarker` +call, including `updateTaskSessionStateWithHook`, `setSessionStarting`, and +execution-correlated activity handlers. STARTING alone proves only an attempt. + +A successful prompt-free resume can end in WAITING_FOR_INPUT. Clear the marker +on that success without requiring a new user prompt. Queued, refused, failed, +and cancelled attempts retain it. Existing activity spinners can temporarily +replace the warning while startup is in progress; the durable marker survives. + +Use existing execution correlation and compare the observed interruption value +when clearing it. An old readiness callback must not erase a newer interruption. +Keep the update and publication retryable through the existing event path. +Publish `task.updated` only after a successful marker change. Never clear the +marker from a browser mount, navigation handler, or optimistic STARTING projection. + +## Shared rendering + +In `apps/web/lib/ui/state-icons.tsx`, update both `TASK_INTERRUPTED_ICON` and +`InterruptedTaskIcon`: use `IconAlertTriangle` and the existing `STYLE_WARNING` +color (`text-yellow-500`). Replace the error-colored focus ring consistently. +Keep the test ID `task-state-interrupted` and translated +`common:interruptedByRestart` label. Do not change unrelated error icons. + +Consumers include sidebar rows, board cards, mobile task-switcher rows, graph +nodes, rich task lists, and the open-task header. Using the shared renderer +keeps their appearance consistent. Retain current permission, clarification, +activity, and explicit terminal-state precedence. + +## Desktop and phone + +Desktop keeps the icon in the current task-row and card status position. +Phone reuses the task drawer and board card status slot. No new overlay or +control is needed. Keep the accessible label and visible triangle shape. +Required meaning must not depend on tooltip hover; the task name remains primary. +Existing layout, scroll ownership, safe areas, and touch behavior remain intact. + +## Verification and mapping + +| Requirement | Design sections | +| --- | --- | +| REQ-TASKS-INTERRUPTED-TASK-INDICATOR-001 | Ownership and data; Marker lifecycle; Shared rendering; Desktop and phone | + +Extend the marker lifecycle tests in `event_handlers_streaming_test.go` and +startup reconciliation tests. Extend `state-icons.test.tsx` to assert triangle, +warning styling, accessible label, and precedence. Browser coverage must verify +sidebar and board rendering, reload persistence, failed recovery retention, +and successful recovery removal on desktop and phone. + +The [work order](../../../plans/orphaned-session-open-recovery/task-03-interruption-warning.md) +contains exact commands and the UI preview. diff --git a/docs/specs/tasks/system-design/restart-orphaned-session-terminalization.md b/docs/specs/tasks/system-design/restart-orphaned-session-terminalization.md index 5a21b7e14f..19a2eb21af 100644 --- a/docs/specs/tasks/system-design/restart-orphaned-session-terminalization.md +++ b/docs/specs/tasks/system-design/restart-orphaned-session-terminalization.md @@ -1,130 +1,91 @@ --- -status: current +status: draft system: tasks requirements: - - REQ-TASKS-RESTART-ORPHAN-SESSIONS-001 + - REQ-TASKS-RESTART-ORPHAN-SESSIONS-002 --- -# Restart-Orphaned Session Terminalization System Design +# Session recovery after backend interruption -## Purpose and boundaries +## Ownership and control flow -The tasks system owns the task-session lifecycle, including the transition of -a session into a terminal state. This design covers the periodic sweep that -terminalizes `STARTING`/`RUNNING` sessions of unarchived tasks that no live -in-memory agent execution backs — the residue of a backend process that died -mid-turn. +The task system owns recovery state. Both reconciliation passes must preserve +conversation availability after execution loss. This replaces the cancellation +policy shipped in PR #3833 and the equivalent healing action in PR #3832. -It extends the existing reconciliation loop -(`internal/task/service/archived_session_reconciliation.go`) with a second -pass; it does not introduce a new loop, new persistence, or adapter changes. -Adjacent contracts this design uses but does not own: the agent runtime's -in-memory execution store (executors system), session-ceiling reservations -(tasks system, reservation contract), and the archived-task reconciliation -pass that owns archived tasks' stranded sessions. +Use `reconcileActiveSessionOnStartup` in `internal/orchestrator/service.go` as +the behavior precedent. It preserves runtime identity, settles abandoned turns, +and projects non-surviving active sessions as `WAITING_FOR_INPUT` for lazy recovery. +It does not replay the prompt. Surviving executions remain running. -## Requirement mapping +The periodic recovery path must also handle sessions without an executor row. +Keep the existing ticker, launch grace, runtime-aware liveness protections, +retry behavior, and per-session candidate identity. Replace orphan cancellation +with a conditional transition to recoverable `WAITING_FOR_INPUT`. -| Requirement | Design section | +## State and persistence + +No new session-state enum is required. Preserve transcript, workspace, resume +token, execution identity, and primary-session assignment. Clear the current +orphan cancellation error on successful recovery. Preserve historical diagnostics. + +Settle only the abandoned turn identified by the recovery snapshot. Use the +existing interruption cleanup behavior, not archive cancellation effects. +Do not publish successful `turn.completed`, advance the workflow, or cancel +healthy siblings. Retire stale execution capacity claims through existing owners. +Do not expire questions or clear queued work solely because the backend restarted. + +The final write must compare observed state, activity, and turn identity. +Recheck live execution and archive/stop ownership at the transition boundary. +A completed or replaced turn, renewed activity, or concurrent explicit stop +rejects the stale recovery attempt. No database transaction spans runtime I/O. +Recovery must be retryable after partial progress without duplicating effects. + +`WAITING_FOR_INPUT` without an unfinished turn is normal idle state. The active +stall sweep must neither alert nor alter it, regardless of elapsed silence. +For interrupted work, retain useful stall diagnostics and recoverable settlement. +The active pass retains its all-active-session eligibility boundary. An idle +sibling blocks that task-wide action; the restart pass remains per-session. + +## Focus and message delivery + +On task focus, use existing `GetTaskSessionStatus` and `session.launch` resume +flow with automatic admission. Opening the application does not eagerly start +all interrupted sessions. Focusing restores provider context but sends no prompt. + +Use normal queued prompt delivery for a message sent during startup. Deliver +that new message once after readiness. Never resend the old prompt to simulate +continuation. Recover legacy `CANCELLED` plus exact orphan-reason rows through +the compatibility exception in the +[stall recovery design](session-stall-visibility.md#recovery-when-opening-a-task). +Other terminal states retain their existing rules. + +When provider context cannot be restored, retain the same Kandev transcript and +workspace. Expose the existing recovery fallback without claiming native context +restoration. Honor auto-start prevention and all existing launch ownership guards. + +## Interruption marker + +Set or preserve `interrupted_at` for lost mid-turn work. Idle reconciliation +must not clear it. Successful correlated provider recovery clears the marker; +STARTING entry alone does not. Follow the +[warning design](interrupted-task-indicator.md) for publication, stale callbacks, +and the shared sidebar/card renderer. + +## UI and verification + +Reuse the current desktop chat and dedicated phone task layout. Show normal +recovery progress and the retained transcript. Use existing retry controls for +actual failures. See the [plan](../../../plans/orphaned-session-open-recovery/plan.md) +for previews and the desktop/phone test matrix. + +| Requirement | Design sections | | --- | --- | -| `REQ-TASKS-RESTART-ORPHAN-SESSIONS-001` | [Control flow](#control-flow), [Failure and recovery](#failure-and-recovery) | +| REQ-TASKS-RESTART-ORPHAN-SESSIONS-002 | Ownership and control flow; State and persistence; Focus and message delivery; UI and verification | -## Components and responsibilities +## Related decision -- **Reconciliation loop** (`Service.StartArchivedSessionReconciliationLoop`): - ticks every minute and runs both passes — the archived pass first, then the - orphan pass. -- **Orphan pass** (`Service.runOrphanedSessionReconciliation`): lists stale - candidates, filters by live-execution absence, cancels each, runs - post-cancellation effects. -- **Session repository** (`sqlite.Repository`): candidate query - `ListStaleRunningSessionsOnUnarchivedTasks` and per-session cancel - `CancelRunningTaskSessionByID`. Not part of the `SessionRepository` - interface; reached through the narrow `orphanedSessionRepository` - capability so test doubles stay unchanged. -- **Execution-liveness seam** (`TaskExecutionLivenessChecker`): reports - whether a session has a live in-memory execution. Satisfied by the - lifecycle adapter (`HasLiveExecution`), which answers from the in-memory - store only and never lazily creates an execution. -- **Shared post-cancellation effects** (`Service.notifyCancelledSessions`): - extracted from the archive cancellation path and reused verbatim: - clarification expiry, parked-projection clearing, ceiling release, and the - `session.state_changed` publish. - -## Data and contracts - -- Candidate query joins `task_sessions` to `tasks` and selects rows where - `ts.state IN ('STARTING','RUNNING')`, `ts.updated_at < staleBefore`, and - `t.archived_at IS NULL`, ordered by `updated_at` ascending. -- `staleBefore` is `now - (constants.AgentLaunchTimeout + 5m grace - allowance)`: one full launch budget plus the start-deadline allowance, read - (not copied) from `constants.AgentLaunchTimeout` so an operator-raised - preparation budget is honored. The same cutoff is passed into the cancel - statement and re-asserted as `updated_at < staleBefore` there, so a row - refreshed between the candidate read and the write no longer matches. -- `CancelRunningTaskSessionByID(sessionID, reason, staleBefore)` is an - `UPDATE ... RETURNING` over rows still in `STARTING`/`RUNNING` whose - `updated_at` still predates the cutoff; it returns `nil` when the session - raced to another state or was refreshed, and the write detaches from the - caller context with a 10-second timeout. The RETURNING clause selects - `is_primary` alongside the other event-payload fields, so the published - cancellation event carries the durable primary flag. -- Cancellation reason constant: `models.SessionOrphanedCancelReason` - (`"orphaned by backend restart"`), distinct from the archive reasons; - `IsArchiveCancelReason` returns false for it. - -## Control flow - -1. Every tick, pass 2 runs after the archived pass. -2. If no execution-liveness checker is wired, the pass is inert: absence - from the store is the only dead signal, and a nil checker can never prove - a session unbacked. -3. List candidates with the stale-before cutoff computed from the launch - budget. -4. For each candidate, re-check `HasLiveExecution(session.ID)` at the moment - of the write — a launch that raced the grace window since the read must - not be reaped. -5. Cancel via `CancelRunningTaskSessionByID`; the statement re-asserts both - the state set (`STARTING`/`RUNNING`) and the staleness cutoff - (`updated_at < staleBefore`), so a row refreshed by an in-flight launch - between the liveness check and this write matches nothing; on `nil` - (raced or refreshed) skip; on error warn and let the next tick retry. -6. Run the shared post-cancellation effects with the orphan reason. - -## Failure and recovery - -- A failed candidate read logs an error and returns; the next tick retries. -- A failed cancel logs a warn and continues to the next candidate; the next - tick retries that session. The sweep never gives up for as long as the - process runs. -- The cancel write is context-detached (`context.WithoutCancel` + 10s bound), - so the terminal transition survives a client disconnect and cannot block a - sweep pass indefinitely on a locked writer. -- Pass 2 is a no-op on clean trees: fresh rows are filtered by the grace - window, live-backed rows by the liveness check, archived tasks' rows by the - archived-pass filter, and terminal rows by the candidate query. - -## Persistence - -No schema change. The sweep writes only `task_sessions.state`, -`error_message`, `completed_at`, and `updated_at`. Survivors re-tracked by -startup recovery are in the in-memory store and stay untouched, so the sweep -composes with agent-survival re-tracking rather than racing it. - -## Security - -No new trust boundary. The sweep runs in-process with the task service's own -credentials and touches only `task_sessions` rows of unarchived tasks. - -## Observability - -- `task-session reconciliation loop started (every 1 minute)` at loop start. -- `orphaned-session reconciliation: found candidates` / `terminalized - unbacked session` (Info, with task and session IDs, previous state). -- `failed to list candidates` / `failed to cancel session` (Error/Warn with - the underlying error). - -## Related decisions - -- Issue #3711 and its approved fix plan: extend the existing sweep rather - than drain-on-shutdown, turn-state persistence, heartbeats, or manual SQL. +The [session-open decision](../../../decisions/2026-09-18-session-open-resumes-conversation.md) +already separates provider recovery from prompt dispatch. The user's September 21 +clarification extends that behavior to interrupted running tasks. The requirement +and this design preserve the rationale without a separate ADR. diff --git a/docs/specs/tasks/system-design/session-stall-visibility.md b/docs/specs/tasks/system-design/session-stall-visibility.md index b1bb8e7de8..e12f02e23c 100644 --- a/docs/specs/tasks/system-design/session-stall-visibility.md +++ b/docs/specs/tasks/system-design/session-stall-visibility.md @@ -1,5 +1,5 @@ --- -status: current +status: draft system: tasks created: 2026-09-20 owners: @@ -32,7 +32,12 @@ Each tick: 1. `ListUnarchivedTasksWithActiveSessions` returns candidate tasks. 2. Per task, `ListActiveTaskSessionsByTaskID` loads the active sessions and the registry reports live ones (`LiveSessionIDsForTask`); sessions - outside that snapshot are orphaned. + outside that snapshot need lifecycle classification. A missing execution + alone does not prove a stall. + For `WAITING_FOR_INPUT`, read the active turn through `GetActiveTurn`. + A nil turn identifies an idle conversation and excludes it from detection + and healing. A read error skips the task for this tick. + `IDLE` remains outside the active-session query. 3. Each orphaned session's activity clock is the newer of its row's `updated_at` and its newest `task_session_messages` row (`GetLastMessageTimeBySessionIDs`). A read failure skips the task this @@ -43,13 +48,15 @@ Each tick: 5. Stalled sessions not yet reported in this episode publish one `task.stalled` event; delivery is recorded only after the publish succeeds, so a failed publish retries next tick (AC .1). -6. A healable set covering every active session of the task re-checks - liveness at the cancellation boundary and then cancels exactly the - classified session IDs through `finalizeCancelledSessionIDs`, reusing the - archived pass's transition: `CANCELLED` with `SessionOrphanedCancelReason` - (`"orphaned session"`), clarification expiry, parked-projection cleanup, - session-ceiling release, and one `session.state_changed` per session - (AC .3, .4). +6. A recoverable set covering every active session rechecks liveness and + settles abandoned work to waiting state under the + [interruption recovery design](restart-orphaned-session-terminalization.md). + Neither this pass nor the restart pass writes `CANCELLED` for execution loss. + +Keep every active session in the task-wide eligibility denominator. An idle +sibling blocks this task-wide action. The restart pass remains per-session. +Carry the observed activity and turn identity through the conditional recovery +write. A completed or replaced turn rejects a stale recovery snapshot. ## Session activity clock @@ -70,10 +77,10 @@ on the sweep's single goroutine and resets when no candidates remain. - No registry wired: the pass skips entirely (AC .6). - Activity read failure: the task is skipped this tick (AC .5). -- Cancellation write failure: retried with the archived pass's bounded +- Recovery write failure: retried with the archived pass's bounded in-line retry, and the next tick retries again. -- A live execution appearing between classification and the cancellation - boundary aborts the heal; the ID-scoped predicate structurally excludes +- A live execution appearing between classification and the recovery + boundary aborts the transition; the ID-scoped predicate structurally excludes sessions that became active after classification (AC .4). ## Configuration @@ -84,12 +91,67 @@ on the sweep's single goroutine and resets when no candidates remain. negative YAML durations are rejected at startup; invalid or non-positive environment values fall back to `2h`. +## Recovery when opening a task + +Extend `GetTaskSessionStatus` in `internal/orchestrator/task_operations.go` with +an exact orphan-reason exception for `CANCELLED`. Use +`models.SessionOrphanedCancelReason`; do not classify arbitrary cancellation +messages as recoverable. Keep archive cancellation reasons distinct. + +Use the existing archive-recovery branch as the implementation pattern: + +- With a supported resume token, validate the profile and return + `is_resumable=true`, `needs_resume=true`, and a distinct orphan recovery reason. +- Without a usable token, use existing same-session initialization eligibility. + Preserve stored messages and workspace identity. Do not claim provider context + was restored when the provider cannot resume it. +- Missing profiles and unavailable prerequisites retain actionable recovery + errors. Do not mark an impossible launch as automatic recovery. +- Existing live executions return the running status without a launch request. + +The frontend continues through `use-session-resumption-operations.ts` and +`session.launch` with `intent=resume`. Opening sends no prompt and never invokes +the new-session action. Keep the automatic admission flag, existing request +deduplication, archive fences, preference gate, and deferred-owner checks. +On successful startup, clear the current cancellation error through the normal +session transition. Preserve historical diagnostic events. + +Use existing silent workspace restoration and explicit retry after a failed +resume. A failed attempt must not remount into an endless automatic retry. +No schema migration or live-data rewrite is required: old orphan reasons are +recognized on read. + +## Desktop and phone behavior + +Use the existing task conversation recovery surface. Desktop retains its chat +panel. Phone retains `session-mobile-layout.tsx`, its session picker, one chat +scroll owner, fixed navigation, and safe-area handling. No new dialog or banner +is introduced. Both surfaces show ordinary startup progress followed by the +same transcript and composer. Existing recovery controls remain on actual failure. + +The [implementation plan](../../../plans/orphaned-session-open-recovery/plan.md) +contains the state preview and desktop/phone regression matrix. Reuse translated +copy. Any required new copy must enter all five locale catalogs. + +## Compatibility and decision + +This draft corrects the classification shipped in PR #3832. The new recovery +exception also covers cancellations from PR #3833. Keep both reconciliation +passes, but replace their orphan cancellation with recoverable interruption +settlement. Archive and explicit-stop cancellation remain unchanged. +The [session-open decision](../../../decisions/2026-09-18-session-open-resumes-conversation.md) +already separates provider recovery from prompt dispatch and workflow ownership. +This local extension needs no additional architecture decision. + ## Requirement mapping | Requirement | Satisfied by | | --- | --- | | REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .1, .2) | Control flow steps 4-5; Episode dedupe | | REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .3) | Control flow step 6 | -| REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .4) | Cancellation boundary re-check; ID-scoped cancel | +| REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .4) | Recovery boundary re-check; conditional session transition | | REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .5) | Control flow step 3 | | REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .6) | Failure and recovery: no registry | +| REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .7) | Lifecycle classification and recovery boundary | +| REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .8, .9) | Recovery when opening a task | +| REQ-TASKS-SESSION-STALL-VISIBILITY-001 (AC .10) | Desktop and phone behavior |