Skip to content

Commit 2dd5e75

Browse files
committed
🤖 fix: register background executors for existing workspaces after restart
- Add ensureExecutorRegistered() called from getOrCreateSession() - Add Config.getWorkspaceMetadataSync() for sync metadata lookup - Make registerExecutor() idempotent (no-op if already registered) Without this fix, background execution tools failed for all pre-existing workspaces after app restart with 'No executor registered' error.
1 parent caf0c3e commit 2dd5e75

File tree

9 files changed

+101
-23
lines changed

9 files changed

+101
-23
lines changed

‎src/node/config.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,33 @@ 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+
363390
/**
364391
* Add a workspace to config.json (single source of truth for workspace metadata).
365392
* Creates project entry if it doesn't exist.

‎src/node/services/backgroundProcessManager.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,14 @@ export class BackgroundProcessManager {
3333

3434
/**
3535
* Register an executor for a workspace.
36-
* Called when workspace is created - local workspaces get LocalBackgroundExecutor,
37-
* SSH workspaces get SSHBackgroundExecutor.
36+
* Called when workspace is created or session is started for existing workspace.
37+
* Local workspaces get LocalBackgroundExecutor, SSH workspaces get SSHBackgroundExecutor.
38+
* Idempotent - no-op if executor already registered.
3839
*/
3940
registerExecutor(workspaceId: string, executor: BackgroundExecutor): void {
41+
if (this.executors.has(workspaceId)) {
42+
return; // Already registered
43+
}
4044
log.debug(`BackgroundProcessManager.registerExecutor(${workspaceId})`);
4145
this.executors.set(workspaceId, executor);
4246
}
@@ -65,7 +69,8 @@ export class BackgroundProcessManager {
6569
if (!executor) {
6670
return {
6771
success: false,
68-
error: "No executor registered for this workspace. Background execution may not be supported.",
72+
error:
73+
"No executor registered for this workspace. Background execution may not be supported.",
6974
};
7075
}
7176

@@ -194,7 +199,7 @@ export class BackgroundProcessManager {
194199
proc.exitTime ??= Date.now();
195200
// Ensure handle is cleaned up even on error
196201
if (proc.handle) {
197-
await proc.handle.dispose().catch(() => {});
202+
await proc.handle.dispose();
198203
}
199204
return { success: true };
200205
}

‎src/node/services/ipcMain.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,25 @@ export class IpcMain {
121121
return new LocalBackgroundExecutor(this.bashExecutionService);
122122
}
123123

124+
/**
125+
* Ensure a background executor is registered for a workspace.
126+
* Called when creating sessions for existing workspaces (after app restart).
127+
* No-op if executor already registered or workspace not found (new workspace being created).
128+
*/
129+
private ensureExecutorRegistered(workspaceId: string): void {
130+
// Look up workspace metadata synchronously from config
131+
const metadata = this.config.getWorkspaceMetadataSync(workspaceId);
132+
if (!metadata) {
133+
// Workspace not in config yet - executor will be registered by creation path
134+
return;
135+
}
136+
137+
const runtime = createRuntime(metadata.runtimeConfig);
138+
const executor = this.createBackgroundExecutor(metadata.runtimeConfig, runtime);
139+
// registerExecutor is idempotent - no-op if already registered
140+
this.backgroundProcessManager.registerExecutor(workspaceId, executor);
141+
}
142+
124143
/**
125144
* Initialize the service. Call this after construction.
126145
* This is separate from the constructor to support async initialization.
@@ -420,6 +439,9 @@ export class IpcMain {
420439
return session;
421440
}
422441

442+
// Ensure executor is registered for existing workspaces (handles app restart case)
443+
this.ensureExecutorRegistered(trimmed);
444+
423445
session = new AgentSession({
424446
workspaceId: trimmed,
425447
config: this.config,

‎src/node/services/localBackgroundExecutor.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ class LocalBackgroundHandle implements BackgroundHandle {
8888
}
8989
}
9090

91-
async isRunning(): Promise<boolean> {
92-
return this.disposable.child.exitCode === null;
91+
isRunning(): Promise<boolean> {
92+
return Promise.resolve(this.disposable.child.exitCode === null);
9393
}
9494

9595
async terminate(): Promise<void> {
@@ -126,8 +126,8 @@ class LocalBackgroundHandle implements BackgroundHandle {
126126
this.terminated = true;
127127
}
128128

129-
async dispose(): Promise<void> {
130-
this.disposable[Symbol.dispose]();
129+
dispose(): Promise<void> {
130+
return Promise.resolve(this.disposable[Symbol.dispose]());
131131
}
132132

133133
/** Get the child process (for spawn event waiting) */
@@ -145,7 +145,9 @@ export class LocalBackgroundExecutor implements BackgroundExecutor {
145145
async spawn(script: string, config: BackgroundExecConfig): Promise<BackgroundSpawnResult> {
146146
log.debug(`LocalBackgroundExecutor: Spawning background process in ${config.cwd}`);
147147

148-
// Create handle first so we can wire up callbacks
148+
// Declare handle before executeStreaming so callbacks can reference it via closure.
149+
// It's assigned immediately after executeStreaming returns, before any callbacks fire.
150+
// eslint-disable-next-line prefer-const
149151
let handle: LocalBackgroundHandle;
150152

151153
// Spawn with streaming callbacks that forward to handle
@@ -190,9 +192,7 @@ export class LocalBackgroundExecutor implements BackgroundExecutor {
190192
return { success: false, error: err.message };
191193
}
192194

193-
log.debug(
194-
`LocalBackgroundExecutor: Process spawned with PID ${handle.child.pid ?? "unknown"}`
195-
);
195+
log.debug(`LocalBackgroundExecutor: Process spawned with PID ${handle.child.pid ?? "unknown"}`);
196196
return { success: true, handle };
197197
}
198198
}

‎src/node/services/sshBackgroundExecutor.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,12 @@ export class SSHBackgroundExecutor implements BackgroundExecutor {
3232
// Runtime will be used for SSH commands when implemented
3333
}
3434

35-
async spawn(_script: string, _config: BackgroundExecConfig): Promise<BackgroundSpawnResult> {
35+
spawn(_script: string, _config: BackgroundExecConfig): Promise<BackgroundSpawnResult> {
3636
// TODO: Implement SSH background execution
3737
// See file header for implementation approach
38-
return {
38+
return Promise.resolve({
3939
success: false,
4040
error: "SSH background execution is not yet implemented",
41-
};
41+
});
4242
}
4343
}

‎src/node/services/tools/bash.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1368,7 +1368,10 @@ describe("bash tool - background execution", () => {
13681368

13691369
it("should reject timeout with background mode", async () => {
13701370
const manager = new BackgroundProcessManager();
1371-
manager.registerExecutor("test-workspace", new LocalBackgroundExecutor(new BashExecutionService()));
1371+
manager.registerExecutor(
1372+
"test-workspace",
1373+
new LocalBackgroundExecutor(new BashExecutionService())
1374+
);
13721375

13731376
const tempDir = new TestTempDir("test-bash-bg");
13741377
const config = createTestToolConfig(process.cwd());
@@ -1395,7 +1398,10 @@ describe("bash tool - background execution", () => {
13951398

13961399
it("should start background process and return process ID", async () => {
13971400
const manager = new BackgroundProcessManager();
1398-
manager.registerExecutor("test-workspace", new LocalBackgroundExecutor(new BashExecutionService()));
1401+
manager.registerExecutor(
1402+
"test-workspace",
1403+
new LocalBackgroundExecutor(new BashExecutionService())
1404+
);
13991405

14001406
const tempDir = new TestTempDir("test-bash-bg");
14011407
const config = createTestToolConfig(process.cwd());

‎src/node/services/tools/bash_background_list.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,14 @@ describe("bash_background_list tool", () => {
110110

111111
it("should only list processes for the current workspace", async () => {
112112
const manager = new BackgroundProcessManager();
113-
manager.registerExecutor("workspace-a", new LocalBackgroundExecutor(new BashExecutionService()));
114-
manager.registerExecutor("workspace-b", new LocalBackgroundExecutor(new BashExecutionService()));
113+
manager.registerExecutor(
114+
"workspace-a",
115+
new LocalBackgroundExecutor(new BashExecutionService())
116+
);
117+
manager.registerExecutor(
118+
"workspace-b",
119+
new LocalBackgroundExecutor(new BashExecutionService())
120+
);
115121

116122
const tempDir = new TestTempDir("test-bash-bg-list");
117123
const config = createTestToolConfig(process.cwd(), { workspaceId: "workspace-a" });

‎src/node/services/tools/bash_background_read.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,14 @@ describe("bash_background_read tool", () => {
206206

207207
it("should not read processes from other workspaces", async () => {
208208
const manager = new BackgroundProcessManager();
209-
manager.registerExecutor("workspace-a", new LocalBackgroundExecutor(new BashExecutionService()));
210-
manager.registerExecutor("workspace-b", new LocalBackgroundExecutor(new BashExecutionService()));
209+
manager.registerExecutor(
210+
"workspace-a",
211+
new LocalBackgroundExecutor(new BashExecutionService())
212+
);
213+
manager.registerExecutor(
214+
"workspace-b",
215+
new LocalBackgroundExecutor(new BashExecutionService())
216+
);
211217

212218
const tempDir = new TestTempDir("test-bash-bg-read");
213219
// Config is for workspace-a

‎src/node/services/tools/bash_background_terminate.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,14 @@ describe("bash_background_terminate tool", () => {
144144

145145
it("should not terminate processes from other workspaces", async () => {
146146
const manager = new BackgroundProcessManager();
147-
manager.registerExecutor("workspace-a", new LocalBackgroundExecutor(new BashExecutionService()));
148-
manager.registerExecutor("workspace-b", new LocalBackgroundExecutor(new BashExecutionService()));
147+
manager.registerExecutor(
148+
"workspace-a",
149+
new LocalBackgroundExecutor(new BashExecutionService())
150+
);
151+
manager.registerExecutor(
152+
"workspace-b",
153+
new LocalBackgroundExecutor(new BashExecutionService())
154+
);
149155

150156
const tempDir = new TestTempDir("test-bash-bg-term");
151157
// Config is for workspace-a

0 commit comments

Comments
 (0)