fix(gateway): prevent session death loop on overloaded fallback#58379
Conversation
Greptile SummaryThis PR fixes a confirmed production "death loop" where sessions would silently stop responding when a provider returned
Both guards are correctly scoped to their respective error types and don't disturb existing retry/rotation behavior for rate limit, auth, billing, or context overflow errors. The logic is sound and the fix directly addresses the root cause. Three minor style/observability nits are noted below. Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions that do not affect runtime behavior or correctness. The core fix is logically correct: both retry counters use consistent off-by-one semantics (strict > check), the overload rotation cap applies only when fallbackConfigured = true (gracefully degrades with no fallback), auth-profile state propagation across retries is preserved, and the new unit and e2e tests cover the critical paths. The three findings are all P2: a misleading log label before FailoverError, a locally-scoped constant that should be exported for consistency, and weak toBeLessThanOrEqual assertions that should be toBe. None affect actual failover behavior. No files require special attention — all issues are non-blocking style/observability concerns.
|
| Filename | Overview |
|---|---|
| src/auto-reply/reply/agent-runner-execution.ts | Adds liveModelSwitchRetries counter and MAX_LIVE_SWITCH_RETRIES=2 guard to break the unbounded while(true) loop on repeated LiveSessionModelSwitchError; returns a user-visible final error on exhaustion. |
| src/agents/pi-embedded-runner/run.ts | Adds overloadProfileRotations counter; when cap exceeded and fallback is configured, throws FailoverError immediately instead of cycling through remaining same-provider profiles. |
| src/agents/pi-embedded-runner/run/helpers.ts | Exports MAX_OVERLOAD_PROFILE_ROTATIONS=1 constant with documentation linking to the issue. |
| src/auto-reply/reply/agent-runner-execution.test.ts | Adds two unit tests: infinite-loop prevention (asserts kind=final after retry exhaustion) and auth-profile state propagation across bounded retries (asserts kind=success with correct profile on 3rd attempt). |
| src/agents/model-fallback.run-embedded.e2e.test.ts | Adds e2e test verifying that with 3 OpenAI profiles and persistent overloaded errors, the runner escalates to the groq fallback after at most 2 OpenAI attempts instead of exhausting all profiles. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run.ts
Line: 1213
Comment:
**Misleading decision label before FailoverError**
`logAssistantFailoverDecision("rotate_profile")` is called here, but the very next action is throwing a `FailoverError` that escalates to cross-provider fallback — not a profile rotation. When this code path fires, any observability tooling (metrics, dashboards, audit logs) that reads decision events will record `"rotate_profile"` for what is actually a `"fallback_model"` decision. The normal fallback escalation path at line ~1261 correctly logs `"fallback_model"`. Consider removing this call entirely (the `log.warn` on line 1216 already documents the decision) or replacing it with the correct label to keep the signal accurate.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-execution.ts
Line: 183
Comment:
**`MAX_LIVE_SWITCH_RETRIES` should be a module-level export**
`MAX_OVERLOAD_PROFILE_ROTATIONS` is exported from `run/helpers.ts` so it can be referenced precisely in tests. `MAX_LIVE_SWITCH_RETRIES` is defined locally inside the function body, which means tests must use indirect assertions (`toBeLessThanOrEqual(3)`) rather than deriving the expected count from the constant. Exporting it alongside `MAX_OVERLOAD_PROFILE_ROTATIONS` would make the limit discoverable, testable, and consistent with the established pattern.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/model-fallback.run-embedded.e2e.test.ts
Line: 563
Comment:
**Weak assertion doesn't enforce the exact rotation count**
The comment above documents the exact expected behavior: 1 initial attempt + 1 rotation = **exactly 2** OpenAI calls. Using `toBeLessThanOrEqual(2)` would still pass if the fix accidentally collapsed to only 1 openai attempt (a regression to over-aggressive capping). A precise assertion would catch that:
```suggestion
expect(openaiAttempts.length).toBe(2);
```
Similarly, `expect(switchCallCount).toBeLessThanOrEqual(3)` in `agent-runner-execution.test.ts` (line 395) should be `toBe(3)` to pin the exact retry count at the outer loop level.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(gateway): prevent session death loop..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Persistent LiveSessionModelSwitchError state can cause repeated per-turn retry loops (DoS risk)
DescriptionIn This creates a persistent bad state across turns:
Impact:
Vulnerable code (early return bypasses state update): if (liveModelSwitchRetries > MAX_LIVE_SWITCH_RETRIES) {
// ... log ...
return {
kind: "final",
payload: { text: switchErrorText },
};
}
params.followupRun.run.provider = err.provider;
params.followupRun.run.model = err.model;
params.followupRun.run.authProfileId = err.authProfileId;RecommendationEnsure the session/model selection state is healed even when the retry cap is exceeded. Options (pick one consistent with intended behavior):
if (liveModelSwitchRetries > MAX_LIVE_SWITCH_RETRIES) {
// Heal state so next turn doesn't repeat the conflict
params.followupRun.run.provider = err.provider;
params.followupRun.run.model = err.model;
params.followupRun.run.authProfileId = err.authProfileId;
params.followupRun.run.authProfileIdSource = err.authProfileId
? err.authProfileIdSource
: undefined;
return {
kind: "final",
payload: { text: switchErrorText },
};
}
Also consider incrementing a metric/counter for this path (e.g., Analyzed PR: #58379 at commit Last updated on: 2026-03-31T14:56:46Z |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d79676fcde
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Quick update on where things stand after the two commits: What the second commit (
CI failures — Both C1 control character sanitization (Medium) — The flag about Everything from the review rounds has been addressed. Ready for review . |
- Add MAX_LIVE_SWITCH_RETRIES=2 guard in agent-runner-execution.ts - Add MAX_OVERLOAD_PROFILE_ROTATIONS=1 cap in run.ts for overloaded errors - Return kind:final with user-visible error on retry exhaustion - Escalate to cross-provider fallback instead of exhausting same-provider profiles Fixes openclaw#58348
- Fix CWE-209: use static safe message instead of raw provider error text - Fix CWE-117: sanitize provider/model in logs via sanitizeForLog - Hide CLI hints from external channels via shouldSurfaceToControlUi - Move overload cap check before advanceAuthProfile to save setup latency - Export MAX_LIVE_SWITCH_RETRIES as module-level constant - Use exact toBe() assertions in tests - Correct failover decision label to fallback_model
882de4c to
2c41439
Compare
obviyus
left a comment
obviyus
left a comment
There was a problem hiding this comment.
Reviewed latest changes; landing now.
2 participants
Summary
overloaded_error, the session enters a "death loop" — a zombie state where it silently stops responding to all new messages without surfacing any error to the user. Observed 97 unbounded retries in production logs over 9 days, with 4+ confirmed incidents. The bot simply goes silent and never recovers.LiveSessionModelSwitchErrorretry insrc/auto-reply/reply/agent-runner-execution.ts(line 618): Thewhile(true)loop catchesLiveSessionModelSwitchErrorandcontinues without any retry limit. When the overloaded primary model triggers fallback to a different model, the innerrunEmbeddedPiAgentdetects a mismatch with the persisted session selection and throwsLiveSessionModelSwitchError. The outer loop rewritesfollowupRun.run.provider/modelback to the switched model and retries, but the persisted session keeps pulling back to the overloaded primary, creating an infinite ping-pong.src/agents/pi-embedded-runner/run.ts(line 1200): WhenshouldRotateis true for anoverloadederror, the runner rotates through all available auth profiles for the same provider. Since overload is a provider-level capacity issue (not per-key), this is futile and only adds unnecessary backoff delays (exponential, up to 30s per rotation) before finally escalating to cross-provider fallback.MAX_LIVE_SWITCH_RETRIES = 2guard inagent-runner-execution.ts. When exceeded, the function immediately returns{ kind: "final" }with a user-visible error message instead of continuing forever. This directly prevents the zombie state.MAX_OVERLOAD_PROFILE_ROTATIONS = 1cap inrun.tsspecifically forassistantFailoverReason === "overloaded". After one probe rotation (to handle transient blips), the runner throwsFailoverErrorto escalate immediately to cross-provider fallback, bypassing remaining same-provider profiles.Both guards are scoped exclusively to their respective error types, preserving existing behavior for all other error classes (rate_limit, auth, billing, etc.).
src/auto-reply/reply/agent-runner-execution.ts: AddedliveModelSwitchRetriescounter andMAX_LIVE_SWITCH_RETRIESguard. On exhaustion, returnskind: "final"with a user-facing error message.src/agents/pi-embedded-runner/run.ts: AddedoverloadProfileRotationscounter. Whenoverloadedrotations exceedMAX_OVERLOAD_PROFILE_ROTATIONS, throwsFailoverErrorto escalate to cross-provider fallback.src/agents/pi-embedded-runner/run/helpers.ts: ExportedMAX_OVERLOAD_PROFILE_ROTATIONS = 1constant with documentation.src/auto-reply/reply/agent-runner-execution.test.ts: Added 2 tests — infinite loop prevention and auth profile state propagation across bounded retries.src/agents/model-fallback.run-embedded.e2e.test.ts: Added e2e test for multi-profile overload rotation capping.overloaded_erroris parsed or classified (failover-matches.ts,failover-error.ts).src/cron/isolated-agent/run.ts), which already has its own bounded retry logic.OVERLOAD_FAILOVER_BACKOFF_POLICYtiming constants.Reproduction
anthropic/claude-opusand fallback chain[openai/gpt-5.4, groq/llama].anthropic/claude-opus.overloaded_errorfrom all Anthropic endpoints.LiveSessionModelSwitchErrorloop, never replies, and the session becomes a zombie. Logs show 97+ retries with no termination.Risk / Mitigation
MAX_LIVE_SWITCH_RETRIES = 2limit could prematurely terminate legitimate rapid model switches if a user triggers multiple valid switches in quick succession.MAX_OVERLOAD_PROFILE_ROTATIONS = 1cap only applies tooverloadederrors and still allows one probe attempt for transient blips. Both limits are tested with dedicated unit and e2e tests that verify exact retry counts and state propagation.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Fixes #58348