diff --git a/apps/backend/internal/agent/lifecycle/manager_lifecycle.go b/apps/backend/internal/agent/lifecycle/manager_lifecycle.go index b24cd57b66..f898a49615 100644 --- a/apps/backend/internal/agent/lifecycle/manager_lifecycle.go +++ b/apps/backend/internal/agent/lifecycle/manager_lifecycle.go @@ -230,7 +230,8 @@ func (m *Manager) cleanupExitedContainer(ctx context.Context, containerID string m.RemoveExecution(execution.ID) } -// CleanupStaleExecutionBySessionID removes a stale execution from tracking without stopping it. +// CleanupStaleExecutionBySessionID cleans up a stale execution: stops the runtime +// instance, closes the client connection, and removes it from tracking. // // A "stale" execution is one where the agent process has stopped externally (crashed, killed, // or terminated outside of our control) but the execution is still tracked in memory. @@ -242,16 +243,15 @@ func (m *Manager) cleanupExitedContainer(ctx context.Context, containerID string // - When IsAgentRunningForSession returns false but execution exists // // This method performs cleanup: -// 1. Closes the agentctl HTTP client connection -// 2. Removes the execution from the in-memory tracking store +// 1. Stops the runtime instance (workspace tracker, shell, etc.) via the executor backend +// 2. Closes the agentctl HTTP client connection +// 3. Removes the execution from the in-memory tracking store // // What this does NOT do: -// - Stop the agent process (assumed already stopped) // - Clean up worktrees or containers (caller's responsibility) // - Update database session state (caller's responsibility) // -// This is safe to call even if the process is still running - it won't send kill signals. -// Use StopAgent if you need to actively terminate a running agent. +// Safe to call even if the process is already stopped — StopInstance is idempotent. // // Returns nil if no execution exists for the session (idempotent). func (m *Manager) CleanupStaleExecutionBySessionID(ctx context.Context, sessionID string) error { @@ -267,6 +267,12 @@ func (m *Manager) CleanupStaleExecutionBySessionID(ctx context.Context, sessionI // End session trace span execution.EndSessionSpan() + // Stop the runtime instance (workspace tracker, shell, etc.) to prevent leaked + // goroutines. Without this, the old agentctl instance keeps running when a new + // execution is created for the same session, causing git polling on deleted worktrees. + // This is idempotent — returns success if the instance is already gone. + m.stopAgentViaBackend(ctx, execution.ID, execution, "stale execution cleanup", false) + // Close agentctl connection if it exists if execution.agentctl != nil { execution.agentctl.Close() diff --git a/apps/backend/internal/agent/lifecycle/manager_lifecycle_test.go b/apps/backend/internal/agent/lifecycle/manager_lifecycle_test.go index f9118c3854..ed8922f53e 100644 --- a/apps/backend/internal/agent/lifecycle/manager_lifecycle_test.go +++ b/apps/backend/internal/agent/lifecycle/manager_lifecycle_test.go @@ -5,6 +5,8 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/agent/executor" + "github.com/kandev/kandev/internal/agentctl/server/process" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -136,6 +138,105 @@ func TestManager_RemoveExecution(t *testing.T) { mgr.RemoveExecution("non-existent") } +func TestManager_CleanupStaleExecution_StopsRuntimeInstance(t *testing.T) { + log := newTestRegistryLogger() + reg := newTestRegistry() + eventBus := &MockEventBus{} + credsMgr := &MockCredentialsManager{} + profileResolver := &MockProfileResolver{} + + // Create executor registry with a mock backend that tracks StopInstance calls + execRegistry := NewExecutorRegistry(log) + mock := &mockStopTracker{name: "standalone"} + execRegistry.Register(mock) + + mgr := NewManager(reg, eventBus, execRegistry, credsMgr, profileResolver, nil, ExecutorFallbackWarn, "", log) + + execution := &AgentExecution{ + ID: "exec-1", + SessionID: "session-1", + RuntimeName: "standalone", + } + mgr.executionStore.Add(execution) + + err := mgr.CleanupStaleExecutionBySessionID(context.Background(), "session-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Verify StopInstance was called on the backend + if !mock.stopCalled { + t.Error("expected StopInstance to be called on the executor backend") + } + if mock.stoppedInstanceID != "exec-1" { + t.Errorf("expected StopInstance with instance ID exec-1, got %q", mock.stoppedInstanceID) + } + + // Verify execution was removed from store + if _, found := mgr.GetExecutionBySessionID("session-1"); found { + t.Error("expected execution to be removed from store") + } +} + +func TestManager_CleanupStaleExecution_NoopForMissingSession(t *testing.T) { + mgr := newTestManager() + + err := mgr.CleanupStaleExecutionBySessionID(context.Background(), "non-existent") + if err != nil { + t.Fatalf("expected nil error for non-existent session, got: %v", err) + } +} + +func TestManager_CleanupStaleExecution_SkipsStopWhenNoRuntime(t *testing.T) { + mgr := newTestManager() // no executor registry + + execution := &AgentExecution{ + ID: "exec-1", + SessionID: "session-1", + RuntimeName: "standalone", + } + mgr.executionStore.Add(execution) + + err := mgr.CleanupStaleExecutionBySessionID(context.Background(), "session-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should still remove from store even without executor registry + if _, found := mgr.GetExecutionBySessionID("session-1"); found { + t.Error("expected execution to be removed from store") + } +} + +// mockStopTracker is a minimal ExecutorBackend that records StopInstance calls. +type mockStopTracker struct { + name executor.Name + stopCalled bool + stoppedInstanceID string +} + +func (m *mockStopTracker) Name() executor.Name { return m.name } +func (m *mockStopTracker) HealthCheck(ctx context.Context) error { + return nil +} +func (m *mockStopTracker) CreateInstance(ctx context.Context, req *ExecutorCreateRequest) (*ExecutorInstance, error) { + return nil, nil +} +func (m *mockStopTracker) StopInstance(ctx context.Context, instance *ExecutorInstance, force bool) error { + m.stopCalled = true + m.stoppedInstanceID = instance.InstanceID + return nil +} +func (m *mockStopTracker) RecoverInstances(ctx context.Context) ([]*ExecutorInstance, error) { + return nil, nil +} +func (m *mockStopTracker) GetInteractiveRunner() *process.InteractiveRunner { + return nil +} +func (m *mockStopTracker) RequiresCloneURL() bool { return false } +func (m *mockStopTracker) ShouldApplyPreferredShell() bool { return false } +func (m *mockStopTracker) IsAlwaysResumable() bool { return false } + func TestManager_StartStop(t *testing.T) { mgr := newTestManager() diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index 739b567d02..1dca98a3d7 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -147,9 +147,10 @@ type AgentManagerClient interface { // sessions are lazily resumed. PollRemoteStatusForRecords(ctx context.Context, records []RemoteStatusPollRequest) - // CleanupStaleExecutionBySessionID removes a stale execution from the in-memory - // tracking store. Used when the agent process has exited but the execution entry - // was not cleaned up (e.g. prepared workspace where agent was never started). + // CleanupStaleExecutionBySessionID stops the runtime instance and removes a stale + // execution from the in-memory tracking store. Used when the agent process has + // exited but the execution entry was not cleaned up (e.g. prepared workspace + // where agent was never started, or session resume after crash). CleanupStaleExecutionBySessionID(ctx context.Context, sessionID string) error // EnsureWorkspaceExecutionForSession ensures an agentctl execution exists for a