fix(coordinator): cache wait_for_signal_done timeouts via shared complete() path

`waitForSignalDone` had two terminal paths: the registered `wrapped`
resolver (which cleared the timer, wrote the replay cache, and
resolved) and the timeout callback (which removed the resolver,
finished the wait, but \*did not\* write the replay cache before
resolving). A client retry that reused the same requestId after a
timeout would therefore re-enter the wait path and register a fresh
resolver, inflating `activeSignalWaitCounts` and re-blocking the
coordinator's notification batching.

- Collapse both paths into a single idempotent `complete(result)` that
  clears the timer, removes itself from `anySignalResolvers`, calls
  `finishSignalWait`, writes the replay cache when `requestId` is set,
  and resolves the promise. The `settled` guard makes repeated calls a
  no-op so external callers can keep shifting the resolver out before
  invoking it.
- Drop the now-redundant `finishSignalWait` calls in the three external
  paths that consume a resolver (PTY exit, cleanupChildTask, signalDone)
  — `complete` handles bookkeeping itself, so leaving the manual call
  in place would double-decrement the wait count.
- In `unregisterCoordinator`, snapshot the resolvers before fan-out
  because `complete` now splices itself from the live array.
- Add a coordinator.test.ts case: same requestId after a timeout
  returns the cached timed-out result quickly instead of starting a
  new live wait.

Addresses finding 1 of issue #157.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: 2 people

electron/mcp/coordinator.test.ts

@@ -5525,6 +5525,24 @@ describe('Coordinator waitForSignalDone — requestId replay after transport fai
     expect(result.taskId).toBe('task-1');
   });
 
+  it('same requestId returns cached timed-out result after a timeout (no second live wait)', async () => {
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+
+    const requestId = 'replay-timeout-id';
+    // First call times out — no unconsumed signal, short timeout.
+    const result1 = await coordinator.waitForSignalDone('coord-1', 25, requestId);
+    expect(result1.timedOut).toBe(true);
+
+    // Retry with the same requestId must replay the cached timed-out result
+    // instead of registering a fresh waiter — otherwise a stale HTTP retry
+    // would inflate activeSignalWaitCounts and re-block the coordinator.
+    const beforeRetry = Date.now();
+    const result2 = await coordinator.waitForSignalDone('coord-1', 10_000, requestId);
+    const elapsed = Date.now() - beforeRetry;
+    expect(result2).toEqual(result1);
+    expect(elapsed).toBeLessThan(100);
+  });
+
   it('cached result for coord-A does not replay for coord-B with the same requestId', async () => {
     coordinator.registerCoordinator('coord-2', 'proj-1');
 

electron/mcp/coordinator.ts

@@ -229,14 +229,15 @@ export class Coordinator {
             this.suppressPendingNotificationForTask(task);
             task.reviewNotificationQueued = true;
             const remaining = this.countRemaining(coordinatorId);
+            // The resolver IS `complete` from waitForSignalDone — it handles
+            // finishSignalWait, replay-cache write, and timer cleanup itself.
             firstAnyResolver({
               taskId: task.id,
               name: task.name,
               status: 'exited',
               signalDoneAt: new Date().toISOString(),
               remaining,
             });
-            this.finishSignalWait(coordinatorId);
           }
           this.maybeQueueReviewNotification(task, 'exited', exitCode ?? null);
           break;
@@ -1926,14 +1927,14 @@ export class Coordinator {
       this.suppressPendingNotificationForTask(task);
       task.reviewNotificationQueued = true;
       const remaining = this.countRemaining(coordinatorId);
+      // Resolver `complete` from waitForSignalDone handles finishSignalWait.
       firstAnyResolver({
         taskId: task.id,
         name: task.name,
         status: 'exited',
         signalDoneAt: new Date().toISOString(),
         remaining,
       });
-      this.finishSignalWait(coordinatorId);
     }
     this.clearAgentBuffers(task.agentId);
     // Delete per-task MCP config tmp file
@@ -2227,7 +2228,11 @@ export class Coordinator {
         signalDoneAt: new Date().toISOString(),
         remaining: 0,
       };
-      for (const resolve of anyResolvers) resolve(syntheticResult);
+      // Snapshot first — each resolver is `complete` from waitForSignalDone,
+      // which splices itself out of the array, and mutating during iteration
+      // would skip waiters.
+      const snapshot = [...anyResolvers];
+      for (const resolve of snapshot) resolve(syntheticResult);
     }
     this.anySignalResolvers.delete(coordinatorTaskId);
     this.activeSignalWaitCounts.delete(coordinatorTaskId);
@@ -2376,14 +2381,14 @@ export class Coordinator {
       // Suppress before finishSignalWait so it doesn't re-stage
       this.suppressPendingNotificationForTask(task);
       const remaining = this.countRemaining(coordinatorId);
+      // Resolver `complete` from waitForSignalDone handles finishSignalWait.
       firstAnyResolver({
         taskId,
         name: task.name,
         status: task.status,
         signalDoneAt: (task.signalDoneAt ?? new Date()).toISOString(),
         remaining,
       });
-      this.finishSignalWait(coordinatorId);
       // Tell renderer — coordinator already gets result via MCP return value, no UI notification needed
       this.notifyRenderer(IPC.MCP_TaskStateSync, {
         taskId,
@@ -2529,36 +2534,44 @@ export class Coordinator {
 
     return new Promise((resolve) => {
       const timerRef = { value: undefined as ReturnType<typeof setTimeout> | undefined };
-
-      const wrapped = (result: WaitForSignalDoneResult) => {
+      let settled = false;
+
+      // Single termination path: timer cleanup, resolver removal, active-wait
+      // bookkeeping, replay-cache write, and promise resolution all happen here
+      // regardless of whether the result came from a signal, an exit, a
+      // coordinator close, or the timeout. Idempotent — repeated calls are a
+      // no-op so external callers can shift the resolver out before invoking.
+      const complete = (result: WaitForSignalDoneResult) => {
+        if (settled) return;
+        settled = true;
         if (timerRef.value !== undefined) clearTimeout(timerRef.value);
-        if (requestId) this.recentlyDelivered.set(coordinatorTaskId, requestId, result);
-        resolve(result);
-      };
-
-      timerRef.value = setTimeout(() => {
         const resolvers = this.anySignalResolvers.get(coordinatorTaskId);
         if (resolvers) {
-          const idx = resolvers.indexOf(wrapped);
+          const idx = resolvers.indexOf(complete);
           if (idx >= 0) resolvers.splice(idx, 1);
         }
         this.finishSignalWait(coordinatorTaskId);
+        if (requestId) this.recentlyDelivered.set(coordinatorTaskId, requestId, result);
+        resolve(result);
+      };
+
+      timerRef.value = setTimeout(() => {
         logWarn('coordinator.signal_wait', `wait_for_signal_done timed out after ${timeoutMs}ms`, {
           coordinatorTaskId,
           reason: 'timeout',
           timeoutMs,
           activeWaitCount: this.activeSignalWaitCounts.get(coordinatorTaskId) ?? 0,
         });
         const remaining = this.countRemaining(coordinatorTaskId);
-        resolve({ remaining, timedOut: true });
+        complete({ remaining, timedOut: true });
       }, timeoutMs);
 
       let resolvers = this.anySignalResolvers.get(coordinatorTaskId);
       if (!resolvers) {
         resolvers = [];
         this.anySignalResolvers.set(coordinatorTaskId, resolvers);
       }
-      resolvers.push(wrapped);
+      resolvers.push(complete);
     });
   }