From 76ccc667ce09bb698e342c5fdb7fb5159fa4d810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 12 Aug 2026 18:21:34 +0100 Subject: [PATCH] fix(backend): don't surface backend-shutdown turn aborts as agent failures (#2571) --- .../agent/runtime/lifecycle/manager_events.go | 8 + .../lifecycle/manager_events_shutdown_test.go | 277 ++++++++++++++++++ .../runtime/lifecycle/manager_interaction.go | 70 ++++- .../shutdown-turn-failure-suppression/plan.md | 156 ++++++++++ .../task-01-shutdown-failure-guard.md | 99 +++++++ docs/specs/INDEX.md | 1 + .../shutdown-turn-failure-suppression/spec.md | 102 +++++++ 7 files changed, 711 insertions(+), 2 deletions(-) create mode 100644 apps/backend/internal/agent/runtime/lifecycle/manager_events_shutdown_test.go create mode 100644 docs/plans/shutdown-turn-failure-suppression/plan.md create mode 100644 docs/plans/shutdown-turn-failure-suppression/task-01-shutdown-failure-guard.md create mode 100644 docs/specs/shutdown-turn-failure-suppression/spec.md diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_events.go b/apps/backend/internal/agent/runtime/lifecycle/manager_events.go index 436f28005b..589ca2e76c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_events.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_events.go @@ -108,6 +108,14 @@ func extractErrorMessage(event *agentctl.AgentEvent) string { func (m *Manager) handleCompleteEventMarkState(execution *AgentExecution, event *agentctl.AgentEvent, isError bool) { if isError { errorMsg := extractErrorMessage(event) + // A turn aborted by backend graceful shutdown is not an agent failure. + // Redirect it to a benign stop so the session stays resumable and the UI + // shows no red error banner. MarkCompleted applies the same guard, but + // routing here keeps the misleading "marking as failed" WARN out of logs. + if m.IsShuttingDown() { + _ = m.markStoppedDuringShutdown(execution, 1, errorMsg) + return + } m.logger.Warn("error completion received, marking execution as failed", zap.String("execution_id", execution.ID), zap.String("task_id", execution.TaskID), diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_events_shutdown_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_events_shutdown_test.go new file mode 100644 index 0000000000..add401ef9a --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_events_shutdown_test.go @@ -0,0 +1,277 @@ +package lifecycle + +import ( + "context" + "testing" + + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + "github.com/kandev/kandev/internal/events" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// setShuttingDown flips the manager into graceful-shutdown state without running +// the teardown loop. StopAllAgents sets shuttingDown before it lists executions +// and returns early when the store is empty, so calling it before any execution +// is added yields IsShuttingDown() == true with no side effects. +func setShuttingDown(t *testing.T, mgr *Manager) { + t.Helper() + if err := mgr.StopAllAgents(context.Background()); err != nil { + t.Fatalf("StopAllAgents: %v", err) + } + if !mgr.IsShuttingDown() { + t.Fatal("StopAllAgents did not set IsShuttingDown() = true") + } +} + +// setShuttingDownFlag flips only the shutdown flag, leaving tracked executions +// in place. Unlike setShuttingDown (which runs StopAllAgents and therefore stops +// and removes every execution), this is for scenarios that need an execution +// still present in the store while IsShuttingDown() is true. +func setShuttingDownFlag(mgr *Manager) { + mgr.shuttingDown.Store(true) +} + +func publishedSubjects(bus *MockEventBusWithTracking) []string { + bus.mu.Lock() + defer bus.mu.Unlock() + subjects := make([]string, 0, len(bus.PublishedEvents)) + for _, ev := range bus.PublishedEvents { + subjects = append(subjects, ev.Subject) + } + return subjects +} + +func containsSubject(subjects []string, want string) bool { + return countSubject(subjects, want) > 0 +} + +func countSubject(subjects []string, want string) int { + n := 0 + for _, s := range subjects { + if s == want { + n++ + } + } + return n +} + +// TestHandleCompleteEventMarkState_ShutdownRaceMarksStopped verifies that an +// error completion arriving while the backend is shutting down marks the +// execution STOPPED and publishes AgentStopped instead of failing it. +func TestHandleCompleteEventMarkState_ShutdownRaceMarksStopped(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + setShuttingDown(t, mgr) + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + errorEvent := &agentctl.AgentEvent{ + Type: "complete", + Error: "-32603 peer disconnected before response", + Data: map[string]any{"is_error": true}, + } + mgr.handleCompleteEventMarkState(execution, errorEvent, true) + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusStopped { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusStopped) + } + subjects := publishedSubjects(eventBus) + if containsSubject(subjects, events.AgentFailed) { + t.Errorf("AgentFailed was published during shutdown; subjects=%v", subjects) + } + if !containsSubject(subjects, events.AgentStopped) { + t.Errorf("AgentStopped was not published during shutdown; subjects=%v", subjects) + } +} + +// TestHandleCompleteEventMarkState_ErrorFailsWhenNotShuttingDown verifies the +// unchanged failure path: without shutdown, an error completion marks the +// execution FAILED and publishes AgentFailed. +func TestHandleCompleteEventMarkState_ErrorFailsWhenNotShuttingDown(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + errorEvent := &agentctl.AgentEvent{ + Type: "complete", + Error: "boom", + Data: map[string]any{"is_error": true}, + } + mgr.handleCompleteEventMarkState(execution, errorEvent, true) + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusFailed { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusFailed) + } + subjects := publishedSubjects(eventBus) + if !containsSubject(subjects, events.AgentFailed) { + t.Errorf("AgentFailed was not published; subjects=%v", subjects) + } + if containsSubject(subjects, events.AgentStopped) { + t.Errorf("AgentStopped should not be published outside shutdown; subjects=%v", subjects) + } +} + +// TestMarkCompleted_ShutdownRaceMarksStopped verifies the process-exit chokepoint +// honors shutdown: a failing MarkCompleted during shutdown becomes STOPPED. +func TestMarkCompleted_ShutdownRaceMarksStopped(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + setShuttingDown(t, mgr) + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + if err := mgr.MarkCompleted("exec-1", 1, "boom"); err != nil { + t.Fatalf("MarkCompleted: %v", err) + } + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusStopped { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusStopped) + } + subjects := publishedSubjects(eventBus) + if containsSubject(subjects, events.AgentFailed) { + t.Errorf("AgentFailed was published during shutdown; subjects=%v", subjects) + } + if !containsSubject(subjects, events.AgentStopped) { + t.Errorf("AgentStopped was not published during shutdown; subjects=%v", subjects) + } +} + +// TestMarkCompleted_SuccessUnaffectedByShutdown verifies a clean (exit 0) +// completion during shutdown still completes normally, not stopped. +func TestMarkCompleted_SuccessUnaffectedByShutdown(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + setShuttingDown(t, mgr) + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + if err := mgr.MarkCompleted("exec-1", 0, ""); err != nil { + t.Fatalf("MarkCompleted: %v", err) + } + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusCompleted { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusCompleted) + } +} + +// TestMarkStoppedDuringShutdown_PreservesTerminalFailure verifies that a genuine +// FAILED outcome recorded before shutdown is not downgraded to STOPPED, and that +// a duplicate shutdown completion does not re-publish AgentStopped. This guards +// the duplicate-terminal race where an ACP error marks FAILED first and a later +// process-exit completion arrives after shutdown begins. +func TestMarkStoppedDuringShutdown_PreservesTerminalFailure(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + // First: a genuine failure while not shutting down -> FAILED + AgentFailed. + if err := mgr.MarkCompleted("exec-1", 1, "boom"); err != nil { + t.Fatalf("MarkCompleted: %v", err) + } + + // Then shutdown begins and a duplicate error completion arrives. Set only the + // flag so exec-1 stays in the store (StopAllAgents would remove it). + setShuttingDownFlag(mgr) + if err := mgr.markStoppedDuringShutdown(execution, 1, "boom"); err != nil { + t.Fatalf("markStoppedDuringShutdown: %v", err) + } + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusFailed { + t.Errorf("status = %v, want %v (terminal failure must not downgrade to stopped)", + got.Status, v1.AgentStatusFailed) + } + subjects := publishedSubjects(eventBus) + if n := countSubject(subjects, events.AgentStopped); n != 0 { + t.Errorf("AgentStopped published %d times for already-terminal execution; want 0; subjects=%v", n, subjects) + } + if n := countSubject(subjects, events.AgentFailed); n != 1 { + t.Errorf("AgentFailed published %d times; want exactly 1; subjects=%v", n, subjects) + } +} + +// TestMarkStoppedDuringShutdown_DuplicatePublishesOnce verifies that when both +// terminal chokepoints reach markStoppedDuringShutdown during shutdown for the +// same execution (ACP error event + process exit), only the first transition +// applies and AgentStopped is published exactly once. +func TestMarkStoppedDuringShutdown_DuplicatePublishesOnce(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + setShuttingDown(t, mgr) + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + if err := mgr.markStoppedDuringShutdown(execution, 1, "boom"); err != nil { + t.Fatalf("markStoppedDuringShutdown (first): %v", err) + } + if err := mgr.markStoppedDuringShutdown(execution, 1, "boom"); err != nil { + t.Fatalf("markStoppedDuringShutdown (second): %v", err) + } + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusStopped { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusStopped) + } + subjects := publishedSubjects(eventBus) + if n := countSubject(subjects, events.AgentStopped); n != 1 { + t.Errorf("AgentStopped published %d times; want exactly 1; subjects=%v", n, subjects) + } +} + +// TestHandleCompleteEvent_ShutdownRaceStillSignalsPromptDone verifies the +// invariant that a shutdown-race error completion, driven through the full +// handleCompleteEvent -> finishPromptCompletion path, still signals promptDoneCh +// so an in-flight SendPrompt waiter unblocks instead of hanging. The signal is +// emitted by handleCompleteEventSignal before the shutdown guard is reached; this +// guards against a future refactor reordering the signal below the guard. +func TestHandleCompleteEvent_ShutdownRaceStillSignalsPromptDone(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + setShuttingDown(t, mgr) + + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + errorEvent := &agentctl.AgentEvent{ + Type: "complete", + Error: "-32603 peer disconnected before response", + Data: map[string]any{"is_error": true}, + } + mgr.handleCompleteEvent(execution, errorEvent) + + select { + case signal := <-execution.promptDoneCh: + if !signal.IsError { + t.Error("expected error signal for shutdown-race abort") + } + if signal.StopReason != "error" { + t.Errorf("stop_reason = %q, want %q", signal.StopReason, "error") + } + default: + t.Fatal("no signal on promptDoneCh; an in-flight prompt waiter would hang") + } + + got, _ := mgr.GetExecution("exec-1") + if got.Status != v1.AgentStatusStopped { + t.Errorf("status = %v, want %v", got.Status, v1.AgentStatusStopped) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 5c36f0542e..c93a0f404e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -1401,8 +1401,10 @@ func (m *Manager) MarkCompleted(executionID string, exitCode int, errorMessage s } // Guard against duplicate completion (e.g. ACP prompt error + process exit error). - // MarkCompleted is a terminal transition — once in Completed/Failed, skip re-publishing. - if execution.Status == v1.AgentStatusCompleted || execution.Status == v1.AgentStatusFailed { + // MarkCompleted is a terminal transition — once in Completed/Failed/Stopped, skip + // re-publishing. Stopped is a terminal state too (shutdown-race abort, StopAgent), + // so a second completion must not overwrite it or re-emit an event. + if isTerminalStatus(execution.Status) { m.logger.Warn("ignoring duplicate MarkCompleted for already-terminal execution", zap.String("execution_id", executionID), zap.String("current_status", string(execution.Status)), @@ -1410,6 +1412,13 @@ func (m *Manager) MarkCompleted(executionID string, exitCode int, errorMessage s return nil } + // A turn aborted because backend graceful shutdown killed the agent + // subprocess is not an agent failure. Treat the terminal error as a benign + // stop so the session stays resumable and the UI shows no red error banner. + if (exitCode != 0 || errorMessage != "") && m.IsShuttingDown() { + return m.markStoppedDuringShutdown(execution, exitCode, errorMessage) + } + _ = m.executionStore.WithLock(executionID, func(exec *AgentExecution) { now := time.Now() exec.FinishedAt = &now @@ -1451,6 +1460,63 @@ func (m *Manager) MarkCompleted(executionID string, exitCode int, errorMessage s return nil } +// isTerminalStatus reports whether a status is a final execution state that +// MarkCompleted / markStoppedDuringShutdown must not overwrite or re-publish. +func isTerminalStatus(status v1.AgentStatus) bool { + return status == v1.AgentStatusCompleted || + status == v1.AgentStatusFailed || + status == v1.AgentStatusStopped +} + +// markStoppedDuringShutdown records a terminal error completion that arrived +// while backend graceful shutdown was in progress as a benign STOPPED outcome +// instead of a FAILED one. It stamps the terminal fields, runs the same teardown +// as the failed branch, and publishes events.AgentStopped — mirroring the +// StopReasonBackendShutdown teardown so the orchestrator treats it as resumable. +// It deliberately does not remove the execution or run classifyAndMaybeRemediate. +// +// Both terminal chokepoints (handleCompleteEventMarkState and MarkCompleted) can +// reach this during shutdown for the same execution (ACP error event + process +// exit). The transition is therefore guarded under the store lock: if the +// execution is already terminal, or was removed by StopAgentWithReason, this is a +// stale/duplicate callback — skip teardown and the AgentStopped publish so the +// orchestrator does not process the same stop twice. +func (m *Manager) markStoppedDuringShutdown(execution *AgentExecution, exitCode int, errorMessage string) error { + applied := false + err := m.executionStore.WithLock(execution.ID, func(exec *AgentExecution) { + if isTerminalStatus(exec.Status) { + return + } + now := time.Now() + exec.FinishedAt = &now + exec.ExitCode = &exitCode + exec.ErrorMessage = errorMessage + exec.Status = v1.AgentStatusStopped + applied = true + }) + if err != nil || !applied { + m.logger.Debug("ignoring stale/duplicate shutdown completion", + zap.String("execution_id", execution.ID), + zap.String("current_status", string(execution.Status)), + zap.Error(err)) + return nil + } + + m.logger.Warn("error completion during shutdown, treating as cancellation", + zap.String("execution_id", execution.ID), + zap.String("task_id", execution.TaskID), + zap.Int("exit_code", exitCode), + zap.String("error", errorMessage)) + + execution.EndSessionSpan() + m.persistExecutorRunning(context.Background(), execution) + m.releaseActivity(executionActivityKey(execution.ID)) + + m.eventPublisher.PublishAgentEvent(context.Background(), events.AgentStopped, execution) + + return nil +} + // classifyAndMaybeRemediate runs routingerr.Classify at the failure // boundary, logs the structured result, and - for codes that carry a // safe remediation (currently CodeNpxCacheCorrupted) - fires a diff --git a/docs/plans/shutdown-turn-failure-suppression/plan.md b/docs/plans/shutdown-turn-failure-suppression/plan.md new file mode 100644 index 0000000000..9683af4f43 --- /dev/null +++ b/docs/plans/shutdown-turn-failure-suppression/plan.md @@ -0,0 +1,156 @@ +--- +spec: docs/specs/shutdown-turn-failure-suppression/spec.md +created: 2026-08-12 +status: implemented +--- + +# Implementation Plan: Do not surface backend-shutdown turn aborts as agent failures + +## Overview +Gate the lifecycle manager's terminal-failure path on graceful-shutdown state so +an in-flight turn aborted by SIGTERM becomes a benign stop instead of a +user-visible FAILED session. The change is a single-package backend fix in +`internal/agent/runtime/lifecycle`: guard `handleCompleteEventMarkState` (the +agent `error`-event chokepoint) and `MarkCompleted` (the process-exit chokepoint) +on the already-existing `Manager.IsShuttingDown()`. When shutting down, mark the +execution `STOPPED` and skip `AgentFailed` publication and +`classifyAndMaybeRemediate`, while still signalling `promptDoneCh` so waiters +unblock. No frontend or orchestrator change is required, because the orchestrator +already treats `AgentStopped` as resumable and never sets `last_agent_error` for +it. + +--- + +## Root cause (confirmed) +Incident task `2a5cad30`, execution `5cb0f5e0`: backend SIGTERM at 19:03:06 tore +down the agent subprocess mid-turn. The agent emitted an `error` event carrying +the ACP transport error `-32603 ... peer disconnected before response`. Flow: +`handleErrorEvent` -> `handleCompleteEvent` -> `finishPromptCompletion` -> +`handleCompleteEventMarkState(isError=true)` -> `MarkCompleted(1, errorMsg)`, +which set status `FAILED`, published `events.AgentFailed`, and ran +`classifyAndMaybeRemediate`. The classifier had no shutdown context, so it logged +`routing_code=agent_runtime_error` (`confidence=low`, +`classifier_rule=phase.poststart.unknown`) and the orchestrator flipped the +session to a terminal FAILED state that the UI rendered as a red error banner. +`Manager.IsShuttingDown()` (set at the top of `StopAllAgents`) was already true at +that moment but was never consulted on the failure path. + +--- + +## Backend + +### Area 1 — Shutdown-race guard in the terminal transitions +Package: `apps/backend/internal/agent/runtime/lifecycle/` + +- `manager_events.go` `handleCompleteEventMarkState(execution, event, isError)` + (~L108): at the top of the `if isError` branch, when `m.IsShuttingDown()` is + true, log WARN `"error completion during shutdown, treating as cancellation"` + (execution_id, task_id, error) and mark the execution `STOPPED` via a shared + helper (see Area 2), then `return` without calling `MarkCompleted(1, errorMsg)`. + This is the primary incident path. `promptDoneCh` is already signalled earlier + in `finishPromptCompletion` via `handleCompleteEventSignal`, so waiters still + unblock — do not move or duplicate that. + +- `manager_interaction.go` `MarkCompleted(executionID, exitCode, errorMessage)` + (~L1397): after computing that the status would be `AgentStatusFailed` (the + `else` branch of the exit-code/error test, ~L1421), when `m.IsShuttingDown()` + is true, set `exec.Status = v1.AgentStatusStopped` instead of + `AgentStatusFailed`, keep `FinishedAt`/`ExitCode`/`ErrorMessage` stamping and + the `persistExecutorRunning`/`releaseActivity`/trace-span teardown, publish + `events.AgentStopped` instead of `events.AgentFailed`, and do NOT call + `classifyAndMaybeRemediate`. This covers process-exit paths that reach + `MarkCompleted` directly (not only the agent `error` event). Keep the existing + duplicate-terminal guard (~L1405) unchanged. + +### Area 2 — Shared benign-stop helper (avoid duplicated blocks) +Add one small unexported helper on `*Manager`, e.g. +`markStoppedDuringShutdown(execution *AgentExecution, exitCode int, errorMessage string)`, +that performs the STOPPED status stamp + terminal teardown + `AgentStopped` +publish used by both call sites, so the two guards do not duplicate a ≥150-token +block (backend lint limit). `handleCompleteEventMarkState` calls it; the +`MarkCompleted` guard reuses the same status/publish decision. Keep each guarded +function under the 80-line / complexity limits by extracting rather than inlining. + +Note: `AgentStatusStopped` and the `events.AgentStopped` publish already mirror +exactly what `StopAgentWithReason` does for the `StopReasonBackendShutdown` path, +so the orchestrator's `handleAgentStopped` handles it as a known, non-FAILED, +resumable outcome with no new wiring. + +--- + +## Frontend +> No user-facing change. Once the backend stops marking the session FAILED, the +> existing `FAILED`-gated red banner (`agent-status.tsx` STATE_CONFIG.FAILED) is +> simply never triggered for this case. No component, client, or store change. + +--- + +## Tests +All backend, in `apps/backend/internal/agent/runtime/lifecycle/`. Use the +existing helpers: `newTestManager(t)` / `createTestManagerWithTracking()` + +`createTestExecution(...)`, and `MockEventBus.OnPublish` to capture published +subjects. + +- **What:** shutdown-race error completion does not fail the execution. + **File:** new `manager_events_shutdown_test.go` (avoid growing the large + existing test file per the 800-line rule). + **How:** build a Manager, `mgr.StopAllAgents(ctx)` (or set shutting-down) so + `IsShuttingDown()` is true, add a RUNNING execution, call + `handleCompleteEventMarkState(exec, errorEvent{is_error:true, error:"...peer + disconnected before response"}, true)`. Assert final status is `STOPPED` (not + `FAILED`), assert no `events.AgentFailed` subject was published (capture via + `MockEventBus.OnPublish`), assert `events.AgentStopped` was published. +- **What:** classifier is not invoked on shutdown-race abort. + **File:** same as above. **How:** inject a recording `remediateNpxCache` stub / + spy on the classify log or use a seam; at minimum assert no `AgentFailed` and + status `STOPPED` (classify only runs on the `AgentFailed` branch, so absence of + `AgentFailed` is the observable proxy). Prefer asserting via the failure branch + not being taken. +- **What:** genuine failure while running normally is unchanged. + **File:** same file. **How:** with `IsShuttingDown()` false, call the same + error completion; assert status `FAILED` and `events.AgentFailed` published + (regression guard mirroring existing `TestManager_MarkCompleted_Failure`). +- **What:** `MarkCompleted` failure branch honors shutdown. + **File:** `manager_lifecycle_test.go` sibling or the new file. **How:** set + `IsShuttingDown()` true, `MarkCompleted("id", 1, "boom")`; assert status + `STOPPED` + `AgentStopped`. With it false, assert `FAILED` + `AgentFailed` + (extends existing `TestManager_MarkCompleted_Failure`). +- **What:** waiter still unblocks on shutdown-race abort (no hang). + **File:** new file. **How:** confirm `promptDoneCh` receives the completion + signal (the signal is emitted before the guard); assert a non-blocking receive + succeeds. + +Run: `cd apps/backend && go test ./internal/agent/runtime/lifecycle/...` +(package uses `goleak.VerifyTestMain`, so ensure `cleanupManagerStopCh` is used +via `newTestManager`). + +--- + +## E2E Tests +> Skipped: no user-visible UI change (the outcome is the absence of a red +> banner). The behavior is fully covered by the backend unit tests above; an E2E +> that kills the backend mid-turn is out of proportion for a logging/state gate. + +--- + +## Verification Results +- `cd apps/backend && go test ./internal/agent/runtime/lifecycle/` -> `ok` + (full package pass, ~48s). +- `golangci-lint run ./internal/agent/runtime/lifecycle/... --new-from-rev="$(git merge-base HEAD origin/main)" --timeout=5m` + -> `0 issues`; `gofmt -l` on changed files -> clean. +- See `task-01-shutdown-failure-guard.md` `## Results` for the full list. + +--- + +## Implementation Waves And Parallel Candidates + +```text +Wave 1: +- [x] [task-01-shutdown-failure-guard](task-01-shutdown-failure-guard.md) +``` + +Single-task fix; no parallelism. The regression tests land in the same task as +the guard so the test fails before and passes after the code change. + +## Open Questions +(none) diff --git a/docs/plans/shutdown-turn-failure-suppression/task-01-shutdown-failure-guard.md b/docs/plans/shutdown-turn-failure-suppression/task-01-shutdown-failure-guard.md new file mode 100644 index 0000000000..26626daba0 --- /dev/null +++ b/docs/plans/shutdown-turn-failure-suppression/task-01-shutdown-failure-guard.md @@ -0,0 +1,99 @@ +--- +id: "01-shutdown-failure-guard" +title: "Gate terminal failure on graceful shutdown" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/shutdown-turn-failure-suppression/spec.md" +parallelism: sequential +--- + +# Task 01: Gate terminal failure on graceful shutdown + +Suppress user-visible agent failure for turns aborted by backend graceful +shutdown. When `Manager.IsShuttingDown()` is true, an error completion / failing +`MarkCompleted` marks the execution `STOPPED` and publishes `events.AgentStopped` +instead of `FAILED` + `events.AgentFailed`, and skips `classifyAndMaybeRemediate`. + +## Root cause +See `plan.md` "Root cause". SIGTERM mid-turn -> agent `error` event (`-32603 peer +disconnected before response`) -> `handleErrorEvent` -> `handleCompleteEvent` -> +`finishPromptCompletion` -> `handleCompleteEventMarkState(isError=true)` -> +`MarkCompleted(1, msg)` -> `FAILED` + `AgentFailed` + classifier +(`agent_runtime_error`) -> orchestrator terminal FAILED -> red UI banner. +`IsShuttingDown()` was already true but never consulted on this path. + +## Acceptance +1. With `IsShuttingDown()` true, an error completion (via + `handleCompleteEventMarkState`) and a failing `MarkCompleted` both leave the + execution `v1.AgentStatusStopped`, publish `events.AgentStopped`, do NOT + publish `events.AgentFailed`, and do NOT run `classifyAndMaybeRemediate`. +2. With `IsShuttingDown()` false, behavior is unchanged: error completion / failing + `MarkCompleted` -> `FAILED` + `AgentFailed` + classifier (existing tests still + pass). +3. The `promptDoneCh` completion signal still fires on the shutdown-race path so + `waitForPromptDone` / `dispatchInitialPrompt` waiters unblock. + +## Implementation notes +- Add unexported helper `(*Manager) markStoppedDuringShutdown(execution + *AgentExecution, exitCode int, errorMessage string)` that stamps + `AgentStatusStopped` + `FinishedAt`/`ExitCode`/`ErrorMessage`, runs the same + terminal teardown as the failed branch (`EndSessionSpan`, + `persistExecutorRunning`, `releaseActivity`), and publishes `events.AgentStopped`. + Reuse it from both guards to avoid a duplicated ≥150-token block (backend lint). +- `manager_events.go` `handleCompleteEventMarkState`: at the top of the + `if isError` branch, if `m.IsShuttingDown()` log WARN "error completion during + shutdown, treating as cancellation" and call the helper, then `return` (skip + `MarkCompleted`). Do not touch the `promptDoneCh` signal (emitted earlier in + `finishPromptCompletion`). +- `manager_interaction.go` `MarkCompleted`: keep the duplicate-terminal guard. + In the branch that would set `AgentStatusFailed`, if `m.IsShuttingDown()`, use + the helper's status/publish decision (STOPPED + `AgentStopped`, no classifier) + instead of `AgentFailed` + `classifyAndMaybeRemediate`. +- Keep each edited function within the 80-line / 50-statement / complexity limits; + extract into the helper rather than inlining. + +## Files likely touched +- `apps/backend/internal/agent/runtime/lifecycle/manager_events.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_events_shutdown_test.go` (new) + +## Tests (must fail before, pass after) +- `manager_events_shutdown_test.go` (new file to respect the 800-line test-file + rule): shutdown-race error completion -> `STOPPED`, `AgentStopped` published, + no `AgentFailed`; normal error completion -> `FAILED`, `AgentFailed`; failing + `MarkCompleted` under shutdown -> `STOPPED`/`AgentStopped`; `promptDoneCh` + non-blocking receive succeeds on the shutdown path. Use `newTestManager(t)` / + `createTestManagerWithTracking()`, `createTestExecution(...)`, and + `MockEventBus.OnPublish` to capture published subjects; drive shutdown via + `mgr.StopAllAgents(ctx)` (which sets `IsShuttingDown()` true). + +## Verification +```bash +cd apps/backend && go test ./internal/agent/runtime/lifecycle/... && \ + golangci-lint run ./internal/agent/runtime/lifecycle/... --new-from-rev="$(git merge-base HEAD origin/main)" --timeout=5m +``` + +## Dependencies +None. + +## Output contract +Summary of the guard + helper, reconciled files-changed list, the exact `go test` +and changed-file linter commands with pass counts, any risks, and status updates +to this task file and `plan.md`. + +## Results +- Added `(*Manager) markStoppedDuringShutdown` in `manager_interaction.go`; + guarded the failure branch of `MarkCompleted` and the `isError` branch of + `handleCompleteEventMarkState` (`manager_events.go`) on `IsShuttingDown()`. +- New tests in `manager_events_shutdown_test.go` (new file): + `TestHandleCompleteEventMarkState_ShutdownRaceMarksStopped`, + `TestHandleCompleteEventMarkState_ErrorFailsWhenNotShuttingDown`, + `TestMarkCompleted_ShutdownRaceMarksStopped`, + `TestMarkCompleted_SuccessUnaffectedByShutdown`. +- `cd apps/backend && go test ./internal/agent/runtime/lifecycle/` -> `ok` (full + package, ~48s). Targeted new tests -> PASS. +- `golangci-lint run ./internal/agent/runtime/lifecycle/... --new-from-rev="$(git merge-base HEAD origin/main)" --timeout=5m` + -> `0 issues`. `gofmt -l` on the three changed files -> clean. +- External side effects: None. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 2beb88079e..f2d22e4cd3 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -264,6 +264,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [no-silent-model-fallback](no-silent-model-fallback/spec.md) | approved | | [e2e-duration-aware-sharding](e2e-duration-aware-sharding/spec.md) | implemented | | [board-step-visibility-filter](board-step-visibility-filter/spec.md) | draft | +| [shutdown-turn-failure-suppression](shutdown-turn-failure-suppression/spec.md) | draft | --- diff --git a/docs/specs/shutdown-turn-failure-suppression/spec.md b/docs/specs/shutdown-turn-failure-suppression/spec.md new file mode 100644 index 0000000000..797c5d90de --- /dev/null +++ b/docs/specs/shutdown-turn-failure-suppression/spec.md @@ -0,0 +1,102 @@ +--- +status: implemented +created: 2026-08-12 +owner: cfl12 +--- + +# Do not surface backend-shutdown turn aborts as agent failures + +## Why +When the Kandev backend receives SIGTERM while an agent turn is in flight, the +agent subprocess is torn down and the in-flight prompt fails with an ACP +transport error (`-32603 ... peer disconnected before response`). The lifecycle +manager currently treats that error completion as a genuine agent failure: it +marks the execution `FAILED`, publishes `AgentFailed`, and runs the routing +classifier, which labels the abort `agent_runtime_error` (low confidence, +`classifier_rule=phase.poststart.unknown`). The orchestrator then flips the +session to a terminal FAILED state and the UI renders a red error banner for +what was actually a clean, expected shutdown. + +This is distinct from the `shutdown-log-noise` spec, which only downgrades log +severity and explicitly excludes any change to state transitions, returned +errors, or the classifier. This spec covers the state/UX regression: a +shutdown-race turn abort must not become a user-visible FAILED session. + +Observed incident: task `2a5cad30-cfea-494f-a19c-99b31579879e`, execution +`5cb0f5e0`. Backend SIGTERM at 19:03:06 correlated with the execution failing at +19:03:08 and surfacing `agent_runtime_error` in the UI. + +## What +- When an in-flight agent turn receives an error completion **because backend + graceful shutdown is in progress** (`Manager.IsShuttingDown()` is true), the + lifecycle manager SHALL NOT mark the execution `FAILED`, SHALL NOT publish + `AgentFailed`, and SHALL NOT run `classifyAndMaybeRemediate`. It SHALL instead + treat the turn as a shutdown-induced stop, consistent with the existing + `StopReasonBackendShutdown` teardown path. +- The session SHALL remain in a resumable (non-terminal, non-FAILED) state after + a shutdown-race turn abort, so the next launch or `Resume` continues cleanly. + No `last_agent_error` / red error banner is presented to the user for this + case. +- The classifier log line `agent failure classified for provider routing` with + `routing_code=agent_runtime_error` SHALL NOT be emitted for a shutdown-race + abort. +- A genuine agent failure on an active session (backend NOT shutting down) SHALL + continue to mark the execution `FAILED`, publish `AgentFailed`, run the + classifier, and surface to the UI exactly as today. Suppression is gated + strictly on shutdown state. +- The completion signal on `promptDoneCh` SHALL still fire so any waiter in + `waitForPromptDone` / `dispatchInitialPrompt` unblocks; only the terminal + FAILED marking and failure publication/classification are suppressed. + +## Affected code and target behavior +1. `internal/agent/runtime/lifecycle/manager_events.go` + `handleCompleteEventMarkState(execution, event, isError=true)` (~L108) is the + single chokepoint reached by both the agent `error` event + (`handleErrorEvent` -> `handleCompleteEvent` -> `finishPromptCompletion`) and + the `is_error` complete path. When `isError` and `m.IsShuttingDown()`, log a + benign WARN ("error completion during shutdown, treating as cancellation") + and skip the `MarkCompleted(1, errorMsg)` call. Leave `promptDoneCh` + signalling (done earlier in `finishPromptCompletion` via + `handleCompleteEventSignal`) untouched. +2. `internal/agent/runtime/lifecycle/manager_interaction.go` `MarkCompleted` + (~L1397) is also reachable from process-exit paths. Guard the failure branch + there too: when the computed status is `Failed` and `m.IsShuttingDown()`, + mark the execution `Stopped` (terminal, non-failure) instead of `Failed`, and + do not publish `AgentFailed` or call `classifyAndMaybeRemediate`. This keeps + the guard correct regardless of which path reaches the terminal transition. + `IsShuttingDown()` already exists (`manager_lifecycle.go` L174), set true at + the very start of `StopAllAgents` before teardown begins. + +## Failure modes +- Gating is on `Manager.IsShuttingDown()` only. It is impossible to suppress a + real failure that happens while the backend is running normally, because the + flag is set exclusively by `StopAllAgents` at the start of graceful shutdown. +- A turn that fails microseconds before SIGTERM (flag still false) is classified + normally; this is acceptable and matches the existing conservative-classifier + convention. The fix targets the dominant race where teardown is already in + progress. + +## Scenarios +- **GIVEN** an agent turn in flight and `Manager.IsShuttingDown()` is true, + **WHEN** the agent error completion `-32603 peer disconnected before response` + arrives, **THEN** the execution is not marked `FAILED`, no `AgentFailed` event + is published, `classifyAndMaybeRemediate` is not called, and no + `routing_code=agent_runtime_error` log line is emitted. +- **GIVEN** the same shutdown-race abort, **WHEN** the session state is later + read, **THEN** it is not in a terminal FAILED state and has no + `last_agent_error`, so no red error banner is shown and the task is resumable. +- **GIVEN** a genuine agent error completion while the backend is running + normally (`IsShuttingDown()` false), **WHEN** it arrives, **THEN** the + execution is marked `FAILED`, `AgentFailed` is published, and the classifier + runs exactly as today. +- **GIVEN** any shutdown-race abort, **WHEN** `waitForPromptDone` / + `dispatchInitialPrompt` are waiting, **THEN** the completion signal still + fires and the waiter unblocks (no hang on shutdown). + +## Out of scope +- Log-severity downgrades of the same teardown races (owned by the + `shutdown-log-noise` spec). +- Changing shutdown ordering, timeouts, or which subprocesses are killed. +- Retry/fallback/provider-routing behavior for genuine failures. +- Frontend changes: no red banner is expected once the backend stops marking the + session FAILED, so no frontend change is required.