Files
kandev/apps/backend/internal/agentctl/server/instance/mcp_validation_test.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

111 lines
3.5 KiB
Go

package instance
import (
"os"
"path/filepath"
"testing"
"github.com/kandev/kandev/internal/agentctl/server/config"
"github.com/kandev/kandev/pkg/agent"
"github.com/stretchr/testify/assert"
)
// TestBuildMcpServerConfigs_DropsMissingStdioCommand is the regression
// test for GH issue #1247's "stale `/snap/bin/brave` MCP" repro: a stdio
// MCP entry whose Command binary no longer exists on disk should not be
// passed through to the agent — it would just spawn a permanently-broken
// child process. URL-transport MCPs should always pass through.
func TestBuildMcpServerConfigs_DropsMissingStdioCommand(t *testing.T) {
mgr := NewManager(&config.Config{
Ports: config.PortConfig{Base: 0, Max: 0},
Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP},
// IdleTimeout zero => reaper goroutine never starts; no shutdown needed.
}, newTestLogger(t))
// Create a valid stdio command so we have at least one survivor.
validCmd := filepath.Join(t.TempDir(), "valid-mcp")
require := func(err error) {
if err != nil {
t.Fatalf("setup: %v", err)
}
}
require(os.WriteFile(validCmd, []byte("#!/bin/sh\nsleep 1\n"), 0o755))
input := []McpServerConfig{
{Name: "missing-stdio", Command: "/nonexistent/abs/path/does-not-exist"},
{Name: "missing-on-path", Command: "this-binary-is-definitely-not-on-path-xyzzy-1247"},
{Name: "ok-stdio", Command: validCmd},
{Name: "ok-http", URL: "http://localhost:9999/mcp", Type: "http"},
{Name: "ok-sse", URL: "http://localhost:9999/sse", Type: "sse"},
}
got := mgr.buildMcpServerConfigs(input)
names := make([]string, 0, len(got))
for _, srv := range got {
names = append(names, srv.Name)
}
assert.NotContains(t, names, "missing-stdio",
"missing absolute-path stdio MCP must be dropped")
assert.NotContains(t, names, "missing-on-path",
"missing on-PATH stdio MCP must be dropped")
assert.Contains(t, names, "ok-stdio",
"resolvable stdio MCP must survive")
assert.Contains(t, names, "ok-http",
"URL-transport MCP must always survive (no command to validate)")
assert.Contains(t, names, "ok-sse",
"URL-transport MCP must always survive (no command to validate)")
}
// TestMcpStdioValidationError covers the helper directly.
func TestMcpStdioValidationError(t *testing.T) {
tests := []struct {
name string
mcp McpServerConfig
expect string // "ok" or "error"
}{
{
name: "url-only entry skips validation",
mcp: McpServerConfig{Name: "x", URL: "http://example.test"},
expect: "ok",
},
{
name: "empty command and empty URL fails",
mcp: McpServerConfig{Name: "x"},
expect: "error",
},
{
name: "missing absolute path fails",
mcp: McpServerConfig{Name: "x", Command: "/this/path/does/not/exist"},
expect: "error",
},
{
name: "missing PATH binary fails",
mcp: McpServerConfig{Name: "x", Command: "definitely-not-a-real-binary-1247"},
expect: "error",
},
{
name: "compound command first token resolves",
mcp: McpServerConfig{Name: "x", Command: "sh -c 'echo hi'"},
expect: "ok",
},
{
name: "compound command with missing first token fails",
mcp: McpServerConfig{Name: "x", Command: "definitely-not-a-real-binary-1247 -m mcp_server"},
expect: "error",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := mcpStdioValidationError(tc.mcp)
if tc.expect == "ok" {
assert.Empty(t, got, "expected validation pass")
} else {
assert.NotEmpty(t, got, "expected validation failure")
}
})
}
}