test(handlers): cover ensureShellExecution stale-execution recovery
The recovery branch (Failed execution → CleanupStaleExecutionBySessionID → relaunch) is the load-bearing piece of shell session resilience and had no direct test. Extract the body into a free function that takes a narrow shellLifecycle interface so the four branches are testable without standing up a real lifecycle.Manager: - happy path (running execution, no cleanup) - stale recovery (Failed → cleanup → fresh) - initial GetOrEnsureExecution failure - cleanup failure must NOT trigger a re-launch loop - relaunch failure surfaces a clear "recover execution" error
This commit is contained in:
@@ -197,10 +197,26 @@ func (h *ShellHandlers) wsShellInput(ctx context.Context, msg *ws.Message) (*ws.
|
||||
})
|
||||
}
|
||||
|
||||
// shellLifecycle is the narrow slice of lifecycle.Manager that
|
||||
// ensureShellExecution needs. *lifecycle.Manager satisfies it; tests can
|
||||
// stub it without touching the rest of the manager.
|
||||
type shellLifecycle interface {
|
||||
GetOrEnsureExecution(ctx context.Context, sessionID string) (*lifecycle.AgentExecution, error)
|
||||
CleanupStaleExecutionBySessionID(ctx context.Context, sessionID string) error
|
||||
}
|
||||
|
||||
func (h *ShellHandlers) ensureShellExecution(ctx context.Context, sessionID string) (*lifecycle.AgentExecution, error) {
|
||||
execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, sessionID)
|
||||
return ensureShellExecution(ctx, h.lifecycleMgr, h.logger, sessionID)
|
||||
}
|
||||
|
||||
// ensureShellExecution returns a live shell execution for sessionID, recovering
|
||||
// from a stale (Failed) execution by cleaning it up and re-launching once.
|
||||
// Extracted as a free function so the recovery branch is directly testable
|
||||
// without standing up a real lifecycle.Manager.
|
||||
func ensureShellExecution(ctx context.Context, lifecycleMgr shellLifecycle, log *logger.Logger, sessionID string) (*lifecycle.AgentExecution, error) {
|
||||
execution, err := lifecycleMgr.GetOrEnsureExecution(ctx, sessionID)
|
||||
if err != nil {
|
||||
h.logger.Debug("failed to ensure shell execution",
|
||||
log.Debug("failed to ensure shell execution",
|
||||
zap.String("session_id", sessionID),
|
||||
zap.Error(err))
|
||||
return nil, fmt.Errorf("no agent running for session %s", sessionID)
|
||||
@@ -208,10 +224,10 @@ func (h *ShellHandlers) ensureShellExecution(ctx context.Context, sessionID stri
|
||||
if execution.Status != v1.AgentStatusFailed {
|
||||
return execution, nil
|
||||
}
|
||||
if err := h.lifecycleMgr.CleanupStaleExecutionBySessionID(ctx, sessionID); err != nil {
|
||||
if err := lifecycleMgr.CleanupStaleExecutionBySessionID(ctx, sessionID); err != nil {
|
||||
return nil, fmt.Errorf("cleanup stale execution for session %s: %w", sessionID, err)
|
||||
}
|
||||
execution, err = h.lifecycleMgr.GetOrEnsureExecution(ctx, sessionID)
|
||||
execution, err = lifecycleMgr.GetOrEnsureExecution(ctx, sessionID)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("recover execution for session %s: %w", sessionID, err)
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ package handlers
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -11,6 +12,7 @@ import (
|
||||
"github.com/kandev/kandev/internal/agent/registry"
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
v1 "github.com/kandev/kandev/pkg/api/v1"
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
)
|
||||
|
||||
@@ -581,3 +583,132 @@ func TestWsUserShellStop_NoInteractiveRunner(t *testing.T) {
|
||||
t.Errorf("expected 'interactive runner not available', got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// stubShellLifecycle satisfies shellLifecycle for ensureShellExecution
|
||||
// tests so the recovery branch can be driven without a real
|
||||
// lifecycle.Manager. Each call records its argument and returns the next
|
||||
// scripted return value.
|
||||
type stubShellLifecycle struct {
|
||||
getResults []*lifecycle.AgentExecution
|
||||
getErrors []error
|
||||
getCalls []string
|
||||
cleanupErr error
|
||||
cleanupCalls []string
|
||||
}
|
||||
|
||||
func (s *stubShellLifecycle) GetOrEnsureExecution(_ context.Context, sessionID string) (*lifecycle.AgentExecution, error) {
|
||||
idx := len(s.getCalls)
|
||||
s.getCalls = append(s.getCalls, sessionID)
|
||||
if idx >= len(s.getResults) {
|
||||
return nil, errors.New("stub exhausted")
|
||||
}
|
||||
return s.getResults[idx], s.getErrors[idx]
|
||||
}
|
||||
|
||||
func (s *stubShellLifecycle) CleanupStaleExecutionBySessionID(_ context.Context, sessionID string) error {
|
||||
s.cleanupCalls = append(s.cleanupCalls, sessionID)
|
||||
return s.cleanupErr
|
||||
}
|
||||
|
||||
func TestEnsureShellExecution_HappyPath(t *testing.T) {
|
||||
stub := &stubShellLifecycle{
|
||||
getResults: []*lifecycle.AgentExecution{{ID: "exec-1", Status: v1.AgentStatusRunning}},
|
||||
getErrors: []error{nil},
|
||||
}
|
||||
|
||||
got, err := ensureShellExecution(context.Background(), stub, newTestLogger(), "session-1")
|
||||
if err != nil {
|
||||
t.Fatalf("happy-path returned error: %v", err)
|
||||
}
|
||||
if got.ID != "exec-1" {
|
||||
t.Errorf("got execution %q, want exec-1", got.ID)
|
||||
}
|
||||
if len(stub.getCalls) != 1 {
|
||||
t.Errorf("expected 1 GetOrEnsureExecution call, got %d", len(stub.getCalls))
|
||||
}
|
||||
if len(stub.cleanupCalls) != 0 {
|
||||
t.Errorf("running execution should not trigger cleanup, got %d calls", len(stub.cleanupCalls))
|
||||
}
|
||||
}
|
||||
|
||||
func TestEnsureShellExecution_RecoversStaleExecution(t *testing.T) {
|
||||
// First call returns a Failed execution → cleanup → second call returns
|
||||
// a healthy one. This is the recovery path that survives a crashed
|
||||
// agent: the user retries shell ops, kandev cleans the stale row, and
|
||||
// re-launches transparently.
|
||||
stub := &stubShellLifecycle{
|
||||
getResults: []*lifecycle.AgentExecution{
|
||||
{ID: "exec-stale", Status: v1.AgentStatusFailed},
|
||||
{ID: "exec-fresh", Status: v1.AgentStatusRunning},
|
||||
},
|
||||
getErrors: []error{nil, nil},
|
||||
}
|
||||
|
||||
got, err := ensureShellExecution(context.Background(), stub, newTestLogger(), "session-1")
|
||||
if err != nil {
|
||||
t.Fatalf("recovery path returned error: %v", err)
|
||||
}
|
||||
if got.ID != "exec-fresh" {
|
||||
t.Errorf("got %q, want exec-fresh after recovery", got.ID)
|
||||
}
|
||||
if len(stub.cleanupCalls) != 1 || stub.cleanupCalls[0] != "session-1" {
|
||||
t.Errorf("expected exactly one CleanupStaleExecutionBySessionID(session-1), got %v", stub.cleanupCalls)
|
||||
}
|
||||
if len(stub.getCalls) != 2 {
|
||||
t.Errorf("expected 2 GetOrEnsureExecution calls (initial + recovery), got %d", len(stub.getCalls))
|
||||
}
|
||||
}
|
||||
|
||||
func TestEnsureShellExecution_InitialErrorWrapped(t *testing.T) {
|
||||
stub := &stubShellLifecycle{
|
||||
getResults: []*lifecycle.AgentExecution{nil},
|
||||
getErrors: []error{errors.New("manager unavailable")},
|
||||
}
|
||||
|
||||
_, err := ensureShellExecution(context.Background(), stub, newTestLogger(), "session-1")
|
||||
if err == nil {
|
||||
t.Fatal("expected error when initial GetOrEnsureExecution fails")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "no agent running for session session-1") {
|
||||
t.Errorf("error message does not surface session id: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestEnsureShellExecution_CleanupErrorPropagates(t *testing.T) {
|
||||
stub := &stubShellLifecycle{
|
||||
getResults: []*lifecycle.AgentExecution{{ID: "exec-stale", Status: v1.AgentStatusFailed}},
|
||||
getErrors: []error{nil},
|
||||
cleanupErr: errors.New("cleanup denied"),
|
||||
}
|
||||
|
||||
_, err := ensureShellExecution(context.Background(), stub, newTestLogger(), "session-1")
|
||||
if err == nil {
|
||||
t.Fatal("expected cleanup failure to surface")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "cleanup stale execution") || !strings.Contains(err.Error(), "session-1") {
|
||||
t.Errorf("cleanup error not propagated cleanly: %v", err)
|
||||
}
|
||||
// Second GetOrEnsureExecution must NOT fire when cleanup fails: re-
|
||||
// launching on a stuck stale row would loop forever.
|
||||
if len(stub.getCalls) != 1 {
|
||||
t.Errorf("expected exactly 1 GetOrEnsureExecution call when cleanup fails, got %d", len(stub.getCalls))
|
||||
}
|
||||
}
|
||||
|
||||
func TestEnsureShellExecution_RecoveryRelaunchFailureWrapped(t *testing.T) {
|
||||
stub := &stubShellLifecycle{
|
||||
getResults: []*lifecycle.AgentExecution{
|
||||
{ID: "exec-stale", Status: v1.AgentStatusFailed},
|
||||
nil,
|
||||
},
|
||||
getErrors: []error{nil, errors.New("relaunch failed")},
|
||||
}
|
||||
|
||||
_, err := ensureShellExecution(context.Background(), stub, newTestLogger(), "session-1")
|
||||
if err == nil {
|
||||
t.Fatal("expected error when recovery re-launch fails")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "recover execution") {
|
||||
t.Errorf("recovery error not labeled clearly: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user