Skip to content

fix: avoid Telegram overflow duplicate retries - #50965

Open
dupermariobro wants to merge 1 commit into
NousResearch:mainfrom
dupermariobro:fix/telegram-overflow-flood-duplicates
Open

fix: avoid Telegram overflow duplicate retries#50965
dupermariobro wants to merge 1 commit into
NousResearch:mainfrom
dupermariobro:fix/telegram-overflow-flood-duplicates

Conversation

@dupermariobro

Copy link
Copy Markdown

Summary

Fixes a Telegram long-response duplicate/flood-control path I reproduced from a live gateway log.

When a streamed Telegram response exceeds the message limit, edit_message() splits it into the edited preview plus continuation messages. If Telegram returns RetryAfter while sending an overflow continuation, the adapter previously treated the Markdown send as a formatting failure and immediately retried the same chunk as plain text. The gateway then fell back to sending the full final response, which can duplicate already-visible chunks and amplify flood-control failures.

This PR:

  • Adds per-chat Telegram flood-control cooldown tracking.
  • Parses RetryAfter consistently from Telegram exceptions and error text.
  • Stops immediate Markdown→plain retry for overflow continuations when the failure is flood control.
  • Makes send() honor an active chat cooldown instead of attempting another full-response fallback during the cooldown window.
  • Adds regression coverage that a RetryAfter on an overflow continuation only attempts one send and returns partial-overflow metadata.

Reproduction evidence

Observed from a live Telegram gateway log while handling a 13,061-character response:

Overflow continuation hit Telegram flood control; deferring for 249.0s: Flood control exceeded. Retry in 249 seconds
Overflow split: stopped at 1/3 chunks delivered
response ready: platform=telegram ... response=13061 chars
[Telegram] Sending response (13061 chars) ...
[Telegram] Send failed: flood_control:237.2 — trying plain-text fallback
[Telegram] Fallback send also failed: flood_control:237.2

That sequence shows a partial overflow delivery followed by a normal full-response send attempt for the same final response.

Tests

Passed:

python -m pytest tests/gateway/test_telegram_overflow_partial.py -q -o 'addopts='
# 5 passed in 0.25s

python -m pytest tests/gateway/test_telegram_overflow_partial.py::test_edit_overflow_split_does_not_plain_retry_after_flood_control -q -o 'addopts='
# 1 passed in 0.16s

Also ran adjacent Telegram tests:

python -m pytest tests/gateway/test_telegram_overflow_partial.py tests/gateway/test_telegram_thread_fallback.py tests/gateway/test_telegram_format.py -q -o 'addopts='

Result: 152 passed, 3 failed. The 3 failures were in test_telegram_thread_fallback.py asserting mocked group chat type resolution ('dm' == 'group') and are unrelated to this change; this PR only touches plugins/platforms/telegram/adapter.py and tests/gateway/test_telegram_overflow_partial.py.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter P2 Medium — degraded but workaround exists labels Jun 22, 2026
@Tosko4

Tosko4 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Thanks for picking this up — I pulled this PR onto current origin/main and the focused regression path applies cleanly.

I verified:

python -m pytest tests/gateway/test_telegram_overflow_partial.py -q -o 'addopts='
# 5 passed

python -m pytest tests/gateway/test_telegram_overflow_partial.py tests/gateway/test_telegram_format.py tests/gateway/test_stream_consumer.py -q -o 'addopts='
# 206 passed

python -m pytest tests/gateway/test_telegram_thread_fallback.py -q -o 'addopts='
# 46 passed

The core diagnosis looks right to me: a Telegram RetryAfter during overflow continuation should not be treated like a Markdown formatting failure, and it should not immediately fan out into plain-text retry + full final fallback attempts.

A couple of merge-readiness concerns though:

  1. New non-secret HERMES_* env var

    This adds HERMES_TELEGRAM_MAX_INLINE_FLOOD_WAIT_SECONDS for behavioral tuning. AGENTS.md / the contribution guide explicitly call out that non-secret behavior settings should not be new HERMES_* env vars; they should either live in config.yaml or remain an internal constant. Since this is not a credential, I think this should be changed before merge.

    For this PR, I’d lean toward an internal constant unless there’s a strong reason users should tune it.

  2. The common short RetryAfter path may still not deliver the tail synchronously

    The live failure shape I’m looking at is a short Telegram wait, e.g. Flood control exceeded. Retry in 9 seconds, on an overflow continuation. With the current default max_inline=5, that path records a cooldown and returns partial-overflow immediately. The stream consumer then enters fallback, but send() also short-circuits while the chat cooldown is active, so the immediate tail/full fallback attempts can still fail during the same cooldown window.

    That avoids duplication, which is good, but it risks turning a recoverable 9s Telegram wait into “first chunk visible, tail not delivered until some later recovery path / user action”. For a final assistant response, waiting ~9–10 seconds and retrying the same continuation once seems preferable to dropping into cooldown/fallback.

    I’d suggest adding a regression that covers this exact shape:

    • first overflow continuation raises RetryAfter(9);
    • asyncio.sleep is monkeypatched so the test doesn’t actually wait;
    • the adapter retries the same continuation once after the wait;
    • if the retry succeeds, _edit_overflow_split() returns full success and the stream consumer suppresses normal final send normally;
    • no Markdown→plain retry happens before respecting the flood wait.

    For longer waits (e.g. hundreds of seconds), deferring is reasonable, but the short-wait finalization path should probably preserve full delivery rather than relying on fallback while the cooldown is intentionally active.

  3. Use the existing typed error taxonomy where possible

    Current main already has SendResult.error_kind and classify_send_error() with rate_limited. The new flood-control result currently returns only error="flood_control:...". Setting error_kind="rate_limited" would make downstream handling less substring-dependent and align with the newer gateway send-error contracts.

So: I think the PR is directionally right and the tests are useful, but I’d tighten these before merge: remove the new env-var knob, cover the short RetryAfter retry path, and set error_kind="rate_limited" on synthetic flood-control results.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused reproduction and regression test. The underlying retry classification remains relevant on current main: plugins/platforms/telegram/adapter.py:4269-4299 still advances a failed MarkdownV2 overflow continuation to a plain-text attempt without a RetryAfter-specific branch.

Problems

  • The added cooldown returns a synthetic flood result without retry_after or error_kind="rate_limited". Current fallback retry selection consumes retry_after in gateway/stream_consumer.py:1237-1251, so it cannot honor the server delay.
  • The overflow branch defers every RetryAfter; for a short wait the tail then reaches the same cooldown-gated send() path. The added test proves only that the first send is not duplicated, not that the final tail is delivered.
  • HERMES_TELEGRAM_MAX_INLINE_FLOOD_WAIT_SECONDS conflicts with the non-secret configuration rule in AGENTS.md:102-105.

Suggested changes

  • Salvage this at the continuation handler on current main: retry a bounded RetryAfter once before the Markdown-to-plain fallback, and return retry_after plus error_kind="rate_limited" when deferring.
  • Keep the threshold internal unless a config.yaml setting is justified, and add the short-RetryAfter successful-delivery regression.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants