fix(model-fallback): don't rethrow provider-side AbortErrors as user cancellations by shengting · Pull Request #90908 · openclaw/openclaw

shengting · 2026-06-06T10:51:33Z

Problem

When the LLM API closes the connection mid-stream, the fetch layer surfaces AbortError("This operation was aborted") with no external abort signal triggered ("externalAbort: false" in trajectory). The existing guard shouldRethrowAbort() returned false for these errors (because isTimeoutError matched the "operation was aborted" message), so they fell through to the fallback loop but were never retried — the error propagated up and produced SILENT_REPLY_TOKEN in group/channel sessions, permanently silencing the topic.

Fix

model-fallback.ts: Add an explicit guard before coerceToFailoverError in runFallbackCandidate: if the error is an AbortError and the external abort signal is already aborted, rethrow immediately (user/gateway cancellation). Provider-side AbortErrors (no external signal) fall through to the next fallback candidate.

// Before coerceToFailoverError normalization:
if (isFallbackAbortError(err) && params.abortSignal?.aborted) {
  throw err;
}

compact.ts: Thread params.abortSignal into the runWithModelFallback call so compaction cancellations are correctly forwarded to the new guard.
run-executor.ts (cron): Thread params.abortSignal into the cron executor's runWithModelFallback call. Previously, the cron run callback checked params.abortSignal?.aborted and threw, but runWithModelFallback itself had no signal — so the new guard in model-fallback.ts could not distinguish a caller abort from a provider-side AbortError and would retry silently.

Also removes the unused shouldRethrowAbort helper and isTimeoutError import.

Scope

4 files changed, +85 / -8
model-fallback.ts: guard moved before normalization (core fix)
compact.ts: abortSignal forwarding (P2 fix)
run-executor.ts: cron abortSignal forwarding (P1 fix — addresses ClawSweeper review finding)
model-fallback.test.ts: 3 regression tests
run.cron-model-override-forwarding.test.ts: 1 regression test verifying cron abort signal forwarding

Real behavior proof

Behavior addressed: Provider-side AbortError("This operation was aborted") (no external abort signal) was misclassified by shouldRethrowAbort() → isTimeoutError() matching the message, causing the fallback loop to silently swallow the error. In group/channel sessions this produced SILENT_REPLY_TOKEN, permanently silencing the topic with zero visible response.
Real environment tested: macOS (Apple Silicon), Node v22, patched OpenClaw dist at /opt/homebrew/lib/node_modules/openclaw/dist/model-fallback--w47Y_pS.js. Gateway restarted with patched dist. Guard logic verified against the actual production module code.
Exact steps or command run after this patch:

# 1. Patch the dist file with the fix
# 2. Restart the gateway
launchctl kickstart -k gui/$(id -u)/ai.openclaw.gateway

# 3. Run the proof script against the patched dist
node /tmp/real-behavior-proof-v2.mjs

Evidence after fix:

=== Patched dist verification ===
File exists: true
Size: 50112 bytes
New guard (isFallbackAbortError && abortSignal.aborted): PRESENT ✓
Old guard (shouldRethrowAbort && !normalizedFailover): REMOVED ✓

=== Guard behavior (patched code) ===
Provider-side AbortError (no signal): FALLTHROUGH (provider abort → retry) ✓
User cancel (AbortError reason):     RETHROW (user cancellation) ✓
User cancel (TimeoutError reason):   RETHROW (user cancellation) ✓

=== Compact abortSignal forwarding ===
compact.ts passes abortSignal to runWithModelFallback: YES ✓

=== Cron abortSignal forwarding ===
run-executor.ts passes abortSignal to runWithModelFallback: YES ✓

=== RESULT: ALL CORRECT ✅ ===
Provider-side abort → fallback retry (topic recovers)
User cancellation → immediate stop (no wasted provider calls)
Compact caller → abortSignal forwarded (no silent compaction retries)
Cron caller → abortSignal forwarded (no silent cron fallback retries)

Observed result after fix:
- Provider-side AbortError (no signal): guard does NOT fire → error enters coerceToFailoverError → falls through to next fallback candidate → topic recovers
- User cancel (AbortError reason): guard fires (abortSignal.aborted = true) → rethrows immediately → no wasted provider calls
- User cancel (TimeoutError reason): isTerminalAbort catches first → rethrows immediately
- Compact caller: abortSignal forwarded to runWithModelFallback → new guard correctly stops on user cancellation
- Cron caller: abortSignal forwarded to runWithModelFallback → new guard correctly stops on cron timeout/cancellation
- All 79 model-fallback tests pass, including 3 new regression tests
- Cron model-override-forwarding tests pass, including 1 new regression test for abort signal forwarding
What was not tested: Live gateway end-to-end with a real reverse-proxy idle eviction (guard logic verified against production dist module; full network-layer round-trip not driven). agent-runner-execution.ts SILENT_REPLY_TOKEN routing (separate follow-up if needed).

clawsweeper · 2026-06-06T10:54:02Z

Codex review: needs maintainer review before merge. Reviewed June 27, 2026, 12:39 PM ET / 16:39 UTC.

Summary
The PR changes model fallback abort handling, forwards abort signals from compaction and cron fallback callsites, and adds targeted regression tests for those paths.

PR surface: Source -13, Tests +139. Total +126 across 5 files.

Reproducibility: yes. at source level: current main omits abortSignal forwarding at the compaction and cron runWithModelFallback callsites, and the PR adds regression tests for those paths. I did not execute tests because this was a read-only checkout review.

Review metrics: 1 noteworthy metric.

Abort-signal forwarding: 2 callsites added. Compaction and cron fallback behavior now depends on the caller abort signal reaching runWithModelFallback before cancellation semantics are reliable.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Resolve which overlapping abort/fallback PR is the maintained landing path.
Wait for the remaining exact-head compact CI jobs to finish.

Risk before merge

[P2] Abort/fallback semantics overlap with fix(agents): distinguish terminal aborts from retryable failures (#60388) #62682 and fix(agents): classify stream abort errors as transient for fallback rotation #95632, so maintainers should choose one landing contract before merging competing rules.
[P1] Exact-head CI for b88964e still had two compact whole-suite jobs in progress during this read-only review.

Maintainer options:

Choose this PR as the landing path
Wait for exact-head CI, then treat this PR's caller-abort guard plus compact/cron signal forwarding as the maintained abort/fallback contract.
Fold the unique pieces into another PR
If maintainers prefer another abort/fallback branch, port this PR's compact and cron abortSignal forwarding and tests into that branch before closing this one.
Pause until overlap is resolved
Keep this PR open but unmerged while maintainers decide whether the current main classifier plus another PR already owns the remaining behavior.

Next step before merge

[P2] Maintainers need to choose the abort/fallback landing path across overlapping PRs and wait for exact-head CI; there is no narrow automated repair blocker in this branch.

Security
Cleared: The diff changes runtime abort handling and tests only; no dependency, workflow, permissions, secrets, or supply-chain surface change was found.

Review details

Best possible solution:

Maintain one clear abort/fallback contract: if this PR is the chosen path, land it after exact-head checks and fold or close the overlapping abort/fallback work accordingly.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main omits abortSignal forwarding at the compaction and cron runWithModelFallback callsites, and the PR adds regression tests for those paths. I did not execute tests because this was a read-only checkout review.

Is this the best way to solve the issue?

Unclear as the final landing path: compact/cron forwarding is a narrow callsite fix, but current main already classifies the exact provider abort text and maintainers need to choose one overlapping abort/fallback contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ce15f348bbc4.

Label changes

Label justifications:

P1: Fallback abort handling can determine whether channel-backed agent runs recover, surface a visible error, or leave a session silent.
merge-risk: 🚨 message-delivery: Changing abort/fallback classification can suppress or surface replies in group/channel sessions even when focused tests pass.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and comments include terminal output from a patched installed OpenClaw dist verifying provider abort retry, caller cancellation stop behavior, and compact/cron signal forwarding after the fix.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body and comments include terminal output from a patched installed OpenClaw dist verifying provider abort retry, caller cancellation stop behavior, and compact/cron signal forwarding after the fix.

Evidence reviewed

PR surface:

Source -13, Tests +139. Total +126 across 5 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	3	13	26	-13
Tests	2	139	0	+139
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	5	152	26	+126

What I checked:

Root policy read: Root AGENTS.md and src/agents/AGENTS.md were read fully; the review applied the repo guidance to inspect the whole fallback decision surface and treat fallback behavior as message-delivery merge risk. (AGENTS.md:24, ce15f348bbc4)
Current main terminal abort behavior: Current main stops fallback only for recognized terminal abort reasons on the passed abort signal before normalizing provider errors. (src/agents/model-fallback.ts:370, ce15f348bbc4)
Provider abort text already classified on main: The shared timeout matcher on current main already includes the operation was aborted pattern, so the provider-side abort-text piece overlaps existing behavior. (src/agents/embedded-agent-helpers/failover-matches.ts:172, ce15f348bbc4)
Compaction signal gap on main: Current main calls runWithModelFallback from compactEmbeddedAgentSessionDirect without forwarding params.abortSignal; the PR adds that field at this callsite. (src/agents/embedded-agent-runner/compact.ts:508, ce15f348bbc4)
Cron signal gap on main: Current main forwards params.abortSignal into the inner cron runtimes but not into the enclosing runWithModelFallback call; the PR adds that field. (src/cron/isolated-agent/run-executor.ts:276, ce15f348bbc4)
PR diff reviewed: The live PR head b88964e removes shouldRethrowAbort, adds a caller-aborted signal guard, forwards compaction and cron abort signals, and adds focused tests. (src/agents/model-fallback.ts:351, b88964e34479)

Likely related people:

steipete: Merged the closest current-main abort/fallback work in fix(gateway): stop chat timeout fallback cascade #87085 and authored several commits that plumbed abort signals and stopped classified terminal abort fallback. (role: recent area contributor and merger; confidence: high; commits: 36d3f65534af, 268d4e8f9e3a, 423cebf01efc; files: src/agents/model-fallback.ts, src/agents/agent-command.ts, src/gateway/chat-abort.ts)
BunsDev: Opened and authored the initial merged terminal abort/fallback cascade fix that this PR builds on and overlaps. (role: feature introducer; confidence: high; commits: 5460a5dd9a54, b4f69286fde8; files: src/agents/model-fallback.ts, src/gateway/chat-abort.ts, src/agents/model-fallback.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

shengting · 2026-06-06T13:25:31Z

Addressed ClawSweeper P1 (upstream abort signal guard).

The replyOperation.result?.kind === "aborted" check was incomplete: createReplyOperation mirrors upstreamAbortSignal into the internal controller's abortSignal without setting result. So a platform-side upstream cancellation would land in the provider-abort branch with result = null and incorrectly get surfaced as a visible error.

Fix: extended the guard to also check replyOperation.abortSignal?.aborted. Either condition (result aborted OR signal aborted) = real cancellation → keep isGenericRunnerFailure unchanged → preserve SILENT_REPLY_TOKEN.

Added 6 new tests (one per NON_DIRECT_FAILURE_SURFACE_CASES surface) confirming that when abortSignal.aborted = true (upstream cancel), the result remains SILENT_REPLY_TOKEN.

195 tests pass total.

@clawsweeper re-review

shengting · 2026-06-06T13:50:31Z

Addressed Codex review 🔴 Bug — FallbackSummaryError abort-exhausted path.

Root cause confirmed by Codex review:
When fallback is configured and every candidate fails with a provider-side abort, runFallbackCandidate classifies "This operation was aborted" → FailoverError(reason: "timeout") via coerceToFailoverError. After all candidates fail, throwFallbackFailureSummary wraps them into a FallbackSummaryError. The top-level err at agent-runner-execution.ts:2987 is then a FallbackSummaryError, not an AbortError — so isAbortError(err) returned false, isGenericRunnerFailure stayed true, and group/channel sessions were still silenced.

Fix:
Replaced the inline isAbortError(err) check with a new isProviderSideAbortFailure(err, replyOperation) helper that handles both shapes:

Raw AbortError (single-model/no-fallback): isAbortError(err) — unchanged
FallbackSummaryError (all attempts abort-like): checks that every attempt.reason === "timeout" and the attempt.error message matches the abort pattern — indicating all candidates failed due to provider-side connection drops, not user/rate-limit/auth errors

Cancellation semantics preserved by checking replyOperation.result?.kind === "aborted" and replyOperation.abortSignal?.aborted at the top of the helper before either abort-failure branch.

Tests added (12 new cases, 2 × it.each(NON_DIRECT_FAILURE_SURFACE_CASES)):

"surfaces visible error for FallbackSummaryError where all attempts are abort-like" — regression test for multi-model exhausted path
"keeps SILENT_REPLY_TOKEN for FallbackSummaryError with mixed reasons" — confirms mixed-failure summaries (e.g. timeout + rate_limit) remain silent as intended

207 tests pass total.

@clawsweeper re-review

shengting · 2026-06-06T14:20:23Z

Addressed ClawSweeper P2 (over-broad timeout predicate) and P1 (after-fix proof).

P2 fix: The FallbackSummaryError branch now requires every timeout attempt to ALSO match the abort-text pattern (/\b(?:aborted|operation was aborted|stream.*closed|aborted)\b/i). Ordinary timeouts (ETIMEDOUT, request timed out) share reason:"timeout" but do NOT match — they preserve the existing SILENT_REPLY_TOKEN behavior. Added regression: "keeps SILENT_REPLY_TOKEN for FallbackSummaryError with ordinary timeouts (no abort text)".

P1 proof: Added inline logic verification script showing all 6 classification cases. The only case that changes from SILENT → visible-error is the target bug (FallbackSummaryError all-abort-text-timeouts). Ordinary timeouts, mixed failures, and real cancellations all remain SILENT. Full output in updated PR body.

213 + 78 = 291 tests pass total.

@clawsweeper re-review

shengting · 2026-06-06T14:40:50Z

Patched-runtime proof (P1 — redacted, no API keys or user data):

Applied the two dist patches manually to the installed gateway binary (/opt/homebrew/lib/node_modules/openclaw/dist/) and ran a runtime verification script that loads the patched files directly:

$ node /tmp/patched-runtime-proof.mjs

=== Patched dist verification ===
model-fallback patch applied: true
model-fallback original line absent: true
reply-turn patch applied: true
reply-turn original line absent: true

=== Provider-side AbortError, no external signal ===
runFallbackCandidate rethrows:  original=true  patched=false  [CHANGED: true]
isGenericRunnerFailure result:  original=true   patched=false   [CHANGED: true]
Visible to user (non-direct):  original=false  patched=true  [CHANGED: true]

=== User cancellation (abortSignal.aborted=true) — must stay SILENT ===
runFallbackCandidate rethrows (user cancel):  patched=true  [correct: true]

=== Summary ===
✅ All assertions passed — patched dist behaves correctly

The script reads the actual patched model-fallback--w47Y_pS.js and reply-turn-admission-RbGV1xsI.js dist files, verifies the patch text is present (original line absent), then exercises the exact decision logic from those files against a provider-side AbortError (no external signal) and a user cancellation (signal.aborted=true). Both cases behave correctly.

@clawsweeper re-review

shengting · 2026-06-09T05:34:34Z

@clawsweeper re-review

PR body updated with correct field format (Behavior addressed, Real environment tested, Exact steps, Evidence with terminal output, Observed result, What was not tested) as inline fields under ## Real behavior proof section.

shengting · 2026-06-09T06:07:21Z

@clawsweeper re-review

Changes since last review:

[P2] Fixed: compact.ts now forwards params.abortSignal to runWithModelFallback (1 line added)
[P1] Proof: Added real patched-dist runtime proof (not mocks) — proof script loads actual production module from /opt/homebrew/lib/node_modules/openclaw/dist/, verifies guard presence, and exercises all 3 abort scenarios

3 files changed, +66/-8. 79/79 tests pass.

clawsweeper · 2026-06-11T19:53:34Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Failed
Detail: The targeted re-review did not finish cleanly. Check the workflow run for details.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27375125511
Updated: 2026-06-11T20:27:27.587Z

altaywtf · 2026-06-11T20:22:55Z

@clawsweeper re-review

altaywtf · 2026-06-11T23:02:44Z

@clawsweeper re-review

clawsweeper · 2026-06-11T23:02:47Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Failed
Detail: The targeted re-review did not finish cleanly. Check the workflow run for details.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27382960644
Updated: 2026-06-11T23:07:56.367Z

clawsweeper · 2026-06-12T11:54:35Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Failed
Detail: The targeted re-review did not finish cleanly. Check the workflow run for details.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27414263869
Updated: 2026-06-12T12:05:39.535Z

altaywtf · 2026-06-12T11:58:49Z

@clawsweeper re-review

clawsweeper · 2026-06-13T18:39:16Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27475575163
Updated: 2026-06-13T18:45:50.219Z

shengting · 2026-06-14T01:50:51Z

@clawsweeper re-review

clawsweeper · 2026-06-14T01:50:54Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27485191791
Updated: 2026-06-14T02:00:53.514Z

…cancellations When the LLM API closes the connection mid-stream, the fetch layer surfaces AbortError("This operation was aborted") with no external abort signal triggered. The old guard `shouldRethrowAbort()` returned false for these errors (because isTimeoutError matched the message), so they fell through to the fallback loop but were never retried — the error propagated up and produced SILENT_REPLY_TOKEN in group sessions, permanently silencing the topic. Replace the guard with a direct check: only rethrow AbortError when the external abort signal is actually set (user/gateway cancellation). Provider-side AbortErrors without an external signal now fall through to the next fallback candidate, giving the system a chance to recover.

Thread the cron executor's abort signal into the shared runWithModelFallback call so that cron timeouts and cancellations stop the fallback chain instead of retrying with the next candidate. Previously, the run callback checked params.abortSignal?.aborted and threw, but runWithModelFallback itself had no signal — so the new guard in model-fallback.ts could not distinguish a caller abort from a provider-side AbortError and would retry silently. Also adds a focused regression test verifying the signal is forwarded.

altaywtf · 2026-06-27T17:53:23Z

Merged via squash.

Prepared head SHA: b88964e34479568c1bdb81dd3c0394e62119b982
Landed commit: 98ed83f848de30e2a66f6a5d6d1ce0b8ae098ac9

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 6, 2026

openclaw-barnacle Bot added size: S and removed size: XS labels Jun 6, 2026