diff --git a/apps/backend/internal/agentctl/server/api/control_instance_test.go b/apps/backend/internal/agentctl/server/api/control_instance_test.go new file mode 100644 index 0000000000..3ed9cb7df3 --- /dev/null +++ b/apps/backend/internal/agentctl/server/api/control_instance_test.go @@ -0,0 +1,28 @@ +package api + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/server/instance" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/pkg/agent" +) + +func TestDeleteInstanceUnknownReturnsNotFound(t *testing.T) { + mgr := instance.NewManager(&config.Config{ + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, logger.Default()) + t.Cleanup(func() { _ = mgr.Shutdown(t.Context()) }) + server := NewControlServer(&config.Config{}, mgr, logger.Default()) + req := httptest.NewRequest(http.MethodDelete, "/api/v1/instances/missing", nil) + resp := httptest.NewRecorder() + + server.Router().ServeHTTP(resp, req) + + if resp.Code != http.StatusNotFound { + t.Fatalf("DELETE unknown instance status = %d, want %d", resp.Code, http.StatusNotFound) + } +} diff --git a/apps/backend/internal/agentctl/server/instance/manager.go b/apps/backend/internal/agentctl/server/instance/manager.go index 2b692c0baf..25f6a9aee7 100644 --- a/apps/backend/internal/agentctl/server/instance/manager.go +++ b/apps/backend/internal/agentctl/server/instance/manager.go @@ -473,13 +473,27 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { if !ok { return fmt.Errorf("instance %s not found", id) } + return m.stopInstance(ctx, id, inst) +} + +func (m *Manager) stopInstance(ctx context.Context, id string, inst *Instance) error { inst.stopMu.Lock() defer inst.stopMu.Unlock() // A concurrent successful stop may have removed the instance while this // caller waited for the per-instance teardown lock. m.mu.Lock() - if current, exists := m.instances[id]; !exists || current != inst { + current, exists := m.instances[id] + if !exists { + alreadyStopped := inst.portReleased + m.mu.Unlock() + if !alreadyStopped { + return fmt.Errorf("instance %s not found", id) + } + m.logger.Debug("StopInstance already completed", zap.String("instance_id", id)) + return nil + } + if current != inst { m.mu.Unlock() return fmt.Errorf("instance %s not found", id) } diff --git a/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go b/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go index add68e781f..52059896ff 100644 --- a/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go +++ b/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go @@ -209,6 +209,79 @@ func TestStopInstanceRetainsPortAfterProcessTeardownFailure(t *testing.T) { require.Equal(t, port, reusedPort) } +func TestStopInstanceReturnsSuccessForCompletedDuplicateStop(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 12345, Max: 12345}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + t.Cleanup(func() { _ = mgr.Shutdown(context.Background()) }) + + port, err := mgr.portAlloc.Allocate("duplicate-stop") + require.NoError(t, err) + stopStarted := make(chan struct{}) + releaseStop := make(chan struct{}) + var releaseStopOnce sync.Once + releaseStopFn := func() { + releaseStopOnce.Do(func() { close(releaseStop) }) + } + t.Cleanup(releaseStopFn) + procMgr := &fakeProcessManager{ + stopStarted: stopStarted, + stopRelease: releaseStop, + } + inst := &Instance{ + ID: "duplicate-stop", + Port: port, + Status: "running", + CreatedAt: time.Now(), + manager: procMgr, + } + mgr.mu.Lock() + mgr.instances[inst.ID] = inst + mgr.mu.Unlock() + + firstDone := make(chan error, 1) + go func() { firstDone <- mgr.stopInstance(context.Background(), inst.ID, inst) }() + select { + case <-stopStarted: + case <-time.After(time.Second): + t.Fatal("first stop did not reach process teardown") + } + + secondDone := make(chan error, 1) + go func() { secondDone <- mgr.stopInstance(context.Background(), inst.ID, inst) }() + releaseStopFn() + + require.NoError(t, <-firstDone) + require.NoError(t, <-secondDone) + _, ok := mgr.GetInstance(inst.ID) + require.False(t, ok, "duplicate stop must leave the instance removed") + reusedPort, err := mgr.portAlloc.Allocate("replacement-instance") + require.NoError(t, err) + require.Equal(t, port, reusedPort, "duplicate stop must release the port once") +} + +func TestStopInstanceRejectsReplacementForCapturedInstance(t *testing.T) { + log := newTestLogger(t) + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: 12345, Max: 12345}, + Defaults: config.InstanceDefaults{Protocol: agent.ProtocolACP}, + }, log) + + oldInst := &Instance{ID: "reused-id", Status: "stopping"} + newInst := &Instance{ID: "reused-id", Status: "running"} + mgr.mu.Lock() + mgr.instances[newInst.ID] = newInst + mgr.mu.Unlock() + + err := mgr.stopInstance(context.Background(), oldInst.ID, oldInst) + require.ErrorContains(t, err, "instance reused-id not found") + _, ok := mgr.GetInstance(newInst.ID) + require.True(t, ok, "replacement instance must remain tracked") + require.Equal(t, "running", newInst.Info().Status) +} + func TestStopHTTPServerTreatsCanceledShutdownAsStoppedAfterClose(t *testing.T) { log := newTestLogger(t) mgr := NewManager(&config.Config{ @@ -241,8 +314,11 @@ func (s *fakeHTTPServer) Close() error { } type fakeProcessManager struct { - stopErr error - stopped bool + stopErr error + stopped bool + stopStarted chan<- struct{} + stopRelease <-chan struct{} + stopStartOnce sync.Once } // legacyResourceReleaseError proves a process teardown error cannot authorize @@ -261,5 +337,11 @@ func (m *fakeProcessManager) CloseAdmission() {} func (m *fakeProcessManager) StopForTeardown(context.Context) error { m.stopped = true + if m.stopStarted != nil { + m.stopStartOnce.Do(func() { close(m.stopStarted) }) + } + if m.stopRelease != nil { + <-m.stopRelease + } return m.stopErr } diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index eacc820119..a8dc63cacf 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -108,6 +108,9 @@ type Manager struct { stdin io.WriteCloser stdout io.ReadCloser stderr io.ReadCloser + stderrPipeMu sync.Mutex + stderrReader io.ReadCloser + stderrWriter io.WriteCloser status atomic.Value // Status exitCode atomic.Int32 exitErr atomic.Value // error @@ -1179,12 +1182,17 @@ func (m *Manager) Start(ctx context.Context) error { // Start the subprocess now that pipes are connected if err := m.cmd.Start(); err != nil { + _ = m.closeStderrPipe() m.status.Store(StatusError) return formatAgentStartError(err, m.cfg.AgentEnv) } + if err := m.closeStderrWriter(); err != nil { + m.logger.Debug("failed to close parent stderr pipe", zap.Error(err)) + } processLifecycle, err := installProcessLifecycle(m.cmd) if err != nil { reapErr := killAndWaitStartedCommand(m.cmd) + _ = m.closeStderrReader() m.status.Store(StatusError) return errors.Join(fmt.Errorf("failed to install agent process lifecycle: %w", err), reapErr) } @@ -1206,6 +1214,7 @@ func (m *Manager) Start(ctx context.Context) error { default: reapErr = killAndWaitStartedCommand(m.cmd) } + _ = m.closeStderrReader() m.status.Store(StatusError) return errors.Join(fmt.Errorf("failed to connect adapter: %w", err), reapErr) } @@ -1473,15 +1482,47 @@ func (m *Manager) startProcessPipes() error { _ = m.stdin.Close() return fmt.Errorf("failed to create stdout pipe: %w", err) } - m.stderr, err = m.cmd.StderrPipe() + stderrReader, stderrWriter, err := os.Pipe() if err != nil { _ = m.stdin.Close() _ = m.stdout.Close() return fmt.Errorf("failed to create stderr pipe: %w", err) } + m.stderr = stderrReader + m.stderrPipeMu.Lock() + m.stderrReader = stderrReader + m.stderrWriter = stderrWriter + m.stderrPipeMu.Unlock() + m.cmd.Stderr = stderrWriter return nil } +func (m *Manager) closeStderrReader() error { + m.stderrPipeMu.Lock() + reader := m.stderrReader + m.stderrReader = nil + m.stderrPipeMu.Unlock() + if reader == nil { + return nil + } + return reader.Close() +} + +func (m *Manager) closeStderrWriter() error { + m.stderrPipeMu.Lock() + writer := m.stderrWriter + m.stderrWriter = nil + m.stderrPipeMu.Unlock() + if writer == nil { + return nil + } + return writer.Close() +} + +func (m *Manager) closeStderrPipe() error { + return errors.Join(m.closeStderrWriter(), m.closeStderrReader()) +} + // startAgentShell auto-creates a shell session when ShellEnabled is configured. // Failure is non-fatal: the agent can still work without a shell session. func (m *Manager) startAgentShell() { @@ -2303,6 +2344,7 @@ func (m *Manager) waitForStderrDrain(stderrDone <-chan struct{}) { case <-stderrDone: case <-timer.C: m.logger.Warn("timed out waiting for agent stderr to drain") + _ = m.closeStderrReader() } } @@ -2352,12 +2394,13 @@ func (m *Manager) waitForExit(stderrDone <-chan struct{}) { defer close(m.doneCh) pid := m.agentPID() - // StderrPipe must be drained before Wait closes its parent-side pipe. The - // second bounded wait covers a reader that was still blocked when the - // process-exit wait began and was released by Wait closing the pipe. - m.waitForStderrDrain(stderrDone) + // The manager owns stderr's read/write pipe, so Wait can reap the process + // without closing the reader. The reader drains the pipe concurrently. err := m.cmd.Wait() + // Wait has observed process exit; now bound the reader drain in case a child + // process inherited the stderr writer and kept the pipe open. m.waitForStderrDrain(stderrDone) + _ = m.closeStderrReader() intentionalStop := m.Status() == StatusStopping switch { diff --git a/apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go b/apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go index c12c3172fa..e38b682400 100644 --- a/apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go @@ -32,6 +32,169 @@ func (r *gatedStderrReader) Read(p []byte) (int, error) { func (r *gatedStderrReader) Close() error { return nil } +type delayedStderrReader struct { + reader io.ReadCloser + started chan<- struct{} + release <-chan struct{} + startedOnce sync.Once +} + +func (r *delayedStderrReader) Read(p []byte) (int, error) { + r.startedOnce.Do(func() { close(r.started) }) + <-r.release + return r.reader.Read(p) +} + +func (r *delayedStderrReader) Close() error { return r.reader.Close() } + +func TestManagerLongRunningProcessHelper(t *testing.T) { + if os.Getenv("KANDEV_MANAGER_LONG_RUNNING_HELPER") != "1" { + return + } + select {} +} + +func TestWaitForExitDoesNotWarnWhileProcessIsRunning(t *testing.T) { + log, observed := newObservedTestLogger(t) + cmd := exec.Command(os.Args[0], "-test.run=TestManagerLongRunningProcessHelper") + cmd.Env = append(os.Environ(), "KANDEV_MANAGER_LONG_RUNNING_HELPER=1") + stderr, stderrWriter, err := os.Pipe() + if err != nil { + t.Fatalf("stderr pipe: %v", err) + } + cmd.Stderr = stderrWriter + m := &Manager{ + cmd: cmd, + stderr: stderr, + logger: log, + doneCh: make(chan struct{}), + updatesCh: make(chan adapter.AgentEvent, 1), + groupAliveFn: func(int) bool { return false }, + } + m.status.Store(StatusRunning) + t.Cleanup(func() { + if cmd.ProcessState == nil && cmd.Process != nil { + _ = cmd.Process.Kill() + } + _ = stderrWriter.Close() + _ = stderr.Close() + m.wg.Wait() + }) + if err := cmd.Start(); err != nil { + t.Fatalf("start process: %v", err) + } + if err := stderrWriter.Close(); err != nil { + t.Fatalf("close parent stderr pipe: %v", err) + } + + stderrDone := make(chan struct{}) + m.wg.Add(2) + go m.readStderr(stderrDone) + waitDone := make(chan struct{}) + go func() { + m.waitForExit(stderrDone) + close(waitDone) + }() + + timer := time.NewTimer(processStderrDrainTimeout + 100*time.Millisecond) + defer timer.Stop() + select { + case <-waitDone: + t.Fatal("waitForExit finished while the process was still running") + case <-timer.C: + if observedLogsContain(observed, "timed out waiting for agent stderr to drain") { + t.Fatal("waitForExit warned about stderr while the process was still running") + } + } + + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("kill process: %v", err) + } + select { + case <-waitDone: + case <-time.After(time.Second): + t.Fatal("waitForExit did not finish after process exit") + } + m.wg.Wait() +} + +func TestWaitForExitPreservesStderrAfterProcessExit(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=TestManagerProcessExitHelper") + cmd.Env = append(os.Environ(), "KANDEV_MANAGER_PROCESS_EXIT_HELPER=1") + m := &Manager{ + cmd: cmd, + logger: newTestLogger(t), + doneCh: make(chan struct{}), + updatesCh: make(chan adapter.AgentEvent, 1), + groupAliveFn: func(int) bool { + return false + }, + } + m.status.Store(StatusRunning) + require.NoError(t, m.startProcessPipes()) + stderrWriter, ok := cmd.Stderr.(*os.File) + if !ok { + t.Fatalf("cmd.Stderr = %T, want owned *os.File pipe", cmd.Stderr) + } + reader := m.stderr + started := make(chan struct{}) + release := make(chan struct{}) + m.stderr = &delayedStderrReader{ + reader: reader, + started: started, + release: release, + } + t.Cleanup(func() { + select { + case <-release: + default: + close(release) + } + if cmd.ProcessState == nil && cmd.Process != nil { + _ = cmd.Process.Kill() + } + _ = stderrWriter.Close() + _ = reader.Close() + _ = m.stdin.Close() + _ = m.stdout.Close() + m.wg.Wait() + }) + + require.NoError(t, cmd.Start()) + require.NoError(t, stderrWriter.Close()) + stderrDone := make(chan struct{}) + m.wg.Add(2) + go m.readStderr(stderrDone) + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("stderr reader did not start") + } + + waitDone := make(chan struct{}) + go func() { + m.waitForExit(stderrDone) + close(waitDone) + }() + drainDelay := time.NewTimer(100 * time.Millisecond) + defer drainDelay.Stop() + <-drainDelay.C + close(release) + select { + case <-waitDone: + case <-time.After(time.Second): + t.Fatal("waitForExit did not finish after stderr drained") + } + m.wg.Wait() + + event := <-m.updatesCh + recent, ok := event.Data["recent_stderr"].([]string) + if !ok { + t.Fatalf("recent_stderr = %#v", event.Data["recent_stderr"]) + } + require.Equal(t, []string{"workspace=https://opencode.ai/workspace/wrk_private"}, recent) +} + func TestWaitForExitWaitsForStderrReaderBeforePublishingError(t *testing.T) { started := make(chan struct{}) release := make(chan struct{}) @@ -48,6 +211,7 @@ func TestWaitForExitWaitsForStderrReaderBeforePublishingError(t *testing.T) { } m.status.Store(StatusRunning) require.NoError(t, cmd.Start()) + // m.stderr is a gated test stand-in; the subprocess writes to io.Discard. var releaseOnce sync.Once releaseReader := func() { releaseOnce.Do(func() { close(release) }) diff --git a/apps/backend/internal/agentctl/server/process/manager_test.go b/apps/backend/internal/agentctl/server/process/manager_test.go index 800f844911..c62e4e5f75 100644 --- a/apps/backend/internal/agentctl/server/process/manager_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_test.go @@ -116,10 +116,11 @@ func TestManagerProcessExitUsesSanitizedStderr(t *testing.T) { log, observed := newObservedTestLogger(t) cmd := exec.Command(os.Args[0], "-test.run=TestManagerProcessExitHelper") cmd.Env = append(os.Environ(), "KANDEV_MANAGER_PROCESS_EXIT_HELPER=1") - stderr, err := cmd.StderrPipe() + stderr, stderrWriter, err := os.Pipe() if err != nil { t.Fatalf("stderr pipe: %v", err) } + cmd.Stderr = stderrWriter m := &Manager{ cmd: cmd, stderr: stderr, @@ -132,9 +133,20 @@ func TestManagerProcessExitUsesSanitizedStderr(t *testing.T) { groupAliveFn: func(int) bool { return false }, } m.status.Store(StatusRunning) + t.Cleanup(func() { + if cmd.ProcessState == nil && cmd.Process != nil { + _ = cmd.Process.Kill() + } + _ = stderrWriter.Close() + _ = stderr.Close() + m.wg.Wait() + }) if err := cmd.Start(); err != nil { t.Fatalf("start process: %v", err) } + if err := stderrWriter.Close(); err != nil { + t.Fatalf("close parent stderr pipe: %v", err) + } stderrDone := make(chan struct{}) m.wg.Add(2) go m.readStderr(stderrDone) @@ -273,8 +285,11 @@ func TestStartProcessPipes_CreatesAllPipes(t *testing.T) { assert.NotNil(t, m.stdout, "stdout pipe should be created") assert.NotNil(t, m.stderr, "stderr pipe should be created") - // Clean up - _ = m.stdin.Close() + t.Cleanup(func() { + _ = m.stdin.Close() + _ = m.stdout.Close() + _ = m.closeStderrPipe() + }) } func TestStartProcessPipes_FailsAfterProcessStarted(t *testing.T) { diff --git a/apps/backend/internal/launcher/dev.go b/apps/backend/internal/launcher/dev.go index 0b92c8be56..798043ea55 100644 --- a/apps/backend/internal/launcher/dev.go +++ b/apps/backend/internal/launcher/dev.go @@ -20,7 +20,7 @@ var ( // Settings-triggered restart rebuilds from source) plus the Vite dev server. // All dev state lives under /.kandev-dev/ so `make dev` never // mutates the user's production ~/.kandev. -func runDev(ctx context.Context, opts Options) int { +func runDev(ctx context.Context, opts Options, build BuildInfo) int { cfg, code := devLaunchConfigFor(opts) if code != 0 { return code @@ -29,7 +29,7 @@ func runDev(ctx context.Context, opts Options) int { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 } - logStartup("dev mode: using local repo", cfg.ports, cfg.dbPath, cfg.logLevel, serverHostForConfig(cfg.startup)) + logStartup("dev mode: using local repo", build.Version, cfg.ports, cfg.dbPath, cfg.logLevel, serverHostForConfig(cfg.startup)) ignoreBrokenPipeSignal() setLauncherShutdownDebug(cfg.debug || os.Getenv("KANDEV_SHUTDOWN_DEBUG") == "1") @@ -77,7 +77,10 @@ func runDev(ctx context.Context, opts Options) int { return 1 } fmt.Printf("[kandev] backend ready at %s\n", cfg.ports.BackendURL) + return runDevWeb(ctx, opts, cfg, supervisor, backend) +} +func runDevWeb(ctx context.Context, opts Options, cfg devLaunchConfig, supervisor *processSupervisor, backend *restartableBackend) int { webURL := fmt.Sprintf("http://localhost:%d", cfg.ports.WebPort) fmt.Println("[kandev] starting web...") webProc, err := launchWebChild(cfg.repoRoot, cfg.ports, supervisor) diff --git a/apps/backend/internal/launcher/dev_test.go b/apps/backend/internal/launcher/dev_test.go index c10b4a027e..c4eaf56336 100644 --- a/apps/backend/internal/launcher/dev_test.go +++ b/apps/backend/internal/launcher/dev_test.go @@ -16,7 +16,7 @@ func itoa(n int) string { return strconv.Itoa(n) } var errTestHealth = errors.New("test health failure") -func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { +func TestRunDevBuildVersionAndLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { repo := makeRepoTree(t) t.Chdir(repo) t.Setenv("KANDEV_TASK_ID", "") @@ -89,10 +89,16 @@ func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { return &managedProcess{}, func() {}, nil } - code := runDev(context.Background(), Options{Command: CommandDev}) + var code int + output := captureLauncherStdout(t, func() { + code = runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{Version: "1.2.3"}) + }) if code != 0 { t.Fatalf("runDev() = %d, want 0", code) } + if !strings.Contains(output, "[kandev] version: 1.2.3\n") { + t.Fatalf("startup output = %q, missing build version", output) + } want := []string{ "new-supervisor", "attach-signals", @@ -212,7 +218,7 @@ func TestRunDevUsesConfiguredHealthTimeoutForBackendAndWeb(t *testing.T) { return &managedProcess{}, func() {}, nil } - if code := runDev(context.Background(), Options{Command: CommandDev}); code != 0 { + if code := runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{}); code != 0 { t.Fatalf("runDev() = %d, want 0", code) } want := 12345 * time.Millisecond @@ -266,7 +272,7 @@ func TestRunDevHeadlessSkipsBrowser(t *testing.T) { return &managedProcess{}, func() {}, nil } - code := runDev(context.Background(), Options{Command: CommandDev, Headless: true}) + code := runDev(context.Background(), Options{Command: CommandDev, Headless: true}, BuildInfo{}) if code != 0 { t.Fatalf("runDev(headless) = %d, want 0", code) } @@ -296,7 +302,7 @@ func TestRunDevAbortsOnBackupFailureBeforeLaunch(t *testing.T) { return nil, nil, nil } - code := runDev(context.Background(), Options{Command: CommandDev}) + code := runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{}) if code != 1 { t.Fatalf("runDev() = %d, want 1", code) } @@ -350,7 +356,7 @@ func TestRunDevShutsDownTreeOnBackendHealthFailure(t *testing.T) { return nil, nil, nil } - code := runDev(context.Background(), Options{Command: CommandDev}) + code := runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{}) if code != 1 { t.Fatalf("runDev() = %d, want 1", code) } @@ -415,7 +421,7 @@ func TestRunDevShutsDownTreeOnBackendReadinessFailure(t *testing.T) { return nil, nil, nil } - code := runDev(context.Background(), Options{Command: CommandDev}) + code := runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{}) if code != 1 { t.Fatalf("runDev() = %d, want 1", code) } @@ -480,7 +486,7 @@ func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { // signal waitForAppExit selects on. close(webDone) - code := runDev(context.Background(), Options{Command: CommandDev, Headless: true}) + code := runDev(context.Background(), Options{Command: CommandDev, Headless: true}, BuildInfo{}) if code != 7 { t.Fatalf("runDev() = %d, want the web child's exit code 7", code) } @@ -492,7 +498,7 @@ func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { func TestRunDevFailsWhenRepoRootNotFound(t *testing.T) { t.Chdir(t.TempDir()) - code := runDev(context.Background(), Options{Command: CommandDev}) + code := runDev(context.Background(), Options{Command: CommandDev}, BuildInfo{}) if code != 2 { t.Fatalf("runDev() = %d, want 2", code) } diff --git a/apps/backend/internal/launcher/launcher.go b/apps/backend/internal/launcher/launcher.go index 483c26aa3a..1df0befa1a 100644 --- a/apps/backend/internal/launcher/launcher.go +++ b/apps/backend/internal/launcher/launcher.go @@ -23,6 +23,7 @@ func Run(args []string, build BuildInfo) int { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 2 } + build.Version = normalizedBuildVersion(build.Version) if opts.ShowVersion { fmt.Println(build.Version) return 0 @@ -36,11 +37,11 @@ func Run(args []string, build BuildInfo) int { ctx := context.Background() switch opts.Command { case CommandStart: - return runStart(ctx, opts) + return runStart(ctx, opts, build) case CommandRun: - return runInstalled(ctx, opts) + return runInstalled(ctx, opts, build) case CommandDev: - return runDev(ctx, opts) + return runDev(ctx, opts, build) } fmt.Fprintf(os.Stderr, "[kandev] native launcher command %q is not implemented yet\n", opts.Command) return 1 diff --git a/apps/backend/internal/launcher/network_test.go b/apps/backend/internal/launcher/network_test.go index 011fb6d8c2..5f72771ffd 100644 --- a/apps/backend/internal/launcher/network_test.go +++ b/apps/backend/internal/launcher/network_test.go @@ -36,7 +36,7 @@ func TestLogStartupPrintsNetworkAddress(t *testing.T) { } output := captureLauncherStdout(t, func() { - logStartup("test", portConfig{ + logStartup("test", "", portConfig{ BackendPort: 38429, BackendURL: "http://localhost:38429", }, "", "") @@ -228,7 +228,7 @@ func TestLogStartupKeepsLocalhostWhenNetworkDiscoveryFails(t *testing.T) { } output := captureLauncherStdout(t, func() { - logStartup("test", portConfig{ + logStartup("test", "", portConfig{ BackendPort: 38429, BackendURL: "http://localhost:38429", }, "", "") @@ -257,7 +257,7 @@ func TestLogStartupSuppressesNetworkAddressesForLoopbackBind(t *testing.T) { } output := captureLauncherStdout(t, func() { - logStartup("test", portConfig{ + logStartup("test", "", portConfig{ BackendPort: 38429, BackendURL: "http://localhost:38429", }, "", "") diff --git a/apps/backend/internal/launcher/run.go b/apps/backend/internal/launcher/run.go index 7934ae3645..4ef1f0d6e9 100644 --- a/apps/backend/internal/launcher/run.go +++ b/apps/backend/internal/launcher/run.go @@ -7,7 +7,7 @@ import ( "path/filepath" ) -func runInstalled(ctx context.Context, opts Options) int { +func runInstalled(ctx context.Context, opts Options, build BuildInfo) int { startupConfig, err := loadBootstrapConfig() if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) @@ -46,6 +46,7 @@ func runInstalled(ctx context.Context, opts Options) int { } return launchManaged(ctx, managedAppConfig{ Header: "release: " + releaseTag, + Version: normalizedBuildVersion(build.Version), Mode: "run", Backend: bundle.Launcher, BackendCWD: filepath.Dir(bundle.Launcher), diff --git a/apps/backend/internal/launcher/start.go b/apps/backend/internal/launcher/start.go index 2bdd016af0..aa7aeec505 100644 --- a/apps/backend/internal/launcher/start.go +++ b/apps/backend/internal/launcher/start.go @@ -28,7 +28,7 @@ var ( } ) -func runStart(ctx context.Context, opts Options) int { +func runStart(ctx context.Context, opts Options, build BuildInfo) int { startupConfig, err := loadBootstrapConfig() if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) @@ -64,6 +64,7 @@ func runStart(ctx context.Context, opts Options) int { } return launchManaged(ctx, managedAppConfig{ Header: "start mode: using local build", + Version: normalizedBuildVersion(build.Version), Mode: "start", Backend: self, BackendCWD: filepath.Dir(self), @@ -77,6 +78,7 @@ func runStart(ctx context.Context, opts Options) int { type managedAppConfig struct { Header string + Version string Mode string Backend string BackendCWD string @@ -131,7 +133,7 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { } cfg.Ports.BackendURL = cfg.Endpoints.accessURL ignoreBrokenPipeSignal() - logStartup(cfg.Header, cfg.Ports, resolveDatabasePathForConfig(cfg.Startup), cfg.LogLevel, serverHostForConfig(cfg.Startup)) + logStartup(cfg.Header, cfg.Version, cfg.Ports, resolveDatabasePathForConfig(cfg.Startup), cfg.LogLevel, serverHostForConfig(cfg.Startup)) 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) @@ -191,8 +193,9 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { return waitForAppExit(supervisor, backend) } -func logStartup(header string, ports portConfig, dbPath, logLevel string, serverHosts ...string) { +func logStartup(header, version string, ports portConfig, dbPath, logLevel string, serverHosts ...string) { fmt.Println("[kandev] " + header) + fmt.Println("[kandev] version:", normalizedBuildVersion(version)) fmt.Println("[kandev] url:", ports.BackendURL) serverHost := os.Getenv("KANDEV_SERVER_HOST") if len(serverHosts) > 0 && serverHosts[0] != "" { @@ -211,6 +214,13 @@ func logStartup(header string, ports portConfig, dbPath, logLevel string, server } } +func normalizedBuildVersion(version string) string { + if version == "" { + return "dev" + } + return version +} + func openBrowser(url string) { if os.Getenv("KANDEV_NO_BROWSER") == "1" { return diff --git a/apps/backend/internal/launcher/start_test.go b/apps/backend/internal/launcher/start_test.go index 5384066bf0..104ef5fe7f 100644 --- a/apps/backend/internal/launcher/start_test.go +++ b/apps/backend/internal/launcher/start_test.go @@ -144,7 +144,7 @@ func TestRunStartUsesSelfExecutableAndBackendCWD(t *testing.T) { // dir must be canonical (macOS temp dirs sit under the /var -> /private/var link). t.Setenv("KANDEV_HOME_DIR", canonicalTempDir(t)) - code := runStart(context.Background(), Options{Command: CommandStart, BackendPort: 48123, Headless: true}) + code := runStart(context.Background(), Options{Command: CommandStart, BackendPort: 48123, Headless: true}, BuildInfo{}) if code != 42 { t.Fatalf("runStart() = %d, want 42", code) } @@ -187,7 +187,7 @@ func TestRunStartUsesConfiguredBindForBackendURL(t *testing.T) { return 42 } - if code := runStart(context.Background(), Options{Command: CommandStart, Headless: true}); code != 42 { + if code := runStart(context.Background(), Options{Command: CommandStart, Headless: true}, BuildInfo{}); code != 42 { t.Fatalf("runStart() = %d, want 42", code) } if got.Ports.BackendURL != "http://192.0.2.10:48123" { diff --git a/apps/backend/internal/launcher/startup_version_test.go b/apps/backend/internal/launcher/startup_version_test.go new file mode 100644 index 0000000000..fcc686d993 --- /dev/null +++ b/apps/backend/internal/launcher/startup_version_test.go @@ -0,0 +1,110 @@ +package launcher + +import ( + "context" + "path/filepath" + "strings" + "testing" +) + +func TestLogStartupPrintsVersion(t *testing.T) { + t.Setenv("KANDEV_SERVER_HOST", "127.0.0.1") + output := captureLauncherStdout(t, func() { + logStartup("test", "1.2.3", portConfig{ + BackendPort: 38429, + BackendURL: "http://localhost:38429", + }, "", "") + }) + + if !strings.Contains(output, "[kandev] version: 1.2.3\n") { + t.Fatalf("startup output = %q, missing version", output) + } + if strings.Index(output, "[kandev] version:") > strings.Index(output, "[kandev] url:") { + t.Fatalf("startup output = %q, version must precede URL", output) + } +} + +func TestLogStartupDefaultsEmptyVersionToDev(t *testing.T) { + t.Setenv("KANDEV_SERVER_HOST", "127.0.0.1") + output := captureLauncherStdout(t, func() { + logStartup("test", "", portConfig{ + BackendPort: 38429, + BackendURL: "http://localhost:38429", + }, "", "") + }) + + if got := strings.Count(output, "[kandev] version:"); got != 1 { + t.Fatalf("startup output = %q, version line count = %d, want 1", output, got) + } + if !strings.Contains(output, "[kandev] version: dev\n") { + t.Fatalf("startup output = %q, want dev fallback", output) + } +} + +func TestRunUsesDevVersionForEmptyBuild(t *testing.T) { + output := captureLauncherStdout(t, func() { + if code := Run([]string{"--version"}, BuildInfo{}); code != 0 { + t.Fatalf("Run(--version) = %d, want 0", code) + } + }) + + if output != "dev\n" { + t.Fatalf("--version output = %q, want %q", output, "dev\n") + } +} + +func TestRunStartBuildVersion(t *testing.T) { + oldExecutablePath := executablePath + oldLaunchManaged := launchManaged + t.Cleanup(func() { + executablePath = oldExecutablePath + launchManaged = oldLaunchManaged + }) + + exe := filepath.Join(canonicalTempDir(t), "bin", "kandev") + executablePath = func() (string, error) { + return exe, nil + } + t.Setenv("KANDEV_HOME_DIR", canonicalTempDir(t)) + + var got managedAppConfig + launchManaged = func(_ context.Context, cfg managedAppConfig) int { + got = cfg + return 42 + } + + if code := runStart(context.Background(), Options{Command: CommandStart, Headless: true}, BuildInfo{Version: "1.2.3"}); code != 42 { + t.Fatalf("runStart() = %d, want 42", code) + } + if got.Version != "1.2.3" { + t.Fatalf("managed startup version = %q, want 1.2.3", got.Version) + } +} + +func TestRunInstalledBuildVersion(t *testing.T) { + oldLaunchManaged := launchManaged + t.Cleanup(func() { launchManaged = oldLaunchManaged }) + + bundle := t.TempDir() + writeFile(t, filepath.Join(bundle, "bin", executableName("kandev"))) + writeFile(t, filepath.Join(bundle, "bin", executableName("agentctl"))) + writeRemoteAgentctlHelpers(t, bundle) + t.Setenv("KANDEV_BUNDLE_DIR", bundle) + clearLauncherConfigurationEnvironment(t) + t.Chdir(t.TempDir()) + homeDir := canonicalTempDir(t) + t.Setenv("KANDEV_HOME_DIR", homeDir) + + var got managedAppConfig + launchManaged = func(_ context.Context, cfg managedAppConfig) int { + got = cfg + return 42 + } + + if code := runInstalled(context.Background(), Options{Command: CommandRun, Headless: true}, BuildInfo{Version: "1.2.3"}); code != 42 { + t.Fatalf("runInstalled() = %d, want 42", code) + } + if got.Version != "1.2.3" { + t.Fatalf("managed startup version = %q, want 1.2.3", got.Version) + } +} diff --git a/docs/plans/startup-observability-cleanup/plan.md b/docs/plans/startup-observability-cleanup/plan.md new file mode 100644 index 0000000000..94dcc74956 --- /dev/null +++ b/docs/plans/startup-observability-cleanup/plan.md @@ -0,0 +1,124 @@ +--- +created: 2026-08-26 +status: in_progress +requirements: + - REQ-PLATFORM-GO-DEV-LAUNCHER-001 + - REQ-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001 + - REQ-PLATFORM-AGENTCTL-INSTANCE-STOP-001 +system_design: + - ../../specs/platform/system-design/go-dev-launcher.md + - ../../specs/platform/system-design/agent-process-exit-drain.md + - ../../specs/platform/system-design/agentctl-instance-stop.md +legacy_specs: [] +--- + +# Implementation Plan: Startup observability and lifecycle cleanup + +## Overview + +The 2026-08-26 log audit found two confirmed implementation defects and one +requested observability gap. First, `waitForExit` emits a false stderr-drain +warning because it waits for EOF before `cmd.Wait`. Second, duplicate instance +stops return HTTP 500 after another stop has already completed. Third, the +native launcher does not print the build version in its startup banner. The +work orders are sequential because each changes a lifecycle boundary and needs +focused regression evidence before the next change is reviewed. + +## Scope + +### In scope + +- Print the native build version in `dev`, `start`, and `run` startup banners. +- Make stderr-drain timeout warnings post-exit diagnostics only. +- Make overlapping stops for the same agentctl instance converge on the stopped + state without masking genuine teardown failures. +- Update the public CLI reference for the new startup line. + +### Out of scope + +- Cursor ACP authentication failure. This requires the operator to log in or + remove the unready Cursor agent configuration. +- SSE MCP filtering. This is expected capability negotiation for ACP agents + that do not support SSE. +- Shell stop grace periods, process-group force-kill policy, Git polling policy, + and log rotation. +- Broad reclassification of unrelated agentctl stderr or startup warnings. + +## Technical approach + +### Startup version identity + +Pass `launcher.BuildInfo.Version` from `launcher.Run` through the mode-specific +launch configuration into the shared `logStartup` function. Render one +`[kandev] version:` line before the existing URL lines, using the native `dev` +default when metadata is empty. Keep `KANDEV_VERSION` as optional installed +release header metadata. Update `docs/public/cli.md` in the same implementation +change. + +### Agent stderr exit sequencing + +Keep `readStderr` running concurrently with the child process. Replace the +manager's `Cmd.StderrPipe` with an explicitly owned `os.Pipe`, then change +`internal/agentctl/server/process.Manager.waitForExit` to call `cmd.Wait()` +before the bounded wait on the generation-specific stderr completion channel. +Close the owned reader when the bounded drain expires. Preserve recent-stderr +sanitization, exit events, intentional-stop behavior, and process-group +reaping. + +### Idempotent instance stop + +Keep the initial unknown-ID 404 and the existing per-instance `stopMu`. After a +stop caller acquires the lock, distinguish removal of the same instance pointer +after successful teardown from an ID remapped to a different instance. Treat +the former as an already-satisfied stop, release the port only once, and retain +HTTP 500 plus the instance for genuine cleanup failures. + +## Tests + +- `AC-PLATFORM-GO-DEV-LAUNCHER-001.9`: add launcher banner coverage in + `apps/backend/internal/launcher/network_test.go` or a focused startup test, + including stamped, empty, and `--version`-equivalent values. +- `AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.1` through `.4`: extend + `apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go` + to prove no pre-exit timeout warning, post-exit bounded drain, preserved + recent stderr, and unchanged intentional/error exit paths. +- `AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.1` and `.4`: add deterministic + concurrent-stop coverage in + `apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go`. +- `AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.2` and `.3`: retain or extend + control-server boundary coverage for unknown IDs and cleanup failures. + +## E2E tests + +The new version contract is native launcher stdout, not browser behavior, so a +Playwright project cannot observe it. The package uses an end-to-end launcher +boundary test in `apps/backend/internal/launcher` and the existing backend +process tests for the two internal lifecycle contracts. No browser E2E test is +needed for these acceptance criteria. + +## Work orders + +- [x] [Task 01: Startup banner version identity](task-01-startup-version.md) (done) +- [x] [Task 02: Agent stderr exit sequencing](task-02-agent-stderr-exit.md) (done) +- [x] [Task 03: Idempotent agent instance stop](task-03-instance-stop.md) (done) + +## Verification results + +- Task 01 focused launcher tests passed. +- Public documentation validation passed (`node --test scripts/validate-public-docs.test.mjs`; `node scripts/validate-public-docs.mjs`). +- The full launcher package ran 259 tests successfully, with two pre-existing + service configuration tests selecting the host's `/root/.kandev/config.yaml` + instead of their temporary fixture. +- The full process-manager package passed 707 tests after the stderr sequencing + fix and owned-pipe correction. +- The combined instance and control-server packages passed 463 tests after the + idempotent-stop fix. + +## Risks + +- The owned stderr writer must close in the parent after startup, while the + reader remains concurrent so a child that writes a full pipe cannot deadlock. +- Treating a missing map entry as already stopped must be tied to the original + instance pointer so an ID reuse cannot be mistaken for completed cleanup. +- The startup banner is consumed by people and scripts, so existing line order + and values must remain stable apart from the additive version line. diff --git a/docs/plans/startup-observability-cleanup/task-01-startup-version.md b/docs/plans/startup-observability-cleanup/task-01-startup-version.md new file mode 100644 index 0000000000..cdd2e3923f --- /dev/null +++ b/docs/plans/startup-observability-cleanup/task-01-startup-version.md @@ -0,0 +1,94 @@ +--- +id: "01-startup-version" +title: "Startup banner version identity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GO-DEV-LAUNCHER-001 +acceptance_criteria: + - AC-PLATFORM-GO-DEV-LAUNCHER-001.9 +system_design: + - ../../specs/platform/system-design/go-dev-launcher.md +--- + +# Task 01: Startup banner version identity + +## Summary + +Add the native build version to the shared `dev`, `start`, and `run` startup +banner. Preserve the existing startup fields and document the new line in the +public CLI reference. + +## In scope + +- Carry `launcher.BuildInfo.Version` into each shared startup path. +- Render the `version:` line before the existing URL lines. +- Use `dev` for an unstamped or empty build value. +- Add focused launcher coverage and update `docs/public/cli.md`. + +## Out of scope + +- Changing service-unit version metadata. +- Changing `KANDEV_VERSION` release-header behavior. +- Changing backend health or system-info payloads. + +## Acceptance + +- Every `dev`, `start`, and `run` startup banner includes exactly one version + line whose value matches the binary's `--version` value. +- Empty build metadata renders `dev`, and all existing URL, MCP, database, and + log-level lines remain present and unchanged. +- The public CLI reference describes the additive version line and its source. + +## Verification + +```bash +cd apps/backend && go test ./internal/launcher -run 'TestLogStartupPrintsVersion|TestRun.*BuildVersion' -count=1 +cd apps/backend && go test ./internal/launcher -count=1 +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `apps/backend/internal/launcher/launcher.go` +- `apps/backend/internal/launcher/start.go` +- `apps/backend/internal/launcher/run.go` +- `apps/backend/internal/launcher/dev.go` +- `apps/backend/internal/launcher/network_test.go` +- `docs/public/cli.md` + +## Dependencies + +None. + +## Risks + +- Direct launcher tests construct mode configuration values, so the build + version must be threaded without changing unrelated startup defaults. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/platform/requirements/go-dev-launcher.md`, AC `.9`. +- `docs/specs/platform/system-design/go-dev-launcher.md`. +- Existing `launcher.BuildInfo`, `logStartup`, and network banner tests. + +## Results + +Implemented the version line for `dev`, `start`, and `run`. The value is +normalized to `dev` when build metadata is empty and is shared with the +launcher `--version` path. Updated the public CLI reference. + +Verification: + +- `go test ./internal/launcher -run 'TestLogStartupPrintsVersion|TestRun.*BuildVersion' -count=1` passed. +- `go test ./internal/launcher -count=1` ran 259 tests; two existing service + configuration tests were blocked by the host's `/root/.kandev/config.yaml`. +- `node --test scripts/validate-public-docs.test.mjs` passed. +- `node scripts/validate-public-docs.mjs` passed. diff --git a/docs/plans/startup-observability-cleanup/task-02-agent-stderr-exit.md b/docs/plans/startup-observability-cleanup/task-02-agent-stderr-exit.md new file mode 100644 index 0000000000..bd1ccdeb77 --- /dev/null +++ b/docs/plans/startup-observability-cleanup/task-02-agent-stderr-exit.md @@ -0,0 +1,94 @@ +--- +id: "02-agent-stderr-exit" +title: "Agent stderr exit sequencing" +status: done +wave: 2 +depends_on: + - "01-startup-version" +plan: "plan.md" +requirements: + - REQ-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001 +acceptance_criteria: + - AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.1 + - AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.2 + - AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.3 + - AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.4 +system_design: + - ../../specs/platform/system-design/agent-process-exit-drain.md +--- + +# Task 02: Agent stderr exit sequencing + +## Summary + +Correct the process-manager exit order so a long-running agent does not produce +a false stderr-drain timeout warning. Keep the bounded post-exit drain and all +existing sanitized exit diagnostics. + +## In scope + +- Replace the manager's `Cmd.StderrPipe` with an explicitly owned stderr pipe. +- Change `Manager.waitForExit` to wait for process exit before stderr drain. +- Preserve generation-specific completion channels and the bounded timeout. +- Add regression coverage for running, successful, intentional-stop, and + non-zero-exit paths. + +## Out of scope + +- Changing the drain timeout value. +- Changing stderr sanitization, redaction, or ring-buffer limits. +- Changing process-group cleanup or ACP adapter behavior. + +## Acceptance + +- No stderr-drain timeout is emitted while a managed process is still running. +- Exit publication waits for the generation-specific stderr reader after + `cmd.Wait`, but remains bounded when the reader is held open. +- Exit code, error event, safe recent stderr, intentional-stop classification, + and process-group cleanup remain unchanged. + +## Verification + +```bash +cd apps/backend && go test ./internal/agentctl/server/process -run 'TestWaitForExit.*|TestReadStderr.*Generation' -count=1 +cd apps/backend && go test ./internal/agentctl/server/process -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/manager.go` +- `apps/backend/internal/agentctl/server/process/manager_stderr_exit_test.go` + +## Dependencies + +Task 01 is a prior plan wave. The code paths are independent, but the plan +keeps lifecycle changes sequential for review. + +## Risks + +- The manager must close its parent stderr writer after start and close its + reader if the bounded post-exit drain expires. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/platform/requirements/agent-process-exit-drain.md`. +- `docs/specs/platform/system-design/agent-process-exit-drain.md`. +- Existing generation-channel and sanitized-stderr tests. + +## Results + +Changed the manager to use an explicitly owned stderr pipe, then wait for the +child process before waiting on the generation-specific stderr reader. Added +regression coverage proving a live process does not emit the drain timeout +warning, exit diagnostics preserve stderr after `Cmd.Wait`, and the reader +drain remains bounded. + +Verification: + +- `go test ./internal/agentctl/server/process -run 'TestWaitForExit.*|TestReadStderr.*Generation' -count=1` passed. +- `go test ./internal/agentctl/server/process -count=1` passed with 707 tests. +- Race-enabled stderr exit tests passed, including owned-pipe preservation. diff --git a/docs/plans/startup-observability-cleanup/task-03-instance-stop.md b/docs/plans/startup-observability-cleanup/task-03-instance-stop.md new file mode 100644 index 0000000000..6b99157d5e --- /dev/null +++ b/docs/plans/startup-observability-cleanup/task-03-instance-stop.md @@ -0,0 +1,94 @@ +--- +id: "03-instance-stop" +title: "Idempotent agent instance stop" +status: done +wave: 3 +depends_on: + - "02-agent-stderr-exit" +plan: "plan.md" +requirements: + - REQ-PLATFORM-AGENTCTL-INSTANCE-STOP-001 +acceptance_criteria: + - AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.1 + - AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.2 + - AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.3 + - AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.4 +system_design: + - ../../specs/platform/system-design/agentctl-instance-stop.md +--- + +# Task 03: Idempotent agent instance stop + +## Summary + +Make an overlapping stop call succeed when the same instance has already been +fully removed by another stop operation. Keep unknown-instance responses and +real cleanup failures distinct. + +## In scope + +- Serialize and recheck the original instance pointer after `stopMu`. +- Treat removal of that same pointer as an already-satisfied stop. +- Release ports once and retain the current failure retry behavior. +- Add deterministic manager and control-boundary regression tests. + +## Out of scope + +- Changing the `ControlClient` lost-response retry contract. +- Changing process or HTTP-server grace periods. +- Changing instance persistence, IDs, or agent protocol behavior. + +## Acceptance + +- Overlapping stops for one instance do not produce a 500 solely because the + first stop removed the instance. +- Unknown IDs remain non-mutating 404 responses, and a different instance + reusing the ID is never mistaken for the original instance. +- Genuine cleanup failures remain HTTP 500/error-level and retain the instance + and port for retry; successful cleanup releases the port once. + +## Verification + +```bash +cd apps/backend && go test ./internal/agentctl/server/instance ./internal/agentctl/server/api -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/instance/manager.go` +- `apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go` +- `apps/backend/internal/agentctl/server/api/control_server.go` +- `apps/backend/internal/agentctl/server/api/*_test.go` + +## Dependencies + +Task 02 is a prior plan wave. No data migration or external service is +required. + +## Risks + +- A missing map entry is only benign when it follows teardown of the exact + pointer captured before waiting; an ID reuse must remain an error. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/platform/requirements/agentctl-instance-stop.md`. +- `docs/specs/platform/system-design/agentctl-instance-stop.md`. +- Existing `Manager.StopInstance` failure-retention tests and + `ControlClient.DeleteInstance` 404 retry test. + +## Results + +Refactored stop handling so the initial instance pointer is passed through a +serialized stop operation. A pointer that was already removed by a successful +stop now returns success, while a replacement pointer still returns not found. +Added manager concurrency, port-reuse, replacement-safety, and unknown-control- +route coverage. + +Verification: + +- `go test ./internal/agentctl/server/instance ./internal/agentctl/server/api -count=1` passed with 463 tests. diff --git a/docs/public/cli.md b/docs/public/cli.md index 7a177e0bf8..538e4793c6 100644 --- a/docs/public/cli.md +++ b/docs/public/cli.md @@ -231,6 +231,8 @@ The launcher derives readiness targets and the access URL from the effective `se When it can enumerate non-loopback interfaces, the launcher also prints `network:` URLs for each unique non-loopback, non-link-local address on the same port, including local-network and Tailscale addresses. IPv6 addresses are shown in brackets. If the effective bind restricts the listener, the launcher only prints matching addresses. These lines are informational and are omitted if interface discovery is unavailable. +The `dev`, `start`, and `run` startup banners also print a `version:` line. Its value matches `kandev --version`; an unstamped local build reports `dev`. + The backend's default `server.host` is `0.0.0.0`. That can expose Kandev to other machines on the network, and the current local product path is not an authenticated multi-user boundary. The launcher uses `localhost` for its default local access URL, but the backend still listens on every interface. Bind it to loopback unless remote access is deliberately protected: ```bash diff --git a/docs/specs/platform/README.md b/docs/specs/platform/README.md index 0850feb8ba..22b0acbd7a 100644 --- a/docs/specs/platform/README.md +++ b/docs/specs/platform/README.md @@ -34,6 +34,8 @@ localization, feature toggles, health, and shared session recovery services. +- [Agent process exit and stderr drain](requirements/agent-process-exit-drain.md) +- [Agentctl instance stop idempotency](requirements/agentctl-instance-stop.md) - [Agent Runtime Availability](requirements/agent-runtime-availability.md) - [Background Work Liveness](requirements/background-work-liveness.md) - [Bounded Task Status Delivery](requirements/bounded-task-status-delivery.md) @@ -70,6 +72,8 @@ localization, feature toggles, health, and shared session recovery services. +- [Agent process exit and stderr drain](system-design/agent-process-exit-drain.md) +- [Agentctl instance stop idempotency](system-design/agentctl-instance-stop.md) - [Browser console retention](system-design/browser-console-retention.md) - [Bounded Task Status Delivery](system-design/bounded-task-status-delivery.md) - [Diagnostic logging System Design Part 1](system-design/diagnostic-logging-01.md) @@ -81,6 +85,7 @@ localization, feature toggles, health, and shared session recovery services. - [Session MCP Attachment Observability](system-design/mcp-session-observability.md) - [Provider Error Recovery](system-design/provider-error-recovery.md) - [Workspace Git Status](system-design/workspace-git-status.md) +- [Go dev launcher and startup version](system-design/go-dev-launcher.md) ## Migration record diff --git a/docs/specs/platform/requirements/agent-process-exit-drain.md b/docs/specs/platform/requirements/agent-process-exit-drain.md new file mode 100644 index 0000000000..3c9904294a --- /dev/null +++ b/docs/specs/platform/requirements/agent-process-exit-drain.md @@ -0,0 +1,44 @@ +--- +status: draft +system: platform +created: 2026-08-26 +owners: + - kandev +--- +# Agent process exit and stderr drain Requirements + +## Overview + +The agent process manager reads child stderr while it supervises process exit. +The current order waits for stderr EOF before waiting for the child process. A +long-running agent therefore produces a false stderr-drain warning after one +second, even though the agent is still starting normally. This requirement keeps +exit diagnostics complete without making process shutdown unbounded. + +## Terminology + +- **stderr drain:** Completion of the generation-specific reader that consumes a + managed agent process's stderr pipe. +- **exit publication:** The process-manager state and update event that expose a + managed agent's final exit result. + +## Requirements + +### REQ-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001: Correct agent process exit drain sequencing + +**Intent:** Make the stderr-drain warning describe a real post-exit cleanup delay, +while preserving safe recent-stderr diagnostics and a bounded shutdown path. + +#### Acceptance criteria + +- **AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.1:** When a managed agent remains running longer than the stderr-drain timeout, the process manager shall not emit a stderr-drain timeout warning solely because the process is still running. +- **AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.2:** When a managed agent exits, the process manager shall wait for its generation-specific stderr reader, up to the existing bounded timeout, before publishing the final exit result. +- **AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.3:** When the stderr reader remains blocked after the managed process exits, the process manager shall emit a warning and continue exit handling within the existing bounded timeout. +- **AC-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001.4:** The exit code, error classification, safe recent-stderr content, process-group cleanup, and intentional-stop behavior shall remain unchanged. + +## Out of scope + +- Changing the stderr timeout duration. +- Changing stderr sanitization or the recent-stderr buffer contents. +- Changing ACP capability filtering, agent authentication, shell grace periods, + or process-group termination policy. diff --git a/docs/specs/platform/requirements/agentctl-instance-stop.md b/docs/specs/platform/requirements/agentctl-instance-stop.md new file mode 100644 index 0000000000..40611617c1 --- /dev/null +++ b/docs/specs/platform/requirements/agentctl-instance-stop.md @@ -0,0 +1,46 @@ +--- +status: draft +system: platform +created: 2026-08-26 +owners: + - kandev +--- +# Agentctl instance stop idempotency Requirements + +## Overview + +The agentctl control API can receive a second stop request while the first +request is finishing teardown. The instance manager removes the instance after +successful cleanup, so the second request currently reports a 500 even though +the requested stopped state is already true. This requirement makes repeated +stops converge on the same safe postcondition while preserving real cleanup +failures. + +## Terminology + +- **Already stopped:** The requested instance was successfully torn down and + removed by another stop operation before this operation completed. +- **Real cleanup failure:** HTTP-server or process-manager cleanup failed, so + the instance or its port must remain available for retry. + +## Requirements + +### REQ-PLATFORM-AGENTCTL-INSTANCE-STOP-001: Idempotent agentctl instance stop + +**Intent:** Prevent a teardown race from being reported as an internal failure +after the instance has already been stopped, without hiding an incomplete +cleanup operation. + +#### Acceptance criteria + +- **AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.1:** When a `DELETE /api/v1/instances/:id` request races with another stop for the same tracked instance and the other stop completes first, the request shall not return HTTP 500 solely because that instance is already stopped; the completed-stop outcome may be HTTP 200 or HTTP 404. +- **AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.2:** When a stop request observes an unknown instance that was not part of a completed stop, the control API shall retain its HTTP 404 response and shall not release an unrelated port or instance. +- **AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.3:** When HTTP-server or process-manager cleanup fails, the control API shall retain its HTTP 500 response and error-level diagnostic, and the instance and its allocated port shall remain retryable. +- **AC-PLATFORM-AGENTCTL-INSTANCE-STOP-001.4:** When cleanup succeeds, the instance shall be removed from tracking and its port shall be released at most once, including when duplicate stop calls overlap. + +## Out of scope + +- Changing the existing `ControlClient` treatment of HTTP 404 after a lost + delete response. +- Changing process termination grace periods or agent protocol behavior. +- Adding persistence for stopped instances. diff --git a/docs/specs/platform/requirements/go-dev-launcher.md b/docs/specs/platform/requirements/go-dev-launcher.md index 743730a440..36bfc88738 100644 --- a/docs/specs/platform/requirements/go-dev-launcher.md +++ b/docs/specs/platform/requirements/go-dev-launcher.md @@ -27,6 +27,7 @@ owners: - **AC-PLATFORM-GO-DEV-LAUNCHER-001.6:** **GIVEN** a host with an IPv6 address, **WHEN** the startup banner is printed, **THEN** the address is rendered as `http://[
]:`. - **AC-PLATFORM-GO-DEV-LAUNCHER-001.7:** **GIVEN** interface enumeration returns an error, **WHEN** the launcher starts, **THEN** it continues startup and prints the localhost URL without a `network:` line. - **AC-PLATFORM-GO-DEV-LAUNCHER-001.8:** **GIVEN** `KANDEV_SERVER_HOST` is a loopback or specific bind address, **WHEN** the startup banner is printed, **THEN** it omits unrelated network addresses. +- **AC-PLATFORM-GO-DEV-LAUNCHER-001.9:** **WHEN** the `dev`, `start`, or `run` launcher prints its startup banner, it shall include a `version:` line whose value is the same build version printed by `--version` for that binary. An unstamped build shall print `dev`. ## Migrated source detail @@ -142,6 +143,13 @@ cannot enumerate its interfaces, the launcher still starts and prints the localh URL. When `KANDEV_SERVER_HOST` restricts the backend to loopback or specific addresses, the banner omits unreachable interface addresses and only advertises matching binds. +### Startup output advertises the build version + +The startup banner for `dev`, `start`, and `run` also prints a `version:` line. Its +value is the launcher build version, the same value returned by `--version`; an +unstamped local binary reports `dev`. The line is informational and does not alter +the existing header, URL, database, or log-level lines. + ### Backend restart from the UI still rebuilds The restart supervisor (ADR 0019) writes a launch manifest and listens on a control @@ -250,3 +258,10 @@ them: building and serving the web app (Vite), the published npm shim, and repo **THEN** it continues startup and prints the localhost URL without a `network:` line. - **GIVEN** `KANDEV_SERVER_HOST` is a loopback or specific bind address, **WHEN** the startup banner is printed, **THEN** it omits unrelated network addresses. +- **GIVEN** a stamped or unstamped launcher binary, **WHEN** `dev`, `start`, or `run` + prints its startup banner, **THEN** the banner includes `version:` with the same + value that `--version` prints, using `dev` for an unstamped binary. + +## System design + +The technical design for launcher startup identity is in [Go dev launcher and startup version](../system-design/go-dev-launcher.md). diff --git a/docs/specs/platform/system-design/agent-process-exit-drain.md b/docs/specs/platform/system-design/agent-process-exit-drain.md new file mode 100644 index 0000000000..4d1813f423 --- /dev/null +++ b/docs/specs/platform/system-design/agent-process-exit-drain.md @@ -0,0 +1,83 @@ +--- +status: draft +system: platform +requirements: + - REQ-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001 +created: 2026-08-26 +owners: + - kandev +--- +# Agent process exit and stderr drain System Design + +## Purpose and boundaries + +The agentctl process manager owns the child process, an explicitly managed +stderr pipe, exit status, and safe recent-stderr diagnostics. This design +changes the stderr pipe ownership and the order in which process exit and +stderr completion are observed. It does not change agent protocol handling, +stderr sanitization, or process-group cleanup. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-PLATFORM-AGENT-PROCESS-EXIT-DRAIN-001` | [Exit flow](#exit-flow) and [Failure and recovery](#failure-and-recovery) | + +## Components and responsibilities + +- `internal/agentctl/server/process.Manager` starts the managed command and + owns its lifecycle. +- `Manager.startProcessPipes` creates an `os.Pipe`, assigns its writer to the + command, and retains the reader and writer for explicit lifecycle cleanup. +- `Manager.readStderr` drains and sanitizes the current process generation's + stderr stream, appending only the safe projection to the ring buffer. +- `Manager.waitForExit` waits for the child process and then coordinates the + generation-specific `stderrDone` channel before publishing exit diagnostics. +- The adapter update channel receives the existing exit error event and recent + stderr fields. + +## Data and contracts + +The process manager keeps `processStderrDrainTimeout` as the upper bound for a +reader that does not finish after process exit. `stderrDone` remains local to a +process generation so a delayed reader from an old command cannot close a +replacement command's completion channel. The command receives the owned +stderr writer as an `*os.File`, so `Cmd.Wait` can reap the child without +closing the manager's reader before the reader drains. The parent writer is +closed immediately after a successful start, and the reader is closed after a +normal drain or when the bounded drain expires. + +No HTTP, WebSocket, database, or agent protocol payload changes are required. + +## Exit flow + +1. Process startup creates the stdin and stdout pipes plus an explicitly owned + stderr `os.Pipe`, closes the parent stderr writer after the command starts, + and starts `readStderr` and `waitForExit` concurrently. +2. `readStderr` continues draining while the child is alive, so a child that + runs longer than the drain timeout does not create a timeout warning merely + because it has not exited. +3. `waitForExit` calls `cmd.Wait()` to observe the child exit. +4. After `cmd.Wait()` returns, `waitForExit` waits on the same generation's + `stderrDone` channel for the bounded drain interval. Because the stderr + reader is not a `Cmd.StderrPipe`, `Cmd.Wait` does not close it first. +5. Exit classification, recent-stderr event construction, process-group reap, + and final stopped status follow the existing paths. + +## Failure and recovery + +If the reader finishes after process exit, final exit diagnostics include the +same sanitized recent stderr. If the reader remains blocked, the manager emits +`timed out waiting for agent stderr to drain`, closes its owned reader to +unblock the goroutine, and continues within the existing bound instead of +hanging process cleanup. That warning then indicates a real post-exit pipe- +drain delay. + +An intentional stop still uses the existing intentional-stop classification. +Non-zero process exits still retain their existing error event and exit code. + +## Observability + +The stderr-drain warning remains at warning level, but its occurrence is now +limited to the post-exit bounded wait. Existing process start, exit, and recent +stderr log fields remain unchanged. diff --git a/docs/specs/platform/system-design/agentctl-instance-stop.md b/docs/specs/platform/system-design/agentctl-instance-stop.md new file mode 100644 index 0000000000..772ceda275 --- /dev/null +++ b/docs/specs/platform/system-design/agentctl-instance-stop.md @@ -0,0 +1,77 @@ +--- +status: draft +system: platform +requirements: + - REQ-PLATFORM-AGENTCTL-INSTANCE-STOP-001 +created: 2026-08-26 +owners: + - kandev +--- +# Agentctl instance stop idempotency System Design + +## Purpose and boundaries + +The platform agentctl instance manager owns the lifecycle of each per-agent +HTTP server, process manager, and allocated port. The control server exposes +that lifecycle through `DELETE /api/v1/instances/:id`. This design makes a +completed duplicate stop benign while preserving failure and retry semantics. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-PLATFORM-AGENTCTL-INSTANCE-STOP-001` | [Stop flow](#stop-flow) and [Failure and recovery](#failure-and-recovery) | + +## Components and responsibilities + +- `internal/agentctl/server/api.ControlServer` validates the requested ID and + maps the manager result to the existing HTTP response. +- `internal/agentctl/server/instance.Manager` snapshots the tracked instance, + serializes teardown with the instance's `stopMu`, and owns map and port + mutation. +- `instance.Instance` owns the per-instance stop lock and one-time port-release + state. +- `internal/agent/runtime/agentctl.ControlClient` retains its existing rule that + a 404 after a lost delete response satisfies the stopped postcondition. + +## Data and contracts + +The existing DELETE route and response bodies remain in use. An unknown ID is +still reported as HTTP 404. A stop that starts with a tracked instance and then +finds that the same instance has already been removed may complete as success; +an already-completed request that reaches the initial lookup after removal may +continue to receive HTTP 404. Neither completed-stop path is an HTTP 500. + +If the ID is reused for a different tracked instance while an old stop finishes, +the manager must not treat that different pointer as the old completed stop. +That safety check prevents one lifecycle from mutating another lifecycle. + +## Stop flow + +1. The control handler performs its existing lookup and returns 404 for an ID + that is absent before the stop begins. +2. `Manager.StopInstance` snapshots the instance pointer and serializes against + other stops for that pointer with `stopMu`. +3. After acquiring the lock, the manager rechecks the ID mapping. If the mapping + is absent because this same pointer was already torn down and removed, it + returns the already-stopped success outcome. If a different pointer occupies + the ID, it retains the safety error. +4. The first successful teardown closes admission, stops the HTTP server and + process manager, releases the port once, and removes the pointer from the + instance map. +5. The control handler returns the existing success response for a nil manager + result. Real manager errors retain the existing failure response. + +## Failure and recovery + +Unknown instances remain non-mutating 404 responses. Completed duplicate stops +do not log an error or return 500. If HTTP-server or process-manager cleanup +fails, the manager retains the instance and allocated port for retry, and the +control handler keeps the error-level diagnostic and HTTP 500 response. + +## Observability + +The normal completed-stop path continues to log successful instance removal. +The already-stopped race path may use debug-level diagnostic context, but it +must not emit the current `failed to stop instance` error with a stack trace. +Real teardown failures keep their current error-level log and request status. diff --git a/docs/specs/platform/system-design/go-dev-launcher.md b/docs/specs/platform/system-design/go-dev-launcher.md new file mode 100644 index 0000000000..7b1714e4ec --- /dev/null +++ b/docs/specs/platform/system-design/go-dev-launcher.md @@ -0,0 +1,77 @@ +--- +status: draft +system: platform +requirements: + - REQ-PLATFORM-GO-DEV-LAUNCHER-001 +created: 2026-08-26 +owners: + - kandev +--- +# Go dev launcher and startup version System Design + +## Purpose and boundaries + +The platform launcher owns the startup banner shown by `dev`, `start`, and +`run`. This design adds build identity to the existing banner without changing +the backend health contract, service installation metadata, or network binding +behavior documented by the platform launcher requirement. + +The launcher consumes build metadata from the native binary entrypoint. It does +not resolve a version from the database, the selected bundle directory, or an +environment variable used only for release display metadata. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-PLATFORM-GO-DEV-LAUNCHER-001` | [Build identity](#build-identity) and [Startup flow](#startup-flow) | + +## Components and responsibilities + +- `apps/backend/cmd/kandev` owns the build-time `Version` variable and passes it + into `launcher.BuildInfo`. +- `internal/launcher.Run` selects the command and forwards the build version to + the selected launch path. +- `internal/launcher.runStart` and `internal/launcher.runInstalled` carry the + version in `managedAppConfig`. +- `internal/launcher.runDev` carries the version through the dev launch path. +- `internal/launcher.logStartup` renders the shared stdout banner. + +## Build identity + +`launcher.BuildInfo.Version` is the canonical value for the launcher banner. It +is the same value used by the native `--version` path and by the backend build +metadata when the binary is dispatched with `__backend`. The compiled default +is `dev`, so a local or otherwise unstamped binary never prints an empty version. + +The existing `KANDEV_VERSION` value remains optional release display metadata for +the installed-release header. It does not override the binary build identity +shown on the `version:` line. + +## Startup flow + +Each shared launch mode resolves ports, endpoints, database path, and log level +before it calls the banner. The banner prints the existing header first, then a +single line in this shape: + +```text +[kandev] version: +``` + +The line appears before the URL, network, MCP, database, and log-level lines. +Those existing lines retain their current order and values. The version line is +printed before child startup so it remains available when backend readiness +fails. + +## Failure and recovery + +If build metadata is empty at an internal call boundary, the launcher uses the +same `dev` fallback as the native binary defaults. Version rendering cannot +prevent launch, change endpoint selection, or change backend readiness behavior. + +## Observability + +The startup banner exposes the build identity on launcher stdout. `--version` +remains the machine-readable single-value command. The backend `/health` and +`/api/v1/system/info` version contracts remain independent but use the same +build metadata for the dispatched backend process.