fix(backend): silence spurious errors during Ctrl+C shutdown (#658)
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user