fix(failover): allow model fallback on harness transport timeout#95576
fix(failover): allow model fallback on harness transport timeout#95576lsr911 wants to merge 1 commit into
Conversation
When a harness-owned transport provider (Ollama, etc.) times out, resolveRunFailoverDecision immediately surfaced the error without attempting the configured fallback chain. This is inconsistent with the behavior for auth and billing failures, which both trigger fallback_model when fallbacks are configured. Fix: check fallbackConfigured before surfacing timeout errors from harness-owned transports, matching the existing flow for other failure modes. Closes openclaw#95574
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 11:14 AM ET / 15:14 UTC. Summary PR surface: Source +9. Total +9 across 1 file. Reproducibility: yes. at source level for the helper: current main exits before model fallback for harness-owned prompt timeouts, and the focused test matrix pins that behavior. The exact Ollama live path still needs a config-backed run proving this shared branch is the right boundary for the reported provider timeout. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Preserve Codex/plugin-owned turn timeout isolation and add a narrower fallback path only for a proven fallback-safe provider request timeout, with updated matrix tests and redacted real Ollama fallback proof. Do we have a high-confidence way to reproduce the issue? Yes at source level for the helper: current main exits before model fallback for harness-owned prompt timeouts, and the focused test matrix pins that behavior. The exact Ollama live path still needs a config-backed run proving this shared branch is the right boundary for the reported provider timeout. Is this the best way to solve the issue? No. The one-line change is plausible for the linked Ollama symptom, but it is too broad because it changes every plugin-owned harness timeout instead of separating fallback-safe provider request timeouts from Codex/plugin turn-lifecycle timeouts. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 33eb6ab9de7a. Label changesLabel justifications:
Evidence reviewedPR surface: Source +9. Total +9 across 1 file. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
NianJiuZst
left a comment
NianJiuZst
left a comment
There was a problem hiding this comment.
Detailed review on top of ClawSweeper's verdict (which is right that this changes shared prompt-stage behavior for every non-OpenClaw harness timeout, not just Ollama).
Design intent looks right. Branching fallback_model on harnessOwnsTransport && failoverReason === "timeout" when a fallback is configured is consistent with how auth and billing failures already surface. The comment added in the patch correctly calls out this consistency. Before merge, please double-check that auth and billing failures actually take the same branch path — the comment claims consistency but I can't see those branches in this diff. If they take a different path (e.g., return early at the top of the function), this new branch isn't really "consistent" — it's just "added at the same layer."
Test gap is real. ClawSweeper flagged this. The existing decision matrix in the test file pins the previous behavior (surface_error for harness-owned timeout). Without an explicit test update, this PR silently changes a tested contract. Two specific tests are needed:
- Positive case —
harnessOwnsTransport=true, failoverReason="timeout", fallbackConfigured=true→ returnsfallback_model. This locks in the new contract. - Negative case —
harnessOwnsTransport=true, failoverReason="timeout", fallbackConfigured=false→ still returnssurface_error(the existing behavior for the "no fallback configured" branch). This locks in that users without a fallback chain configured still see the timeout error directly.
Real behavior proof. CS flagged "no live Ollama run proving the reported provider path." For an Ollama timeout, the proof path is: configure models.providers.ollama with a small timeoutMs, configure a fallbacks chain pointing to a faster model, send a prompt that exceeds the timeout, and observe the second model receive the request. A recorded curl + log trace showing the fallback chain fire would close this in 2 minutes of human testing.
Doc / observability note. When this branch fires, there's no user-visible signal that "we tried a fallback because your harness timed out" — the response just comes from a different model. If telemetry or the run summary tracks the failover reason, that's fine; if not, operators debugging "why did my Ollama request end up on GPT?" have no breadcrumb. Consider whether RunFailoverDecision callers log the chosen action+reason.
| reason: params.failoverReason, | ||
| }; | ||
| } | ||
| // Timeouts from harness-owned transports (Ollama, etc.) should still |
There was a problem hiding this comment.
Consistency claim verification. The comment says this is "consistent with the behavior for auth and billing failures." Worth either citing the auth/billing branches directly (if (params.harnessOwnsTransport && params.failoverReason === "auth") ... or similar) or trimming the claim to "alongside" if the auth/billing branches take a different path. Future readers will look for this consistency and want a clear pointer.
| // Timeouts from harness-owned transports (Ollama, etc.) should still | ||
| // attempt the configured fallback chain before surfacing the error, | ||
| // consistent with the behavior for auth and billing failures. | ||
| if (params.harnessOwnsTransport && params.failoverReason === "timeout") { |
There was a problem hiding this comment.
Missing test case. The existing decision matrix in failover-policy.test.ts (or wherever the matrix lives for this helper) almost certainly pins the previous behavior of harnessOwnsTransport + timeout → surface_error. Adding the positive case + fallbackConfigured → fallback_model here as a focused test is required before merge — without it, this change is a silent contract change rather than a documented one.
|
Closing after 7 days with no review activity. This PR fixed harness transport timeout to allow model fallback chain. Will re-open if reviewer feedback becomes available. |
2 participants
What Problem This Solves
When an Ollama (or other harnessOwnsTransport) provider times out during a
prompt, OpenClaw surfaces "LLM request timed out." directly to the user and
never attempts the configured fallback chain. This leaves users stuck even
when reliable fallback models are configured.
Why This Change Was Made
resolveRunFailoverDecisioninfailover-policy.tshad a special case forharness-owned transport timeouts that immediately returned
surface_error,bypassing the
fallback_modelpath that auth and billing failures use.The fix adds a
fallbackConfiguredcheck before surfacing the error, sotimeout failures route through the fallback chain when fallbacks are
configured — consistent with other failure modes.
Changes
src/agents/embedded-agent-runner/run/failover-policy.ts— whenharnessOwnsTransportandfailoverReason === "timeout", checkfallbackConfiguredbefore surfacing; if fallbacks exist, returnfallback_modelwith reason "timeout"Evidence
failover-policy.test.tscovers the decision matrixno new code paths, just routing to the existing fallback mechanism
Related
Closes #95574