Browse Source

fix: abort session when runtimeOverride=false skips fallback

Greptile review on PR #692 (P2) found that the runtimeOverride=false
guard returned early without aborting the session, leaving it in a
silent retry loop rather than surfacing the error as documented. For
the session.status path this created an unbounded cycle: checkRetryBudget
increments the counter, once exhausted clears it and returns true,
tryFallback runs, guard returns early, cycle repeats.

Fix: call abortSessionWithTimeout before returning in the guard block.
Reviewed by @oracle: retry counter is already cleared by checkRetryBudget
before tryFallback runs, so no additional cleanup needed.

Also adds 2 tests (session.error and session.status paths with
runtimeOverride=false) per Greptile's second finding that only
message.updated was covered. Updates the existing out-of-chain test's
abort assertion (0 -> 1) to match the new behavior.

All 45 foreground-fallback tests pass. Full suite: 1389 pass, 1 pre-existing
unrelated failure.
dragon-Elec 1 month ago
parent
commit
d3feff1a73

+ 95 - 2
src/hooks/foreground-fallback/index.test.ts

@@ -1000,9 +1000,9 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       },
       },
     });
     });
 
 
-    // runtimeOverride=false + model not in chain → should NOT fall back
+    // runtimeOverride=false + model not in chain → abort session, no fallback
     expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
     expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalledTimes(0);
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
   });
   });
 
 
   test('always falls back for in-chain model regardless of runtimeOverride=false', async () => {
   test('always falls back for in-chain model regardless of runtimeOverride=false', async () => {
@@ -1104,4 +1104,97 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
     // Model IS in chain (resolved via model matching) → should fall back
     // Model IS in chain (resolved via model matching) → should fall back
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
   });
   });
+
+  test('session.error with runtimeOverride=false and out-of-chain model aborts session', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      undefined,
+      false, // runtimeOverride
+    );
+
+    // Seed session with out-of-chain model
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-err-override',
+          agent: 'orchestrator',
+          providerID: 'custom',
+          modelID: 'expensive-model',
+        },
+      },
+    });
+
+    // Trigger session.error with rate limit
+    await mgr.handleEvent({
+      type: 'session.error',
+      properties: {
+        sessionID: 'sess-err-override',
+        error: { message: 'Rate limit exceeded' },
+      },
+    });
+
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
+  });
+
+  test('session.status with runtimeOverride=false and out-of-chain model aborts after retry budget exhausted', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3, // maxRetries
+      undefined,
+      false, // runtimeOverride
+    );
+
+    // Seed session with out-of-chain model
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-status-override',
+          agent: 'orchestrator',
+          providerID: 'custom',
+          modelID: 'expensive-model',
+        },
+      },
+    });
+
+    // First retry (attempt 1) — absorbed by retry budget
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-status-override',
+        status: { type: 'retry', message: 'rate limit, retrying...' },
+      },
+    });
+    expect(mocks.abort).toHaveBeenCalledTimes(0);
+
+    // Second retry (attempt 2) — absorbed
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-status-override',
+        status: { type: 'retry', message: 'rate limit, retrying...' },
+      },
+    });
+    expect(mocks.abort).toHaveBeenCalledTimes(0);
+
+    // Third retry (attempt 3) — budget exhausted, tryFallback runs, guard aborts
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-status-override',
+        status: { type: 'retry', message: 'rate limit, retrying...' },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
+  });
 });
 });

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

@@ -356,6 +356,9 @@ export class ForegroundFallbackManager {
           currentModel,
           currentModel,
           chain,
           chain,
         });
         });
+        // Abort the session so the rate-limit error surfaces to the user
+        // instead of leaving the session in a silent retry loop.
+        await abortSessionWithTimeout(this.client, sessionID);
         return;
         return;
       }
       }