fix(agents): do not misclassify client-disconnect abort as run timeout#90936
Conversation
|
Codex review: passed. Reviewed June 14, 2026, 4:11 AM ET / 08:11 UTC. Summary PR surface: Source +5, Tests +32. Total +37 across 3 files. Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180-second proxy disconnect run. Current main routes a default AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow signal-reason classifier after maintainer acceptance, keeping the broad provider error classifier unchanged for model/failover errors. Do we have a high-confidence way to reproduce the issue? Yes from source inspection and a focused Node abort-reason check, but not from a live 180-second proxy disconnect run. Current main routes a default AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Is this the best way to solve the issue? Yes. This is the best current fix shape because it narrows only abort-signal timeout detection to explicit TimeoutError reasons while preserving the broader message heuristics for provider and stream failover classification. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d2c0d3ac9bc2. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +5, Tests +32. Total +37 across 3 files. 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
|
93467c8 to
2a171ad
Compare
|
@clawsweeper automerge |
|
🦞✅ Source: What merged:
Automerge notes:
The automerge loop is complete. Automerge progress:
|
AbortController.abort() with no args produces DOMException("This operation
was aborted", "AbortError"). The onAbort / onExternalAbortSignal handlers
in attempt.ts called isTimeoutError() on that reason; isTimeoutError delegates
to hasTimeoutHint → isTimeoutErrorMessage, which matches the pattern
/\boperation was aborted\b/i (added for Ollama stream aborts), so the plain
cancellation was mis-labelled timeout=true → timedOut=true.
Because watchClientDisconnect fires abort() with no args whenever the HTTP
connection closes (e.g. reverse-proxy idle eviction after ~120s of no SSE
data), embedded agent runs reported "Request timed out before a response was
generated." regardless of agents.defaults.timeoutSeconds or
models.providers.*.timeoutSeconds — the underlying LLM call was still in its
thinking phase.
Fix: replace isTimeoutError with isSignalTimeoutReason in both abort-signal
handlers. isSignalTimeoutReason checks only name==="TimeoutError", which is
the name produced by AbortSignal.timeout() and makeTimeoutAbortReason(). It
never matches plain AbortError, so client disconnections are kept as
externalAbort=true / timeout=false.
Fixes openclaw#90764
2a171ad to
2708b0a
Compare
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
|
Hi @vincentkoc, @Takhoffman — could a maintainer take a look at this merge when you have a moment? We're hitting this exact bug on ~5 production user pods running Status checks look clean, Happy to share redacted server logs from our cluster confirming the exact failure mode if useful. cc @openperf — thanks for the fix! |
openclaw#90936) Summary: - The PR adds an abort-signal-specific timeout classifier, switches two embedded attempt abort handlers to it, and adds focused failover tests. - PR surface: Source +5, Tests +32. Total +37 across 3 files. - Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180- ... ault AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): do not misclassify client-disconnect abort as run timeout Validation: - ClawSweeper review passed for head 2708b0a. - Required merge gates passed before the squash merge. Prepared head SHA: 2708b0a Review: openclaw#90936 (comment) Co-authored-by: openperf <[email protected]> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
#90936) Summary: - The PR adds an abort-signal-specific timeout classifier, switches two embedded attempt abort handlers to it, and adds focused failover tests. - PR surface: Source +5, Tests +32. Total +37 across 3 files. - Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180- ... ault AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): do not misclassify client-disconnect abort as run timeout Validation: - ClawSweeper review passed for head 2708b0a. - Required merge gates passed before the squash merge. Prepared head SHA: 2708b0a Review: #90936 (comment) Co-authored-by: openperf <[email protected]> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> (cherry picked from commit fd80e0d)
openclaw#90936) Summary: - The PR adds an abort-signal-specific timeout classifier, switches two embedded attempt abort handlers to it, and adds focused failover tests. - PR surface: Source +5, Tests +32. Total +37 across 3 files. - Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180- ... ault AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): do not misclassify client-disconnect abort as run timeout Validation: - ClawSweeper review passed for head 2708b0a. - Required merge gates passed before the squash merge. Prepared head SHA: 2708b0a Review: openclaw#90936 (comment) Co-authored-by: openperf <[email protected]> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
openclaw#90936) Summary: - The PR adds an abort-signal-specific timeout classifier, switches two embedded attempt abort handlers to it, and adds focused failover tests. - PR surface: Source +5, Tests +32. Total +37 across 3 files. - Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180- ... ault AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): do not misclassify client-disconnect abort as run timeout Validation: - ClawSweeper review passed for head 2708b0a. - Required merge gates passed before the squash merge. Prepared head SHA: 2708b0a Review: openclaw#90936 (comment) Co-authored-by: openperf <[email protected]> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
openclaw#90936) Summary: - The PR adds an abort-signal-specific timeout classifier, switches two embedded attempt abort handlers to it, and adds focused failover tests. - PR surface: Source +5, Tests +32. Total +37 across 3 files. - Reproducibility: yes. from source inspection and a focused Node abort-reason check, but not from a live 180- ... ault AbortController abort reason through the broad timeout classifier used by the embedded abort handlers. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): do not misclassify client-disconnect abort as run timeout Validation: - ClawSweeper review passed for head 2708b0a. - Required merge gates passed before the squash merge. Prepared head SHA: 2708b0a Review: openclaw#90936 (comment) Co-authored-by: openperf <[email protected]> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
…penclaw#90936) Add isSignalTimeoutReason and use it in pi-embedded attempt abort handling so plain AbortError disconnects are not treated as run timeouts. Co-authored-by: Cursor <[email protected]>
Summary
agents.defaults.timeoutSecondsormodels.providers.*.timeoutSeconds. The run exits with "Request timed out before a response was generated." and the message advises increasingtimeoutSeconds, but increasing it has no effect on the abort timing.onAbortandonExternalAbortSignalinsrc/agents/embedded-agent-runner/run/attempt.tscallisTimeoutError(reason)on the abort-signal reason to decide whether to settimedOut = true.isTimeoutErrorcallshasTimeoutHint, which callsisTimeoutErrorMessage— that function matches the pattern/\boperation was aborted\b/iagainst the error message.AbortController.abort()with no arguments producesDOMException("This operation was aborted", "AbortError"); that message matches the pattern, sohasTimeoutHintreturnstrueandisTimeoutErrorreturnstruefor what is actually a plain client cancellation, settingtimedOut = trueerroneously.watchClientDisconnectinsrc/gateway/http-common.tscallsabortController.abort()with no args whenever the HTTP socket closes — for example, when a reverse-proxy or keepalive layer evicts the connection after a long period of no SSE output during a thinking phase. This firesparams.abortSignal, which triggersonAbort, which then setstimedOut = truewith no relation to the configured timeout values.isTimeoutErrorwith a newisSignalTimeoutReasonexport fromsrc/agents/failover-error.ts.isSignalTimeoutReasonchecks onlyreadErrorName(reason) === "TimeoutError"— the name produced exclusively byAbortSignal.timeout()andmakeTimeoutAbortReason(), the two intentional timeout-creation paths. PlainAbortError(name = "AbortError") is a cancellation and returnsfalse.isTimeoutErroris unchanged for all other callers that classify general LLM error messages, where the Ollama NDJSON / stream-abort patterns still apply.src/agents/failover-error.ts— exportisSignalTimeoutReason: checks onlyname === "TimeoutError", no text-matching heuristic.src/agents/embedded-agent-runner/run/attempt.ts— replaceisTimeoutErrorimport and both abort-signal handler call sites (onExternalAbortSignalandonAbort) withisSignalTimeoutReason.src/agents/failover-error.test.ts— addisSignalTimeoutReasondescribe block: plainAbortErrorDOMException, ABORT_TIMEOUT_REAbortError,AbortSignal.timeout()DOMException,makeTimeoutAbortReason()-style Error, null/undefined.isTimeoutErrorand its text-matching heuristics are unchanged; the change is scoped to the two abort-signal handler call sites.docs/reference/config).extensions/*api/runtime-api, registry/loader).Reproduction
agents.defaults.timeoutSeconds: 600andmodels.providers.custom-api-anthropic-com.timeoutSeconds: 600.watchClientDisconnectfiresabortController.abort()with no args.onAbortcallsisTimeoutError(DOMException("This operation was aborted", "AbortError"))—hasTimeoutHintmatches the message viaisTimeoutErrorMessage(/\boperation was aborted\b/i), returnstrue;timedOut = true; run exits at ~165–180 s with "Request timed out before a response was generated."; increasingtimeoutSecondshas no effect.isSignalTimeoutReason(DOMException("This operation was aborted", "AbortError"))checksreadErrorName(reason) === "TimeoutError"→false;timedOutis not set; the abort is treated as an external cancellation, not a run timeout.Real behavior proof
Behavior addressed (#90764): after the fix, a client-disconnect abort no longer sets
timedOut = truein the embedded attempt; only abort-signal reasons withname === "TimeoutError"(fromAbortSignal.timeout()ormakeTimeoutAbortReason()) are treated as run timeouts.Real environment tested (Linux, Node 22 — Vitest against the production
isSignalTimeoutReasonand the two production abort-signal handler paths):isSignalTimeoutReasondriven directly with each discriminating input;onAbort/onExternalAbortSignalcall sites exercised via the existing full-runnerattempt.test.tssuite.Exact steps or command run after this patch:
pnpm test src/agents/failover-error.test.ts;pnpm test src/agents/embedded-agent-runner/run/attempt.test.ts;pnpm format:checkandnode scripts/run-oxlint.mjson the three changed files;pnpm tsgo.Evidence after fix:
The
isSignalTimeoutReasondescribe covers:DOMException("This operation was aborted", "AbortError")→false(watchClientDisconnect regression case; oldisTimeoutErrorreturnedtrueviaisTimeoutErrorMessagematching/\boperation was aborted\b/i);Error("request aborted", name="AbortError")→false(oldisTimeoutErrorreturnedtrueviaABORT_TIMEOUT_RE);DOMException("signal timed out", "TimeoutError")→true;Error("request timed out", name="TimeoutError")→true; null/undefined →false.Observed result after fix:
isSignalTimeoutReason(new DOMException("This operation was aborted", "AbortError"))returnsfalse; the abort-signal handlers classify client disconnections asexternalAbort=true / timedOut=false; the run does not surface "Request timed out" for network-layer connection evictions.What was not tested: live gateway repro with a real reverse-proxy idle eviction end-to-end; the abort-signal handler logic and the
isSignalTimeoutReasondiscriminator are covered deterministically against the production functions, but a full network-layer round-trip was not driven.Risk / Mitigation
isSignalTimeoutReasonis strictly narrower thanisTimeoutErrorfor the two affected call sites; a timeout path that passesparams.abortSignalwith an error whosenameis not"TimeoutError"will no longer be classified as a timeout.makeTimeoutAbortReason()andAbortSignal.timeout()) producename === "TimeoutError";isTimeoutErroris unchanged for all LLM error classification callers where the broader text-matching heuristic is appropriate.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Fixes #90764