From 4b49284baed5a0547812773671c4edefb24da475 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Tue, 17 Feb 2026 01:12:44 -0800 Subject: [PATCH] Fix #389: Kill shellper processes during af cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, af cleanup relied solely on the Tower REST API to kill shellper processes. This silently failed when Tower wasn't running, the terminal was already removed from Tower's registry, or Tower was restarted — leaving shellper processes (and their PTY children like Claude) orphaned and consuming resources. Added killShellperProcesses() that searches ps output for shellper-main.js processes whose JSON config cwd matches the builder's worktree path, and kills them via process group signal (SIGTERM to -pid). This runs after the Tower API kill attempt as a reliable fallback for all non-shell builders. --- .../__tests__/cleanup-shellper-kill.test.ts | 166 ++++++++++++++++++ .../codev/src/agent-farm/commands/cleanup.ts | 65 +++++++ 2 files changed, 231 insertions(+) create mode 100644 packages/codev/src/agent-farm/__tests__/cleanup-shellper-kill.test.ts 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