fix(gateway): honor server retry_after for flood-capped sends instead of plain-text fallback

[cdepuy]

Summary

Fixes a Telegram send-path bug that amplifies flooding and can lock the bot out of sending for an hour+.

The Telegram adapter's fail-closed flood path (_flood_cap_result in plugins/platforms/telegram/adapter.py) returns a SendResult carrying retry_after (the server's FloodWait seconds) but does not set retryable=True. _send_with_retry in gateway/platforms/base.py gated its retry-with-backoff path purely on result.retryable or self._is_retryable_error(error), and the flood_control:<n> error string is not in _RETRYABLE_ERROR_PATTERNS. Result: an over-cap FloodWait skips backoff entirely and falls straight into the plain-text fallback send, which immediately re-hits the same ban.

Repeated while a long ban is active, this renews the FloodWait and keeps the bot from delivering anything for tens of minutes. Observed in production: ~35x send amplification against a 1h+ ban (130 real responses -> ~4,600 send attempts), across 6 separate days.

Fix

Four coordinated changes to _send_with_retry in gateway/platforms/base.py (+1 _is_rate_limited_error helper):

1. Treat rate-limit as transient. Gate retry on classify_send_error(...) == "rate_limited" in addition to retryable/retry_after, so flood-capped sends from any platform — including Weixin, which surfaces a bare RuntimeError with no retry_after — route into the retry path instead of the truncating plain-text fallback.
2. Honor server retry_after as the backoff delay (Telegram FloodWait seconds) instead of the default exponential schedule. Retry-aware: a retry that itself returns a new retry_after re-honors it.
3. Reclassify per attempt, never reuse the first-send classification, so the in-loop break/continue reflects the current failure kind (covers transient → flood, and rate-limited → permanent-formatting transitions).
4. Never take the truncating plain-text fallback for a rate-limited send — return the typed failure so the delivery ledger owns redelivery after the cooldown. And when retries exhaust on a still-rate-limited / retry_after-carrying failure, return before the delivery-failure notice: the notice send would land inside the same flood penalty and re-enter the ban ([0, 189, 378, 378] → [0, 189, 378]; no 4th send). Ordinary exhausted network errors keep the existing notice behavior.

Timeout results are unaffected: retry_after is None for them, so is_network stays False and the no-duplicate-on-timeout guarantee is preserved.

Files changed

• gateway/platforms/base.py: _is_rate_limited_error helper + rewritten _send_with_retry.
• tests/gateway/test_send_retry.py: 10 new regression tests — rate-limit classifier, rate-limited-without-retry_after retries-and-succeeds, rate-limited exhaustion returns a typed failure (no fallback, no notice inside the penalty), and failure-kind transitions between attempts.

Rebased onto current main (resolves a 5000+ commit drift / CONFLICTING state). Reviewer feedback from a third-party deployment confirmed the root cause independently and identified the retry-loop else-branch notice issue; that is folded in and covered by an updated regression test.

Test plan

pytest tests/gateway/test_send_retry.py        # 22 passed (incl. 10 new)
pytest tests/gateway/test_slack_send_retry.py tests/gateway/test_send_error_classification.py  # 19 passed

Run locally against current main (Python 3.11 / 3.12). CI on this fork-PR awaits maintainer approval to execute (action_required).

Relationship to #103669 (flood-ledger redelivery)

This PR is the inline-retry half of the FloodWait handling: it makes an over-cap send honor the server's retry_after and back off inside _send_with_retry. #103669 (fix/ledger-flood-retry by @AlexxRussell) is the deferred half: it assigns long flood waits to the delivery ledger's scheduled redelivery. The two are complementary, not duplicates — #103669 touches _finalize_delivery_obligation / delivery_ledger.py (the post-exhaustion path), while this PR fixes the inline retry loop itself (the pre-exhaustion path). Ownership boundary is explicit: inline backoff here, ledger redelivery there. Happy to rebase or adjust this PR to land cleanly alongside #103669 either order.

[marian001]

Independently hit this same bug in production today and arrived at the same root-cause analysis, so this is confirmation from a second deployment plus two gaps I think the current diff leaves open.

Independent reproduction

Telegram gateway, two consecutive turns silently lost. The log shows the exact sequence this PR describes:

16:37:00 INFO  [Telegram] Sending response (3692 chars) to -50848XXXXX
16:37:00 WARN  [Telegram] Telegram flood control on send (retry_after=20.0s > 5s); failing closed instead of sleeping
16:37:00 WARN  [Telegram] Send failed: flood_control:20.0 — trying plain-text fallback
16:37:01 ERROR [Telegram] Fallback send also failed: flood_control:20.0

16:40:57 INFO  [Telegram] Sending response (2647 chars) to -50848XXXXX
16:40:58 WARN  [Telegram] Send failed: flood_control:35.0 — trying plain-text fallback
16:40:58 ERROR [Telegram] Fallback send also failed: flood_control:35.0

Both turns completed and burned their tokens; the user received nothing and no notice. From their side the agent looked hung, which is a rough failure mode — they were (understandably) annoyed that it "got stuck" twice.

Gap 1 — retry_after-presence gate misses rate limits that carry no delay

Gating on result.retry_after is not None fixes Telegram, because _flood_cap_result always populates it. But not every adapter parses a delay out of the rejection. Weixin raises its cooldown as a bare RuntimeError (gateway/platforms/weixin.py:1830), and send() wraps it at line 1978 as:

return SendResult(success=False, error=str(exc))

No retry_after, no retryable — but error is "iLink sendmessage rate limited; cooldown active for 42.0s", and classify_send_error() already returns "rate_limited" for it. Checked against this PR's gate:

Weixin-style rate-limit SendResult:
  retry_after-presence gate  -> False   (falls through to plain-text fallback)
  classification-based gate  -> True    (routes into retry)

So the amplification this PR fixes for Telegram still happens on Weixin. Gating on classify_send_error(...) == "rate_limited" instead of (or in addition to) retry_after is not None covers both, and reuses the classifier already in the file rather than adding a second notion of "is this a rate limit".

Gap 2 — the plain-text fallback is still reachable, and it truncates

With this diff, an over-cap flood routes into retry — good. But once max_retries is exhausted the code still falls through to the fallback, which re-enters the same ban. Worse, that path is lossy even on success:

content=f"(Response formatting failed, plain text:)\n\n{content[:3500]}"

A rate limit is not a formatting failure, so plain text cannot help by construction, and the 3500-char clamp means a long answer gets silently clipped. In my repro both payloads (3692 and 2647 chars) would have been affected. Returning the typed failure instead lets the delivery ledger own redelivery — which is what flood_control failing closed was for.

Suggested increment on top of this PR

@staticmethod
def _is_rate_limited_error(error: Optional[str]) -> bool:
    return classify_send_error(None, error or "") == "rate_limited"

Used in three places: the first-send gate, the retry-loop continuation gate, and an early return before the plain-text fallback when the failure is a rate limit.

Validation

Ran locally against main (b20cc5f) with 4 added regression tests — full-content retry, server-delay honoring, no-fallback-on-rate-limit, and a pin that genuine formatting errors still use the fallback:

tests/gateway/test_send_retry_rate_limited.py .... 4 passed
tests/gateway/ ................................ 7334 passed, 44 skipped, 2 xfailed

(7 unrelated pre-existing failures in test_discord_send.py, test_session_store_prune.py, test_teams_dotenv_isolation.py, test_telegram_polling_health_confirmation.py — they reproduce with the change stashed; test-ordering pollution around HERMES_HOME, not caused here.)

Happy to open a follow-up PR with the classifier-based gate + fallback skip if you'd prefer to land this one first and layer that on top — or fold it into this PR if @cdepuy would rather keep it as one change. Either way this diff is the right fix for the Telegram path; I'd just widen the gate before closing the bug class out.

[cdepuy]

Friendly nudge on this one — both of these landed the same day from independent deployments, and the real-world impact is aggravating: a single flood-capped send currently falls through to the plain-text fallback, re-enters the server ban, and silently eats the turn (user sees a reply that never arrives). It recurs repeatedly.

marian001 already confirmed the root-cause analysis independently, and CI is green on the rebased head (f58bbddb89). This fixes the whole bug class: honors server retry_after, treats flood/rate-limit as transient even when the platform omits retry_after (the Weixin case), and never truncates via the plain-text fallback. Includes 2 new regression tests.

@EmpireOperating @Teknium — review when you get a chance; happy to rebase if anything's drifted since.

[cdepuy]

@marian001 your two gaps are already folded into the current head (f58bbddb89, pushed right after your comment) — wanted to confirm point-by-point so reviewers see both concerns are closed:

Gap 1 (Weixin / no-retry_after rate limits): the gate now checks classify_send_error(...) == "rate_limited" in addition to retry_after is not None, in three places — the first-send gate, the retry-loop continuation gate, and the pre-fallback guard. So a Weixin-style SendResult(success=False, error="...rate limited...") with no retry_after/retryable now routes into the retry path instead of the plain-text fallback. This is exactly the classifier-based gate you suggested.

Gap 2 (fallback reachable + truncating): added an early return before the plain-text fallback when the failure classifies as rate_limited — returns the typed failure so the delivery ledger owns redelivery, instead of re-entering the ban or clipping at 3500 chars. Genuine formatting errors still use the fallback.

Current head includes 2 regression tests (rate-limited-without-retry_after retries & succeeds; rate-limited exhaustion returns a typed failure, never the truncating fallback). Your 4-test suite in test_send_retry_rate_limited.py is a good complementary pin — if you want to add it here rather than a follow-up PR, happy to have you push it onto the branch, or I can drop it in.

Thanks again for the independent confirmation and for widening the gate — much appreciated.

[marian001]

@cdepuy Confirmed — pulled f58bbddb89 into a worktree and ran it. Both gaps are closed exactly as described, and the classifier-based gate is the right shape. Thanks for folding it in that fast.

One residual issue on the current head, though: is_rate_limited is computed once from the first send's error and then reused inside the retry loop's continuation gate. It goes stale after attempt 1, and it breaks in both directions.

1. Flood, then a genuine formatting error → fallback becomes unreachable

The flag stays True, so the gate never breaks out. The loop retries a permanent can't parse entities until exhaustion and the plain-text fallback — the one path that would actually fix a formatting error — is never reached:

attempt 1: flood_control:35.0   -> retry  (correct)
attempt 2: can't parse entities -> retry  (should have broken out)
result: 3 identical sends + delivery-failure notice, no fallback attempted

2. Network error, then a rate limit → retry budget silently abandoned

This is the Weixin case the PR set out to cover, just arriving on a later attempt. is_rate_limited was computed on ConnectionResetError, so it is False; when the retry comes back rate-limited with no retry_after/retryable, the gate fails and the loop gives up early:

max_retries=3, 3rd attempt would have succeeded
f58bbddb89:                        2 attempts, success=False
+ per-attempt reclassification:    3 attempts, success=True

The pre-fallback guard does still save it from the truncating fallback, so nothing is clipped — but the send fails when it did not have to.

Fix — one line, where error_str is already refreshed

error_str = result.error or ""
if result.retry_after is not None:
    server_retry_after = result.retry_after
# Re-classify per attempt: the failure kind can change between attempts, and a
# stale flag from the *first* send would either pin a now-permanent error in
# the retry loop or drop a newly-arrived rate limit out of it.
is_rate_limited = classify_send_error(None, error_str) == "rate_limited"
if not (
    result.retryable
    or is_rate_limited
    or result.retry_after is not None
    or self._is_retryable_error(error_str)
):
    break

Validation

Two probes for the cases above, run against pristine f58bbddb89 and then against f58bbddb89 + the line:

pristine head:        2 failed
head + reclassify:   36 passed
  (test_send_retry.py, test_send_error_classification.py,
   test_telegram_send_path_health.py, + both probes)

Full tests/gateway/ on the patched head: 7387 passed, 44 skipped, 2 xfailed. 8 failures, 7 of which are the pre-existing HERMES_HOME test-ordering pollution (test_discord_send.py, test_send_multiple_images.py, test_session_store_prune.py, test_teams_dotenv_isolation.py, test_telegram_polling_health_confirmation.py) — they reproduce with the change stashed and pass when those files are run in isolation. The 8th is my own test calling a _is_rate_limited_error helper that does not exist on your head, which brings me to:

On the test suite

Happy to push it onto your branch. One note first — my version factors the classifier call into a _is_rate_limited_error(error) static helper and one test asserts against it directly. Your head calls classify_send_error inline in three places, so either:

• (a) add the helper and have the three sites call it, or
• (b) drop that one unit test and keep the three behavioral ones.

Mild preference for (a): three inline copies of the same classification is exactly the kind of thing that drifts, and it makes the per-attempt reclassification above read as one call rather than a fourth copy.

Say which you'd rather have and I'll push accordingly — or I can open it as a follow-up PR against your branch if that's easier to review.

[EmpireOperating]

Review feedback — change needed before merge

The rate-limit classification is calculated from the initial send result and then reused throughout the retry loop. That value can become stale when the failure kind changes between attempts.

Please recalculate is_rate_limited from the refreshed error_str after each retry result, before deciding whether to continue the loop.

Please add regression coverage for both transitions:

1. rate_limited → formatting error: stop retrying the permanent formatting error and reach the existing plain-text fallback.
2. network/transient error → rate_limited → success: preserve the retry budget when a later attempt becomes rate-limited.

The existing tests cover initial rate limiting and exhausted retries, but not these failure-type transitions. Once the per-attempt classification and those tests are in place, the overall approach looks right.

[cdepuy]

Addressed the review feedback (per-attempt reclassification + both failure-type transition tests). Pushed as 2fe426f54e on fix/telegram-flood-retry-after.

Code change (gateway/platforms/base.py): is_rate_limited is now recomputed from the refreshed error_str after every retry result, before the loop's continue/break decision — no longer reused from the initial send's classification.

Two regression tests (in tests/gateway/test_send_retry.py, TestSendWithRetryFailureTypeTransitions):

1. rate_limited → formatting error: verified it stops retrying the permanent can't parse entities and reaches the existing plain-text fallback.
2. network/transient → rate_limited → success: verified a later rate-limited attempt is treated as transient, preserving the retry budget so the send recovers.

Both new tests FAIL against the prior head (f58bbddb89) and pass with the change — confirmed RED→GREEN locally. Full send-retry / error-classification / Telegram send-path suite: 183 passed, no regressions.

Noting that GitHub's head-ref cache was still resolving the PR to f58bbddb89 for several minutes after the fast-forward push, though the fork branch ref is confirmed at 2fe426f54e — flagging in case the PR page shows the pre-change head; a refresh / re-sync should pick it up.

[gertsio]

Verified on fd59a4b1. The retry loop's else branch still sends the failure notice during the latest flood penalty.

With three SendResult(success=False, error="flood_control:189.0", retry_after=189.0) results, max_retries=2, a fake clock and zero jitter, send times are [0, 189, 378, 378]. The fourth send is the notice, immediately after another 189-second refusal.

Return the failure before that notice:

if self._is_rate_limited_error(error_str) or result.retry_after is not None:
    return result

Update test_rate_limited_exhausted_returns_typed_failure_not_fallback to expect three sends; it currently requires the fourth. No extra timer or retry is needed.

Validation through scripts/run_tests.sh: added regression failed, 22 existing tests passed. With this guard, the regression and three network-notice/failure-transition controls passed. Full suite not run.

[cdepuy]

@gertsio — addressed. Thank you for the precise repro; it was spot on.

The exhaustion-else branch now returns the typed failure BEFORE attempting the delivery-failure notice whenever the final failure is a rate limit or still carries a server retry_after. So the notice is never sent inside the active flood penalty, and no extra timer or request is added:

if self._is_rate_limited_error(error_str) or result.retry_after is not None:
    logger.error(... "returning typed failure for redelivery (no notice sent inside active flood penalty)")
    return result

Your exact scenario (three flood_control:189.0 results, max_retries=2, fake clock, zero jitter) now yields send times [0.0, 189.0, 378.0] — three sends, no fourth inside the penalty. Verified locally with the run_tests harness against current main base plus these changes.

test_rate_limited_exhausted_returns_typed_failure_not_fallback was updated as you requested to expect three sends (initial + 2 retries) and to assert no notice/fallback content. The two failure-type transition tests you confirmed are present and passing, and they now also cover the "rate-limited final result returns before the notice" path.

CI on this fork PR requires maintainer approval to run (action_required); I've also rebased onto current main so the branch is MERGEABLE (was CONFLICTING). Net targeted test counts (Python 3.11, current main base):

• tests/gateway/test_send_retry.py — 22 passed (incl. your regression scenario + the corrected exhaustion test)
• tests/gateway/test_send_error_classification.py + test_slack_send_retry.py — 19 passed

@EmpireOperating @Teknium — friendly ping: CI needs a maintainer to approve the run on this fork PR; the branch is mergeable. Happy to rebase again if anything has drifted since ebfdea0e19.

[kshitijk4poor]

Merged via #104370 as a cherry-pick of your commit (authorship preserved) — main 9e4135fad8.

What landed: your classifier-based rate-limit handling, per-attempt reclassification (@EmpireOperating's ask), and the typed failure before any delivery-failure notice (@gertsio's ask), no truncating plain-text fallback for a rate-limited send. Live probe with a stub adapter returning flood_control:5820: main attempted the plain-text fallback; the branch made one send, slept 0s, and returned the typed failure.

Two follow-ups of mine on top: (1) the inline retry_after sleep is capped at 60s — honouring a 97-minute FloodWait verbatim would pin the send coroutine (the #91969 class, which is why the Telegram adapter already fails closed at 5s and defers to this loop); past the cap the typed failure returns immediately and the delivery ledger owns redelivery. (2) The post-loop rate-limit guard was unreachable (rate-limited errors classify as network and the loop only breaks on non-transient, non-rate-limited errors), so it was removed; tests trimmed to the contracts.

Thanks, and to @marian001 for the second-deployment repro. #103754 touches the same send path and will need a rebase onto this. Closing in favour of the merged salvage.