Browse Source

Merge pull request #837 from Jiajun0413/fix/parallel-task-early-registration

fix(task-session): parallel early-reg mis-attribution + idle/FG false-complete race
Alvin 2 weeks ago
parent
commit
ed12e8f52b

+ 4 - 0
src/cache-safety-tripwire.test.ts

@@ -62,6 +62,10 @@ const ALLOWLIST = new Map<string, string>([
     'hooks/task-session-manager/task-context-tracker.ts',
     'Date.now() records lastReadAt for internal recency ordering; formatted prompt output (background job board) is confined to the volatile trailing message.',
   ],
+  [
+    'hooks/task-session-manager/index.ts',
+    'Date.now() captures idleObservedAt to detect post-idle busy recovery from foreground-fallback re-prompts; never serialized into prompt content.',
+  ],
   [
     'hooks/image-hook.ts',
     'Date.now() throttles temp-image cleanup; extracted image paths are deterministic per part id.',

+ 153 - 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,105 @@ 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.deleted cancels pending child idle-reconcile (FG teardown race)', async () => {
+    // FG aborts the child session mid-idle-delay; onSessionDeleted must
+    // cancel the pending timer so it cannot fire after FG finishes and
+    // re-check isFallbackInProgress=false, falsely reconciling the board
+    // entry while the re-prompted session keeps working.
+    const coordinator = new SessionLifecycle(() => {});
+    const board = new BackgroundJobBoard();
+    board.registerLaunch({
+      taskID: 'child-b',
+      parentSessionID: 'parent-1',
+      agent: 'councillor-reviewer-b',
+      description: 'audit distributed',
+    });
+
+    let fgInProgress = false;
+    const { hook } = createHook({
+      backgroundJobBoard: board,
+      coordinator,
+      shouldManageSession: () => false,
+      isFallbackInProgress: () => fgInProgress,
+      idleReconcileDelayMs: 30,
+    });
+
+    // idle fires before FG sets isFallbackInProgress — schedules timer T.
+    await hook.event({
+      event: { type: 'session.idle', properties: { sessionID: 'child-b' } },
+    });
+    // FG claims the session and aborts it; OpenCode emits session.deleted
+    // while the timer is still pending. onSessionDeleted must cancel T.
+    fgInProgress = true;
+    coordinator.dispatchSessionDeleted('child-b');
+    // FG finishes; isFallbackInProgress goes false before T would fire.
+    fgInProgress = false;
+
+    await new Promise((r) => setTimeout(r, 60));
+    // Board entry survives (isFallbackInProgress was true at delete time)
+    // but is NOT reconciled — the timer was cancelled on session.deleted.
+    const job = board.get('child-b');
+    expect(job).toBeDefined();
+    expect(job?.state).toBe('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 +2642,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',
@@ -2553,6 +2650,57 @@ describe('task-session-manager hook', () => {
     });
   });
 
+  test('session.created early registration attributes each parallel child to its own pending call', async () => {
+    // Regression: when a parent launches several task tools in parallel with
+    // different subagent types (e.g. council reviewers a/b/c), the old
+    // peekByParent() returned the FIRST pending call for every child, so
+    // all children were registered with the first subagent's agentType.
+    // info.agent on the child session disambiguates which pending call
+    // started it.
+    const board = new BackgroundJobBoard();
+    const { hook } = createHook({ backgroundJobBoard: board });
+
+    // Parent fires three task tools in parallel: oracle / explorer / fixer.
+    await hook['tool.execute.before'](
+      { tool: 'task', sessionID: 'parent-1', callID: 'call-a' },
+      { args: { subagent_type: 'oracle', description: 'audit loss' } },
+    );
+    await hook['tool.execute.before'](
+      { tool: 'task', sessionID: 'parent-1', callID: 'call-b' },
+      { args: { subagent_type: 'explorer', description: 'audit data' } },
+    );
+    await hook['tool.execute.before'](
+      { tool: 'task', sessionID: 'parent-1', callID: 'call-c' },
+      { args: { subagent_type: 'fixer', description: 'audit fix' } },
+    );
+
+    // Each child session is created while the parent tool calls are still
+    // in flight (before any tool.execute.after). info.agent identifies the
+    // subagent that owns each child.
+    await hook.event({
+      event: {
+        type: 'session.created',
+        properties: { info: { id: 'child-a', parentID: 'parent-1', agent: 'oracle' } },
+      },
+    });
+    await hook.event({
+      event: {
+        type: 'session.created',
+        properties: { info: { id: 'child-b', parentID: 'parent-1', agent: 'explorer' } },
+      },
+    });
+    await hook.event({
+      event: {
+        type: 'session.created',
+        properties: { info: { id: 'child-c', parentID: 'parent-1', agent: 'fixer' } },
+      },
+    });
+
+    expect(board.get('child-a')).toMatchObject({ agent: 'oracle', description: 'audit loss' });
+    expect(board.get('child-b')).toMatchObject({ agent: 'explorer', description: 'audit data' });
+    expect(board.get('child-c')).toMatchObject({ agent: 'fixer', description: 'audit fix' });
+  });
+
   test('cancelled job is not reconciled from idle', async () => {
     const board = new BackgroundJobBoard();
     board.registerLaunch({
@@ -2567,11 +2715,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');
@@ -2591,6 +2741,7 @@ describe('task-session-manager hook', () => {
     const { hook } = createHook({
       backgroundJobBoard: board,
       shouldManageSession: (id) => id === 'parent-1',
+      idleReconcileDelayMs: 0,
     });
 
     await hook.event({
@@ -2599,6 +2750,7 @@ describe('task-session-manager hook', () => {
         properties: { sessionID: 'child-1', status: { type: 'idle' } },
       },
     });
+    await flushChildIdleReconcile();
 
     expect(board.get('child-1')).toMatchObject({
       state: 'reconciled',

+ 87 - 29
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>();
@@ -444,10 +448,67 @@ 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);
       clearInputWaits(sessionId);
+      const pendingChildIdle = childIdleReconcileTimers.get(sessionId);
+      if (pendingChildIdle) {
+        clearTimeout(pendingChildIdle);
+        childIdleReconcileTimers.delete(sessionId);
+      }
       // During a foreground fallback abort/re-prompt cycle, the session
       // is being torn down and immediately recreated with a fallback model.
       // Dropping the job from the board here would make the orchestrator
@@ -1005,7 +1066,7 @@ export function createTaskSessionManagerHook(
       event: {
         type: string;
         properties?: {
-          info?: { id?: string; parentID?: string };
+          info?: { id?: string; parentID?: string; agent?: string };
           id?: string;
           requestID?: string;
           sessionID?: string;
@@ -1037,7 +1098,15 @@ export function createTaskSessionManagerHook(
           // reports runningJobForSession:false and the orchestrator sees
           // "Task cancelled" while the child is still working (#765).
           // Peek (don't take) so tool.execute.after can still re-register.
-          const pending = pendingCallTracker.peekByParent(info.parentID);
+          //
+          // When the parent has multiple task calls in flight at once (e.g.
+          // parallel council reviewers), `info.agent` on the child session
+          // identifies which subagent started it; prefer the matching
+          // pending call so we don't attribute the child to the wrong agent.
+          const pending = pendingCallTracker.peekByParentAndAgent(
+            info.parentID,
+            info.agent,
+          );
           if (
             pending &&
             !pending.resumedTaskId &&
@@ -1065,6 +1134,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(),
@@ -1105,33 +1178,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;
       }
@@ -1181,6 +1230,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;

+ 24 - 0
src/hooks/task-session-manager/pending-call-tracker.ts

@@ -47,6 +47,30 @@ export function createPendingCallTracker() {
       return undefined;
     },
 
+    /**
+     * Peek a pending call for a parent, preferring one whose agentType
+     * matches `agentHint`. Used by session.created early registration:
+     * when a parent launches several parallel task tools with different
+     * subagent types (e.g. council reviewers), `info.agent` on the
+     * child session identifies which subagent started it, so we can
+     * avoid attributing the child to the wrong pending call.
+     * Falls back to the oldest pending call for the parent when no
+     * agent match is found (preserves prior behavior).
+     */
+    peekByParentAndAgent(
+      parentSessionId: string,
+      agentHint?: string,
+    ) {
+      if (!agentHint) return this.peekByParent(parentSessionId);
+      let fallback: PendingTaskCall | undefined;
+      for (const call of pendingCalls.values()) {
+        if (call.parentSessionId !== parentSessionId) continue;
+        if (!fallback) fallback = call;
+        if (call.agentType === agentHint) return call;
+      }
+      return fallback;
+    },
+
     clearSession(sessionId: string) {
       for (const [callId, pending] of pendingCalls.entries()) {
         if (pending.parentSessionId === sessionId) {