Skip to content

Commit caf0c3e

Browse files
committed
🤖 refactor: extract BackgroundExecutor interface for runtime-agnostic background execution
- Add BackgroundExecutor interface and BackgroundHandle for abstracting process spawning - Create LocalBackgroundExecutor wrapping BashExecutionService for local processes - Add SSHBackgroundExecutor stub (returns 'not yet implemented' error) - BackgroundProcessManager now uses per-workspace executor registration - Remove pid from public API (runtime-specific detail) - Remove supportsBackgroundExecution from Runtime interface (executor error suffices) - Share single BashExecutionService instance across all local executors This refactor prepares for SSH background execution support by making the execution backend swappable per workspace.
1 parent d681ceb commit caf0c3e

17 files changed

+554
-166
lines changed

‎src/common/types/tools.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,6 @@ export type BashBackgroundReadResult =
212212
process_id: string;
213213
status: "running" | "exited" | "killed" | "failed";
214214
script: string;
215-
pid?: number; // OS process ID
216215
uptime_ms: number;
217216
exitCode?: number;
218217
stdout: string[];
@@ -238,7 +237,6 @@ export interface BashBackgroundListProcess {
238237
process_id: string;
239238
status: "running" | "exited" | "killed" | "failed";
240239
script: string;
241-
pid?: number;
242240
uptime_ms: number;
243241
exitCode?: number;
244242
}

‎src/node/runtime/LocalRuntime.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,6 @@ import { expandTilde } from "./tildeExpansion";
3939
export class LocalRuntime implements Runtime {
4040
private readonly srcBaseDir: string;
4141

42-
/** LocalRuntime supports background execution via detached process groups */
43-
readonly supportsBackgroundExecution = true;
44-
4542
constructor(srcBaseDir: string) {
4643
// Expand tilde to actual home directory path for local file system operations
4744
this.srcBaseDir = expandTilde(srcBaseDir);

‎src/node/runtime/Runtime.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -364,13 +364,6 @@ export interface Runtime {
364364
* @returns Result with new workspace path and source branch, or error
365365
*/
366366
forkWorkspace(params: WorkspaceForkParams): Promise<WorkspaceForkResult>;
367-
368-
/**
369-
* Whether this runtime supports background process execution.
370-
* LocalRuntime: true (uses detached process groups)
371-
* SSHRuntime: false (TODO: implement via nohup/setsid + file-based output capture)
372-
*/
373-
readonly supportsBackgroundExecution: boolean;
374367
}
375368

376369
/**

‎src/node/runtime/SSHRuntime.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,12 +71,6 @@ export class SSHRuntime implements Runtime {
7171
private readonly config: SSHRuntimeConfig;
7272
private readonly controlPath: string;
7373

74-
/**
75-
* SSHRuntime does not yet support background execution.
76-
* TODO: Implement via nohup/setsid + file-based output capture on remote host
77-
*/
78-
readonly supportsBackgroundExecution = false;
79-
8074
constructor(config: SSHRuntimeConfig) {
8175
// Note: srcBaseDir may contain tildes - they will be resolved via resolvePath() before use
8276
// The WORKSPACE_CREATE IPC handler resolves paths before storing in config
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/**
2+
* Background process execution abstraction.
3+
*
4+
* This interface allows BackgroundProcessManager to work with different
5+
* execution backends (local processes, SSH remote processes, etc.)
6+
*/
7+
8+
/**
9+
* Configuration for background execution
10+
*/
11+
export interface BackgroundExecConfig {
12+
/** Working directory for command execution */
13+
cwd: string;
14+
/** Environment variables to inject */
15+
env?: Record<string, string>;
16+
/** Process niceness level (-20 to 19) */
17+
niceness?: number;
18+
}
19+
20+
/**
21+
* Handle to a background process.
22+
* Abstracts away whether process is local or remote.
23+
*/
24+
export interface BackgroundHandle {
25+
/**
26+
* Register callback for stdout lines.
27+
* For local: called in real-time as output arrives.
28+
* For SSH: called when output is polled/read.
29+
*/
30+
onStdout(callback: (line: string) => void): void;
31+
32+
/**
33+
* Register callback for stderr lines.
34+
*/
35+
onStderr(callback: (line: string) => void): void;
36+
37+
/**
38+
* Register callback for process exit.
39+
* @param callback Receives exit code (128+signal for signal termination)
40+
*/
41+
onExit(callback: (exitCode: number) => void): void;
42+
43+
/**
44+
* Check if process is still running.
45+
* For local: checks ChildProcess.exitCode
46+
* For SSH: runs `kill -0 $PID` on remote
47+
*/
48+
isRunning(): Promise<boolean>;
49+
50+
/**
51+
* Terminate the process (SIGTERM → wait → SIGKILL).
52+
* For local: process.kill(-pid, signal)
53+
* For SSH: ssh "kill -TERM -$PID"
54+
*/
55+
terminate(): Promise<void>;
56+
57+
/**
58+
* Clean up resources (called after process exits or on error).
59+
* For local: disposes ChildProcess
60+
* For SSH: removes remote temp files
61+
*/
62+
dispose(): Promise<void>;
63+
}
64+
65+
/**
66+
* Result of spawning a background process
67+
*/
68+
export type BackgroundSpawnResult =
69+
| { success: true; handle: BackgroundHandle }
70+
| { success: false; error: string };
71+
72+
/**
73+
* Executor interface for spawning background processes.
74+
*
75+
* Implementations:
76+
* - LocalBackgroundExecutor: Uses BashExecutionService for local processes
77+
* - SSHBackgroundExecutor: Uses nohup/setsid + file-based output (TODO)
78+
*/
79+
export interface BackgroundExecutor {
80+
/**
81+
* Spawn a background process.
82+
* @param script Bash script to execute
83+
* @param config Execution configuration
84+
* @returns BackgroundHandle on success, or error
85+
*/
86+
spawn(script: string, config: BackgroundExecConfig): Promise<BackgroundSpawnResult>;
87+
}

‎src/node/services/backgroundProcessManager.test.ts‎

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,27 @@
11
import { describe, it, expect, beforeEach } from "bun:test";
22
import { BackgroundProcessManager } from "./backgroundProcessManager";
33
import { BashExecutionService } from "./bashExecutionService";
4+
import { LocalBackgroundExecutor } from "./localBackgroundExecutor";
5+
6+
// Helper to create manager with executor registered for a workspace
7+
function createManagerWithExecutor(workspaceId: string): BackgroundProcessManager {
8+
const manager = new BackgroundProcessManager();
9+
manager.registerExecutor(workspaceId, new LocalBackgroundExecutor(new BashExecutionService()));
10+
return manager;
11+
}
412

513
describe("BackgroundProcessManager", () => {
614
let manager: BackgroundProcessManager;
7-
let bashService: BashExecutionService;
15+
const testWorkspaceId = "workspace-1";
16+
const testWorkspaceId2 = "workspace-2";
817

918
beforeEach(() => {
10-
bashService = new BashExecutionService();
11-
manager = new BackgroundProcessManager(bashService);
19+
manager = createManagerWithExecutor(testWorkspaceId);
1220
});
1321

1422
describe("spawn", () => {
1523
it("should spawn a background process and return process ID", async () => {
16-
const result = await manager.spawn("workspace-1", "echo hello", {
24+
const result = await manager.spawn(testWorkspaceId, "echo hello", {
1725
cwd: process.cwd(),
1826
});
1927

@@ -23,16 +31,28 @@ describe("BackgroundProcessManager", () => {
2331
}
2432
});
2533

34+
it("should return error when no executor is registered", async () => {
35+
const managerNoExecutor = new BackgroundProcessManager();
36+
const result = await managerNoExecutor.spawn("unregistered-workspace", "echo hello", {
37+
cwd: process.cwd(),
38+
});
39+
40+
expect(result.success).toBe(false);
41+
if (!result.success) {
42+
expect(result.error).toContain("No executor registered");
43+
}
44+
});
45+
2646
it("should return error on spawn failure", async () => {
27-
const result = await manager.spawn("workspace-1", "echo test", {
47+
const result = await manager.spawn(testWorkspaceId, "echo test", {
2848
cwd: "/nonexistent/path/that/does/not/exist",
2949
});
3050

3151
expect(result.success).toBe(false);
3252
});
3353

3454
it("should capture stdout and stderr", async () => {
35-
const result = await manager.spawn("workspace-1", "echo hello; echo world >&2", {
55+
const result = await manager.spawn(testWorkspaceId, "echo hello; echo world >&2", {
3656
cwd: process.cwd(),
3757
});
3858

@@ -55,7 +75,7 @@ describe("BackgroundProcessManager", () => {
5575
.map((_, i) => `echo line${i}`)
5676
.join("; ");
5777

58-
const result = await manager.spawn("workspace-1", script, {
78+
const result = await manager.spawn(testWorkspaceId, script, {
5979
cwd: process.cwd(),
6080
});
6181

@@ -73,7 +93,7 @@ describe("BackgroundProcessManager", () => {
7393

7494
describe("getProcess", () => {
7595
it("should return process by ID", async () => {
76-
const spawnResult = await manager.spawn("workspace-1", "sleep 1", {
96+
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 1", {
7797
cwd: process.cwd(),
7898
});
7999

@@ -93,30 +113,36 @@ describe("BackgroundProcessManager", () => {
93113

94114
describe("list", () => {
95115
it("should list all processes", async () => {
96-
await manager.spawn("workspace-1", "sleep 1", { cwd: process.cwd() });
97-
await manager.spawn("workspace-1", "sleep 1", { cwd: process.cwd() });
116+
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
117+
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
98118

99119
const processes = manager.list();
100120
expect(processes.length).toBeGreaterThanOrEqual(2);
101121
});
102122

103123
it("should filter by workspace ID", async () => {
104-
await manager.spawn("workspace-1", "sleep 1", { cwd: process.cwd() });
105-
await manager.spawn("workspace-2", "sleep 1", { cwd: process.cwd() });
124+
// Register second workspace executor
125+
manager.registerExecutor(
126+
testWorkspaceId2,
127+
new LocalBackgroundExecutor(new BashExecutionService())
128+
);
106129

107-
const ws1Processes = manager.list("workspace-1");
108-
const ws2Processes = manager.list("workspace-2");
130+
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
131+
await manager.spawn(testWorkspaceId2, "sleep 1", { cwd: process.cwd() });
132+
133+
const ws1Processes = manager.list(testWorkspaceId);
134+
const ws2Processes = manager.list(testWorkspaceId2);
109135

110136
expect(ws1Processes.length).toBeGreaterThanOrEqual(1);
111137
expect(ws2Processes.length).toBeGreaterThanOrEqual(1);
112-
expect(ws1Processes.every((p) => p.workspaceId === "workspace-1")).toBe(true);
113-
expect(ws2Processes.every((p) => p.workspaceId === "workspace-2")).toBe(true);
138+
expect(ws1Processes.every((p) => p.workspaceId === testWorkspaceId)).toBe(true);
139+
expect(ws2Processes.every((p) => p.workspaceId === testWorkspaceId2)).toBe(true);
114140
});
115141
});
116142

117143
describe("terminate", () => {
118144
it("should terminate a running process", async () => {
119-
const spawnResult = await manager.spawn("workspace-1", "sleep 10", {
145+
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 10", {
120146
cwd: process.cwd(),
121147
});
122148

@@ -135,7 +161,7 @@ describe("BackgroundProcessManager", () => {
135161
});
136162

137163
it("should be idempotent (double-terminate succeeds)", async () => {
138-
const spawnResult = await manager.spawn("workspace-1", "sleep 10", {
164+
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 10", {
139165
cwd: process.cwd(),
140166
});
141167

@@ -151,17 +177,28 @@ describe("BackgroundProcessManager", () => {
151177

152178
describe("cleanup", () => {
153179
it("should kill all processes for a workspace and remove them from memory", async () => {
154-
await manager.spawn("workspace-1", "sleep 10", { cwd: process.cwd() });
155-
await manager.spawn("workspace-1", "sleep 10", { cwd: process.cwd() });
156-
await manager.spawn("workspace-2", "sleep 10", { cwd: process.cwd() });
180+
// Register second workspace executor
181+
manager.registerExecutor(
182+
testWorkspaceId2,
183+
new LocalBackgroundExecutor(new BashExecutionService())
184+
);
157185

158-
await manager.cleanup("workspace-1");
186+
await manager.spawn(testWorkspaceId, "sleep 10", { cwd: process.cwd() });
187+
await manager.spawn(testWorkspaceId, "sleep 10", { cwd: process.cwd() });
188+
await manager.spawn(testWorkspaceId2, "sleep 10", { cwd: process.cwd() });
159189

160-
const ws1Processes = manager.list("workspace-1");
161-
const ws2Processes = manager.list("workspace-2");
190+
await manager.cleanup(testWorkspaceId);
162191

163-
// All workspace-1 processes should be removed from memory
192+
const ws1Processes = manager.list(testWorkspaceId);
193+
const ws2Processes = manager.list(testWorkspaceId2);
194+
// All testWorkspaceId processes should be removed from memory
164195
expect(ws1Processes.length).toBe(0);
196+
// Executor should also be unregistered - spawning should fail
197+
const spawnResult = await manager.spawn(testWorkspaceId, "echo test", { cwd: process.cwd() });
198+
expect(spawnResult.success).toBe(false);
199+
if (!spawnResult.success) {
200+
expect(spawnResult.error).toContain("No executor registered");
201+
}
165202
// workspace-2 processes should still exist and be running
166203
expect(ws2Processes.length).toBeGreaterThanOrEqual(1);
167204
expect(ws2Processes.some((p) => p.status === "running")).toBe(true);
@@ -170,7 +207,7 @@ describe("BackgroundProcessManager", () => {
170207

171208
describe("process state tracking", () => {
172209
it("should track process exit", async () => {
173-
const result = await manager.spawn("workspace-1", "exit 42", {
210+
const result = await manager.spawn(testWorkspaceId, "exit 42", {
174211
cwd: process.cwd(),
175212
});
176213

@@ -186,7 +223,7 @@ describe("BackgroundProcessManager", () => {
186223
});
187224

188225
it("should keep buffer after process exits", async () => {
189-
const result = await manager.spawn("workspace-1", "echo test; exit 0", {
226+
const result = await manager.spawn(testWorkspaceId, "echo test; exit 0", {
190227
cwd: process.cwd(),
191228
});
192229

@@ -201,7 +238,7 @@ describe("BackgroundProcessManager", () => {
201238

202239
it("should preserve killed status after onExit callback fires", async () => {
203240
// Spawn a long-running process
204-
const result = await manager.spawn("workspace-1", "sleep 60", {
241+
const result = await manager.spawn(testWorkspaceId, "sleep 60", {
205242
cwd: process.cwd(),
206243
});
207244

0 commit comments

Comments
 (0)