diff --git a/packages/codev/src/agent-farm/__tests__/cleanup-shellper-kill.test.ts b/packages/codev/src/agent-farm/__tests__/cleanup-shellper-kill.test.ts new file mode 100644 index 000000000..f86d44aa4 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/cleanup-shellper-kill.test.ts @@ -0,0 +1,166 @@ +/** + * Tests for cleanup command — shellper process kill (Bugfix #389) + * + * When `af cleanup` runs, it must kill shellper processes associated with the + * builder's worktree. Previously, cleanup relied solely on the Tower API which + * silently fails when Tower is not running or the terminal was already removed. + * + * The fix adds a direct `ps`-based search for shellper-main.js processes whose + * JSON config contains the worktree path, killing them via process group signal. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { killShellperProcesses } from '../commands/cleanup.js'; +import { execFile } from 'node:child_process'; + +// Mock execFile to simulate ps output +vi.mock('node:child_process', async () => { + const actual = await vi.importActual('node:child_process'); + return { + ...actual, + execFile: vi.fn(), + }; +}); + +const mockExecFile = vi.mocked(execFile); + +// Mock process.kill to track kill signals +const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('killShellperProcesses (Bugfix #389)', () => { + const worktree = '/workspace/.builders/bugfix-42-login-fails'; + + function simulatePsOutput(stdout: string): void { + mockExecFile.mockImplementation((_cmd, _args, callback) => { + (callback as (err: Error | null, stdout: string) => void)(null, stdout); + return {} as ReturnType; + }); + } + + function simulatePsError(): void { + mockExecFile.mockImplementation((_cmd, _args, callback) => { + (callback as (err: Error | null, stdout: string) => void)(new Error('ps failed'), ''); + return {} as ReturnType; + }); + } + + it('kills shellper processes matching the worktree cwd', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` + + `99999 node /usr/bin/some-other-process\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(1); + // Should attempt process group kill first (-pid) + expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM'); + }); + + it('does not kill shellper processes for different worktrees', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"/workspace/.builders/bugfix-99-other","socketPath":"/tmp/shellper-abc.sock"}\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(0); + expect(killSpy).not.toHaveBeenCalled(); + }); + + it('does not match partial worktree path prefixes', async () => { + // bugfix-42 should NOT match bugfix-42-login-fails-continued + const shortWorktree = '/workspace/.builders/bugfix-42'; + simulatePsOutput( + ` PID ARGS\n` + + `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${shortWorktree}-login-fails-continued","socketPath":"/tmp/shellper-abc.sock"}\n` + ); + + const killed = await killShellperProcesses(shortWorktree); + + expect(killed).toBe(0); + expect(killSpy).not.toHaveBeenCalled(); + }); + + it('kills multiple shellper processes for the same worktree', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` + + `12346 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-def.sock"}\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(2); + expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM'); + expect(killSpy).toHaveBeenCalledWith(-12346, 'SIGTERM'); + }); + + it('falls back to individual PID kill when process group kill fails', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `12345 node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` + ); + + // First call (-pid group kill) fails, second call (individual pid) succeeds + killSpy + .mockImplementationOnce(() => { throw new Error('ESRCH'); }) + .mockImplementationOnce(() => true); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(1); + expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM'); + expect(killSpy).toHaveBeenCalledWith(12345, 'SIGTERM'); + }); + + it('returns 0 when no shellper processes found', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `99999 node /usr/bin/some-other-process\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(0); + expect(killSpy).not.toHaveBeenCalled(); + }); + + it('returns 0 gracefully when ps fails', async () => { + simulatePsError(); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(0); + }); + + it('skips non-shellper processes even if they mention the worktree', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `12345 claude --cwd ${worktree}\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(0); + expect(killSpy).not.toHaveBeenCalled(); + }); + + it('does not kill its own process', async () => { + simulatePsOutput( + ` PID ARGS\n` + + `${process.pid} node /path/to/shellper-main.js {"command":"/bin/bash","args":[],"cwd":"${worktree}","socketPath":"/tmp/shellper-abc.sock"}\n` + ); + + const killed = await killShellperProcesses(worktree); + + expect(killed).toBe(0); + expect(killSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/codev/src/agent-farm/commands/cleanup.ts b/packages/codev/src/agent-farm/commands/cleanup.ts index 92ab592a1..d0a4f2681 100644 --- a/packages/codev/src/agent-farm/commands/cleanup.ts +++ b/packages/codev/src/agent-farm/commands/cleanup.ts @@ -5,6 +5,7 @@ import { existsSync, readdirSync } from 'node:fs'; import { rm } from 'node:fs/promises'; import { join } from 'node:path'; +import { execFile } from 'node:child_process'; import type { Builder, Config } from '../types.js'; import { getConfig } from '../utils/index.js'; import { logger, fatal } from '../utils/logger.js'; @@ -37,6 +38,60 @@ async function cleanupPorchState(projectId: string, config: Config): Promise { + let killed = 0; + try { + const stdout = await new Promise((resolve, reject) => { + // -ww prevents arg truncation on macOS/Linux + execFile('ps', ['-ww', '-eo', 'pid,args'], (err, out) => { + if (err) { reject(err); return; } + resolve(out); + }); + }); + + // Match shellper-main.js processes whose JSON config cwd is this worktree. + // The shellper is spawned with JSON as argv[2]: {"cwd":"/path/to/worktree",...} + const cwdPattern = `"cwd":"${worktreePath}"`; + + for (const line of stdout.split('\n')) { + if (!line.includes('shellper-main.js')) continue; + if (!line.includes(cwdPattern)) continue; + + const pid = parseInt(line.trim(), 10); + if (isNaN(pid) || pid <= 0 || pid === process.pid) continue; + + try { + // Kill process group (shellper + its PTY child) to prevent orphaned + // PTY processes. Shellper is spawned with detached:true, so it's a + // process group leader. + process.kill(-pid, 'SIGTERM'); + killed++; + } catch { + // Process group kill failed — try individual PID + try { + process.kill(pid, 'SIGTERM'); + killed++; + } catch { + // Process already dead + } + } + } + } catch { + // ps not available or failed — non-fatal + } + return killed; +} + export interface CleanupOptions { project?: string; issue?: number; @@ -202,6 +257,16 @@ async function cleanupBuilder(builder: Builder, force?: boolean, issueNumber?: n } } + // Bugfix #389: Kill shellper processes directly by worktree path. + // The Tower API kill may fail if Tower isn't running, the terminal was already + // removed, or Tower was restarted. This catches any surviving shellper processes. + if (!isShellMode && builder.worktree) { + const shellpersKilled = await killShellperProcesses(builder.worktree); + if (shellpersKilled > 0) { + logger.info(`Killed ${shellpersKilled} shellper process(es)`); + } + } + // For ephemeral builders (bugfix, task): actually remove worktree and delete branches if (isEphemeral && !isShellMode) { // Remove worktree