From 7e7865a82cb84fdf84c1374ef1cd388a1b1367df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 20 Apr 2026 22:31:30 +0100 Subject: [PATCH] fix(backend): silence spurious errors during Ctrl+C shutdown (#658) --- .../internal/agent/lifecycle/manager.go | 7 ++++ .../agent/lifecycle/manager_interaction.go | 30 +++++++++++--- .../agent/lifecycle/manager_lifecycle.go | 10 +++++ .../agent/lifecycle/manager_passthrough.go | 23 +++++++++++ .../lifecycle/manager_passthrough_test.go | 40 +++++++++++++++++++ 5 files changed, 104 insertions(+), 6 deletions(-) diff --git a/apps/backend/internal/agent/lifecycle/manager.go b/apps/backend/internal/agent/lifecycle/manager.go index 65ea57e232..ac5f961e0e 100644 --- a/apps/backend/internal/agent/lifecycle/manager.go +++ b/apps/backend/internal/agent/lifecycle/manager.go @@ -4,6 +4,7 @@ package lifecycle import ( "sync" + "sync/atomic" "time" "go.uber.org/zap" @@ -82,6 +83,12 @@ type Manager struct { remoteStatusBySession map[string]*RemoteStatus stopCh chan struct{} wg sync.WaitGroup + // shuttingDown is flipped true when graceful shutdown begins (see + // StopAllAgents) so handlers running in detached goroutines can + // short-circuit work that would otherwise race the teardown and log + // confusing errors against children that already died from the same + // terminal-wide SIGINT. + shuttingDown atomic.Bool // pollAggregator routes hub session-mode events to agentctl. See // manager_subscription.go. diff --git a/apps/backend/internal/agent/lifecycle/manager_interaction.go b/apps/backend/internal/agent/lifecycle/manager_interaction.go index 4051c6149d..e8ca27209e 100644 --- a/apps/backend/internal/agent/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/lifecycle/manager_interaction.go @@ -395,9 +395,18 @@ func (m *Manager) StopAgentWithReason(ctx context.Context, executionID string, r if execution.agentctl != nil { if !force { if err := execution.agentctl.Stop(ctx); err != nil { - m.logger.Warn("failed to stop agent via agentctl", - zap.String("execution_id", executionID), - zap.Error(err)) + // During shutdown agentctl typically received the same + // terminal-wide SIGINT and is already gone, so a failed + // HTTP call here is expected, not noteworthy. + if m.IsShuttingDown() { + m.logger.Debug("failed to stop agent via agentctl", + zap.String("execution_id", executionID), + zap.Error(err)) + } else { + m.logger.Warn("failed to stop agent via agentctl", + zap.String("execution_id", executionID), + zap.Error(err)) + } } } execution.agentctl.Close() @@ -994,9 +1003,18 @@ func (m *Manager) stopAgentViaBackend(ctx context.Context, executionID string, e StopReason: reason, } if err := rt.StopInstance(ctx, runtimeInstance, force); err != nil { - m.logger.Warn("failed to stop runtime instance, continuing with cleanup", - zap.String("execution_id", executionID), - zap.Error(err)) + // During shutdown the runtime instance (e.g. a standalone agentctl) + // often already exited via the shared SIGINT, so StopInstance returns + // a benign 404. Only surface this at WARN outside shutdown. + if m.IsShuttingDown() { + m.logger.Debug("failed to stop runtime instance, continuing with cleanup", + zap.String("execution_id", executionID), + zap.Error(err)) + } else { + m.logger.Warn("failed to stop runtime instance, continuing with cleanup", + zap.String("execution_id", executionID), + zap.Error(err)) + } } } diff --git a/apps/backend/internal/agent/lifecycle/manager_lifecycle.go b/apps/backend/internal/agent/lifecycle/manager_lifecycle.go index f898a49615..079aca54cf 100644 --- a/apps/backend/internal/agent/lifecycle/manager_lifecycle.go +++ b/apps/backend/internal/agent/lifecycle/manager_lifecycle.go @@ -133,6 +133,14 @@ func (m *Manager) GetRecoveredExecutions() []RecoveredExecution { return result } +// IsShuttingDown reports whether graceful shutdown has begun. Set by +// StopAllAgents before it starts tearing down executions so concurrent +// handlers (e.g. passthrough exit auto-restart, agentctl HTTP calls) can +// skip or downgrade work that would otherwise race the teardown. +func (m *Manager) IsShuttingDown() bool { + return m.shuttingDown.Load() +} + // Stop stops the lifecycle manager and releases resources held by executors. func (m *Manager) Stop() error { m.logger.Info("stopping lifecycle manager") @@ -150,6 +158,8 @@ func (m *Manager) Stop() error { // StopAllAgents attempts a graceful shutdown of all active agents concurrently. func (m *Manager) StopAllAgents(ctx context.Context) error { + m.shuttingDown.Store(true) + executions := m.executionStore.List() if len(executions) == 0 { return nil diff --git a/apps/backend/internal/agent/lifecycle/manager_passthrough.go b/apps/backend/internal/agent/lifecycle/manager_passthrough.go index fd563519fa..80dcbf733c 100644 --- a/apps/backend/internal/agent/lifecycle/manager_passthrough.go +++ b/apps/backend/internal/agent/lifecycle/manager_passthrough.go @@ -586,9 +586,24 @@ func (m *Manager) handlePassthroughExit(execution *AgentExecution, status *agent sessionID := execution.SessionID + if m.IsShuttingDown() { + m.logger.Debug("skipping passthrough auto-restart during shutdown", + zap.String("session_id", sessionID)) + return + } + // Wait a bit for the old process to be cleaned up from the process map time.Sleep(cleanupDelay) + // Shutdown may have started during cleanupDelay; re-check before emitting + // the "attempting auto-restart" log and the terminal banner, which would + // otherwise mislead the user during a clean shutdown. + if m.IsShuttingDown() { + m.logger.Debug("skipping passthrough auto-restart during shutdown", + zap.String("session_id", sessionID)) + return + } + interactiveRunner := m.GetInteractiveRunner() if interactiveRunner == nil { m.logger.Debug("no interactive runner available for auto-restart", @@ -623,6 +638,14 @@ func (m *Manager) handlePassthroughExit(execution *AgentExecution, status *agent // Delay before restart time.Sleep(restartDelay) + // Shutdown may have started during the sleep; re-check before touching + // state that the teardown is racing to remove. + if m.IsShuttingDown() { + m.logger.Debug("skipping passthrough auto-restart during shutdown", + zap.String("session_id", sessionID)) + return + } + // Check WebSocket is still connected after delay (use session-level tracking) if !interactiveRunner.HasActiveWebSocketBySession(sessionID) { m.logger.Debug("WebSocket disconnected during restart delay, aborting", diff --git a/apps/backend/internal/agent/lifecycle/manager_passthrough_test.go b/apps/backend/internal/agent/lifecycle/manager_passthrough_test.go index 97d78efe4e..3d620c8791 100644 --- a/apps/backend/internal/agent/lifecycle/manager_passthrough_test.go +++ b/apps/backend/internal/agent/lifecycle/manager_passthrough_test.go @@ -3,8 +3,11 @@ package lifecycle import ( "context" "testing" + "testing/synctest" + "time" "github.com/kandev/kandev/internal/agent/agents" + agentctltypes "github.com/kandev/kandev/internal/agentctl/types" ) // mockPassthroughProfileResolver is a mock for testing passthrough verification @@ -218,6 +221,43 @@ func TestBuildPassthroughCommand(t *testing.T) { } } +// TestManager_HandlePassthroughExit_SkipsDuringShutdown verifies that the +// detached goroutine spawned when a passthrough child exits bails out +// immediately once graceful shutdown has begun, instead of racing the +// teardown and logging a spurious "failed to auto-restart passthrough +// session" error. Regression test for the Ctrl+C-in-terminal shutdown +// noise. +// +// Uses testing/synctest so the assertion is "the function returned without +// any time advancing" — i.e. it short-circuited before the cleanupDelay +// sleep. Under fake time, a non-short-circuit path would advance by +// cleanupDelay (and then take the nil-runner branch in the test rig). +func TestManager_HandlePassthroughExit_SkipsDuringShutdown(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + mgr := newTestManager() + + if mgr.IsShuttingDown() { + t.Fatal("fresh manager reports IsShuttingDown() == true") + } + + if err := mgr.StopAllAgents(context.Background()); err != nil { + t.Fatalf("StopAllAgents returned error: %v", err) + } + if !mgr.IsShuttingDown() { + t.Fatal("StopAllAgents did not set IsShuttingDown() = true") + } + + execution := &AgentExecution{ID: "exec-1", SessionID: "sess-1"} + status := &agentctltypes.ProcessStatusUpdate{SessionID: "sess-1"} + + start := time.Now() + mgr.handlePassthroughExit(execution, status) + if elapsed := time.Since(start); elapsed != 0 { + t.Errorf("handlePassthroughExit advanced fake time by %v — did not short-circuit during shutdown", elapsed) + } + }) +} + func TestManager_VerifyPassthroughEnabled(t *testing.T) { tests := []struct { name string