fix(backend): don't surface backend-shutdown turn aborts as agent failures (#2571)
This commit is contained in:
@@ -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),
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
@@ -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.
|
||||
@@ -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 |
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user