fix: stop runtime instance in CleanupStaleExecutionBySessionID to prevent leaked git polling (#531)
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user