diff --git a/Makefile b/Makefile index 969fe96356..198c1564f0 100644 --- a/Makefile +++ b/Makefile @@ -214,7 +214,7 @@ start: @$(MAKE) -s build-backend-quiet $(call success,Build complete) $(call phase,Starting Server) - @$(BACKEND_DIR)/bin/kandev start $(if $(filter 1 true yes,$(VERBOSE)),--verbose,) $(if $(filter 1 true yes,$(DEBUG)),--debug,) + @exec $(BACKEND_DIR)/bin/kandev start $(if $(filter 1 true yes,$(VERBOSE)),--verbose,) $(if $(filter 1 true yes,$(DEBUG)),--debug,) .PHONY: start-verbose start-verbose: diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 928e552fbd..b8579aa4bc 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -148,6 +148,11 @@ apps/backend/ - Exposes workspace operations (shell, git, files) - Supports multiple concurrent instances on different ports +Standalone agentctl is launched in its own process group so terminal Ctrl+C is +handled by the backend lifecycle manager first. Do not make standalone agentctl +share the backend's foreground process group; that bypasses supervised shutdown +and can leak ACP subprocesses. + **Executor Types** (database model): - `local_pc` - Standalone process on host - `local_docker` - Docker container on host diff --git a/apps/backend/cmd/winjob/main_windows.go b/apps/backend/cmd/winjob/main_windows.go index 47a1a4ce27..8389f4cb8f 100644 --- a/apps/backend/cmd/winjob/main_windows.go +++ b/apps/backend/cmd/winjob/main_windows.go @@ -21,9 +21,8 @@ import ( "os" "os/exec" "os/signal" - "unsafe" - "golang.org/x/sys/windows" + "github.com/kandev/kandev/internal/agentctl/server/winproc" ) func main() { @@ -47,33 +46,27 @@ func main() { } }() - job, err := createKillOnCloseJob() - if err != nil { - die("create job: %v", err) - } - // Don't `defer CloseHandle(job)` — we want the OS to close it as part of - // process teardown so KILL_ON_JOB_CLOSE fires correctly on any exit path - // including os.Exit and panics. Manual close on success is also fine - // because the child has already exited by then. - cmd := exec.Command(os.Args[1], os.Args[2:]...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Env = os.Environ() if err := cmd.Start(); err != nil { - _ = windows.CloseHandle(job) die("start child: %v", err) } - if err := assignProcessToJob(job, cmd.Process.Pid); err != nil { + job, err := winproc.InstallKillOnCloseJobForCommand(cmd) + if err != nil { _ = cmd.Process.Kill() - _ = windows.CloseHandle(job) die("assign job: %v", err) } + // Don't `defer job.Close()` — we want the OS to close it as part of + // process teardown so KILL_ON_JOB_CLOSE fires correctly on any exit path + // including os.Exit and panics. Manual close on success is also fine + // because the child has already exited by then. waitErr := cmd.Wait() - _ = windows.CloseHandle(job) + _ = job.Close() if waitErr != nil { if exitErr, ok := waitErr.(*exec.ExitError); ok { os.Exit(exitErr.ExitCode()) @@ -82,44 +75,6 @@ func main() { } } -func createKillOnCloseJob() (windows.Handle, error) { - job, err := windows.CreateJobObject(nil, nil) - if err != nil { - return 0, fmt.Errorf("CreateJobObject: %w", err) - } - info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ - BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ - LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, - }, - } - if _, err := windows.SetInformationJobObject( - job, - windows.JobObjectExtendedLimitInformation, - uintptr(unsafe.Pointer(&info)), - uint32(unsafe.Sizeof(info)), - ); err != nil { - _ = windows.CloseHandle(job) - return 0, fmt.Errorf("SetInformationJobObject: %w", err) - } - return job, nil -} - -func assignProcessToJob(job windows.Handle, pid int) error { - procHandle, err := windows.OpenProcess( - windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, - false, - uint32(pid), - ) - if err != nil { - return fmt.Errorf("OpenProcess(%d): %w", pid, err) - } - defer windows.CloseHandle(procHandle) - if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { - return fmt.Errorf("AssignProcessToJobObject: %w", err) - } - return nil -} - func die(format string, args ...any) { fmt.Fprintf(os.Stderr, "winjob: "+format+"\n", args...) os.Exit(1) diff --git a/apps/backend/internal/agent/agents/agent.go b/apps/backend/internal/agent/agents/agent.go index 5accd673e7..a07f31b9be 100644 --- a/apps/backend/internal/agent/agents/agent.go +++ b/apps/backend/internal/agent/agents/agent.go @@ -208,11 +208,11 @@ type RuntimeConfig struct { // agents whose ACP adapter does not wire session/new mcpServers through to // the underlying CLI. ProjectMCPStrategy mcpconfig.PassthroughMCPStrategy - // RequiresProcessKill is true for agents whose subprocess does not exit - // when stdin is closed (e.g. OpenCode's ACP runtime, which keeps its HTTP - // server and MCP child tree alive). When true, the agentctl process - // manager kills the whole process group on shutdown so MCP children - // don't leak. + // RequiresProcessKill is true for agents whose subprocess should skip the + // graceful stdin-close wait because it is known not to exit on EOF (e.g. + // OpenCode's ACP runtime, which keeps its HTTP server and MCP child tree + // alive). Agentctl still reaps the process group for every agent after + // graceful shutdown or timeout; this flag only makes that reap immediate. RequiresProcessKill bool } diff --git a/apps/backend/internal/agent/agents/opencode_acp.go b/apps/backend/internal/agent/agents/opencode_acp.go index 07c3d16908..71b1762550 100644 --- a/apps/backend/internal/agent/agents/opencode_acp.go +++ b/apps/backend/internal/agent/agents/opencode_acp.go @@ -98,8 +98,8 @@ func (a *OpenCodeACP) Runtime() *RuntimeConfig { ProjectSkillDir: ".agents/skills", UserSkillDir: ".config/opencode/skills", // opencode acp runs its HTTP server + MCP child tree alongside the - // ACP stdin/stdout. Closing stdin doesn't terminate the process — - // we have to kill its process group to reap the MCP children. + // ACP stdin/stdout. Closing stdin doesn't terminate the process, so + // skip the graceful wait and reap its process group immediately. // See GH issue #1247. RequiresProcessKill: true, SessionConfig: SessionConfig{ diff --git a/apps/backend/internal/agent/agents/opencode_acp_test.go b/apps/backend/internal/agent/agents/opencode_acp_test.go index 395cea0f72..20174510f7 100644 --- a/apps/backend/internal/agent/agents/opencode_acp_test.go +++ b/apps/backend/internal/agent/agents/opencode_acp_test.go @@ -8,9 +8,9 @@ import ( // TestOpenCodeACPRuntime_RequiresProcessKill is the regression test for GH // issue #1247: opencode acp keeps its HTTP server + MCP child tree alive // when stdin closes, so its RuntimeConfig must signal that the process -// group has to be killed on shutdown. Without this flag the ACP adapter -// returns RequiresProcessKill=false and the process manager only closes -// stdin — opencode never exits and MCP children leak. +// group should be reaped immediately. Without this flag the ACP adapter +// returns RequiresProcessKill=false and the process manager waits for the +// graceful EOF path before it falls back to process-group cleanup. func TestOpenCodeACPRuntime_RequiresProcessKill(t *testing.T) { rt := NewOpenCodeACP().Runtime() if rt == nil { @@ -23,9 +23,8 @@ func TestOpenCodeACPRuntime_RequiresProcessKill(t *testing.T) { // TestACPAgents_DefaultProcessKill confirms the rest of the ACP agents // stick with the default (false). They communicate over plain stdin/stdout -// and exit on EOF, so the process manager's graceful-stdin-close path is -// the correct shutdown signal — flipping this true would force-kill agents -// that are perfectly capable of cleanup. +// and should get a short graceful EOF path before the process manager reaps +// any remaining process-group descendants. func TestACPAgents_DefaultProcessKill(t *testing.T) { cases := []struct { name string diff --git a/apps/backend/internal/agent/hostutility/manager.go b/apps/backend/internal/agent/hostutility/manager.go index 4559009e57..19ffcb5c85 100644 --- a/apps/backend/internal/agent/hostutility/manager.go +++ b/apps/backend/internal/agent/hostutility/manager.go @@ -48,6 +48,8 @@ type Manager struct { mu sync.RWMutex instances map[string]*instance // keyed by agent type createGroup singleflight.Group + startCancel context.CancelFunc + stopped bool } // instance is a single warm agentctl instance bound to an agent type. @@ -86,13 +88,39 @@ func (m *Manager) SetAuthToken(token string) { // initial probe against each in parallel. Individual agent failures are // captured in the cache but do not abort the other agents. func (m *Manager) Start(ctx context.Context) error { + startCtx, cancel := context.WithCancel(ctx) + m.mu.Lock() + if m.stopped { + m.mu.Unlock() + cancel() + return nil + } + m.startCancel = cancel + m.mu.Unlock() + defer func() { + m.mu.Lock() + m.startCancel = nil + m.mu.Unlock() + cancel() + }() + // Create a process-scoped parent dir so concurrent kandev processes do not // share state, and so Stop only removes dirs owned by this process. parent, err := os.MkdirTemp("", fmt.Sprintf("kandev-host-utility-%d-*", os.Getpid())) if err != nil { return fmt.Errorf("create host utility tmp dir: %w", err) } + m.mu.Lock() + if m.stopped { + m.mu.Unlock() + if err := os.RemoveAll(parent); err != nil { + m.log.Warn("failed to remove unused host utility parent tmp dir", + zap.String("path", parent), zap.Error(err)) + } + return nil + } m.parentTmpDir = parent + m.mu.Unlock() m.log.Info("host utility parent tmp dir created", zap.String("path", parent)) targets := m.eligibleAgents() @@ -101,7 +129,7 @@ func (m *Manager) Start(ctx context.Context) error { return nil } - g, gctx := errgroup.WithContext(ctx) + g, gctx := errgroup.WithContext(startCtx) for _, ag := range targets { ag := ag g.Go(func() error { @@ -118,14 +146,47 @@ func (m *Manager) Start(ctx context.Context) error { // are untouched. func (m *Manager) Stop(ctx context.Context) { m.mu.Lock() + m.stopped = true + cancel := m.startCancel + m.startCancel = nil instances := make([]*instance, 0, len(m.instances)) for _, inst := range m.instances { instances = append(instances, inst) } m.instances = make(map[string]*instance) + parentTmpDir := m.parentTmpDir + m.parentTmpDir = "" m.mu.Unlock() + if cancel != nil { + cancel() + } + for _, inst := range instances { + deleteCtx, cancel := hostUtilityDeleteContext(ctx) + m.deleteInstance(deleteCtx, inst) + cancel() + } + + if parentTmpDir != "" { + if err := os.RemoveAll(parentTmpDir); err != nil { + m.log.Warn("failed to remove host utility parent tmp dir", + zap.String("path", parentTmpDir), zap.Error(err)) + } + } +} + +const hostUtilityDeleteTimeout = 2 * time.Second + +func hostUtilityDeleteContext(parent context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(parent), hostUtilityDeleteTimeout) +} + +func (m *Manager) deleteInstance(ctx context.Context, inst *instance) { + if inst == nil { + return + } + if m.controlClient != nil { if err := m.controlClient.DeleteInstance(ctx, inst.instanceID); err != nil { m.log.Warn("failed to delete host utility instance", zap.String("agent_type", inst.agentType), @@ -133,13 +194,14 @@ func (m *Manager) Stop(ctx context.Context) { zap.Error(err)) } } - - if m.parentTmpDir != "" { - if err := os.RemoveAll(m.parentTmpDir); err != nil { - m.log.Warn("failed to remove host utility parent tmp dir", - zap.String("path", m.parentTmpDir), zap.Error(err)) - } - m.parentTmpDir = "" + if inst.workDir == "" { + return + } + if err := os.RemoveAll(inst.workDir); err != nil { + m.log.Warn("failed to remove host utility work dir", + zap.String("agent_type", inst.agentType), + zap.String("path", inst.workDir), + zap.Error(err)) } } @@ -217,7 +279,21 @@ func (m *Manager) bootstrapAgent(ctx context.Context, ia agents.InferenceAgent) return } + if ctx.Err() != nil { + deleteCtx, cancel := hostUtilityDeleteContext(ctx) + m.deleteInstance(deleteCtx, inst) + cancel() + return + } + m.mu.Lock() + if m.stopped { + m.mu.Unlock() + deleteCtx, cancel := hostUtilityDeleteContext(ctx) + m.deleteInstance(deleteCtx, inst) + cancel() + return + } m.instances[agentType] = inst m.mu.Unlock() @@ -242,7 +318,17 @@ func (m *Manager) createInstance(ctx context.Context, agentType string) (*instan if !safeAgentTypeName.MatchString(agentType) { return nil, fmt.Errorf("invalid agent type %q: must match %s", agentType, safeAgentTypeName.String()) } - workDir := filepath.Join(m.parentTmpDir, agentType) + m.mu.RLock() + parentTmpDir := m.parentTmpDir + stopped := m.stopped + m.mu.RUnlock() + if stopped { + return nil, errManagerStopped + } + if parentTmpDir == "" { + return nil, errors.New("host utility manager not started") + } + workDir := filepath.Join(parentTmpDir, agentType) if err := os.MkdirAll(workDir, 0o755); err != nil { return nil, fmt.Errorf("mkdir %s: %w", workDir, err) } @@ -266,7 +352,14 @@ func (m *Manager) createInstance(ctx context.Context, agentType string) (*instan healthCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() if err := waitForClientHealthy(healthCtx, client); err != nil { - _ = m.controlClient.DeleteInstance(context.Background(), resp.ID) + deleteCtx, deleteCancel := hostUtilityDeleteContext(ctx) + m.deleteInstance(deleteCtx, &instance{ + agentType: agentType, + instanceID: resp.ID, + workDir: workDir, + client: client, + }) + deleteCancel() return nil, fmt.Errorf("instance %s not healthy: %w", resp.ID, err) } @@ -303,6 +396,7 @@ func waitForClientHealthy(ctx context.Context, c *agentctlclient.Client) error { // installed on the host. The Refresh path uses it to classify Status // accurately instead of lumping every error into StatusFailed. var errAgentNotInstalled = errors.New("agent not installed") +var errManagerStopped = errors.New("host utility manager stopped") // getInstance returns the warm instance for the agent type, lazily recreating // it if missing (e.g. after a previous failure or crash). @@ -320,13 +414,18 @@ func (m *Manager) getInstance(ctx context.Context, agentType string) (*instance, } m.mu.RLock() + if m.stopped { + m.mu.RUnlock() + return nil, nil, errManagerStopped + } inst := m.instances[agentType] + parentTmpDir := m.parentTmpDir m.mu.RUnlock() if inst != nil { return inst, ia, nil } - if m.parentTmpDir == "" { + if parentTmpDir == "" { return nil, nil, errors.New("host utility manager not started") } @@ -351,6 +450,13 @@ func (m *Manager) getInstance(ctx context.Context, agentType string) (*instance, return nil, cerr } m.mu.Lock() + if m.stopped { + m.mu.Unlock() + deleteCtx, cancel := hostUtilityDeleteContext(ctx) + m.deleteInstance(deleteCtx, created) + cancel() + return nil, errManagerStopped + } m.instances[agentType] = created m.mu.Unlock() return created, nil diff --git a/apps/backend/internal/agent/hostutility/manager_test.go b/apps/backend/internal/agent/hostutility/manager_test.go new file mode 100644 index 0000000000..206ecd6cf3 --- /dev/null +++ b/apps/backend/internal/agent/hostutility/manager_test.go @@ -0,0 +1,144 @@ +package hostutility + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/agent/agents" + "github.com/kandev/kandev/internal/agent/registry" + "github.com/kandev/kandev/internal/agent/usage" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/pkg/agent" + "github.com/stretchr/testify/require" +) + +func TestStopCancelsInFlightBootstrap(t *testing.T) { + log := newTestLogger(t) + reg := registry.NewRegistry(log) + + started := make(chan struct{}) + canceled := make(chan struct{}) + require.NoError(t, reg.Register(&blockingInferenceAgent{ + started: started, + canceled: canceled, + })) + + mgr := NewManager(reg, "127.0.0.1", 1, nil, log) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { + done <- mgr.Start(ctx) + }() + + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("host utility bootstrap did not start installation check") + } + + mgr.Stop(context.Background()) + + select { + case <-canceled: + case <-time.After(500 * time.Millisecond): + t.Fatal("host utility Stop did not cancel in-flight bootstrap") + } + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("host utility Start did not return after Stop") + } +} + +func TestDeleteInstanceRemovesWorkDirWithoutControlClient(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(registry.NewRegistry(log), "127.0.0.1", 1, nil, log) + workDir := filepath.Join(t.TempDir(), "codex-acp") + require.NoError(t, os.MkdirAll(workDir, 0o755)) + + mgr.deleteInstance(context.Background(), &instance{ + agentType: "codex-acp", + workDir: workDir, + }) + + _, err := os.Stat(workDir) + require.ErrorIs(t, err, os.ErrNotExist) +} + +func TestHostUtilityDeleteContextIgnoresParentCancellation(t *testing.T) { + parent, cancelParent := context.WithCancel(context.Background()) + cancelParent() + + ctx, cancel := hostUtilityDeleteContext(parent) + defer cancel() + + select { + case <-ctx.Done(): + t.Fatal("delete context should not inherit parent cancellation") + default: + } +} + +func newTestLogger(t *testing.T) *logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{ + Level: "error", + Format: "json", + }) + if err != nil { + t.Fatalf("failed to create logger: %v", err) + } + return log +} + +type blockingInferenceAgent struct { + once sync.Once + started chan struct{} + canceled chan struct{} +} + +func (a *blockingInferenceAgent) ID() string { return "blocking-acp" } +func (a *blockingInferenceAgent) Name() string { return "Blocking ACP" } +func (a *blockingInferenceAgent) DisplayName() string { return "Blocking ACP" } +func (a *blockingInferenceAgent) Description() string { return "blocking test agent" } +func (a *blockingInferenceAgent) Enabled() bool { return true } +func (a *blockingInferenceAgent) DisplayOrder() int { return 1 } +func (a *blockingInferenceAgent) Logo(agents.LogoVariant) []byte { + return nil +} +func (a *blockingInferenceAgent) IsInstalled(ctx context.Context) (*agents.DiscoveryResult, error) { + a.once.Do(func() { + close(a.started) + }) + <-ctx.Done() + close(a.canceled) + return nil, ctx.Err() +} +func (a *blockingInferenceAgent) BuildCommand(agents.CommandOptions) agents.Command { + return agents.NewCommand("blocking-acp") +} +func (a *blockingInferenceAgent) PermissionSettings() map[string]agents.PermissionSetting { + return nil +} +func (a *blockingInferenceAgent) Runtime() *agents.RuntimeConfig { + return &agents.RuntimeConfig{Protocol: agent.ProtocolACP} +} +func (a *blockingInferenceAgent) BillingType() usage.BillingType { + return usage.BillingTypeSubscription +} +func (a *blockingInferenceAgent) RemoteAuth() *agents.RemoteAuth { return nil } +func (a *blockingInferenceAgent) InstallScript() string { return "" } +func (a *blockingInferenceAgent) InferenceConfig() *agents.InferenceConfig { + return &agents.InferenceConfig{ + Supported: true, + Command: agents.NewCommand("blocking-acp"), + } +} diff --git a/apps/backend/internal/agent/hostutility/testmain_test.go b/apps/backend/internal/agent/hostutility/testmain_test.go new file mode 100644 index 0000000000..39125a865e --- /dev/null +++ b/apps/backend/internal/agent/hostutility/testmain_test.go @@ -0,0 +1,11 @@ +package hostutility + +import ( + "testing" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} diff --git a/apps/backend/internal/agent/runtime/agentctl/control.go b/apps/backend/internal/agent/runtime/agentctl/control.go index 0c02f4e8f0..255784420d 100644 --- a/apps/backend/internal/agent/runtime/agentctl/control.go +++ b/apps/backend/internal/agent/runtime/agentctl/control.go @@ -55,10 +55,10 @@ type CreateInstanceRequest struct { AssumeMcpSse bool `json:"assume_mcp_sse,omitempty"` // Assume agent supports SSE MCP servers AssumeMcpHttp bool `json:"assume_mcp_http,omitempty"` // Assume agent supports HTTP MCP servers McpMode string `json:"mcp_mode,omitempty"` // MCP tool mode: "task" (default), "config", or "office" - // RequiresProcessKill forces agentctl to kill the agent's process group - // (not just close stdin) on shutdown. Required for agents whose runtime - // keeps child processes (e.g. MCP servers) alive when stdin closes — - // notably opencode acp. + // RequiresProcessKill tells agentctl to skip the graceful stdin-close wait + // and reap the agent process group immediately. Required for agents whose + // runtime keeps child processes (e.g. MCP servers) alive when stdin closes + // — notably opencode acp. RequiresProcessKill bool `json:"requires_process_kill,omitempty"` // BaseBranches maps RepositoryName → base branch ref for the task's diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/launcher.go b/apps/backend/internal/agent/runtime/agentctl/launcher/launcher.go index d01a53d45d..0262627de1 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/launcher.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/launcher.go @@ -184,9 +184,7 @@ func (l *Launcher) Start(ctx context.Context) error { l.mu.Unlock() if err := l.waitForHealthy(ctx); err != nil { - if killErr := l.cmd.Process.Kill(); killErr != nil { - l.logger.Warn("failed to kill agentctl process after failed health check", zap.Error(killErr)) - } + l.forceKill(l.cmd.Process.Pid) l.closeParentPipe() l.releaseChildLifecycle() return fmt.Errorf("agentctl failed to become healthy: %w", err) @@ -266,9 +264,7 @@ func (l *Launcher) performHandshake(ctx context.Context, nonce string) (string, ctl := agentctlclient.NewControlClient(l.host, l.port, l.logger) token, err := ctl.Handshake(ctx, nonce) if err != nil { - if killErr := l.cmd.Process.Kill(); killErr != nil { - l.logger.Warn("failed to kill agentctl after handshake failure", zap.Error(killErr)) - } + l.forceKill(l.cmd.Process.Pid) l.closeParentPipe() l.releaseChildLifecycle() return "", fmt.Errorf("agentctl handshake failed: %w", err) @@ -299,6 +295,7 @@ func (l *Launcher) Stop(ctx context.Context) error { l.mu.Unlock() l.logger.Info("stopping agentctl subprocess", zap.Int("pid", pid)) + l.logger.Debug("agentctl subprocess stop requested", zap.Int("pid", pid)) // Close the liveness pipe first — this signals agentctl that the parent // is shutting down, complementing the SIGTERM that follows. diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows.go b/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows.go index fea44dc5b8..3a0c525317 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows.go @@ -6,8 +6,8 @@ import ( "fmt" "os/exec" "sync/atomic" - "unsafe" + "github.com/kandev/kandev/internal/agentctl/server/winproc" "go.uber.org/zap" "golang.org/x/sys/windows" ) @@ -29,34 +29,18 @@ func (l *Launcher) installChildLifecycle(cmd *exec.Cmd) error { return fmt.Errorf("installChildLifecycle: process not started") } - job, err := createKillOnCloseJob() + job, err := winproc.InstallKillOnCloseJobForCommand(cmd) if err != nil { return err } - procHandle, err := windows.OpenProcess( - windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, - false, - uint32(cmd.Process.Pid), - ) - if err != nil { - _ = windows.CloseHandle(job) - return fmt.Errorf("OpenProcess(pid=%d): %w", cmd.Process.Pid, err) - } - defer windows.CloseHandle(procHandle) - - if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { - _ = windows.CloseHandle(job) - return fmt.Errorf("AssignProcessToJobObject: %w", err) - } - // Defensive: if a previous lifecycle was somehow installed without a // matching release (unexpected restart, future code change), close the // stale handle instead of silently leaking it. Doing the swap *after* the // new job is assigned avoids killing the new agentctl before we've stored // its handle — the new job has KILL_ON_JOB_CLOSE so we must hold a // reference at all times. - previous := atomic.SwapUintptr(&l.jobHandle, uintptr(job)) + previous := atomic.SwapUintptr(&l.jobHandle, job.RawHandle()) if previous != 0 { if err := windows.CloseHandle(windows.Handle(previous)); err != nil { l.logger.Warn("failed to close stale job object handle", zap.Error(err)) @@ -85,28 +69,3 @@ func (l *Launcher) releaseChildLifecycle() { l.logger.Warn("failed to close job object handle", zap.Error(err)) } } - -// createKillOnCloseJob creates an unnamed Job Object whose handle, when the -// last reference is closed by the OS, terminates every process assigned to it. -func createKillOnCloseJob() (windows.Handle, error) { - job, err := windows.CreateJobObject(nil, nil) - if err != nil { - return 0, fmt.Errorf("CreateJobObject: %w", err) - } - - info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ - BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ - LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, - }, - } - if _, err := windows.SetInformationJobObject( - job, - windows.JobObjectExtendedLimitInformation, - uintptr(unsafe.Pointer(&info)), - uint32(unsafe.Sizeof(info)), - ); err != nil { - _ = windows.CloseHandle(job) - return 0, fmt.Errorf("SetInformationJobObject: %w", err) - } - return job, nil -} diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows_test.go b/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows_test.go index eed36d74f0..8d1603c04e 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows_test.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/lifecycle_windows_test.go @@ -8,8 +8,8 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/agentctl/server/winproc" "github.com/kandev/kandev/internal/common/logger" - "golang.org/x/sys/windows" ) func newTestLauncher(t *testing.T) *Launcher { @@ -21,19 +21,6 @@ func newTestLauncher(t *testing.T) *Launcher { return &Launcher{logger: log} } -func TestCreateKillOnCloseJob(t *testing.T) { - job, err := createKillOnCloseJob() - if err != nil { - t.Fatalf("createKillOnCloseJob: %v", err) - } - if job == 0 { - t.Fatal("got null job handle") - } - if err := windows.CloseHandle(job); err != nil { - t.Errorf("CloseHandle: %v", err) - } -} - func TestReleaseChildLifecycle_NoHandleIsNoop(t *testing.T) { l := newTestLauncher(t) // Must not panic when no handle has been installed. @@ -44,13 +31,20 @@ func TestReleaseChildLifecycle_NoHandleIsNoop(t *testing.T) { } func TestReleaseChildLifecycle_Idempotent(t *testing.T) { - job, err := createKillOnCloseJob() + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + job, err := winproc.InstallKillOnCloseJobForCommand(cmd) if err != nil { - t.Fatalf("createKillOnCloseJob: %v", err) + t.Fatalf("InstallKillOnCloseJobForCommand: %v", err) + } + if err := cmd.Wait(); err != nil { + t.Fatalf("wait child: %v", err) } l := newTestLauncher(t) - atomic.StoreUintptr(&l.jobHandle, uintptr(job)) + atomic.StoreUintptr(&l.jobHandle, job.RawHandle()) l.releaseChildLifecycle() // Second call must not double-close the handle. diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix.go b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix.go index 26e6d7f4e5..06aca093a1 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix.go @@ -8,18 +8,25 @@ import ( "go.uber.org/zap" ) -// gracefulStop sends SIGTERM to the process for graceful shutdown. +// gracefulStop sends SIGTERM to the process group for graceful shutdown. // Falls back to SIGKILL if SIGTERM fails. func (l *Launcher) gracefulStop(pid int) error { - if err := syscall.Kill(pid, syscall.SIGTERM); err != nil { + l.logger.Debug("agentctl subprocess SIGTERM requested", zap.Int("pgid", pid)) + if err := syscall.Kill(-pid, syscall.SIGTERM); err != nil { l.logger.Warn("failed to send SIGTERM, trying SIGKILL", zap.Error(err)) - _ = syscall.Kill(pid, syscall.SIGKILL) + l.logger.Debug("agentctl subprocess SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "sigterm_failed")) + _ = syscall.Kill(-pid, syscall.SIGKILL) return err } return nil } -// forceKill sends SIGKILL to the agentctl process. +// forceKill sends SIGKILL to the agentctl process group. func (l *Launcher) forceKill(pid int) { - _ = syscall.Kill(pid, syscall.SIGKILL) + l.logger.Debug("agentctl subprocess SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "force_kill")) + _ = syscall.Kill(-pid, syscall.SIGKILL) } diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix_test.go b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix_test.go new file mode 100644 index 0000000000..c78b0ee7c0 --- /dev/null +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_unix_test.go @@ -0,0 +1,141 @@ +//go:build !windows + +package launcher + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +func TestLauncherLogsSignalAttempts(t *testing.T) { + core, observed := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + launcher := &Launcher{logger: log} + const missingPID = 1 << 30 + + if err := launcher.gracefulStop(missingPID); err == nil { + t.Fatal("expected missing process to fail graceful stop") + } + launcher.forceKill(missingPID) + + for _, message := range []string{ + "agentctl subprocess SIGTERM requested", + "agentctl subprocess SIGKILL requested", + } { + if !launcherLogsContain(observed, message) { + t.Fatalf("expected debug log %q, got %#v", message, observed.All()) + } + } +} + +func TestLauncherForceKillSignalsProcessGroup(t *testing.T) { + tmp := t.TempDir() + childPIDFile := filepath.Join(tmp, "child.pid") + cmd := exec.Command("sh", "-c", `sleep 30 & echo $! > "$CHILD_PID_FILE"; wait`) + cmd.Env = append(os.Environ(), "CHILD_PID_FILE="+childPIDFile) + cmd.SysProcAttr = buildSysProcAttr() + if err := cmd.Start(); err != nil { + t.Fatalf("start command: %v", err) + } + pgid := cmd.Process.Pid + var childPID int + t.Cleanup(func() { + _ = syscall.Kill(-pgid, syscall.SIGKILL) + _ = cmd.Wait() + }) + + waitUntilLauncherTest(t, time.Second, func() bool { + raw, err := os.ReadFile(childPIDFile) + if err != nil { + return false + } + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + if err != nil { + return false + } + childPID = pid + return processExistsForLauncherTest(childPID) + }, "child process pid was not written or running") + + launcher := &Launcher{logger: newLauncherTestLogger(t)} + launcher.forceKill(pgid) + + waitUntilLauncherTest(t, 2*time.Second, func() bool { + return !processExistsForLauncherTest(childPID) + }, "forceKill did not kill process-group child %d", childPID) +} + +func launcherLogsContain(logs *observer.ObservedLogs, message string) bool { + for _, entry := range logs.All() { + if entry.Message == message { + return true + } + } + return false +} + +func newLauncherTestLogger(t *testing.T) *logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{ + Level: "error", + Format: "json", + }) + if err != nil { + t.Fatalf("new logger: %v", err) + } + return log +} + +func processExistsForLauncherTest(pid int) bool { + if pid <= 0 { + return false + } + if runtime.GOOS == "linux" { + raw, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return false + } + s := string(raw) + idx := strings.LastIndex(s, ") ") + if idx == -1 || idx+2 >= len(s) { + return false + } + return s[idx+2] != 'Z' + } + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + return proc.Signal(syscall.Signal(0)) == nil +} + +func waitUntilLauncherTest(t *testing.T, timeout time.Duration, condition func() bool, format string, args ...any) { + t.Helper() + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if condition() { + return + } + time.Sleep(20 * time.Millisecond) + } + if condition() { + return + } + t.Fatalf(format, args...) +} diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/platform_windows.go b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_windows.go index 0d5ec0b4e3..20a1dfc8aa 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/platform_windows.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/platform_windows.go @@ -16,8 +16,12 @@ func (l *Launcher) gracefulStop(pid int) error { return err } // os.Interrupt on Windows sends CTRL_BREAK_EVENT + l.logger.Debug("agentctl subprocess interrupt requested", zap.Int("pid", pid)) if err := proc.Signal(os.Interrupt); err != nil { l.logger.Warn("failed to send interrupt, trying kill", zap.Error(err)) + l.logger.Debug("agentctl subprocess kill requested", + zap.Int("pid", pid), + zap.String("reason", "interrupt_failed")) _ = proc.Kill() return err } @@ -27,6 +31,9 @@ func (l *Launcher) gracefulStop(pid int) error { // forceKill terminates the process on Windows. func (l *Launcher) forceKill(pid int) { if proc, err := os.FindProcess(pid); err == nil { + l.logger.Debug("agentctl subprocess kill requested", + zap.Int("pid", pid), + zap.String("reason", "force_kill")) _ = proc.Kill() } } diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_default.go b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_default.go index 8e620681ad..16c9316fa6 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_default.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_default.go @@ -1,9 +1,13 @@ -//go:build !linux && !windows +//go:build unix && !linux package launcher import "syscall" func buildSysProcAttr() *syscall.SysProcAttr { - return &syscall.SysProcAttr{} + return &syscall.SysProcAttr{ + // Keep standalone agentctl out of the terminal foreground process + // group so Ctrl+C is sequenced by the backend shutdown path. + Setpgid: true, + } } diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_linux.go b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_linux.go index ab76fcce86..b634567127 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_linux.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_linux.go @@ -6,6 +6,9 @@ import "syscall" func buildSysProcAttr() *syscall.SysProcAttr { return &syscall.SysProcAttr{ + // Keep standalone agentctl out of the terminal foreground process + // group so Ctrl+C is sequenced by the backend shutdown path. + Setpgid: true, Pdeathsig: syscall.SIGTERM, } } diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_other.go b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_other.go new file mode 100644 index 0000000000..b0dee6b390 --- /dev/null +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_other.go @@ -0,0 +1,9 @@ +//go:build !unix && !windows + +package launcher + +import "syscall" + +func buildSysProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{} +} diff --git a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_test.go b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_test.go index 09f185449d..990d5851b3 100644 --- a/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_test.go +++ b/apps/backend/internal/agent/runtime/agentctl/launcher/sysprocattr_test.go @@ -1,4 +1,4 @@ -//go:build !windows +//go:build unix package launcher @@ -6,9 +6,9 @@ import ( "testing" ) -func TestBuildSysProcAttr_NoSetpgid(t *testing.T) { +func TestBuildSysProcAttr_IsolatesAgentctlFromTerminalInterrupt(t *testing.T) { attr := buildSysProcAttr() - if attr.Setpgid { - t.Error("Setpgid must be false: standalone agentctl should share the backend's process group") + if !attr.Setpgid { + t.Error("Setpgid must be true: standalone agentctl should not receive terminal Ctrl+C directly") } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go index 62e8c5310f..e63ef3b47d 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go @@ -298,9 +298,10 @@ type ExecutorInstance struct { StandalonePort int // Standalone // Common fields - WorkspacePath string - Metadata map[string]interface{} - StopReason string + WorkspacePath string + Metadata map[string]interface{} + StopReason string + AgentStopFailed bool // AuthToken is the agentctl auth token retrieved via handshake. // Populated by Docker executor for encrypted storage in SecretStore. diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go b/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go index d0e3e625f6..d0537338fd 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go @@ -22,6 +22,8 @@ import ( ) const dockerWorkspacePath = "/workspace" +const dockerStopContainerTimeout = 30 * time.Second +const dockerFallbackCleanupTimeout = dockerStopContainerTimeout + 5*time.Second // getMetadataString retrieves a string value from metadata map. func getMetadataString(metadata map[string]interface{}, key string) string { @@ -595,7 +597,8 @@ func (r *DockerExecutor) StopInstance(ctx context.Context, instance *ExecutorIns // the kandev-managed per-container session dir so we don't leak GBs of // agent state on disk. Plain stops preserve the dir so resume re-attaches // to the same agent state, mirroring the Sprites preserve-on-stop rule. - if shouldRunExecutorCleanup(instance.StopReason) && r.kandevHomeDir != "" && instance.InstanceID != "" { + teardownContainer := shouldTeardownDockerContainer(instance.StopReason) + if teardownContainer && r.kandevHomeDir != "" && instance.InstanceID != "" { CleanupAgentSessionDir(InstanceSessionRoot(r.kandevHomeDir, instance.InstanceID), r.logger) } @@ -603,15 +606,31 @@ func (r *DockerExecutor) StopInstance(ctx context.Context, instance *ExecutorIns return nil // No container to stop } + // Plain "stop the agent" runs (e.g. user clicks Stop, then later wants to + // resume) must not stop the Docker container after agentctl stopped cleanly. + // The container holds the cloned workspace and agentctl process for fast + // resume; destructive lifecycle events or failed agentctl stops should tear + // it down. + if !force && !instance.AgentStopFailed && !teardownContainer { + r.logger.Info("preserving docker container after agent stop", + zap.String("container_id", instance.ContainerID), + zap.String("instance_id", instance.InstanceID), + zap.String("stop_reason", instance.StopReason)) + return nil + } + dockerClient, _, err := r.ensureClient() if err != nil { return fmt.Errorf("docker unavailable: %w", err) } + cleanupCtx, cancel := dockerCleanupContext(ctx, instance.AgentStopFailed) + defer cancel() + if force { - err = dockerClient.KillContainer(ctx, instance.ContainerID, "SIGKILL") + err = dockerClient.KillContainer(cleanupCtx, instance.ContainerID, "SIGKILL") } else { - err = dockerClient.StopContainer(ctx, instance.ContainerID, 30*time.Second) + err = dockerClient.StopContainer(cleanupCtx, instance.ContainerID, dockerStopContainerTimeout) } if err != nil { @@ -621,6 +640,25 @@ func (r *DockerExecutor) StopInstance(ctx context.Context, instance *ExecutorIns return nil } +// shouldTeardownDockerContainer extends terminal cleanup with stale execution +// cleanup for Docker only. A stale Docker execution owns a local container and +// per-instance session dir that become untracked before retry/resume launches a +// replacement. Sprites intentionally keep stale sandboxes; see +// shouldRunExecutorCleanup for that shared runtime policy. +func shouldTeardownDockerContainer(reason string) bool { + if shouldRunExecutorCleanup(reason) { + return true + } + return strings.ToLower(strings.TrimSpace(reason)) == stopReasonStaleExecutionCleanup +} + +func dockerCleanupContext(ctx context.Context, agentStopFailed bool) (context.Context, context.CancelFunc) { + if agentStopFailed { + return context.WithTimeout(context.WithoutCancel(ctx), dockerFallbackCleanupTimeout) + } + return ctx, func() {} +} + func (r *DockerExecutor) RecoverInstances(_ context.Context) ([]*ExecutorInstance, error) { // No-op: Docker client is initialized lazily on first use. // If no session has used Docker yet, there's nothing to recover. diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go index 9b36e27cf5..e2eb80ac39 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go @@ -87,6 +87,87 @@ func TestDockerExecutor_GetInteractiveRunner(t *testing.T) { } } +func TestDockerStopInstancePreservesContainerOnPlainStop(t *testing.T) { + log := newTestDockerLogger() + exec := NewDockerExecutor(config.DockerConfig{}, "", log) + exec.newClientFunc = func(_ config.DockerConfig, _ *logger.Logger) (*docker.Client, error) { + t.Fatal("plain stop should not initialize docker client") + return nil, nil + } + + if err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "inst-1", + ContainerID: "container-1", + StopReason: "stopped via API", + }, false); err != nil { + t.Fatalf("StopInstance: %v", err) + } +} + +func TestDockerStopInstanceStopsContainerWhenAgentStopFailed(t *testing.T) { + log := newTestDockerLogger() + exec := NewDockerExecutor(config.DockerConfig{}, "", log) + exec.newClientFunc = failingClientFactory("docker required after agent stop failure") + + err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "inst-1", + ContainerID: "container-1", + StopReason: "stopped via API", + AgentStopFailed: true, + }, false) + if err == nil { + t.Fatal("StopInstance should attempt Docker cleanup after agentctl stop failure") + } + if !strings.Contains(err.Error(), "docker required after agent stop failure") { + t.Fatalf("StopInstance error = %v", err) + } +} + +func TestDockerStopInstanceStopsContainerOnStaleCleanup(t *testing.T) { + log := newTestDockerLogger() + exec := NewDockerExecutor(config.DockerConfig{}, "", log) + exec.newClientFunc = failingClientFactory("docker required for stale cleanup") + + err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "inst-1", + ContainerID: "container-1", + StopReason: stopReasonStaleExecutionCleanup, + }, false) + if err == nil { + t.Fatal("StopInstance should attempt Docker cleanup for stale executions") + } + if !strings.Contains(err.Error(), "docker required for stale cleanup") { + t.Fatalf("StopInstance error = %v", err) + } +} + +func TestDockerCleanupContextIgnoresCanceledParentAfterAgentStopFailed(t *testing.T) { + parent, parentCancel := context.WithCancel(context.Background()) + parentCancel() + + cleanupCtx, cleanupCancel := dockerCleanupContext(parent, true) + defer cleanupCancel() + + if err := cleanupCtx.Err(); err != nil { + t.Fatalf("cleanup context should remain usable after parent cancellation, got %v", err) + } + if _, ok := cleanupCtx.Deadline(); !ok { + t.Fatal("cleanup context should keep its own timeout") + } +} + +func TestDockerCleanupContextKeepsParentCancellationForNormalStops(t *testing.T) { + parent, parentCancel := context.WithCancel(context.Background()) + cleanupCtx, cleanupCancel := dockerCleanupContext(parent, false) + defer cleanupCancel() + + parentCancel() + + if err := cleanupCtx.Err(); err != context.Canceled { + t.Fatalf("cleanup context error = %v, want context.Canceled", err) + } +} + func TestDockerExecutor_EnsureClient_Success(t *testing.T) { log := newTestDockerLogger() exec := NewDockerExecutor(config.DockerConfig{}, "", log) diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_sprites_lifecycle.go b/apps/backend/internal/agent/runtime/lifecycle/executor_sprites_lifecycle.go index 994e9ffbc1..f2015fe34b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_sprites_lifecycle.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_sprites_lifecycle.go @@ -132,7 +132,10 @@ func (r *SpritesExecutor) runTerminalCleanupScript(ctx context.Context, sprite * // Terminal stop reasons that trigger destructive executor cleanup // (sandbox teardown, container removal, per-instance session-dir removal). // Anything outside this set is treated as a "preserve" stop — see -// shouldRunExecutorCleanup. +// shouldRunExecutorCleanup. Stale execution cleanup is intentionally excluded +// from this shared set: Docker has a runtime-specific helper to remove local +// stale containers, while Sprites preserves cloud sandboxes for faster and less +// destructive recovery. const ( StopReasonTaskArchived = "task archived" StopReasonTaskDeleted = "task deleted" diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager.go b/apps/backend/internal/agent/runtime/lifecycle/manager.go index 169e50943d..34f89d9777 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager.go @@ -92,8 +92,7 @@ type Manager struct { // shuttingDown is flipped true when graceful shutdown begins (see // StopAllAgents) so handlers running in detached goroutines can // short-circuit work that would otherwise race the teardown and log - // confusing errors against children that already died from the same - // terminal-wide SIGINT. + // confusing errors against children already being stopped. shuttingDown atomic.Bool // pollAggregator routes hub session-mode events to agentctl. See diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 5f3848839d..1996f12ebd 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -521,12 +521,13 @@ func (m *Manager) StopAgentWithReason(ctx context.Context, executionID string, r zap.Stringer("runtime", execution.RuntimeName)) // Try to gracefully stop via agentctl first, then always close connections + agentStopFailed := false if execution.agentctl != nil { if !force { if err := execution.agentctl.Stop(ctx); err != nil { - // During shutdown agentctl typically received the same - // terminal-wide SIGINT and is already gone, so a failed - // HTTP call here is expected, not noteworthy. + agentStopFailed = true + // During shutdown the instance may already be stopping through + // another lifecycle path, so a failed HTTP call is expected. if m.IsShuttingDown() { m.logger.Debug("failed to stop agent via agentctl", zap.String("execution_id", executionID), @@ -542,7 +543,7 @@ func (m *Manager) StopAgentWithReason(ctx context.Context, executionID string, r } // Stop the agent execution via the runtime that created it - m.stopAgentViaBackend(ctx, executionID, execution, reason, force) + m.stopAgentViaBackend(ctx, executionID, execution, reason, force, agentStopFailed) // Update execution status and remove from tracking _ = m.executionStore.WithLock(executionID, func(exec *AgentExecution) { @@ -1254,7 +1255,7 @@ func (m *Manager) RespondToPermissionBySessionID(sessionID, pendingID, optionID } // stopAgentViaBackend stops the agent execution via the runtime that created it. -func (m *Manager) stopAgentViaBackend(ctx context.Context, executionID string, execution *AgentExecution, reason string, force bool) { +func (m *Manager) stopAgentViaBackend(ctx context.Context, executionID string, execution *AgentExecution, reason string, force bool, agentStopFailed bool) { if execution.RuntimeName == "" || m.executorRegistry == nil { return } @@ -1275,11 +1276,11 @@ func (m *Manager) stopAgentViaBackend(ctx context.Context, executionID string, e StandalonePort: execution.standalonePort, Metadata: execution.Metadata, StopReason: reason, + AgentStopFailed: agentStopFailed, } if err := rt.StopInstance(ctx, runtimeInstance, force); err != nil { - // During shutdown the runtime instance (e.g. a standalone agentctl) - // often already exited via the shared SIGINT, so StopInstance returns - // a benign 404. Only surface this at WARN outside shutdown. + // During shutdown the runtime instance may already be stopping or + // absent. Only surface this at WARN outside shutdown. if m.IsShuttingDown() { m.logger.Debug("failed to stop runtime instance, continuing with cleanup", zap.String("execution_id", executionID), diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go index 5d0de5811e..1fadb8f093 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_lifecycle.go @@ -223,6 +223,8 @@ func (m *Manager) StopAllAgents(ctx context.Context) error { return errors.Join(errs...) } +const stopReasonStaleExecutionCleanup = "stale execution cleanup" + // cleanupExitedContainer handles cleanup for a single exited container. func (m *Manager) cleanupExitedContainer(ctx context.Context, containerID string) { execution, tracked := m.executionStore.GetByContainerID(containerID) @@ -313,7 +315,7 @@ func (m *Manager) CleanupStaleExecutionBySessionID(ctx context.Context, sessionI // 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) + m.stopAgentViaBackend(ctx, execution.ID, execution, stopReasonStaleExecutionCleanup, false, false) // Close agentctl connection if it exists if execution.agentctl != nil { diff --git a/apps/backend/internal/agentctl/AGENTS.md b/apps/backend/internal/agentctl/AGENTS.md index 562bbc11bc..a8ba7d0289 100644 --- a/apps/backend/internal/agentctl/AGENTS.md +++ b/apps/backend/internal/agentctl/AGENTS.md @@ -71,20 +71,33 @@ Kandev renders subagents (the `Task` tool) as cards and *wants* to nest each sub Claude is the one that works today: `claude-agent-acp` (since PR #341) sets `_meta.claudeCode.parentToolUseId` on a subagent's internal calls, and its value already equals the parent Task tool_call id — so it maps straight onto our `parent_tool_call_id`. Cursor exposes nothing to nest. OpenCode needs a kandev-side child-session merge. (Top-level `parentToolCallId` is NOT in the ACP schema — `_meta` is the spec-compliant carrier.) -## Process-group kill for HTTP-server agents (`RequiresProcessKill`) +## Process-group cleanup and `RequiresProcessKill` -Most ACP agents (Claude Code, Codex, Cursor, …) exit cleanly when stdin is closed. Some agents — notably `opencode acp` — keep their HTTP server and an MCP child tree alive after stdin EOF. For those agents, closing stdin on shutdown is not enough; the process group has to be SIGKILLed so the MCP children don't re-parent to init and leak (GH issue #1247). +All ACP agents are launched in their own process group, and shutdown must not +report success while any process in that group is still alive. Some agents — +notably `opencode acp` — keep their HTTP server and an MCP child tree alive after +stdin EOF. For those agents, closing stdin on shutdown is not useful; the process +group has to be killed immediately so the MCP children don't re-parent to init +and leak (GH issue #1247). -The signal flows agent → adapter → process manager via a single bool: +The "skip graceful wait" signal flows agent → adapter → process manager via a +single bool: 1. `agents.RuntimeConfig.RequiresProcessKill` (set `true` on `OpenCodeACP`). 2. The lifecycle executors copy it into `agentctl.CreateInstanceRequest.RequiresProcessKill`. 3. `instance.Manager` stores it on `config.InstanceConfig.RequiresProcessKill`. 4. `process.Manager.buildAdapterConfig` forwards it into `adapter.Config` → `shared.Config`. 5. `acp.Adapter.RequiresProcessKill()` returns `cfg.RequiresProcessKill`. -6. On shutdown, `process.Manager.killProcessGroupIfRequired` consults the adapter and, when true, sends SIGKILL to the whole pgid via `killProcessGroup` (`syscall.Kill(-pid, SIGKILL)` on Unix). If `Stop(ctx)` further times out before the process exits, `waitForProcessExit`'s `ctx.Done` branch re-runs the pgid SIGKILL (falling back to a leader-only kill if the group call errors) — belt-and-braces for any future ACP CLI that decides to ignore stdin close. +6. On shutdown, `process.Manager.killProcessGroupIfRequired` consults the adapter and, when true, sends SIGKILL to the whole pgid via `killProcessGroup` (`syscall.Kill(-pid, SIGKILL)` on Unix). -To add another agent that needs this: set `RequiresProcessKill: true` in its `Runtime()` config — that's all. +`RequiresProcessKill: false` does **not** mean children may be left running. It +only allows the normal graceful path first (stdin close, adapter close, process +wait). After the command leader exits, `waitForProcessExit` still checks the +agent process group and sends SIGTERM/SIGKILL if descendants remain. If `Stop(ctx)` +times out, it also re-runs the pgid SIGKILL fallback. + +To add another agent that needs immediate kill instead of graceful stdin close: +set `RequiresProcessKill: true` in its `Runtime()` config. ## Idle-instance reaper (`KANDEV_ACP_IDLE_TIMEOUT`) diff --git a/apps/backend/internal/agentctl/server/config/config.go b/apps/backend/internal/agentctl/server/config/config.go index 3f15ece89b..c7e9e797e2 100644 --- a/apps/backend/internal/agentctl/server/config/config.go +++ b/apps/backend/internal/agentctl/server/config/config.go @@ -216,9 +216,9 @@ type InstanceConfig struct { // Inherited from the parent Config at instance creation time. AuthToken string - // RequiresProcessKill forces the agent's process group to be killed on - // shutdown instead of relying on stdin close. Required for agents whose - // runtime keeps child processes alive when stdin closes (opencode acp). + // RequiresProcessKill skips the graceful stdin-close wait and reaps the + // agent's process group immediately. Required for agents whose runtime + // keeps child processes alive when stdin closes (opencode acp). RequiresProcessKill bool // BaseBranches maps RepositoryName → base branch ref for per-repo diff diff --git a/apps/backend/internal/agentctl/server/instance/instance.go b/apps/backend/internal/agentctl/server/instance/instance.go index 5b18ea9ed3..cc5320ed5a 100644 --- a/apps/backend/internal/agentctl/server/instance/instance.go +++ b/apps/backend/internal/agentctl/server/instance/instance.go @@ -4,7 +4,6 @@ package instance import ( - "net/http" "sync/atomic" "time" @@ -39,7 +38,7 @@ type Instance struct { manager *process.Manager // server is the HTTP server for this instance's API (unexported) - server *http.Server + server instanceHTTPServer // lastActivityNanos is the unix-nano timestamp of the most recently // observed HTTP request on this instance's port. Used by the idle reaper diff --git a/apps/backend/internal/agentctl/server/instance/manager.go b/apps/backend/internal/agentctl/server/instance/manager.go index 154a7be7ae..596641e96d 100644 --- a/apps/backend/internal/agentctl/server/instance/manager.go +++ b/apps/backend/internal/agentctl/server/instance/manager.go @@ -24,6 +24,8 @@ import ( // ServerFactory creates an HTTP handler for an instance given its config and process manager. type ServerFactory func(cfg *config.InstanceConfig, procMgr *process.Manager, log *logger.Logger) http.Handler +const instanceHTTPShutdownGrace = 250 * time.Millisecond + // Manager manages multiple agent instances. // It handles creation, tracking, and removal of agent instances, // each with their own HTTP server on a dedicated port. @@ -345,12 +347,10 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { zap.String("instance_id", id), zap.Int("port", inst.Port)) - // Shutdown HTTP server (potentially slow, done without lock) + var stopErr error if inst.server != nil { - if err := inst.server.Shutdown(ctx); err != nil { - m.logger.Warn("error shutting down HTTP server", - zap.String("instance_id", id), - zap.Error(err)) + if err := m.stopHTTPServer(ctx, id, inst.Port, inst.server); err != nil { + stopErr = err } } @@ -363,6 +363,10 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { m.portAlloc.Release(inst.Port) m.mu.Unlock() + if stopErr != nil { + return stopErr + } + m.logger.Info("StopInstance completed", zap.String("instance_id", id), zap.Int("port", inst.Port)) @@ -370,6 +374,35 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { return nil } +type instanceHTTPServer interface { + Shutdown(context.Context) error + Close() error +} + +func (m *Manager) stopHTTPServer(ctx context.Context, id string, port int, server instanceHTTPServer) error { + serverCtx, cancel := context.WithTimeout(ctx, instanceHTTPShutdownGrace) + err := server.Shutdown(serverCtx) + cancel() + if err == nil || errors.Is(err, http.ErrServerClosed) { + return nil + } + m.logger.Debug("StopInstance: HTTP server graceful shutdown expired, closing active connections", + zap.String("instance_id", id), + zap.Int("port", port), + zap.Duration("grace", instanceHTTPShutdownGrace), + zap.Error(err)) + if closeErr := server.Close(); closeErr != nil && !errors.Is(closeErr, http.ErrServerClosed) { + m.logger.Warn("error closing HTTP server", + zap.String("instance_id", id), + zap.Error(closeErr)) + return fmt.Errorf("close HTTP server for instance %s: %w", id, closeErr) + } + if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, context.Canceled) { + return fmt.Errorf("shutdown HTTP server for instance %s: %w", id, err) + } + return nil +} + // Shutdown stops all instances gracefully. func (m *Manager) Shutdown(ctx context.Context) error { // Stop the idle reaper first so it doesn't fire StopInstance concurrently diff --git a/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go b/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go new file mode 100644 index 0000000000..786ccad1e0 --- /dev/null +++ b/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go @@ -0,0 +1,172 @@ +package instance + +import ( + "context" + "errors" + "net" + "net/http" + "testing" + "time" + + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/pkg/agent" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStopInstanceBoundsHTTPServerShutdown(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 0, Max: 0}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + t.Cleanup(func() { + _ = mgr.Shutdown(context.Background()) + }) + + requestStarted := make(chan struct{}) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + close(requestStarted) + select { + case <-r.Context().Done(): + return + case <-time.After(2 * time.Second): + w.WriteHeader(http.StatusOK) + } + }) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + server := &http.Server{Handler: handler} + serveErr := make(chan error, 1) + go func() { + serveErr <- server.Serve(listener) + }() + + inst := &Instance{ + ID: "slow-http-shutdown", + Port: listener.Addr().(*net.TCPAddr).Port, + Status: "running", + CreatedAt: time.Now(), + server: server, + } + mgr.mu.Lock() + mgr.instances[inst.ID] = inst + mgr.mu.Unlock() + + clientDone := make(chan struct{}) + go func() { + defer close(clientDone) + resp, err := http.Get("http://" + listener.Addr().String() + "/probe") + if err == nil { + _ = resp.Body.Close() + } + }() + + select { + case <-requestStarted: + case <-time.After(time.Second): + t.Fatal("test HTTP request did not reach server") + } + + start := time.Now() + require.NoError(t, mgr.StopInstance(context.Background(), inst.ID)) + elapsed := time.Since(start) + + assert.Less(t, elapsed, 700*time.Millisecond, + "StopInstance should not wait for long-running probe handlers during shutdown") + + select { + case <-clientDone: + case <-time.After(time.Second): + t.Fatal("test HTTP client did not finish after instance shutdown") + } + + select { + case err := <-serveErr: + require.ErrorIs(t, err, http.ErrServerClosed) + case <-time.After(time.Second): + t.Fatal("test HTTP server did not stop") + } +} + +func TestStopHTTPServerReturnsCloseError(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 0, Max: 0}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + + closeErr := errors.New("listener close failed") + server := &fakeHTTPServer{ + shutdownErr: context.DeadlineExceeded, + closeErr: closeErr, + } + + err := mgr.stopHTTPServer(context.Background(), "close-failure", 12345, server) + require.ErrorIs(t, err, closeErr) + require.True(t, server.closed) +} + +func TestStopInstanceReleasesPortWhenHTTPServerCloseFails(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 12345, Max: 12345}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + + port, err := mgr.portAlloc.Allocate("close-failure") + require.NoError(t, err) + + closeErr := errors.New("listener close failed") + inst := &Instance{ + ID: "close-failure", + Port: port, + Status: "running", + CreatedAt: time.Now(), + server: &fakeHTTPServer{ + shutdownErr: context.DeadlineExceeded, + closeErr: closeErr, + }, + } + + mgr.mu.Lock() + mgr.instances[inst.ID] = inst + mgr.mu.Unlock() + + err = mgr.StopInstance(context.Background(), inst.ID) + require.ErrorIs(t, err, closeErr) + + reusedPort, err := mgr.portAlloc.Allocate("next-instance") + require.NoError(t, err) + require.Equal(t, port, reusedPort) +} + +func TestStopHTTPServerTreatsCanceledShutdownAsStoppedAfterClose(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 0, Max: 0}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + + server := &fakeHTTPServer{shutdownErr: context.Canceled} + + err := mgr.stopHTTPServer(context.Background(), "canceled-shutdown", 12345, server) + require.NoError(t, err) + require.True(t, server.closed) +} + +type fakeHTTPServer struct { + shutdownErr error + closeErr error + closed bool +} + +func (s *fakeHTTPServer) Shutdown(context.Context) error { + return s.shutdownErr +} + +func (s *fakeHTTPServer) Close() error { + s.closed = true + return s.closeErr +} diff --git a/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go b/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go index ee6e456d74..72956e24fa 100644 --- a/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go +++ b/apps/backend/internal/agentctl/server/process/interactive_lifecycle.go @@ -404,6 +404,9 @@ func (r *InteractiveRunner) Stop(ctx context.Context, processID string) error { // Terminate the process directly (PTY handles its own session management) if proc.cmd != nil && proc.cmd.Process != nil { + r.logger.Debug("interactive process terminate requested", + zap.String("process_id", processID), + zap.Int("pid", pid)) _ = terminateProcess(proc.cmd.Process) // Wait for the wait() goroutine to finish (it calls cmd.Wait). @@ -414,11 +417,19 @@ func (r *InteractiveRunner) Stop(ctx context.Context, processID string) error { zap.String("process_id", processID), zap.Int("os_pid", pid), zap.Error(ctx.Err())) + r.logger.Debug("interactive process SIGKILL requested", + zap.String("process_id", processID), + zap.Int("pid", pid), + zap.String("reason", "context_canceled")) _ = proc.cmd.Process.Kill() case <-time.After(2 * time.Second): r.logger.Warn("interactive process stop timed out; killing process", zap.String("process_id", processID), zap.Int("os_pid", pid)) + r.logger.Debug("interactive process SIGKILL requested", + zap.String("process_id", processID), + zap.Int("pid", pid), + zap.String("reason", "grace_expired")) _ = proc.cmd.Process.Kill() case <-proc.waitDone: // Process exited cleanly diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 1d9db98f12..b114314677 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -70,19 +70,31 @@ const defaultStderrBufferSize = 50 const agentTempDirRoot = "kandev-agent" +const processKillRequiredExitGrace = 250 * time.Millisecond + +// processDefaultExitGrace is the no-deadline upper bound for graceful adapters +// after adapter/stdin close. It is intentionally longer than the kill-required +// fast path so ACP-style agents can flush state, while still bounding callers +// that pass context.Background(). +const processDefaultExitGrace = 5 * time.Second + +const processGroupTerminateGrace = 2 * time.Second +const processGroupPollInterval = 50 * time.Millisecond + // Manager manages the agent subprocess type Manager struct { cfg *config.InstanceConfig logger *logger.Logger // Process state - cmd *exec.Cmd - stdin io.WriteCloser - stdout io.ReadCloser - stderr io.ReadCloser - status atomic.Value // Status - exitCode atomic.Int32 - exitErr atomic.Value // error + cmd *exec.Cmd + processLifecycle processLifecycleHandle + stdin io.WriteCloser + stdout io.ReadCloser + stderr io.ReadCloser + status atomic.Value // Status + exitCode atomic.Int32 + exitErr atomic.Value // error // Stderr buffering for error context stderrBuffer []string @@ -733,12 +745,23 @@ func (m *Manager) Start(ctx context.Context) error { m.status.Store(StatusError) return formatAgentStartError(err, m.cfg.AgentEnv) } + processLifecycle, err := installProcessLifecycle(m.cmd) + if err != nil { + m.logger.Warn("failed to install agent process lifecycle; falling back to process-tree cleanup", + zap.Error(err)) + } else { + m.processLifecycle = processLifecycle + } m.stopCh = make(chan struct{}) m.doneCh = make(chan struct{}) // Connect adapter to the process stdin/stdout pipes if err := m.adapter.Connect(m.stdin, m.stdout); err != nil { + releaseProcessLifecycle(m.processLifecycle) + m.processLifecycle = processLifecycleHandle{} + _ = m.cmd.Process.Kill() + _ = m.cmd.Wait() m.status.Store(StatusError) return fmt.Errorf("failed to connect adapter: %w", err) } @@ -882,7 +905,7 @@ func (m *Manager) buildFinalCommand() error { // Create a new process group so we can kill all child processes together. // This is important for adapters like OpenCode that spawn child processes // (npx -> sh -> node -> opencode binary). - setProcGroup(m.cmd) + setAgentProcGroup(m.cmd) envBytes, largestEnv := summarizeEnvBytes(m.cfg.AgentEnv, 3) m.logger.Info("agent command prepared", @@ -1214,16 +1237,28 @@ func (m *Manager) Stop(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() + // Stop intentionally serializes the full teardown under m.mu. The shutdown + // path closes adapters, stdin, shell sessions, and process groups as one + // lifecycle transition; concurrent readers may briefly block while process + // group reaping finishes. + // Stop trackers before the status guard: passthrough never calls Start() so the early return below would otherwise leak them. m.stopWorkspaceTrackers() status := m.Status() if status == StatusStopped || status == StatusStopping { - m.logger.Info("Stop called but already stopped/stopping", zap.String("status", string(status))) + m.logger.Info("Stop called but already stopped/stopping", + zap.String("status", string(status)), + zap.Int("pid", m.agentPID())) return nil } - m.logger.Info("stopping agent process - START") + m.logger.Info("stopping agent process - START", + zap.Int("pid", m.agentPID()), + zap.String("protocol", m.agentProtocol())) + m.logger.Debug("agent process stop requested", + zap.Int("pid", m.agentPID()), + zap.String("protocol", m.agentProtocol())) m.status.Store(StatusStopping) m.stopShellAndProcesses(ctx) @@ -1236,6 +1271,20 @@ func (m *Manager) Stop(ctx context.Context) error { return nil } +func (m *Manager) agentPID() int { + if m.cmd == nil || m.cmd.Process == nil { + return 0 + } + return m.cmd.Process.Pid +} + +func (m *Manager) agentProtocol() string { + if m.cfg == nil { + return "" + } + return string(m.cfg.Protocol) +} + // stopShellAndProcesses stops the shell session, VS Code, and workspace processes. func (m *Manager) stopShellAndProcesses(ctx context.Context) { // Stop VS Code server if running @@ -1294,8 +1343,10 @@ func (m *Manager) closeAdapterAndStdin() { m.logger.Debug("stdin closed") } -// killProcessGroupIfRequired kills the entire process group for adapters (such as -// OpenCode) that run as HTTP servers and do not exit when stdin is closed. +// killProcessGroupIfRequired immediately kills the entire process group for +// adapters (such as OpenCode) that are known not to exit when stdin is closed. +// Other adapters still get process-group cleanup in waitForProcessExit after +// their graceful stdin-close path has had a chance to finish. func (m *Manager) killProcessGroupIfRequired() { if m.adapter == nil || !m.adapter.RequiresProcessKill() { return @@ -1306,9 +1357,14 @@ func (m *Manager) killProcessGroupIfRequired() { // We kill the process group to ensure all child processes are killed too. // This is important because OpenCode spawns: npx -> sh -> node -> opencode binary pid := m.cmd.Process.Pid - m.logger.Debug("killing process group", zap.Int("pgid", pid)) + m.logger.Debug("agent process group SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "adapter_requires_process_kill")) if err := killProcessGroup(pid); err != nil { m.logger.Debug("failed to kill process group, trying single process", zap.Error(err)) + m.logger.Debug("agent process SIGKILL requested", + zap.Int("pid", pid), + zap.String("reason", "process_group_kill_failed")) if err := m.cmd.Process.Kill(); err != nil { m.logger.Warn("failed to kill process", zap.Error(err)) } @@ -1319,9 +1375,11 @@ func (m *Manager) killProcessGroupIfRequired() { // // On timeout the entire process group is killed (not just the leader) so that // child processes — most importantly MCP servers spawned by the agent — don't -// re-parent to init and leak. setProcGroup at command-build time puts the -// agent in its own pgid; here we deliver SIGKILL to that pgid. Falls back to -// a single-process kill only if the process-group call fails. +// re-parent to init and leak. setAgentProcGroup at command-build time puts the +// agent in its own pgid; here we deliver SIGKILL to that pgid. If the command +// leader exits before its descendants do, we still terminate the remaining +// process group before reporting shutdown complete. Falls back to a +// single-process kill only if the process-group call fails. func (m *Manager) waitForProcessExit(ctx context.Context) { m.logger.Debug("waiting for process to exit") done := make(chan struct{}) @@ -1330,21 +1388,176 @@ func (m *Manager) waitForProcessExit(ctx context.Context) { close(done) }() + pid := 0 + if m.cmd != nil && m.cmd.Process != nil { + pid = m.cmd.Process.Pid + } + + exitGrace := m.processExitGrace(ctx) + if waitForManagerDone(ctx, done, exitGrace) { + m.logger.Info("agent process stopped gracefully") + m.reapRemainingProcessGroup(ctx, pid) + return + } + if pid == 0 { + return + } + if ctx.Err() != nil { + m.logger.Warn("force killing agent process group", zap.Int("pgid", pid)) + m.forceKillProcessGroupAndWait(done, pid) + return + } + + m.logger.Warn("agent process did not exit after graceful wait; terminating process group", + zap.Int("pgid", pid), + zap.Duration("grace", exitGrace)) + m.logger.Debug("agent process group SIGTERM requested", + zap.Int("pgid", pid), + zap.String("reason", "graceful_wait_expired"), + zap.Duration("grace", exitGrace)) + if err := terminateProcessGroup(pid); err != nil { + if !isProcessGroupMissing(err) { + m.logger.Warn("failed to terminate agent process group, force killing", + zap.Int("pgid", pid), + zap.Error(err)) + m.forceKillProcessGroupAndWait(done, pid) + } + return + } + if waitForManagerDone(ctx, done, processGroupTerminateGrace) { + m.logger.Info("agent process stopped after process group termination", + zap.Int("pgid", pid)) + m.reapRemainingProcessGroup(ctx, pid) + return + } + m.logger.Warn("agent process did not stop after termination; force killing process group", + zap.Int("pgid", pid)) + m.forceKillProcessGroupAndWait(done, pid) +} + +// processExitGrace returns the initial graceful wait before process-group +// termination. Adapters that declare RequiresProcessKill are known not to exit +// from stdin close, so they keep the short legacy wait. Normal adapters get the +// caller's remaining deadline during backend shutdown, or processDefaultExitGrace +// when the caller did not provide one. +func (m *Manager) processExitGrace(ctx context.Context) time.Duration { + if m.adapter != nil && m.adapter.RequiresProcessKill() { + return processKillRequiredExitGrace + } + if deadline, ok := ctx.Deadline(); ok { + if remaining := time.Until(deadline); remaining > 0 { + return remaining + } + return 0 + } + return processDefaultExitGrace +} + +func waitForManagerDone(ctx context.Context, done <-chan struct{}, timeout time.Duration) bool { + timer := time.NewTimer(timeout) + defer timer.Stop() select { case <-done: - m.logger.Info("agent process stopped gracefully") + return true case <-ctx.Done(): - if m.cmd == nil || m.cmd.Process == nil { + return false + case <-timer.C: + return false + } +} + +func (m *Manager) reapRemainingProcessGroup(ctx context.Context, pid int) { + if pid == 0 || !processGroupAlive(pid) { + return + } + + m.logger.Warn("agent process group still alive after leader exit; terminating", + zap.Int("pgid", pid)) + m.logger.Debug("agent process group SIGTERM requested", + zap.Int("pgid", pid), + zap.String("reason", "leader_exited_with_live_descendants")) + if err := terminateProcessGroup(pid); err != nil { + if isProcessGroupMissing(err) { return } - pid := m.cmd.Process.Pid - m.logger.Warn("force killing agent process group", zap.Int("pgid", pid)) - if err := killProcessGroup(pid); err != nil { + m.logger.Warn("failed to terminate agent process group, force killing", + zap.Int("pgid", pid), + zap.Error(err)) + m.forceKillProcessGroupAndWait(nil, pid) + return + } + + waitCtx, cancel := context.WithTimeout(ctx, processGroupTerminateGrace) + defer cancel() + if waitForProcessGroupExit(waitCtx, pid) { + m.logger.Info("agent process group stopped after termination", + zap.Int("pgid", pid)) + return + } + + m.logger.Warn("agent process group did not stop after termination; force killing", + zap.Int("pgid", pid)) + m.forceKillProcessGroupAndWait(nil, pid) +} + +func (m *Manager) forceKillProcessGroup(pid int) { + m.logger.Debug("agent process group SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "force_kill")) + if err := killProcessGroup(pid); err != nil { + if isProcessGroupMissing(err) { + return + } + if m.cmd != nil && m.cmd.Process != nil { m.logger.Warn("failed to kill agent process group, falling back to single-process kill", zap.Error(err)) + m.logger.Debug("agent process SIGKILL requested", + zap.Int("pid", m.cmd.Process.Pid), + zap.String("reason", "process_group_kill_failed")) if err := m.cmd.Process.Kill(); err != nil { m.logger.Warn("failed to kill agent process", zap.Error(err)) } + return + } + m.logger.Warn("failed to kill agent process group", zap.Error(err)) + } +} + +func (m *Manager) forceKillProcessGroupAndWait(done <-chan struct{}, pid int) { + m.forceKillProcessGroup(pid) + + waitCtx, cancel := context.WithTimeout(context.Background(), processGroupTerminateGrace) + defer cancel() + if done != nil && waitForManagerDone(waitCtx, done, processGroupTerminateGrace) { + m.logger.Info("agent process stopped after force kill", + zap.Int("pgid", pid)) + m.reapRemainingProcessGroup(context.Background(), pid) + return + } + if waitForProcessGroupExit(waitCtx, pid) { + m.logger.Info("agent process group stopped after force kill", + zap.Int("pgid", pid)) + return + } + m.logger.Warn("agent process group still alive after force kill wait", + zap.Int("pgid", pid), + zap.Duration("grace", processGroupTerminateGrace)) +} + +func waitForProcessGroupExit(ctx context.Context, pid int) bool { + if !processGroupAlive(pid) { + return true + } + ticker := time.NewTicker(processGroupPollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return !processGroupAlive(pid) + case <-ticker.C: + if !processGroupAlive(pid) { + return true + } } } } @@ -1412,7 +1625,10 @@ func (m *Manager) waitForExit() { defer m.wg.Done() defer close(m.doneCh) + pid := m.agentPID() err := m.cmd.Wait() + releaseProcessLifecycle(m.processLifecycle) + m.processLifecycle = processLifecycleHandle{} if err != nil { m.exitErr.Store(errorWrapper{err: err}) @@ -1450,6 +1666,9 @@ func (m *Manager) waitForExit() { m.logger.Info("agent process exited successfully") } + if m.Status() != StatusStopping { + m.reapRemainingProcessGroup(context.Background(), pid) + } m.status.Store(StatusStopped) } diff --git a/apps/backend/internal/agentctl/server/process/manager_killgroup_test.go b/apps/backend/internal/agentctl/server/process/manager_killgroup_test.go index 6c31a467b9..9c8e1b8d49 100644 --- a/apps/backend/internal/agentctl/server/process/manager_killgroup_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_killgroup_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/agentctl/server/adapter" "github.com/stretchr/testify/require" ) @@ -24,7 +25,7 @@ import ( // After the fix, killProcessGroup is delivered to the leader's pgid so the // whole tree dies. func TestWaitForProcessExit_KillsProcessGroupOnTimeout(t *testing.T) { - log := newTestLogger(t) + log, observed := newObservedTestLogger(t) pidFile := filepath.Join(t.TempDir(), "child.pid") m := &Manager{ @@ -68,6 +69,8 @@ func TestWaitForProcessExit_KillsProcessGroupOnTimeout(t *testing.T) { cancel() m.waitForProcessExit(ctx) + require.True(t, observedLogsContain(observed, "agent process group SIGKILL requested"), + "shutdown should log the process-group SIGKILL attempt") // Both parent and child must be reaped within a short window. We waited // the parent ourselves; for the child we treat both "process gone" and @@ -82,6 +85,238 @@ func TestWaitForProcessExit_KillsProcessGroupOnTimeout(t *testing.T) { "child process %d should be killed by process-group reap", childPID) } +func TestWaitForProcessExit_ContextCanceledWaitsAfterForceKill(t *testing.T) { + log := newTestLogger(t) + pidFile := filepath.Join(t.TempDir(), "child.pid") + + m := &Manager{ + logger: log, + } + m.cmd = fixtureCmd("sleep-with-child " + pidFile + " 30") + setProcGroup(m.cmd) + require.NoError(t, m.cmd.Start()) + parentPID := m.cmd.Process.Pid + + waitDone := make(chan struct{}) + m.wg.Add(1) + go func() { + defer m.wg.Done() + _ = m.cmd.Wait() + close(waitDone) + }() + t.Cleanup(func() { + _ = killProcessGroup(parentPID) + select { + case <-waitDone: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for fixture parent to exit") + } + }) + + childPID := waitForChildPID(t, pidFile, 5*time.Second) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + m.waitForProcessExit(ctx) + + select { + case <-waitDone: + default: + t.Fatal("waitForProcessExit returned before command wait completed after force kill") + } + require.Eventually(t, func() bool { + return !processAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, + "child process %d should be killed by process-group force kill", childPID) +} + +func TestWaitForProcessExit_ReapsProcessGroupAfterLeaderExit(t *testing.T) { + log := newTestLogger(t) + pidFile := filepath.Join(t.TempDir(), "child.pid") + + m := &Manager{ + logger: log, + } + m.cmd = fixtureCmd("exit-with-child " + pidFile + " 30") + setProcGroup(m.cmd) + require.NoError(t, m.cmd.Start()) + parentPID := m.cmd.Process.Pid + + waitDone := make(chan struct{}) + m.wg.Add(1) + go func() { + defer m.wg.Done() + _ = m.cmd.Wait() + close(waitDone) + }() + t.Cleanup(func() { + _ = killProcessGroup(parentPID) + select { + case <-waitDone: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for fixture parent to exit") + } + }) + + childPID := waitForChildPID(t, pidFile, 5*time.Second) + childPGID, err := syscall.Getpgid(childPID) + require.NoError(t, err) + require.Equal(t, parentPID, childPGID, + "child must remain in the leader's pgid after the leader exits") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + m.waitForProcessExit(ctx) + + require.Eventually(t, func() bool { + return !processAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, + "child process %d should be reaped even after the group leader exits", childPID) +} + +func TestWaitForExit_ReapsProcessGroupAfterNaturalLeaderExit(t *testing.T) { + log := newTestLogger(t) + pidFile := filepath.Join(t.TempDir(), "child.pid") + + m := &Manager{ + logger: log, + updatesCh: make(chan adapter.AgentEvent, 1), + doneCh: make(chan struct{}), + } + m.status.Store(StatusRunning) + m.cmd = fixtureCmd("exit-with-child " + pidFile + " 30") + setProcGroup(m.cmd) + require.NoError(t, m.cmd.Start()) + parentPID := m.cmd.Process.Pid + t.Cleanup(func() { + _ = killProcessGroup(parentPID) + }) + + childPID := waitForChildPID(t, pidFile, 5*time.Second) + m.wg.Add(1) + go m.waitForExit() + + select { + case <-m.doneCh: + case <-time.After(15 * time.Second): + t.Fatal("timed out waiting for process manager waitForExit") + } + require.Eventually(t, func() bool { + return !processAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, + "child process %d should be reaped when the leader exits naturally", childPID) +} + +func TestWaitForProcessExit_TerminatesBeforeParentContextDeadline(t *testing.T) { + log, observed := newObservedTestLogger(t) + + m := &Manager{ + logger: log, + adapter: &stubAdapter{requiresProcessKill: true}, + } + m.cmd = fixtureCmd("sleep 30") + setProcGroup(m.cmd) + require.NoError(t, m.cmd.Start()) + parentPID := m.cmd.Process.Pid + + waitDone := make(chan struct{}) + m.wg.Add(1) + go func() { + defer m.wg.Done() + _ = m.cmd.Wait() + close(waitDone) + }() + t.Cleanup(func() { + _ = killProcessGroup(parentPID) + select { + case <-waitDone: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for fixture parent to exit") + } + }) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + startedAt := time.Now() + m.waitForProcessExit(ctx) + elapsed := time.Since(startedAt) + + require.Less(t, elapsed, 700*time.Millisecond, + "shutdown should terminate a non-exiting agent without spending a full second in local grace") + require.True(t, observedLogsContain(observed, "agent process group SIGTERM requested"), + "shutdown should log the process-group SIGTERM attempt") + require.Eventually(t, func() bool { + return !processAlive(parentPID) + }, 5*time.Second, 50*time.Millisecond, + "agent process %d should be killed after local graceful wait", parentPID) +} + +func TestProcessExitGraceUsesCallerDeadlineForGracefulAdapter(t *testing.T) { + m := &Manager{ + adapter: &stubAdapter{}, + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + grace := m.processExitGrace(ctx) + require.Greater(t, grace, 1500*time.Millisecond) + require.LessOrEqual(t, grace, 2*time.Second) +} + +func TestProcessExitGraceUsesDefaultForGracefulAdapterWithoutDeadline(t *testing.T) { + m := &Manager{ + adapter: &stubAdapter{}, + } + + require.Equal(t, processDefaultExitGrace, m.processExitGrace(context.Background())) +} + +func TestProcessExitGraceUsesShortGraceForKillRequiredAdapter(t *testing.T) { + m := &Manager{ + adapter: &stubAdapter{requiresProcessKill: true}, + } + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + require.Equal(t, processKillRequiredExitGrace, m.processExitGrace(ctx)) +} + +func TestStop_DoesNotReapStaleProcessGroupWhenStatusAlreadyStopped(t *testing.T) { + log, observed := newObservedTestLogger(t) + + m := &Manager{ + logger: log, + } + m.status.Store(StatusStopped) + m.cmd = fixtureCmd("sleep 30") + setProcGroup(m.cmd) + require.NoError(t, m.cmd.Start()) + parentPID := m.cmd.Process.Pid + + waitDone := make(chan struct{}) + go func() { + _ = m.cmd.Wait() + close(waitDone) + }() + t.Cleanup(func() { + _ = killProcessGroup(parentPID) + select { + case <-waitDone: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for fixture parent to exit") + } + }) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + require.NoError(t, m.Stop(ctx)) + require.True(t, processAlive(parentPID), "stopped manager should not signal a cached process PID") + require.False(t, observedLogsContain(observed, "agent process group SIGTERM requested"), + "stopped manager should not attempt process-group termination") +} + // waitForChildPID polls pidFile until it contains a valid PID or timeout // expires. Returns the parsed PID. Fails the test on timeout. func waitForChildPID(t *testing.T, pidFile string, timeout time.Duration) int { diff --git a/apps/backend/internal/agentctl/server/process/manager_test.go b/apps/backend/internal/agentctl/server/process/manager_test.go index d4a3db9595..dbff6a460a 100644 --- a/apps/backend/internal/agentctl/server/process/manager_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_test.go @@ -20,8 +20,9 @@ import ( // stubAdapter is a minimal AgentAdapter for testing the startup sequence. type stubAdapter struct { - connectCalled bool - updatesCh chan adapter.AgentEvent + connectCalled bool + requiresProcessKill bool + updatesCh chan adapter.AgentEvent } func newStubAdapter() *stubAdapter { @@ -50,7 +51,7 @@ func (s *stubAdapter) Close() error { close(s.updatesCh) return nil } -func (s *stubAdapter) RequiresProcessKill() bool { return false } +func (s *stubAdapter) RequiresProcessKill() bool { return s.requiresProcessKill } func TestStartProcessPipes_CreatesAllPipes(t *testing.T) { m := &Manager{ @@ -134,8 +135,21 @@ func TestManager_Start_PipesCreatedBeforeProcessStart(t *testing.T) { // Now start the process — this is the fix: Start() happens after pipes. err = m.cmd.Start() require.NoError(t, err) + waited := false + t.Cleanup(func() { + if waited || m.cmd == nil || m.cmd.Process == nil { + return + } + _ = killProcessGroup(m.cmd.Process.Pid) + _ = m.cmd.Process.Kill() + _ = m.cmd.Wait() + }) assert.NotNil(t, m.cmd.Process, "process should be running") + processLifecycle, err := installProcessLifecycle(m.cmd) + require.NoError(t, err) + defer releaseProcessLifecycle(processLifecycle) + // Adapter connects after process starts. err = stub.Connect(m.stdin, m.stdout) require.NoError(t, err) @@ -144,6 +158,7 @@ func TestManager_Start_PipesCreatedBeforeProcessStart(t *testing.T) { // Clean up: close stdin so cat exits, then wait. _ = m.stdin.Close() _ = m.cmd.Wait() + waited = true } func TestFormatAgentStartError_E2BIGIncludesEnvDiagnostics(t *testing.T) { diff --git a/apps/backend/internal/agentctl/server/process/procattr_linux.go b/apps/backend/internal/agentctl/server/process/procattr_linux.go index b9e5c618d1..a22edd2786 100644 --- a/apps/backend/internal/agentctl/server/process/procattr_linux.go +++ b/apps/backend/internal/agentctl/server/process/procattr_linux.go @@ -3,6 +3,7 @@ package process import ( + "errors" "os/exec" "syscall" ) @@ -18,6 +19,18 @@ func setProcGroup(cmd *exec.Cmd) { } } +func setAgentProcGroup(cmd *exec.Cmd) { + setProcGroup(cmd) +} + +type processLifecycleHandle struct{} + +func installProcessLifecycle(_ *exec.Cmd) (processLifecycleHandle, error) { + return processLifecycleHandle{}, nil +} + +func releaseProcessLifecycle(_ processLifecycleHandle) {} + // killProcessGroup kills the entire process group for the given PID. // Returns nil if successful, or an error if the kill failed. func killProcessGroup(pid int) error { @@ -29,3 +42,15 @@ func killProcessGroup(pid int) error { func terminateProcessGroup(pid int) error { return syscall.Kill(-pid, syscall.SIGTERM) } + +func processGroupAlive(pid int) bool { + if pid <= 0 { + return false + } + err := syscall.Kill(-pid, 0) + return err == nil || errors.Is(err, syscall.EPERM) +} + +func isProcessGroupMissing(err error) bool { + return errors.Is(err, syscall.ESRCH) +} diff --git a/apps/backend/internal/agentctl/server/process/procattr_unix.go b/apps/backend/internal/agentctl/server/process/procattr_unix.go index 66ee87b3d8..cfc2850b03 100644 --- a/apps/backend/internal/agentctl/server/process/procattr_unix.go +++ b/apps/backend/internal/agentctl/server/process/procattr_unix.go @@ -3,6 +3,7 @@ package process import ( + "errors" "os/exec" "syscall" ) @@ -15,6 +16,18 @@ func setProcGroup(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} } +func setAgentProcGroup(cmd *exec.Cmd) { + setProcGroup(cmd) +} + +type processLifecycleHandle struct{} + +func installProcessLifecycle(_ *exec.Cmd) (processLifecycleHandle, error) { + return processLifecycleHandle{}, nil +} + +func releaseProcessLifecycle(_ processLifecycleHandle) {} + // killProcessGroup kills the entire process group for the given PID. // Returns nil if successful, or an error if the kill failed. func killProcessGroup(pid int) error { @@ -26,3 +39,15 @@ func killProcessGroup(pid int) error { func terminateProcessGroup(pid int) error { return syscall.Kill(-pid, syscall.SIGTERM) } + +func processGroupAlive(pid int) bool { + if pid <= 0 { + return false + } + err := syscall.Kill(-pid, 0) + return err == nil || errors.Is(err, syscall.EPERM) +} + +func isProcessGroupMissing(err error) bool { + return errors.Is(err, syscall.ESRCH) +} diff --git a/apps/backend/internal/agentctl/server/process/procattr_windows.go b/apps/backend/internal/agentctl/server/process/procattr_windows.go index c92c2a0591..1d3b85b409 100644 --- a/apps/backend/internal/agentctl/server/process/procattr_windows.go +++ b/apps/backend/internal/agentctl/server/process/procattr_windows.go @@ -3,34 +3,69 @@ package process import ( + "errors" "fmt" "os/exec" "syscall" + + "github.com/kandev/kandev/internal/agentctl/server/winproc" + "golang.org/x/sys/windows" ) // setProcGroup configures the command to run in its own process group. -// On Windows, we use CREATE_NEW_PROCESS_GROUP flag. func setProcGroup(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{ CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, } } +// setAgentProcGroup configures long-running agent processes for process-tree +// cleanup. On Windows, agents start suspended so installProcessLifecycle can +// bind them to a kill-on-close Job Object before they can spawn descendants. +func setAgentProcGroup(cmd *exec.Cmd) { + setProcGroup(cmd) + cmd.SysProcAttr.CreationFlags |= windows.CREATE_SUSPENDED +} + +type processLifecycleHandle struct { + job winproc.KillOnCloseJob +} + +func installProcessLifecycle(cmd *exec.Cmd) (processLifecycleHandle, error) { + job, err := winproc.InstallKillOnCloseJobForSuspendedCommand(cmd) + if err != nil { + return processLifecycleHandle{}, err + } + return processLifecycleHandle{job: job}, nil +} + +func releaseProcessLifecycle(lifecycle processLifecycleHandle) { + _ = lifecycle.job.Close() +} + // killProcessGroup kills the entire process tree for the given PID. // On Windows, we use taskkill with /T flag to kill the process tree. func killProcessGroup(pid int) error { - // taskkill /F /T /PID - // /F = force kill - // /T = kill child processes (tree) - // /PID = process ID - kill := exec.Command("taskkill", "/F", "/T", "/PID", fmt.Sprintf("%d", pid)) - return kill.Run() + return runProcessTaskkill("/F", "/T", "/PID", fmt.Sprintf("%d", pid)) } // terminateProcessGroup sends a graceful shutdown signal to the process tree. // Without /F, taskkill sends WM_CLOSE to console and windowed apps, which is // the closest Windows equivalent of SIGTERM. func terminateProcessGroup(pid int) error { - kill := exec.Command("taskkill", "/T", "/PID", fmt.Sprintf("%d", pid)) - return kill.Run() + return runProcessTaskkill("/T", "/PID", fmt.Sprintf("%d", pid)) +} + +func processGroupAlive(_ int) bool { + // taskkill /T is the authoritative process-tree operation on Windows. + // There is no Unix-style process group to poll here. + return false +} + +func isProcessGroupMissing(err error) bool { + return errors.Is(err, syscall.ESRCH) +} + +func runProcessTaskkill(args ...string) error { + return winproc.RunTaskkill(args...) } diff --git a/apps/backend/internal/agentctl/server/process/procattr_windows_test.go b/apps/backend/internal/agentctl/server/process/procattr_windows_test.go new file mode 100644 index 0000000000..4d824ae11f --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/procattr_windows_test.go @@ -0,0 +1,104 @@ +//go:build windows + +package process + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func TestWindowsSetProcGroupDoesNotSuspendSharedHelpers(t *testing.T) { + cmd := exec.Command("kandev") + setProcGroup(cmd) + + require.NotNil(t, cmd.SysProcAttr) + require.NotZero(t, cmd.SysProcAttr.CreationFlags&syscall.CREATE_NEW_PROCESS_GROUP) + require.Zero(t, cmd.SysProcAttr.CreationFlags&windows.CREATE_SUSPENDED) +} + +func TestWindowsSetAgentProcGroupStartsSuspended(t *testing.T) { + cmd := exec.Command("kandev") + setAgentProcGroup(cmd) + + require.NotNil(t, cmd.SysProcAttr) + require.NotZero(t, cmd.SysProcAttr.CreationFlags&syscall.CREATE_NEW_PROCESS_GROUP) + require.NotZero(t, cmd.SysProcAttr.CreationFlags&windows.CREATE_SUSPENDED) +} + +func TestWindowsProcessLifecycleJobKillsDescendants(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + cmd := fixtureCmd(fmt.Sprintf("delay-then-child %s 200 30", pidFile)) + setAgentProcGroup(cmd) + require.NoError(t, cmd.Start()) + parentPID := cmd.Process.Pid + waited := false + t.Cleanup(func() { + if waited { + return + } + _ = killProcessGroup(parentPID) + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + lifecycle, err := installProcessLifecycle(cmd) + require.NoError(t, err) + childPID := waitForWindowsChildPID(t, pidFile, 5*time.Second) + + releaseProcessLifecycle(lifecycle) + _ = cmd.Wait() + waited = true + + require.Eventually(t, func() bool { + return !windowsProcessAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, + "child process %d should be killed when the job handle is released", childPID) +} + +func TestWindowsProcessLifecycleInstallFailsForInvalidPID(t *testing.T) { + _, err := installProcessLifecycle(&exec.Cmd{Process: &os.Process{Pid: -1}}) + require.Error(t, err) +} + +func waitForWindowsChildPID(t *testing.T, pidFile string, timeout time.Duration) int { + t.Helper() + deadline := time.Now().Add(timeout) + for { + raw, err := os.ReadFile(pidFile) + if err == nil { + pid, perr := strconv.Atoi(strings.TrimSpace(string(raw))) + if perr == nil && pid > 0 { + return pid + } + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for fixture to write child pid to %s", pidFile) + } + time.Sleep(20 * time.Millisecond) + } +} + +func windowsProcessAlive(pid int) bool { + if pid <= 0 { + return false + } + output, err := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %d", pid), "/NH").Output() + if err != nil { + return false + } + text := strings.ToLower(string(output)) + if strings.Contains(text, "no tasks") { + return false + } + return strings.Contains(text, strconv.Itoa(pid)) +} diff --git a/apps/backend/internal/agentctl/server/process/runner.go b/apps/backend/internal/agentctl/server/process/runner.go index 1c2efd1021..3ecc2c42d5 100644 --- a/apps/backend/internal/agentctl/server/process/runner.go +++ b/apps/backend/internal/agentctl/server/process/runner.go @@ -366,17 +366,39 @@ func (r *ProcessRunner) Stop(ctx context.Context, req StopProcessRequest) error // Attempt graceful shutdown, then escalate to force-kill if proc.cmd != nil && proc.cmd.Process != nil { pid := proc.cmd.Process.Pid + info := proc.snapshot(false) + r.logger.Debug("workspace process stop requested", + zap.String("process_id", req.ProcessID), + zap.Int("pid", pid), + zap.String("session_id", info.SessionID), + zap.String("kind", string(info.Kind)), + zap.String("script_name", info.ScriptName), + zap.String("command", info.Command), + zap.String("working_dir", info.WorkingDir)) // Phase 1: Graceful shutdown (SIGTERM on Unix, interrupt on Windows) + r.logger.Debug("workspace process interrupt requested", + zap.String("process_id", req.ProcessID), + zap.Int("pid", pid), + zap.String("signal", fmt.Sprint(os.Interrupt))) _ = proc.cmd.Process.Signal(os.Interrupt) // Wait for graceful exit (2 seconds) or context cancellation select { case <-ctx.Done(): // Context cancelled - force kill immediately + r.logger.Debug("workspace process group SIGKILL requested", + zap.String("process_id", req.ProcessID), + zap.Int("pgid", pid), + zap.String("reason", "context_canceled"), + zap.Error(ctx.Err())) _ = killProcessGroup(pid) case <-time.After(2 * time.Second): // Phase 2: Grace period expired - force kill entire process tree + r.logger.Debug("workspace process group SIGKILL requested", + zap.String("process_id", req.ProcessID), + zap.Int("pgid", pid), + zap.String("reason", "grace_expired")) _ = killProcessGroup(pid) } } @@ -398,6 +420,8 @@ func (r *ProcessRunner) StopAll(ctx context.Context) error { } r.mu.RUnlock() + r.logger.Debug("workspace process stop all requested", zap.Int("processes", len(ids))) + var errs []error for _, id := range ids { if err := r.Stop(ctx, StopProcessRequest{ProcessID: id}); err != nil { diff --git a/apps/backend/internal/agentctl/server/process/runner_test.go b/apps/backend/internal/agentctl/server/process/runner_test.go index a793cfdd53..6ce29c4c34 100644 --- a/apps/backend/internal/agentctl/server/process/runner_test.go +++ b/apps/backend/internal/agentctl/server/process/runner_test.go @@ -7,6 +7,9 @@ import ( "time" "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) func newTestLogger(t *testing.T) *logger.Logger { @@ -20,6 +23,25 @@ func newTestLogger(t *testing.T) *logger.Logger { return log } +func newObservedTestLogger(t *testing.T) (*logger.Logger, *observer.ObservedLogs) { + t.Helper() + core, observed := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("failed to create observed logger: %v", err) + } + return log, observed +} + +func observedLogsContain(logs *observer.ObservedLogs, message string) bool { + for _, entry := range logs.All() { + if entry.Message == message { + return true + } + } + return false +} + func TestRingBufferTrimsOldest(t *testing.T) { buffer := newRingBuffer(10) buffer.append(ProcessOutputChunk{Stream: "stdout", Data: "hello", Timestamp: time.Now()}) // 5 @@ -79,6 +101,55 @@ func TestProcessRunnerCapturesOutput(t *testing.T) { t.Fatal("process output not captured in time") } +func TestProcessRunnerStopLogsSignalAttempts(t *testing.T) { + log, observed := newObservedTestLogger(t) + runner := NewProcessRunner(nil, log, 2*1024*1024) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + cmd, env := fixtureShellExec("sleep 30") + info, err := runner.Start(ctx, StartProcessRequest{ + SessionID: "session-1", + Kind: "dev", + Command: cmd, + Env: env, + WorkingDir: "", + }) + if err != nil { + t.Fatalf("failed to start process: %v", err) + } + t.Cleanup(func() { + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cleanupCancel() + _ = runner.Stop(cleanupCtx, StopProcessRequest{ProcessID: info.ID}) + }) + + stopCtx, stopCancel := context.WithCancel(context.Background()) + stopCancel() + if err := runner.Stop(stopCtx, StopProcessRequest{ProcessID: info.ID}); err != nil { + t.Fatalf("failed to stop process: %v", err) + } + + for _, message := range []string{ + "workspace process stop requested", + "workspace process interrupt requested", + "workspace process group SIGKILL requested", + } { + if !observedLogsContain(observed, message) { + t.Fatalf("expected debug log %q, got %#v", message, observed.All()) + } + } + + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, ok := runner.Get(info.ID, false); !ok { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatal("process was not removed after stop") +} + func TestStripANSI(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/agentctl/server/process/testmain_test.go b/apps/backend/internal/agentctl/server/process/testmain_test.go index 9b3121d3c3..a35e8f9182 100644 --- a/apps/backend/internal/agentctl/server/process/testmain_test.go +++ b/apps/backend/internal/agentctl/server/process/testmain_test.go @@ -42,6 +42,8 @@ func TestMain(m *testing.M) { // echo — print args joined by spaces, plus newline // cat — copy stdin to stdout until EOF // echo-then-sleep — print msg, then sleep +// delay-then-child +// — wait, spawn a sleeping child, write its PID, then sleep // // New commands can be added here as tests need them; the goal is to keep the // surface tiny so the helper stays inspectable. @@ -112,6 +114,61 @@ func runFixture(spec string) { os.Exit(2) } time.Sleep(time.Duration(secs) * time.Second) + case "delay-then-child": + // Waits briefly before spawning the child so platform lifecycle hooks + // installed immediately after parent Start() can be applied before the + // child inherits them. Used by Windows Job Object tests. + if len(parts) != 4 { + fmt.Fprintln(os.Stderr, "fixture: delay-then-child takes 3 args") + os.Exit(2) + } + pidFile := parts[1] + delayMS, err := strconv.Atoi(parts[2]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: bad delay %q\n", parts[2]) + os.Exit(2) + } + secs, err := strconv.Atoi(parts[3]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: bad seconds %q\n", parts[3]) + os.Exit(2) + } + time.Sleep(time.Duration(delayMS) * time.Millisecond) + childCmd := exec.Command(os.Args[0]) + childCmd.Env = append(os.Environ(), kandevTestFixtureEnv+"=sleep "+strconv.Itoa(secs)) + if err := childCmd.Start(); err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: spawn child: %v\n", err) + os.Exit(2) + } + if err := os.WriteFile(pidFile, []byte(strconv.Itoa(childCmd.Process.Pid)), 0o600); err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: write pidfile: %v\n", err) + os.Exit(2) + } + time.Sleep(time.Duration(secs) * time.Second) + case "exit-with-child": + // Forks a sleeping child, writes its PID, then exits immediately. + // This models wrappers such as npx/node that can exit while a native + // descendant remains alive in the inherited process group. + if len(parts) != 3 { + fmt.Fprintln(os.Stderr, "fixture: exit-with-child takes 2 args") + os.Exit(2) + } + pidFile := parts[1] + secs, err := strconv.Atoi(parts[2]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: exit-with-child: bad seconds %q\n", parts[2]) + os.Exit(2) + } + childCmd := exec.Command(os.Args[0]) + childCmd.Env = append(os.Environ(), kandevTestFixtureEnv+"=sleep "+strconv.Itoa(secs)) + if err := childCmd.Start(); err != nil { + fmt.Fprintf(os.Stderr, "fixture: exit-with-child: spawn child: %v\n", err) + os.Exit(2) + } + if err := os.WriteFile(pidFile, []byte(strconv.Itoa(childCmd.Process.Pid)), 0o600); err != nil { + fmt.Fprintf(os.Stderr, "fixture: exit-with-child: write pidfile: %v\n", err) + os.Exit(2) + } default: fmt.Fprintf(os.Stderr, "fixture: unknown command %q\n", parts[0]) os.Exit(2) diff --git a/apps/backend/internal/agentctl/server/process/vscode.go b/apps/backend/internal/agentctl/server/process/vscode.go index b28b10963a..b8b882b58d 100644 --- a/apps/backend/internal/agentctl/server/process/vscode.go +++ b/apps/backend/internal/agentctl/server/process/vscode.go @@ -334,6 +334,9 @@ func (v *VscodeManager) Stop(ctx context.Context) error { logChildProcesses(v.logger, pid) // Phase 1: Graceful SIGTERM to the entire process group. + v.logger.Debug("code-server process group SIGTERM requested", + zap.Int("pgid", pid), + zap.String("reason", "stop_requested")) if err := terminateProcessGroup(pid); err != nil { v.logger.Warn("failed to send SIGTERM to code-server group", zap.Int("pgid", pid), zap.Error(err)) @@ -353,6 +356,9 @@ func (v *VscodeManager) Stop(ctx context.Context) error { zap.Int("pgid", pid)) } + v.logger.Debug("code-server process group SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "grace_expired_or_context_canceled")) if err := killProcessGroup(pid); err != nil { v.logger.Warn("failed to force kill code-server group", zap.Int("pgid", pid), zap.Error(err)) diff --git a/apps/backend/internal/agentctl/server/shell/process_group_unix.go b/apps/backend/internal/agentctl/server/shell/process_group_unix.go new file mode 100644 index 0000000000..a405ebe0f9 --- /dev/null +++ b/apps/backend/internal/agentctl/server/shell/process_group_unix.go @@ -0,0 +1,28 @@ +//go:build !windows + +package shell + +import ( + "errors" + "os" + "os/exec" + "syscall" +) + +func configureShellProcess(_ *exec.Cmd) {} + +func killShellProcessGroup(p *os.Process) error { + if p == nil { + return nil + } + if err := syscall.Kill(-p.Pid, syscall.SIGKILL); err != nil { + if errors.Is(err, syscall.ESRCH) { + return nil + } + if killErr := p.Kill(); killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + return errors.Join(err, killErr) + } + return err + } + return nil +} diff --git a/apps/backend/internal/agentctl/server/shell/process_group_windows.go b/apps/backend/internal/agentctl/server/shell/process_group_windows.go new file mode 100644 index 0000000000..9ba779aa90 --- /dev/null +++ b/apps/backend/internal/agentctl/server/shell/process_group_windows.go @@ -0,0 +1,43 @@ +//go:build windows + +package shell + +import ( + "errors" + "fmt" + "os" + "os/exec" + "strings" + "syscall" +) + +func configureShellProcess(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{ + CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, + } +} + +func killShellProcessGroup(p *os.Process) error { + if p == nil { + return nil + } + if err := runShellTaskkill("/F", "/T", "/PID", fmt.Sprintf("%d", p.Pid)); err != nil { + if killErr := p.Kill(); killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + return errors.Join(err, killErr) + } + return err + } + return nil +} + +func runShellTaskkill(args ...string) error { + output, err := exec.Command("taskkill", args...).CombinedOutput() + if err == nil { + return nil + } + msg := strings.TrimSpace(string(output)) + if msg == "" { + return err + } + return fmt.Errorf("%w: %s", err, msg) +} diff --git a/apps/backend/internal/agentctl/server/shell/session.go b/apps/backend/internal/agentctl/server/shell/session.go index 21a86346fa..6a62bb1e2d 100644 --- a/apps/backend/internal/agentctl/server/shell/session.go +++ b/apps/backend/internal/agentctl/server/shell/session.go @@ -51,6 +51,7 @@ type Session struct { // Maximum size of output buffer (16KB should be enough for recent history) const maxOutputBufferSize = 16 * 1024 +const shellStopGrace = 250 * time.Millisecond // Config holds shell session configuration type Config struct { @@ -166,6 +167,7 @@ func (s *Session) start(cfg Config) error { s.cmd = exec.Command(s.shell, s.shellArgs...) s.cmd.Dir = cfg.WorkDir s.cmd.Env = buildShellEnv(cfg.WorkDir) + configureShellProcess(s.cmd) // Start with PTY var err error @@ -206,13 +208,26 @@ func (s *Session) Stop() error { s.stopping = true // Mark as stopping to prevent respawn s.mu.Unlock() - s.logger.Info("stopping shell session") + pid := 0 + if s.cmd != nil && s.cmd.Process != nil { + pid = s.cmd.Process.Pid + } + s.logger.Info("stopping shell session", + zap.String("shell", s.shell), + zap.String("cwd", s.workDir), + zap.Int("pid", pid), + zap.Duration("grace", shellStopGrace)) + s.logger.Debug("shell session stop requested", + zap.Int("pid", pid), + zap.String("shell", s.shell), + zap.String("cwd", s.workDir)) // Signal stop close(s.stopCh) // Close PTY (sends SIGHUP to process on Unix) if s.pty != nil { + s.logger.Debug("shell session PTY close requested", zap.Int("pid", pid)) _ = s.pty.Close() } @@ -220,16 +235,32 @@ func (s *Session) Stop() error { select { case <-s.doneCh: s.logger.Info("shell session stopped gracefully") - case <-time.After(5 * time.Second): - s.logger.Warn("shell session stop timeout, force killing") - if s.cmd != nil && s.cmd.Process != nil { - _ = s.cmd.Process.Kill() - } + s.cleanupShellProcessGroup(pid, "leader_exited") + case <-time.After(shellStopGrace): + s.logger.Warn("shell session stop timeout, force killing", + zap.Int("pid", pid), + zap.Duration("grace", shellStopGrace)) + s.cleanupShellProcessGroup(pid, "stop_timeout") } return nil } +func (s *Session) cleanupShellProcessGroup(pid int, reason string) { + if s.cmd == nil || s.cmd.Process == nil { + return + } + s.logger.Debug("shell session process group cleanup requested", + zap.Int("pid", pid), + zap.String("reason", reason)) + if err := killShellProcessGroup(s.cmd.Process); err != nil { + s.logger.Debug("shell session process group cleanup failed", + zap.Int("pid", pid), + zap.String("reason", reason), + zap.Error(err)) + } +} + // Write sends input to the shell func (s *Session) Write(data []byte) (int, error) { s.mu.RLock() diff --git a/apps/backend/internal/agentctl/server/shell/session_stop_unix_test.go b/apps/backend/internal/agentctl/server/shell/session_stop_unix_test.go new file mode 100644 index 0000000000..49aedca4fe --- /dev/null +++ b/apps/backend/internal/agentctl/server/shell/session_stop_unix_test.go @@ -0,0 +1,101 @@ +//go:build !windows + +package shell + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestStopKillsShellProcessGroupOnTimeout(t *testing.T) { + sh, err := exec.LookPath("sh") + require.NoError(t, err) + + pidFile := filepath.Join(t.TempDir(), "child.pid") + script := fmt.Sprintf("sleep 30 & echo $! > %s; trap '' HUP TERM; wait", shellQuote(pidFile)) + session, err := NewSession(Config{ + WorkDir: t.TempDir(), + ShellCommand: sh, + ShellArgs: []string{"-c", script}, + }, newTestLogger()) + require.NoError(t, err) + + childPID := waitForShellChildPID(t, pidFile) + require.NoError(t, session.Stop()) + require.Eventually(t, func() bool { + return !shellProcessAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, "shell child process %d should be killed", childPID) +} + +func TestStopKillsShellProcessGroupAfterLeaderExits(t *testing.T) { + sh, err := exec.LookPath("sh") + require.NoError(t, err) + + pidFile := filepath.Join(t.TempDir(), "child.pid") + script := fmt.Sprintf("trap 'exit 0' HUP TERM; (trap '' HUP TERM; sleep 30) & echo $! > %s; wait", shellQuote(pidFile)) + session, err := NewSession(Config{ + WorkDir: t.TempDir(), + ShellCommand: sh, + ShellArgs: []string{"-c", script}, + }, newTestLogger()) + require.NoError(t, err) + + childPID := waitForShellChildPID(t, pidFile) + require.NoError(t, session.Stop()) + require.Eventually(t, func() bool { + return !shellProcessAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, "shell child process %d should be killed after shell leader exits", childPID) +} + +func waitForShellChildPID(t *testing.T, pidFile string) int { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + raw, err := os.ReadFile(pidFile) + if err == nil { + pid, perr := strconv.Atoi(strings.TrimSpace(string(raw))) + if perr == nil && pid > 0 { + return pid + } + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for shell child pid file %s", pidFile) + return 0 +} + +func shellProcessAlive(pid int) bool { + if pid <= 0 { + return false + } + if _, err := os.Stat(fmt.Sprintf("/proc/%d", pid)); err == nil { + raw, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return false + } + stat := string(raw) + idx := strings.LastIndex(stat, ") ") + if idx == -1 || idx+2 >= len(stat) { + return false + } + return stat[idx+2] != 'Z' + } + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + return proc.Signal(syscall.Signal(0)) == nil +} + +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" +} diff --git a/apps/backend/internal/agentctl/server/shell/session_test.go b/apps/backend/internal/agentctl/server/shell/session_test.go index aef37eb358..132d2b508d 100644 --- a/apps/backend/internal/agentctl/server/shell/session_test.go +++ b/apps/backend/internal/agentctl/server/shell/session_test.go @@ -206,6 +206,26 @@ func TestSessionStop(t *testing.T) { } } +func TestSessionStopTimeoutIsShortForShutdown(t *testing.T) { + log := newTestLogger() + session := &Session{ + logger: log, + running: true, + stopCh: make(chan struct{}), + doneCh: make(chan struct{}), + } + + start := time.Now() + if err := session.Stop(); err != nil { + t.Errorf("Stop failed: %v", err) + } + elapsed := time.Since(start) + + if elapsed > 700*time.Millisecond { + t.Fatalf("Stop took %s, want less than 700ms for shutdown fallback", elapsed) + } +} + // TestSessionWrite tests writing to the shell func TestSessionWrite(t *testing.T) { if runtime.GOOS == "windows" { diff --git a/apps/backend/internal/agentctl/server/utility/acp_command.go b/apps/backend/internal/agentctl/server/utility/acp_command.go new file mode 100644 index 0000000000..f7956906d4 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_command.go @@ -0,0 +1,165 @@ +package utility + +import ( + "context" + "os" + "os/exec" + "time" + + "go.uber.org/zap" +) + +func configureACPCommand(cmd *exec.Cmd, log *zap.Logger) { + log = acpCommandLogger(log) + setACPCommandProcAttr(cmd) + cmd.Cancel = func() error { + if cmd.Process == nil { + return os.ErrProcessDone + } + log.Debug("ACP command process group SIGTERM requested", + zap.Int("pgid", cmd.Process.Pid), + zap.String("reason", "context_cancel")) + if err := terminateACPProcessGroup(cmd.Process.Pid); err != nil { + if acpProcessGroupMissing(err) { + return os.ErrProcessDone + } + log.Debug("ACP command process group SIGTERM failed", + zap.Int("pgid", cmd.Process.Pid), + zap.String("reason", "context_cancel"), + zap.Error(err)) + return err + } + return nil + } +} + +func cleanupACPCommand(ctx context.Context, cmd *exec.Cmd, lifecycle acpCommandLifecycleHandle, log *zap.Logger) { + log = acpCommandLogger(log) + defer releaseACPCommandLifecycle(lifecycle) + if cmd == nil { + log.Debug("ACP command cleanup skipped", zap.String("reason", "nil_command")) + return + } + if cmd.Process == nil { + log.Debug("ACP command cleanup skipped", zap.String("reason", "nil_process")) + return + } + pid := cmd.Process.Pid + cleanupCtx, cancel := context.WithTimeout( + context.WithoutCancel(ctx), + 2*acpCommandTerminateGrace+2*acpCommandForceKillGrace, + ) + defer cancel() + log.Debug("ACP command cleanup requested", + zap.Int("pid", pid), + zap.String("path", cmd.Path), + zap.Strings("args", cmd.Args)) + log.Debug("ACP command process group SIGTERM requested", + zap.Int("pgid", pid), + zap.String("reason", "cleanup")) + if err := terminateACPProcessGroup(pid); err != nil && !acpProcessGroupMissing(err) { + log.Debug("ACP command process group SIGTERM failed", + zap.Int("pgid", pid), + zap.String("reason", "cleanup"), + zap.Error(err)) + } + + waitCh := make(chan error, 1) + go func() { + waitCh <- cmd.Wait() + }() + + if waitForACPCommand(cleanupCtx, waitCh, acpCommandTerminateGrace) { + log.Debug("ACP command exited after SIGTERM", zap.Int("pid", pid)) + reapACPProcessGroup(cleanupCtx, pid, log) + return + } + + log.Debug("ACP command process group SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "cleanup_timeout")) + if err := killACPProcessGroup(pid); err != nil && !acpProcessGroupMissing(err) { + log.Debug("ACP command process group SIGKILL failed; killing process", + zap.Int("pgid", pid), + zap.Int("pid", pid), + zap.Error(err)) + log.Debug("ACP command process SIGKILL requested", + zap.Int("pid", pid), + zap.String("reason", "process_group_kill_failed")) + _ = cmd.Process.Kill() + } + _ = waitForACPCommand(cleanupCtx, waitCh, acpCommandForceKillGrace) + reapACPProcessGroup(cleanupCtx, pid, log) +} + +func waitForACPCommand(ctx context.Context, waitCh <-chan error, timeout time.Duration) bool { + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-waitCh: + return true + case <-ctx.Done(): + return false + case <-timer.C: + return false + } +} + +func reapACPProcessGroup(ctx context.Context, pid int, log *zap.Logger) { + log = acpCommandLogger(log) + if !acpProcessGroupAlive(pid) { + return + } + log.Debug("ACP command process group still alive; SIGTERM requested", + zap.Int("pgid", pid), + zap.String("reason", "reap_descendants")) + if err := terminateACPProcessGroup(pid); err != nil && !acpProcessGroupMissing(err) { + log.Debug("ACP command process group SIGTERM failed", + zap.Int("pgid", pid), + zap.String("reason", "reap_descendants"), + zap.Error(err)) + } + waitForACPProcessGroupExit(ctx, pid, acpCommandTerminateGrace) + if !acpProcessGroupAlive(pid) { + return + } + log.Debug("ACP command process group SIGKILL requested", + zap.Int("pgid", pid), + zap.String("reason", "reap_descendants_timeout")) + if err := killACPProcessGroup(pid); err != nil && !acpProcessGroupMissing(err) { + log.Debug("ACP command process group SIGKILL failed", + zap.Int("pgid", pid), + zap.String("reason", "reap_descendants_timeout"), + zap.Error(err)) + } + waitForACPProcessGroupExit(ctx, pid, acpCommandForceKillGrace) +} + +func acpCommandLogger(log *zap.Logger) *zap.Logger { + if log == nil { + return zap.NewNop() + } + return log +} + +func waitForACPProcessGroupExit(ctx context.Context, pid int, timeout time.Duration) bool { + if !acpProcessGroupAlive(pid) { + return true + } + timer := time.NewTimer(timeout) + defer timer.Stop() + ticker := time.NewTicker(acpCommandPollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return false + case <-timer.C: + return false + case <-ticker.C: + if !acpProcessGroupAlive(pid) { + return true + } + } + } +} diff --git a/apps/backend/internal/agentctl/server/utility/acp_command_linux.go b/apps/backend/internal/agentctl/server/utility/acp_command_linux.go new file mode 100644 index 0000000000..6810cb2323 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_command_linux.go @@ -0,0 +1,44 @@ +//go:build linux + +package utility + +import ( + "errors" + "os/exec" + "syscall" +) + +func setACPCommandProcAttr(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{ + Setpgid: true, + Pdeathsig: syscall.SIGTERM, + } +} + +type acpCommandLifecycleHandle struct{} + +func installACPCommandLifecycle(_ *exec.Cmd) (acpCommandLifecycleHandle, error) { + return acpCommandLifecycleHandle{}, nil +} + +func releaseACPCommandLifecycle(_ acpCommandLifecycleHandle) {} + +func terminateACPProcessGroup(pid int) error { + return syscall.Kill(-pid, syscall.SIGTERM) +} + +func killACPProcessGroup(pid int) error { + return syscall.Kill(-pid, syscall.SIGKILL) +} + +func acpProcessGroupAlive(pid int) bool { + if pid <= 0 { + return false + } + err := syscall.Kill(-pid, 0) + return err == nil || errors.Is(err, syscall.EPERM) +} + +func acpProcessGroupMissing(err error) bool { + return errors.Is(err, syscall.ESRCH) +} diff --git a/apps/backend/internal/agentctl/server/utility/acp_command_unix.go b/apps/backend/internal/agentctl/server/utility/acp_command_unix.go new file mode 100644 index 0000000000..c40f8f5360 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_command_unix.go @@ -0,0 +1,41 @@ +//go:build unix && !linux + +package utility + +import ( + "errors" + "os/exec" + "syscall" +) + +func setACPCommandProcAttr(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +type acpCommandLifecycleHandle struct{} + +func installACPCommandLifecycle(_ *exec.Cmd) (acpCommandLifecycleHandle, error) { + return acpCommandLifecycleHandle{}, nil +} + +func releaseACPCommandLifecycle(_ acpCommandLifecycleHandle) {} + +func terminateACPProcessGroup(pid int) error { + return syscall.Kill(-pid, syscall.SIGTERM) +} + +func killACPProcessGroup(pid int) error { + return syscall.Kill(-pid, syscall.SIGKILL) +} + +func acpProcessGroupAlive(pid int) bool { + if pid <= 0 { + return false + } + err := syscall.Kill(-pid, 0) + return err == nil || errors.Is(err, syscall.EPERM) +} + +func acpProcessGroupMissing(err error) bool { + return errors.Is(err, syscall.ESRCH) +} diff --git a/apps/backend/internal/agentctl/server/utility/acp_command_windows.go b/apps/backend/internal/agentctl/server/utility/acp_command_windows.go new file mode 100644 index 0000000000..f1d04e0df4 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_command_windows.go @@ -0,0 +1,76 @@ +//go:build windows + +package utility + +import ( + "fmt" + "os/exec" + "strconv" + "strings" + "syscall" + + "github.com/kandev/kandev/internal/agentctl/server/winproc" + "golang.org/x/sys/windows" +) + +// setACPCommandProcAttr starts ACP utility commands suspended so the cleanup +// lifecycle can attach a kill-on-close Job Object before the command runs. +func setACPCommandProcAttr(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{ + CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP | windows.CREATE_SUSPENDED, + } +} + +type acpCommandLifecycleHandle struct { + job winproc.KillOnCloseJob +} + +func installACPCommandLifecycle(cmd *exec.Cmd) (acpCommandLifecycleHandle, error) { + job, err := winproc.InstallKillOnCloseJobForSuspendedCommand(cmd) + if err != nil { + return acpCommandLifecycleHandle{}, err + } + return acpCommandLifecycleHandle{job: job}, nil +} + +func releaseACPCommandLifecycle(lifecycle acpCommandLifecycleHandle) { + _ = lifecycle.job.Close() +} + +func terminateACPProcessGroup(pid int) error { + return runTaskkill("/T", "/PID", fmt.Sprintf("%d", pid)) +} + +func killACPProcessGroup(pid int) error { + return runTaskkill("/F", "/T", "/PID", fmt.Sprintf("%d", pid)) +} + +func runTaskkill(args ...string) error { + return winproc.RunTaskkill(args...) +} + +func acpProcessGroupAlive(pid int) bool { + if pid <= 0 { + return false + } + output, err := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %d", pid), "/NH").Output() + if err != nil { + return false + } + text := strings.ToLower(string(output)) + if strings.Contains(text, "no tasks") { + return false + } + return strings.Contains(text, strconv.Itoa(pid)) +} + +func acpProcessGroupMissing(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "not found") || + strings.Contains(msg, "not be found") || + strings.Contains(msg, "no running instance") || + strings.Contains(msg, "not running") +} diff --git a/apps/backend/internal/agentctl/server/utility/acp_command_windows_test.go b/apps/backend/internal/agentctl/server/utility/acp_command_windows_test.go new file mode 100644 index 0000000000..eaad8ba388 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_command_windows_test.go @@ -0,0 +1,151 @@ +//go:build windows + +package utility + +import ( + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const acpCommandTestFixtureEnv = "KANDEV_ACP_COMMAND_TEST_FIXTURE" + +func TestMain(m *testing.M) { + if spec := os.Getenv(acpCommandTestFixtureEnv); spec != "" { + runACPCommandTestFixture(spec) + return + } + os.Exit(m.Run()) +} + +func TestWindowsACPCommandLifecycleJobKillsDescendants(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + cmd := exec.Command(os.Args[0]) + cmd.Env = append(os.Environ(), acpCommandTestFixtureEnv+"="+fmt.Sprintf("delay-then-child %s 200 30", pidFile)) + setACPCommandProcAttr(cmd) + require.NoError(t, cmd.Start()) + parentPID := cmd.Process.Pid + waited := false + t.Cleanup(func() { + if waited { + return + } + _ = killACPProcessGroup(parentPID) + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + lifecycle, err := installACPCommandLifecycle(cmd) + require.NoError(t, err) + childPID := waitForACPCommandChildPID(t, pidFile, 5*time.Second) + + releaseACPCommandLifecycle(lifecycle) + _ = cmd.Wait() + waited = true + + require.Eventually(t, func() bool { + return !acpCommandWindowsProcessAlive(childPID) + }, 5*time.Second, 50*time.Millisecond, + "child process %d should be killed when the job handle is released", childPID) +} + +func TestWindowsACPCommandLifecycleInstallFailsForInvalidPID(t *testing.T) { + _, err := installACPCommandLifecycle(&exec.Cmd{Process: &os.Process{Pid: -1}}) + require.Error(t, err) +} + +func runACPCommandTestFixture(spec string) { + parts := strings.Fields(spec) + if len(parts) == 0 { + fmt.Fprintln(os.Stderr, "fixture: empty spec") + os.Exit(2) + } + switch parts[0] { + case "sleep": + if len(parts) != 2 { + fmt.Fprintln(os.Stderr, "fixture: sleep takes 1 arg") + os.Exit(2) + } + secs, err := strconv.Atoi(parts[1]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: sleep: bad seconds %q\n", parts[1]) + os.Exit(2) + } + time.Sleep(time.Duration(secs) * time.Second) + case "cat": + _, _ = io.Copy(os.Stdout, os.Stdin) + case "delay-then-child": + if len(parts) != 4 { + fmt.Fprintln(os.Stderr, "fixture: delay-then-child takes 3 args") + os.Exit(2) + } + pidFile := parts[1] + delayMS, err := strconv.Atoi(parts[2]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: bad delay %q\n", parts[2]) + os.Exit(2) + } + secs, err := strconv.Atoi(parts[3]) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: bad seconds %q\n", parts[3]) + os.Exit(2) + } + time.Sleep(time.Duration(delayMS) * time.Millisecond) + childCmd := exec.Command(os.Args[0]) + childCmd.Env = append(os.Environ(), acpCommandTestFixtureEnv+"=sleep "+strconv.Itoa(secs)) + if err := childCmd.Start(); err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: spawn child: %v\n", err) + os.Exit(2) + } + if err := os.WriteFile(pidFile, []byte(strconv.Itoa(childCmd.Process.Pid)), 0o600); err != nil { + fmt.Fprintf(os.Stderr, "fixture: delay-then-child: write pidfile: %v\n", err) + os.Exit(2) + } + time.Sleep(time.Duration(secs) * time.Second) + default: + fmt.Fprintf(os.Stderr, "fixture: unknown command %q\n", parts[0]) + os.Exit(2) + } + os.Exit(0) +} + +func waitForACPCommandChildPID(t *testing.T, pidFile string, timeout time.Duration) int { + t.Helper() + deadline := time.Now().Add(timeout) + for { + raw, err := os.ReadFile(pidFile) + if err == nil { + pid, perr := strconv.Atoi(strings.TrimSpace(string(raw))) + if perr == nil && pid > 0 { + return pid + } + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for fixture to write child pid to %s", pidFile) + } + time.Sleep(20 * time.Millisecond) + } +} + +func acpCommandWindowsProcessAlive(pid int) bool { + if pid <= 0 { + return false + } + output, err := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %d", pid), "/NH").Output() + if err != nil { + return false + } + text := strings.ToLower(string(output)) + if strings.Contains(text, "no tasks") { + return false + } + return strings.Contains(text, strconv.Itoa(pid)) +} diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor.go b/apps/backend/internal/agentctl/server/utility/acp_executor.go index 389e422d4f..9a9a8a04a7 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_executor.go +++ b/apps/backend/internal/agentctl/server/utility/acp_executor.go @@ -23,6 +23,10 @@ import ( const ( openCodeCommand = "opencode" openCodeACPSubcommand = "acp" + + acpCommandTerminateGrace = 250 * time.Millisecond + acpCommandForceKillGrace = 500 * time.Millisecond + acpCommandPollInterval = 25 * time.Millisecond ) // ACPInferenceExecutor executes one-shot prompts using the ACP protocol. @@ -72,6 +76,7 @@ func (e *ACPInferenceExecutor) Execute(ctx context.Context, req *PromptRequest) //nolint:gosec // resolvedCmd is from a hard-coded allow-list; args[1:] are CLI flags cmd := exec.CommandContext(ctx, resolvedCmd, args[1:]...) cmd.Dir = workDir + configureACPCommand(cmd, e.logger) stdin, err := cmd.StdinPipe() if err != nil { @@ -86,12 +91,13 @@ func (e *ACPInferenceExecutor) Execute(ctx context.Context, req *PromptRequest) if err := cmd.Start(); err != nil { return &PromptResponse{Success: false, Error: fmt.Sprintf("start: %v", err)}, nil } + lifecycle, err := installACPCommandLifecycle(cmd) + if err != nil { + e.logger.Warn("failed to install ACP command lifecycle; falling back to process-tree cleanup", + zap.Error(err)) + } - // Ensure process cleanup - defer func() { - _ = cmd.Process.Kill() - _ = cmd.Wait() - }() + defer cleanupACPCommand(ctx, cmd, lifecycle, e.logger) // Execute ACP protocol mcpServers, dropped := toACPMcpServers(req.MCPServers) @@ -316,6 +322,7 @@ func (e *ACPInferenceExecutor) Probe(ctx context.Context, req *ProbeRequest) (*P //nolint:gosec // resolvedCmd is from a hard-coded allow-list; args[1:] are CLI flags cmd := exec.CommandContext(ctx, resolvedCmd, args[1:]...) cmd.Dir = workDir + configureACPCommand(cmd, e.logger) stdin, err := cmd.StdinPipe() if err != nil { @@ -328,10 +335,12 @@ func (e *ACPInferenceExecutor) Probe(ctx context.Context, req *ProbeRequest) (*P if err := cmd.Start(); err != nil { return &ProbeResponse{Success: false, Error: fmt.Sprintf("start: %v", err)}, nil } - defer func() { - _ = cmd.Process.Kill() - _ = cmd.Wait() - }() + lifecycle, err := installACPCommandLifecycle(cmd) + if err != nil { + e.logger.Warn("failed to install ACP command lifecycle; falling back to process-tree cleanup", + zap.Error(err)) + } + defer cleanupACPCommand(ctx, cmd, lifecycle, e.logger) resp, err := e.probeACPSession(ctx, stdin, stdout, workDir) if err != nil { diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor_process_unix_test.go b/apps/backend/internal/agentctl/server/utility/acp_executor_process_unix_test.go new file mode 100644 index 0000000000..40ebdd8e58 --- /dev/null +++ b/apps/backend/internal/agentctl/server/utility/acp_executor_process_unix_test.go @@ -0,0 +1,208 @@ +//go:build linux + +package utility + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +func TestProbeCleansUpDescendantProcessOnTimeout(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "child") + installLeakyMockAgent(t, tmp, marker) + t.Setenv("PATH", tmp+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("ACP_LEAK_MARKER", marker) + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + core, observed := observer.New(zapcore.DebugLevel) + executor := NewACPInferenceExecutor(zap.New(core)) + resp, err := executor.Probe(ctx, &ProbeRequest{ + AgentID: "mock-agent", + InferenceConfig: &InferenceConfigDTO{ + Command: []string{"mock-agent"}, + WorkDir: tmp, + }, + }) + if err != nil { + t.Fatalf("Probe returned error: %v", err) + } + if resp.Success { + t.Fatalf("Probe unexpectedly succeeded") + } + + pid := readPID(t, marker+".pid") + t.Cleanup(func() { + if processRunning(pid) { + _ = syscall.Kill(pid, syscall.SIGKILL) + } + }) + + waitUntil(t, 2*time.Second, func() bool { + return !processRunning(pid) + }, "descendant process %d was still running after Probe returned", pid) + + for _, message := range []string{ + "ACP command process group SIGTERM requested", + "ACP command process group SIGKILL requested", + } { + if !zapLogsContain(observed, message) { + t.Fatalf("expected debug log %q, got %#v", message, observed.All()) + } + } +} + +func TestWaitForACPProcessGroupExitHonorsContextCancellation(t *testing.T) { + cmd := exec.Command("sleep", "30") + setACPCommandProcAttr(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start sleep: %v", err) + } + pid := cmd.Process.Pid + t.Cleanup(func() { + _ = killACPProcessGroup(pid) + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case <-done: + case <-time.After(time.Second): + _ = cmd.Process.Kill() + } + }) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + if waitForACPProcessGroupExit(ctx, pid, 2*time.Second) { + t.Fatal("wait unexpectedly reported process group exit") + } + if elapsed := time.Since(start); elapsed > 100*time.Millisecond { + t.Fatalf("wait ignored canceled context; elapsed=%s", elapsed) + } +} + +func TestCleanupACPCommandWaitsAfterRequestContextCancellation(t *testing.T) { + cmd := exec.Command("sleep", "30") + setACPCommandProcAttr(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start sleep: %v", err) + } + pid := cmd.Process.Pid + waited := false + t.Cleanup(func() { + if waited { + return + } + _ = killACPProcessGroup(pid) + _ = cmd.Wait() + }) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + core, observed := observer.New(zapcore.DebugLevel) + + cleanupACPCommand(ctx, cmd, acpCommandLifecycleHandle{}, zap.New(core)) + waited = true + + if !zapLogsContain(observed, "ACP command exited after SIGTERM") { + t.Fatalf("cleanup did not wait for SIGTERM exit after canceled context; logs=%#v", observed.All()) + } + if processRunning(pid) { + t.Fatalf("process %d still running after cleanup", pid) + } +} + +func zapLogsContain(logs *observer.ObservedLogs, message string) bool { + for _, entry := range logs.All() { + if entry.Message == message { + return true + } + } + return false +} + +func installLeakyMockAgent(t *testing.T, dir, marker string) { + t.Helper() + + agentPath := filepath.Join(dir, "mock-agent") + childPath := filepath.Join(dir, "mock-agent-child") + writeExecutable(t, agentPath, fmt.Sprintf(`#!/bin/sh +"%s" "$ACP_LEAK_MARKER" & +echo "$!" > "$ACP_LEAK_MARKER.pid" +sleep 30 +`, childPath)) + writeExecutable(t, childPath, `#!/bin/sh +trap '' TERM INT HUP +touch "$1.ready" +sleep 30 +`) + + t.Setenv("ACP_LEAK_MARKER", marker) +} + +func writeExecutable(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o755); err != nil { + t.Fatalf("write executable %s: %v", path, err) + } +} + +func readPID(t *testing.T, path string) int { + t.Helper() + var raw []byte + waitUntil(t, 2*time.Second, func() bool { + var err error + raw, err = os.ReadFile(path) + return err == nil + }, "pid file %s was not written", path) + + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + if err != nil { + t.Fatalf("parse pid file %s: %v", path, err) + } + return pid +} + +func waitUntil(t *testing.T, timeout time.Duration, condition func() bool, format string, args ...any) { + t.Helper() + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if condition() { + return + } + time.Sleep(20 * time.Millisecond) + } + if condition() { + return + } + t.Fatalf(format, args...) +} + +func processRunning(pid int) bool { + if pid <= 0 { + return false + } + if err := syscall.Kill(pid, 0); err != nil { + return false + } + stat, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return true + } + return !strings.Contains(string(stat), ") Z ") +} diff --git a/apps/backend/internal/agentctl/server/winproc/lifecycle_other.go b/apps/backend/internal/agentctl/server/winproc/lifecycle_other.go new file mode 100644 index 0000000000..0521ca50e8 --- /dev/null +++ b/apps/backend/internal/agentctl/server/winproc/lifecycle_other.go @@ -0,0 +1,3 @@ +//go:build !windows + +package winproc diff --git a/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go new file mode 100644 index 0000000000..13b7f27544 --- /dev/null +++ b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go @@ -0,0 +1,190 @@ +//go:build windows + +package winproc + +import ( + "errors" + "fmt" + "os/exec" + "strings" + "syscall" + "unsafe" + + "golang.org/x/sys/windows" +) + +// KillOnCloseJob owns a Windows Job Object configured to terminate all assigned +// processes when the handle is closed. +type KillOnCloseJob struct { + handle windows.Handle +} + +// InstallKillOnCloseJobForSuspendedCommand assigns a suspended child process to +// a kill-on-close Job Object before resuming it. If job setup fails, the child +// is still resumed so callers can fall back to explicit taskkill cleanup. +func InstallKillOnCloseJobForSuspendedCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { + if cmd == nil || cmd.Process == nil { + return KillOnCloseJob{}, fmt.Errorf("process not started") + } + return InstallKillOnCloseJobForSuspendedProcess(cmd.Process.Pid) +} + +// InstallKillOnCloseJobForCommand assigns an already-running child process to +// a kill-on-close Job Object. Use the suspended variant when the child must not +// execute before job assignment. +func InstallKillOnCloseJobForCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { + if cmd == nil || cmd.Process == nil { + return KillOnCloseJob{}, fmt.Errorf("process not started") + } + return InstallKillOnCloseJobForProcess(cmd.Process.Pid) +} + +func InstallKillOnCloseJobForProcess(pid int) (KillOnCloseJob, error) { + job, err := createKillOnCloseJob() + if err != nil { + return KillOnCloseJob{}, err + } + if err := assignProcessToJob(job, pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, err + } + return KillOnCloseJob{handle: job}, nil +} + +func InstallKillOnCloseJobForSuspendedProcess(pid int) (KillOnCloseJob, error) { + job, err := createKillOnCloseJob() + if err != nil { + return KillOnCloseJob{}, errors.Join(err, ResumeSuspendedProcess(pid)) + } + if err := assignProcessToJob(job, pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, errors.Join( + err, + ResumeSuspendedProcess(pid), + ) + } + if err := ResumeSuspendedProcess(pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, err + } + return KillOnCloseJob{handle: job}, nil +} + +func (j KillOnCloseJob) Close() error { + if j.handle == 0 { + return nil + } + return windows.CloseHandle(j.handle) +} + +func (j KillOnCloseJob) RawHandle() uintptr { + return uintptr(j.handle) +} + +func createKillOnCloseJob() (windows.Handle, error) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, fmt.Errorf("CreateJobObject: %w", err) + } + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(job) + return 0, fmt.Errorf("SetInformationJobObject: %w", err) + } + return job, nil +} + +func assignProcessToJob(job windows.Handle, pid int) error { + procHandle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(pid), + ) + if err != nil { + return fmt.Errorf("OpenProcess(pid=%d): %w", pid, err) + } + defer windows.CloseHandle(procHandle) + if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { + return fmt.Errorf("AssignProcessToJobObject: %w", err) + } + return nil +} + +func ResumeSuspendedProcess(pid int) error { + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + if err != nil { + return fmt.Errorf("CreateToolhelp32Snapshot: %w", err) + } + defer windows.CloseHandle(snapshot) + + entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} + if err := windows.Thread32First(snapshot, &entry); err != nil { + return fmt.Errorf("Thread32First: %w", err) + } + + resumed := 0 + for { + if entry.OwnerProcessID == uint32(pid) { + if err := resumeThread(entry.ThreadID); err != nil { + return err + } + resumed++ + } + if err := windows.Thread32Next(snapshot, &entry); err != nil { + if errors.Is(err, windows.ERROR_NO_MORE_FILES) { + break + } + return fmt.Errorf("Thread32Next: %w", err) + } + } + if resumed == 0 { + return fmt.Errorf("no threads found for pid %d", pid) + } + return nil +} + +func resumeThread(threadID uint32) error { + thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, threadID) + if err != nil { + return fmt.Errorf("OpenThread(thread_id=%d): %w", threadID, err) + } + defer windows.CloseHandle(thread) + if _, err := windows.ResumeThread(thread); err != nil { + return fmt.Errorf("ResumeThread(thread_id=%d): %w", threadID, err) + } + return nil +} + +func RunTaskkill(args ...string) error { + output, err := exec.Command("taskkill", args...).CombinedOutput() + if err == nil { + return nil + } + msg := strings.TrimSpace(string(output)) + if IsTaskkillMissing(msg) { + return syscall.ESRCH + } + if msg == "" { + return err + } + return fmt.Errorf("%w: %s", err, msg) +} + +func IsTaskkillMissing(msg string) bool { + if msg == "" { + return false + } + lower := strings.ToLower(msg) + return strings.Contains(lower, "not found") || + strings.Contains(lower, "not be found") || + strings.Contains(lower, "no running instance") +} diff --git a/apps/backend/internal/agentctl/server/winproc/lifecycle_windows_test.go b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows_test.go new file mode 100644 index 0000000000..afe7ef40da --- /dev/null +++ b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows_test.go @@ -0,0 +1,67 @@ +//go:build windows + +package winproc + +import ( + "os/exec" + "testing" +) + +func TestIsTaskkillMissing(t *testing.T) { + tests := []struct { + name string + msg string + want bool + }{ + {name: "empty", msg: "", want: false}, + {name: "not found", msg: "ERROR: The process \"123\" not found.", want: true}, + {name: "not be found", msg: "ERROR: The process with PID 123 could not be found.", want: true}, + {name: "no running instance", msg: "ERROR: No running instance of the task.", want: true}, + {name: "other error", msg: "ERROR: Access is denied.", want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsTaskkillMissing(tt.msg); got != tt.want { + t.Fatalf("IsTaskkillMissing(%q) = %v, want %v", tt.msg, got, tt.want) + } + }) + } +} + +func TestInstallKillOnCloseJobForSuspendedCommandRejectsNilCommand(t *testing.T) { + if _, err := InstallKillOnCloseJobForSuspendedCommand(nil); err == nil { + t.Fatal("InstallKillOnCloseJobForSuspendedCommand(nil) error = nil, want non-nil") + } + if _, err := InstallKillOnCloseJobForSuspendedCommand(&exec.Cmd{}); err == nil { + t.Fatal("InstallKillOnCloseJobForSuspendedCommand(cmd without process) error = nil, want non-nil") + } +} + +func TestInstallKillOnCloseJobForCommandRejectsNilCommand(t *testing.T) { + if _, err := InstallKillOnCloseJobForCommand(nil); err == nil { + t.Fatal("InstallKillOnCloseJobForCommand(nil) error = nil, want non-nil") + } + if _, err := InstallKillOnCloseJobForCommand(&exec.Cmd{}); err == nil { + t.Fatal("InstallKillOnCloseJobForCommand(cmd without process) error = nil, want non-nil") + } +} + +func TestInstallKillOnCloseJobForCommandHappyPath(t *testing.T) { + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + job, err := InstallKillOnCloseJobForCommand(cmd) + if err != nil { + t.Fatalf("InstallKillOnCloseJobForCommand: %v", err) + } + if job.RawHandle() == 0 { + t.Fatal("got null job handle") + } + if err := cmd.Wait(); err != nil { + t.Fatalf("wait child: %v", err) + } + if err := job.Close(); err != nil { + t.Errorf("job.Close: %v", err) + } +} diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index b570ce502b..5536869de8 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -86,6 +86,9 @@ import ( const ( desktopHealthTokenEnv = "KANDEV_DESKTOP_HEALTH_TOKEN" desktopHealthTokenHeader = "X-Kandev-Desktop-Health-Token" + agentShutdownTimeout = 20 * time.Second + httpShutdownTimeout = 10 * time.Second + tracingShutdownTimeout = 5 * time.Second ) // buildSessionDataProvider constructs the session data provider function used by the WebSocket hub @@ -1152,46 +1155,66 @@ func runGracefulShutdown( runCleanups func(), log *logger.Logger, ) { - log.Info("Shutting down Kandev...") + start := time.Now() + var shutdownErrs []error + log.Info("Graceful shutdown started", + zap.Int("http_timeout_seconds", int(httpShutdownTimeout/time.Second)), + zap.Int("agent_timeout_seconds", int(agentShutdownTimeout/time.Second)), + zap.Int("tracing_timeout_seconds", int(tracingShutdownTimeout/time.Second))) - shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second) + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), httpShutdownTimeout) defer shutdownCancel() if err := server.Shutdown(shutdownCtx); err != nil { + shutdownErrs = append(shutdownErrs, err) log.Error("HTTP server shutdown error", zap.Error(err)) } if err := orchestratorSvc.Stop(); err != nil { + shutdownErrs = append(shutdownErrs, err) log.Error("Orchestrator stop error", zap.Error(err)) } - stopLifecycleManager(lifecycleMgr, log) + if err := stopLifecycleManager(lifecycleMgr, log); err != nil { + shutdownErrs = append(shutdownErrs, err) + } runCleanups() // Flush pending OTel spans before exit - traceCtx, traceCancel := context.WithTimeout(context.Background(), 5*time.Second) + traceCtx, traceCancel := context.WithTimeout(context.Background(), tracingShutdownTimeout) if err := tracing.Shutdown(traceCtx); err != nil { + shutdownErrs = append(shutdownErrs, err) log.Error("Tracer shutdown error", zap.Error(err)) } traceCancel() - log.Info("Kandev stopped") + log.Info("Graceful shutdown complete", + zap.Duration("duration", time.Since(start)), + zap.Int("error_count", len(shutdownErrs))) _ = log.Sync() } // stopLifecycleManager gracefully stops all agents and the lifecycle manager. -func stopLifecycleManager(lifecycleMgr *lifecycle.Manager, log *logger.Logger) { +func stopLifecycleManager(lifecycleMgr *lifecycle.Manager, log *logger.Logger) error { if lifecycleMgr == nil { - return + return nil } - log.Info("Stopping agents gracefully...") - stopCtx, stopCancel := context.WithTimeout(context.Background(), 5*time.Second) + var shutdownErrs []error + log.Info("Stopping agents gracefully", + zap.Int("timeout_seconds", int(agentShutdownTimeout/time.Second))) + stopCtx, stopCancel := context.WithTimeout(context.Background(), agentShutdownTimeout) if err := lifecycleMgr.StopAllAgents(stopCtx); err != nil { + shutdownErrs = append(shutdownErrs, err) log.Error("Graceful agent stop error", zap.Error(err)) } stopCancel() if err := lifecycleMgr.Stop(); err != nil { + shutdownErrs = append(shutdownErrs, err) log.Error("Lifecycle manager stop error", zap.Error(err)) } + if len(shutdownErrs) == 0 { + log.Info("Agents stopped gracefully") + } + return errors.Join(shutdownErrs...) } diff --git a/apps/backend/internal/backendapp/helpers_test.go b/apps/backend/internal/backendapp/helpers_test.go index 757e6a6e9f..0fc44ca207 100644 --- a/apps/backend/internal/backendapp/helpers_test.go +++ b/apps/backend/internal/backendapp/helpers_test.go @@ -12,7 +12,9 @@ import ( "github.com/gin-gonic/gin" "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/agent/executor" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + "github.com/kandev/kandev/internal/agentctl/server/process" "github.com/kandev/kandev/internal/common/config" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" @@ -114,6 +116,40 @@ func TestAppendAgentctlStatusMessage_IncludesWorkspacePathForReload(t *testing.T } } +func TestStopLifecycleManagerAllowsAgentctlInstanceCleanupWindow(t *testing.T) { + log, err := logger.NewLogger(logger.LoggingConfig{ + Level: "error", + Format: "console", + OutputPath: "stdout", + }) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + + stopper := &shutdownDeadlineExecutor{} + execRegistry := lifecycle.NewExecutorRegistry(log) + execRegistry.Register(stopper) + mgr := lifecycle.NewManager(nil, nil, execRegistry, nil, nil, nil, lifecycle.ExecutorFallbackDeny, t.TempDir(), log) + if err := mgr.ExecutionStoreForTesting().Add(&lifecycle.AgentExecution{ + ID: "exec-1", + TaskID: "task-1", + SessionID: "sess-1", + RuntimeName: executor.NameStandalone, + }); err != nil { + t.Fatalf("add execution: %v", err) + } + + startedAt := time.Now() + _ = stopLifecycleManager(mgr, log) + + if stopper.deadline.IsZero() { + t.Fatal("StopInstance was not called with a deadline") + } + if got := stopper.deadline.Sub(startedAt); got < 15*time.Second { + t.Fatalf("agent shutdown timeout = %v, want at least 15s for agentctl instance cleanup", got) + } +} + func TestBootInitialStateIncludesFeatureFlags(t *testing.T) { state := bootInitialState( context.Background(), @@ -137,6 +173,44 @@ func TestBootInitialStateIncludesFeatureFlags(t *testing.T) { } } +type shutdownDeadlineExecutor struct { + deadline time.Time +} + +func (s *shutdownDeadlineExecutor) Name() executor.Name { return executor.NameStandalone } + +func (s *shutdownDeadlineExecutor) HealthCheck(context.Context) error { return nil } + +func (s *shutdownDeadlineExecutor) CreateInstance( + context.Context, + *lifecycle.ExecutorCreateRequest, +) (*lifecycle.ExecutorInstance, error) { + return nil, nil +} + +func (s *shutdownDeadlineExecutor) StopInstance( + ctx context.Context, + _ *lifecycle.ExecutorInstance, + _ bool, +) error { + if deadline, ok := ctx.Deadline(); ok { + s.deadline = deadline + } + return nil +} + +func (s *shutdownDeadlineExecutor) RecoverInstances(context.Context) ([]*lifecycle.ExecutorInstance, error) { + return nil, nil +} + +func (s *shutdownDeadlineExecutor) GetInteractiveRunner() *process.InteractiveRunner { return nil } + +func (s *shutdownDeadlineExecutor) RequiresCloneURL() bool { return false } + +func (s *shutdownDeadlineExecutor) ShouldApplyPreferredShell() bool { return true } + +func (s *shutdownDeadlineExecutor) IsAlwaysResumable() bool { return false } + func TestBootInitialStateHomeIncludesKanbanFirstPaintState(t *testing.T) { taskSvc, workflowSvc := newBootStateTestServices(t) ctx := context.Background() diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 7c720aa1d3..04858bd034 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -1678,6 +1678,9 @@ func awaitShutdown( // ============================================ quit := make(chan os.Signal, 2) signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) + log.Debug("shutdown signal handler armed", + zap.Int("pid", os.Getpid()), + zap.Int("ppid", os.Getppid())) sig := <-quit // If we get a second signal, exit immediately. @@ -1688,6 +1691,8 @@ func awaitShutdown( os.Exit(1) }() - log.Info("Received shutdown signal", zap.String("signal", sig.String())) + log.Info("Received shutdown signal", + zap.String("signal", sig.String()), + zap.Int("pid", os.Getpid())) runGracefulShutdown(server, orchestratorSvc, lifecycleMgr, runCleanups, log) } diff --git a/apps/backend/internal/common/securityutil/git_test.go b/apps/backend/internal/common/securityutil/git_test.go index 35d0316de4..688afbbeff 100644 --- a/apps/backend/internal/common/securityutil/git_test.go +++ b/apps/backend/internal/common/securityutil/git_test.go @@ -4,21 +4,21 @@ import "testing" func TestIsValidBaseBranchRef(t *testing.T) { cases := map[string]bool{ - "main": true, - "release/v2": true, - "feature/foo-bar": true, - "origin/main": true, - "origin/release/v2": true, - "": false, - "main/": false, // trailing slash - "origin/main/": false, // trailing slash after origin strip - "a//b": false, // consecutive slashes - "bad..ref": false, // path traversal - "feature.lock": false, // .lock suffix - "bad branch": false, // space - "bad;rm -rf": false, // shell metacharacters - "-flag": false, // leading dash (regex requires alphanumeric first) - "/leading": false, // leading slash + "main": true, + "release/v2": true, + "feature/foo-bar": true, + "origin/main": true, + "origin/release/v2": true, + "": false, + "main/": false, // trailing slash + "origin/main/": false, // trailing slash after origin strip + "a//b": false, // consecutive slashes + "bad..ref": false, // path traversal + "feature.lock": false, // .lock suffix + "bad branch": false, // space + "bad;rm -rf": false, // shell metacharacters + "-flag": false, // leading dash (regex requires alphanumeric first) + "/leading": false, // leading slash } for ref, want := range cases { if got := IsValidBaseBranchRef(ref); got != want { diff --git a/apps/backend/internal/launcher/process.go b/apps/backend/internal/launcher/process.go index 5447f18db2..bc8caa7051 100644 --- a/apps/backend/internal/launcher/process.go +++ b/apps/backend/internal/launcher/process.go @@ -1,22 +1,37 @@ package launcher import ( + "errors" "fmt" + "io" "os" "os/exec" "os/signal" "sync" + "sync/atomic" "syscall" + "time" ) const capturedOutputLimit = 64 * 1024 +// Keep this above the backend's serial graceful-shutdown budgets so the +// launcher does not preempt agent/session cleanup that is still in progress. +const managedProcessShutdownGrace = 75 * time.Second +const managedProcessForceKillWait = 2 * time.Second + +var launcherShutdownDebug atomic.Bool +var launcherStatusOutput io.Writer = os.Stderr +var launcherExit = os.Exit + type processSupervisor struct { - mu sync.Mutex - children []*managedProcess + mu sync.Mutex + children []*managedProcess + shutdownOnce sync.Once } type managedProcess struct { + label string cmd *exec.Cmd exitCode int exited bool @@ -24,33 +39,119 @@ type managedProcess struct { done chan struct{} } +type managedProcessShutdownResult struct { + label string + pid int + duration time.Duration + graceful bool + forceKilled bool + err error +} + +type shutdownSummary struct { + graceful int + forceKilled int + failed int +} + func newSupervisor() *processSupervisor { return &processSupervisor{} } +func setLauncherShutdownDebug(enabled bool) { + launcherShutdownDebug.Store(enabled) +} + +func shutdownDebugf(format string, args ...interface{}) { + if !launcherShutdownDebug.Load() { + return + } + fmt.Fprintf(os.Stderr, "[kandev] [SHUTDOWN-DEBUG] "+format+"\n", args...) +} + +func launcherInfof(format string, args ...interface{}) { + _, _ = fmt.Fprintf(launcherStatusOutput, "[kandev] "+format+"\n", args...) +} + func (s *processSupervisor) add(proc *managedProcess) { s.mu.Lock() defer s.mu.Unlock() s.children = append(s.children, proc) + shutdownDebugf("supervisor add child pid=%d total_children=%d", proc.cmd.Process.Pid, len(s.children)) } func (s *processSupervisor) shutdown(reason string) { - fmt.Printf("[kandev] shutting down (%s)...\n", reason) + s.shutdownOnce.Do(func() { + s.runShutdown(reason) + }) +} + +func (s *processSupervisor) runShutdown(reason string) { s.mu.Lock() children := append([]*managedProcess(nil), s.children...) s.mu.Unlock() - for _, child := range children { - child.kill() + start := time.Now() + launcherInfof("graceful shutdown started (reason=%s, timeout=%s, processes=%d)", + reason, managedProcessShutdownGrace, len(children)) + shutdownDebugf("launcher shutdown begin reason=%q launcher_pid=%d children=%d", reason, os.Getpid(), len(children)) + results := make([]managedProcessShutdownResult, len(children)) + var wg sync.WaitGroup + for i, child := range children { + wg.Add(1) + go func(i int, child *managedProcess) { + defer wg.Done() + results[i] = child.kill() + }(i, child) } + wg.Wait() + logShutdownComplete(time.Since(start), results) + shutdownDebugf("launcher shutdown complete reason=%q", reason) +} + +func (s *processSupervisor) forceKillAll(reason string) []managedProcessShutdownResult { + s.mu.Lock() + children := append([]*managedProcess(nil), s.children...) + s.mu.Unlock() + shutdownDebugf("launcher force kill begin reason=%q launcher_pid=%d children=%d", reason, os.Getpid(), len(children)) + results := make([]managedProcessShutdownResult, len(children)) + var wg sync.WaitGroup + for i, child := range children { + wg.Add(1) + go func(i int, child *managedProcess) { + defer wg.Done() + results[i] = child.forceKill(reason) + }(i, child) + } + wg.Wait() + shutdownDebugf("launcher force kill complete reason=%q", reason) + return results } func (s *processSupervisor) attachSignals() { ch := make(chan os.Signal, 1) signal.Notify(ch, os.Interrupt, syscall.SIGTERM) + shutdownDebugf("launcher signal handler armed launcher_pid=%d", os.Getpid()) go func() { sig := <-ch - s.shutdown("signal " + sig.String()) - os.Exit(0) + shutdownDebugf("launcher received signal=%s launcher_pid=%d", sig.String(), os.Getpid()) + shutdownDone := make(chan struct{}) + go func() { + s.shutdown("signal " + sig.String()) + close(shutdownDone) + }() + select { + case nextSig := <-ch: + shutdownDebugf("launcher received signal during shutdown=%s launcher_pid=%d", nextSig.String(), os.Getpid()) + forceStart := time.Now() + launcherInfof("forced shutdown after second signal (signal=%s)", nextSig.String()) + results := s.forceKillAll("second signal " + nextSig.String()) + logForcedShutdownComplete(time.Since(forceStart), results) + signal.Stop(ch) + launcherExit(1) + case <-shutdownDone: + signal.Stop(ch) + launcherExit(0) + } }() } @@ -61,18 +162,23 @@ func startProcess(command string, args []string, cwd string, env []string, quiet cmd.Stdin = nil configureManagedProcess(cmd) stdout := newLimitedBuffer(capturedOutputLimit) - if quiet { - cmd.Stdout = stdout - cmd.Stderr = os.Stderr - } else { - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + var stdoutSink io.Writer + if !quiet { + stdoutSink = os.Stdout } + cmd.Stdout = newProcessOutput(stdout, stdoutSink, os.Stderr, label+".stdout") + cmd.Stderr = newProcessOutput(nil, os.Stderr, nil, label+".stderr") + supervisor.mu.Lock() if err := cmd.Start(); err != nil { + supervisor.mu.Unlock() return nil, nil, err } - proc := &managedProcess{cmd: cmd, done: make(chan struct{})} - supervisor.add(proc) + proc := &managedProcess{label: label, cmd: cmd, done: make(chan struct{})} + supervisor.children = append(supervisor.children, proc) + childCount := len(supervisor.children) + supervisor.mu.Unlock() + shutdownDebugf("managed process started label=%q pid=%d command=%q args=%q cwd=%q", label, cmd.Process.Pid, command, args, cwd) + shutdownDebugf("supervisor add child pid=%d total_children=%d", proc.cmd.Process.Pid, childCount) go func() { err := cmd.Wait() code := 0 @@ -82,6 +188,11 @@ func startProcess(command string, args []string, cwd string, env []string, quiet code = exitErr.ExitCode() } } + state := "" + if cmd.ProcessState != nil { + state = cmd.ProcessState.String() + } + shutdownDebugf("managed process wait complete label=%q pid=%d code=%d state=%q err=%v", label, cmd.Process.Pid, code, state, err) proc.mu.Lock() proc.exitCode = code proc.exited = true @@ -108,6 +219,47 @@ type limitedBuffer struct { buf []byte } +type processOutput struct { + mu sync.Mutex + buffer *limitedBuffer + sink io.Writer + fallbackSink io.Writer + label string + sinkDisabled bool +} + +func newProcessOutput(buffer *limitedBuffer, sink io.Writer, fallbackSink io.Writer, label string) *processOutput { + return &processOutput{buffer: buffer, sink: sink, fallbackSink: fallbackSink, label: label} +} + +func (w *processOutput) Write(p []byte) (int, error) { + if w.buffer != nil { + _, _ = w.buffer.Write(p) + } + + w.mu.Lock() + defer w.mu.Unlock() + if w.sink == nil || w.sinkDisabled { + return len(p), nil + } + if _, err := w.sink.Write(p); err != nil { + if w.fallbackSink != nil && w.fallbackSink != w.sink { + shutdownDebugf("switching broken output sink to fallback label=%q err=%v", w.label, err) + w.sink = w.fallbackSink + w.fallbackSink = nil + var fallbackErr error + if _, fallbackErr = w.sink.Write(p); fallbackErr == nil { + return len(p), nil + } + err = fallbackErr + } + w.sinkDisabled = true + shutdownDebugf("disabling broken output sink label=%q err=%v", w.label, err) + launcherInfof("warning: disabling %s output sink after write failure: %v", w.label, err) + } + return len(p), nil +} + func newLimitedBuffer(limit int) *limitedBuffer { return &limitedBuffer{limit: limit} } @@ -134,14 +286,132 @@ func (p *managedProcess) Exited() (bool, int) { return p.exited, p.exitCode } -func (p *managedProcess) kill() { +func (p *managedProcess) kill() managedProcessShutdownResult { + start := time.Now() + result := managedProcessShutdownResult{label: p.label} if p.cmd.Process == nil { - return + shutdownDebugf("managed process kill skipped: process nil") + result.err = fmt.Errorf("process is nil") + return result } - if err := killManagedProcessGroup(p.cmd.Process.Pid); err != nil { + pid := p.cmd.Process.Pid + result.pid = pid + select { + case <-p.done: + result.duration = time.Since(start) + result.graceful = true + shutdownDebugf("managed process kill skipped; already exited label=%q pid=%d", p.label, pid) + return result + default: + } + shutdownDebugf("managed process kill begin label=%q pid=%d grace=%s", p.label, pid, managedProcessShutdownGrace) + shutdownDebugf("managed process group SIGTERM requested label=%q pgid=%d", p.label, pid) + if err := terminateManagedProcessGroup(pid); err != nil { + if errors.Is(err, syscall.ESRCH) { + result.duration = time.Since(start) + result.graceful = true + shutdownDebugf("managed process group already gone label=%q pid=%d", p.label, pid) + return result + } + shutdownDebugf("managed process group SIGTERM failed pid=%d err=%v; killing process", pid, err) + shutdownDebugf("managed process SIGKILL requested label=%q pid=%d reason=%q", p.label, pid, "sigterm_failed") _ = p.cmd.Process.Kill() + result.forceKilled = true + result.err = err + if waitForManagedProcessKillDone(p.done, managedProcessForceKillWait) { + shutdownDebugf("managed process killed after SIGTERM failure pid=%d", pid) + } else { + result.err = errors.Join(result.err, fmt.Errorf("timed out waiting for process %d after SIGKILL", pid)) + shutdownDebugf("managed process kill wait timed out after SIGTERM failure pid=%d", pid) + } + result.duration = time.Since(start) + return result + } + shutdownDebugf("managed process group SIGTERM sent pgid=%d", pid) + select { + case <-p.done: + result.duration = time.Since(start) + result.graceful = true + shutdownDebugf("managed process exited within grace pid=%d", pid) + return result + case <-time.After(managedProcessShutdownGrace): + } + shutdownDebugf("managed process grace expired; sending SIGKILL pgid=%d", pid) + result.forceKilled = true + if err := killManagedProcessGroup(pid); err != nil { + if errors.Is(err, syscall.ESRCH) { + result.forceKilled = false + result.graceful = true + result.duration = time.Since(start) + shutdownDebugf("managed process group already gone before SIGKILL label=%q pid=%d", p.label, pid) + return result + } + shutdownDebugf("managed process group SIGKILL failed pid=%d err=%v; killing process", pid, err) + shutdownDebugf("managed process SIGKILL requested label=%q pid=%d reason=%q", p.label, pid, "process_group_kill_failed") + _ = p.cmd.Process.Kill() + result.err = err + } + if waitForManagedProcessKillDone(p.done, managedProcessForceKillWait) { + shutdownDebugf("managed process killed after grace pid=%d", pid) + } else { + result.err = errors.Join(result.err, fmt.Errorf("timed out waiting for process %d after SIGKILL", pid)) + shutdownDebugf("managed process kill wait timed out after SIGKILL pid=%d", pid) + } + result.duration = time.Since(start) + return result +} + +func (p *managedProcess) forceKill(reason string) managedProcessShutdownResult { + start := time.Now() + result := managedProcessShutdownResult{label: p.label} + if p.cmd.Process == nil { + shutdownDebugf("managed process force kill skipped: process nil") + result.err = fmt.Errorf("process is nil") + return result + } + pid := p.cmd.Process.Pid + result.pid = pid + select { + case <-p.done: + result.duration = time.Since(start) + result.graceful = true + shutdownDebugf("managed process force kill skipped; already exited label=%q pid=%d reason=%q", p.label, pid, reason) + return result + default: + } + shutdownDebugf("managed process group SIGKILL requested label=%q pgid=%d reason=%q", p.label, pid, reason) + result.forceKilled = true + if err := killManagedProcessGroup(pid); err != nil { + if errors.Is(err, syscall.ESRCH) { + result.forceKilled = false + result.graceful = true + result.duration = time.Since(start) + shutdownDebugf("managed process group already gone before force kill label=%q pid=%d", p.label, pid) + return result + } + shutdownDebugf("managed process group SIGKILL failed pid=%d err=%v; killing process", pid, err) + shutdownDebugf("managed process SIGKILL requested label=%q pid=%d reason=%q", p.label, pid, "force_kill_group_failed") + _ = p.cmd.Process.Kill() + result.err = err + } + if waitForManagedProcessKillDone(p.done, managedProcessForceKillWait) { + result.duration = time.Since(start) + shutdownDebugf("managed process force killed pid=%d", pid) + return result + } + result.duration = time.Since(start) + result.err = errors.Join(result.err, fmt.Errorf("timed out waiting for process %d after SIGKILL", pid)) + shutdownDebugf("managed process force kill wait timed out pid=%d", pid) + return result +} + +func waitForManagedProcessKillDone(done <-chan struct{}, timeout time.Duration) bool { + select { + case <-done: + return true + case <-time.After(timeout): + return false } - <-p.done } func waitForAppExit(supervisor *processSupervisor, backend *restartableBackend) int { @@ -149,3 +419,46 @@ func waitForAppExit(supervisor *processSupervisor, backend *restartableBackend) supervisor.shutdown("backend exit") return code } + +func logForcedShutdownComplete(duration time.Duration, results []managedProcessShutdownResult) { + summary := summarizeShutdown(results) + launcherInfof("forced shutdown complete (duration=%s, graceful=%d, force_killed=%d, failed=%d)", + duration.Round(time.Millisecond), summary.graceful, summary.forceKilled, summary.failed) +} + +func logShutdownComplete(duration time.Duration, results []managedProcessShutdownResult) { + summary := summarizeShutdown(results) + launcherInfof("graceful shutdown complete (duration=%s, graceful=%d, force_killed=%d, failed=%d)", + duration.Round(time.Millisecond), summary.graceful, summary.forceKilled, summary.failed) + for _, result := range results { + if result.forceKilled || result.err != nil { + label := result.label + if label == "" { + label = "process" + } + if result.err != nil { + launcherInfof("shutdown detail: %s pid=%d required force cleanup after %s: %v", + label, result.pid, result.duration.Round(time.Millisecond), result.err) + continue + } + launcherInfof("shutdown detail: %s pid=%d required SIGKILL after %s", + label, result.pid, result.duration.Round(time.Millisecond)) + } + } +} + +func summarizeShutdown(results []managedProcessShutdownResult) shutdownSummary { + var summary shutdownSummary + for _, result := range results { + if result.forceKilled { + summary.forceKilled++ + } + if result.err != nil { + summary.failed++ + } + if result.graceful { + summary.graceful++ + } + } + return summary +} diff --git a/apps/backend/internal/launcher/process_group_default.go b/apps/backend/internal/launcher/process_group_default.go index 330fb48c72..9ef1d9dcbb 100644 --- a/apps/backend/internal/launcher/process_group_default.go +++ b/apps/backend/internal/launcher/process_group_default.go @@ -1,4 +1,4 @@ -//go:build !linux && !darwin +//go:build !linux && !darwin && !windows package launcher @@ -7,9 +7,16 @@ import ( "os/exec" ) +func ignoreBrokenPipeSignal() { +} + func configureManagedProcess(_ *exec.Cmd) { } func killManagedProcessGroup(_ int) error { return errors.New("process groups are not supported on this platform") } + +func terminateManagedProcessGroup(_ int) error { + return errors.New("process groups are not supported on this platform") +} diff --git a/apps/backend/internal/launcher/process_group_unix.go b/apps/backend/internal/launcher/process_group_unix.go index 5d9c7b1831..0dc2a3e9d5 100644 --- a/apps/backend/internal/launcher/process_group_unix.go +++ b/apps/backend/internal/launcher/process_group_unix.go @@ -4,9 +4,14 @@ package launcher import ( "os/exec" + "os/signal" "syscall" ) +func ignoreBrokenPipeSignal() { + signal.Ignore(syscall.SIGPIPE) +} + func configureManagedProcess(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} } @@ -14,3 +19,7 @@ func configureManagedProcess(cmd *exec.Cmd) { func killManagedProcessGroup(pid int) error { return syscall.Kill(-pid, syscall.SIGKILL) } + +func terminateManagedProcessGroup(pid int) error { + return syscall.Kill(-pid, syscall.SIGTERM) +} diff --git a/apps/backend/internal/launcher/process_group_unix_test.go b/apps/backend/internal/launcher/process_group_unix_test.go index 0bd54b22a3..636e8550c9 100644 --- a/apps/backend/internal/launcher/process_group_unix_test.go +++ b/apps/backend/internal/launcher/process_group_unix_test.go @@ -3,10 +3,20 @@ package launcher import ( + "bytes" + "os" "os/exec" + "os/signal" + "path/filepath" + "strings" + "sync" + "syscall" "testing" + "time" ) +const launcherSignalHelperEnv = "KANDEV_LAUNCHER_SIGNAL_HELPER" + func TestConfigureManagedProcessCreatesProcessGroup(t *testing.T) { cmd := exec.Command("kandev") configureManagedProcess(cmd) @@ -15,3 +25,253 @@ func TestConfigureManagedProcessCreatesProcessGroup(t *testing.T) { t.Fatalf("configureManagedProcess should set Setpgid") } } + +func TestManagedProcessKillSendsGracefulSignalBeforeForceKill(t *testing.T) { + tempDir := t.TempDir() + readyFile := filepath.Join(tempDir, "ready") + termFile := filepath.Join(tempDir, "term") + cmd := exec.Command(os.Args[0], "-test.run=TestLauncherSignalHelper") + cmd.Env = append(os.Environ(), + launcherSignalHelperEnv+"=1", + "KANDEV_LAUNCHER_SIGNAL_HELPER_READY_FILE="+readyFile, + "KANDEV_LAUNCHER_SIGNAL_HELPER_FILE="+termFile, + ) + configureManagedProcess(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start fixture: %v", err) + } + + proc := &managedProcess{cmd: cmd, done: make(chan struct{})} + go func() { + err := cmd.Wait() + code := 0 + if err != nil { + code = 1 + if exitErr, ok := err.(*exec.ExitError); ok { + code = exitErr.ExitCode() + } + } + proc.mu.Lock() + proc.exitCode = code + proc.exited = true + proc.mu.Unlock() + close(proc.done) + }() + t.Cleanup(func() { + if exited, _ := proc.Exited(); !exited { + _ = killManagedProcessGroup(cmd.Process.Pid) + <-proc.done + } + }) + + waitForFile(t, readyFile) + proc.kill() + + raw, err := os.ReadFile(termFile) + if err != nil { + t.Fatalf("expected fixture to handle SIGTERM before force kill: %v", err) + } + if string(raw) != "term" { + t.Fatalf("SIGTERM marker = %q, want term", string(raw)) + } +} + +func TestAttachSignalsSingleSignalGracefulShutdown(t *testing.T) { + tempDir := t.TempDir() + termFile := filepath.Join(tempDir, "term") + proc := startManagedSignalHelper(t, termFile, "") + + exitCh, output := captureLauncherExit(t) + supervisor := newSupervisor() + supervisor.add(proc) + supervisor.attachSignals() + + sendLauncherTestSignal(t, os.Interrupt) + + waitForLauncherExitCode(t, exitCh, 0) + waitForManagedProcessDone(t, proc, 5*time.Second) + if raw, err := os.ReadFile(termFile); err != nil || string(raw) != "term" { + t.Fatalf("SIGTERM marker = %q, %v; want term, nil", string(raw), err) + } + if got := output.String(); !strings.Contains(got, "graceful shutdown complete") { + t.Fatalf("shutdown output missing graceful completion: %q", got) + } +} + +func TestAttachSignalsSecondSignalForceKillsChildren(t *testing.T) { + tempDir := t.TempDir() + termFile := filepath.Join(tempDir, "term") + proc := startManagedSignalHelper(t, termFile, "hold-after-term") + + exitCh, output := captureLauncherExit(t) + supervisor := newSupervisor() + supervisor.add(proc) + supervisor.attachSignals() + + sendLauncherTestSignal(t, os.Interrupt) + waitForFile(t, termFile) + sendLauncherTestSignal(t, os.Interrupt) + + waitForLauncherExitCode(t, exitCh, 1) + waitForManagedProcessDone(t, proc, 5*time.Second) + waitForOutputContains(t, output, "forced shutdown after second signal") + waitForOutputContains(t, output, "forced shutdown complete") + waitForOutputContains(t, output, "graceful shutdown complete") +} + +func TestLauncherSignalHelper(t *testing.T) { + if os.Getenv(launcherSignalHelperEnv) != "1" { + return + } + termFile := os.Getenv("KANDEV_LAUNCHER_SIGNAL_HELPER_FILE") + if termFile == "" { + os.Exit(2) + } + signals := make(chan os.Signal, 1) + signal.Notify(signals, syscall.SIGTERM) + readyFile := os.Getenv("KANDEV_LAUNCHER_SIGNAL_HELPER_READY_FILE") + if readyFile == "" { + os.Exit(4) + } + if err := os.WriteFile(readyFile, []byte("ready"), 0o600); err != nil { + os.Exit(5) + } + <-signals + if err := os.WriteFile(termFile, []byte("term"), 0o600); err != nil { + os.Exit(3) + } + if os.Getenv("KANDEV_LAUNCHER_SIGNAL_HELPER_MODE") == "hold-after-term" { + for { + time.Sleep(time.Hour) + } + } + os.Exit(0) +} + +func startManagedSignalHelper(t *testing.T, termFile string, mode string) *managedProcess { + t.Helper() + readyFile := filepath.Join(t.TempDir(), "ready") + cmd := exec.Command(os.Args[0], "-test.run=TestLauncherSignalHelper") + cmd.Env = append(os.Environ(), + launcherSignalHelperEnv+"=1", + "KANDEV_LAUNCHER_SIGNAL_HELPER_READY_FILE="+readyFile, + "KANDEV_LAUNCHER_SIGNAL_HELPER_FILE="+termFile, + "KANDEV_LAUNCHER_SIGNAL_HELPER_MODE="+mode, + ) + configureManagedProcess(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start fixture: %v", err) + } + proc := &managedProcess{label: "fixture", cmd: cmd, done: make(chan struct{})} + go func() { + err := cmd.Wait() + code := 0 + if err != nil { + code = 1 + if exitErr, ok := err.(*exec.ExitError); ok { + code = exitErr.ExitCode() + } + } + proc.mu.Lock() + proc.exitCode = code + proc.exited = true + proc.mu.Unlock() + close(proc.done) + }() + t.Cleanup(func() { + if exited, _ := proc.Exited(); !exited { + _ = killManagedProcessGroup(cmd.Process.Pid) + waitForManagedProcessDone(t, proc, 5*time.Second) + } + }) + waitForFile(t, readyFile) + return proc +} + +func captureLauncherExit(t *testing.T) (chan int, *safeOutput) { + t.Helper() + exitCh := make(chan int, 1) + output := &safeOutput{} + oldExit := launcherExit + oldStatusOutput := launcherStatusOutput + launcherExit = func(code int) { + exitCh <- code + } + launcherStatusOutput = output + t.Cleanup(func() { + launcherExit = oldExit + launcherStatusOutput = oldStatusOutput + }) + return exitCh, output +} + +func sendLauncherTestSignal(t *testing.T, sig os.Signal) { + t.Helper() + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatalf("find current process: %v", err) + } + if err := proc.Signal(sig); err != nil { + t.Fatalf("send signal %v: %v", sig, err) + } +} + +func waitForLauncherExitCode(t *testing.T, exitCh <-chan int, want int) { + t.Helper() + select { + case got := <-exitCh: + if got != want { + t.Fatalf("launcher exit code = %d, want %d", got, want) + } + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for launcher exit code %d", want) + } +} + +func waitForManagedProcessDone(t *testing.T, proc *managedProcess, timeout time.Duration) { + t.Helper() + select { + case <-proc.done: + case <-time.After(timeout): + t.Fatalf("timed out waiting for managed process pid=%d", proc.cmd.Process.Pid) + } +} + +func waitForOutputContains(t *testing.T, output *safeOutput, want string) { + t.Helper() + for range 500 { + if strings.Contains(output.String(), want) { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("shutdown output missing %q: %q", want, output.String()) +} + +type safeOutput struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (o *safeOutput) Write(p []byte) (int, error) { + o.mu.Lock() + defer o.mu.Unlock() + return o.buf.Write(p) +} + +func (o *safeOutput) String() string { + o.mu.Lock() + defer o.mu.Unlock() + return o.buf.String() +} + +func waitForFile(t *testing.T, path string) { + t.Helper() + for range 100 { + if _, err := os.Stat(path); err == nil { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", path) +} diff --git a/apps/backend/internal/launcher/process_group_windows.go b/apps/backend/internal/launcher/process_group_windows.go new file mode 100644 index 0000000000..67c5580018 --- /dev/null +++ b/apps/backend/internal/launcher/process_group_windows.go @@ -0,0 +1,32 @@ +//go:build windows + +package launcher + +import ( + "fmt" + "os/exec" + "syscall" + + "github.com/kandev/kandev/internal/agentctl/server/winproc" +) + +func ignoreBrokenPipeSignal() { +} + +func configureManagedProcess(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{ + CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, + } +} + +func killManagedProcessGroup(pid int) error { + return runLauncherTaskkill("/F", "/T", "/PID", fmt.Sprintf("%d", pid)) +} + +func terminateManagedProcessGroup(pid int) error { + return runLauncherTaskkill("/T", "/PID", fmt.Sprintf("%d", pid)) +} + +func runLauncherTaskkill(args ...string) error { + return winproc.RunTaskkill(args...) +} diff --git a/apps/backend/internal/launcher/process_group_windows_test.go b/apps/backend/internal/launcher/process_group_windows_test.go new file mode 100644 index 0000000000..22ff612daa --- /dev/null +++ b/apps/backend/internal/launcher/process_group_windows_test.go @@ -0,0 +1,47 @@ +//go:build windows + +package launcher + +import ( + "os" + "os/exec" + "testing" +) + +const launcherWindowsHelperEnv = "KANDEV_LAUNCHER_WINDOWS_HELPER" + +func TestWindowsManagedProcessForceKillTreatsAlreadyExitedPIDAsGraceful(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=TestWindowsLauncherHelper") + cmd.Env = append(os.Environ(), launcherWindowsHelperEnv+"=1") + configureManagedProcess(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + pid := cmd.Process.Pid + if err := cmd.Wait(); err != nil { + t.Fatalf("wait helper: %v", err) + } + + proc := &managedProcess{ + label: "already-exited", + cmd: cmd, + done: make(chan struct{}), + } + result := proc.forceKill("test") + if result.err != nil { + t.Fatalf("forceKill err = %v, want nil", result.err) + } + if !result.graceful || result.forceKilled { + t.Fatalf("forceKill result graceful=%v forceKilled=%v, want true/false", result.graceful, result.forceKilled) + } + if result.pid != pid { + t.Fatalf("forceKill pid = %d, want %d", result.pid, pid) + } +} + +func TestWindowsLauncherHelper(t *testing.T) { + if os.Getenv(launcherWindowsHelperEnv) != "1" { + return + } + os.Exit(0) +} diff --git a/apps/backend/internal/launcher/process_test.go b/apps/backend/internal/launcher/process_test.go index 93947ea6d7..5f94fb349d 100644 --- a/apps/backend/internal/launcher/process_test.go +++ b/apps/backend/internal/launcher/process_test.go @@ -2,7 +2,10 @@ package launcher import ( "bytes" + "errors" + "strings" "testing" + "time" ) func TestLimitedBufferKeepsOnlyTail(t *testing.T) { @@ -31,3 +34,126 @@ func TestLimitedBufferBytesReturnsSnapshot(t *testing.T) { t.Fatalf("mutating snapshot changed buffer: %q", buf.Bytes()) } } + +func TestProcessOutputKeepsDrainingAfterSinkBreaks(t *testing.T) { + var output bytes.Buffer + oldStatusOutput := launcherStatusOutput + launcherStatusOutput = &output + t.Cleanup(func() { + launcherStatusOutput = oldStatusOutput + }) + + sink := &failingWriter{err: errors.New("broken pipe")} + buf := newLimitedBuffer(20) + out := newProcessOutput(buf, sink, nil, "test") + + if n, err := out.Write([]byte("first")); err != nil || n != len("first") { + t.Fatalf("first Write() = (%d, %v), want (%d, nil)", n, err, len("first")) + } + if n, err := out.Write([]byte("second")); err != nil || n != len("second") { + t.Fatalf("second Write() = (%d, %v), want (%d, nil)", n, err, len("second")) + } + if sink.calls != 1 { + t.Fatalf("sink calls = %d, want 1", sink.calls) + } + if got := string(buf.Bytes()); got != "firstsecond" { + t.Fatalf("buffer = %q, want firstsecond", got) + } + if got := output.String(); !strings.Contains(got, "warning: disabling test output sink") { + t.Fatalf("status output did not warn about disabled sink: %q", got) + } +} + +func TestProcessOutputFallsBackAfterSinkBreaks(t *testing.T) { + sink := &failingWriter{err: errors.New("broken pipe")} + var fallback bytes.Buffer + out := newProcessOutput(nil, sink, &fallback, "test") + + if n, err := out.Write([]byte("first")); err != nil || n != len("first") { + t.Fatalf("first Write() = (%d, %v), want (%d, nil)", n, err, len("first")) + } + if n, err := out.Write([]byte("second")); err != nil || n != len("second") { + t.Fatalf("second Write() = (%d, %v), want (%d, nil)", n, err, len("second")) + } + + if sink.calls != 1 { + t.Fatalf("sink calls = %d, want 1", sink.calls) + } + if got := fallback.String(); got != "firstsecond" { + t.Fatalf("fallback output = %q, want firstsecond", got) + } +} + +func TestSummarizeShutdownCountsGracefulForceKilledAndFailed(t *testing.T) { + errStop := errors.New("stop failed") + got := summarizeShutdown([]managedProcessShutdownResult{ + {graceful: true}, + {forceKilled: true}, + {forceKilled: true, err: errStop}, + }) + + if got.graceful != 1 { + t.Fatalf("graceful = %d, want 1", got.graceful) + } + if got.forceKilled != 2 { + t.Fatalf("forceKilled = %d, want 2", got.forceKilled) + } + if got.failed != 1 { + t.Fatalf("failed = %d, want 1", got.failed) + } +} + +func TestSupervisorShutdownRunsOnce(t *testing.T) { + var output bytes.Buffer + oldStatusOutput := launcherStatusOutput + launcherStatusOutput = &output + t.Cleanup(func() { + launcherStatusOutput = oldStatusOutput + }) + + supervisor := newSupervisor() + supervisor.shutdown("signal interrupt") + supervisor.shutdown("backend exit") + + got := output.String() + if count := strings.Count(got, "graceful shutdown started"); count != 1 { + t.Fatalf("shutdown start log count = %d, want 1; output:\n%s", count, got) + } + if count := strings.Count(got, "graceful shutdown complete"); count != 1 { + t.Fatalf("shutdown complete log count = %d, want 1; output:\n%s", count, got) + } + if strings.Contains(got, "backend exit") { + t.Fatalf("second shutdown reason was logged; output:\n%s", got) + } +} + +func TestWaitForManagedProcessKillDoneReturnsOnClose(t *testing.T) { + done := make(chan struct{}) + close(done) + + if !waitForManagedProcessKillDone(done, time.Second) { + t.Fatal("expected closed done channel to return true") + } +} + +func TestWaitForManagedProcessKillDoneTimesOut(t *testing.T) { + done := make(chan struct{}) + start := time.Now() + + if waitForManagedProcessKillDone(done, 10*time.Millisecond) { + t.Fatal("expected open done channel to time out") + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("timeout wait took too long: %s", elapsed) + } +} + +type failingWriter struct { + calls int + err error +} + +func (w *failingWriter) Write(p []byte) (int, error) { + w.calls++ + return 0, w.err +} diff --git a/apps/backend/internal/launcher/start.go b/apps/backend/internal/launcher/start.go index 6fc0188a8f..7cb212ae74 100644 --- a/apps/backend/internal/launcher/start.go +++ b/apps/backend/internal/launcher/start.go @@ -9,8 +9,14 @@ import ( ) var ( - executablePath = os.Executable - launchManaged = runManagedApp + executablePath = os.Executable + launchManaged = runManagedApp + newSupervisorFn = newSupervisor + launchBackendFn = launchRestartableBackend + waitForHealthFn = waitForHealth + attachSignalsFn = func(supervisor *processSupervisor) { + supervisor.attachSignals() + } ) func runStart(opts Options) int { @@ -72,18 +78,23 @@ func resolveLogLevel(opts Options) string { } func runManagedApp(cfg managedAppConfig) int { + ignoreBrokenPipeSignal() logStartup(cfg.Header, cfg.Ports, resolveDatabasePath(), cfg.LogLevel) + setLauncherShutdownDebug(cfg.Opts.Debug || os.Getenv("KANDEV_SHUTDOWN_DEBUG") == "1") + shutdownDebugf("runManagedApp start mode=%q backend=%q backend_cwd=%q debug=%t", cfg.Mode, cfg.Backend, cfg.BackendCWD, cfg.Opts.Debug) - supervisor := newSupervisor() - supervisor.attachSignals() + supervisor := newSupervisorFn() + attachSignalsFn(supervisor) + shutdownDebugf("runManagedApp signal handler attached") showOutput := cfg.Opts.Verbose || cfg.Opts.Debug - backend, dumpLogs, err := launchRestartableBackend(cfg.Backend, []string{"__backend"}, cfg.BackendCWD, backendEnv(cfg.Ports, cfg.LogLevel, cfg.Opts.Debug), !showOutput, cfg.Ports, cfg.Mode, supervisor) + backend, dumpLogs, err := launchBackendFn(cfg.Backend, []string{"__backend"}, cfg.BackendCWD, backendEnv(cfg.Ports, cfg.LogLevel, cfg.Opts.Debug), !showOutput, cfg.Ports, cfg.Mode, supervisor) if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 } + shutdownDebugf("runManagedApp backend launched") fmt.Println("[kandev] starting backend...") - if err := waitForHealth(cfg.Ports.BackendURL, backend, healthTimeout(healthTimeoutReleaseMS), dumpLogs); err != nil { + if err := waitForHealthFn(cfg.Ports.BackendURL, backend, healthTimeout(healthTimeoutReleaseMS), dumpLogs); err != nil { supervisor.shutdown("backend health failure") fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 diff --git a/apps/backend/internal/launcher/start_test.go b/apps/backend/internal/launcher/start_test.go index 007765997b..143dabaaa8 100644 --- a/apps/backend/internal/launcher/start_test.go +++ b/apps/backend/internal/launcher/start_test.go @@ -2,7 +2,9 @@ package launcher import ( "path/filepath" + "reflect" "testing" + "time" ) func TestRunStartUsesSelfExecutableAndBackendCWD(t *testing.T) { @@ -45,3 +47,64 @@ func TestRunStartUsesSelfExecutableAndBackendCWD(t *testing.T) { t.Fatal("expected Headless option to be preserved") } } + +func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { + oldNewSupervisor := newSupervisorFn + oldLaunchBackend := launchBackendFn + oldAttachSignals := attachSignalsFn + oldWaitForHealth := waitForHealthFn + t.Cleanup(func() { + newSupervisorFn = oldNewSupervisor + launchBackendFn = oldLaunchBackend + attachSignalsFn = oldAttachSignals + waitForHealthFn = oldWaitForHealth + }) + + var events []string + newSupervisorFn = func() *processSupervisor { + events = append(events, "new-supervisor") + return newSupervisor() + } + launchBackendFn = func( + _ string, + _ []string, + _ string, + _ []string, + _ bool, + _ portConfig, + _ string, + _ *processSupervisor, + ) (*restartableBackend, func(), error) { + events = append(events, "launch-backend") + exitCh := make(chan int, 1) + exitCh <- 0 + return &restartableBackend{exitCh: exitCh}, func() {}, nil + } + attachSignalsFn = func(_ *processSupervisor) { + events = append(events, "attach-signals") + } + waitForHealthFn = func(_ string, _ childState, _ time.Duration, _ func()) error { + events = append(events, "wait-health") + return nil + } + t.Setenv("KANDEV_HOME_DIR", t.TempDir()) + + code := runManagedApp(managedAppConfig{ + Header: "test", + Mode: "start", + Backend: "kandev", + BackendCWD: t.TempDir(), + Ports: portConfig{ + BackendPort: 48123, + BackendURL: "http://localhost:48123", + }, + Opts: Options{Headless: true}, + }) + if code != 0 { + t.Fatalf("runManagedApp() = %d, want 0", code) + } + want := []string{"new-supervisor", "attach-signals", "launch-backend", "wait-health"} + if !reflect.DeepEqual(events, want) { + t.Fatalf("events = %v, want %v", events, want) + } +} diff --git a/docs/specs/tasks/runtime-cleanup.md b/docs/specs/tasks/runtime-cleanup.md index 2de516d941..e89a5f776c 100644 --- a/docs/specs/tasks/runtime-cleanup.md +++ b/docs/specs/tasks/runtime-cleanup.md @@ -29,8 +29,17 @@ worktrees, or executor rows behind and the machine slowly runs out of memory. preserved for retry/diagnosis. - Agent subprocess shutdown kills the whole agent process group when graceful shutdown does not finish within the configured stop timeout. +- Agent subprocess shutdown does not treat the command leader exiting as + sufficient while descendants remain alive in the same process group. - Agentctl instance shutdown never leaves an agent subprocess group alive as a child of `init`/PID 1. +- Standalone agentctl is isolated from terminal foreground interrupts so the + backend remains the owner of Ctrl+C shutdown sequencing. +- Top-level launchers, including `make start-debug` through `kandev start`, send + the backend a graceful termination signal before any force kill so backend + shutdown can stop agents and agentctl instances. +- Backend shutdown waits long enough for standalone agentctl's instance cleanup + window before reporting shutdown complete. - Backend startup reconciles stale runtime rows for archived/deleted/missing task state and attempts safe cleanup instead of treating the rows as live sessions. - Cleanup is idempotent: repeating archive/delete cleanup, startup reconciliation, @@ -126,6 +135,12 @@ Allowed transitions: the resource-specific retry path. - If an agentctl process exits unexpectedly, its owned agent subprocess group is killed before agentctl shutdown completes. +- If the user sends Ctrl+C to a standalone Kandev process tree, agentctl does + not receive the terminal interrupt directly; it is stopped through backend + lifecycle shutdown, parent liveness, or an explicit backend signal. +- If the user sends Ctrl+C while running through `make start-debug`, the launcher + forwards a graceful stop to the backend and waits before escalating, rather + than immediately killing the backend process group. - If startup reconciliation finds rows for archived tasks, deleted tasks, missing sessions, or terminal sessions with no live runtime, it removes only rows that are positively confirmed safe to remove. @@ -162,6 +177,17 @@ Allowed transitions: - **GIVEN** agentctl is stopped while an ACP child process ignores stdin EOF, **WHEN** the stop timeout expires, **THEN** the ACP process group is killed and no ACP child is reparented to PID 1. +- **GIVEN** an ACP wrapper process exits after spawning a native child in the + same process group, **WHEN** agentctl completes shutdown, **THEN** the + remaining process-group descendants are terminated before shutdown is reported + complete. +- **GIVEN** standalone Kandev owns an active agentctl instance, **WHEN** the user + stops Kandev with Ctrl+C, **THEN** backend shutdown supervises agentctl + cleanup and waits for the instance stop window instead of letting agentctl exit + directly from the terminal interrupt. +- **GIVEN** Kandev is running under `make start-debug`, **WHEN** the user stops it + with Ctrl+C, **THEN** the launcher gives the backend a graceful shutdown window + before any force kill so active ACP process groups are reaped. - **GIVEN** the `executors_running` query fails during archive cleanup, **WHEN** cleanup evaluates destructive actions, **THEN** it logs the failure and does not delete runtime tracking rows based on incomplete information.