Browse Source

Merge pull request #657 from mhenke/fix/foreground-fallback-diagnostic

fix(foreground-fallback): make dedup model-aware so cascade continues
Alvin 1 month ago
parent
commit
ad2762b6db
2 changed files with 82 additions and 3 deletions
  1. 57 0
      src/hooks/foreground-fallback/index.test.ts
  2. 25 3
      src/hooks/foreground-fallback/index.ts

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

@@ -573,6 +573,63 @@ describe('ForegroundFallbackManager deduplication', () => {
 
     expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
   });
+
+  test('cascade continues when second error arrives within dedup window after model switch', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+
+    // Seed session: current model is first entry in orchestrator chain
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-cascade',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // First error — model A fails, falls back to model B (openai/gpt-4o)
+    await mgr.handleEvent({
+      type: 'session.error',
+      properties: {
+        sessionID: 'sess-cascade',
+        error: { message: 'Rate limit exceeded' },
+      },
+    });
+
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    expect(mocks.promptAsync.mock.calls[0][0]).toEqual(
+      expect.objectContaining({
+        body: expect.objectContaining({
+          model: { providerID: 'openai', modelID: 'gpt-4o' },
+        }),
+      }),
+    );
+
+    // Second error — model B also fails within the 5s dedup window.
+    // This is a DIFFERENT incident (new model), so dedup is bypassed
+    // because the current model differs from lastTriggerModel.
+    await mgr.handleEvent({
+      type: 'session.error',
+      properties: {
+        sessionID: 'sess-cascade',
+        error: { message: 'Monthly usage limit reached' },
+      },
+    });
+
+    // Should trigger a second fallback despite being within the original
+    // 5-second dedup window, because the model changed (modelChanged bypass).
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+    expect(mocks.promptAsync.mock.calls[1][0]).toEqual(
+      expect.objectContaining({
+        body: expect.objectContaining({
+          model: { providerID: 'google', modelID: 'gemini-2.5-pro' },
+        }),
+      }),
+    );
+  });
 });
 
 // ---------------------------------------------------------------------------

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

@@ -95,6 +95,10 @@ export class ForegroundFallbackManager {
   private readonly inProgress = new Set<string>();
   /** sessionID → timestamp of last trigger (for deduplication) */
   private readonly lastTrigger = new Map<string, number>();
+  /** sessionID → model in use when lastTrigger was set; dedup is bypassed
+   *  when the model has changed, allowing the cascade to continue when a
+   *  new fallback model also fails within the dedup window. */
+  private readonly lastTriggerModel = new Map<string, string>();
 
   constructor(
     private readonly client: OpencodeClient,
@@ -162,8 +166,12 @@ export class ForegroundFallbackManager {
               status?: { type?: string; message?: string };
             }
           | undefined;
-        if (!props?.sessionID || props.status?.type !== 'retry') break;
-        const msg = props.status.message?.toLowerCase() ?? '';
+        if (!props?.sessionID || !props.status?.message) break;
+        const msg = props.status.message.toLowerCase();
+        // Check for rate-limit signals in the status message regardless of
+        // status type. OpenCode proxies may emit monthly/weekly/5-hour usage
+        // limit errors with type 'error' instead of 'retry' on fresh sessions
+        // where no retry is attempted — the retry-type guard would miss them.
         if (
           msg.includes('rate limit') ||
           msg.includes('usage limit') ||
@@ -208,6 +216,7 @@ export class ForegroundFallbackManager {
           this.sessionTried.delete(id);
           this.inProgress.delete(id);
           this.lastTrigger.delete(id);
+          this.lastTriggerModel.delete(id);
         }
         break;
       }
@@ -223,9 +232,22 @@ export class ForegroundFallbackManager {
     if (this.inProgress.has(sessionID)) return;
 
     // Deduplicate: multiple events can fire for a single rate-limit event.
+    // Bypass dedup when the model changed since the last trigger — the new
+    // model's failure is a separate incident and the cascade should continue.
     const now = Date.now();
-    if (now - (this.lastTrigger.get(sessionID) ?? 0) < DEDUP_WINDOW_MS) return;
+    const curModel = this.sessionModel.get(sessionID);
+    const modelChanged =
+      this.lastTriggerModel.has(sessionID) &&
+      this.lastTriggerModel.get(sessionID) !== curModel;
+    if (
+      !modelChanged &&
+      now - (this.lastTrigger.get(sessionID) ?? 0) < DEDUP_WINDOW_MS
+    )
+      return;
     this.lastTrigger.set(sessionID, now);
+    if (curModel !== undefined) {
+      this.lastTriggerModel.set(sessionID, curModel);
+    }
 
     this.inProgress.add(sessionID);
     try {