Files
kandev/apps/backend/internal/agentctl/server/instance/manager.go
T
João Salavisa 6a684211fc fix(agentctl): reap disconnected ACP sessions + MCP child trees (#1249)
* fix(agentctl): reap disconnected ACP sessions + MCP child trees (#1247)

Two independent leaks let opencode acp sessions accumulate indefinitely:

1. instance.Manager never reaped disconnected sessions. StopInstance
   only ran via the kandev DELETE route, so any session whose backend
   went away (UI closed mid-run, WS dropped, opencode failed to emit
   complete) was orphaned forever.
2. Even when DELETE fired, the ACP adapter unconditionally returned
   RequiresProcessKill=false. opencode acp keeps an HTTP server + MCP
   child tree alive after stdin EOF, so closing stdin was a no-op and
   the children re-parented to init.

Fix both:

- Plumb RuntimeConfig.RequiresProcessKill from agents.OpenCodeACP
  through the ExecutorCreateRequest -> CreateInstanceRequest ->
  InstanceConfig -> adapter.Config -> shared.Config chain. acp.Adapter
  now reads cfg.RequiresProcessKill instead of hardcoding false.
- Belt-and-braces: process.Manager.waitForProcessExit timeout fallback
  now killProcessGroup first (only falls back to single-process kill
  if that errors), so any future ACP CLI that ignores stdin EOF still
  gets reaped.
- Add an idle-instance reaper to instance.Manager. Per-instance
  activity middleware tracks inflight HTTP requests + lastActivity;
  a background goroutine sweeps every minute and StopInstance's any
  instance idle past KANDEV_ACP_IDLE_TIMEOUT (default 1h, set to 0
  to disable). Long-lived /agent/stream WS connections keep the
  inflight counter above zero so active sessions are never reaped.
- Validate stdio MCP commands at CreateInstance time via
  exec.LookPath; missing entries are dropped with a warn instead of
  spawning a permanently broken child every session (fixes the
  stale /snap/bin/brave MCP repro in the issue).

Regression tests: process-group kill (parent+child fixture), idle
reaper end-to-end, MCP validation, opencode-acp opt-in vs other
ACP agents' default.

* fix: address PR review feedback on #1247 fix

- idle_reaper: extract reapSingleIdleInstance helper with defer cancel(),
  add reaperStop polling between sweep iterations so Shutdown drains in
  one StopInstance window instead of N * timeout (cubic, claude, greptile).
- instance.Manager: use sync.Once instead of m.mu to gate reaper-stop
  channel close (claude).
- mcpStdioValidationError: tolerate compound Command strings by
  resolving only the first whitespace-delimited token; reject entries
  with neither URL nor Command outright; document agentctl-vs-agent
  PATH skew in Docker/SSH executor modes (claude, greptile, cubic).
- manager_killgroup_test: treat zombie state as dead via /proc/<pid>/stat
  on Linux + bump Eventually window to 15s — CI containers' PID 1 can
  delay harvesting orphans long enough to flake signal(0) probes.
- instance package: add goleak TestMain for the new reaper goroutine
  (coderabbit).
- AGENTS.md: correct SIGKILL vs SIGTERM wording; clarify the timeout
  fallback only fires on ctx.Done (coderabbit, cubic).

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
2026-06-02 21:46:49 +01:00

413 lines
13 KiB
Go
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
// Package instance provides utilities for managing multi-agent instances.
package instance
import (
"context"
"errors"
"fmt"
"net"
"net/http"
"os/exec"
"strings"
"sync"
"syscall"
"time"
"github.com/google/uuid"
"github.com/kandev/kandev/internal/agentctl/server/config"
"github.com/kandev/kandev/internal/agentctl/server/process"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/pkg/agent"
"go.uber.org/zap"
)
// 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
// Manager manages multiple agent instances.
// It handles creation, tracking, and removal of agent instances,
// each with their own HTTP server on a dedicated port.
type Manager struct {
config *config.Config
logger *logger.Logger
instances map[string]*Instance
portAlloc *PortAllocator
serverFactory ServerFactory
mu sync.RWMutex
// reaperStop is closed by Shutdown to signal the idle reaper to exit.
// reaperStopOnce serializes the close so concurrent Shutdown calls can't
// double-close; sync.Once is used instead of m.mu because m.mu guards
// the instances map and shouldn't gate a one-shot lifecycle signal.
// reaperWG waits for the reaper goroutine to finish before Shutdown returns.
reaperStop chan struct{}
reaperStopOnce sync.Once
reaperWG sync.WaitGroup
}
// NewManager creates a new instance manager.
// If cfg.IdleTimeout > 0, a background goroutine periodically reaps
// instances that have been idle (no in-flight HTTP requests and no
// activity) for the configured duration.
func NewManager(cfg *config.Config, log *logger.Logger) *Manager {
// Clean up code-server processes orphaned by a previous session (safety net).
process.CleanupOrphanedCodeServers(log)
m := &Manager{
config: cfg,
logger: log.WithFields(zap.String("component", "instance-manager")),
instances: make(map[string]*Instance),
portAlloc: NewPortAllocator(cfg.Ports.Base, cfg.Ports.Max),
reaperStop: make(chan struct{}),
}
if cfg.IdleTimeout > 0 {
m.reaperWG.Add(1)
go m.runIdleReaper(cfg.IdleTimeout, cfg.IdleReaperInterval)
}
return m
}
// SetServerFactory sets the factory function for creating HTTP handlers for instances.
// This must be called before creating any instances.
func (m *Manager) SetServerFactory(factory ServerFactory) {
m.serverFactory = factory
}
// CreateInstance creates a new agent instance.
func (m *Manager) CreateInstance(ctx context.Context, req *CreateRequest) (*CreateResponse, error) {
m.mu.Lock()
defer m.mu.Unlock()
id := req.ID
if id == "" {
id = uuid.New().String()
}
if _, exists := m.instances[id]; exists {
return nil, fmt.Errorf("instance with ID %s already exists", id)
}
port, listener, err := m.allocatePortAndListener(id)
if err != nil {
return nil, err
}
agentCmd := m.resolveAgentCommand(req)
autoStart := req.AutoStart
mcpServers := m.buildMcpServerConfigs(req.McpServers)
m.logger.Info("CreateInstance: received request",
zap.String("req_protocol", req.Protocol),
zap.String("workspace_path", req.WorkspacePath))
overrides := &config.InstanceOverrides{
InstanceID: id,
Protocol: agent.Protocol(req.Protocol),
AgentCommand: agentCmd,
WorkDir: req.WorkspacePath,
AutoStart: &autoStart,
Env: config.CollectAgentEnv(req.Env),
AgentType: req.AgentType,
McpServers: mcpServers,
SessionID: req.SessionID,
TaskID: req.TaskID,
DisableAskQuestion: req.DisableAskQuestion,
AssumeMcpSse: req.AssumeMcpSse,
AssumeMcpHttp: req.AssumeMcpHttp,
McpMode: req.McpMode,
RequiresProcessKill: req.RequiresProcessKill,
}
m.logger.Info("CreateInstance: applying overrides",
zap.String("override_protocol", string(overrides.Protocol)))
// Create instance config using the unified method
instanceCfg := m.config.NewInstanceConfig(port, overrides)
m.logger.Info("CreateInstance: instance config created",
zap.String("config_protocol", string(instanceCfg.Protocol)))
// Create process manager
procMgr := process.NewManager(instanceCfg, m.logger)
// Start root + per-repo trackers so file-change events fire even in passthrough mode.
procMgr.StartAllWorkspaceTrackers(context.Background())
// Create instance up-front so the activity middleware can reference it.
inst := &Instance{
ID: id,
Port: port,
Status: "running",
WorkspacePath: req.WorkspacePath,
AgentCommand: agentCmd,
Env: req.Env,
CreatedAt: time.Now(),
manager: procMgr,
}
inst.MarkActivity()
handler := activityMiddleware(inst)(m.buildHTTPHandler(instanceCfg, procMgr))
httpServer := m.startHTTPServer(port, listener, handler, id)
inst.server = httpServer
m.instances[id] = inst
m.logger.Info("created instance",
zap.String("instance_id", id),
zap.Int("port", port),
zap.String("workspace", req.WorkspacePath))
return &CreateResponse{
ID: id,
Port: port,
}, nil
}
// allocatePortAndListener allocates a free port and binds a TCP listener to it.
func (m *Manager) allocatePortAndListener(id string) (int, net.Listener, error) {
maxAttempts := m.config.Ports.Max - m.config.Ports.Base + 1
for attempt := 0; attempt < maxAttempts; attempt++ {
allocated, err := m.portAlloc.Allocate(id)
if err != nil {
return 0, nil, fmt.Errorf("failed to allocate port: %w", err)
}
ln, err := net.Listen("tcp", fmt.Sprintf(":%d", allocated))
if err != nil {
if errors.Is(err, syscall.EADDRINUSE) || strings.Contains(err.Error(), "address already in use") {
m.portAlloc.MarkUnavailable(allocated)
m.logger.Warn("port already in use; retrying",
zap.String("instance_id", id),
zap.Int("port", allocated))
continue
}
m.portAlloc.Release(allocated)
return 0, nil, fmt.Errorf("failed to bind instance port %d: %w", allocated, err)
}
return allocated, ln, nil
}
return 0, nil, fmt.Errorf("failed to allocate an available port for instance %s", id)
}
// resolveAgentCommand returns the effective agent command for a create request.
func (m *Manager) resolveAgentCommand(req *CreateRequest) string {
agentCmd := req.AgentCommand
if agentCmd == "" {
agentCmd = m.config.Defaults.AgentCommand
}
if req.WorkspacePath != "" && req.WorkspaceFlag != "" && !strings.Contains(agentCmd, req.WorkspaceFlag) {
agentCmd = agentCmd + " " + req.WorkspaceFlag + " " + req.WorkspacePath
}
return agentCmd
}
// buildMcpServerConfigs converts instance McpServerConfig entries to
// config.McpServerConfig. Stdio entries whose Command can't be resolved
// (binary missing from PATH, no longer installed, etc.) are dropped with
// a warning so the agent doesn't spawn a permanently-broken child for an
// MCP it can never invoke. See GH issue #1247 — the `/snap/bin/brave`
// stale-MCP repro.
func (m *Manager) buildMcpServerConfigs(mcpServers []McpServerConfig) []config.McpServerConfig {
result := make([]config.McpServerConfig, 0, len(mcpServers))
for _, mcp := range mcpServers {
if reason := mcpStdioValidationError(mcp); reason != "" {
m.logger.Warn("dropping MCP server: stdio command unavailable",
zap.String("mcp_name", mcp.Name),
zap.String("command", mcp.Command),
zap.String("reason", reason))
continue
}
result = append(result, config.McpServerConfig{
Name: mcp.Name,
URL: mcp.URL,
Type: mcp.Type,
Command: mcp.Command,
Args: mcp.Args,
Env: mcp.Env,
Headers: mcp.Headers,
})
}
return result
}
// mcpStdioValidationError returns an empty string when the MCP entry is
// either non-stdio (URL transport) or its stdio Command resolves on PATH.
// Otherwise it returns a human-readable reason the entry should be dropped.
//
// Caveat: PATH is resolved against the agentctl process's environment.
// In Docker and SSH executor modes the agent may launch in a different
// environment than agentctl, so a binary that lives only inside the
// container/remote host but not on the agentctl host will be dropped
// here even though it would have worked at agent runtime. For Standalone
// and Sprites this is unambiguously correct; for Docker/SSH it's an
// acceptable false positive — surfacing the warn log is better than
// spawning a permanently broken child every session (the `/snap/bin/brave`
// repro in GH issue #1247).
func mcpStdioValidationError(mcp McpServerConfig) string {
// Non-stdio transports (sse, http, streamable_http) carry their endpoint
// in URL — nothing to validate locally.
if mcp.URL != "" {
return ""
}
if mcp.Command == "" {
return "stdio MCP entry has neither URL nor Command"
}
// Tolerate a compound `Command` string like "python3 -m mcp_server"
// where the user collapsed Command+Args into Command. The first token
// is the actual binary to look up; everything else is argv that the
// agent will splice in later. This is more permissive than the
// schema strictly allows, but it matches what real configs look like.
bin := mcp.Command
if i := strings.IndexAny(bin, " \t"); i > 0 {
bin = bin[:i]
}
if _, err := exec.LookPath(bin); err != nil {
return err.Error()
}
return ""
}
// buildHTTPHandler creates the HTTP handler for an instance.
func (m *Manager) buildHTTPHandler(instanceCfg *config.InstanceConfig, procMgr *process.Manager) http.Handler {
if m.serverFactory != nil {
return m.serverFactory(instanceCfg, procMgr, m.logger)
}
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusServiceUnavailable)
if _, err := w.Write([]byte("server factory not configured")); err != nil {
m.logger.Debug("failed to write default response", zap.Error(err))
}
})
}
// startHTTPServer creates and starts an HTTP server on the given listener.
func (m *Manager) startHTTPServer(port int, listener net.Listener, handler http.Handler, id string) *http.Server {
httpServer := &http.Server{
Addr: fmt.Sprintf(":%d", port),
Handler: handler,
}
go func() {
if err := httpServer.Serve(listener); err != nil && err != http.ErrServerClosed {
m.logger.Error("instance server error",
zap.String("instance_id", id),
zap.Error(err))
}
}()
return httpServer
}
// GetInstance returns an instance by ID.
func (m *Manager) GetInstance(id string) (*Instance, bool) {
m.mu.RLock()
defer m.mu.RUnlock()
inst, ok := m.instances[id]
return inst, ok
}
// ListInstances returns info for all instances.
func (m *Manager) ListInstances() []*InstanceInfo {
m.mu.RLock()
defer m.mu.RUnlock()
result := make([]*InstanceInfo, 0, len(m.instances))
for _, inst := range m.instances {
result = append(result, inst.Info())
}
return result
}
// StopInstance stops and removes an instance by ID.
func (m *Manager) StopInstance(ctx context.Context, id string) error {
// Get instance and remove from map under lock (quick operation)
m.mu.Lock()
inst, ok := m.instances[id]
if !ok {
m.mu.Unlock()
return fmt.Errorf("instance %s not found", id)
}
// Remove from map immediately so new instances aren't blocked
delete(m.instances, id)
m.mu.Unlock()
m.logger.Debug("stopping instance", zap.String("instance_id", id))
// Stop the process manager (potentially slow, done without lock)
if inst.manager != nil {
if err := inst.manager.Stop(ctx); err != nil {
m.logger.Warn("error stopping process manager",
zap.String("instance_id", id),
zap.Error(err))
}
}
m.logger.Debug("StopInstance: shutting down HTTP server",
zap.String("instance_id", id),
zap.Int("port", inst.Port))
// Shutdown HTTP server (potentially slow, done without lock)
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))
}
}
m.logger.Debug("StopInstance: releasing port",
zap.String("instance_id", id),
zap.Int("port", inst.Port))
// Release port (quick operation, re-acquire lock)
m.mu.Lock()
m.portAlloc.Release(inst.Port)
m.mu.Unlock()
m.logger.Info("StopInstance completed",
zap.String("instance_id", id),
zap.Int("port", inst.Port))
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
// with our explicit per-instance shutdown loop.
m.stopReaperOnce()
m.mu.Lock()
ids := make([]string, 0, len(m.instances))
for id := range m.instances {
ids = append(ids, id)
}
m.mu.Unlock()
var lastErr error
for _, id := range ids {
if err := m.StopInstance(ctx, id); err != nil {
m.logger.Error("error stopping instance during shutdown",
zap.String("instance_id", id),
zap.Error(err))
lastErr = err
}
}
return lastErr
}
// stopReaperOnce closes the reaper stop channel exactly once and waits for
// the reaper goroutine to drain. Safe to call multiple times.
//
// Note: reaperWG.Wait() blocks until the reaper finishes whatever sweep
// is currently in flight. Each instance in that sweep gets its own bounded
// context (idleReaperShutdownTimeout) independent of any caller deadline,
// so a Shutdown(ctx) caller with a tight deadline can find that the
// reaper drain consumed it before the main shutdown loop starts. The
// reaper polls reaperStop between instances, so the worst-case drain is
// one StopInstance round (15s), not N×timeout.
func (m *Manager) stopReaperOnce() {
m.reaperStopOnce.Do(func() {
close(m.reaperStop)
})
m.reaperWG.Wait()
}