Skip to content

Commit fbb4a9d

Browse files
committed
🤖 refactor: lazy executor pattern for background processes
Moves from pre-registration to lazy executor creation/caching: - Remove registerExecutor()/unregisterExecutor() from BackgroundProcessManager - spawn() now takes executor as first arg, caches per workspace - AIService creates executor alongside runtime in streamMessage() - Remove ensureExecutorRegistered() and getWorkspaceMetadataSync() - Remove 3 registration calls from IpcMain workspace creation paths This follows the existing pattern where runtime is created on-demand from metadata rather than pre-registered. Executors are still cached for SSH connection pooling. Net ~100 LoC removed.
1 parent 2dd5e75 commit fbb4a9d

File tree

11 files changed

+129
-235
lines changed

11 files changed

+129
-235
lines changed

src/common/utils/tools/tools.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { log } from "@/node/services/log";
1616
import type { Runtime } from "@/node/runtime/Runtime";
1717
import type { InitStateManager } from "@/node/services/initStateManager";
1818
import type { BackgroundProcessManager } from "@/node/services/backgroundProcessManager";
19+
import type { BackgroundExecutor } from "@/node/services/backgroundExecutor";
1920

2021
/**
2122
* Configuration for tools that need runtime context
@@ -35,6 +36,8 @@ export interface ToolConfiguration {
3536
overflow_policy?: "truncate" | "tmpfile";
3637
/** Background process manager for bash tool (optional, AI-only) */
3738
backgroundProcessManager?: BackgroundProcessManager;
39+
/** Background executor for this workspace (optional, AI-only) */
40+
backgroundExecutor?: BackgroundExecutor;
3841
/** Workspace ID for tracking background processes (optional for token estimation) */
3942
workspaceId?: string;
4043
}

src/node/config.ts

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -360,33 +360,6 @@ export class Config {
360360
return workspaceMetadata;
361361
}
362362

363-
/**
364-
* Get workspace metadata by ID synchronously.
365-
* Used for executor registration when creating sessions for existing workspaces.
366-
* Only returns workspaces that have complete metadata in config (not legacy).
367-
*/
368-
getWorkspaceMetadataSync(workspaceId: string): WorkspaceMetadata | null {
369-
const config = this.loadConfigOrDefault();
370-
371-
for (const [projectPath, projectConfig] of config.projects) {
372-
for (const workspace of projectConfig.workspaces) {
373-
// Only check new format workspaces (have id and name in config)
374-
if (workspace.id === workspaceId && workspace.name) {
375-
return {
376-
id: workspace.id,
377-
name: workspace.name,
378-
projectName: this.getProjectName(projectPath),
379-
projectPath,
380-
createdAt: workspace.createdAt,
381-
runtimeConfig: workspace.runtimeConfig ?? DEFAULT_RUNTIME_CONFIG,
382-
};
383-
}
384-
}
385-
}
386-
387-
return null;
388-
}
389-
390363
/**
391364
* Add a workspace to config.json (single source of truth for workspace metadata).
392365
* Creates project entry if it doesn't exist.

src/node/services/aiService.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ import { createRuntime } from "@/node/runtime/runtimeFactory";
2121
import { secretsToRecord } from "@/common/types/secrets";
2222
import type { MuxProviderOptions } from "@/common/types/providerOptions";
2323
import type { BackgroundProcessManager } from "@/node/services/backgroundProcessManager";
24+
import type { BackgroundExecutor } from "@/node/services/backgroundExecutor";
25+
import { LocalBackgroundExecutor } from "@/node/services/localBackgroundExecutor";
26+
import { SSHBackgroundExecutor } from "@/node/services/sshBackgroundExecutor";
27+
import { BashExecutionService } from "@/node/services/bashExecutionService";
28+
import { isSSHRuntime, type RuntimeConfig } from "@/common/types/runtime";
29+
import type { Runtime } from "@/node/runtime/Runtime";
2430
import { log } from "./log";
2531
import {
2632
transformModelMessages,
@@ -143,6 +149,7 @@ export class AIService extends EventEmitter {
143149
private readonly mockModeEnabled: boolean;
144150
private readonly mockScenarioPlayer?: MockScenarioPlayer;
145151
private readonly backgroundProcessManager?: BackgroundProcessManager;
152+
private readonly bashExecutionService: BashExecutionService;
146153

147154
constructor(
148155
config: Config,
@@ -160,6 +167,7 @@ export class AIService extends EventEmitter {
160167
this.partialService = partialService;
161168
this.initStateManager = initStateManager;
162169
this.backgroundProcessManager = backgroundProcessManager;
170+
this.bashExecutionService = new BashExecutionService();
163171
this.streamManager = new StreamManager(historyService, partialService);
164172
void this.ensureSessionsDir();
165173
this.setupStreamEventForwarding();
@@ -173,6 +181,20 @@ export class AIService extends EventEmitter {
173181
}
174182
}
175183

184+
/**
185+
* Create a background executor appropriate for the given runtime.
186+
* Local runtimes use LocalBackgroundExecutor, SSH runtimes use SSHBackgroundExecutor.
187+
*/
188+
private createBackgroundExecutor(
189+
runtimeConfig: RuntimeConfig,
190+
runtime: Runtime
191+
): BackgroundExecutor {
192+
if (isSSHRuntime(runtimeConfig)) {
193+
return new SSHBackgroundExecutor(runtime);
194+
}
195+
return new LocalBackgroundExecutor(this.bashExecutionService);
196+
}
197+
176198
/**
177199
* Forward all stream events from StreamManager to AIService consumers
178200
*/
@@ -719,16 +741,21 @@ export class AIService extends EventEmitter {
719741
}
720742

721743
// Get workspace path - handle both worktree and in-place modes
722-
const runtime = createRuntime(
723-
metadata.runtimeConfig ?? { type: "local", srcBaseDir: this.config.srcDir }
724-
);
744+
const runtimeConfig = metadata.runtimeConfig ?? {
745+
type: "local" as const,
746+
srcBaseDir: this.config.srcDir,
747+
};
748+
const runtime = createRuntime(runtimeConfig);
725749
// In-place workspaces (CLI/benchmarks) have projectPath === name
726750
// Use path directly instead of reconstructing via getWorkspacePath
727751
const isInPlace = metadata.projectPath === metadata.name;
728752
const workspacePath = isInPlace
729753
? metadata.projectPath
730754
: runtime.getWorkspacePath(metadata.projectPath, metadata.name);
731755

756+
// Create background executor for this workspace (cached on first spawn)
757+
const backgroundExecutor = this.createBackgroundExecutor(runtimeConfig, runtime);
758+
732759
// Build system message from workspace metadata
733760
const systemMessage = await buildSystemMessage(
734761
metadata,
@@ -767,6 +794,7 @@ export class AIService extends EventEmitter {
767794
secrets: secretsToRecord(projectSecrets),
768795
runtimeTempDir,
769796
backgroundProcessManager: this.backgroundProcessManager,
797+
backgroundExecutor,
770798
workspaceId,
771799
},
772800
workspaceId,

src/node/services/backgroundProcessManager.test.ts

Lines changed: 27 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,27 @@ import { describe, it, expect, beforeEach } from "bun:test";
22
import { BackgroundProcessManager } from "./backgroundProcessManager";
33
import { BashExecutionService } from "./bashExecutionService";
44
import { LocalBackgroundExecutor } from "./localBackgroundExecutor";
5+
import type { BackgroundExecutor } from "./backgroundExecutor";
56

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;
7+
// Create a shared executor for tests
8+
function createTestExecutor(): BackgroundExecutor {
9+
return new LocalBackgroundExecutor(new BashExecutionService());
1110
}
1211

1312
describe("BackgroundProcessManager", () => {
1413
let manager: BackgroundProcessManager;
14+
let executor: BackgroundExecutor;
1515
const testWorkspaceId = "workspace-1";
1616
const testWorkspaceId2 = "workspace-2";
1717

1818
beforeEach(() => {
19-
manager = createManagerWithExecutor(testWorkspaceId);
19+
manager = new BackgroundProcessManager();
20+
executor = createTestExecutor();
2021
});
2122

2223
describe("spawn", () => {
2324
it("should spawn a background process and return process ID", async () => {
24-
const result = await manager.spawn(testWorkspaceId, "echo hello", {
25+
const result = await manager.spawn(executor, testWorkspaceId, "echo hello", {
2526
cwd: process.cwd(),
2627
});
2728

@@ -31,28 +32,16 @@ describe("BackgroundProcessManager", () => {
3132
}
3233
});
3334

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-
4635
it("should return error on spawn failure", async () => {
47-
const result = await manager.spawn(testWorkspaceId, "echo test", {
36+
const result = await manager.spawn(executor, testWorkspaceId, "echo test", {
4837
cwd: "/nonexistent/path/that/does/not/exist",
4938
});
5039

5140
expect(result.success).toBe(false);
5241
});
5342

5443
it("should capture stdout and stderr", async () => {
55-
const result = await manager.spawn(testWorkspaceId, "echo hello; echo world >&2", {
44+
const result = await manager.spawn(executor, testWorkspaceId, "echo hello; echo world >&2", {
5645
cwd: process.cwd(),
5746
});
5847

@@ -75,7 +64,7 @@ describe("BackgroundProcessManager", () => {
7564
.map((_, i) => `echo line${i}`)
7665
.join("; ");
7766

78-
const result = await manager.spawn(testWorkspaceId, script, {
67+
const result = await manager.spawn(executor, testWorkspaceId, script, {
7968
cwd: process.cwd(),
8069
});
8170

@@ -93,7 +82,7 @@ describe("BackgroundProcessManager", () => {
9382

9483
describe("getProcess", () => {
9584
it("should return process by ID", async () => {
96-
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 1", {
85+
const spawnResult = await manager.spawn(executor, testWorkspaceId, "sleep 1", {
9786
cwd: process.cwd(),
9887
});
9988

@@ -113,22 +102,18 @@ describe("BackgroundProcessManager", () => {
113102

114103
describe("list", () => {
115104
it("should list all processes", async () => {
116-
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
117-
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
105+
await manager.spawn(executor, testWorkspaceId, "sleep 1", { cwd: process.cwd() });
106+
await manager.spawn(executor, testWorkspaceId, "sleep 1", { cwd: process.cwd() });
118107

119108
const processes = manager.list();
120109
expect(processes.length).toBeGreaterThanOrEqual(2);
121110
});
122111

123112
it("should filter by workspace ID", async () => {
124-
// Register second workspace executor
125-
manager.registerExecutor(
126-
testWorkspaceId2,
127-
new LocalBackgroundExecutor(new BashExecutionService())
128-
);
113+
const executor2 = createTestExecutor();
129114

130-
await manager.spawn(testWorkspaceId, "sleep 1", { cwd: process.cwd() });
131-
await manager.spawn(testWorkspaceId2, "sleep 1", { cwd: process.cwd() });
115+
await manager.spawn(executor, testWorkspaceId, "sleep 1", { cwd: process.cwd() });
116+
await manager.spawn(executor2, testWorkspaceId2, "sleep 1", { cwd: process.cwd() });
132117

133118
const ws1Processes = manager.list(testWorkspaceId);
134119
const ws2Processes = manager.list(testWorkspaceId2);
@@ -142,7 +127,7 @@ describe("BackgroundProcessManager", () => {
142127

143128
describe("terminate", () => {
144129
it("should terminate a running process", async () => {
145-
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 10", {
130+
const spawnResult = await manager.spawn(executor, testWorkspaceId, "sleep 10", {
146131
cwd: process.cwd(),
147132
});
148133

@@ -161,7 +146,7 @@ describe("BackgroundProcessManager", () => {
161146
});
162147

163148
it("should be idempotent (double-terminate succeeds)", async () => {
164-
const spawnResult = await manager.spawn(testWorkspaceId, "sleep 10", {
149+
const spawnResult = await manager.spawn(executor, testWorkspaceId, "sleep 10", {
165150
cwd: process.cwd(),
166151
});
167152

@@ -176,29 +161,19 @@ describe("BackgroundProcessManager", () => {
176161
});
177162

178163
describe("cleanup", () => {
179-
it("should kill all processes for a workspace and remove them from memory", async () => {
180-
// Register second workspace executor
181-
manager.registerExecutor(
182-
testWorkspaceId2,
183-
new LocalBackgroundExecutor(new BashExecutionService())
184-
);
164+
it("should kill all processes for a workspace and clear cached executor", async () => {
165+
const executor2 = createTestExecutor();
185166

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() });
167+
await manager.spawn(executor, testWorkspaceId, "sleep 10", { cwd: process.cwd() });
168+
await manager.spawn(executor, testWorkspaceId, "sleep 10", { cwd: process.cwd() });
169+
await manager.spawn(executor2, testWorkspaceId2, "sleep 10", { cwd: process.cwd() });
189170

190171
await manager.cleanup(testWorkspaceId);
191172

192173
const ws1Processes = manager.list(testWorkspaceId);
193174
const ws2Processes = manager.list(testWorkspaceId2);
194175
// All testWorkspaceId processes should be removed from memory
195176
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-
}
202177
// workspace-2 processes should still exist and be running
203178
expect(ws2Processes.length).toBeGreaterThanOrEqual(1);
204179
expect(ws2Processes.some((p) => p.status === "running")).toBe(true);
@@ -207,7 +182,7 @@ describe("BackgroundProcessManager", () => {
207182

208183
describe("process state tracking", () => {
209184
it("should track process exit", async () => {
210-
const result = await manager.spawn(testWorkspaceId, "exit 42", {
185+
const result = await manager.spawn(executor, testWorkspaceId, "exit 42", {
211186
cwd: process.cwd(),
212187
});
213188

@@ -223,7 +198,7 @@ describe("BackgroundProcessManager", () => {
223198
});
224199

225200
it("should keep buffer after process exits", async () => {
226-
const result = await manager.spawn(testWorkspaceId, "echo test; exit 0", {
201+
const result = await manager.spawn(executor, testWorkspaceId, "echo test; exit 0", {
227202
cwd: process.cwd(),
228203
});
229204

@@ -238,7 +213,7 @@ describe("BackgroundProcessManager", () => {
238213

239214
it("should preserve killed status after onExit callback fires", async () => {
240215
// Spawn a long-running process
241-
const result = await manager.spawn(testWorkspaceId, "sleep 60", {
216+
const result = await manager.spawn(executor, testWorkspaceId, "sleep 60", {
242217
cwd: process.cwd(),
243218
});
244219

0 commit comments

Comments
 (0)