Browse Source

fix(task-session): fix parallel early-reg + idle/FG false-complete race

Two related bugs observed when launching parallel council reviewers:

1. session.created early registration used peekByParent(), which always
   returned the first pending call for the parent. Parallel children
   (reviewer-b/c) were registered as reviewer-a. OpenCode sets
   info.agent on the child SessionInfo to the subagent name — prefer
   that match via peekByParentAndAgent.

2. Child idle was reconciled to completed immediately. OpenCode can emit
   idle for a rate-limited child before ForegroundFallbackManager sets
   isFallbackInProgress; the board then marked completed while FG
   re-prompted and the session kept working ("cancelled but still
   running"). Delay child idle-reconcile, re-check fallback/busy after
   the delay, and cancel the timer on live busy.
Jiajun0413 3 weeks ago
parent
commit
32be2a8e6d
2 changed files with 132 additions and 28 deletions
  1. 60 1
      src/hooks/task-session-manager/index.test.ts
  2. 72 27
      src/hooks/task-session-manager/index.ts

+ 60 - 1
src/hooks/task-session-manager/index.test.ts

@@ -25,6 +25,11 @@ async function flushContinuation(): Promise<void> {
   await new Promise((resolve) => setTimeout(resolve, 0));
 }
 
+/** Flush delayed child idle-reconcile timers when idleReconcileDelayMs is 0. */
+async function flushChildIdleReconcile(): Promise<void> {
+  await new Promise((resolve) => setTimeout(resolve, 5));
+}
+
 function createHook(options?: {
   shouldManageSession?: (sessionID: string) => boolean;
   registerSessionAsOrchestrator?: (sessionID: string) => void;
@@ -2324,11 +2329,13 @@ describe('task-session-manager hook', () => {
     const { hook } = createHook({
       backgroundJobBoard: board,
       shouldManageSession: (id) => id === 'parent-1',
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
 
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',
@@ -2373,11 +2380,13 @@ describe('task-session-manager hook', () => {
       backgroundJobBoard: board,
       shouldManageSession: (id) => id === 'parent-1',
       isFallbackInProgress: (id) => id === 'child-1',
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
 
     // Job should still be running — not reconciled
     expect(board.get('child-1')).toMatchObject({ state: 'running' });
@@ -2448,11 +2457,13 @@ describe('task-session-manager hook', () => {
       shouldManageSession: (id) => id === 'parent-1',
       // isFallbackInProgress returns false for child-1
       isFallbackInProgress: () => false,
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
 
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',
@@ -2477,12 +2488,14 @@ describe('task-session-manager hook', () => {
       backgroundJobBoard: board,
       shouldManageSession: () => false,
       isFallbackInProgress: (id) => id === 'child-1',
+      idleReconcileDelayMs: 0,
     });
 
     // First idle (abort from fallback) — guarded, no reconciliation
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
     expect(board.get('child-1')).toMatchObject({ state: 'running' });
 
     // Busy signal (fallback re-prompt) — updates lastLiveBusyAt
@@ -2499,22 +2512,63 @@ describe('task-session-manager hook', () => {
       backgroundJobBoard: board,
       shouldManageSession: () => false,
       isFallbackInProgress: () => false,
+      idleReconcileDelayMs: 0,
     });
     await hook2.hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',
       terminalState: 'completed',
     });
   });
 
+  test('busy after idle cancels pending child idle-reconcile (FG race)', async () => {
+    // OpenCode can emit idle for a rate-limited child BEFORE FG sets
+    // isFallbackInProgress. Immediate reconcile would mark completed while
+    // FG re-prompts and the child keeps working. Delay + busy cancel keeps
+    // the job running (the observed council-b false-complete race).
+    const board = new BackgroundJobBoard();
+    board.registerLaunch({
+      taskID: 'child-b',
+      parentSessionID: 'parent-1',
+      agent: 'councillor-reviewer-b',
+      description: 'audit distributed',
+    });
+
+    const { hook } = createHook({
+      backgroundJobBoard: board,
+      shouldManageSession: () => false,
+      isFallbackInProgress: () => false,
+      idleReconcileDelayMs: 30,
+    });
+
+    await hook.event({
+      event: { type: 'session.idle', properties: { sessionID: 'child-b' } },
+    });
+    expect(board.get('child-b')).toMatchObject({ state: 'running' });
+
+    await hook.event({
+      event: {
+        type: 'session.status',
+        properties: { sessionID: 'child-b', status: { type: 'busy' } },
+      },
+    });
+
+    await new Promise((r) => setTimeout(r, 50));
+    expect(board.get('child-b')).toMatchObject({ state: 'running' });
+  });
+
   test('session.created early-registers board job so after-hook cancellation cannot orphan the child', async () => {
     // Reproduces #765: parent tool may be cancelled before tool.execute.after,
     // so the job never lands on the board. Early registration from
     // session.created keeps runningJobForSession true and lets idle reconcile.
     const board = new BackgroundJobBoard();
-    const { hook } = createHook({ backgroundJobBoard: board });
+    const { hook } = createHook({
+      backgroundJobBoard: board,
+      idleReconcileDelayMs: 0,
+    });
 
     await hook['tool.execute.before'](
       { tool: 'task', sessionID: 'parent-1', callID: 'call-1' },
@@ -2546,6 +2600,7 @@ describe('task-session-manager hook', () => {
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
 
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',
@@ -2618,11 +2673,13 @@ describe('task-session-manager hook', () => {
     const { hook } = createHook({
       backgroundJobBoard: board,
       shouldManageSession: () => false,
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
       event: { type: 'session.idle', properties: { sessionID: 'child-1' } },
     });
+    await flushChildIdleReconcile();
 
     // Should remain cancelled — idle does not override terminal state
     const job = board.get('child-1');
@@ -2642,6 +2699,7 @@ describe('task-session-manager hook', () => {
     const { hook } = createHook({
       backgroundJobBoard: board,
       shouldManageSession: (id) => id === 'parent-1',
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
@@ -2650,6 +2708,7 @@ describe('task-session-manager hook', () => {
         properties: { sessionID: 'child-1', status: { type: 'idle' } },
       },
     });
+    await flushChildIdleReconcile();
 
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',

+ 72 - 27
src/hooks/task-session-manager/index.ts

@@ -155,6 +155,10 @@ export function createTaskSessionManagerHook(
   const processedInjectedCompletionOrder: string[] = [];
   const terminalJobsInjectedByParent = new Map<string, Set<string>>();
   const idleReconcileTimers = new Map<string, ReturnType<typeof setTimeout>>();
+  const childIdleReconcileTimers = new Map<
+    string,
+    ReturnType<typeof setTimeout>
+  >();
   const continuationSessionTokens = new Map<string, symbol>();
   const activeContinuationEvaluations = new Map<string, Set<symbol>>();
   const continuationConsumed = new Set<string>();
@@ -441,6 +445,58 @@ export function createTaskSessionManagerHook(
     idleReconcileTimers.set(parentSessionID, timer);
   }
 
+  /**
+   * Delay child idle→completed reconciliation. Immediate reconcile races
+   * ForegroundFallbackManager: OpenCode can emit idle for a rate-limited
+   * child before FG sets isFallbackInProgress, marking the job completed
+   * while FG re-prompts and the session keeps working (false cancel/complete).
+   * Re-check running state, fallback-in-progress, and lastLiveBusyAt after
+   * the delay so a post-idle busy from the fallback re-prompt wins.
+   */
+  function scheduleChildIdleReconciliation(
+    sessionID: string,
+    idleObservedAt: number,
+  ): void {
+    if (childIdleReconcileTimers.has(sessionID)) return;
+    if (options.isFallbackInProgress?.(sessionID)) return;
+
+    const timer = setTimeout(() => {
+      childIdleReconcileTimers.delete(sessionID);
+      if (options.isFallbackInProgress?.(sessionID)) return;
+
+      const job = backgroundJobBoard.get(sessionID);
+      if (!job || job.state !== 'running') return;
+
+      // Busy after the idle means the session recovered (e.g. FG re-prompt).
+      if (
+        job.lastLiveBusyAt !== undefined &&
+        job.lastLiveBusyAt > idleObservedAt
+      ) {
+        return;
+      }
+
+      log('[task-session-manager] reconciled running job from idle', {
+        sessionID,
+        alias: job.alias,
+        parentSessionID: job.parentSessionID,
+      });
+      backgroundJobBoard.updateStatus({
+        taskID: sessionID,
+        state: 'completed',
+        resultSummary:
+          'Background task completed (reconciled from idle event)',
+      });
+      backgroundJobBoard.markReconciled(sessionID);
+      taskContextTracker.pendingManagedTaskIds.delete(sessionID);
+      backgroundJobBoard.addContext(
+        sessionID,
+        taskContextTracker.contextFilesForPrompt(sessionID),
+      );
+      taskContextTracker.prune(backgroundJobBoard);
+    }, idleReconcileDelayMs).unref?.();
+    childIdleReconcileTimers.set(sessionID, timer);
+  }
+
   if (options.coordinator) {
     options.coordinator.onSessionDeleted((sessionId) => {
       clearContinuation(sessionId);
@@ -1070,6 +1126,10 @@ export function createTaskSessionManagerHook(
       }
 
       if (input.event.type === 'server.instance.disposed') {
+        for (const timer of childIdleReconcileTimers.values()) {
+          clearTimeout(timer);
+        }
+        childIdleReconcileTimers.clear();
         const continuationSessionIDs = new Set([
           ...idleReconcileTimers.keys(),
           ...continuationSessionTokens.keys(),
@@ -1110,33 +1170,9 @@ export function createTaskSessionManagerHook(
         // Fallback: for background child sessions that go idle without
         // an injected completion, reconcile the board entry since the
         // session being idle is itself the completion signal.
-        // Guard: skip when a foreground-fallback abort/re-prompt is in
-        // flight for this session — the idle is transient, not a real
-        // completion.
-        if (
-          job &&
-          sessionId &&
-          job.state === 'running' &&
-          !options.isFallbackInProgress?.(sessionId)
-        ) {
-          log('[task-session-manager] reconciled running job from idle', {
-            sessionID: sessionId,
-            alias: job.alias,
-            parentSessionID: job.parentSessionID,
-          });
-          backgroundJobBoard.updateStatus({
-            taskID: sessionId,
-            state: 'completed',
-            resultSummary:
-              'Background task completed (reconciled from idle event)',
-          });
-          backgroundJobBoard.markReconciled(sessionId);
-          taskContextTracker.pendingManagedTaskIds.delete(sessionId);
-          backgroundJobBoard.addContext(
-            sessionId,
-            taskContextTracker.contextFilesForPrompt(sessionId),
-          );
-          taskContextTracker.prune(backgroundJobBoard);
+        // Delayed so FG can claim the session before we mark completed.
+        if (job && sessionId && job.state === 'running') {
+          scheduleChildIdleReconciliation(sessionId, Date.now());
         }
         return;
       }
@@ -1186,6 +1222,15 @@ export function createTaskSessionManagerHook(
         if (statusType !== 'busy') {
           return;
         }
+        // Live busy cancels a pending child idle-reconcile — the session
+        // recovered (FG re-prompt or continued work).
+        if (sessionId) {
+          const pendingChildIdle = childIdleReconcileTimers.get(sessionId);
+          if (pendingChildIdle) {
+            clearTimeout(pendingChildIdle);
+            childIdleReconcileTimers.delete(sessionId);
+          }
+        }
         const before = sessionId
           ? backgroundJobBoard.get(sessionId)
           : undefined;