fix(telegram): prevent draft-failure cascade that causes 4-minute flood-control silence

[elphamale]

Root cause

A single failed sendMessageDraft call permanently disabled draft streaming for the rest of the session, forcing every subsequent token through editMessageText. On a long response (180 s+, multiple tool calls) this produced 200+ edits, exhausted Telegram per-chat edit quota (RetryAfter ~280 s), and left the user staring at a stuck cursor in a frozen partial message for 4+ minutes.

Five targeted fixes

1. Draft-frame failure tolerance (stream_consumer.py)

Introduce _MAX_DRAFT_FAILURES = 3. Draft streaming is now only disabled after 3 consecutive failed frames. A single transient error no longer cascades into 200+ editMessageText calls. Consecutive successes reset the streak.

2. Suppress non-retryable draft failures (adapter.py)

Non-flood, non-MarkdownV2 sendMessageDraft failures now return SendResult(success=True, message_id=None). The consumer treats this as a silently dropped frame, stays in draft mode, never sets _message_id, and never switches to the editMessageText path. The final message arrives via the base send() call as normal.

Short RetryAfter (<=5 s): sleep + inline retry; if retry also fails, suppress the frame.
Long RetryAfter (>5 s): return retryable=True (and retry_after=) so the consumer skips counting it against the threshold and can cool down properly (see fix 5).

3. Cursor-strip: leave partial intact on failure

When the cursor-strip edit is flood-controlled or raises, leave the message as-is. A partial reply with a stuck cursor is better UX than deleting it and leaving the user with a blank screen for the flood-control wait.

4. Edit stale partial in-place at fallback

When fallback fires and the complete final text fits in one message, attempt editMessageText on the stale partial first. If the edit succeeds: no new message, no deletion, message keeps its position. If it fails, fall through to the original send+delete path.

5. Long draft-flood waits no longer cascade to a real send/edit or a duplicate call

szafranski's review (echoed independently by hermes-sweeper) identified two remaining edge cases where a long RetryAfter still triggers the same class of flood-control cascade this PR set out to fix:

• _send_draft_frame's retryable branch correctly skipped counting the failure, but _send_or_edit treated any False return as "drafts already gave up, fall through to adapter.send()/edit_message()" — so a long flood-control wait immediately spent another call (a real message send) in the same window instead of waiting it out.
• TelegramAdapter._try_send_rich_draft collapsed every exception, including RetryAfter, to a bare False, so send_draft() always fell through to the legacy sendMessageDraft endpoint for the same frame — burning a second call in the same flood-control bucket.

Fixed:

• _try_send_rich_draft now returns (success, retry_after); a RetryAfter propagates as SendResult(retryable=True, retry_after=...) directly, without touching the legacy endpoint. The legacy path's own long-wait branch now also sets retry_after (previously only embedded in the error string, which the consumer never parsed).
• _send_draft_frame stores that retry_after as a cooldown deadline (_draft_cooldown_until) and skips the send_draft call outright while still cooling down, rather than retrying every tick.
• _send_or_edit only falls through to the real send/edit path when the miss was NOT a retryable cooldown (_last_draft_retryable) — ordinary (non-retryable) draft misses still fall through immediately exactly as before, so the existing get-a-real-message-fast behavior for genuine rejections is unchanged.

Tests (51 across 3 files, 8 new)

TestDraftFallbackOnFailure: single_failure_does_not_disable / three_consecutive_failures_disable / success_resets_failure_streak / retryable_failure_not_counted / long_retryable_failure_does_not_fall_through_to_send_or_edit / retryable_cooldown_skips_subsequent_send_draft_calls / expired_cooldown_resumes_calling_send_draft / draft_failure_falls_back_and_delivers_final_via_send

TestDraftSuppressionPreventsEditCascade: suppressed_drafts_never_call_edit_message / suppressed_drafts_do_not_accumulate_failures

TestTryStripCursor: successful_edit_skips_delete / flood_controlled_edit_leaves_message_intact / raised_edit_exception_leaves_message_intact / no_message_id_is_a_noop / no_edit_sentinel_is_a_noop

TestSendFallbackFinalEditInPlace: edit_in_place_skips_send_and_delete / failed_edit_falls_through_to_send_and_delete / raised_edit_falls_through_to_send / no_stale_message_id_goes_straight_to_send / no_edit_sentinel_goes_straight_to_send / preserve_partial_skips_edit

test_telegram_send_draft_format.py: passes_markdownv2_parse_mode / falls_back_to_plain_text_on_markdownv2_error / non_badrequest_is_suppressed

test_telegram_send_draft_flood_control.py: short_flood_control_sleeps_and_retries_successfully / short_flood_control_retry_failure_is_suppressed / long_flood_control_returns_retryable_without_sleeping (now also asserts retry_after) / boundary_five_seconds_treated_as_short / boundary_just_above_five_seconds_treated_as_long (now also asserts retry_after)

test_telegram_rich_messages.py: rich_draft_flood_control_propagates_retry_after_without_legacy_fallback (new)

test_stream_consumer.py::TestSegmentBreakOnToolBoundary: edits_stale_partial_in_place_when_it_fits (new) — plus the two pre-existing #10807/#16668 tests updated to accept delivery via edit_message() as well as send() (see rebase note below)

Note: a 4th commit (edit-in-place + test cleanup) was added after the initial approval, a 5th commit addressed szafranski's post-approval review and hermes-sweeper's first two review points, and this branch has now been rebased onto current origin/main (it was 2372 commits behind) with a 6th commit reconciling the result — addressing hermes-sweeper's third point ("the fallback-final implementation has moved materially since this branch... reconcile with current main before salvage").

The rebase itself cleanly resolved a textual conflict in _try_strip_cursor: origin/main's 04898631c/4aa499ff9 (the two commits sweeper named) had independently converged on the same "leave message intact on failure" behavior this PR's first commit introduces, so no design conflict there. It did surface one real interaction: this PR's own edit-in-place commit changed how _send_fallback_final delivers content (edit vs. send+delete) for the single-message case, which three pre-existing TestSegmentBreakOnToolBoundary regression tests hadn't been updated to account for — they asserted delivery specifically via send()/delete_message(). Fixed by updating those tests to accept either delivery mechanism (same outcome: correct content visible, no stale/duplicate message) and adding a dedicated test that forces the edit to fail so the send+delete fallback still gets direct coverage. Confirmed all 252 tests across the affected files pass, and separately confirmed the 9 pre-existing telegram test-order-pollution failures in the wider suite are identical on a clean origin/main checkout with none of this branch's commits — unrelated to this PR.

Generated with Claude Code

[tonydwb]

Code Review Summary

Verdict: Approved

Telegram draft-failure cascade prevention (5 files). Clean resilience fix:

• New _MAX_DRAFT_FAILURES = 3 threshold (was: 1 failure permanently killed drafts)
• Consecutive failures trigger edit-based fallback; successful frames reset the counter
• Good change from "permanent disable" to "consecutive threshold" — single transient errors no longer cascade
• Updated docstrings accurately describe the new behavior

The consecutive-successes-reset pattern is the right fix for flood-control hiccups. No concerns.

[elphamale]

@tonydwb Thanks for the review! But I added another commit (1ef5861) after your approval. It touches only _send_fallback_final in stream_consumer.py and the draft test file (3 test removals + 6 new tests for the new path). The first 3 commits are unchanged. I am still not satisfied with bot's behavior with long, markdown/rich-formatted replies and thinking about how to improve it. So I am open to suggestions, but this PR is finalized so far.

[szafranski]

I compared this with a live sendMessageDraft RetryAfter repro. The PR looks
pointed in the right direction, but I think two edge cases still need coverage.

1. Long draft flood can still fall through to regular send/edit.

For RetryAfter > 5s, TelegramAdapter.send_draft() returns
SendResult(success=False, retryable=True). _send_draft_frame() then returns
False without disabling draft streaming or counting a failure, but
_send_or_edit() treats any False from _send_draft_frame() as "fall through
to the regular edit/send path below".

So the long-flood path can still create a real message or enter edit streaming
for the same frame. That looks like the same cascade, just through the
retryable=True branch. The current tests check _send_draft_frame() directly,
but not the _send_or_edit() behavior.

A targeted regression test would be:

• adapter send_draft() returns SendResult(success=False, retryable=True, retry_after=280) or equivalent;
• call _send_or_edit() mid-stream with draft streaming enabled;
• assert adapter.send and adapter.edit_message are not called;
• assert draft streaming remains enabled and the frame is treated as handled or
  cooled down.

2. sendRichMessageDraft RetryAfter immediately falls back to legacy
   sendMessageDraft.

_try_send_rich_draft() still returns only bool, so any
sendRichMessageDraft exception returns False. send_draft() then
immediately spends a legacy sendMessageDraft call for the same frame.

That fallback is right for capability failures, but for a Bot API RetryAfter
it likely burns another call in the same draft/flood bucket. It would be safer
to propagate a structured rate-limit result from the rich draft path and let
the stream consumer cool down instead of trying the plain draft API immediately.

A targeted regression test would be:

• rich drafts enabled;
• do_api_request("sendRichMessageDraft", ...) raises an exception with
  retry_after;
• assert send_message_draft is not called for that same frame;
• assert the result preserves rate-limit metadata.

One design concern: returning success=True for most non-Markdown draft
exceptions prevents edit cascades, but it also masks hard/capability failures as
successful frames. A narrower contract would be: rate limits are handled or
cooled down; hard draft failures stay hard failures and can still use the
existing fallback path.

[teknium1]

Thanks for isolating the draft-failure cascade and adding focused coverage. The bug remains present on current main, but the proposed rate-limit contract is not carried through the full streaming path.

Problems

• A long draft flood still cascades: the PR returns success=False, retryable=True, _send_draft_frame() returns False, and gateway/stream_consumer.py:1693-1701 falls through to regular streaming; with no message id, gateway/stream_consumer.py:1922 calls adapter.send(). The direct _send_draft_frame() tests do not cover this caller behavior.
• Rich drafts have the same unhandled RetryAfter path. plugins/platforms/telegram/adapter.py:1812-1850 reduces all rich-draft exceptions to False, then send_draft() tries legacy send_message_draft at :4422-4428, creating another call in the same frame.
• The fallback-final implementation has moved materially since this branch: current main's 04898631c and 4aa499ff9 add bounded flood recovery and segment-scoped preview cleanup.

Suggested changes

• Make draft cooldown a handled outcome in _send_or_edit() and add an end-to-end long-RetryAfter test asserting no normal send/edit call.
• Propagate rich-draft RetryAfter separately from capability fallback, with a regression test.
• Reconcile the fallback-final work with current main before salvage.

Automated hermes-sweeper review.

[elphamale]

@szafranski Sorry for the silence on this — both edge cases you flagged were real and still present on main, and today's hermes-sweeper review re-raised the same two points independently. Pushed 64b8d8d76 fixing both:

Long draft flood still falling through to a real send/edit. _send_draft_frame's retryable branch correctly didn't count the failure, but _send_or_edit treated any False return as "drafts already gave up, fall through to adapter.send()/edit_message()" — so a long RetryAfter immediately spent another call (a real message) in the same flood-control window. Fixed: the retryable result now sets a cooldown deadline (_draft_cooldown_until) that skips further send_draft calls until it passes, and _send_or_edit only falls through when the miss wasn't a retryable cooldown — ordinary (non-retryable) draft misses still fall through immediately, unchanged from before, so the existing "get a real message fast on a genuine rejection" behavior is untouched.

sendRichMessageDraft RetryAfter immediately burning a legacy sendMessageDraft call. _try_send_rich_draft collapsed every exception (capability, transient, RetryAfter alike) to a bare False, so send_draft() always fell through to the legacy endpoint for the same frame. It now returns (success, retry_after); a RetryAfter propagates straight out as SendResult(retryable=True, retry_after=...) without touching the legacy path at all.

Both regression tests you described are in there (test_long_retryable_failure_does_not_fall_through_to_send_or_edit, test_rich_draft_flood_control_propagates_retry_after_without_legacy_fallback), plus cooldown tests confirming send_draft isn't re-invoked while cooling down and resumes once the deadline passes.

@tonydwb — the original approval predates szafranski's review; flagging so it isn't mistaken for current sign-off on this branch.

[elphamale]

@teknium1 (hermes-sweeper) — following up on your third point separately: "the fallback-final implementation has moved materially since this branch... reconcile with current main before salvage."

You were right, and I'd missed it in my first reply. This branch was 2372 commits behind origin/main. Rebased it just now (pushed as 138dc6593):

• The textual conflict landed exactly where you'd expect — _try_strip_cursor, touched by both this PR's first commit and your named 04898631c/4aa499ff9. Turned out to be a non-issue: origin/main had already independently converged on the same "leave the message intact on failure" behavior this PR introduces, just via a different commit. No design conflict, just a mechanical rebase resolution.
• The rebase did surface one real thing worth flagging: this PR's own "edit stale partial in-place" commit (added after the initial approval) changes how _send_fallback_final delivers content for the single-message case — editing the stale partial instead of send-then-delete. Three pre-existing TestSegmentBreakOnToolBoundary regression tests ([Bug]: 【高优 BUG】execute_code 超 30 秒后静默丢失，无返回、无日志、无回执 #10807/Telegram streaming flood control can leave partial message and send duplicate final response #16668) asserted delivery specifically via send()/delete_message() and hadn't been updated for that. Fixed by accepting either delivery mechanism in those tests (same outcome — correct content visible, no stale/duplicate message) and adding a dedicated test that forces the edit to fail so the send+delete fallback path still gets direct coverage.

Confirmed 252 tests pass across the affected files, and separately confirmed the 9 pre-existing telegram test-order-pollution failures elsewhere in the suite are identical on a clean origin/main checkout with none of this branch's commits — unrelated to this PR, not something the rebase introduced or should chase down here.

[elphamale]

Pushed 796bbd6f1 addressing a follow-up concern that surfaced while closing out #54337 (a stacked PR whose own edge-case fixes turned out to already be covered here — closed as superseded): send_draft()'s legacy sendMessageDraft path was returning success=True for every non-flood-control failure (ok=False, expired typing action, unsupported chat, network hiccup, and the post-retry failure case), not just flood control.

That's broader suppression than the cascade-prevention goal actually needs, and it had a real side effect: it silently defeated _MAX_DRAFT_FAILURES for the legacy draft path — a genuine/permanent failure (bot blocked, chat gone, bad draft_id) never counted toward the 3-strikes fallback to edit-based delivery, since every failure was reported as success regardless of cause.

Narrowed suppression to flood control only (retryable=True + retry_after) — the one case that actually needs special handling to avoid burning another call in the same rate-limit window. Every other failure now returns a normal success=False miss, so it correctly participates in the existing consecutive-failure fallback like any other adapter failure. Also fixed the same gap in the post-sleep retry branch: if that retry itself hits flood control again, it now propagates as retryable with the new retry_after instead of being flattened into either a blanket success or an uncounted miss.

Updated/added tests: test_send_draft_non_badrequest_is_a_normal_miss (renamed from ..._is_suppressed), test_short_flood_control_retry_hits_flood_again_is_retryable, test_short_flood_control_retry_hard_failure_is_a_normal_miss (renamed from ..._is_suppressed). Full draft/telegram test sweep: 1224 passed, 9 pre-existing failures (confirmed via git stash to exist on unmodified code — unrelated cross-file sys.modules["telegram"] mock pollution).