From 35d1a85d741624e2e1c02ab7b793b31324b2e556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 28 Jul 2026 09:23:30 +0100 Subject: [PATCH] fix: recover failed agent resumes and managed runtimes (#1991) --- .../agent/agents/managed_npm_runtime.go | 13 ++ .../agent/agents/managed_npm_runtime_test.go | 7 + .../runtime/lifecycle/manager_interaction.go | 14 ++ .../lifecycle/manager_interaction_test.go | 26 +++ .../agent/runtime/lifecycle/manager_launch.go | 17 ++ .../runtime/lifecycle/manager_launch_test.go | 4 + .../agent/settings/controller/agent_update.go | 38 +++++ .../settings/controller/agent_update_job.go | 16 +- .../settings/controller/agent_update_test.go | 77 +++++++++ .../handlers/agent_update_handlers_test.go | 4 + .../agentctl/server/config/config_test.go | 26 +++ .../orchestrator/event_handlers_agent.go | 95 ++++------- .../orchestrator/event_handlers_test.go | 63 +------ .../execution_teardown_ownership_test.go | 25 +++ .../orchestrator/executor/executor_resume.go | 89 ++++++++-- .../executor/executor_resume_test.go | 113 +++++++++++++ .../settings/copy-files-symlink-helpers.ts | 13 +- .../agent-resume-runtime-recovery/plan.md | 157 ++++++++++++++++++ .../task-01-preserve-resume-token.md | 55 ++++++ ...task-02-resume-state-before-credentials.md | 62 +++++++ .../task-03-managed-npm-cache-repair.md | 63 +++++++ docs/public/agents-and-profiles.md | 3 + docs/specs/INDEX.md | 1 + .../agent-resume-runtime-recovery/spec.md | 91 ++++++++++ docs/specs/agents/runtime-updates.md | 13 +- 25 files changed, 948 insertions(+), 137 deletions(-) create mode 100644 docs/plans/agent-resume-runtime-recovery/plan.md create mode 100644 docs/plans/agent-resume-runtime-recovery/task-01-preserve-resume-token.md create mode 100644 docs/plans/agent-resume-runtime-recovery/task-02-resume-state-before-credentials.md create mode 100644 docs/plans/agent-resume-runtime-recovery/task-03-managed-npm-cache-repair.md create mode 100644 docs/specs/agent-resume-runtime-recovery/spec.md diff --git a/apps/backend/internal/agent/agents/managed_npm_runtime.go b/apps/backend/internal/agent/agents/managed_npm_runtime.go index c38b95583b..b645d3e2c0 100644 --- a/apps/backend/internal/agent/agents/managed_npm_runtime.go +++ b/apps/backend/internal/agent/agents/managed_npm_runtime.go @@ -1,5 +1,10 @@ package agents +import ( + "crypto/sha512" + "encoding/hex" +) + // ManagedNPMRuntimeSpec defines a built-in npm-distributed ACP runtime. // Package and ACPArgs must come from trusted agent metadata, never request // input, because update jobs execute them directly. @@ -8,6 +13,14 @@ type ManagedNPMRuntimeSpec struct { ACPArgs []string } +// ExecutionCacheKey returns npm's deterministic _npx execution-tree key for +// this trusted package spec. npm derives it from the full package string using +// SHA-512 and the first 16 lowercase hexadecimal characters. +func (s ManagedNPMRuntimeSpec) ExecutionCacheKey() string { + digest := sha512.Sum512([]byte(s.Package)) + return hex.EncodeToString(digest[:])[:16] +} + // CachedACPCommand returns the normal launch command. The package is // intentionally unversioned and prefer-offline lets npm reuse a suitable // execution-cache entry while still fetching when the cache is empty. diff --git a/apps/backend/internal/agent/agents/managed_npm_runtime_test.go b/apps/backend/internal/agent/agents/managed_npm_runtime_test.go index 417b07fa2a..b78aa64270 100644 --- a/apps/backend/internal/agent/agents/managed_npm_runtime_test.go +++ b/apps/backend/internal/agent/agents/managed_npm_runtime_test.go @@ -52,6 +52,13 @@ func TestManagedNPMRuntimeContracts(t *testing.T) { } } +func TestManagedNPMRuntimeExecutionCacheKeyMatchesNPM(t *testing.T) { + spec := ManagedNPMRuntimeSpec{Package: "opencode-ai"} + if got := spec.ExecutionCacheKey(); got != "e2094862b59aac7b" { + t.Fatalf("ExecutionCacheKey = %q, want npm key e2094862b59aac7b", got) + } +} + func assertUnversionedPackage(t *testing.T, argv []string, wantPackage string) { t.Helper() packageArg := argv[3] diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 2abb103dae..50b40e8691 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -963,6 +963,8 @@ func (m *Manager) ListExecutions() []*AgentExecution { return m.executionStore.List() } +const agentStartupLivenessGrace = 75 * time.Second + // IsAgentRunningForSession checks if an agent process is running or starting for a session. // // For passthrough sessions (direct PTY mode), it checks whether the PTY process is alive @@ -986,6 +988,18 @@ func (m *Manager) IsAgentRunningForSession(ctx context.Context, sessionID string return false } + // Launch registers the execution before agentctl is guaranteed to answer + // status requests. During that bounded boot window, a failed status probe + // means "not ready yet", not "stale". Treat the execution-store STARTING + // state as live long enough to cover StartAgentProcess's 60-second agentctl + // readiness deadline; after the grace expires, normal probing allows stale + // starting entries to be reaped. + if execution.Status == v1.AgentStatusStarting && + !execution.StartedAt.IsZero() && + time.Since(execution.StartedAt) <= agentStartupLivenessGrace { + return true + } + // Passthrough sessions run as direct PTY processes via InteractiveRunner, // bypassing agentctl's ACP protocol. Check the process directly. if execution.PassthroughProcessID != "" { diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go index ae7b33965b..5562d33edb 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go @@ -823,6 +823,32 @@ func TestIsAgentRunningForSession(t *testing.T) { require.False(t, mgr.IsAgentRunningForSession(context.Background(), "nonexistent")) }) + t.Run("fresh starting execution is live before agentctl responds", func(t *testing.T) { + store := NewExecutionStore() + require.NoError(t, store.Add(&AgentExecution{ + ID: "exec-acp-starting", + SessionID: "session-acp-starting", + Status: v1.AgentStatusStarting, + StartedAt: time.Now(), + })) + + mgr := &Manager{executionStore: store, logger: newTestLogger().WithFields()} + require.True(t, mgr.IsAgentRunningForSession(context.Background(), "session-acp-starting")) + }) + + t.Run("expired starting execution without agentctl is stale", func(t *testing.T) { + store := NewExecutionStore() + require.NoError(t, store.Add(&AgentExecution{ + ID: "exec-acp-starting-stale", + SessionID: "session-acp-starting-stale", + Status: v1.AgentStatusStarting, + StartedAt: time.Now().Add(-5 * time.Minute), + })) + + mgr := &Manager{executionStore: store, logger: newTestLogger().WithFields()} + require.False(t, mgr.IsAgentRunningForSession(context.Background(), "session-acp-starting-stale")) + }) + t.Run("passthrough with alive process returns true", func(t *testing.T) { log := newTestLogger() runner := process.NewInteractiveRunner(nil, log, 2*1024*1024) diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index 0c44311ed0..7f2971ee5e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -23,6 +23,7 @@ import ( storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/worktree" + v1 "github.com/kandev/kandev/pkg/api/v1" ) const legacyExecutorTypeLocalPC = "local_pc" @@ -902,6 +903,7 @@ func (m *Manager) Launch(ctx context.Context, req *LaunchRequest) (*AgentExecuti activityLease.SetKind(activity.KindExecutionPreparing) execution, launchErr := m.launchInternal(ctx, req) if launchErr == nil && req.StartAgent { + m.markAgentStartPending(execution) m.trackActivity(executionActivityKey(execution.ID), activityLease) transferredActivity = true } @@ -936,11 +938,26 @@ func (m *Manager) Launch(ctx context.Context, req *LaunchRequest) (*AgentExecuti if err != nil { return nil, err } + m.markAgentStartPending(execution) m.trackActivity(executionActivityKey(execution.ID), activityLease) } return execution, nil } +// markAgentStartPending distinguishes a launch that owns an imminent agent +// start from a workspace-only execution. It happens synchronously before +// Launch returns so a concurrent resume cannot mistake the registered runtime +// for stale state while StartAgentProcess is waiting for agentctl readiness. +func (m *Manager) markAgentStartPending(execution *AgentExecution) { + if execution == nil { + return + } + _ = m.executionStore.WithLock(execution.ID, func(current *AgentExecution) { + current.Status = v1.AgentStatusStarting + current.StartedAt = time.Now() + }) +} + // promoteWorkspaceExecution populates the agent command fields on a // workspace-only execution so a subsequent StartAgentProcess() can configure // and start the agent subprocess. Concurrent promoters serialize through a diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go index 33598fad58..d5be91d54a 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go @@ -29,6 +29,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/secrets" "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" ) // resumeTestAgent is a minimal agent with a BuildCommand that respects the @@ -1015,6 +1016,9 @@ func TestLaunchKeepsInitialExecutionActivityAfterReturning(t *testing.T) { if err != nil { t.Fatalf("Launch: %v", err) } + if execution.Status != v1.AgentStatusStarting { + t.Fatalf("execution status after start-owning Launch = %s, want %s", execution.Status, v1.AgentStatusStarting) + } maintenance, _, err := coordinator.TryAcquireMaintenance(context.Background(), 0) if maintenance != nil { maintenance.Release() diff --git a/apps/backend/internal/agent/settings/controller/agent_update.go b/apps/backend/internal/agent/settings/controller/agent_update.go index 62d64c5071..4c1518d54d 100644 --- a/apps/backend/internal/agent/settings/controller/agent_update.go +++ b/apps/backend/internal/agent/settings/controller/agent_update.go @@ -7,7 +7,9 @@ import ( "errors" "fmt" "io" + "os" "os/exec" + "path/filepath" "strings" "sync" @@ -69,6 +71,7 @@ type RuntimeUpdater interface { CurrentCapabilities(agentName string) (hostutility.AgentCapabilities, bool) ResolveTarget(ctx context.Context, packageName string) (string, error) RunUpdate(ctx context.Context, command agents.Command, onChunk func(string)) error + InvalidateExecutionCache(ctx context.Context, packageName string) error Refresh( ctx context.Context, agentName string, @@ -145,6 +148,41 @@ func (u *hostRuntimeUpdater) RunUpdate( return u.executor.Stream(ctx, command, onChunk) } +func (u *hostRuntimeUpdater) InvalidateExecutionCache(ctx context.Context, packageName string) error { + packageName = strings.TrimSpace(packageName) + if packageName == "" { + return errors.New("managed runtime package is empty") + } + output, err := u.executor.Output(ctx, agents.NewCommand("npm", "config", "get", "cache")) + if err != nil { + return fmt.Errorf("resolve npm cache root: %w", err) + } + cacheRoot := strings.TrimSpace(output) + if !filepath.IsAbs(cacheRoot) { + return fmt.Errorf("npm cache root is not absolute: %q", cacheRoot) + } + cacheRoot = filepath.Clean(cacheRoot) + if cacheRoot == string(filepath.Separator) { + return errors.New("refusing to invalidate execution cache under filesystem root") + } + npxRoot := filepath.Join(cacheRoot, "_npx") + if info, statErr := os.Lstat(npxRoot); statErr == nil && info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to invalidate execution cache through symlink: %s", npxRoot) + } else if statErr != nil && !errors.Is(statErr, os.ErrNotExist) { + return fmt.Errorf("inspect npm execution cache: %w", statErr) + } + key := (agents.ManagedNPMRuntimeSpec{Package: packageName}).ExecutionCacheKey() + target := filepath.Join(npxRoot, key) + rel, err := filepath.Rel(npxRoot, target) + if err != nil || rel != key { + return fmt.Errorf("invalid npm execution cache target: %s", target) + } + if err := os.RemoveAll(target); err != nil { + return fmt.Errorf("remove npm execution cache %s: %w", target, err) + } + return nil +} + func (u *hostRuntimeUpdater) Refresh( ctx context.Context, agentName string, diff --git a/apps/backend/internal/agent/settings/controller/agent_update_job.go b/apps/backend/internal/agent/settings/controller/agent_update_job.go index a5b012c7af..e1d0c34a54 100644 --- a/apps/backend/internal/agent/settings/controller/agent_update_job.go +++ b/apps/backend/internal/agent/settings/controller/agent_update_job.go @@ -150,8 +150,20 @@ func (s *AgentUpdateJobStore) run( err = s.updater.RunUpdate(ctx, spec.CacheUpdateCommand(), flusher.append) flusher.flush() if err != nil { - s.finishFailed(job, ctx, fmt.Errorf("update runtime: %w", err), ref) - return + flusher.append("managed runtime cache appears stale; repairing execution cache\n") + flusher.flush() + if repairErr := s.updater.InvalidateExecutionCache(ctx, spec.Package); repairErr != nil { + s.finishFailed(job, ctx, fmt.Errorf("repair runtime execution cache: %w", repairErr), ref) + return + } + flusher.append("retrying managed runtime update\n") + flusher.flush() + err = s.updater.RunUpdate(ctx, spec.CacheUpdateCommand(), flusher.append) + flusher.flush() + if err != nil { + s.finishFailed(job, ctx, fmt.Errorf("update runtime after cache repair: %w", err), ref) + return + } } s.setStatus(job, dto.AgentUpdateJobStatusRefreshing) diff --git a/apps/backend/internal/agent/settings/controller/agent_update_test.go b/apps/backend/internal/agent/settings/controller/agent_update_test.go index a5e80b1ee3..73c1fd142b 100644 --- a/apps/backend/internal/agent/settings/controller/agent_update_test.go +++ b/apps/backend/internal/agent/settings/controller/agent_update_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "os" + "path/filepath" "strings" "sync" "testing" @@ -23,6 +25,8 @@ type fakeRuntimeUpdater struct { target string resolveErr error runErr error + runErrs []error + invalidateErr error refreshCaps hostutility.AgentCapabilities refreshErr error runCommand []string @@ -31,6 +35,8 @@ type fakeRuntimeUpdater struct { runStarted chan struct{} releaseRun chan struct{} runCalls int + invalidateCalls int + invalidatePkg string refreshCalls int resolvedPackage string } @@ -73,6 +79,35 @@ func TestHostRuntimeUpdaterResolvesTargetWithDirectNPMArgv(t *testing.T) { } } +func TestHostRuntimeUpdaterInvalidatesOnlyManagedNPMExecutionTree(t *testing.T) { + cacheRoot := t.TempDir() + spec := agents.ManagedNPMRuntimeSpec{Package: "opencode-ai"} + target := filepath.Join(cacheRoot, "_npx", spec.ExecutionCacheKey()) + other := filepath.Join(cacheRoot, "_npx", "keep-me") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatalf("mkdir target: %v", err) + } + if err := os.MkdirAll(other, 0o755); err != nil { + t.Fatalf("mkdir other: %v", err) + } + + executor := &recordingCommandExecutor{output: cacheRoot + "\n"} + updater := &hostRuntimeUpdater{executor: executor} + if err := updater.InvalidateExecutionCache(context.Background(), spec.Package); err != nil { + t.Fatalf("InvalidateExecutionCache: %v", err) + } + if _, err := os.Stat(target); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("target stat error = %v, want not exists", err) + } + if _, err := os.Stat(other); err != nil { + t.Fatalf("unrelated cache entry was removed: %v", err) + } + want := []string{"npm", "config", "get", "cache"} + if got := strings.Join(executor.outputCommand, "\x00"); got != strings.Join(want, "\x00") { + t.Fatalf("command = %v, want %v", executor.outputCommand, want) + } +} + func (f *fakeRuntimeUpdater) CurrentCapabilities(string) (hostutility.AgentCapabilities, bool) { return f.current, f.currentFound } @@ -96,6 +131,9 @@ func (f *fakeRuntimeUpdater) RunUpdate( release := f.releaseRun output := f.updateOutput err := f.runErr + if f.runCalls-1 < len(f.runErrs) { + err = f.runErrs[f.runCalls-1] + } f.mu.Unlock() if started != nil { select { @@ -115,6 +153,14 @@ func (f *fakeRuntimeUpdater) RunUpdate( return err } +func (f *fakeRuntimeUpdater) InvalidateExecutionCache(_ context.Context, packageName string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.invalidateCalls++ + f.invalidatePkg = packageName + return f.invalidateErr +} + func (f *fakeRuntimeUpdater) Refresh( _ context.Context, _ string, @@ -365,6 +411,37 @@ func TestAgentUpdateRegistryFailureStopsBeforeMutation(t *testing.T) { } } +func TestAgentUpdateRepairsExecutionCacheAndRetriesOnce(t *testing.T) { + updater := &fakeRuntimeUpdater{ + target: "1.1.0", + runErrs: []error{errors.New("truncated npm execution tree"), nil}, + refreshCaps: hostutility.AgentCapabilities{ + Status: hostutility.StatusOK, + AgentVersion: "1.1.0", + }, + } + store, completed := newUpdateTestStore(updater, newMaintenanceCoordinator(), nil) + + job, err := store.Enqueue("managed-acp", managedRuntimeSpec()) + if err != nil { + t.Fatalf("Enqueue: %v", err) + } + final := waitForUpdateStatus(t, completed, job.ID, dto.AgentUpdateJobStatusSucceeded) + if !strings.Contains(final.Output, "repairing execution cache") || + !strings.Contains(final.Output, "retrying managed runtime update") { + t.Fatalf("recovery output = %q", final.Output) + } + + updater.mu.Lock() + defer updater.mu.Unlock() + if updater.runCalls != 2 { + t.Fatalf("update calls = %d, want 2", updater.runCalls) + } + if updater.invalidateCalls != 1 || updater.invalidatePkg != managedRuntimeSpec().Package { + t.Fatalf("cache repair = %d calls for %q", updater.invalidateCalls, updater.invalidatePkg) + } +} + func TestAgentUpdateHardFailuresRemainFailed(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/agent/settings/handlers/agent_update_handlers_test.go b/apps/backend/internal/agent/settings/handlers/agent_update_handlers_test.go index e47255b1f8..0b96e71a7b 100644 --- a/apps/backend/internal/agent/settings/handlers/agent_update_handlers_test.go +++ b/apps/backend/internal/agent/settings/handlers/agent_update_handlers_test.go @@ -55,6 +55,10 @@ func (u *handlerRuntimeUpdater) RunUpdate( return nil } +func (u *handlerRuntimeUpdater) InvalidateExecutionCache(context.Context, string) error { + return nil +} + func (u *handlerRuntimeUpdater) Refresh( context.Context, string, diff --git a/apps/backend/internal/agentctl/server/config/config_test.go b/apps/backend/internal/agentctl/server/config/config_test.go index 2eb264c931..bd78b615dd 100644 --- a/apps/backend/internal/agentctl/server/config/config_test.go +++ b/apps/backend/internal/agentctl/server/config/config_test.go @@ -19,6 +19,7 @@ func TestCollectAgentEnvKeepsGitHubCLIShimAheadOfProfilePath(t *testing.T) { } func TestCollectAgentEnvPreservesParentIndexedGitConfig(t *testing.T) { + clearParentIndexedGitConfig(t) t.Setenv("GIT_CONFIG_COUNT", "2") t.Setenv("GIT_CONFIG_KEY_0", "core.hooksPath") t.Setenv("GIT_CONFIG_VALUE_0", "/opt/locstat/hooks") @@ -45,6 +46,7 @@ func TestCollectAgentEnvPreservesParentIndexedGitConfig(t *testing.T) { } func TestCollectAgentEnvPreservesParentGitConfigHook(t *testing.T) { + clearParentIndexedGitConfig(t) repository := t.TempDir() hooks := t.TempDir() marker := filepath.Join(t.TempDir(), "hook-ran") @@ -75,6 +77,30 @@ func TestCollectAgentEnvPreservesParentGitConfigHook(t *testing.T) { } } +func clearParentIndexedGitConfig(t *testing.T) { + t.Helper() + original := make(map[string]string) + for _, entry := range os.Environ() { + key, value, ok := strings.Cut(entry, "=") + if !ok || (key != "GIT_CONFIG_COUNT" && + !strings.HasPrefix(key, "GIT_CONFIG_KEY_") && + !strings.HasPrefix(key, "GIT_CONFIG_VALUE_")) { + continue + } + original[key] = value + if err := os.Unsetenv(key); err != nil { + t.Fatalf("unset inherited %s: %v", key, err) + } + } + t.Cleanup(func() { + for key, value := range original { + if err := os.Setenv(key, value); err != nil { + t.Errorf("restore inherited %s: %v", key, err) + } + } + }) +} + func runGit(t *testing.T, env []string, args ...string) { t.Helper() command := exec.Command("git", args...) diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index 00427c450d..1893f7ca23 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -827,26 +827,15 @@ func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.Agen ) // Terminal from here. Finalize run-mode automation runs — every branch - // below returns early (resume failure, session-backed recoverable failure, - // no-session retry), and run-mode automations need their AutomationRun - // flipped + worktree reaped on *every* terminal failure path. + // below returns early (session-backed recoverable failure, no-session retry), + // and run-mode automations need their AutomationRun flipped + worktree reaped + // on *every* terminal failure path. errMsg := data.ErrorMessage if errMsg == "" { errMsg = "agent failed" } s.finalizeAutomationRunIfEphemeral(ctx, data.TaskID, data.SessionID, false, errMsg) - // Check if the agent was started with a resume token AND session init hadn't completed. - // If init completed, this is a normal prompt failure (e.g. agent internal timeout), - // not a resume failure — skip the resume cleanup path. - if data.SessionID != "" && s.wasResumeAttempt(ctx, data.SessionID) && - !s.agentManager.WasSessionInitialized(data.AgentExecutionID) { - if s.handleResumeFailure(ctx, data) { - return // Resume token cleared, session set to WAITING_FOR_INPUT - } - // Fall through to normal failure handling if cleanup failed - } - // Make all agent CLI failures recoverable — let the user choose to resume or start fresh. if data.SessionID != "" { s.handleRecoverableFailure(ctx, data) @@ -1035,6 +1024,18 @@ func (s *Service) deleteExecutionTeardownClaimIfExpired(key string, expiresAt ti } } +func (s *Service) hasExecutionTeardownOwner(sessionID, executionID string) bool { + if sessionID == "" || executionID == "" { + return false + } + value, ok := s.executionTeardownClaims.Load(terminalExecutionKey(sessionID, executionID)) + if !ok { + return false + } + claim, ok := value.(executionTeardownClaim) + return ok && time.Now().Before(claim.expiresAt) +} + // wasResumeAttempt checks whether the session's last execution used a resume token. // If the token is still present in the DB, the agent was started with --resume. func (s *Service) wasResumeAttempt(ctx context.Context, sessionID string) bool { @@ -1046,8 +1047,9 @@ func (s *Service) wasResumeAttempt(ctx context.Context, sessionID string) bool { } // clearResumeToken removes the resume token from the executor running record so -// the next agent start won't use --resume. Used by both automatic resume failure -// handling and user-initiated fresh start recovery. +// the next agent start won't use --resume. It is reserved for explicit +// user-initiated fresh-start recovery; ordinary ACP startup failures retain the +// token so the session can be retried. // // Unconditional clear: passes expectedExecID="" so the narrow update is not // CAS-guarded — clearing a token is always intentional regardless of which @@ -1061,54 +1063,6 @@ func (s *Service) clearResumeToken(ctx context.Context, sessionID string) { } } -// handleResumeFailure handles the case where an agent failed while using a resume token. -// It clears the token so the next attempt starts fresh, and notifies the user. -// -// The session is set to WAITING_FOR_INPUT so the user can send a new message -// (which triggers a fresh agent start without --resume). -// -// Returns true to signal that the caller should skip normal failure handling -// (scheduler retry, FAILED state) since we've handled the state transition ourselves. -func (s *Service) handleResumeFailure(ctx context.Context, data watcher.AgentEventData) bool { - s.logger.Warn("detected resume failure, clearing token for fresh start on next user action", - zap.String("task_id", data.TaskID), - zap.String("session_id", data.SessionID), - zap.String("error", data.ErrorMessage)) - - // 1. Clear the resume token so the next attempt won't use --resume. - s.clearResumeToken(ctx, data.SessionID) - - // 2. Send a status message about the failed resume. - if s.messageCreator != nil { - statusMsg := fmt.Sprintf("Previous agent session could not be restored (%s). Send a new message to start a fresh session.", data.ErrorMessage) - if err := s.messageCreator.CreateSessionMessage( - ctx, - data.TaskID, - statusMsg, - data.SessionID, - string(v1.MessageTypeStatus), - s.getActiveTurnID(data.SessionID), - map[string]interface{}{ - "variant": "warning", - "resume_failed": true, - }, - false, - ); err != nil { - s.logger.Warn("failed to create resume failure status message", - zap.String("task_id", data.TaskID), - zap.Error(err)) - } - } - - // 3. Set session to WAITING_FOR_INPUT (not FAILED) so the user can interact. - s.updateTaskSessionState(ctx, data.TaskID, data.SessionID, models.TaskSessionStateWaitingForInput, "", false) - - // 4. Ensure task is in REVIEW state unless another session is still working. - s.writeTaskReviewState(ctx, data.TaskID, data.SessionID) - - return true -} - // handleRecoverableFailure handles agent failures by keeping the session recoverable. // Instead of marking the session FAILED (terminal), it sets WAITING_FOR_INPUT and // creates an error message with recovery action buttons so the user can choose to @@ -1395,6 +1349,19 @@ func (s *Service) handleAgentStopped(ctx context.Context, data watcher.AgentEven // on that self-inflicted stop would abort the retry. The loop is freed on // ready/completed (success), cancel, and exhaustion instead. + // An explicit teardown owner already decided and persisted the session + // state before stopping this exact execution. Treat the resulting stopped + // event as an acknowledgement, not a new state decision. Recovery may have + // advanced the row to STARTING before the predecessor's delayed stop event + // arrives, while the replacement is not yet visible in the runtime store. + if s.hasExecutionTeardownOwner(data.SessionID, data.AgentExecutionID) { + s.logger.Info("ignoring agent.stopped for explicitly owned teardown", + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("agent_execution_id", data.AgentExecutionID)) + return + } + // Drop stopped events that belong to a previous (rotated) execution. The // session might already be running a fresh resume cycle; flipping its state // to CANCELLED based on the corpse of the prior execution poisons the diff --git a/apps/backend/internal/orchestrator/event_handlers_test.go b/apps/backend/internal/orchestrator/event_handlers_test.go index fb21dfb9a1..aba873aa86 100644 --- a/apps/backend/internal/orchestrator/event_handlers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_test.go @@ -2512,60 +2512,6 @@ func TestHandleAgentStartFailed(t *testing.T) { }) } -func TestHandleResumeFailure(t *testing.T) { - ctx := context.Background() - - t.Run("clears resume token and sets WAITING_FOR_INPUT", func(t *testing.T) { - repo := setupTestRepo(t) - now := time.Now().UTC() - seedSession(t, repo, "t1", "s1", "step1") - - // Add executor running with resume token - _ = repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ - ID: "er1", SessionID: "s1", TaskID: "t1", ResumeToken: "acp-session-old", - CreatedAt: now, UpdatedAt: now, - }) - - taskRepo := newMockTaskRepo() - seedMockTaskState(taskRepo, "t1", v1.TaskStateInProgress) - svc := createTestService(repo, newMockStepGetter(), taskRepo) - - result := svc.handleResumeFailure(ctx, watcher.AgentEventData{ - TaskID: "t1", - SessionID: "s1", - AgentExecutionID: "exec-1", - ErrorMessage: "resume failed: session expired", - }) - - if !result { - t.Error("expected handleResumeFailure to return true") - } - - // Verify resume token was cleared - running, err := repo.GetExecutorRunningBySessionID(ctx, "s1") - if err != nil { - t.Fatalf("failed to get executor running: %v", err) - } - if running.ResumeToken != "" { - t.Errorf("expected empty resume token, got %q", running.ResumeToken) - } - - // Verify session state - session, err := repo.GetTaskSession(ctx, "s1") - if err != nil { - t.Fatalf("failed to get session: %v", err) - } - if session.State != models.TaskSessionStateWaitingForInput { - t.Errorf("expected session state %q, got %q", models.TaskSessionStateWaitingForInput, session.State) - } - - // Verify task moved to REVIEW - if state, ok := taskRepo.updatedStates["t1"]; !ok || state != v1.TaskStateReview { - t.Errorf("expected task state %q, got %q (ok=%v)", v1.TaskStateReview, state, ok) - } - }) -} - func TestHandleAgentFailed_RecoverableWithSession(t *testing.T) { ctx := context.Background() @@ -2602,7 +2548,7 @@ func TestHandleAgentFailed_RecoverableWithSession(t *testing.T) { } }) - t.Run("routes to resume failure when resume token exists and init not completed", func(t *testing.T) { + t.Run("retains resume token when ACP fails before initialization", func(t *testing.T) { repo := setupTestRepo(t) now := time.Now().UTC() seedSession(t, repo, "t1", "s1", "step1") @@ -2623,13 +2569,14 @@ func TestHandleAgentFailed_RecoverableWithSession(t *testing.T) { ErrorMessage: "resume failed", }) - // Resume token should be cleared + // A pre-ACP failure is retryable; only explicit fresh-start recovery clears + // the provider-native session identity. running, err := repo.GetExecutorRunningBySessionID(ctx, "s1") if err != nil { t.Fatalf("failed to get executor running: %v", err) } - if running.ResumeToken != "" { - t.Errorf("expected resume token to be cleared, got %q", running.ResumeToken) + if running.ResumeToken != "acp-session-old" { + t.Errorf("expected resume token to be retained, got %q", running.ResumeToken) } // Session should be WAITING_FOR_INPUT diff --git a/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go b/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go index 53e019fee3..c226ea82cc 100644 --- a/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go +++ b/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go @@ -8,6 +8,7 @@ import ( "time" orchestratorexec "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/orchestrator/watcher" "github.com/kandev/kandev/internal/task/models" "github.com/stretchr/testify/require" ) @@ -53,6 +54,30 @@ func TestRegisterExecutionStopOwner_SuppressesOrphanCleanupAndRecordsForceEscala require.Equal(t, executionTeardownIntentForce, claim.intent) } +func TestHandleAgentStopped_OwnedPredecessorDoesNotCancelStartingReplacement(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + const ( + taskID = "task-stale-stop" + sessionID = "session-stale-stop" + oldExecutionID = "execution-stale-stop" + ) + seedTaskAndSession(t, repo, taskID, sessionID, models.TaskSessionStateStarting) + + svc := newCoordinatorStopTestService(repo, newMockTaskRepo(), &mockAgentManager{}) + svc.RegisterExecutionStopOwner(sessionID, oldExecutionID, true) + + svc.handleAgentStopped(ctx, watcher.AgentEventData{ + TaskID: taskID, + SessionID: sessionID, + AgentExecutionID: oldExecutionID, + }) + + session, err := repo.GetTaskSession(ctx, sessionID) + require.NoError(t, err) + require.Equal(t, models.TaskSessionStateStarting, session.State) +} + func TestCleanupAgentExecution_CancelledSessionUsesExactExecutionOwnership(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 6d472f2748..a55c62df90 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -409,11 +409,13 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio zap.String("resume_token", req.ACPSessionID), zap.Bool("use_worktree", req.UseWorktree)) + resumeInitialState := session.State + wasTerminalResume := isTerminalSessionState(resumeInitialState) // Force-cleanup any stale in-memory execution / agentctl state for terminal-state // sessions. Their agent process is dead by definition, so "already running" signals // from the execution store or agentctl's "starting" status are stale and would // otherwise block the relaunch. - if isTerminalSessionState(session.State) { + if wasTerminalResume { if cleanupErr := e.agentManager.CleanupStaleExecutionBySessionID(ctx, session.ID); cleanupErr != nil { e.logger.Warn("failed to force-cleanup stale execution before terminal-state resume", zap.String("session_id", session.ID), @@ -423,6 +425,15 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio req.Env = e.applyPreferredShellEnv(ctx, req.ExecutorType, req.Env) + // Persist STARTING before LaunchAgent requests provider credentials. The + // credential broker rejects terminal sessions, so the guarded transition + // must be visible before the launch boundary is crossed. + if startAgent { + if err := e.persistResumeState(ctx, task.ID, session, true); err != nil { + return nil, err + } + } + resp, err := e.agentManager.LaunchAgent(ctx, req) if err != nil && isAgentAlreadyRunningError(err) { // "already has an agent running" fires both for live executions (a concurrent @@ -432,10 +443,13 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio // so skip the probe and go straight to cleanup+retry — this avoids a silent // regression to ErrExecutionAlreadyRunning if the preemptive cleanup above // failed and agentctl still reports a stale "starting" status. - if !isTerminalSessionState(session.State) && e.agentManager.IsAgentRunningForSession(ctx, session.ID) { + if !wasTerminalResume && e.agentManager.IsAgentRunningForSession(ctx, session.ID) { e.logger.Info("resume race: agent already running for session, returning ErrExecutionAlreadyRunning", zap.String("task_id", task.ID), zap.String("session_id", session.ID)) + if startAgent { + e.rollbackResumeStateAfterLaunchFailure(ctx, task.ID, session.ID, resumeInitialState, err) + } return nil, ErrExecutionAlreadyRunning } e.logger.Info("cleaning up stale execution and retrying launch", @@ -449,6 +463,9 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio resp, err = e.agentManager.LaunchAgent(ctx, req) } if err != nil { + if startAgent { + e.rollbackResumeStateAfterLaunchFailure(ctx, task.ID, session.ID, resumeInitialState, err) + } e.logger.Error("failed to relaunch agent for session", zap.String("task_id", task.ID), zap.String("session_id", session.ID), @@ -456,9 +473,11 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio return nil, err } - if err := e.persistResumeState(ctx, task.ID, session, startAgent); err != nil { - e.cleanupUnstartedExecutionAfterPersistError(ctx, session.ID, resp.AgentExecutionID, err) - return nil, err + if !startAgent { + if err := e.persistResumeState(ctx, task.ID, session, false); err != nil { + e.cleanupUnstartedExecutionAfterPersistError(ctx, session.ID, resp.AgentExecutionID, err) + return nil, err + } } e.persistWorktreeAssociation(ctx, task.ID, session, repositoryID, resp) // Refresh task_environments after a successful resume so the row reflects @@ -499,6 +518,54 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio return execution, nil } +// rollbackResumeStateAfterLaunchFailure restores the state observed before a +// resume attempt only while the session is still STARTING. A concurrent +// terminal transition wins and is left untouched by transitionSessionState. +func (e *Executor) rollbackResumeStateAfterLaunchFailure(ctx context.Context, taskID, sessionID string, priorState models.TaskSessionState, launchErr error) { + if e.onSessionStateTransition != nil { + current, err := e.repo.GetTaskSession(ctx, sessionID) + if err != nil || current == nil || current.State != models.TaskSessionStateStarting { + return + } + _, _, rollbackErr := e.transitionSessionState(ctx, taskID, sessionID, priorState, launchErr.Error()) + if rollbackErr != nil { + e.logger.Warn("failed to roll back session state after resume launch failure", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(rollbackErr)) + } + return + } + if updater, ok := e.repo.(interface { + UpdateTaskSessionStateIfCurrent(context.Context, string, models.TaskSessionState, models.TaskSessionState, string) (bool, time.Time, error) + }); ok { + if _, _, err := updater.UpdateTaskSessionStateIfCurrent( + ctx, + sessionID, + models.TaskSessionStateStarting, + priorState, + launchErr.Error(), + ); err != nil { + e.logger.Warn("failed to roll back session state after resume launch failure", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(err)) + } + return + } + current, err := e.repo.GetTaskSession(ctx, sessionID) + if err != nil || current == nil || current.State != models.TaskSessionStateStarting { + return + } + _, _, rollbackErr := e.transitionSessionState(ctx, taskID, sessionID, priorState, launchErr.Error()) + if rollbackErr != nil { + e.logger.Warn("failed to roll back session state after resume launch failure", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(rollbackErr)) + } +} + // validateAndLockResume validates the session is resumable, acquires the per-session lock, // and loads the associated task. Returns the task, an unlock function, and any error. // The caller must call unlock() when the critical section is complete. @@ -597,6 +664,7 @@ func (e *Executor) buildResumeRequest(ctx context.Context, task *v1.Task, sessio TaskTitle: task.Title, AgentProfileID: executionProfileID, OfficeAgentProfileID: session.AgentProfileID, + StartAgent: startAgent, TaskDescription: task.Description, Priority: task.Priority, IsEphemeral: task.IsEphemeral, @@ -942,11 +1010,12 @@ func resolveResumeTaskDirName(existingEnv *models.TaskEnvironment, task *v1.Task return worktree.SemanticWorktreeName(task.Title, worktree.SmallSuffix(3)) } -// persistResumeState updates the session row after a successful resume launch. -// Like persistLaunchState, executors_running is owned by the lifecycle manager -// and not touched here — see lifecycle.persistExecutorRunning. The -// orchestrator's only remaining responsibility is the session-row state -// machine (STARTING / CompletedAt-clear). +// persistResumeState updates the session row for a resume launch. For an agent +// launch this is called before LaunchAgent so credential issuance observes the +// guarded STARTING state. Like persistLaunchState, executors_running is owned +// by the lifecycle manager and not touched here — see +// lifecycle.persistExecutorRunning. The orchestrator's remaining +// responsibility is the session-row state machine (STARTING / CompletedAt-clear). func (e *Executor) persistResumeState(ctx context.Context, taskID string, session *models.TaskSession, startAgent bool) error { expectedState := session.State session.ErrorMessage = "" diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_test.go index 45141a85fe..bfd32b56bf 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_test.go @@ -71,6 +71,119 @@ func setupLiveResumeTestFixture(repo *mockRepository) { } } +func TestResumeSession_PersistsStartingBeforeLaunch(t *testing.T) { + repo := newMockRepository() + setupLiveResumeTestFixture(repo) + repo.sessions["sess-1"].State = models.TaskSessionStateFailed + + agentMgr := &mockAgentManager{ + launchAgentFunc: func(ctx context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { + if !req.StartAgent { + return nil, errors.New("resume launch did not retain startup ownership") + } + current, err := repo.GetTaskSession(ctx, "sess-1") + if err != nil { + return nil, err + } + if current.State != models.TaskSessionStateStarting { + return nil, fmt.Errorf("session state at launch = %s, want %s", current.State, models.TaskSessionStateStarting) + } + return &LaunchAgentResponse{AgentExecutionID: "exec-new", Status: v1.AgentStatusStarting}, nil + }, + } + exec := newTestExecutor(t, agentMgr, repo) + + if _, err := exec.ResumeSession(context.Background(), repo.sessions["sess-1"], true); err != nil { + t.Fatalf("ResumeSession: %v", err) + } +} + +func TestResumeSession_RollsBackStartingWhenLaunchFails(t *testing.T) { + repo := newMockRepository() + setupLiveResumeTestFixture(repo) + repo.sessions["sess-1"].State = models.TaskSessionStateFailed + + launchErr := errors.New("credential broker rejected launch") + agentMgr := &mockAgentManager{ + launchAgentFunc: func(ctx context.Context, _ *LaunchAgentRequest) (*LaunchAgentResponse, error) { + current, err := repo.GetTaskSession(ctx, "sess-1") + if err != nil { + return nil, err + } + if current.State != models.TaskSessionStateStarting { + return nil, fmt.Errorf("session state at launch = %s, want %s", current.State, models.TaskSessionStateStarting) + } + return nil, launchErr + }, + } + exec := newTestExecutor(t, agentMgr, repo) + + if _, err := exec.ResumeSession(context.Background(), repo.sessions["sess-1"], true); !errors.Is(err, launchErr) { + t.Fatalf("ResumeSession error = %v, want %v", err, launchErr) + } + current := repo.sessions["sess-1"] + if current.State != models.TaskSessionStateFailed { + t.Fatalf("session state after failed launch = %s, want %s", current.State, models.TaskSessionStateFailed) + } + if !strings.Contains(current.ErrorMessage, launchErr.Error()) { + t.Fatalf("session error = %q, want launch error", current.ErrorMessage) + } +} + +func TestRollbackResumeStateAfterLaunchFailure_SkipsTransitionAfterConcurrentStateChange(t *testing.T) { + repo := newMockRepository() + setupLiveResumeTestFixture(repo) + repo.sessions["sess-1"].State = models.TaskSessionStateCancelled + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetOnSessionStateTransition(func( + context.Context, + string, + string, + models.TaskSessionState, + string, + func(), + ) (bool, models.TaskSessionState, error) { + t.Fatal("state transition must not run after a concurrent state change") + return false, models.TaskSessionStateCancelled, nil + }) + + exec.rollbackResumeStateAfterLaunchFailure( + context.Background(), + "task-1", + "sess-1", + models.TaskSessionStateFailed, + errors.New("launch failed"), + ) + if got := repo.sessions["sess-1"].State; got != models.TaskSessionStateCancelled { + t.Fatalf("session state = %s, want %s", got, models.TaskSessionStateCancelled) + } +} + +func TestResumeSession_RollsBackStartingOnLiveAlreadyRunningRace(t *testing.T) { + repo := newMockRepository() + setupLiveResumeTestFixture(repo) + var runningChecks int + agentMgr := &mockAgentManager{ + launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { + return nil, fmt.Errorf("%w: session %q", lifecycle.ErrAgentAlreadyRunning, req.SessionID) + }, + isAgentRunningForSessionFunc: func(_ context.Context, _ string) bool { + runningChecks++ + return runningChecks > 1 + }, + } + exec := newTestExecutor(t, agentMgr, repo) + + _, err := exec.ResumeSession(context.Background(), repo.sessions["sess-1"], true) + if !errors.Is(err, ErrExecutionAlreadyRunning) { + t.Fatalf("ResumeSession error = %v, want ErrExecutionAlreadyRunning", err) + } + if repo.sessions["sess-1"].State != models.TaskSessionStateWaitingForInput { + t.Fatalf("session state after live race = %s, want %s", repo.sessions["sess-1"].State, models.TaskSessionStateWaitingForInput) + } +} + func TestResumeSession_PassesResolvedTaskSessionMCPModeToAgentManager(t *testing.T) { tests := []struct { name string diff --git a/apps/web/e2e/tests/settings/copy-files-symlink-helpers.ts b/apps/web/e2e/tests/settings/copy-files-symlink-helpers.ts index 0427195a73..f32bb88bd3 100644 --- a/apps/web/e2e/tests/settings/copy-files-symlink-helpers.ts +++ b/apps/web/e2e/tests/settings/copy-files-symlink-helpers.ts @@ -12,7 +12,18 @@ export async function configureSymlinkAndCreateTask(options: { const { page, apiClient, seedData, title } = options; await page.goto(`/settings/workspace/${seedData.workspaceId}/repositories`); await expect(page.getByRole("button", { name: "Add Local Repository" })).toBeVisible(); - await page.getByRole("button", { name: "Edit" }).first().click(); + + const repositoriesResponse = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/repositories`, + ); + expect(repositoriesResponse.ok).toBe(true); + const { repositories } = (await repositoriesResponse.json()) as { + repositories: Array<{ id: string; name: string }>; + }; + const repository = repositories.find(({ id }) => id === seedData.repositoryId); + if (!repository) throw new Error("seed repository missing from workspace repository list"); + await page.getByRole("heading", { name: repository.name, exact: true }).click(); const input = page.getByTestId(`copy-files-input-${seedData.repositoryId}`); await expect(input).toBeVisible(); diff --git a/docs/plans/agent-resume-runtime-recovery/plan.md b/docs/plans/agent-resume-runtime-recovery/plan.md new file mode 100644 index 0000000000..9ebc0d8a45 --- /dev/null +++ b/docs/plans/agent-resume-runtime-recovery/plan.md @@ -0,0 +1,157 @@ +--- +spec: docs/specs/agent-resume-runtime-recovery/spec.md +related_specs: + - docs/specs/agents/runtime-updates.md +created: 2026-07-27 +status: implemented +--- + +# Implementation Plan: Agent Resume and Runtime Recovery + +## Overview + +Preserve provider-native session identity across failures that happen before a +successful replacement, move the guarded resume-state transition ahead of +GitHub credential issuance, and make explicit managed npm updates repair one +corrupt package execution tree. The changes stay inside existing recovery, +credential, and update surfaces; there is no API or frontend contract change. + +## Confirmed root causes + +- `handleAgentFailed` classifies any pre-initialization failure with a non-empty + `executors_running.resume_token` as an unusable saved session and calls + `clearResumeToken`, even when ACP `initialize` never returned and + `session/load` was never reached. +- `Executor.ResumeSession` calls `agentManager.LaunchAgent` before + `persistResumeState`. GitHub credential issuance occurs inside that launch, + while `githubBrokerScopeAuthorizer` still sees `FAILED` or `CANCELLED` and + rejects the lease as terminal. +- `CacheUpdateCommand` can reuse an already-extracted `_npx` directory. npm's + package-content cache can be valid while that execution tree is truncated, + so another `npm exec` does not necessarily repair it. + +--- + +## Backend + +Tasks 01–03 are complete; implementation and focused verification are done. + +### Preserve resume identity on startup failure + +- Update + `apps/backend/internal/orchestrator/event_handlers_agent.go` so agent startup + and ACP initialization failures follow the existing recoverable-failure path + without automatically clearing the resume token. +- Keep `clearResumeToken` as the implementation of the explicit + `session.recover` `fresh_start` action. Remove or narrow the obsolete + automatic `handleResumeFailure` path and its misleading status message. +- Update `apps/backend/internal/orchestrator/event_handlers_test.go` with a + failing regression fixture containing a resume token, an uninitialized + execution, and an ACP initialize/transport failure. Assert that the token is + retained and recovery state/actions remain available. + +### Persist STARTING before credential issuance + +- Refactor `apps/backend/internal/orchestrator/executor/executor_resume.go` so + the existing `persistResumeState` guarded `FAILED|CANCELLED -> STARTING` + transition occurs under the per-session lock after stale-execution cleanup + but before `agentManager.LaunchAgent`. +- Capture whether the request began from a terminal state before mutating the + session snapshot, so the existing stale-agent fallback keeps its terminal + cleanup semantics. +- Add a guarded rollback helper that restores the prior recoverable state when + launch fails only if the session is still `STARTING`; preserve a concurrent + `COMPLETED`, `FAILED`, or `CANCELLED` winner. +- Keep + `apps/backend/internal/backendapp/services.go` terminal-session credential + rejection unchanged. +- Add executor-level and service-level regression coverage in + `apps/backend/internal/orchestrator/executor/executor_resume_test.go` and + `apps/backend/internal/orchestrator/task_operations_resume_test.go`. The fake + launch boundary must read the repository and reject any lease attempt unless + the session is already `STARTING`. + +### Repair one managed npm execution cache + +- Add a deterministic npm execution-cache key helper for + `agents.ManagedNPMRuntimeSpec` in + `apps/backend/internal/agent/agents/managed_npm_runtime.go`. The key is the + first 16 lowercase hex characters of SHA-512 over the trusted package spec, + matching npm's `_npx` package key. +- Extend the runtime-updater boundary in + `apps/backend/internal/agent/settings/controller/agent_update.go` with a + targeted execution-cache invalidation operation. The production + implementation obtains npm's cache root with direct argv + `npm config get cache`, requires an absolute clean root, constructs exactly + `/_npx/`, and removes only that path. +- Update + `apps/backend/internal/agent/settings/controller/agent_update_job.go` to + invoke the targeted repair only after the first update command fails, append + bounded recovery progress, and retry the same built-in update command once. + A repair or second update failure ends the job. +- Document the operator recovery behavior in + `docs/public/agents-and-profiles.md`. + +--- + +## Tests + +- **What:** a pre-ACP failure with a resume token retains the token and enters + ordinary recoverable failure handling; explicit fresh start still clears it. + **File:** `apps/backend/internal/orchestrator/event_handlers_test.go` and + `apps/backend/internal/orchestrator/session_launch_test.go`. + **How:** real temporary SQLite repository plus existing mock agent manager and + direct recovery-service call. +- **What:** terminal resume is persisted as `STARTING` before the launch + boundary requests credentials, terminal stale cleanup still runs, and a + launch error cannot strand `STARTING`. + **File:** + `apps/backend/internal/orchestrator/executor/executor_resume_test.go` and + `apps/backend/internal/orchestrator/task_operations_resume_test.go`. + **How:** repository-observing fake `LaunchAgent`, table-driven prior states, + and a concurrent-terminal-winner case. +- **What:** deterministic keys match npm for built-in packages and cache + invalidation cannot escape the resolved `_npx` root. + **File:** + `apps/backend/internal/agent/agents/managed_npm_runtime_test.go` and + `apps/backend/internal/agent/settings/controller/agent_update_test.go`. + **How:** table-driven key assertions plus a temporary cache root and fake + direct-command executor. +- **What:** an initial update failure triggers exactly one targeted repair and + one retry; success still requires refresh, while repair/retry failures are + terminal. + **File:** + `apps/backend/internal/agent/settings/controller/agent_update_test.go`. + **How:** fake runtime updater call ordering and terminal job snapshots. + +No E2E test is planned because the existing Resume, Start fresh, and Update +agent UI contracts do not change; backend tests exercise the repaired behavior. + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [x] [Task 01: Preserve resume tokens on startup failure](task-01-preserve-resume-token.md) +- [x] [Task 03: Repair corrupt managed npm execution caches](task-03-managed-npm-cache-repair.md) (`parallel-safe`; disjoint agent-settings/runtime files) + +Wave 2: + +- [x] [Task 02: Transition resume state before credential issuance](task-02-resume-state-before-credentials.md) + +Execution remains sequential in the primary conversation by default. The +parallel-safe label does not authorize delegation. + +## Validation commands + +- `make -C apps/backend test` +- Targeted commands are recorded in each task file and run before the broad + backend package check. + +## Risks and non-goals + +- npm's `_npx` key is an implementation detail; key tests pin the behavior used + by the supported npm runtime and a repair failure remains bounded. +- Moving `STARTING` earlier changes when observers see resume progress; guarded + rollback and state-race tests prevent stale active state. +- The plan does not weaken credential authorization, auto-start fresh + conversations, clean global npm caches, or add launch-time cache mutation. diff --git a/docs/plans/agent-resume-runtime-recovery/task-01-preserve-resume-token.md b/docs/plans/agent-resume-runtime-recovery/task-01-preserve-resume-token.md new file mode 100644 index 0000000000..2a32bd80c4 --- /dev/null +++ b/docs/plans/agent-resume-runtime-recovery/task-01-preserve-resume-token.md @@ -0,0 +1,55 @@ +--- +id: "01-preserve-resume-token" +title: "Preserve resume tokens on startup failure" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/agent-resume-runtime-recovery/spec.md" +--- + +# Task 01: Preserve resume tokens on startup failure + +## Acceptance + +- Process startup, ACP initialize, and pre-session transport failures do not + clear `executors_running.resume_token`. +- Those failures use the existing recoverable-failure state and action path. +- Explicit `session.recover` with `fresh_start` still clears the token before + launching fresh. + +## Verification + +- `cd apps/backend && go test ./internal/orchestrator -run 'TestHandleAgentFailed|TestRecoverSession'` + +The new token-retention regression must fail for the expected blank-token +assertion before production code changes and pass afterward. + +## Files likely touched + +- `apps/backend/internal/orchestrator/event_handlers_agent.go` +- `apps/backend/internal/orchestrator/event_handlers_test.go` +- `apps/backend/internal/orchestrator/session_launch_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. It defines the recovery semantics consumed by Task 02. + +## Inputs + +- Repair spec sections `Expected behavior`, `Persistence and security + constraints`, and the first two regression scenarios +- Plan section `Preserve resume identity on startup failure` +- Existing `handleRecoverableFailure`, `clearResumeToken`, and + `RecoverSession` patterns + +## Output contract + +Report the failing-test evidence, changed recovery branches, retained explicit +fresh-start behavior, exact test result, files changed, blockers, and risk +notes. Mark this task `done` and update its plan checkbox in the same +conversation. diff --git a/docs/plans/agent-resume-runtime-recovery/task-02-resume-state-before-credentials.md b/docs/plans/agent-resume-runtime-recovery/task-02-resume-state-before-credentials.md new file mode 100644 index 0000000000..75284db5c6 --- /dev/null +++ b/docs/plans/agent-resume-runtime-recovery/task-02-resume-state-before-credentials.md @@ -0,0 +1,62 @@ +--- +id: "02-resume-state-before-credentials" +title: "Transition resume state before credential issuance" +status: completed +wave: 2 +depends_on: + - "01-preserve-resume-token" +plan: "plan.md" +spec: "../../specs/agent-resume-runtime-recovery/spec.md" +--- + +# Task 02: Transition resume state before credential issuance + +## Acceptance + +- A failed or cancelled session is guardedly persisted as `STARTING` before + `LaunchAgent` can request a GitHub credential lease. +- Terminal stale-execution cleanup and concurrent-resume protection retain + their existing behavior after the earlier state mutation. +- Launch failure rolls back a still-`STARTING` session without overwriting a + concurrent terminal transition. + +## Verification + +- `cd apps/backend && go test ./internal/orchestrator/executor -run 'TestResumeSession'` +- `cd apps/backend && go test ./internal/orchestrator -run 'TestResumeTaskSession'` +- `cd apps/backend && go test ./internal/backendapp -run 'TestGitHubBrokerScopeAuthorizer'` + +The repository-observing launch regression must first fail because it sees the +terminal state at the credential boundary, then pass after the transition is +moved. + +## Files likely touched + +- `apps/backend/internal/orchestrator/executor/executor_resume.go` +- `apps/backend/internal/orchestrator/executor/executor_resume_test.go` +- `apps/backend/internal/orchestrator/task_operations_resume_test.go` + +## Dependencies + +Task 01. + +## Parallelism + +Sequential. It shares the session-recovery state contract established by Task +01 and must land after it. + +## Inputs + +- Repair spec `Persistence and security constraints` +- Plan section `Persist STARTING before credential issuance` +- Existing `persistResumeState`, `updateSessionStarting`, + `validateAndLockResume`, and terminal stale-cleanup tests +- Existing credential-broker terminal rejection in + `apps/backend/internal/backendapp/services.go` + +## Output contract + +Report the red/green test evidence, exact state transition and rollback +ordering, concurrency behavior, exact test results, files changed, blockers, +and risk notes. Mark this task `done` and update its plan checkbox in the same +conversation. diff --git a/docs/plans/agent-resume-runtime-recovery/task-03-managed-npm-cache-repair.md b/docs/plans/agent-resume-runtime-recovery/task-03-managed-npm-cache-repair.md new file mode 100644 index 0000000000..003e956568 --- /dev/null +++ b/docs/plans/agent-resume-runtime-recovery/task-03-managed-npm-cache-repair.md @@ -0,0 +1,63 @@ +--- +id: "03-managed-npm-cache-repair" +title: "Repair corrupt managed npm execution caches" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/agent-resume-runtime-recovery/spec.md" +--- + +# Task 03: Repair corrupt managed npm execution caches + +## Acceptance + +- An explicit managed-runtime update that fails once invalidates only the + selected built-in package's deterministic `_npx` execution directory and + retries exactly once. +- Cache-root and target validation prevent caller-controlled or broad + deletion; no global npm cache clean is executed. +- A repaired update is successful only after the existing ACP refresh succeeds, + while repair or retry failure is terminal and visible. + +## Verification + +- `cd apps/backend && go test ./internal/agent/agents -run 'TestManagedNPMRuntime'` +- `cd apps/backend && go test ./internal/agent/settings/controller -run 'TestAgentUpdate'` + +The update-job regression must first fail because no repair/retry call occurs, +then pass with the exact call order `update -> repair -> update -> refresh`. + +## Files likely touched + +- `apps/backend/internal/agent/agents/managed_npm_runtime.go` +- `apps/backend/internal/agent/agents/managed_npm_runtime_test.go` +- `apps/backend/internal/agent/settings/controller/agent_update.go` +- `apps/backend/internal/agent/settings/controller/agent_update_job.go` +- `apps/backend/internal/agent/settings/controller/agent_update_test.go` +- `docs/public/agents-and-profiles.md` + +## Dependencies + +None. + +## Parallelism + +`parallel-safe` relative to Tasks 01 and 02: it touches disjoint agent settings +and runtime files, with no shared schema, migration, generated contract, +lockfile, or package configuration. + +## Inputs + +- Repair spec final two regression scenarios and cache safety constraints +- `docs/specs/agents/runtime-updates.md` failure mode and corrupt-cache scenario +- Plan section `Repair one managed npm execution cache` +- Existing direct-argv `RuntimeUpdater`, job output ring, and update retry + patterns + +## Output contract + +Report the red/green test evidence, npm key derivation, deletion guards, retry +ordering, public-doc change, exact test results, files changed, blockers, and +risk notes. Mark this task `done` and update its plan checkbox in the same +conversation. diff --git a/docs/public/agents-and-profiles.md b/docs/public/agents-and-profiles.md index 0590ff27f1..0e0d78cbd8 100644 --- a/docs/public/agents-and-profiles.md +++ b/docs/public/agents-and-profiles.md @@ -52,6 +52,9 @@ package updates but the follow-up probe requires authentication, the dialog reports the update plus the refresh error; authenticate the host agent and refresh again. Update job history is short-lived and does not survive a backend restart, and npm cache contents are not a durable Kandev installation record. +When the first managed package update fails, Kandev removes only that +package's extracted npm execution tree and retries the same update once; a +repair or second failure is reported as failed and does not retry indefinitely. ### Add a custom terminal agent diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index a733e77865..83c84a69da 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -151,6 +151,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | Spec | Status | |---|---| +| [agent-resume-runtime-recovery](agent-resume-runtime-recovery/spec.md) | shipped | | [auth](auth/spec.md) | building | | [create-local-repository](create-local-repository/spec.md) | shipped | | [workflow-cycle-guardrails](workflow-cycle-guardrails/spec.md) | building | diff --git a/docs/specs/agent-resume-runtime-recovery/spec.md b/docs/specs/agent-resume-runtime-recovery/spec.md new file mode 100644 index 0000000000..47feff6d9d --- /dev/null +++ b/docs/specs/agent-resume-runtime-recovery/spec.md @@ -0,0 +1,91 @@ +--- +status: shipped +created: 2026-07-27 +owner: Kandev +--- + +# Agent Resume and Runtime Recovery + +## Broken behavior + +An agent process or ACP handshake can fail before Kandev has attempted to load +the stored provider-native session. Kandev currently treats every such failure +as proof that the saved session is unusable and clears the operational resume +token. A later retry therefore starts a new provider-native conversation even +though the original session remains valid. + +A failed or cancelled GitHub-backed session also requests a new scoped +credential lease while its persisted state is still terminal. The credential +broker correctly rejects terminal sessions, so the user-visible Resume action +cannot reach the agent launch. + +Managed npm runtimes can retain a truncated or otherwise corrupt extracted +`_npx` execution tree even when npm's content-addressable package cache is +healthy. Re-running the current update command can reuse that tree and fail +without repairing it. + +## Expected behavior + +- A process startup, ACP initialize, or transport failure does not discard the + stored resume token. Resume retries the same provider-native session. +- The stored token is cleared only when the user explicitly chooses + **Start fresh**, or is replaced after the agent successfully creates a new + provider-native session. +- An authorized resume moves the task session to `STARTING` under the existing + per-session resume lock before launching the agent. This makes the session + eligible for a scoped GitHub credential lease without weakening the + credential broker's terminal-session rejection. +- If launch fails after that early transition, Kandev restores the prior + recoverable session state unless another terminal transition won the race. +- The explicit managed-runtime update path may invalidate only the + deterministic `_npx` execution directory for the selected built-in package + after an initial update failure, then retry once and run the normal ACP + capability probe. + +## Persistence and security constraints + +- `task_sessions.metadata.acp.session_id` and + `executors_running.resume_token` continue to identify the provider-native + conversation until explicit fresh start or successful replacement. +- A failed pre-session launch must not blank either persisted identity. +- Resume state changes use the existing guarded session transition and + publication path; direct unguarded state writes are not introduced. +- The GitHub credential broker continues to reject `COMPLETED`, `FAILED`, and + `CANCELLED` sessions. Recovery obtains a lease only after the authorized + resume has persisted `STARTING`. +- Runtime cache repair accepts only package names from built-in agent metadata, + resolves npm's cache root through direct argv, and removes only + `/_npx/`. It never accepts a caller-provided path or runs + a global cache clean. + +## Regression scenarios + +- **GIVEN** a valid OpenCode resume token, **WHEN** the OpenCode child exits + before answering ACP `initialize`, **THEN** Kandev shows the normal recovery + action and retains the same token for the next Resume attempt. +- **GIVEN** a resume attempt whose ACP transport disconnects before + `session/load` completes, **WHEN** the attempt fails, **THEN** Kandev retains + the token so a later healthy process can retry it. +- **GIVEN** a failed GitHub-backed session, **WHEN** the user selects Resume, + **THEN** the session is persisted as `STARTING` before the credential lease + is requested and the launch can proceed. +- **GIVEN** that launch fails after the early `STARTING` transition, **WHEN** + recovery handling completes, **THEN** the session is recoverable and no + stale `STARTING` state remains. +- **GIVEN** an extracted managed npm runtime is corrupt, **WHEN** the first + explicit update attempt fails, **THEN** only that package's deterministic + execution directory is invalidated, the update runs once more, and success + is reported only after ACP initialization succeeds. +- **GIVEN** the targeted cache repair or retry also fails, **WHEN** the update + job becomes terminal, **THEN** it reports the bounded failure and performs no + additional retry or broad cache deletion. + +## Out of scope + +- Silently falling back to a fresh provider-native conversation after Resume. +- Reconstructing or rewriting provider-owned conversation history. +- Automatically mutating npm caches on every normal agent launch. +- Global npm cache cleanup, exact runtime-version pins, rollback, or selection + of historical runtime versions. +- Relaxing credential-broker authorization for terminal sessions. +- New recovery buttons or other frontend behavior. diff --git a/docs/specs/agents/runtime-updates.md b/docs/specs/agents/runtime-updates.md index 941c043c1a..5c70669e8e 100644 --- a/docs/specs/agents/runtime-updates.md +++ b/docs/specs/agents/runtime-updates.md @@ -157,10 +157,13 @@ Jobs are terminal after `succeeded` or `failed`. Retrying creates a new job. - If npm registry metadata cannot be resolved, the job fails before changing the runtime and retains the prior capability data. +- If the first package update command fails, Kandev may remove only the + deterministic npm execution-cache directory for that built-in package and + retry the update once. It never runs a global npm cache clean. If the repair + or retry fails or times out, the job fails and retains the prior capability + data. - If preview version resolution fails, the dialog shows the error and keeps approval disabled. No update job or package command starts. -- If the package update command fails or times out, the job fails and retains - the prior capability data. - If the package update succeeds but the capability probe fails because authentication is required or another recoverable probe error occurs, the job reports package-update success plus `refresh_error`; the previous model @@ -215,6 +218,10 @@ Jobs are terminal after `succeeded` or `failed`. Retrying creates a new job. approval because the registry changed, **WHEN** the update job resolves its target again, **THEN** the dialog shows a retryable update failure and retains the previous models. +- **GIVEN** a managed package's extracted `_npx` execution tree is corrupt, + **WHEN** the first update preparation fails, **THEN** Kandev invalidates only + that built-in package's deterministic execution-cache directory, retries + once, and probes the rebuilt runtime before reporting success. - **GIVEN** the package update succeeds but the fresh probe requires authentication, **WHEN** the job finishes, **THEN** the dialog reports the new package version and refresh error, the previous models remain available, and @@ -236,6 +243,8 @@ Jobs are terminal after `succeeded` or `failed`. Retrying creates a new job. ## Out of scope - Scheduled or automatic package updates. +- Automatically deleting or rebuilding npm execution caches during every + ordinary agent launch. - Exact package-version pins, version allowlists, rollback, or user-selected historical versions. - Updating configured remote executors or every running container from the