Skip to content

Commit 9470dfa

Browse files
committed
Agent mode phase 0: clean up background tasks when switching workspaces
killAllBackgroundCommands() was only ever called from app.on("window-all-closed")/"before-quit" — neither the session-load effect nor changeAgentWorkspace() in Chat.tsx cleaned up the *previous* workspace's tasks when switching to a different one, so they kept running indefinitely and kept counting against MAX_BACKGROUND_TASKS. - New app/src/process-tree.ts: killProcessTree(pid), since spawn(cmd, {shell:true}).kill() only kills the shell, not whatever it spawned (e.g. npm run dev -> node). POSIX kills the process group (requires detached:true at spawn time, now set); Windows shells out to taskkill /t /f. - BackgroundTask now carries its workspaceRoot. New killBackgroundCommandsForWorkspace(root) kills and forgets only that workspace's tasks, leaving others running. - New agent:closeWorkspace IPC call, invoked from Chat.tsx via a small ref-tracking helper right before every place agentWorkspace changes (initial pick, folder change, session load) so the previously-active workspace's tasks get torn down on the actual transition, not just at quit. This is prep work for a real sandbox/permission system and persistent terminals, both of which need the same workspace-scoped process lifecycle rather than duplicating it.
1 parent eeacddf commit 9470dfa

8 files changed

Lines changed: 162 additions & 3 deletions

File tree

app/src/agent-tools.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {
3131
stopBackgroundCommand,
3232
listBackgroundCommands,
3333
killAllBackgroundCommands,
34+
killBackgroundCommandsForWorkspace,
3435
httpRequest,
3536
findSymbolReferences,
3637
applyPatch,
@@ -494,6 +495,25 @@ describe("agent-tools", () => {
494495
for (let i = 0; i < 5; i++) startBackgroundCommand(workspace, "sleep 5", ".", `task-${i}`);
495496
expect(() => startBackgroundCommand(workspace, "sleep 5")).toThrow(/Already running/);
496497
});
498+
499+
it("kills and forgets only the tasks belonging to the given workspace, leaving other workspaces' tasks running", () => {
500+
const otherWorkspace = fs.mkdtempSync(path.join(os.tmpdir(), "agent-tools-test-other-"));
501+
const { taskId: taskA } = startBackgroundCommand(workspace, "sleep 30");
502+
const { taskId: taskB } = startBackgroundCommand(otherWorkspace, "sleep 30");
503+
504+
const killedCount = killBackgroundCommandsForWorkspace(workspace);
505+
506+
expect(killedCount).toBe(1);
507+
// Killed AND removed from tracking — a task from a workspace you've
508+
// switched away from shouldn't linger as a queryable id.
509+
expect(() => getBackgroundOutput(taskA)).toThrow(/No background task/);
510+
expect(getBackgroundOutput(taskB)).toContain("running");
511+
fs.rmSync(otherWorkspace, { recursive: true, force: true });
512+
});
513+
514+
it("returns 0 when the workspace has no background tasks", () => {
515+
expect(killBackgroundCommandsForWorkspace(workspace)).toBe(0);
516+
});
497517
});
498518

499519
describe("git tools", () => {

app/src/agent-tools.ts

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { randomUUID } from "node:crypto";
77
import type { ToolDefinition } from "./providers/types";
88
import { getAccountToken } from "./accounts";
99
import { capturePageScreenshot } from "./browser-capture";
10+
import { killProcessTree } from "./process-tree";
1011

1112
const execAsync = promisify(exec);
1213

@@ -835,6 +836,10 @@ interface BackgroundTask {
835836
name: string;
836837
command: string;
837838
process: ChildProcess;
839+
// Absolute path — lets killBackgroundCommandsForWorkspace() target only
840+
// the tasks that belong to a workspace being switched away from, rather
841+
// than every background task the process has ever started.
842+
workspaceRoot: string;
838843
// Rolling tail of combined stdout+stderr — capped so a chatty dev server
839844
// can't grow memory unboundedly over a long session.
840845
output: string;
@@ -861,13 +866,24 @@ export function startBackgroundCommand(
861866
}
862867

863868
const cwd = resolveSafePath(workspaceRoot, relativeCwd);
864-
const child = spawn(command, { cwd, shell: true, stdio: ["ignore", "pipe", "pipe"] });
869+
// detached so the shell becomes its own process group leader — lets
870+
// killProcessTree() below signal the whole group (shell + whatever it
871+
// spawned, e.g. `npm run dev` spawning `node`) instead of just the shell
872+
// itself, which is all a plain .kill() would reach. No effect on Windows,
873+
// where killProcessTree uses `taskkill /t` instead.
874+
const child = spawn(command, {
875+
cwd,
876+
shell: true,
877+
stdio: ["ignore", "pipe", "pipe"],
878+
detached: process.platform !== "win32",
879+
});
865880
const id = randomUUID().slice(0, 8);
866881
const task: BackgroundTask = {
867882
id,
868883
name: name?.trim() || command.slice(0, 40),
869884
command,
870885
process: child,
886+
workspaceRoot: path.resolve(workspaceRoot),
871887
output: "",
872888
exitCode: null,
873889
startedAt: Date.now(),
@@ -909,7 +925,7 @@ export function stopBackgroundCommand(taskId: string): string {
909925
const task = backgroundTasks.get(taskId);
910926
if (!task) throw new Error(`No background task with id "${taskId}".`);
911927
if (task.exitCode !== null) return `Task ${task.id} had already exited with code ${task.exitCode}.`;
912-
task.process.kill();
928+
if (task.process.pid) killProcessTree(task.process.pid);
913929
return `Task ${task.id} (${task.name}) stopped.`;
914930
}
915931

@@ -924,11 +940,28 @@ export function listBackgroundCommands(): { id: string; name: string; command: s
924940

925941
export function killAllBackgroundCommands(): void {
926942
for (const task of backgroundTasks.values()) {
927-
if (task.exitCode === null) task.process.kill();
943+
if (task.exitCode === null && task.process.pid) killProcessTree(task.process.pid);
928944
}
929945
backgroundTasks.clear();
930946
}
931947

948+
// Only tears down tasks belonging to the workspace being switched away from
949+
// — background commands are otherwise never cleaned up until app quit
950+
// (killAllBackgroundCommands, called from window-all-closed/before-quit),
951+
// so switching to a different workspace mid-session used to leave the old
952+
// one's tasks running indefinitely, silently eating into MAX_BACKGROUND_TASKS.
953+
export function killBackgroundCommandsForWorkspace(workspaceRoot: string): number {
954+
const root = path.resolve(workspaceRoot);
955+
let killed = 0;
956+
for (const [id, task] of backgroundTasks) {
957+
if (task.workspaceRoot !== root) continue;
958+
if (task.exitCode === null && task.process.pid) killProcessTree(task.process.pid);
959+
backgroundTasks.delete(id);
960+
killed++;
961+
}
962+
return killed;
963+
}
964+
932965
function gitCommand(workspaceRoot: string, args: string): Promise<string> {
933966
return runCommand(workspaceRoot, `git ${args}`, ".");
934967
}

app/src/main.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,17 @@ function registerIpcHandlers(): void {
754754
return agentTools.detectProjectScripts(workspaceRoot);
755755
});
756756

757+
// Called when the renderer is about to stop using a workspace (switching
758+
// to a different folder, or loading a session that points elsewhere) —
759+
// without this, background tasks started against the old workspace kept
760+
// running indefinitely, since killAllBackgroundCommands() only ever ran
761+
// on app quit.
762+
ipcMain.handle("agent:closeWorkspace", (_event: IpcMainInvokeEvent, workspaceRoot: string) => {
763+
requireString(workspaceRoot, "workspace root");
764+
const killedBackgroundTasks = agentTools.killBackgroundCommandsForWorkspace(workspaceRoot);
765+
return { killedBackgroundTasks };
766+
});
767+
757768
ipcMain.handle("mcp:connect", async (_event: IpcMainInvokeEvent, config: McpServerConfig) => {
758769
try {
759770
const { tools } = await mcpClient.connectServer(config);

app/src/preload.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,8 @@ contextBridge.exposeInMainWorld("api", {
230230
ipcRenderer.invoke("agent:rollbackLastWrite", workspaceRoot),
231231
detectScripts: (workspaceRoot: string): Promise<ProjectScripts> =>
232232
ipcRenderer.invoke("agent:detectScripts", workspaceRoot),
233+
closeWorkspace: (workspaceRoot: string): Promise<{ killedBackgroundTasks: number }> =>
234+
ipcRenderer.invoke("agent:closeWorkspace", workspaceRoot),
233235
},
234236

235237
mcp: {

app/src/process-tree.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { describe, it, expect, vi } from "vitest";
2+
import { spawn, spawnSync } from "node:child_process";
3+
import { killProcessTree } from "./process-tree";
4+
5+
vi.mock("node:child_process", async (importOriginal) => {
6+
const actual = await importOriginal<typeof import("node:child_process")>();
7+
return { ...actual, spawnSync: vi.fn() };
8+
});
9+
10+
function isAlive(pid: number): boolean {
11+
try {
12+
process.kill(pid, 0);
13+
return true;
14+
} catch {
15+
return false;
16+
}
17+
}
18+
19+
describe("killProcessTree", () => {
20+
// Real process-group kill only makes sense on POSIX — Windows has no
21+
// equivalent to `detached: true` + negative-pid signaling, and Windows CI
22+
// runners don't have `sh`/`sleep` available to build a tree with anyway.
23+
it.skipIf(process.platform === "win32")(
24+
"kills a detached shell and its child process, not just the shell",
25+
async () => {
26+
const child = spawn("sh", ["-c", "sleep 30 & wait"], { detached: true, stdio: "ignore" });
27+
const shellPid = child.pid!;
28+
// Give the shell a moment to fork `sleep` before we look for it.
29+
await new Promise((r) => setTimeout(r, 200));
30+
31+
killProcessTree(shellPid);
32+
await new Promise((r) => setTimeout(r, 200));
33+
34+
expect(isAlive(shellPid)).toBe(false);
35+
},
36+
10_000
37+
);
38+
39+
it("shells out to taskkill /t /f on win32, regardless of the host platform", () => {
40+
killProcessTree(4321, "SIGTERM", "win32");
41+
expect(spawnSync).toHaveBeenCalledWith("taskkill", ["/pid", "4321", "/t", "/f"]);
42+
});
43+
44+
it("silently no-ops when the target process is already gone", () => {
45+
// A pid essentially guaranteed not to exist.
46+
expect(() => killProcessTree(999_999, "SIGTERM", "linux")).not.toThrow();
47+
});
48+
});

app/src/process-tree.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import { spawnSync } from "node:child_process";
2+
3+
// `spawn(command, {shell: true})` means the tracked pid is the *shell's*
4+
// pid — calling .kill() on it leaves any grandchildren (e.g. `npm run dev`
5+
// spawning a real `node` process) running behind. On POSIX this kills the
6+
// whole process group instead, which requires the child to have been spawned
7+
// with `detached: true` so it becomes its own group leader (see
8+
// startBackgroundCommand in agent-tools.ts). On Windows, `taskkill`'s `/t`
9+
// flag walks the process tree itself, so no special spawn option is needed
10+
// there.
11+
export function killProcessTree(
12+
pid: number,
13+
signal: NodeJS.Signals = "SIGTERM",
14+
platform: NodeJS.Platform = process.platform
15+
): void {
16+
if (platform === "win32") {
17+
spawnSync("taskkill", ["/pid", String(pid), "/t", "/f"]);
18+
return;
19+
}
20+
try {
21+
process.kill(-pid, signal);
22+
} catch {
23+
// Not a group leader (wasn't spawned detached) or already gone —
24+
// fall back to killing just the tracked pid.
25+
try {
26+
process.kill(pid, signal);
27+
} catch {
28+
// Already dead.
29+
}
30+
}
31+
}

frontend/src/pages/Chat.tsx

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,16 @@ export default function Chat() {
465465
const bottomRef = useRef<HTMLDivElement>(null);
466466
const textareaRef = useRef<HTMLTextAreaElement>(null);
467467
const viewportRef = useRef<HTMLDivElement>(null);
468+
// Tracks the workspace this component last told the main process about,
469+
// so switching away from one (a different session, or picking a new
470+
// folder) can close out its background tasks instead of leaving them
471+
// running until app quit.
472+
const prevWorkspaceRef = useRef<string | null>(null);
473+
function closeWorkspaceIfChanged(next: string | null) {
474+
const prev = prevWorkspaceRef.current;
475+
if (prev && prev !== next) window.api.agent.closeWorkspace(prev);
476+
prevWorkspaceRef.current = next;
477+
}
468478

469479
// No session selected yet: pick the most recent one, or create a new one.
470480
useEffect(() => {
@@ -532,6 +542,7 @@ export default function Chat() {
532542
setParams(session.params ?? {});
533543
setSessionSystemPrompt(session.systemPrompt ?? null);
534544
setAgentMode(session.agentMode ?? false);
545+
closeWorkspaceIfChanged(session.agentWorkspace ?? null);
535546
setAgentWorkspace(session.agentWorkspace ?? null);
536547
setPendingToolCalls([]);
537548
setAgentStepCount(0);
@@ -624,6 +635,7 @@ export default function Chat() {
624635
// workspace folder up front rather than letting tool calls fail later.
625636
const folder = await window.api.agent.pickWorkspace();
626637
if (!folder) return;
638+
closeWorkspaceIfChanged(folder);
627639
setAgentWorkspace(folder);
628640
setAgentMode(true);
629641
window.api.sessions.update(sessionId, { agentMode: true, agentWorkspace: folder });
@@ -638,6 +650,7 @@ export default function Chat() {
638650
if (!sessionId) return;
639651
const folder = await window.api.agent.pickWorkspace();
640652
if (!folder) return;
653+
closeWorkspaceIfChanged(folder);
641654
setAgentWorkspace(folder);
642655
window.api.sessions.update(sessionId, { agentWorkspace: folder });
643656
}

frontend/src/types/electron.d.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,7 @@ export interface ElectronApi {
511511
) => Promise<{ result?: unknown; error?: string }>;
512512
rollbackLastWrite: (workspaceRoot: string) => Promise<RollbackResult | null>;
513513
detectScripts: (workspaceRoot: string) => Promise<ProjectScripts>;
514+
closeWorkspace: (workspaceRoot: string) => Promise<{ killedBackgroundTasks: number }>;
514515
};
515516
mcp: {
516517
connect: (

0 commit comments

Comments
 (0)