Merge pull request #977 from MyGO-Mujica/fix/foreground-fallback-chain-exhaustion

fix(foreground-fallback): bound chain-exhaustion re-fallback loop

src/hooks/foreground-fallback/index.test.ts

@@ -1416,6 +1416,243 @@ describe('ForegroundFallbackManager chain exhaustion', () => {
     expect(mocks2.abort).toHaveBeenCalledTimes(1);
     expect(mocks2.promptAsync).not.toHaveBeenCalled();
   });
+
+  test('aborts after one re-fallback instead of looping when the whole chain keeps failing', async () => {
+    // Regression for issue #966: two-model chain [gpt-b, gpt-c], both dead.
+    // The reporter's log showed "from glm to glm" every ~10s: the reset path
+    // re-prompted the sticky model forever. It must be allowed once (sticky
+    // gets one retry), then abort and stop intervening. Failures are spaced
+    // beyond the dedup window (as in the real 10s-interval report).
+    const { mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      { orchestrator: ['openai/gpt-b', 'openai/gpt-c'] },
+      true,
+      { directory: '/test' } as any,
+    );
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-loop',
+          providerID: 'openai',
+          modelID: 'gpt-b',
+          role: 'assistant',
+        },
+      },
+    });
+
+    const realNowFn = Date.now;
+    let fakeNow = realNowFn();
+    Date.now = () => fakeNow;
+    try {
+      const fail = async () => {
+        fakeNow += 6_000; // skip the 5s dedup window
+        await mgr.handleEvent({
+          type: 'session.error',
+          properties: {
+            sessionID: 'sess-loop',
+            error: { message: 'Rate limit exceeded' },
+          },
+        });
+      };
+
+      // Fail 1: gpt-b → gpt-c.
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+      expect(mocks.abort).toHaveBeenCalledTimes(0);
+
+      // Fail 2: gpt-c fails → first chain exhaustion → reset, re-prompt gpt-c once.
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+      expect(mocks.abort).toHaveBeenCalledTimes(0);
+
+      // Fail 3: gpt-c fails again → second exhaustion → abort, no re-prompt.
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+      expect(mocks.abort).toHaveBeenCalledTimes(1);
+
+      // Fail 4/5: exhaustion state is terminal → no further intervention.
+      await fail();
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+      expect(mocks.abort).toHaveBeenCalledTimes(1);
+    } finally {
+      Date.now = realNowFn;
+    }
+  });
+
+  test('clears exhaustion state on a successful response (sticky fallback recovered)', async () => {
+    const { mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(makeChains(), true, {
+      directory: '/test',
+    } as any);
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-recover',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+          role: 'assistant',
+        },
+      },
+    });
+
+    const realNowFn = Date.now;
+    let fakeNow = realNowFn();
+    Date.now = () => fakeNow;
+    try {
+      const fail = async () => {
+        fakeNow += 6_000;
+        await mgr.handleEvent({
+          type: 'session.error',
+          properties: {
+            sessionID: 'sess-recover',
+            error: { message: 'Rate limit exceeded' },
+          },
+        });
+      };
+
+      // Walk the chain to the first exhaustion reset (stage 1).
+      await fail();
+      await fail();
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(3);
+
+      // Successful response clears the exhaustion stage.
+      await mgr.handleEvent({
+        type: 'message.updated',
+        properties: {
+          info: {
+            sessionID: 'sess-recover',
+            providerID: 'google',
+            modelID: 'gemini-2.5-pro',
+            role: 'assistant',
+            time: { created: 1, completed: 2 },
+          },
+        },
+      });
+
+      // Next failure gets a fresh reset chance instead of aborting immediately.
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(4);
+      expect(mocks.abort).toHaveBeenCalledTimes(0);
+    } finally {
+      Date.now = realNowFn;
+    }
+  });
+
+  test('does not recover from an incomplete assistant message', async () => {
+    const { mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      { orchestrator: ['openai/gpt-b', 'openai/gpt-c'] },
+      true,
+      { directory: '/test' } as any,
+    );
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-incomplete-recovery',
+          providerID: 'openai',
+          modelID: 'gpt-b',
+          role: 'assistant',
+        },
+      },
+    });
+
+    const realNowFn = Date.now;
+    let fakeNow = realNowFn();
+    Date.now = () => fakeNow;
+    try {
+      const fail = async () => {
+        fakeNow += 6_000;
+        await mgr.handleEvent({
+          type: 'session.error',
+          properties: {
+            sessionID: 'sess-incomplete-recovery',
+            error: { message: 'Rate limit exceeded' },
+          },
+        });
+      };
+
+      // Reach stage 1: gpt-b → gpt-c, then the sticky gpt-c retry.
+      await fail();
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+
+      // A streaming assistant update is not proof of recovery.
+      await mgr.handleEvent({
+        type: 'message.updated',
+        properties: {
+          info: {
+            sessionID: 'sess-incomplete-recovery',
+            providerID: 'openai',
+            modelID: 'gpt-c',
+            role: 'assistant',
+            time: { created: 1 },
+          },
+        },
+      });
+
+      // Stage 1 remains terminal on the next exhaustion: abort, no third prompt.
+      await fail();
+      expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+      expect(mocks.abort).toHaveBeenCalledTimes(1);
+    } finally {
+      Date.now = realNowFn;
+    }
+  });
+
+  test('does not abort repeatedly for single-model chains after exhaustion', async () => {
+    const { mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      { orchestrator: ['openai/gpt-b'] },
+      true,
+      { directory: '/test' } as any,
+    );
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-solo',
+          providerID: 'openai',
+          modelID: 'gpt-b',
+        },
+      },
+    });
+
+    const realNowFn = Date.now;
+    let fakeNow = realNowFn();
+    Date.now = () => fakeNow;
+    try {
+      const fail = async () => {
+        fakeNow += 6_000;
+        await mgr.handleEvent({
+          type: 'session.error',
+          properties: {
+            sessionID: 'sess-solo',
+            error: { message: 'rate limit exceeded' },
+          },
+        });
+      };
+
+      await fail();
+      expect(mocks.abort).toHaveBeenCalledTimes(1);
+      expect(mocks.promptAsync).not.toHaveBeenCalled();
+
+      // Second error must not abort again (no abort loop).
+      await fail();
+      expect(mocks.abort).toHaveBeenCalledTimes(1);
+      expect(mocks.promptAsync).not.toHaveBeenCalled();
+    } finally {
+      Date.now = realNowFn;
+    }
+  });
 });
 
 // ---------------------------------------------------------------------------

src/hooks/foreground-fallback/index.ts

@@ -234,6 +234,11 @@ export class ForegroundFallbackManager {
   /** sessionID → consecutive 429 count for the current model.
    *  Reset on model swap or session deletion. */
   private readonly sessionRetries = new Map<string, number>();
+  /** sessionID → chain-exhaustion stage:
+   *   0 = not exhausted; 1 = chain exhausted once, reset to sticky fallback
+   *   (one retry chance); 2 = exhausted again, aborted — stop intervening.
+   *   Reset to 0 on successful responses or session deletion. */
+  private readonly chainExhaustion = new Map<string, number>();
 
   /** Exposed for task-session-manager: prevents idle reconciliation
    *  while a fallback abort/re-prompt is in flight for this session. */
@@ -292,6 +297,7 @@ export class ForegroundFallbackManager {
         this.lastTrigger.delete(id);
         this.lastTriggerModel.delete(id);
         this.sessionRetries.delete(id);
+        this.chainExhaustion.delete(id);
       });
     }
   }
@@ -327,14 +333,23 @@ export class ForegroundFallbackManager {
             `${info.providerID}/${info.modelID}`,
           );
         }
+        const messageTime = info.time;
+        const isCompletedSuccessfulAssistant =
+          info.role === 'assistant' &&
+          !info.error &&
+          typeof messageTime === 'object' &&
+          messageTime !== null &&
+          'completed' in messageTime &&
+          typeof messageTime.completed === 'number';
         // Failover-worthy error on an individual message
         if (info.error && isFailoverError(info.error)) {
           if (this.shouldTriggerFallback(sessionID)) {
             await this.tryFallback(sessionID);
           }
-        } else {
-          // Successful response: clear retry count so recovery is not forgotten.
+        } else if (isCompletedSuccessfulAssistant) {
+          // Only a completed, successful assistant response proves recovery.
           this.sessionRetries.delete(sessionID);
+          this.chainExhaustion.delete(sessionID);
         }
         break;
       }
@@ -404,17 +419,13 @@ export class ForegroundFallbackManager {
           break;
         }
 
-        if (this.isRecoveredStatus(props.status?.type)) {
-          // Recovered/terminal status: clear retry count.
-          this.sessionRetries.delete(sessionID);
-        }
         // Note: do NOT clear sessionRetries here on non-rate-limit statuses.
         // Abort events triggered by our own fallback carry non-rate-limit
         // messages and would reset the counter, creating an infinite loop:
         // abort → fallback → set retries to 1 → abort event clears retries
         // → next retry sees tried=0 → abort+fallback again → repeat.
-        // Retries are only cleared on successful response (message.updated
-        // without error) or session deletion.
+        // Retries are only cleared on a completed successful assistant
+        // response or session deletion.
         break;
       }
 
@@ -475,16 +486,6 @@ export class ForegroundFallbackManager {
     return this.consumeRetryBudget(sessionID);
   }
 
-  private isRecoveredStatus(statusType: string | undefined): boolean {
-    return (
-      statusType === 'idle' ||
-      statusType === 'complete' ||
-      statusType === 'completed' ||
-      statusType === 'success' ||
-      statusType === 'terminal'
-    );
-  }
-
   // ---------------------------------------------------------------------------
   // Core fallback logic
   // ---------------------------------------------------------------------------
@@ -555,6 +556,10 @@ export class ForegroundFallbackManager {
 
   private async execFallback(sessionID: string): Promise<void> {
     try {
+      // After the chain has been exhausted twice (reset retry failed and we
+      // aborted), do not intervene again for this session: re-entering would
+      // keep aborting in a loop. Surface errors to the user instead.
+      if (this.chainExhaustion.get(sessionID) === 2) return;
       let currentModel = this.sessionModel.get(sessionID);
       const agentName = this.sessionAgent.get(sessionID);
       const chain = this.resolveChain(agentName, currentModel);
@@ -580,11 +585,29 @@ export class ForegroundFallbackManager {
       let nextModel = chain.find((m) => !tried.has(m));
       if (!nextModel) {
         if (chain.length > 1) {
-          // Chain exhausted but we have fallbacks: reset tried set and
-          // stick to the deepest fallback model so we stop re-trying the
-          // dead primary model on every subsequent message.
+          // Chain exhausted but we have fallbacks: on the first exhaustion
+          // reset the tried set and stick to the deepest fallback model so
+          // we stop re-trying the dead primary model on every subsequent
+          // message. If the sticky fallback itself fails afterwards (second
+          // exhaustion), abort once and stop intervening — otherwise the
+          // reset re-prompt would loop forever on a fully dead chain.
           const primary = chain[0];
           const stickyFallback = chain[chain.length - 1];
+          if ((this.chainExhaustion.get(sessionID) ?? 0) >= 1) {
+            this.chainExhaustion.set(sessionID, 2);
+            log(
+              '[foreground-fallback] chain exhausted after re-fallback, aborting',
+              {
+                sessionID,
+                agentName,
+                currentModel,
+                tried: [...tried],
+              },
+            );
+            await abortSessionWithTimeout(getClient(this.input), sessionID);
+            return;
+          }
+          this.chainExhaustion.set(sessionID, 1);
           log('[foreground-fallback] resetting tried set for re-fallback', {
             sessionID,
             agentName,
@@ -598,6 +621,7 @@ export class ForegroundFallbackManager {
           this.sessionTried.set(sessionID, tried);
           nextModel = stickyFallback;
         } else {
+          this.chainExhaustion.set(sessionID, 2);
           log('[foreground-fallback] fallback chain exhausted, aborting', {
             sessionID,
             agentName,