Merge pull request #736 from mhenke/fix/session-status-immediate-fallback

fix(foreground-fallback): trigger immediate fallback on first session.status retry

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

@@ -405,7 +405,7 @@ describe('ForegroundFallbackManager message.updated', () => {
 // ---------------------------------------------------------------------------
 
 describe('ForegroundFallbackManager session.status', () => {
-  test('aborts active retry-budget-exhausted session before fallback re-prompt', async () => {
+  test('aborts session before fallback re-prompt on first failover retry', async () => {
     const calls: string[] = [];
     const { client, mocks } = createMockClient({
       abortImpl: async () => {
@@ -429,19 +429,17 @@ describe('ForegroundFallbackManager session.status', () => {
       },
     });
 
-    for (const attempt of [1, 2, 3]) {
-      await mgr.handleEvent({
-        type: 'session.status',
-        properties: {
-          sessionID: 'sess-retry-abort-before-prompt',
-          status: {
-            type: 'retry',
-            attempt,
-            message: 'rate limit, retrying...',
-          },
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-retry-abort-before-prompt',
+        status: {
+          type: 'retry',
+          attempt: 1,
+          message: 'rate limit, retrying...',
         },
-      });
-    }
+      },
+    });
 
     expect(mocks.abort).toHaveBeenCalledTimes(1);
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
@@ -627,7 +625,7 @@ describe('ForegroundFallbackManager session.status', () => {
     expect(mocks.promptAsync).not.toHaveBeenCalled();
   });
 
-  test('absorbs failover retries until the retry budget is exhausted', async () => {
+  test('triggers immediate fallback on first failover retry', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
 
@@ -642,7 +640,6 @@ describe('ForegroundFallbackManager session.status', () => {
       },
     });
 
-    // The first retry is absorbed; only exhaustion triggers a failover.
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
@@ -654,10 +651,10 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
   });
 
-  test('switches models after three failover retries', async () => {
+  test('switches to fallback model on first failover retry', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
 
@@ -672,7 +669,6 @@ describe('ForegroundFallbackManager session.status', () => {
       },
     });
 
-    // First retry is absorbed.
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
@@ -684,31 +680,6 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-
-    // Second retry is also absorbed; the third exhausts the budget.
-    await mgr.handleEvent({
-      type: 'session.status',
-      properties: {
-        sessionID: 'sess-retry2',
-        status: {
-          type: 'retry',
-          attempt: 2,
-          message: 'rate limit, retrying...',
-        },
-      },
-    });
-    await mgr.handleEvent({
-      type: 'session.status',
-      properties: {
-        sessionID: 'sess-retry2',
-        status: {
-          type: 'retry',
-          attempt: 3,
-          message: 'rate limit, retrying...',
-        },
-      },
-    });
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
   });
 
@@ -766,7 +737,7 @@ describe('ForegroundFallbackManager session.status', () => {
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
   });
 
-  test('non-rate-limit status does not clear retries (no infinite loop from abort side effects)', async () => {
+  test('non-rate-limit retry does not trigger fallback but rate-limit does', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
 
@@ -781,7 +752,17 @@ describe('ForegroundFallbackManager session.status', () => {
       },
     });
 
-    // First rate-limit is absorbed.
+    // Non-rate-limit retry (e.g. abort side effect): must NOT trigger fallback.
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-nonrl',
+        status: { type: 'retry', attempt: 1, message: 'aborted' },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+
+    // Genuine rate-limit retry triggers immediate fallback.
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
@@ -793,25 +774,67 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+
+  test('ignores stale retry event from original model after fallback switches models', async () => {
+    // greptile-apps race condition: after a fallback succeeds and the manager
+    // switches to model B, a delayed retry event from model A's original retry
+    // loop (already in-flight when the abort happened) should NOT trigger a
+    // second fallback — it carries the old model's error, not model B's.
+    const calls: string[] = [];
+    const { client, mocks } = createMockClient({
+      abortImpl: async () => {
+        calls.push('abort');
+      },
+      promptAsyncImpl: async () => {
+        calls.push('promptAsync');
+        return {};
+      },
+    });
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+
+    // Seed session with model A (anthropic/claude-opus-4-5)
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-stale',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
 
-    // Non-rate-limit status (e.g. abort side effect): must NOT reset retries.
-    // If it did, the next rate-limit would see tried=0 and trigger immediate
-    // fallback again — the infinite loop.
+    // First retry event: model A rate-limited → triggers fallback to model B
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
-        sessionID: 'sess-nonrl',
-        status: { type: 'retry', attempt: 1, message: 'aborted' },
+        sessionID: 'sess-stale',
+        status: {
+          type: 'retry',
+          attempt: 1,
+          message: 'rate limit, retrying...',
+        },
       },
     });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
 
-    // Second rate-limit remains within the budget.
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const firstCall = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    expect(firstCall[0].body.model).toEqual({
+      providerID: 'openai',
+      modelID: 'gpt-4o',
+    });
+
+    // Stale retry event from the ORIGINAL model A arrives after the switch.
+    // The session model is now openai/gpt-4o, so this event should be ignored.
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
-        sessionID: 'sess-nonrl',
+        sessionID: 'sess-stale',
         status: {
           type: 'retry',
           attempt: 2,
@@ -819,21 +842,89 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
 
-    // Third rate-limit exhausts the budget.
+    // Should NOT trigger another fallback — the event is stale
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+
+  test('does NOT ignore genuine retry from fallback model within dedup window', async () => {
+    // greptile-apps issue #2: a genuine retry from the fallback model (model B)
+    // arriving within the dedup window should trigger a fallback, not be ignored.
+    // The previous fix used lastTriggerModel which still held model A, causing
+    // model B's genuine retry to be mistaken for a stale retry from model A.
+    const calls: string[] = [];
+    const { client, mocks } = createMockClient({
+      abortImpl: async () => {
+        calls.push('abort');
+      },
+      promptAsyncImpl: async () => {
+        calls.push('promptAsync');
+        return {};
+      },
+    });
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 1); // maxRetries=1 for immediate fallback
+
+    // Seed session with model A
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-genuine-retry',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // First retry event: model A rate-limited → triggers fallback to model B
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
-        sessionID: 'sess-nonrl',
+        sessionID: 'sess-genuine-retry',
         status: {
           type: 'retry',
-          attempt: 3,
+          attempt: 1,
           message: 'rate limit, retrying...',
         },
       },
     });
+
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const firstCall = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    expect(firstCall[0].body.model).toEqual({
+      providerID: 'openai',
+      modelID: 'gpt-4o',
+    });
+
+    // Now model B (openai/gpt-4o) is active. A GENUINE retry from model B
+    // arrives within the dedup window (immediately after). This should trigger
+    // another fallback to model C (google/gemini-2.5-pro), NOT be ignored.
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-genuine-retry',
+        status: {
+          type: 'retry',
+          attempt: 1, // attempt resets for new model
+          message: 'rate limit, retrying...',
+        },
+      },
+    });
+
+    // Should trigger a second fallback to model C
+    expect(mocks.abort).toHaveBeenCalledTimes(2);
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+    const secondCall = mocks.promptAsync.mock.calls[1] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    expect(secondCall[0].body.model).toEqual({
+      providerID: 'google',
+      modelID: 'gemini-2.5-pro',
+    });
   });
 });
 

src/hooks/foreground-fallback/index.ts

@@ -4,7 +4,9 @@
  * When OpenCode fires a session.error, message.updated, or session.status
  * event containing a rate-limit signal, this manager:
  *   1. Looks up the next untried model in the agent's configured chain
- *   2. Aborts the rate-limited prompt via client.session.abort()
+ *   2. Aborts the rate-limited prompt via client.session.abort() on the
+ *      session.status retry path; session.error and message.updated paths
+ *      re-prompt directly without abort.
  *   3. Re-queues the last user message via client.session.promptAsync()
  *      with the new model - promptAsync returns immediately so we never
  *      block the event handler waiting for a full LLM response.
@@ -49,6 +51,7 @@ const RATE_LIMIT_PATTERNS = [
 ];
 
 const OUTAGE_STATUS_CODES = new Set([500, 502, 503, 504]);
+// ponytail: validated against real OpenCode error shapes
 const TRANSPORT_CODES = new Set([
   'ECONNREFUSED',
   'ECONNRESET',
@@ -279,7 +282,7 @@ export class ForegroundFallbackManager {
         }
         // Failover-worthy error on an individual message
         if (info.error && isFailoverError(info.error)) {
-          if (this.shouldIntervene(sessionID)) {
+          if (this.shouldTriggerFallback(sessionID)) {
             await this.tryFallback(sessionID);
           }
         } else {
@@ -299,7 +302,7 @@ export class ForegroundFallbackManager {
           sessionID &&
           props.error &&
           isFailoverError(props.error) &&
-          this.shouldIntervene(sessionID)
+          this.shouldTriggerFallback(sessionID)
         ) {
           await this.tryFallback(sessionID);
         }
@@ -324,7 +327,31 @@ export class ForegroundFallbackManager {
             (props.status.message !== undefined &&
               isFailoverError({ message: props.status.message })));
         if (isFailoverRetry) {
-          if (this.checkRetryBudget(sessionID)) {
+          // Guard: stale retry event from a previous model's retry loop.
+          // After a fallback, lastTriggerModel holds the OLD model (set by
+          // isDeduped before the fallback), while sessionModel holds the NEW
+          // model. A stale retry from the old model arrives with attempt > 1
+          // (continuation of old retry loop). A genuine retry from the new
+          // model arrives with attempt === 1 (first retry for new model).
+          const prevModel = this.lastTriggerModel.get(sessionID);
+          const curModel = this.sessionModel.get(sessionID);
+          const lastTriggerTime = this.lastTrigger.get(sessionID) ?? 0;
+          const attempt = props.status?.attempt ?? 1;
+          const modelChanged =
+            prevModel !== undefined &&
+            curModel !== undefined &&
+            prevModel !== curModel;
+          const withinDedupWindow =
+            Date.now() - lastTriggerTime < DEDUP_WINDOW_MS;
+          if (modelChanged && withinDedupWindow && attempt > 1) {
+            // Model changed since last trigger, within dedup window, and
+            // attempt > 1: this is a stale retry from the old model's
+            // retry loop (continuation of previous attempts). Skip it.
+            break;
+          }
+          // Otherwise (attempt === 1, or model didn't change, or outside
+          // dedup window): process as genuine retry for current model.
+          if (this.shouldTriggerFallback(sessionID)) {
             await this.tryFallbackWithAbort(sessionID);
           }
           break;
@@ -375,11 +402,10 @@ export class ForegroundFallbackManager {
   // ---------------------------------------------------------------------------
 
   /** Increment retry counter and return true when the budget is exhausted.
-   *  Used by the session.status retry path — each retry counts toward the
+   *  Used by shouldIntervene when tried > 0 — each retry counts toward the
    *  budget and only triggers fallback after maxRetries - 1 absorptions.
-   *  Non-retry paths (session.error / message.updated) use shouldIntervene(),
-   *  which bypasses the counter on first occurrence. */
-  private checkRetryBudget(sessionID: string): boolean {
+   *  First failover retry (tried === 0) bypasses the counter via shouldIntervene. */
+  private consumeRetryBudget(sessionID: string): boolean {
     const tried = this.sessionRetries.get(sessionID) ?? 0;
     if (tried < this.maxRetries - 1) {
       this.sessionRetries.set(sessionID, tried + 1);
@@ -394,12 +420,12 @@ export class ForegroundFallbackManager {
     return true;
   }
 
-  /** For non-retry paths (session.error, message.updated): intervene immediately
-   *  unless the session is already in a retry window (has prior retries). */
-  private shouldIntervene(sessionID: string): boolean {
+  /** Intervene immediately on first occurrence (tried === 0), otherwise
+   *  delegate to retry budget. Used by all three event paths. */
+  private shouldTriggerFallback(sessionID: string): boolean {
     const tried = this.sessionRetries.get(sessionID) ?? 0;
     if (tried === 0) return true;
-    return this.checkRetryBudget(sessionID);
+    return this.consumeRetryBudget(sessionID);
   }
 
   private isRecoveredStatus(statusType: string | undefined): boolean {