Browse Source

fix(fallback): resolve three deadlock points in foreground fallback + task-session-manager

Three independent deadlock points break fallback when the primary model
hits its usage limit (e.g. ChatGPT OAuth 'The usage limit has been reached'):

A. Chain-exhausted permanent deadlock (foreground-fallback):
   tryFallback returns without resetting the tried set when all models
   have been attempted. Since sessionTried is only cleared on
   session.deleted, the session is permanently stuck retrying the dead
   primary model. Fix: when chain.length > 1, reset tried and stick to
   the deepest fallback model instead of deadlocking.

B. Unknown agents excluded from fallback (foreground-fallback):
   resolveChain returns [] for any agent without a configured chain.
   This is correct for known omos agents (preserves cross-agent
   isolation from PR #199), but OpenCode built-in agents like
   'compaction' and 'title' are NOT omos agents and never have chains —
   they are silently excluded from fallback entirely. Fix: fall through
   to model-matching for non-omos agents so they inherit a chain from a
   configured agent that shares their model.

C. session.error erases terminal job state (task-session-manager):
   Both ForegroundFallbackManager and task-session-manager listen for
   session.error. FFM recovers via abort+reprompt, but TSM unconditionally
   deletes terminalJobsInjectedByParent — losing track of completed
   background tasks so the orchestrator cannot dispatch follow-ups.
   Fix: only clear terminal job state for non-rate-limit errors.

Closes #592
Partially addresses #560
Moss 1 month ago
parent
commit
ca14c113fb

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

@@ -698,4 +698,39 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     expect(call[0].body.model.providerID).toBe('openai');
     expect(call[0].body.model.modelID).toBe('gpt-4o');
   });
+
+  test('falls through to model matching for non-omos agents (e.g. compaction)', async () => {
+    // compaction is an OpenCode built-in agent that is NOT an omos agent,
+    // so it has no chain configured. It should fall through to model
+    // matching and inherit a chain from a configured agent that shares
+    // its model, instead of being silently excluded from fallback.
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      { orchestrator: ['openai/gpt-5.4', 'new-api/glm-5.2'] },
+      true,
+    );
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'compaction-sess',
+          agent: 'compaction', // NOT a known omos built-in agent
+          providerID: 'openai',
+          modelID: 'gpt-5.4',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // compaction's model (openai/gpt-5.4) matches orchestrator's chain
+    // → should fall back to the next untried model in that chain
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const call = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    expect(call[0].body.model.providerID).toBe('new-api');
+    expect(call[0].body.model.modelID).toBe('glm-5.2');
+  });
 });

+ 41 - 11
src/hooks/foreground-fallback/index.ts

@@ -15,6 +15,7 @@
  */
 
 import type { PluginInput } from '@opencode-ai/plugin';
+import { ALL_AGENT_NAMES } from '../../config/constants';
 import { log } from '../../utils/logger';
 import {
   abortSessionWithTimeout,
@@ -236,17 +237,37 @@ export class ForegroundFallbackManager {
         this.sessionTried.set(sessionID, new Set());
       }
       // biome-ignore lint/style/noNonNullAssertion: We just set this above
-      const tried = this.sessionTried.get(sessionID)!;
+      let tried = this.sessionTried.get(sessionID)!;
       if (currentModel) tried.add(currentModel);
 
-      const nextModel = chain.find((m) => !tried.has(m));
+      let nextModel = chain.find((m) => !tried.has(m));
       if (!nextModel) {
-        log('[foreground-fallback] fallback chain exhausted', {
-          sessionID,
-          agentName,
-          tried: [...tried],
-        });
-        return;
+        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.
+          const primary = chain[0];
+          const stickyFallback = chain[chain.length - 1];
+          log('[foreground-fallback] resetting tried set for re-fallback', {
+            sessionID,
+            agentName,
+            currentModel,
+            prevTried: [...tried],
+            nextModel: stickyFallback,
+          });
+          tried = new Set();
+          if (primary) tried.add(primary);
+          if (currentModel && currentModel !== primary) tried.add(currentModel);
+          this.sessionTried.set(sessionID, tried);
+          nextModel = stickyFallback;
+        } else {
+          log('[foreground-fallback] fallback chain exhausted', {
+            sessionID,
+            agentName,
+            tried: [...tried],
+          });
+          return;
+        }
       }
       tried.add(nextModel);
 
@@ -352,9 +373,18 @@ export class ForegroundFallbackManager {
     currentModel: string | undefined,
   ): string[] {
     if (agentName) {
-      // Agent is known: use its chain exactly, or no chain at all.
-      // Never fall through to cross-agent chains when the agent is identified.
-      return this.chains[agentName] ?? [];
+      // Agent is known: use its chain exactly if configured.
+      const chain = this.chains[agentName];
+      if (chain) return chain;
+      // Known omos built-in agent (oracle, librarian, …) without a
+      // configured chain: keep isolation — do NOT bleed into other
+      // agents' chains (preserves the cross-agent isolation contract
+      // from PR #199).
+      if ((ALL_AGENT_NAMES as readonly string[]).includes(agentName)) return [];
+      // Unknown agent (e.g. OpenCode built-in "compaction" or "title"
+      // that don't appear in the user preset): fall through to
+      // model-matching so they can inherit a chain from a configured
+      // agent that shares their model.
     }
 
     // Agent unknown: try to infer from the current model.

+ 12 - 1
src/hooks/task-session-manager/index.ts

@@ -12,6 +12,7 @@ import {
 } from '../../utils';
 import { isRecord as isObjectRecord } from '../../utils/guards';
 import { log } from '../../utils/logger';
+import { isRateLimitError } from '../foreground-fallback/index';
 import type { MessagePart, MessageWithParts } from '../types';
 
 interface TaskArgs {
@@ -745,7 +746,17 @@ export function createTaskSessionManagerHook(
         const sessionId =
           input.event.properties?.info?.id ?? input.event.properties?.sessionID;
         if (sessionId && options.shouldManageSession(sessionId)) {
-          terminalJobsInjectedByParent.delete(sessionId);
+          // Only clear injected terminal jobs for fatal errors.
+          // Rate-limit errors are recovered by ForegroundFallbackManager
+          // (abort + reprompt with fallback model); clearing the injected
+          // job state here would make the orchestrator lose track of
+          // completed background tasks and unable to dispatch follow-ups.
+          const props = input.event.properties as
+            | { error?: unknown }
+            | undefined;
+          if (!props?.error || !isRateLimitError(props.error)) {
+            terminalJobsInjectedByParent.delete(sessionId);
+          }
         }
 
         return;