From 8c010ebecacfb791b75edaf3128f0bf42dbcf071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 23 Jul 2026 17:24:54 +0100 Subject: [PATCH] fix: inherit service temporary environment (#1894) --- .../server/api/vscode_handlers_test.go | 21 - .../agentctl/server/instance/manager.go | 11 +- .../server/instance/manager_shutdown_test.go | 30 +- .../agentctl/server/process/manager.go | 240 +------- .../server/process/manager_temp_test.go | 566 +++--------------- .../agentctl/server/process/manager_test.go | 27 +- .../agentctl/server/shell/session_test.go | 10 +- .../orchestrator/event_handlers_github.go | 3 + .../event_handlers_github_ci_automation.go | 6 +- apps/backend/internal/orchestrator/service.go | 6 +- apps/web/e2e/fixtures/backend.ts | 415 ++++++++----- apps/web/e2e/helpers/ssh.ts | 22 +- .../system/backend-fixture-lifecycle.spec.ts | 153 +++++ .../0045-install-wide-storage-maintenance.md | 57 +- docs/decisions/INDEX.md | 2 +- docs/plans/storage-maintenance/plan.md | 22 + .../task-10-inherit-service-temp.md | 56 ++ .../task-11-e2e-temp-cleanup.md | 63 ++ .../task-12-inherited-temp-docs.md | 61 ++ .../task-13-inherited-temp-verification.md | 79 +++ .../workspace-git-status-scalability/plan.md | 7 +- .../task-04-agent-temp-teardown.md | 7 +- docs/public/operations.md | 23 +- docs/specs/system-page/storage-maintenance.md | 52 +- 24 files changed, 963 insertions(+), 976 deletions(-) create mode 100644 apps/web/e2e/tests/system/backend-fixture-lifecycle.spec.ts create mode 100644 docs/plans/storage-maintenance/task-10-inherit-service-temp.md create mode 100644 docs/plans/storage-maintenance/task-11-e2e-temp-cleanup.md create mode 100644 docs/plans/storage-maintenance/task-12-inherited-temp-docs.md create mode 100644 docs/plans/storage-maintenance/task-13-inherited-temp-verification.md diff --git a/apps/backend/internal/agentctl/server/api/vscode_handlers_test.go b/apps/backend/internal/agentctl/server/api/vscode_handlers_test.go index d551586c89..1f2e25cbf9 100644 --- a/apps/backend/internal/agentctl/server/api/vscode_handlers_test.go +++ b/apps/backend/internal/agentctl/server/api/vscode_handlers_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "os" - "path/filepath" "strings" "testing" "time" @@ -73,26 +72,6 @@ func TestHandleVscodeStart_ManagerStopping(t *testing.T) { } } -func TestHandleVscodeStart_TempSetupFailure(t *testing.T) { - tempFile := filepath.Join(t.TempDir(), "not-a-directory") - if err := os.WriteFile(tempFile, []byte("file"), 0o600); err != nil { - t.Fatalf("write temp file: %v", err) - } - t.Setenv("TMPDIR", tempFile) - s := newTestServer(t) - - body, _ := json.Marshal(VscodeStartRequest{Theme: "dark"}) - req := httptest.NewRequest(http.MethodPost, "/api/v1/vscode/start", bytes.NewReader(body)) - req.Header.Set("Content-Type", "application/json") - w := httptest.NewRecorder() - - s.router.ServeHTTP(w, req) - - if w.Code != http.StatusInternalServerError { - t.Errorf("expected 500, got %d", w.Code) - } -} - func TestHandleVscodeStop_WhenNotRunning(t *testing.T) { s := newTestServer(t) diff --git a/apps/backend/internal/agentctl/server/instance/manager.go b/apps/backend/internal/agentctl/server/instance/manager.go index 1325173686..c9329b0a21 100644 --- a/apps/backend/internal/agentctl/server/instance/manager.go +++ b/apps/backend/internal/agentctl/server/instance/manager.go @@ -366,7 +366,7 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { } stopErr := errors.Join(httpStopErr, processStopErr) - if httpStopErr != nil || (processStopErr != nil && !canReleaseInstanceResources(processStopErr)) { + if httpStopErr != nil || processStopErr != nil { return stopErr } @@ -390,15 +390,6 @@ func (m *Manager) StopInstance(ctx context.Context, id string) error { return stopErr } -type instanceResourceReleaseError interface { - CanReleaseInstanceResources() bool -} - -func canReleaseInstanceResources(err error) bool { - var classified instanceResourceReleaseError - return errors.As(err, &classified) && classified.CanReleaseInstanceResources() -} - type instanceHTTPServer interface { Shutdown(context.Context) error Close() error 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 d453c67168..add68e781f 100644 --- a/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go +++ b/apps/backend/internal/agentctl/server/instance/manager_shutdown_test.go @@ -153,7 +153,7 @@ func TestStopInstanceRetainsPortWhenHTTPServerCloseFails(t *testing.T) { shutdownErr: context.DeadlineExceeded, closeErr: closeErr, }, - manager: &fakeProcessManager{stopErr: cleanupOnlyError{err: errors.New("temp cleanup failed")}}, + manager: &fakeProcessManager{stopErr: errors.New("process teardown failed")}, } mgr.mu.Lock() @@ -167,7 +167,7 @@ func TestStopInstanceRetainsPortWhenHTTPServerCloseFails(t *testing.T) { require.Error(t, err) } -func TestStopInstanceReleasesPortAfterTempCleanupFailure(t *testing.T) { +func TestStopInstanceRetainsPortAfterProcessTeardownFailure(t *testing.T) { log := newTestLogger(t) mgr := NewManager(&config.Config{ Ports: config.PortConfig{Base: 12345, Max: 12345}, @@ -176,8 +176,8 @@ func TestStopInstanceReleasesPortAfterTempCleanupFailure(t *testing.T) { port, err := mgr.portAlloc.Allocate("process-cleanup-failure") require.NoError(t, err) - processErr := errors.New("agent temp cleanup failed") - procMgr := &fakeProcessManager{stopErr: cleanupOnlyError{err: processErr}} + processErr := errors.New("process teardown failed") + procMgr := &fakeProcessManager{stopErr: legacyResourceReleaseError{err: processErr}} server := &fakeHTTPServer{} inst := &Instance{ ID: "process-cleanup-failure", @@ -196,17 +196,17 @@ func TestStopInstanceReleasesPortAfterTempCleanupFailure(t *testing.T) { require.True(t, procMgr.stopped) require.True(t, server.shutdown) _, ok := mgr.GetInstance(inst.ID) - require.True(t, ok, "cleanup-only failure must retain a retry tombstone") - reusedPort, err := mgr.portAlloc.Allocate("next-instance") - require.NoError(t, err) - require.Equal(t, port, reusedPort) + require.True(t, ok, "process teardown failure must retain the instance for retry") + _, err = mgr.portAlloc.Allocate("next-instance") + require.Error(t, err, "process teardown failure must retain the instance port") procMgr.stopErr = nil require.NoError(t, mgr.StopInstance(context.Background(), inst.ID)) _, ok = mgr.GetInstance(inst.ID) require.False(t, ok) - _, err = mgr.portAlloc.Allocate("third-instance") - require.Error(t, err, "retry must not release a port that was reassigned") + reusedPort, err := mgr.portAlloc.Allocate("third-instance") + require.NoError(t, err) + require.Equal(t, port, reusedPort) } func TestStopHTTPServerTreatsCanceledShutdownAsStoppedAfterClose(t *testing.T) { @@ -245,13 +245,15 @@ type fakeProcessManager struct { stopped bool } -type cleanupOnlyError struct { +// legacyResourceReleaseError proves a process teardown error cannot authorize +// releasing the instance's port while its process ownership is unresolved. +type legacyResourceReleaseError struct { err error } -func (e cleanupOnlyError) Error() string { return e.err.Error() } -func (e cleanupOnlyError) Unwrap() error { return e.err } -func (e cleanupOnlyError) CanReleaseInstanceResources() bool { +func (e legacyResourceReleaseError) Error() string { return e.err.Error() } +func (e legacyResourceReleaseError) Unwrap() error { return e.err } +func (e legacyResourceReleaseError) CanReleaseInstanceResources() bool { return true } diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 5d69f313c4..3b4d1f7916 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -4,7 +4,6 @@ package process import ( "bufio" "context" - "crypto/sha256" "errors" "fmt" "io" @@ -70,8 +69,6 @@ type PermissionNotification struct { // defaultStderrBufferSize is the number of recent stderr lines to keep for error context const defaultStderrBufferSize = 50 -const agentTempDirRoot = "kandev-agent" - const processKillRequiredExitGrace = 250 * time.Millisecond // processDefaultExitGrace is the no-deadline upper bound for graceful adapters @@ -183,15 +180,6 @@ type Manager struct { // Final command string (full command with all adapter args) finalCommand string - // agentTempRoot and agentTempDir record the exact temporary directory - // created for this manager. The retained Root keeps cleanup anchored to the - // directory opened at creation even if its pathname is later replaced. - agentTempRoot string - agentTempDir string - agentTempChild string - agentTempRootHandle *os.Root - agentTempMu sync.Mutex - // Synchronization mu sync.RWMutex wg sync.WaitGroup @@ -602,10 +590,6 @@ func (m *Manager) StartProcess(ctx context.Context, req StartProcessRequest) (*P if m.processRunner == nil { return nil, fmt.Errorf("process runner not available") } - if err := m.ensureAgentTempEnv(); err != nil { - return nil, err - } - req.Env = m.ownedProcessEnv(req.Env) return m.processRunner.Start(ctx, req) } @@ -819,11 +803,6 @@ func (m *Manager) Start(ctx context.Context) error { return fmt.Errorf("no agent command configured") } - if err := m.ensureAgentTempEnv(); err != nil { - m.status.Store(StatusError) - return err - } - // Build adapter config and create protocol adapter if err := m.buildAdapterConfig(); err != nil { m.status.Store(StatusError) @@ -908,11 +887,6 @@ func (m *Manager) Start(ctx context.Context) error { // startOneShot initialises a one-shot adapter without spawning a long-lived subprocess. // The adapter manages its own per-prompt subprocess lifecycle internally. func (m *Manager) startOneShot() error { - // One-shot adapters bypass buildFinalCommand, so restore temp env vars - // after StripEnv before their per-prompt subprocesses inherit Env. - if err := m.ensureAgentTempEnv(); err != nil { - return err - } if m.adapterCfg != nil && m.adapterCfg.OneShotConfig != nil { m.adapterCfg.OneShotConfig.Env = m.cfg.AgentEnv } @@ -1014,10 +988,6 @@ func (m *Manager) buildAdapterConfig() error { // buildFinalCommand assembles the full command args and creates the exec.Cmd. // The process group is set so child processes can be killed together. func (m *Manager) buildFinalCommand() error { - if err := m.ensureAgentTempEnv(); err != nil { - return err - } - extraArgs := m.adapter.PrepareCommandArgs() cmdArgs := make([]string, 0, len(m.cfg.AgentArgs)-1+len(extraArgs)) @@ -1111,167 +1081,6 @@ func formatEnvEntrySizes(entries []envEntrySize) string { return "[" + strings.Join(parts, ",") + "]" } -func (m *Manager) ensureAgentTempEnv() error { - m.agentTempMu.Lock() - defer m.agentTempMu.Unlock() - - root := filepath.Join(os.TempDir(), agentTempDirRoot) - child := agentTempDirName(m.cfg.SessionID, m.cfg.InstanceID, m.cfg.Port) - dir := filepath.Join(root, child) - if m.agentTempRootHandle != nil { - if m.agentTempRoot == root && m.agentTempDir == dir && m.agentTempChild == child { - for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - m.cfg.AgentEnv = upsertEnvValue(m.cfg.AgentEnv, key, dir) - } - return nil - } - return fmt.Errorf("agent temp ownership already initialized for %q", m.agentTempDir) - } - rootHandle, err := openOwnedTempRoot(root) - if err != nil { - return err - } - if err := ensureOwnedTempChild(rootHandle, child); err != nil { - _ = rootHandle.Close() - return fmt.Errorf("failed to create agent temp dir: %w", err) - } - m.agentTempRoot = root - m.agentTempDir = dir - m.agentTempChild = child - m.agentTempRootHandle = rootHandle - for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - m.cfg.AgentEnv = upsertEnvValue(m.cfg.AgentEnv, key, dir) - } - return nil -} - -func (m *Manager) ownedProcessEnv(env map[string]string) map[string]string { - m.agentTempMu.Lock() - defer m.agentTempMu.Unlock() - - managed := make(map[string]string, len(env)+3) - for key, value := range env { - managed[key] = value - } - for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - managed[key] = m.agentTempDir - } - return managed -} - -func openOwnedTempRoot(path string) (*os.Root, error) { - if err := os.Mkdir(path, 0o700); err != nil && !errors.Is(err, os.ErrExist) { - return nil, fmt.Errorf("failed to create agent temp root %q: %w", path, err) - } - pathInfo, err := os.Lstat(path) - if err != nil { - return nil, fmt.Errorf("failed to inspect agent temp root %q: %w", path, err) - } - if pathInfo.Mode()&os.ModeSymlink != 0 || !pathInfo.IsDir() { - return nil, fmt.Errorf("unsafe agent temp root %q: expected a directory, not a symlink", path) - } - root, err := os.OpenRoot(path) - if err != nil { - return nil, fmt.Errorf("failed to open agent temp root %q: %w", path, err) - } - openedInfo, err := root.Stat(".") - if err != nil || !os.SameFile(pathInfo, openedInfo) { - _ = root.Close() - return nil, fmt.Errorf("unsafe agent temp root %q: path changed while opening", path) - } - if err := root.Chmod(".", 0o700); err != nil { - _ = root.Close() - return nil, fmt.Errorf("failed to secure agent temp root %q: %w", path, err) - } - return root, nil -} - -func ensureOwnedTempChild(root *os.Root, name string) error { - if err := root.Mkdir(name, 0o700); err != nil && !errors.Is(err, os.ErrExist) { - return err - } - info, err := root.Lstat(name) - if err != nil { - return err - } - if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { - return fmt.Errorf("unsafe agent temp child %q: expected a directory, not a symlink", name) - } - return root.Chmod(name, 0o700) -} - -func (m *Manager) cleanupAgentTempDir() error { - m.agentTempMu.Lock() - defer m.agentTempMu.Unlock() - - root := filepath.Clean(m.agentTempRoot) - target := filepath.Clean(m.agentTempDir) - if m.agentTempRoot == "" && m.agentTempDir == "" { - return nil - } - if m.agentTempRoot == "" || m.agentTempDir == "" || target == root { - return fmt.Errorf("refusing to clean invalid agent temp dir %q under root %q", m.agentTempDir, m.agentTempRoot) - } - rel, err := filepath.Rel(root, target) - if err != nil || filepath.IsAbs(rel) || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return fmt.Errorf("refusing to clean agent temp dir outside root: target %q root %q", m.agentTempDir, m.agentTempRoot) - } - if rel != m.agentTempChild || strings.ContainsRune(rel, filepath.Separator) || m.agentTempRootHandle == nil { - return fmt.Errorf("refusing to clean unowned agent temp dir %q under root %q", m.agentTempDir, m.agentTempRoot) - } - if err := m.agentTempRootHandle.RemoveAll(rel); err != nil { - return fmt.Errorf("failed to remove agent temp dir %q: %w", target, err) - } - closeErr := m.agentTempRootHandle.Close() - m.agentTempRoot = "" - m.agentTempDir = "" - m.agentTempChild = "" - m.agentTempRootHandle = nil - if closeErr != nil { - return fmt.Errorf("failed to close agent temp root %q: %w", root, closeErr) - } - return nil -} - -func agentTempDirName(sessionID, instanceID string, port int) string { - name := strings.TrimSpace(sessionID) - if name == "" { - name = strings.TrimSpace(instanceID) - } - if name == "" && port > 0 { - name = fmt.Sprintf("port-%d", port) - } - if name == "" { - name = "default" - } - - var b strings.Builder - for _, r := range name { - if isAgentTempDirRune(r) { - b.WriteRune(r) - } else { - b.WriteByte('_') - } - } - cleaned := strings.Trim(b.String(), "._-") - if cleaned == "" { - cleaned = "default" - } - if len(cleaned) > 40 { - cleaned = cleaned[:40] - } - identity := fmt.Sprintf("%d:%s|%d:%s|%d", len(sessionID), sessionID, len(instanceID), instanceID, port) - digest := sha256.Sum256([]byte(identity)) - return fmt.Sprintf("%s-%x", cleaned, digest[:12]) -} - -func isAgentTempDirRune(r rune) bool { - return r >= 'a' && r <= 'z' || - r >= 'A' && r <= 'Z' || - r >= '0' && r <= '9' || - r == '.' || r == '_' || r == '-' -} - func upsertEnvValue(env []string, key, value string) []string { prefix := key + "=" next := make([]string, 0, len(env)+1) @@ -1322,7 +1131,6 @@ func (m *Manager) startAgentShell() { } shellCfg := shell.DefaultConfig(m.cfg.WorkDir) shellCfg.ShellCommand = preferredShellCommand(m.cfg.AgentEnv) - shellCfg.Env = m.ownedProcessEnv(nil) shellSession, err := shell.NewSession(shellCfg, m.logger) if err != nil { m.logger.Warn("failed to create shell session", zap.Error(err)) @@ -1489,20 +1297,20 @@ func (m *Manager) GetSessionID() string { // Stop stops the agent process func (m *Manager) Stop(ctx context.Context) error { - return m.stop(ctx, false) + return m.stop(ctx) } -// StopForTeardown permanently closes process admission, drains prior owners, -// and removes the manager's temp directory after every owned process is reaped. +// StopForTeardown permanently closes process admission and drains prior owners +// before stopping every process owned by the manager. func (m *Manager) StopForTeardown(ctx context.Context) error { m.CloseAdmission() if err := m.WaitForAdmission(ctx); err != nil { return fmt.Errorf("wait for process admission to drain: %w", err) } - return m.stop(ctx, true) + return m.stop(ctx) } -func (m *Manager) stop(ctx context.Context, terminalStop bool) error { +func (m *Manager) stop(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() @@ -1529,9 +1337,6 @@ func (m *Manager) stop(ctx context.Context, terminalStop bool) error { } m.mainReapPending.Store(false) } - if terminalStop { - return classifyTempCleanupError(m.cleanupAgentTempDir()) - } return nil } return nil @@ -1557,29 +1362,9 @@ func (m *Manager) stop(ctx context.Context, terminalStop bool) error { return stopErr } m.mainReapPending.Store(false) - if terminalStop { - return classifyTempCleanupError(m.cleanupAgentTempDir()) - } return nil } -type tempCleanupError struct { - err error -} - -func (e tempCleanupError) Error() string { return e.err.Error() } -func (e tempCleanupError) Unwrap() error { return e.err } -func (e tempCleanupError) CanReleaseInstanceResources() bool { - return true -} - -func classifyTempCleanupError(err error) error { - if err == nil { - return nil - } - return tempCleanupError{err: err} -} - func (m *Manager) agentPID() int { if m.cmd == nil || m.cmd.Process == nil { return 0 @@ -2338,13 +2123,8 @@ func (m *Manager) StartShell() error { if !m.cfg.ShellEnabled { return nil } - if err := m.ensureAgentTempEnv(); err != nil { - return err - } - shellCfg := shell.DefaultConfig(m.cfg.WorkDir) shellCfg.ShellCommand = preferredShellCommand(m.cfg.AgentEnv) - shellCfg.Env = m.ownedProcessEnv(nil) shellSession, err := shell.NewSession(shellCfg, m.logger) if err != nil { return fmt.Errorf("failed to create shell session: %w", err) @@ -2355,17 +2135,13 @@ func (m *Manager) StartShell() error { return nil } -// StartTerminalShell creates a managed per-terminal shell with the instance temp environment. +// StartTerminalShell creates a managed per-terminal shell. func (m *Manager) StartTerminalShell(terminalID string, cfg shell.Config) (*shell.Session, error) { release, err := m.admitStart() if err != nil { return nil, err } defer release() - if err := m.ensureAgentTempEnv(); err != nil { - return nil, err - } - cfg.Env = m.ownedProcessEnv(cfg.Env) return m.shellMgr.Start(terminalID, cfg) } @@ -2378,9 +2154,6 @@ func (m *Manager) StartVscode(_ context.Context, theme string) error { return err } defer release() - if err := m.ensureAgentTempEnv(); err != nil { - return err - } return m.startVscode(theme) } @@ -2405,7 +2178,6 @@ func (m *Manager) startVscode(theme string) error { strategy := codeServerInstallStrategy(m.logger) m.vscode = NewVscodeManager(command, m.cfg.WorkDir, theme, strategy, m.logger) - m.vscode.setEnv(m.ownedProcessEnv(nil)) m.vscode.Start() return nil } diff --git a/apps/backend/internal/agentctl/server/process/manager_temp_test.go b/apps/backend/internal/agentctl/server/process/manager_temp_test.go index 1e20864064..9b5088e971 100644 --- a/apps/backend/internal/agentctl/server/process/manager_temp_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_temp_test.go @@ -3,422 +3,90 @@ package process import ( "context" "errors" - "io" "os" "os/exec" "path/filepath" - "runtime" "strings" - "sync" "testing" "time" - "github.com/kandev/kandev/internal/agentctl/server/adapter" "github.com/kandev/kandev/internal/agentctl/server/config" "github.com/kandev/kandev/internal/agentctl/server/shell" - "github.com/kandev/kandev/internal/agentctl/types" - v1 "github.com/kandev/kandev/pkg/api/v1" ) -type stubAgentAdapter struct{} - -type notifyingCloser struct { - closed chan struct{} - once sync.Once -} - -func (c *notifyingCloser) Write(p []byte) (int, error) { return len(p), nil } - -func (c *notifyingCloser) Close() error { - c.once.Do(func() { close(c.closed) }) - return nil -} - -func (stubAgentAdapter) PrepareEnvironment() (map[string]string, error) { return nil, nil } -func (stubAgentAdapter) PrepareCommandArgs() []string { return nil } -func (stubAgentAdapter) Connect(io.Writer, io.Reader) error { return nil } -func (stubAgentAdapter) Initialize(context.Context) error { return nil } -func (stubAgentAdapter) GetAgentInfo() *adapter.AgentInfo { return nil } -func (stubAgentAdapter) NewSession(context.Context, []types.McpServer) (string, error) { - return "", nil -} -func (stubAgentAdapter) LoadSession(context.Context, string, []types.McpServer) error { - return nil -} -func (stubAgentAdapter) Prompt(context.Context, string, []v1.MessageAttachment, uint64) error { - return nil -} -func (stubAgentAdapter) Cancel(context.Context) error { return nil } -func (stubAgentAdapter) Updates() <-chan adapter.AgentEvent { return nil } -func (stubAgentAdapter) GetSessionID() string { return "" } -func (stubAgentAdapter) GetOperationID() string { return "" } -func (stubAgentAdapter) SetPermissionHandler(adapter.PermissionHandler) {} -func (stubAgentAdapter) Close() error { return nil } -func (stubAgentAdapter) RequiresProcessKill() bool { return false } - func envValue(env []string, key string) string { prefix := key + "=" for _, item := range env { - if len(item) > len(prefix) && item[:len(prefix)] == prefix { - return item[len(prefix):] + if strings.HasPrefix(item, prefix) { + return strings.TrimPrefix(item, prefix) } } return "" } -func TestManager_BuildFinalCommandInjectsIsolatedTempDir(t *testing.T) { - setAgentTempTestEnv(t) +func hasEnvValue(env []string, key string) bool { + prefix := key + "=" + for _, item := range env { + if strings.HasPrefix(item, prefix) { + return true + } + } + return false +} + +func TestManager_BuildFinalCommandPreservesConfiguredTempEnvironment(t *testing.T) { + serviceTemp := setServiceTempTestEnv(t) mgr := NewManager(&config.InstanceConfig{ WorkDir: t.TempDir(), - SessionID: "session-123", AgentArgs: []string{"echo"}, - AgentEnv: []string{"PATH=/usr/bin"}, + AgentEnv: []string{ + "PATH=/usr/bin", + "TMPDIR=/configured/tmpdir", + "TMP=/configured/tmp", + "TEMP=/configured/temp", + }, }, newTestLogger(t)) - mgr.adapter = stubAgentAdapter{} - t.Cleanup(func() { _ = mgr.StopForTeardown(context.Background()) }) + mgr.adapter = newStubAdapter() + + if err := mgr.buildFinalCommand(); err != nil { + t.Fatalf("buildFinalCommand() error = %v", err) + } + + want := map[string]string{ + "TMPDIR": "/configured/tmpdir", + "TMP": "/configured/tmp", + "TEMP": "/configured/temp", + } + for key, value := range want { + if got := envValue(mgr.cmd.Env, key); got != value { + t.Fatalf("%s = %q, want configured service value %q", key, got, value) + } + } + assertNoAgentTempRoot(t, serviceTemp) +} + +func TestManager_BuildFinalCommandLeavesUnsetTempEnvironmentUnset(t *testing.T) { + serviceTemp := setServiceTempTestEnv(t) + mgr := NewManager(&config.InstanceConfig{ + WorkDir: t.TempDir(), + AgentArgs: []string{"echo"}, + AgentEnv: []string{"PATH=/usr/bin"}, + }, newTestLogger(t)) + mgr.adapter = newStubAdapter() if err := mgr.buildFinalCommand(); err != nil { t.Fatalf("buildFinalCommand() error = %v", err) } - want := filepath.Join(os.TempDir(), "kandev-agent", agentTempDirName("session-123", "", 0)) for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - if got := envValue(mgr.cmd.Env, key); got != want { - t.Fatalf("%s = %q, want %q", key, got, want) + if hasEnvValue(mgr.cmd.Env, key) { + t.Fatalf("%s unexpectedly added to child environment: %q", key, mgr.cmd.Env) } } - if info, err := os.Stat(want); err != nil || !info.IsDir() { - t.Fatalf("expected temp dir %q to exist, stat=%v err=%v", want, info, err) - } + assertNoAgentTempRoot(t, serviceTemp) } -func TestManager_OwnedProcessEnvOverridesRequestedTemp(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-child-env", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - - env := mgr.ownedProcessEnv(map[string]string{ - "TMPDIR": "unmanaged", - "CUSTOM": "preserved", - }) - for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - if got := env[key]; got != mgr.agentTempDir { - t.Fatalf("%s = %q, want %q", key, got, mgr.agentTempDir) - } - } - if got := env["CUSTOM"]; got != "preserved" { - t.Fatalf("CUSTOM = %q, want preserved", got) - } - if err := mgr.StopForTeardown(context.Background()); err != nil { - t.Fatalf("StopForTeardown() error = %v", err) - } -} - -func TestManager_StopRemovesAlreadyStoppedTempDirAndPreservesSibling(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-owned", - }, newTestLogger(t)) - - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := envValue(mgr.cfg.AgentEnv, "TMPDIR") - sibling := filepath.Join(filepath.Dir(owned), "session-sibling") - if err := os.MkdirAll(sibling, 0o700); err != nil { - t.Fatalf("create sibling temp dir: %v", err) - } - - if err := mgr.StopForTeardown(context.Background()); err != nil { - t.Fatalf("Stop() error = %v", err) - } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("owned temp dir still exists after Stop(), err = %v", err) - } - if info, err := os.Stat(sibling); err != nil || !info.IsDir() { - t.Fatalf("sibling temp dir was changed, stat = %v, err = %v", info, err) - } - if info, err := os.Stat(filepath.Dir(owned)); err != nil || !info.IsDir() { - t.Fatalf("shared temp root was changed, stat = %v, err = %v", info, err) - } -} - -func TestManager_StopRemovesTempOnlyAfterProcessTeardown(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-running", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := envValue(mgr.cfg.AgentEnv, "TMPDIR") - - stdin := ¬ifyingCloser{closed: make(chan struct{})} - mgr.stdin = stdin - mgr.status.Store(StatusRunning) - reap := make(chan struct{}) - var releaseOnce sync.Once - release := func() { releaseOnce.Do(func() { close(reap) }) } - mgr.wg.Add(1) - go func() { - defer mgr.wg.Done() - <-reap - }() - t.Cleanup(release) - - stopDone := make(chan error, 1) - go func() { stopDone <- mgr.StopForTeardown(context.Background()) }() - <-stdin.closed - if info, err := os.Stat(owned); err != nil || !info.IsDir() { - t.Fatalf("temp dir removed before process reap, stat = %v, err = %v", info, err) - } - - release() - if err := <-stopDone; err != nil { - t.Fatalf("Stop() error = %v", err) - } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("owned temp dir still exists after process reap, err = %v", err) - } -} - -func TestManager_StopUsesStoredTempOwnershipInsteadOfMutableEnvironment(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-stored", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := envValue(mgr.cfg.AgentEnv, "TMPDIR") - outside := filepath.Join(t.TempDir(), "must-remain") - if err := os.MkdirAll(outside, 0o700); err != nil { - t.Fatalf("create outside dir: %v", err) - } - mgr.cfg.AgentEnv = upsertEnvValue(mgr.cfg.AgentEnv, "TMPDIR", outside) - - if err := mgr.StopForTeardown(context.Background()); err != nil { - t.Fatalf("Stop() error = %v", err) - } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("stored owned temp dir still exists, err = %v", err) - } - if info, err := os.Stat(outside); err != nil || !info.IsDir() { - t.Fatalf("environment-selected outside dir was changed, stat = %v, err = %v", info, err) - } -} - -func TestManager_StopRejectsInvalidTempCleanupTargets(t *testing.T) { - base := t.TempDir() - root := filepath.Join(base, agentTempDirRoot) - outside := filepath.Join(base, "outside") - for _, path := range []string{root, outside} { - if err := os.MkdirAll(path, 0o700); err != nil { - t.Fatalf("create fixture dir %q: %v", path, err) - } - } - - tests := []struct { - name string - root string - target string - }{ - {name: "empty target", root: root}, - {name: "shared root", root: root, target: root}, - {name: "outside root", root: root, target: outside}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) - mgr.agentTempRoot = tt.root - mgr.agentTempDir = tt.target - - err := mgr.StopForTeardown(context.Background()) - if err == nil || !strings.Contains(err.Error(), "refusing to clean") { - t.Fatalf("Stop() error = %v, want fail-closed cleanup error", err) - } - }) - } - - for _, path := range []string{root, outside} { - if info, err := os.Stat(path); err != nil || !info.IsDir() { - t.Fatalf("invalid cleanup changed %q, stat = %v, err = %v", path, info, err) - } - } -} - -func TestManager_StopRejectsTempCleanupWithoutOwnershipHandle(t *testing.T) { - base := t.TempDir() - root := filepath.Join(base, agentTempDirRoot) - if err := os.Mkdir(root, 0o700); err != nil { - t.Fatalf("create root dir: %v", err) - } - mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) - mgr.agentTempRoot = root - mgr.agentTempDir = filepath.Join(root, "session\x00invalid") - - err := mgr.StopForTeardown(context.Background()) - if err == nil || !strings.Contains(err.Error(), "refusing to clean unowned") { - t.Fatalf("Stop() error = %v, want fail-closed ownership error", err) - } - if mgr.agentTempDir == "" { - t.Fatal("cleanup ownership cleared after failed removal") - } -} - -func TestManager_EnsureTempRejectsSymlinkedSharedRoot(t *testing.T) { - tempBase := setAgentTempTestEnv(t) - outside := t.TempDir() - root := filepath.Join(tempBase, agentTempDirRoot) - if err := os.Symlink(outside, root); err != nil { - t.Skipf("create symlink fixture: %v", err) - } - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-symlink", - }, newTestLogger(t)) - - err := mgr.ensureAgentTempEnv() - if err == nil || !strings.Contains(err.Error(), "agent temp root") { - t.Fatalf("ensureAgentTempEnv() error = %v, want unsafe-root error", err) - } - if _, err := os.Stat(filepath.Join(outside, "session-symlink")); !os.IsNotExist(err) { - t.Fatalf("ensureAgentTempEnv() traversed symlinked root, err = %v", err) - } -} - -func TestManager_StopUsesOpenedRootAfterPathIsReplaced(t *testing.T) { - tempBase := setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-replaced-root", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - - root := filepath.Join(tempBase, agentTempDirRoot) - parkedRoot := filepath.Join(tempBase, "original-kandev-agent") - if err := os.Rename(root, parkedRoot); err != nil { - if runtime.GOOS == "windows" { - if stopErr := mgr.StopForTeardown(context.Background()); stopErr != nil { - t.Fatalf("StopForTeardown() after protected-root rename = %v", stopErr) - } - return - } - t.Fatalf("rename original temp root: %v", err) - } - outside := t.TempDir() - outsideSession := filepath.Join(outside, "session-replaced-root") - if err := os.MkdirAll(outsideSession, 0o700); err != nil { - t.Fatalf("create outside session dir: %v", err) - } - if err := os.Symlink(outside, root); err != nil { - t.Skipf("create replacement symlink fixture: %v", err) - } - - ownedChild := mgr.agentTempChild - err := mgr.StopForTeardown(context.Background()) - if err != nil { - t.Fatalf("Stop() error = %v", err) - } - if info, err := os.Stat(outsideSession); err != nil || !info.IsDir() { - t.Fatalf("cleanup traversed replaced root, stat = %v, err = %v", info, err) - } - if _, err := os.Stat(filepath.Join(parkedRoot, ownedChild)); !os.IsNotExist(err) { - t.Fatalf("owned child under original root remains after cleanup, err = %v", err) - } -} - -func TestManager_TempNamesDoNotCollideAfterSanitization(t *testing.T) { - setAgentTempTestEnv(t) - first := NewManager(&config.InstanceConfig{ - InstanceID: "instance-first", - WorkDir: t.TempDir(), - SessionID: "a/b", - Port: 41001, - }, newTestLogger(t)) - second := NewManager(&config.InstanceConfig{ - InstanceID: "instance-second", - WorkDir: t.TempDir(), - SessionID: "a_b", - Port: 41002, - }, newTestLogger(t)) - for _, mgr := range []*Manager{first, second} { - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - } - if first.agentTempDir == second.agentTempDir { - t.Fatalf("distinct instance identities share temp dir %q", first.agentTempDir) - } - firstDir := first.agentTempDir - secondSentinel := filepath.Join(second.agentTempDir, "must-remain") - if err := os.WriteFile(secondSentinel, []byte("owned by second"), 0o600); err != nil { - t.Fatalf("create second sentinel: %v", err) - } - - if err := first.StopForTeardown(context.Background()); err != nil { - t.Fatalf("first Stop() error = %v", err) - } - if _, err := os.Stat(firstDir); !os.IsNotExist(err) { - t.Fatalf("first temp dir still exists after Stop(), err = %v", err) - } - if data, err := os.ReadFile(secondSentinel); err != nil || string(data) != "owned by second" { - t.Fatalf("first cleanup changed second temp dir, data = %q, err = %v", data, err) - } - if err := second.StopForTeardown(context.Background()); err != nil { - t.Fatalf("second Stop() error = %v", err) - } -} - -func TestManager_StoppedAgentKeepsTempUntilWorkspaceProcessReaped(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - InstanceID: "instance-natural-exit", - WorkDir: t.TempDir(), - SessionID: "session-natural-exit", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := mgr.agentTempDir - proc := &commandProcess{ - info: ProcessInfo{ID: "workspace-blocked-reap"}, - stopSignal: make(chan struct{}), - done: make(chan struct{}), - } - mgr.processRunner.processes[proc.info.ID] = proc - - stopDone := make(chan error, 1) - go func() { stopDone <- mgr.StopForTeardown(context.Background()) }() - select { - case <-proc.stopSignal: - case err := <-stopDone: - t.Fatalf("Stop() returned before workspace process stop/reap: %v", err) - } - if info, err := os.Stat(owned); err != nil || !info.IsDir() { - t.Fatalf("temp dir removed before workspace process reap, stat = %v, err = %v", info, err) - } - - close(proc.done) - if err := <-stopDone; err != nil { - t.Fatalf("Stop() error = %v", err) - } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("temp dir still exists after workspace process reap, err = %v", err) - } -} - -func TestManager_BeginStopWaitsForInFlightTempOwnerAdmission(t *testing.T) { +func TestManager_BeginStopWaitsForInFlightAdmission(t *testing.T) { mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) release, err := mgr.admitStart() if err != nil { @@ -431,7 +99,7 @@ func TestManager_BeginStopWaitsForInFlightTempOwnerAdmission(t *testing.T) { }() select { case <-stopAdmissionDone: - t.Fatal("BeginStop() returned before in-flight owner admission completed") + t.Fatal("BeginStop() returned before in-flight admission completed") default: } release() @@ -454,55 +122,8 @@ func TestManager_BeginStopWaitsForInFlightTempOwnerAdmission(t *testing.T) { } } -func TestManager_OrdinaryStopCannotCleanTempDuringTerminalAdmissionDrain(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-concurrent-stop", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := mgr.agentTempDir - release, err := mgr.admitStart() - if err != nil { - t.Fatalf("admitStart() error = %v", err) - } - mgr.CloseAdmission() - - if err := mgr.Stop(context.Background()); err != nil { - t.Fatalf("ordinary Stop() error = %v", err) - } - if info, err := os.Stat(owned); err != nil || !info.IsDir() { - t.Fatalf("ordinary Stop() removed terminal-owned temp dir, stat = %v, err = %v", info, err) - } - - terminalDone := make(chan error, 1) - go func() { terminalDone <- mgr.StopForTeardown(context.Background()) }() - select { - case err := <-terminalDone: - t.Fatalf("terminal stop returned before admission drained: %v", err) - default: - } - release() - if err := <-terminalDone; err != nil { - t.Fatalf("StopForTeardown() error = %v", err) - } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("terminal stop did not remove temp dir after drain, err = %v", err) - } -} - -func TestManager_TerminalAdmissionDrainHonorsContextAndRetries(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - WorkDir: t.TempDir(), - SessionID: "session-drain-timeout", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := mgr.agentTempDir +func TestManager_TeardownAdmissionDrainHonorsContextAndRetries(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) release, err := mgr.admitStart() if err != nil { t.Fatalf("admitStart() error = %v", err) @@ -514,30 +135,43 @@ func TestManager_TerminalAdmissionDrainHonorsContextAndRetries(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatalf("StopForTeardown() error = %v, want %v", err, context.Canceled) } - if info, statErr := os.Stat(owned); statErr != nil || !info.IsDir() { - t.Fatalf("canceled drain removed temp ownership, stat = %v, err = %v", info, statErr) - } release() if err := mgr.StopForTeardown(context.Background()); err != nil { t.Fatalf("StopForTeardown() retry error = %v", err) } - if _, err := os.Stat(owned); !os.IsNotExist(err) { - t.Fatalf("retry did not remove temp dir, err = %v", err) +} + +func TestManager_TeardownWaitsForWorkspaceProcessReap(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) + proc := &commandProcess{ + info: ProcessInfo{ID: "workspace-blocked-reap"}, + stopSignal: make(chan struct{}), + done: make(chan struct{}), + } + mgr.processRunner.processes[proc.info.ID] = proc + + stopDone := make(chan error, 1) + go func() { stopDone <- mgr.StopForTeardown(context.Background()) }() + select { + case <-proc.stopSignal: + case err := <-stopDone: + t.Fatalf("StopForTeardown() returned before workspace process stop/reap: %v", err) + } + select { + case err := <-stopDone: + t.Fatalf("StopForTeardown() returned before workspace process reap: %v", err) + default: + } + + close(proc.done) + if err := <-stopDone; err != nil { + t.Fatalf("StopForTeardown() error = %v", err) } } -func TestManager_TempCleanupWaitsForMainProcessGroupReap(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - InstanceID: "instance-main-reap-failure", - WorkDir: t.TempDir(), - SessionID: "session-main-reap-failure", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := mgr.agentTempDir +func TestManager_TeardownReportsMainProcessGroupReapFailure(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) mgr.cmd = &exec.Cmd{Process: &os.Process{Pid: 424242}} mgr.status.Store(StatusRunning) mgr.groupAliveFn = func(int) bool { return true } @@ -549,26 +183,15 @@ func TestManager_TempCleanupWaitsForMainProcessGroupReap(t *testing.T) { mgr.waitGroupExitFn = nil _ = mgr.StopForTeardown(context.Background()) }) + err := mgr.StopForTeardown(context.Background()) if err == nil || !strings.Contains(err.Error(), "remains alive") { - t.Fatalf("Stop() error = %v, want process-group reap failure", err) - } - if info, statErr := os.Stat(owned); statErr != nil || !info.IsDir() { - t.Fatalf("temp dir removed after failed main reap, stat = %v, err = %v", info, statErr) + t.Fatalf("StopForTeardown() error = %v, want process-group reap failure", err) } } -func TestManager_TempCleanupWaitsForMainGoroutineReap(t *testing.T) { - setAgentTempTestEnv(t) - mgr := NewManager(&config.InstanceConfig{ - InstanceID: "instance-main-wait-failure", - WorkDir: t.TempDir(), - SessionID: "session-main-wait-failure", - }, newTestLogger(t)) - if err := mgr.ensureAgentTempEnv(); err != nil { - t.Fatalf("ensureAgentTempEnv() error = %v", err) - } - owned := mgr.agentTempDir +func TestManager_TeardownReportsMainGoroutineReapFailure(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) mgr.cmd = &exec.Cmd{Process: &os.Process{Pid: 424243}} mgr.status.Store(StatusRunning) mgr.wg.Add(1) @@ -581,16 +204,14 @@ func TestManager_TempCleanupWaitsForMainGoroutineReap(t *testing.T) { mgr.terminateGroupFn = func(int) error { return nil } mgr.killGroupFn = func(int) error { return nil } mgr.managerWaitFn = func(context.Context, <-chan struct{}, time.Duration) bool { return false } + err := mgr.StopForTeardown(context.Background()) if err == nil || !strings.Contains(err.Error(), "goroutines were not reaped") { - t.Fatalf("Stop() error = %v, want manager reap failure", err) - } - if info, statErr := os.Stat(owned); statErr != nil || !info.IsDir() { - t.Fatalf("temp dir removed after failed manager reap, stat = %v, err = %v", info, statErr) + t.Fatalf("StopForTeardown() error = %v, want manager reap failure", err) } } -func setAgentTempTestEnv(t *testing.T) string { +func setServiceTempTestEnv(t *testing.T) string { t.Helper() root := t.TempDir() for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { @@ -598,3 +219,10 @@ func setAgentTempTestEnv(t *testing.T) string { } return root } + +func assertNoAgentTempRoot(t *testing.T, serviceTemp string) { + t.Helper() + if _, err := os.Stat(filepath.Join(serviceTemp, "kandev-agent")); !os.IsNotExist(err) { + t.Fatalf("unexpected kandev-agent root, err = %v", err) + } +} diff --git a/apps/backend/internal/agentctl/server/process/manager_test.go b/apps/backend/internal/agentctl/server/process/manager_test.go index 8c4b85f6c4..7f1b524b31 100644 --- a/apps/backend/internal/agentctl/server/process/manager_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_test.go @@ -6,7 +6,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "slices" "strings" "syscall" @@ -232,21 +231,28 @@ func TestBuildAdapterConfig_StripEnvRemovesDeclaredVars(t *testing.T) { } } -func TestStartOneShotRestoresTempEnvAfterStrip(t *testing.T) { +func TestStartOneShotPreservesConfiguredTempEnvironment(t *testing.T) { log := newTestLogger(t) workDir := t.TempDir() + serviceTemp := setServiceTempTestEnv(t) + agentEnv := []string{ + "PATH=/usr/bin", + "TMPDIR=/configured/tmpdir", + "TMP=/configured/tmp", + "TEMP=/configured/temp", + } m := &Manager{ cfg: &config.InstanceConfig{ SessionID: "session-1", WorkDir: workDir, - AgentEnv: []string{"PATH=/usr/bin"}, + AgentEnv: agentEnv, }, logger: log, adapter: newOneShotStubAdapter(), adapterCfg: &adapter.Config{ OneShotConfig: &adapter.OneShotConfig{ - Env: []string{"PATH=/usr/bin"}, + Env: agentEnv, WorkDir: workDir, }, }, @@ -262,10 +268,15 @@ func TestStartOneShotRestoresTempEnvAfterStrip(t *testing.T) { m.workspaceTracker.Stop() }) - wantDir := filepath.Join(os.TempDir(), agentTempDirRoot, agentTempDirName("session-1", "", 0)) - for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - if got := lookupEnvValue(m.adapterCfg.OneShotConfig.Env, key); got != wantDir { - t.Fatalf("OneShotConfig.Env %s = %q, want %q", key, got, wantDir) + want := map[string]string{ + "TMPDIR": "/configured/tmpdir", + "TMP": "/configured/tmp", + "TEMP": "/configured/temp", + } + for key, value := range want { + if got := lookupEnvValue(m.adapterCfg.OneShotConfig.Env, key); got != value { + t.Fatalf("OneShotConfig.Env %s = %q, want configured service value %q", key, got, value) } } + assertNoAgentTempRoot(t, serviceTemp) } diff --git a/apps/backend/internal/agentctl/server/shell/session_test.go b/apps/backend/internal/agentctl/server/shell/session_test.go index 30b0623d64..14aa989755 100644 --- a/apps/backend/internal/agentctl/server/shell/session_test.go +++ b/apps/backend/internal/agentctl/server/shell/session_test.go @@ -582,17 +582,17 @@ func TestBuildShellEnv(t *testing.T) { } } -func TestBuildShellEnvOverridesManagedTemp(t *testing.T) { +func TestBuildShellEnvAppliesExplicitTempOverrides(t *testing.T) { workDir := t.TempDir() t.Setenv("TMPDIR", "unmanaged") env := buildShellEnv(workDir, map[string]string{ - "TMPDIR": "/tmp/kandev-agent/session", - "TMP": "/tmp/kandev-agent/session", - "TEMP": "/tmp/kandev-agent/session", + "TMPDIR": "/srv/kandev/shared-tmp", + "TMP": "/srv/kandev/shared-tmp", + "TEMP": "/srv/kandev/shared-tmp", }) for _, key := range []string{"TMPDIR", "TMP", "TEMP"} { - want := key + "=/tmp/kandev-agent/session" + want := key + "=/srv/kandev/shared-tmp" found := false for _, entry := range env { if entry == want { diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 79404e1fae..a58dba59cc 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -22,6 +22,9 @@ import ( const ( issueWatchIDKey = "issue_watch_id" issueNumberKey = "issue_number" + githubPRStateOpen = "open" + githubPRStateClosed = "closed" + githubPRStateMerged = "merged" ciAutomationDetachedTimeout = 2 * time.Minute ) diff --git a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go index 5d43e284e5..5922bfdacd 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go @@ -446,14 +446,14 @@ func (s *Service) recordCIAutomationError(ctx context.Context, pr *github.TaskPR } func ciAutomationCanAutoFixFromFeedback(pr *github.TaskPR) bool { - return pr != nil && pr.State != "closed" && pr.State != "merged" + return pr != nil && pr.State != githubPRStateClosed && pr.State != githubPRStateMerged } func ciAutomationCanAutoFixFromFeedbackPR(feedback *github.PRFeedback) bool { if feedback == nil || feedback.PR == nil { return true } - return feedback.PR.State != "closed" && feedback.PR.State != "merged" + return feedback.PR.State != githubPRStateClosed && feedback.PR.State != githubPRStateMerged } func ciAutomationChecksSettledForAutoFix(pr *github.TaskPR, feedback *github.PRFeedback) bool { @@ -517,7 +517,7 @@ func ciAutomationShouldIncludeFeedbackComment(pr *github.TaskPR, comment github. } func ciAutomationReadyToMerge(pr *github.TaskPR) bool { - if pr == nil || pr.State != "open" { + if pr == nil || pr.State != githubPRStateOpen { return false } if pr.ChecksState != ciAutomationCheckSuccess || pr.MergeableState != "clean" { diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 851a77d24c..0399a98f6e 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -1610,7 +1610,7 @@ func taskPRState(prs []*github.TaskPR, repositoryID, branch string) (string, boo } matched = true state = strings.ToLower(strings.TrimSpace(pr.State)) - if state != "open" && state != "closed" && state != "merged" { + if state != githubPRStateOpen && state != githubPRStateClosed && state != githubPRStateMerged { return "", true } } @@ -1642,14 +1642,14 @@ func (s *Service) handleSessionLaunchFailed(ctx context.Context, taskID, session branch := extractMissingBranchName(launchErr) prState := s.matchingTaskPRState(ctx, taskID, repositoryID, branch) - if prState == "open" { + if prState == githubPRStateOpen { return } content := "Kandev couldn't fetch the requested branch from the configured repository. Verify the task's repository branch or PR link, then retry." if branch != "" { content = "Kandev couldn't fetch branch \"" + branch + "\" from the configured repository. Verify the task's repository branch or PR link, then retry." } - authoritativeMissingBranch := prState == "closed" || prState == "merged" + authoritativeMissingBranch := prState == githubPRStateClosed || prState == githubPRStateMerged if authoritativeMissingBranch { content = "This task references a PR branch that no longer exists on remote." if branch != "" { diff --git a/apps/web/e2e/fixtures/backend.ts b/apps/web/e2e/fixtures/backend.ts index a68dfea258..dbec249d90 100644 --- a/apps/web/e2e/fixtures/backend.ts +++ b/apps/web/e2e/fixtures/backend.ts @@ -50,23 +50,57 @@ export type BackendContext = { restart: (envOverrides?: Record) => Promise; }; -async function waitForHealth(url: string, timeoutMs: number, proc?: ChildProcess): Promise { - const deadline = Date.now() + timeoutMs; - - // Track process exit so we can fail fast instead of polling for the full timeout. - let processExited = false; - let processExitCode: number | null = null; +function observeProcessExit(proc?: ChildProcess): { + state: { exited: boolean; exitCode: number | null; error?: Error }; + dispose: () => void; +} { + const state: { exited: boolean; exitCode: number | null; error?: Error } = { + exited: proc?.exitCode !== null && proc?.exitCode !== undefined, + exitCode: proc?.exitCode ?? null, + }; const onExit = (code: number | null) => { - processExited = true; - processExitCode = code; + state.exited = true; + state.exitCode = code; + }; + const onError = (error: Error) => { + state.error = error; }; proc?.once("exit", onExit); + proc?.once("error", onError); + + // Close the gap between inspecting exitCode and subscribing to the event. + // Node records exitCode before emitting `exit`, so this second read catches + // a child that exited between those two operations. + if (proc?.exitCode !== null && proc?.exitCode !== undefined) { + state.exited = true; + state.exitCode = proc.exitCode; + } + + return { + state, + dispose: () => { + proc?.off("exit", onExit); + proc?.off("error", onError); + }, + }; +} + +export async function waitForHealth( + url: string, + timeoutMs: number, + proc?: ChildProcess, +): Promise { + const deadline = Date.now() + timeoutMs; + const processExit = observeProcessExit(proc); try { while (Date.now() < deadline) { - if (processExited) { + if (processExit.state.error) { + throw processExit.state.error; + } + if (processExit.state.exited) { throw new Error( - `Backend process exited with code ${processExitCode} while waiting for health at ${url}`, + `Backend process exited with code ${processExit.state.exitCode} while waiting for health at ${url}`, ); } try { @@ -79,7 +113,7 @@ async function waitForHealth(url: string, timeoutMs: number, proc?: ChildProcess } throw new Error(`Service did not become healthy at ${url} within ${timeoutMs}ms`); } finally { - proc?.off("exit", onExit); + processExit.dispose(); } } @@ -147,6 +181,61 @@ function killProcessGroup(proc: ChildProcess): Promise { }); } +type BackendFixtureLifecycle = { + stopProcess?: (proc: ChildProcess) => Promise; + removeTempRoot?: (tmpDir: string) => void; +}; + +function removeOwnedTempRoot(tmpDir: string): void { + fs.rmSync(tmpDir, { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 100, + }); +} + +export async function runOwnedBackendFixture( + tmpDir: string, + run: (registerProcess: (proc: ChildProcess) => void) => Promise, + lifecycle: BackendFixtureLifecycle = {}, +): Promise { + const stopProcess = lifecycle.stopProcess ?? killProcessGroup; + const removeTempRoot = lifecycle.removeTempRoot ?? removeOwnedTempRoot; + let backendProc: ChildProcess | undefined; + let result: T | undefined; + const failures: unknown[] = []; + + try { + result = await run((proc) => { + backendProc = proc; + }); + } catch (error) { + failures.push(error); + } + + if (backendProc) { + try { + await stopProcess(backendProc); + } catch (error) { + failures.push(error); + } + } + + try { + removeTempRoot(tmpDir); + } catch (error) { + failures.push(new Error(`Failed to remove E2E temporary root ${tmpDir}`, { cause: error })); + } + + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) { + throw new AggregateError(failures, "Backend fixture failed and cleanup did not complete"); + } + + return result as T; +} + /** * Spawn a backend process with the given environment. Returns the child process. * The process is spawned with `detached: true` so it becomes a process group leader. @@ -197,51 +286,54 @@ export const backendFixture = base.extend({ const tmpDir = fs.mkdtempSync( path.join(os.tmpdir(), `kandev-e2e-${workerInfo.workerIndex}-`), ); - const homeDir = path.join(tmpDir, ".kandev"); - const dbPath = path.join(tmpDir, "kandev.db"); - const worktreeBase = path.join(tmpDir, "worktrees"); - const repoCloneBase = path.join(tmpDir, "repos"); + let backendProc: ChildProcess | undefined; - fs.mkdirSync(homeDir, { recursive: true }); - fs.mkdirSync(worktreeBase, { recursive: true }); - fs.mkdirSync(repoCloneBase, { recursive: true }); + await runOwnedBackendFixture(tmpDir, async (registerProcess) => { + const homeDir = path.join(tmpDir, ".kandev"); + const dbPath = path.join(tmpDir, "kandev.db"); + const worktreeBase = path.join(tmpDir, "worktrees"); + const repoCloneBase = path.join(tmpDir, "repos"); - // Write a minimal .gitconfig so git doesn't prompt for identity - // and disable signing to avoid SSH/GPG key lookups in the isolated HOME. - fs.writeFileSync( - path.join(tmpDir, ".gitconfig"), - "[user]\n name = E2E Test\n email = e2e@test.local\n[commit]\n gpgsign = false\n[tag]\n gpgsign = false\n", - ); + fs.mkdirSync(homeDir, { recursive: true }); + fs.mkdirSync(worktreeBase, { recursive: true }); + fs.mkdirSync(repoCloneBase, { recursive: true }); - // Give each worker its own agentctl port range, offset from the default - // range (41001-41100) to avoid conflicts with a running dev instance. - // The async cleanup of agent instances runs after each test deletes its - // tasks, so during a 60+ test shard the in-flight cleanup queue can hold - // several dozen ports at any given moment. 200 ports per worker keeps - // headroom for that without overflowing the 65535 port space. With the - // current playwright config (workers: 1, so workerIndex == 0) and shard - // offsets capped at 29, the highest port used is 30001 + 29*1000 + 199 - // = 59200. The `workerIndex * 200` term is defensive for the case where - // a future config sets workers > 1. - const agentctlPortBase = 30001 + E2E_PORT_OFFSET * 1000 + workerInfo.workerIndex * 200; - const agentctlPortMax = agentctlPortBase + 199; + // Write a minimal .gitconfig so git doesn't prompt for identity + // and disable signing to avoid SSH/GPG key lookups in the isolated HOME. + fs.writeFileSync( + path.join(tmpDir, ".gitconfig"), + "[user]\n name = E2E Test\n email = e2e@test.local\n[commit]\n gpgsign = false\n[tag]\n gpgsign = false\n", + ); - // Install a `git` shim that can sleep on `fetch`/`pull` before execing - // the real git binary. Tests that need to simulate slow network git - // operations write a millisecond value to `${tmpDir}/git-delay-ms`; the - // shim reads it on every invocation and sleeps the matching duration. - // When the file is absent the shim is a transparent passthrough, so - // other tests in the same worker are unaffected. - const shimDir = path.join(tmpDir, "bin"); - const shimPath = path.join(shimDir, "git"); - const shimDelayFile = path.join(tmpDir, "git-delay-ms"); - const shimGitLabPushFile = path.join(tmpDir, "gitlab-push-remote"); - const shimGitLabPushRecordFile = path.join(tmpDir, "gitlab-push-record"); - const originalPath = process.env.PATH ?? ""; - fs.mkdirSync(shimDir, { recursive: true }); - fs.writeFileSync( - shimPath, - `#!/bin/sh + // Give each worker its own agentctl port range, offset from the default + // range (41001-41100) to avoid conflicts with a running dev instance. + // The async cleanup of agent instances runs after each test deletes its + // tasks, so during a 60+ test shard the in-flight cleanup queue can hold + // several dozen ports at any given moment. 200 ports per worker keeps + // headroom for that without overflowing the 65535 port space. With the + // current playwright config (workers: 1, so workerIndex == 0) and shard + // offsets capped at 29, the highest port used is 30001 + 29*1000 + 199 + // = 59200. The `workerIndex * 200` term is defensive for the case where + // a future config sets workers > 1. + const agentctlPortBase = 30001 + E2E_PORT_OFFSET * 1000 + workerInfo.workerIndex * 200; + const agentctlPortMax = agentctlPortBase + 199; + + // Install a `git` shim that can sleep on `fetch`/`pull` before execing + // the real git binary. Tests that need to simulate slow network git + // operations write a millisecond value to `${tmpDir}/git-delay-ms`; the + // shim reads it on every invocation and sleeps the matching duration. + // When the file is absent the shim is a transparent passthrough, so + // other tests in the same worker are unaffected. + const shimDir = path.join(tmpDir, "bin"); + const shimPath = path.join(shimDir, "git"); + const shimDelayFile = path.join(tmpDir, "git-delay-ms"); + const shimGitLabPushFile = path.join(tmpDir, "gitlab-push-remote"); + const shimGitLabPushRecordFile = path.join(tmpDir, "gitlab-push-record"); + const originalPath = process.env.PATH ?? ""; + fs.mkdirSync(shimDir, { recursive: true }); + fs.writeFileSync( + shimPath, + `#!/bin/sh if [ -f "$KANDEV_E2E_GIT_DELAY_FILE" ] && { [ "$1" = "fetch" ] || [ "$1" = "pull" ]; }; then delay_ms=$(cat "$KANDEV_E2E_GIT_DELAY_FILE" 2>/dev/null) case "$delay_ms" in @@ -266,127 +358,120 @@ fi export PATH="$KANDEV_E2E_ORIGINAL_PATH" exec git "$@" `, - { mode: 0o755 }, - ); + { mode: 0o755 }, + ); - // Opt-in: Docker E2E project or KANDEV_E2E_DOCKER=1 enables real - // container execution. Default is off so the regular suite stays fast - // and runs without a Docker daemon. See e2e/README.md. - const dockerEnabled = isContainerProjectActive(workerInfo.project.name); - const mockAgentLinuxBinary = path.join(BACKEND_DIR, "bin", "mock-agent-linux-amd64"); - const agentctlLinuxBinary = path.join(BACKEND_DIR, "bin", "agentctl-linux-amd64"); + // Opt-in: Docker E2E project or KANDEV_E2E_DOCKER=1 enables real + // container execution. Default is off so the regular suite stays fast + // and runs without a Docker daemon. See e2e/README.md. + const dockerEnabled = isContainerProjectActive(workerInfo.project.name); + const mockAgentLinuxBinary = path.join(BACKEND_DIR, "bin", "mock-agent-linux-amd64"); + const agentctlLinuxBinary = path.join(BACKEND_DIR, "bin", "agentctl-linux-amd64"); - const backendEnv = { - ...sanitizeInheritedEnv(process.env as Record), - // Prepend the kandev bin dir so the host utility probe can locate - // the `mock-agent` binary via PATH. In production that dir is the - // same as the running kandev binary's dir, but e2e spawns via an - // absolute path and doesn't inherit that location. - PATH: `${shimDir}:${path.join(BACKEND_DIR, "bin")}:${originalPath}`, - KANDEV_E2E_ORIGINAL_PATH: originalPath, - KANDEV_E2E_GIT_DELAY_FILE: shimDelayFile, - KANDEV_E2E_GITLAB_PUSH_FILE: shimGitLabPushFile, - KANDEV_E2E_GITLAB_PUSH_RECORD_FILE: shimGitLabPushRecordFile, - KANDEV_E2E_GITLAB_REMOTE_URL: `http://localhost:${backendPort}/platform/kandev.git`, - HOME: tmpDir, - KANDEV_HOME_DIR: homeDir, - KANDEV_SERVER_PORT: String(backendPort), - KANDEV_WEB_DIST_DIR: WEB_DIST_DIR, - KANDEV_DATABASE_PATH: dbPath, - // Profile selector. KANDEV_E2E_MOCK=true tells the backend to - // apply the `e2e:` profile from profiles.yaml at startup — - // which sets the mock agent and third-party provider flags, - // KANDEV_FEATURES_OFFICE, AGENTCTL_AUTO_APPROVE_PERMISSIONS, - // KANDEV_PLAN_COALESCE_WINDOW_MS, etc. We don't re-set those - // here. KANDEV_MOCK_PROVIDERS stays opt-in per-spec because it - // changes agent counts; the five office-routing-* specs pass - // it to backend.restart() when needed (see - // registry.RoutableProviderIDs). - KANDEV_E2E_MOCK: "true", - KANDEV_DOCKER_ENABLED: dockerEnabled ? "true" : "false", - // When Docker is on, point the lifecycle resolvers at the linux/amd64 - // binaries the test runner pre-built, so containers can bind-mount them. - ...(dockerEnabled - ? { - KANDEV_AGENTCTL_LINUX_BINARY: agentctlLinuxBinary, - KANDEV_MOCK_AGENT_LINUX_BINARY: mockAgentLinuxBinary, - } - : {}), - KANDEV_WORKTREE_ENABLED: "true", - KANDEV_WORKTREE_BASEPATH: worktreeBase, - KANDEV_REPOCLONE_BASEPATH: repoCloneBase, - KANDEV_LOG_LEVEL: process.env.KANDEV_LOG_LEVEL ?? "warn", - AGENTCTL_INSTANCE_PORT_BASE: String(agentctlPortBase), - AGENTCTL_INSTANCE_PORT_MAX: String(agentctlPortMax), - // AGENTCTL_AUTO_APPROVE_PERMISSIONS=true and - // KANDEV_PLAN_COALESCE_WINDOW_MS=2000 are applied by the - // backend's profile loader (profiles.yaml `e2e:` column). - // Specs that need different values (e.g. - // permission-approval.spec.ts setting auto-approve=false) set - // process.env.X before spawn — that already flows through the - // `...sanitizeInheritedEnv(process.env)` spread above, and the - // backend's ApplyProfile leaves already-set vars alone. (Note: - // KANDEV_FEATURES_* is the exception — it's stripped from the - // inherited env so the profile always governs feature flags.) - GIT_AUTHOR_NAME: "E2E Test", - GIT_AUTHOR_EMAIL: "e2e@test.local", - GIT_COMMITTER_NAME: "E2E Test", - GIT_COMMITTER_EMAIL: "e2e@test.local", - }; + const backendEnv = { + ...sanitizeInheritedEnv(process.env as Record), + // Prepend the kandev bin dir so the host utility probe can locate + // the `mock-agent` binary via PATH. In production that dir is the + // same as the running kandev binary's dir, but e2e spawns via an + // absolute path and doesn't inherit that location. + PATH: `${shimDir}:${path.join(BACKEND_DIR, "bin")}:${originalPath}`, + KANDEV_E2E_ORIGINAL_PATH: originalPath, + KANDEV_E2E_GIT_DELAY_FILE: shimDelayFile, + KANDEV_E2E_GITLAB_PUSH_FILE: shimGitLabPushFile, + KANDEV_E2E_GITLAB_PUSH_RECORD_FILE: shimGitLabPushRecordFile, + KANDEV_E2E_GITLAB_REMOTE_URL: `http://localhost:${backendPort}/platform/kandev.git`, + HOME: tmpDir, + KANDEV_HOME_DIR: homeDir, + KANDEV_SERVER_PORT: String(backendPort), + KANDEV_WEB_DIST_DIR: WEB_DIST_DIR, + KANDEV_DATABASE_PATH: dbPath, + // Profile selector. KANDEV_E2E_MOCK=true tells the backend to + // apply the `e2e:` profile from profiles.yaml at startup — + // which sets the mock agent and third-party provider flags, + // KANDEV_FEATURES_OFFICE, AGENTCTL_AUTO_APPROVE_PERMISSIONS, + // KANDEV_PLAN_COALESCE_WINDOW_MS, etc. We don't re-set those + // here. KANDEV_MOCK_PROVIDERS stays opt-in per-spec because it + // changes agent counts; the five office-routing-* specs pass + // it to backend.restart() when needed (see + // registry.RoutableProviderIDs). + KANDEV_E2E_MOCK: "true", + KANDEV_DOCKER_ENABLED: dockerEnabled ? "true" : "false", + // When Docker is on, point the lifecycle resolvers at the linux/amd64 + // binaries the test runner pre-built, so containers can bind-mount them. + ...(dockerEnabled + ? { + KANDEV_AGENTCTL_LINUX_BINARY: agentctlLinuxBinary, + KANDEV_MOCK_AGENT_LINUX_BINARY: mockAgentLinuxBinary, + } + : {}), + KANDEV_WORKTREE_ENABLED: "true", + KANDEV_WORKTREE_BASEPATH: worktreeBase, + KANDEV_REPOCLONE_BASEPATH: repoCloneBase, + KANDEV_LOG_LEVEL: process.env.KANDEV_LOG_LEVEL ?? "warn", + AGENTCTL_INSTANCE_PORT_BASE: String(agentctlPortBase), + AGENTCTL_INSTANCE_PORT_MAX: String(agentctlPortMax), + // AGENTCTL_AUTO_APPROVE_PERMISSIONS=true and + // KANDEV_PLAN_COALESCE_WINDOW_MS=2000 are applied by the + // backend's profile loader (profiles.yaml `e2e:` column). + // Specs that need different values (e.g. + // permission-approval.spec.ts setting auto-approve=false) set + // process.env.X before spawn — that already flows through the + // `...sanitizeInheritedEnv(process.env)` spread above, and the + // backend's ApplyProfile leaves already-set vars alone. (Note: + // KANDEV_FEATURES_* is the exception — it's stripped from the + // inherited env so the profile always governs feature flags.) + GIT_AUTHOR_NAME: "E2E Test", + GIT_AUTHOR_EMAIL: "e2e@test.local", + GIT_COMMITTER_NAME: "E2E Test", + GIT_COMMITTER_EMAIL: "e2e@test.local", + }; - const debug = !!process.env.E2E_DEBUG; - const baseUrl = `http://localhost:${backendPort}`; + const debug = !!process.env.E2E_DEBUG; + const baseUrl = `http://localhost:${backendPort}`; - // Snapshot the baseline env so `restart(envOverrides)` rebuilds from - // a clean copy each call instead of accumulating leftover keys (e.g. - // KANDEV_MOCK_PROVIDERS, KANDEV_PROVIDER_FAILURES) from prior tests. - const baselineEnv = { ...backendEnv } as Record; + // Snapshot the baseline env so `restart(envOverrides)` rebuilds from + // a clean copy each call instead of accumulating leftover keys (e.g. + // KANDEV_MOCK_PROVIDERS, KANDEV_PROVIDER_FAILURES) from prior tests. + const baselineEnv = { ...backendEnv } as Record; - // --- Spawn backend --- - let backendProc = spawnBackendProcess(backendEnv, debug, backendPort); - await waitForHealth(`${baseUrl}/health`, HEALTH_TIMEOUT_MS); - const frontendUrl = baseUrl; - - /** - * Kill the backend process group and respawn with the same config. - * SQLite DB, tmpDir, and all persisted data survive the restart. - * Only in-memory execution state (running agents, WS connections) is lost. - */ - const restart = async (envOverrides?: Record) => { - // Rebuild from the baseline snapshot so a previous restart's - // overrides don't leak into this one (e.g. KANDEV_MOCK_PROVIDERS - // set by the routing specs would otherwise stick for the rest of - // the worker's lifetime and register canonical agent IDs that - // sibling specs count). - const nextEnv: Record = { ...baselineEnv }; - if (envOverrides) { - for (const [k, v] of Object.entries(envOverrides)) { - nextEnv[k] = v; - } - } - await killProcessGroup(backendProc); - // Poll until the OS releases the TCP port rather than sleeping a fixed - // 2 s. TIME_WAIT can linger for 30–120 s under load; the probe exits - // as soon as the port stops accepting connections (typically <200 ms). - await waitForPortFree(backendPort); - backendProc = spawnBackendProcess(nextEnv, debug, backendPort); - // Pass the process so waitForHealth fails fast if it exits (e.g. port still in use) + // --- Spawn backend --- + backendProc = spawnBackendProcess(backendEnv, debug, backendPort); + registerProcess(backendProc); await waitForHealth(`${baseUrl}/health`, HEALTH_TIMEOUT_MS, backendProc); - }; + const frontendUrl = baseUrl; + + /** + * Kill the backend process group and respawn with the same config. + * SQLite DB, tmpDir, and all persisted data survive the restart. + * Only in-memory execution state (running agents, WS connections) is lost. + */ + const restart = async (envOverrides?: Record) => { + // Rebuild from the baseline snapshot so a previous restart's + // overrides don't leak into this one (e.g. KANDEV_MOCK_PROVIDERS + // set by the routing specs would otherwise stick for the rest of + // the worker's lifetime and register canonical agent IDs that + // sibling specs count). + const nextEnv: Record = { ...baselineEnv }; + if (envOverrides) { + for (const [k, v] of Object.entries(envOverrides)) { + nextEnv[k] = v; + } + } + const runningProcess = backendProc; + if (!runningProcess) throw new Error("Backend process is not running"); + await killProcessGroup(runningProcess); + // Poll until the OS releases the TCP port rather than sleeping a fixed + // 2 s. TIME_WAIT can linger for 30–120 s under load; the probe exits + // as soon as the port stops accepting connections (typically <200 ms). + await waitForPortFree(backendPort); + backendProc = spawnBackendProcess(nextEnv, debug, backendPort); + registerProcess(backendProc); + // Pass the process so waitForHealth fails fast if it exits (e.g. port still in use) + await waitForHealth(`${baseUrl}/health`, HEALTH_TIMEOUT_MS, backendProc); + }; - try { await use({ port: backendPort, baseUrl, frontendPort, frontendUrl, tmpDir, restart }); - } finally { - // Shutdown the backend process group. - await killProcessGroup(backendProc); - - // Cleanup temp directory — ignore errors (backend may still hold files briefly) - try { - fs.rmSync(tmpDir, { recursive: true, force: true }); - } catch { - // Non-fatal: OS may not have released all file handles yet - } - } + }); }, { scope: "worker", timeout: 60_000 }, ], diff --git a/apps/web/e2e/helpers/ssh.ts b/apps/web/e2e/helpers/ssh.ts index dd446dd612..6c6ad4ef7b 100644 --- a/apps/web/e2e/helpers/ssh.ts +++ b/apps/web/e2e/helpers/ssh.ts @@ -110,8 +110,26 @@ export function startSSHServer( * Tear down the container started by startSSHServer. Safe to call even if * the container has already exited. */ -export function stopSSHServer(handle: SSHServerHandle): void { - spawnSync("docker", ["rm", "-f", handle.containerName], { stdio: "ignore" }); +type DockerCommand = (args: string[]) => void; + +export function stopSSHServer( + handle: SSHServerHandle, + runDocker: DockerCommand = (args) => { + spawnSync("docker", args, { stdio: "ignore" }); + }, +): void { + // The sshd entrypoint chowns this bind mount to the container user. Hand it + // back to the test runner so the backend fixture can remove its exact root. + const owner = fs.statSync(handle.workDir); + runDocker([ + "exec", + handle.containerName, + "chown", + "-R", + `${owner.uid}:${owner.gid}`, + "/home/kandev/.kandev", + ]); + runDocker(["rm", "-f", handle.containerName]); } /** diff --git a/apps/web/e2e/tests/system/backend-fixture-lifecycle.spec.ts b/apps/web/e2e/tests/system/backend-fixture-lifecycle.spec.ts new file mode 100644 index 0000000000..4b249c1bc7 --- /dev/null +++ b/apps/web/e2e/tests/system/backend-fixture-lifecycle.spec.ts @@ -0,0 +1,153 @@ +import assert from "node:assert/strict"; +import { type ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { test } from "@playwright/test"; + +import { runOwnedBackendFixture, waitForHealth } from "../../fixtures/backend"; +import { stopSSHServer, type SSHServerHandle } from "../../helpers/ssh"; + +test.describe("backend fixture lifecycle", () => { + test("fails fast when the backend exited before health polling began", async () => { + const exitedProcess = Object.assign(new EventEmitter(), { + exitCode: 17, + signalCode: null, + }) as unknown as ChildProcess; + + await assert.rejects( + waitForHealth("http://127.0.0.1:1/health", 50, exitedProcess), + /Backend process exited with code 17/, + ); + }); + + test("fails fast when the backend process cannot spawn", async () => { + const failedProcess = Object.assign(new EventEmitter(), { + exitCode: null, + signalCode: null, + }) as unknown as ChildProcess; + const health = waitForHealth("http://127.0.0.1:1/health", 5_000, failedProcess); + + queueMicrotask(() => failedProcess.emit("error", new Error("spawn failed"))); + + await assert.rejects(health, /spawn failed/); + }); + + test("removes its exact temporary root when setup fails before process spawn", async () => { + const parentDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-lifecycle-test-")); + const ownedDir = fs.mkdtempSync(path.join(parentDir, "owned-")); + const peerDir = fs.mkdtempSync(path.join(parentDir, "peer-")); + fs.writeFileSync(path.join(ownedDir, "partial.db"), "partial"); + fs.writeFileSync(path.join(peerDir, "keep.db"), "keep"); + + try { + await assert.rejects( + runOwnedBackendFixture(ownedDir, async () => { + throw new Error("setup failed"); + }), + /setup failed/, + ); + + assert.equal(fs.existsSync(ownedDir), false); + assert.equal(fs.readFileSync(path.join(peerDir, "keep.db"), "utf8"), "keep"); + } finally { + fs.rmSync(parentDir, { recursive: true, force: true }); + } + }); + + test("removes its temporary root after successful fixture use", async () => { + const ownedDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-lifecycle-success-")); + + await runOwnedBackendFixture(ownedDir, async () => { + fs.writeFileSync(path.join(ownedDir, "kandev.db"), "complete"); + }); + + assert.equal(fs.existsSync(ownedDir), false); + }); + + test("stops the registered process before cleanup after a health failure", async () => { + const events: string[] = []; + const child = new EventEmitter() as unknown as ChildProcess; + + await assert.rejects( + runOwnedBackendFixture( + "/tmp/exact-owned-root", + async (registerProcess) => { + registerProcess(child); + events.push("run"); + throw new Error("health failed"); + }, + { + stopProcess: async (registeredChild) => { + assert.equal(registeredChild, child); + events.push("stop"); + }, + removeTempRoot: (tmpDir) => { + assert.equal(tmpDir, "/tmp/exact-owned-root"); + events.push("remove"); + }, + }, + ), + /health failed/, + ); + + assert.deepEqual(events, ["run", "stop", "remove"]); + }); + + test("surfaces cleanup failure alongside the fixture failure", async () => { + const fixtureFailure = new Error("fixture failed"); + const cleanupFailure = new Error("cleanup failed"); + + await assert.rejects( + runOwnedBackendFixture( + "/tmp/exact-owned-root", + async () => { + throw fixtureFailure; + }, + { + removeTempRoot: () => { + throw cleanupFailure; + }, + }, + ), + (error) => { + assert.ok(error instanceof AggregateError); + assert.equal(error.errors[0], fixtureFailure); + assert.match(error.errors[1].message, /Failed to remove E2E temporary root/); + assert.equal(error.errors[1].cause, cleanupFailure); + return true; + }, + ); + }); + + test("returns the SSH bind mount to the host owner before removing the container", () => { + const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-ssh-cleanup-test-")); + const owner = fs.statSync(workDir); + const commands: string[][] = []; + const handle = { + containerName: "kandev-sshd-e2e-7", + workDir, + } as SSHServerHandle; + + try { + stopSSHServer(handle, (args) => { + commands.push(args); + }); + } finally { + fs.rmSync(workDir, { recursive: true, force: true }); + } + + assert.deepEqual(commands, [ + [ + "exec", + handle.containerName, + "chown", + "-R", + `${owner.uid}:${owner.gid}`, + "/home/kandev/.kandev", + ], + ["rm", "-f", handle.containerName], + ]); + }); +}); diff --git a/docs/decisions/0045-install-wide-storage-maintenance.md b/docs/decisions/0045-install-wide-storage-maintenance.md index 99e3c1f9c3..38ae1b8e2c 100644 --- a/docs/decisions/0045-install-wide-storage-maintenance.md +++ b/docs/decisions/0045-install-wide-storage-maintenance.md @@ -1,6 +1,6 @@ # 0045: Install-wide storage maintenance uses typed ownership providers and quarantine -**Status:** accepted (amended 2026-07-19) +**Status:** accepted (amended 2026-07-22) **Date:** 2026-07-14 **Area:** backend, frontend, infra @@ -44,20 +44,28 @@ pending archive job and restores a matching quarantined workspace when possible. does not delete historical archived-task worktree rows or branch metadata used by PR #1687's branch recovery. -Agentctl session temporary roots are a separate lifecycle-owned resource. They remain beneath the -operating system's temporary directory under a readable, collision-resistant digest of the raw -session and instance identity so concurrent sessions have isolated sockets, compiler work files, -and command scratch space without mixing ephemeral data into `KANDEV_HOME_DIR`. After agent, shell, -VS Code, and workspace subprocesses are reaped during permanent instance teardown, agentctl deletes -only that instance's validated owned root. A later session resume creates a replacement instance -and does not depend on the prior instance's scratch. Session temp data is not quarantined because it -is explicitly ephemeral and contains no task recovery state. +Host-local agent instances inherit the Kandev service's `TMPDIR`, `TMP`, and `TEMP` environment +unchanged. Agentctl does not replace those values with a per-instance directory and does not claim +ownership of files written to the shared operating-system temporary location. An operator-level +temporary-directory override remains authoritative for the whole service; with no override, normal +platform defaults apply. -Terminal instance teardown closes process admission before its ownership sweep. A failed HTTP -shutdown or process-tree reap retains a stopping instance tombstone and its allocated port so a -replacement cannot adopt unresolved runtime resources. A temporary-directory-only deletion failure -is reported but does not retain the execution port. Ordinary agent stop/start remains restartable; -the irreversible admission close belongs to instance teardown. +This boundary is intentionally separate from persistent tool-cache ownership. Go's default +`GOCACHE`, for example, is already a shared user cache and is not derived from `TMPDIR`; Kandev only +injects its managed cache path when that existing Storage option is explicitly enabled. Tools that +derive reusable caches from the system temp directory may now share them across agents instead of +duplicating them beneath per-instance roots. + +Terminal instance teardown still closes process admission and reaps the owned process tree, but it +does not sweep shared temp contents. Kandev-specific sockets, locks, or scratch paths that require +isolation must carry an explicit collision-resistant name at their own boundary. A demonstrated +tool-specific collision may justify a narrow override for that tool, not a replacement of the +temporary environment inherited by every child process. + +Directories under `/tmp/kandev-agent/*` created by older versions are legacy host data. New agent +instances no longer add to that root. Storage maintenance does not delete legacy entries by name or +age because the existing directories lack durable ownership evidence; host temporary-file policy or +a deliberate stopped-service cleanup remains the authority for that historical footprint. ## Consequences @@ -72,8 +80,13 @@ the irreversible admission close belongs to instance teardown. reclaimable bytes for the operator. - The existing Office GC package and startup wiring are removed or reduced to adapters over the System storage service to avoid two competing sweepers. -- Session teardown reclaims its temporary storage immediately and independently of scheduled - maintenance, while path containment prevents removal of the shared temp root or sibling sessions. +- Node, Playwright, and similar temp-derived caches may be reused across agent instances instead of + being duplicated under a task-scoped temp root. +- Transient compiler work, test databases, plugin fixtures, sockets, and other scratch still exist; + they move to the inherited shared temp location and are governed by tool cleanup plus host policy. +- Kandev loses per-task attribution and exact recursive deletion for arbitrary temp contents. This + is an accepted tradeoff because operators prefer cache sharing and normal host temp lifecycle. +- Existing `/tmp/kandev-agent` usage is not reclaimed automatically by this change. ## Alternatives Considered @@ -93,5 +106,13 @@ the irreversible admission close belongs to instance teardown. - **Store session scratch data under `KANDEV_HOME_DIR`.** Rejected because it mixes transient process data with persistent application state and makes backups, retention, and disk accounting retain data that should disappear at teardown or reboot. -- **Share one temporary directory across agent sessions.** Rejected because filenames, sockets, and - compiler work directories can collide and teardown cannot safely determine which files it owns. +- **Give every agent instance its own `TMPDIR`, `TMP`, and `TEMP`.** Rejected after operational + evidence showed duplicated Node/Playwright caches, explicitly redirected Go caches, and abandoned + E2E/compiler scratch consuming many GiB beneath long-lived agent roots. It improves attribution + and defensive isolation, but prevents useful cache sharing and makes interrupted task roots a + concentrated storage leak. +- **Keep per-instance temp roots but mount shared subdirectories for selected caches.** Rejected for + now because cache discovery is tool-specific and creates a growing allowlist. Persistent caches + such as `GOCACHE` retain their dedicated policy; observed collisions can receive narrow fixes. +- **Delete old `/tmp/kandev-agent/*` entries by name or age.** Rejected because neither proves task + archival, installation ownership, process liveness, or protection from path replacement. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 0933b22f39..e45bf3074c 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -52,7 +52,7 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-07-14-typed-utility-chat-sessions | [Typed Utility Chats Share the Quick Chat Session Model](2026-07-14-typed-utility-chat-sessions.md) | accepted | backend, frontend | 2026-07-14 | | 2026-07-15-office-agent-execution-profile-routing | [Separate Office identity from routed execution profiles](2026-07-15-office-agent-execution-profile-routing.md) | proposed | backend, frontend | 2026-07-15 | | 0044 | [ACP agent compatibility dialects](0044-acp-agent-compatibility-dialects.md) | accepted | backend, protocol | 2026-07-16 | -| 0045 | [Install-wide storage maintenance uses typed ownership providers and quarantine](0045-install-wide-storage-maintenance.md) | accepted (amended 2026-07-19) | backend, frontend, infra | 2026-07-14 | +| 0045 | [Install-wide storage maintenance uses typed ownership providers and quarantine](0045-install-wide-storage-maintenance.md) | accepted (amended 2026-07-22) | backend, frontend, infra | 2026-07-14 | | 0046 | [Settings route save coordinator](0046-settings-route-save-coordinator.md) | accepted | frontend | 2026-07-14 | | 0047 | [Plugins read conversation content via a capability-gated Host RPC](0047-plugin-host-conversation-reads.md) | accepted | backend, protocol | 2026-07-21 | | 0048 | [Plugins invoke a settings-selectable utility agent](0048-plugin-host-utility-agent-invoke.md) | accepted | backend, frontend, protocol | 2026-07-21 | diff --git a/docs/plans/storage-maintenance/plan.md b/docs/plans/storage-maintenance/plan.md index c2881c2c97..1e50a74370 100644 --- a/docs/plans/storage-maintenance/plan.md +++ b/docs/plans/storage-maintenance/plan.md @@ -18,6 +18,12 @@ The implementation preserves PR #1687's unarchive contract: archived filesystem reclaimed, historical worktree rows and branch metadata remain, and pending archive cleanup is cancelled when a task becomes active again. +The 2026-07-22 follow-up removes per-instance temporary-directory injection. Host-local agents +inherit the Kandev service's `TMPDIR`, `TMP`, and `TEMP` unchanged so temp-derived Node/Playwright +caches can be reused. The superseded marker, snapshot, Storage-provider, UI, and E2E work is removed; +managed Go-cache defaults and injection remain unchanged. A separate test-harness task addresses +the observed accumulation of `kandev-e2e-*` roots. + --- ## Backend @@ -288,3 +294,19 @@ Wave 4: Wave 5: - [x] [Task 09 — E2E, QA, and final verification](task-09-e2e-qa-verification.md) + +Wave 6: + +- [x] [Task 10 — Inherit the service temp environment](task-10-inherit-service-temp.md) + +Wave 7: + +- [x] [Task 11 — Contain E2E temporary roots](task-11-e2e-temp-cleanup.md) + +Wave 8: + +- [x] [Task 12 — Update inherited-temp operations docs](task-12-inherited-temp-docs.md) + +Wave 9: + +- [x] [Task 13 — Review and verify inherited temp behavior](task-13-inherited-temp-verification.md) diff --git a/docs/plans/storage-maintenance/task-10-inherit-service-temp.md b/docs/plans/storage-maintenance/task-10-inherit-service-temp.md new file mode 100644 index 0000000000..9200315952 --- /dev/null +++ b/docs/plans/storage-maintenance/task-10-inherit-service-temp.md @@ -0,0 +1,56 @@ +--- +id: "10-inherit-service-temp" +title: "Inherit the service temp environment" +status: done +wave: 6 +depends_on: [] +plan: "plan.md" +spec: "../../specs/system-page/storage-maintenance.md" +--- + +# Task 10: Inherit the service temp environment + +Stop replacing `TMPDIR`, `TMP`, and `TEMP` for host-local agent instances and remove the superseded +uncommitted ownership/maintenance implementation. + +## Acceptance + +- Agentctl does not create `/kandev-agent/` or inject per-instance + `TMPDIR`, `TMP`, or `TEMP`; configured service values are inherited unchanged and unset values stay + unset at Kandev's environment-construction boundary. +- Existing agent stop/reap behavior remains intact, but there is no per-instance temp marker, + descriptor, cleanup snapshot, Storage provider/API/UI action, or agent-temp E2E scenario in the + final diff. +- `GOCACHE` behavior is unchanged: the existing managed cache is injected only when enabled, and + no new Go cache is placed beneath the inherited temp directory. +- Red-first tests prove inherited/set/unset values and prove starting an instance does not create a + `kandev-agent` root. + +## Verification + +```bash +cd apps/backend && go test ./internal/agentctl/server/process ./internal/agentctl/server/instance ./internal/agent/runtime/lifecycle ./internal/backendapp ./internal/system/storage/... ./internal/task/service +cd apps && pnpm --filter @kandev/web test -- --run components/settings/system/storage hooks/domains/system/use-storage-maintenance.test.tsx +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/manager.go` +- `apps/backend/internal/agentctl/server/process/manager_temp_test.go` +- superseded agent-temp DTO, lifecycle, task cleanup, Storage provider, frontend, and E2E files in + the current uncommitted diff + +## Dependencies + +None. + +## Inputs + +- Spec: Agent session temporary data and scenarios +- ADR 0045 inherited-temp amendment +- Current `git diff`, whose marker/provider/UI changes are superseded rather than additive + +## Output contract + +Report the exact inherited environment behavior, removed superseded surfaces, red/green tests, +commands/results, blockers, and update this task plus the serialized plan checkbox when complete. diff --git a/docs/plans/storage-maintenance/task-11-e2e-temp-cleanup.md b/docs/plans/storage-maintenance/task-11-e2e-temp-cleanup.md new file mode 100644 index 0000000000..eb6ac2caf5 --- /dev/null +++ b/docs/plans/storage-maintenance/task-11-e2e-temp-cleanup.md @@ -0,0 +1,63 @@ +--- +id: "11-e2e-temp-cleanup" +title: "Contain E2E temporary roots" +status: complete +wave: 7 +depends_on: ["10-inherit-service-temp"] +plan: "plan.md" +spec: "../../specs/system-page/storage-maintenance.md" +--- + +# Task 11: Contain E2E temporary roots + +Prevent normal managed E2E runs from leaving large numbers of `kandev-e2e-*` backend roots in the +service's inherited temporary directory. + +## Acceptance + +- Backend fixture directories created by one managed run have one explicit run owner and are removed + on normal success and failure without deleting another concurrent run's files. +- The cleanup guard begins immediately after `mkdtempSync`, covers setup/spawn/initial health + failures, passes the spawned process to the initial health wait for fast early-exit detection, and + reports cleanup failures instead of silently discarding them. +- Cleanup targets only the exact run-owned root; interrupted-process residue remains subject to host + temp policy unless a safe existing runner lifecycle can prove ownership. +- Tests or a deterministic harness regression prove success/failure cleanup and concurrent-run + isolation. No test uses the host's real Kandev data directory. + +## Verification + +```bash +cd apps/web && pnpm e2e:run tests/system/storage-maintenance.spec.ts +cd apps/web && pnpm e2e:run tests/system/mobile-storage-maintenance.spec.ts -- --project=mobile-chrome +``` + +## Files likely touched + +- `apps/web/e2e/fixtures/backend.ts` +- a focused fixture-lifecycle test beside the existing E2E harness tests +- focused fixture tests + +## Dependencies + +Task 10. + +## Inputs + +- Read-only audit of 1,099 abandoned `kandev-e2e-*` roots +- Existing managed runner cleanup and concurrency conventions + +## Output contract + +Report root ownership, success/failure/interruption semantics, concurrency safety, tests/commands, +remaining host-policy boundary, and update this task plus the serialized plan checkbox when done. + +## Completion + +- The atomically-created worker root is now guarded immediately after creation. Setup, spawn, + initial-health, test-use, and restart failures all converge on the same exact-root cleanup path. +- The latest registered backend process group is stopped before bounded directory removal. Cleanup + failures are surfaced, including alongside the original fixture failure. +- Focused lifecycle coverage proves early child-exit detection, success/failure cleanup, process-stop + ordering, peer-root isolation, and cleanup-error reporting. +- Both required managed Storage E2E commands passed, and neither left a new `kandev-e2e-*` root. diff --git a/docs/plans/storage-maintenance/task-12-inherited-temp-docs.md b/docs/plans/storage-maintenance/task-12-inherited-temp-docs.md new file mode 100644 index 0000000000..4a5ce93019 --- /dev/null +++ b/docs/plans/storage-maintenance/task-12-inherited-temp-docs.md @@ -0,0 +1,61 @@ +--- +id: "12-inherited-temp-docs" +title: "Update inherited-temp operations docs" +status: complete +wave: 8 +depends_on: ["10-inherit-service-temp", "11-e2e-temp-cleanup"] +plan: "plan.md" +spec: "../../specs/system-page/storage-maintenance.md" +--- + +# Task 12: Update inherited-temp operations docs + +Make public operations guidance match the service-inherited temp boundary and legacy cleanup limits. + +## Acceptance + +- Public docs explain that host-local agents inherit service `TMPDIR`/`TMP`/`TEMP`, persistent + `GOCACHE` is separate, and host temp policy owns shared scratch cleanup. +- Docs do not advertise an agent-temp Storage row/action or automatic deletion of legacy + `/tmp/kandev-agent/*`; they explain that confirmed-inactive legacy data needs deliberate host + cleanup. +- Public docs validation passes. + +## Verification + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `docs/public/operations.md` +- affected spec/ADR/plan wording discovered by docs audit + +## Dependencies + +Tasks 10 and 11. + +## Inputs + +- `/docs-maintainer` +- Updated Storage spec and ADR 0045 + +## Output contract + +Report public/internal docs updated, validation results, stale terminology removed, blockers, and +update this task plus the serialized plan checkbox when complete. + +## Completion + +- Updated the public operations guide to document service-inherited `TMPDIR`, `TMP`, and `TEMP`, + platform defaults when those variables are unset, and shared cache behavior for temp-derived + tooling. +- Clarified that the default and managed Go build caches are governed by `GOCACHE`, with Kandev's + managed path remaining opt-in and independent from temporary-directory policy. +- Removed the superseded agent-temp Storage analysis/action, marker, cleanup-record, and + archive/delete directory-removal guidance. Shared scratch cleanup now belongs to host temp policy. +- Documented deliberate host-administrator cleanup for confirmed-inactive legacy + `/tmp/kandev-agent/*` directories, with no automatic name- or age-based removal. +- Public-doc tests and validation passed; `git diff --check` reported no whitespace errors. diff --git a/docs/plans/storage-maintenance/task-13-inherited-temp-verification.md b/docs/plans/storage-maintenance/task-13-inherited-temp-verification.md new file mode 100644 index 0000000000..78084ebcee --- /dev/null +++ b/docs/plans/storage-maintenance/task-13-inherited-temp-verification.md @@ -0,0 +1,79 @@ +--- +id: "13-inherited-temp-verification" +title: "Review and verify inherited temp behavior" +status: done +wave: 9 +depends_on: ["10-inherit-service-temp", "11-e2e-temp-cleanup", "12-inherited-temp-docs"] +plan: "plan.md" +spec: "../../specs/system-page/storage-maintenance.md" +--- + +# Task 13: Review and verify inherited temp behavior + +Review the simplified implementation for environment, process-lifecycle, cache, concurrency, and +documentation regressions, then run repository-wide verification. + +## Acceptance + +- Review proves no application path recreates per-instance `kandev-agent` temp roots or injects + instance-specific temp variables, while service overrides and managed `GOCACHE` remain correct. +- QA covers concurrent agents sharing temp-derived caches, archive/delete process reaping, E2E + fixture cleanup, and absence of the superseded Storage API/UI surface. +- Formatting, backend tests/lint, frontend typecheck/tests/lint, focused desktop/mobile E2E, public + docs validation, and `git diff --check` pass or exact environmental blockers are reported. + +## Verification + +```bash +make -C apps/backend fmt +make -C apps/backend test +make -C apps/backend lint +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web test +cd apps && pnpm --filter @kandev/web lint +cd apps/web && pnpm e2e:run tests/system/storage-maintenance.spec.ts +cd apps/web && pnpm e2e:run tests/system/mobile-storage-maintenance.spec.ts -- --project=mobile-chrome +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +## Files likely touched + +- only files required by review, simplification, QA, verification, and docs reconciliation findings + +## Dependencies + +Tasks 10–12. + +## Inputs + +- Completed implementation packets +- `/code-review`, `/simplify`, `/qa`, `/verify`, and ADR 0045 + +## Output contract + +Report findings/fixes, commands/results, residual collision/legacy-cleanup risks, final spec/plan +status, and update this task plus the serialized plan checkbox when complete. + +## Completion + +- Removed the obsolete instance-resource release classification that existed only for the retired + agent-temp cleanup error. Every unresolved process teardown now retains the instance and port for + retry; focused tests and the process/instance race suites pass. +- Removed the stale VS Code temp-setup failure test and reconciled the earlier workspace Git-status + plan so it records that per-instance temp teardown was superseded by ADR 0045's amendment. +- Hardened backend fixture health polling for asynchronous child-process spawn errors. The fixture + lifecycle suite passes all six setup, spawn, health, success, ordering, isolation, and cleanup + error cases. +- Confirmed production searches contain no agent-temp root creator, temp-variable injector, + cleanup classifier, Storage resource/API/UI symbol, or marker implementation. Managed `GOCACHE` + injection remains confined to its existing opt-in lifecycle provider paths. +- Formatting, repository-wide typecheck/tests/lint, desktop Storage E2E (3 tests), mobile Storage + E2E (1 test), public-doc tests (58 tests), public-doc validation (40 pages), race tests, and + `git diff --check` pass. Both managed E2E runs left the pre-existing `/tmp/kandev-e2e-*` count + unchanged at three. +- Residual boundaries are deliberate: shared-temp tool collisions require narrow tool-specific + fixes when observed; explicit Unix service temp paths must remain short enough for local socket + limits; an uncatchable hard kill can still leave E2E scratch for host policy; and inactive legacy + `/tmp/kandev-agent/*` roots require deliberate stopped-service cleanup. diff --git a/docs/plans/workspace-git-status-scalability/plan.md b/docs/plans/workspace-git-status-scalability/plan.md index 66157318a8..ce3467ea5d 100644 --- a/docs/plans/workspace-git-status-scalability/plan.md +++ b/docs/plans/workspace-git-status-scalability/plan.md @@ -10,6 +10,10 @@ status: done First keep reusable verification caches shared and all verification storage outside Git worktrees, ignore legacy worktree-local cache paths, and reclaim each owned agent temp root during permanent instance teardown. Then make diff enrichment linear and context-aware while preserving complete changed-path metadata and all existing limits. Next coalesce overlapping live observations through the workspace tracker using Kandev's established detached, bounded singleflight pattern. Finish with HTTP-boundary concurrency and multi-repository regression coverage. +The agent-temp teardown portion is a historical implementation record. ADR 0045 was amended on +2026-07-22, and Storage Maintenance Task 10 superseded it by removing per-instance temp injection +and cleanup. Host-local agents now inherit the Kandev service temporary environment. + The incident that motivated this work involved 13,869 untracked Go cache files. Diff enrichment repeatedly recomputed total diff bytes by scanning the complete file map inside per-file loops, producing quadratic work. Six concurrent fresh multi-repository requests then repeated that computation and held agentctl near full CPU for roughly three minutes. ## Backend @@ -105,7 +109,8 @@ Wave 0: Wave 1 (parallel): - [x] [task-01-linear-diff-enrichment](task-01-linear-diff-enrichment.md) - done -- [x] [task-04-agent-temp-teardown](task-04-agent-temp-teardown.md) - done +- [x] [task-04-agent-temp-teardown](task-04-agent-temp-teardown.md) - superseded by Storage + Maintenance Task 10 Wave 2 (depends on Wave 1): diff --git a/docs/plans/workspace-git-status-scalability/task-04-agent-temp-teardown.md b/docs/plans/workspace-git-status-scalability/task-04-agent-temp-teardown.md index a6f5078eca..be758f9637 100644 --- a/docs/plans/workspace-git-status-scalability/task-04-agent-temp-teardown.md +++ b/docs/plans/workspace-git-status-scalability/task-04-agent-temp-teardown.md @@ -1,15 +1,20 @@ --- id: "04-agent-temp-teardown" title: "Agent temp teardown" -status: done +status: superseded wave: 1 depends_on: [] plan: "plan.md" spec: "../../specs/system-page/storage-maintenance.md" +superseded_by: "../storage-maintenance/task-10-inherit-service-temp.md" --- # Task 04: Agent temp teardown +> Superseded on 2026-07-22 by the inherited service-temp boundary in ADR 0045 and Storage +> Maintenance Task 10. This file records the earlier implementation packet; the per-instance +> directory and cleanup behavior below is no longer active. + ## Acceptance - Agent instance teardown removes its exact collision-resistant `/kandev-agent/-` directory only after owned subprocesses are stopped and reaped. diff --git a/docs/public/operations.md b/docs/public/operations.md index 76fced5c91..261074198c 100644 --- a/docs/public/operations.md +++ b/docs/public/operations.md @@ -86,6 +86,26 @@ build-cache and unused-image cleanup remain disabled until you confirm that Kand Docker daemon. Do not enable those rules on a daemon shared with unrelated workloads. +Host-local agents inherit the Kandev service's `TMPDIR`, `TMP`, and `TEMP` values unchanged. If the +service leaves them unset, agent tools use their normal operating-system defaults; if an operator +sets them on the service, all host-local agents share those configured locations. This permits +tools whose caches are derived from the temporary directory, such as Node or Playwright tooling, to +reuse cache data across agent instances. Kandev does not create a per-instance temporary root or +claim ownership of arbitrary files in the shared temporary directory. On Unix, keep an explicit +service temp path short enough for local Unix-domain socket paths; an excessively long path can +exceed the platform socket-address limit. + +Persistent Go build caching is separate: Go's default `GOCACHE` is not derived from `TMPDIR`, and +Kandev injects its managed Go-cache location only when that opt-in Storage setting is enabled. +Scratch cleanup in the inherited temporary directory belongs to the operating system or the host's +temporary-file policy. Archive and delete stop and reap the task's host-local processes, but do not +recursively delete shared temporary files. + +Older versions may have left inactive directories under `/tmp/kandev-agent/*`. Storage analysis and +cleanup do not delete this legacy data by name or age. Remove it only through a deliberate +host-administrator procedure after confirming that no live process references the target; stopping +the Kandev service first provides the clearest maintenance boundary. + ## Database operation ### SQLite @@ -196,7 +216,7 @@ When reporting an incident, record timestamp/timezone, Kandev version and commit **Settings > System > Status** walks `data`, worktrees, repositories, sessions, tasks, quick chat, and backups. Results are cached for two hours; **Refresh** forces a new single-flight walk. Permission failures appear as warnings. The displayed total intentionally counts `data/backups` both inside the `data` row and again as the separate `backups` row, so use filesystem or volume metrics for quota enforcement. -Archiving or deleting a task stops active sessions and starts asynchronous cleanup with a 60-second bound. Depending on executor, cleanup can delete a managed worktree and its local branch, remove a container, destroy a Sprite, or attempt to stop the remote SSH controller and remove only its per-session runtime directory. SSH process/session cleanup is best-effort when the connection is failing, and the task directory always remains for deliberate, audited cleanup; there is no automatic sweeper for it today. The task can disappear from the UI before cleanup finishes. +Archiving or deleting a task stops active sessions and starts durable asynchronous cleanup. Depending on executor, cleanup can delete a managed worktree and its local branch, remove a container, destroy a Sprite, reap a host-local agent's process tree, or attempt to stop the remote SSH controller and remove only its per-session runtime directory. Failed cleanup remains retryable across a backend restart. Kandev does not sweep arbitrary files from the shared temporary directory during archive or delete. SSH process/session cleanup is best-effort when the connection is failing, and the task directory always remains for deliberate, audited cleanup; there is no automatic sweeper for it today. The task can disappear from the UI before cleanup finishes. **Reset Environment** uses a separate teardown path. For Sprites, the current reset request can lose the profile credential context and report success while leaving the provider sandbox behind. After a Sprites reset, inspect **Settings > Executors > Sprites.dev** and explicitly destroy the old sandbox there if it remains. See [Executors](executors.md#spritesdev) for the executor-specific lifecycle. @@ -261,6 +281,7 @@ drawer mirrors it as the saved left sequence followed by the saved right sequenc | **Apply update** is absent | Install method shown on the Updates page | Expected for current native services; upgrade the package, reinstall the service with the same flags, and restart | | Metrics show unavailable | OS support, disk path, executor connectivity | Select supported metrics and verify permissions/network; the collector reports errors per sample | | Disk total exceeds filesystem expectation | Separate `data` and `backups` rows | Backups are counted twice in the UI total; use volume metrics for capacity decisions | +| Legacy `/tmp/kandev-agent/*` uses disk | Process inventory and open-file references for the exact directory | This is data from older Kandev versions, not a current Storage resource. Stop Kandev, confirm no live process references the target, then remove only the confirmed-inactive legacy directory through host administration. | | Archived task's remote resource remains | Backend, Docker/SSH/Sprites, and provider logs | Cleanup is asynchronous and bounded. SSH task directories are retained by design; for other leftovers, verify work is preserved, then remove the exact resource manually | | Sprites reset removed the environment but not the sandbox | **Settings > Executors > Sprites.dev** | Current reset can omit the provider credential during destroy; find the old Kandev-named sandbox and destroy it explicitly | diff --git a/docs/specs/system-page/storage-maintenance.md b/docs/specs/system-page/storage-maintenance.md index 1ed9902ea6..f5929f41c2 100644 --- a/docs/specs/system-page/storage-maintenance.md +++ b/docs/specs/system-page/storage-maintenance.md @@ -140,26 +140,26 @@ Decision: [ADR-2026-07-19-workspace-symlink-entries](../../decisions/2026-07-19- ### Agent session temporary data -- Each agent instance receives an isolated operating-system temporary directory at - `/kandev-agent/-` through `TMPDIR`, `TMP`, and - `TEMP`. The deterministic name includes the raw session ID, instance ID, and port so distinct - instance identities cannot collide after unsafe characters are sanitized. -- The directory is owned by that agent instance and remains available while its agent, shell, - VS Code, or workspace subprocesses may still be running. -- Instance teardown first closes every process-start admission path, including requests already in - flight, then removes the owned session directory only after each process leader and its owned - process tree are reaped. On Unix this includes verified process-group disappearance; Windows uses - the executor's existing process-tree termination primitive. - Teardown validates that the target is a non-root child of the Kandev agent-temp root and never - removes the shared root or a sibling session directory. -- Cleanup runs whenever the owning agentctl instance is permanently deleted, including task/session - archive and delete stops and when teardown observes that the main agent process has already - stopped. A later resume creates a new instance and does not depend on the prior instance's scratch - files. A cleanup failure is reported without broadening deletion to another path. The instance - and port remain reserved only when HTTP shutdown or process reaping is unresolved; a - temporary-directory-only failure retains a retry tombstone but releases the execution port. -- Session temporary data is ephemeral and is not quarantined, restored, or included in task - recovery. See [ADR 0045](../../decisions/0045-install-wide-storage-maintenance.md). +- Host-local agent instances inherit `TMPDIR`, `TMP`, and `TEMP` from the Kandev service unchanged. + Kandev does not create or inject a per-instance temporary root. When the service leaves those + variables unset, agents and their child tools use the operating system default temporary + location; when an operator configures them for the service, every host-local agent shares that + configured location. +- Tool-managed caches may therefore be shared when the tool's own default uses the temporary + location. Persistent caches remain governed by their own variables and policies: in particular, + Go's default `GOCACHE` is separate from `TMPDIR`, and Kandev only injects its managed Go-cache path + when the existing Storage setting is explicitly enabled. +- Kandev-specific files that require collision-free identity must use an explicit unique path or + filename. A future collision in one tool is fixed at that tool boundary; it does not justify + replacing the complete temporary environment for every agent child process. +- Archive/delete teardown still closes process admission and reaps each owned process tree. It does + not recursively delete arbitrary files from the inherited system temporary directory because + those files are shared and cannot be attributed safely to one task. +- Existing `/tmp/kandev-agent/*` directories created by older versions are legacy host data. The + Storage scheduler does not delete them by name or age, and new agent runs do not add to that root. + Operators may remove confirmed-inactive legacy data through their normal host temporary-file + policy or a deliberate one-time maintenance procedure. See + [ADR 0045](../../decisions/0045-install-wide-storage-maintenance.md). ### Docker storage @@ -460,6 +460,14 @@ enabling host-global Docker cleanup require explicit UI confirmation and server- - **GIVEN** `/root/.cache/go-build` is the service user's default Go cache and is not adopted, **WHEN** storage analysis runs, **THEN** its path and bytes are reported read-only while cleanup remains unavailable for that path. +- **GIVEN** the Kandev service has no temporary-directory variables configured, **WHEN** two + host-local agents start, **THEN** neither instance receives an injected `TMPDIR`, `TMP`, or `TEMP` + value and their tools use the operating system defaults. +- **GIVEN** an operator sets `TMPDIR`, `TMP`, or `TEMP` on the Kandev service, **WHEN** a host-local + agent starts, **THEN** it inherits those values unchanged rather than receiving a per-instance + replacement. +- **GIVEN** a task is archived or deleted, **WHEN** its local instance tears down, **THEN** Kandev + reaps its owned processes but does not sweep the shared default temporary directory. - **GIVEN** the Docker daemon reports image-layer usage, **WHEN** storage analysis runs, **THEN** image-layer bytes are shown separately from build-cache and managed-container writable bytes. - **GIVEN** an exited container has `kandev.managed=true` and its task is positively absent, @@ -484,6 +492,10 @@ enabling host-global Docker cleanup require explicit UI confirmation and server- - Cleaning remote SSH executor filesystems; remote maintenance requires a separate explicit design. - Restoring uncommitted files after their quarantine retention has expired. - Automatically cleaning a pre-existing user Go cache without explicit path adoption. +- Age-based or name-based deletion of unmarked `/tmp/kandev-agent/*` directories. +- A Kandev-owned general-purpose sweeper for the operating system's shared temporary directory. +- Guaranteed compatibility with tools that require a fixed, globally unique name in shared temp; + those tools need a scoped path override when a real collision is observed. ## Implementation plan