fix(channels): retry every transient timeout and parse Retry-After safely - #782
Conversation
andreapn
left a comment
There was a problem hiding this comment.
This is the strongest of the four PRs on #599 and the one I want to land — it is the only one that covers #599 and both #642 defects together, as requested on the issue thread. Verified against origin/main (8cdd5a3): merges cleanly, tests/test_channels/ 331 passed, scheduler/delivery/webhook/retry selection 635 passed, ruff + mypy clean. Every retry_request caller ends in raise_for_status(), so returning the exhausted 429 strictly improves what they see. The 300s clamp on the remote-controlled Retry-After is a real hardening win.
One blocker before merge:
CHANGELOG entry lands in a released section. The hunk anchors on the ### Fixed block that was under [Unreleased] when you branched. 2026.9.2 shipped since, so that block is now inside ## [2026.9.2] - 2026-09-02. Git still auto-merges it, but the result puts this fix at line 109, under the released 2026.9.2 heading — claiming a fix that is not in the published artifact.
Fix: rebase on current main and move the entry under ## [Unreleased] with its own ### Fixed heading (currently empty).
Nothing else needs changing.
…fely `retry_request` caught `(httpx.ConnectError, httpx.ReadTimeout)`. `ConnectTimeout`, `WriteTimeout` and `PoolTimeout` descend from `TimeoutException`, a sibling of `ConnectError` under `TransportError`, so a DNS lookup, TLS handshake, upload or connection-pool stall on any Slack/Discord/Telegram/webhook call went straight past the backoff and crashed the caller on the first blip. The clause is now `(ConnectError, TimeoutException)`, which covers all four timeout types. `Retry-After` was read with a bare `float()`. RFC 7231 §7.1.3 permits an HTTP-date, so a provider sending one turned a rate limit into a `ValueError` inside the retry loop. `_retry_after_delay` now resolves either form, falls back to the computed backoff when the header is unparseable, non-finite, negative or already past, and clamps the honoured delay to `MAX_RETRY_AFTER_S` (300s) so a provider cannot park a channel send for hours. The 429 branch also gains the `attempt < max_retries` guard the 5xx branch already had (use-agent-os#599). Without it the final rate-limited attempt slept once more and then fell out of the loop into `RuntimeError("retry_request exhausted")`, discarding the status, `Retry-After` and provider error body; it now returns the response like an exhausted 5xx does. Fixes use-agent-os#642 Fixes use-agent-os#599 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5mxRuJ5TQhEaUKz1g8QJq
d5549da to
4ec5b71
Compare
|
Re-reviewed against current Local gate in a clean worktree on the merge result:
Approving. Merging once CI is green. |
|
why always lost from hani sir ? any guide to competed ? |
Fixes #642
Fixes #599
Per the maintainer note on both issues ("please coordinate so the three land coherently rather than as conflicting edits to one 30-line function"), this covers all three defects in
retry_requestin one change.1. Timeouts that never reached the backoff (#642)
The clause was
except (httpx.ConnectError, httpx.ReadTimeout).ConnectTimeout,WriteTimeoutandPoolTimeoutdescend fromTimeoutException— a sibling ofConnectErrorunderTransportError— so a DNS lookup, TLS handshake, upload or connection-pool stall on any Slack/Discord/Telegram/webhook call went straight past the retry loop and crashed the caller on the first blip.(httpx.ConnectError, httpx.TimeoutException)covers all four timeout types in one clause.ReadTimeoutis included by inheritance, so nothing that was retried before stops being retried.2.
Retry-Aftercrash (#642)float(resp.headers.get("Retry-After", ...))is a bare parse of a caller-controlled header, and RFC 7231 §7.1.3 permits an HTTP-date — a provider sending one turned a rate limit into aValueErrorinside the retry loop._retry_after_delaynow:nan/infboth survivefloat()), negative, or an HTTP-date already in the past;MAX_RETRY_AFTER_S = 300.0, the "sane upper clamp" asked for in the issue thread — a provider can otherwise ask us to sleep for hours, and the header is remote input.The caller's own backoff is never clamped; only the remote value is.
3. Exhausted 429 discarded the response (#599)
The 5xx branch is guarded by
attempt < max_retriesand falls through toreturn respon the last attempt; the 429 branch had no such guard, so the final rate-limited attempt slept once more and then fell out of the loop intoraise last_exc or RuntimeError("retry_request exhausted")— withlast_exc is Nonethere, the caller got a bareRuntimeErrorand the status,Retry-Afterand provider error body were all thrown away.The 429 branch now carries the same guard, so an exhausted rate limit returns the response exactly like an exhausted 5xx does. Callers that
raise_for_status()(Slack, Discord) now get anHTTPStatusErrorcarrying the provider payload instead of an opaqueRuntimeError. Nothing in the tree caught thatRuntimeError(grep -rn "exhausted" src/agentos/channels src/agentos/schedulerfinds no consumer).Tests
27 new tests in
tests/test_channels/test_channel_retry_util.py, pinning the helper's own contract rather than going through an adapter:ConnectError,ConnectTimeout,ReadTimeout,WriteTimeout,PoolTimeout), and re-raised once retries are exhausted;UnsupportedProtocolstill surfaces on the first attempt with no sleep;Retry-Afteras seconds, as an HTTP-date, missing, empty/blank, malformed, ISO-8601,nan,inf, negative, in the past, and oversized (clamped);18 of the 27 fail against
mainand pass with the patch.Verification
Note: #643 and #645 also target #642, and #603 targets all three — offered as an alternative for whichever you prefer.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q5mxRuJ5TQhEaUKz1g8QJq