fix(coordinator): cleanup failure retained — MCP_TaskCleanupFailed, Docker kill awaited (johannesjo#41)

Author: brooksc

electron/ipc/channels.ts

@@ -161,4 +161,5 @@ export enum IPC {
   MCP_TaskHydrated = 'mcp_task_hydrated',
   MCP_StaleUrlWarning = 'mcp_stale_url_warning',
   MCP_CoordinatedTaskClosed = 'mcp_coordinated_task_closed',
+  MCP_TaskCleanupFailed = 'mcp_task_cleanup_failed',
 }

electron/mcp/coordinator.test.ts

@@ -3,6 +3,21 @@ import os from 'os';
 import { join, dirname } from 'path';
 
 // --- fs / child_process mocks (must come before dynamic import) ---
+const mockExecFile = vi.fn(
+  (
+    _cmd: string,
+    _args: string[],
+    _opts: unknown,
+    cb: (err: Error | null, stdout: string, stderr: string) => void,
+  ) => {
+    cb(null, '', '');
+  },
+);
+
+vi.mock('child_process', () => ({
+  execFile: mockExecFile,
+}));
+
 const mockWriteFileSync = vi.fn();
 const mockReadFileSync = vi.fn(() => '# existing\n');
 const mockExistsSync = vi.fn(() => false);
@@ -76,6 +91,7 @@ vi.mock('../ipc/channels.js', () => ({
   IPC: {
     MCP_TaskCreated: 'mcp_task_created',
     MCP_TaskClosed: 'mcp_task_closed',
+    MCP_TaskCleanupFailed: 'mcp_task_cleanup_failed',
     MCP_TaskStateSync: 'mcp_task_state_sync',
     MCP_CoordinatorNotificationStaged: 'mcp_coordinator_notification_staged',
     MCP_CoordinatorNotificationCleared: 'mcp_coordinator_notification_cleared',
@@ -1796,16 +1812,37 @@ describe('Coordinator cleanupTask — failure resilience', () => {
     coordinator.registerCoordinator('coord-1', 'proj-1');
   });
 
-  it('deleteTask failure is swallowed and task is removed from map', async () => {
+  it('deleteTask failure retains task in map and emits MCP_TaskCleanupFailed, not MCP_TaskClosed', async () => {
     const { deleteTask: mockDeleteTask } =
       await vi.importMock<typeof import('../ipc/tasks.js')>('../ipc/tasks.js');
     vi.mocked(mockDeleteTask).mockRejectedValueOnce(new Error('delete failed'));
 
     await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
     await coordinator.closeTask('task-1');
 
-    expect(coordinator.getTask('task-1')).toBeUndefined();
-    expect(mockNotifyRenderer).toHaveBeenCalledWith('mcp_task_closed', { taskId: 'task-1' });
+    // Task must remain in backend map so retry is possible
+    expect(coordinator.getTask('task-1')).toBeDefined();
+    // MCP_TaskClosed must NOT be sent
+    expect(mockNotifyRenderer).not.toHaveBeenCalledWith('mcp_task_closed', expect.anything());
+    // Failure event must be sent with the error message
+    expect(mockNotifyRenderer).toHaveBeenCalledWith('mcp_task_cleanup_failed', {
+      taskId: 'task-1',
+      error: 'delete failed',
+    });
+  });
+
+  it('deleteTask failure preserves controlMap and blockedByHumanControl state', async () => {
+    const { deleteTask: mockDeleteTask } =
+      await vi.importMock<typeof import('../ipc/tasks.js')>('../ipc/tasks.js');
+    vi.mocked(mockDeleteTask).mockRejectedValueOnce(new Error('delete failed'));
+
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    // Simulate the task being under coordinator control
+    coordinator.setTaskControl('task-1', 'coordinator');
+    await coordinator.closeTask('task-1');
+
+    // Backend task still findable — retry via closeTask should work
+    expect(coordinator.getTask('task-1')).toBeDefined();
   });
 
   it('MCP config file deletion failure is swallowed and task is removed', async () => {
@@ -1838,6 +1875,87 @@ describe('Coordinator cleanupTask — failure resilience', () => {
   });
 });
 
+// ─── Docker cleanup sequencing ────────────────────────────────────────────────
+
+describe('Coordinator cleanupTask — Docker inner-process kill sequencing', () => {
+  let coordinator: InstanceType<typeof Coordinator>;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    // Default: execFile calls its callback synchronously (success)
+    mockExecFile.mockImplementation(
+      (
+        _cmd: string,
+        _args: string[],
+        _opts: unknown,
+        cb: (err: Error | null, stdout: string, stderr: string) => void,
+      ) => {
+        cb(null, '', '');
+      },
+    );
+    mockExistsSync.mockReturnValue(false);
+    mockCreateBackendTask.mockResolvedValue({
+      id: 'task-1',
+      branch_name: 'task/test',
+      worktree_path: '/tmp/test',
+    });
+    coordinator = new Coordinator();
+    coordinator.setWindow(mockWin);
+    coordinator.setDefaultProject('proj-1', '/tmp/project');
+    coordinator.registerCoordinator('coord-1', 'proj-1');
+    coordinator.setDockerContainerName('coord-1', 'my-coord-container');
+  });
+
+  it('awaits docker inner-process kill before calling deleteTask', async () => {
+    let resolveDockerKill!: () => void;
+    mockExecFile.mockImplementation(
+      (
+        _cmd: string,
+        _args: string[],
+        _opts: unknown,
+        cb: (err: Error | null, stdout: string, stderr: string) => void,
+      ) => {
+        resolveDockerKill = () => cb(null, '', '');
+      },
+    );
+
+    const { deleteTask: mockDeleteTask } =
+      await vi.importMock<typeof import('../ipc/tasks.js')>('../ipc/tasks.js');
+    vi.mocked(mockDeleteTask).mockResolvedValue(undefined);
+
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    const closePromise = coordinator.closeTask('task-1');
+
+    // Flush microtasks — docker kill is pending, deleteTask must not have been called yet
+    await Promise.resolve();
+    expect(vi.mocked(mockDeleteTask)).not.toHaveBeenCalled();
+
+    // Unblock docker kill — deleteTask should now be called
+    resolveDockerKill();
+    await closePromise;
+    expect(vi.mocked(mockDeleteTask)).toHaveBeenCalled();
+  });
+
+  it('docker kill failure does not prevent deleteTask from being called', async () => {
+    mockExecFile.mockImplementation(
+      (
+        _cmd: string,
+        _args: string[],
+        _opts: unknown,
+        cb: (err: Error | null, stdout: string, stderr: string) => void,
+      ) => {
+        cb(new Error('container not found'), '', '');
+      },
+    );
+
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    await coordinator.closeTask('task-1');
+
+    expect(coordinator.getTask('task-1')).toBeUndefined();
+    expect(mockNotifyRenderer).toHaveBeenCalledWith('mcp_task_closed', { taskId: 'task-1' });
+  });
+});
+
 // ─── Token rotation tests ──────────────────────────────────────────────────────
 
 describe('Coordinator setMCPServerInfo — token rotation', () => {

electron/mcp/coordinator.ts

@@ -1087,25 +1087,27 @@ export class Coordinator {
     // (each sub-task has a unique worktree path) rather than pkill -f claude,
     // which would terminate sibling sub-tasks and the coordinator itself.
     if (task.dockerContainerName) {
-      execFile(
-        'docker',
-        [
-          'exec',
-          task.dockerContainerName,
-          'sh',
-          '-c',
-          'for p in /proc/[0-9]*/cwd; do [ "$(readlink "$p" 2>/dev/null)" = "$1" ] && kill -TERM "$(basename "$(dirname "$p")")" 2>/dev/null; done',
-          '--',
-          task.worktreePath,
-        ],
-        { timeout: 5000 },
-        () => {
-          // Intentionally ignore errors: inner process may have already exited.
-        },
-      );
+      try {
+        await execAsync(
+          'docker',
+          [
+            'exec',
+            task.dockerContainerName,
+            'sh',
+            '-c',
+            'for p in /proc/[0-9]*/cwd; do [ "$(readlink "$p" 2>/dev/null)" = "$1" ] && kill -TERM "$(basename "$(dirname "$p")")" 2>/dev/null; done',
+            '--',
+            task.worktreePath,
+          ],
+          { timeout: 5000 },
+        );
+      } catch {
+        // Inner process may have already exited — failure is expected and harmless.
+      }
     }
 
-    // Remove worktree
+    // Remove worktree. If this fails, keep all coordinator state so the caller
+    // can retry. Do NOT emit MCP_TaskClosed — the task still exists on disk.
     try {
       await deleteTask({
         agentIds: [task.agentId],
@@ -1115,6 +1117,12 @@ export class Coordinator {
       });
     } catch (err) {
       console.warn('Failed to delete coordinated task worktree:', err);
+      this.closingTaskIds.delete(taskId);
+      this.notifyRenderer(IPC.MCP_TaskCleanupFailed, {
+        taskId,
+        error: err instanceof Error ? err.message : String(err),
+      });
+      return;
     }
 
     // Clean up internal state — resolve idle and signal waiters before deleting

src/store/tasks.test.ts

@@ -483,3 +483,47 @@ describe('sendPrompt', () => {
     expect(writePayloads()).toEqual(['\x1b[I', '\x1b[200~line 1\nline 2\x1b[201~', '\r']);
   });
 });
+
+// ─── MCP_TaskCleanupFailed IPC handler ───────────────────────────────────────
+
+const cleanupFailedHandler = ipcHandlers.get(IPC.MCP_TaskCleanupFailed);
+if (!cleanupFailedHandler) throw new Error('mcp_task_cleanup_failed handler not registered');
+
+describe('MCP_TaskCleanupFailed IPC handler', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockSetStore.mockImplementation((...args: unknown[]) => applySetStore(...args));
+    mockTasks = {
+      'task-1': {
+        agentIds: ['agent-1'],
+        shellAgentIds: [],
+        closingStatus: 'closing',
+      },
+    };
+  });
+
+  it('marks the task closingStatus as error', () => {
+    cleanupFailedHandler({ taskId: 'task-1', error: 'git worktree delete failed' });
+
+    expect(mockTasks['task-1'].closingStatus).toBe('error');
+  });
+
+  it('sets closingError to the error message', () => {
+    cleanupFailedHandler({ taskId: 'task-1', error: 'git worktree delete failed' });
+
+    expect(mockTasks['task-1'].closingError).toBe('git worktree delete failed');
+  });
+
+  it('does NOT remove the task from the store', () => {
+    cleanupFailedHandler({ taskId: 'task-1', error: 'delete failed' });
+
+    expect(mockTasks['task-1']).toBeDefined();
+  });
+
+  it('is a no-op if the task does not exist', () => {
+    expect(() =>
+      cleanupFailedHandler({ taskId: 'nonexistent', error: 'delete failed' }),
+    ).not.toThrow();
+    expect(mockTasks['task-1'].closingStatus).toBe('closing');
+  });
+});

src/store/tasks.ts

@@ -1078,6 +1078,16 @@ export function initMCPListeners(): () => void {
     }),
   );
 
+  cleanups.push(
+    window.electron.ipcRenderer.on(IPC.MCP_TaskCleanupFailed, (data: unknown) => {
+      const { taskId, error } = data as { taskId: string; error: string };
+      const task = store.tasks[taskId];
+      if (!task) return;
+      setStore('tasks', taskId, 'closingStatus', 'error');
+      setStore('tasks', taskId, 'closingError', error);
+    }),
+  );
+
   cleanups.push(
     window.electron.ipcRenderer.on(IPC.MCP_CoordinatorNotificationStaged, (data: unknown) => {
       const evt = data as {