Browse Source

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

mygo 1 week ago
parent
commit
cb69386a3d
2 changed files with 207 additions and 3 deletions
  1. 173 0
      src/hooks/foreground-fallback/index.test.ts
  2. 34 3
      src/hooks/foreground-fallback/index.ts

+ 173 - 0
src/hooks/foreground-fallback/index.test.ts

@@ -1315,6 +1315,179 @@ 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',
+          },
+        },
+      });
+
+      // 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 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;
+    }
+  });
 });
 
 // ---------------------------------------------------------------------------

+ 34 - 3
src/hooks/foreground-fallback/index.ts

@@ -233,6 +233,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. */
@@ -291,6 +296,7 @@ export class ForegroundFallbackManager {
         this.lastTrigger.delete(id);
         this.lastTriggerModel.delete(id);
         this.sessionRetries.delete(id);
+        this.chainExhaustion.delete(id);
       });
     }
   }
@@ -334,6 +340,7 @@ export class ForegroundFallbackManager {
         } else {
           // Successful response: clear retry count so recovery is not forgotten.
           this.sessionRetries.delete(sessionID);
+          this.chainExhaustion.delete(sessionID);
         }
         break;
       }
@@ -406,6 +413,7 @@ export class ForegroundFallbackManager {
         if (this.isRecoveredStatus(props.status?.type)) {
           // Recovered/terminal status: clear retry count.
           this.sessionRetries.delete(sessionID);
+          this.chainExhaustion.delete(sessionID);
         }
         // Note: do NOT clear sessionRetries here on non-rate-limit statuses.
         // Abort events triggered by our own fallback carry non-rate-limit
@@ -554,6 +562,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);
@@ -579,11 +591,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,
@@ -597,6 +627,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,