Browse Source

fix(foreground-fallback): resolve greptile P1 issues - stale retry guard uses attempt number

- Stale retry from old model: attempt > 1 (continuation of old retry loop) → skipped
- Genuine retry from new model: attempt === 1 (first retry for new model) → processed

Replaces time-based inference with OpenCode's actual retry attempt number from session.status event. Fixes both P1 issues:
1. Stale retry advancing new model
2. Ambiguous retry origin

Added regression test for genuine retry from fallback model within dedup window.
Michael Henke 1 month ago
parent
commit
3752d1858b

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

@@ -847,6 +847,85 @@ describe('ForegroundFallbackManager session.status', () => {
     expect(mocks.abort).toHaveBeenCalledTimes(1);
     expect(mocks.abort).toHaveBeenCalledTimes(1);
     expect(mocks.promptAsync).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-genuine-retry',
+        status: {
+          type: 'retry',
+          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',
+    });
+  });
 });
 });
 
 
 // ---------------------------------------------------------------------------
 // ---------------------------------------------------------------------------

+ 17 - 8
src/hooks/foreground-fallback/index.ts

@@ -328,20 +328,29 @@ export class ForegroundFallbackManager {
               isFailoverError({ message: props.status.message })));
               isFailoverError({ message: props.status.message })));
         if (isFailoverRetry) {
         if (isFailoverRetry) {
           // Guard: stale retry event from a previous model's retry loop.
           // Guard: stale retry event from a previous model's retry loop.
-          // When the model changed since the last trigger and we're still
-          // within the dedup window, this is a delayed event from the old
-          // model after a fallback already switched models. Skip it.
+          // 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 prevModel = this.lastTriggerModel.get(sessionID);
           const curModel = this.sessionModel.get(sessionID);
           const curModel = this.sessionModel.get(sessionID);
-          const lastTime = this.lastTrigger.get(sessionID) ?? 0;
-          if (
+          const lastTriggerTime = this.lastTrigger.get(sessionID) ?? 0;
+          const attempt = props.status?.attempt ?? 1;
+          const modelChanged =
             prevModel !== undefined &&
             prevModel !== undefined &&
             curModel !== undefined &&
             curModel !== undefined &&
-            prevModel !== curModel &&
-            Date.now() - lastTime < DEDUP_WINDOW_MS
-          ) {
+            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;
             break;
           }
           }
+          // Otherwise (attempt === 1, or model didn't change, or outside
+          // dedup window): process as genuine retry for current model.
           if (this.shouldTriggerFallback(sessionID)) {
           if (this.shouldTriggerFallback(sessionID)) {
             await this.tryFallbackWithAbort(sessionID);
             await this.tryFallbackWithAbort(sessionID);
           }
           }