Repository navigation
fix(telegram): stop flood control from rewriting a finalized reply as plain text - #100446
AlexxRussell wants to merge 1 commit into
Conversation
A carefully reasoned and well-tested fix. The bug is real: the finalize-edit fallback caught bare Points worth attention:
Verdict: LGTM |
cca8195 to
e86cb5f
Compare
|
Rebased onto current main (2e24e06) and re-implemented against the refactored adapter: the finalize edit fallback now lives in the shared Same fix and the same deliberate narrowness as before. New in this revision:
Tests moved to |
e86cb5f to
0eeac51
Compare
|
Cosmetic follow-up (22dc0dd): wrapped the new comments and calls to 100 columns. Body corrected: the test file is tests/gateway/test_telegram_flood_keeps_markdown.py with 29 cases. |
|
Docs follow-up (6b1202e): fixed a self-contradiction in the test docstring ("could never run" then "even when it did run") and the fix-pinning note in the body (the test module imports helpers not on main, so against unmodified main it fails at collection). |
6b1202e to
0a7504f
Compare
|
Rebased onto current main ( Worth saying why it needed one. #104370 reworked
|
0a7504f to
90b839f
Compare
|
Rebased onto current main ( One conflict, in The branch had the flood log before the inline-cap check: if _looks_like_flood_error(e):
wait = _flood_wait_seconds(e)
logger.warning("[%s] Telegram flood control, waiting %.1fs", self.name, wait)
if wait > _FLOOD_INLINE_WAIT_CAP_SECS:
return _flood_cap_result(wait)That ordering came from this branch's base, not from this change. Main has since if _looks_like_flood_error(e):
wait = _flood_wait_seconds(e)The contribution is unchanged. A flood refusal is re-raised out of
One weakness I would rather flag than leave for you to find: the module imports |
… plain text The finalize edit helper _edit_markdown_or_plain caught bare Exception and answered every failure with a _strip_mdv2 plain-text rewrite. That rescue is right for a MarkdownV2 parse error, where the markup is what Telegram rejected. It also fired on flood control, which says nothing about the markup, so a reply whose MarkdownV2 was perfectly valid arrived with its syntax showing: headings as a literal ## and links as a literal [text](url). The flood branch in edit_message could never run while the helper swallowed the exception first, and when it did run its inline retry re-sent the raw content, which showed the same syntax. Fix, ported to the refactored adapter: - Shared helpers _looks_like_flood_error (retry_after attribute, or Telegram's own "Flood control exceeded. Retry in Ns" text, which the old "retry after" test did not match) and _flood_wait_seconds (float, or timedelta under PTB_TIMEDELTA=1, or the delay parsed from the message). Comparing a timedelta against the inline cap raised TypeError from inside the handler. - _edit_markdown_or_plain re-raises flood errors and, through an optional retry_state dict, records the payload in flight (the MarkdownV2 render, or the stripped text after a genuine parse fallback). - edit_message's flood branch uses the helpers and retries with that recorded payload instead of raw content. Because the retry now carries MarkdownV2 it can meet a parse rejection the refused first attempt never reached, so it keeps the same plain-text rescue and not-modified no-op the helper gives a first attempt; a second flood refusal fails closed. - _edit_overflow_split fails closed with the shared flood result on a flood-refused first chunk; _send_overflow_continuation reports the failure instead of resending the chunk unformatted. Deliberately narrow: parse errors, timeouts and "message to edit not found" keep the plain-text rescue. GatewayStreamConsumer does not consult retryable on an edit failure, so re-raising a timeout would send the tail again on top of an edit Telegram may already have applied. Tests: 29 in tests/gateway/test_telegram_flood_keeps_markdown.py, covering the over-cap and inline paths, timedelta and text-only refusals, the parse fallback that must survive (before and after a flood wait), the overflow first chunk and continuation, and the helper classification. Each guard was mutation-tested.
90b839f to
a7db31c
Compare
|
One more fix on this branch ( Upstream added a second-refusal block to
Those are the exact two gaps Neither is a regression from this branch: on its old base that path ended at a New test |
What does this PR do?
TelegramAdapter.edit_messagesends a finalized reply withparse_mode=MarkdownV2. When that call raises, an inner handler rewrites themessage as stripped plain text. That rescue is right for a MarkdownV2 parse
failure, where the markup is what Telegram rejected. It was applied to flood
control too, because the handler caught bare
Exception.A
RetryAfterrefusal says nothing about the markup, so a reply whoseMarkdownV2 was perfectly valid arrived with its syntax showing: headings as a
literal
##, links as a literal[text](url).Observed on a live gateway, twice in three days:
The outer handler in the same method was already written for exactly this, and
its own comment states the intent: short flood waits are retried inline, and an
over-cap wait "return[s] a failure immediately so streaming can fall back to a
normal final send instead of leaving a truncated partial". It rarely ran,
because the inner handler swallowed the flood exception first (the plain-text
rescue could itself be refused by flood control and reach the outer handler,
but only after the downgrade had already been attempted).
The scope is deliberately limited to flood control, which is explained under
Changes Made below.
Related Issue
No existing issue. Reproduction and log evidence are above.
Type of Change
Changes Made
plugins/platforms/telegram/adapter.pyTwo shared helpers.
_looks_like_flood_errorrecognises a refusal fromPTB's
retry_afteror from Telegram's ownFlood control exceeded. Retry in Ns, which the outer handler's"retry after"test never matched._flood_wait_secondsnormalises the delay, including reading it out of thatmessage when no attribute is present, so a text-matched refusal still fails
closed on a long wait instead of retrying blind inside the window.
The inner finalize fallback re-raises on flood control so it reaches the
outer handler. The not-modified shortcut is unchanged. The bounded retry
after a short flood wait keeps its own plain-text rescue for a parse
rejection the refused first attempt never reached, and does not wait on a
second flood refusal.
The same rule now applies inside
_edit_overflow_split, for both thefirst-chunk edit and the continuation sends. Those carried their own
catch-all downgrades, so any reply past the 4,096 unit cap kept the old
behaviour regardless of the guard above.
The inline flood retry re-sends the MarkdownV2 render rather than the raw
content. It previously re-edited with the unformatted source, which showed
the same raw syntax whenever a short flood wait hit a finalized reply. When
a genuine parse failure has already degraded the payload, the retry
re-sends that plain text instead, rather than reinstating markup Telegram
just rejected.
retry_afteris normalised before it is compared against the inline waitcap. python-telegram-bot exposes it as a
datetime.timedeltaunderPTB_TIMEDELTA=1, its supported migration mode for the 23.x default, and thecomparison raised
TypeErrorfrom inside the exception handler, soedit_messageescaped without retrying or returning aSendResult.Why only flood control. Re-raising a timeout would return
retryable=True, andGatewayStreamConsumerdoes not consultretryableonan edit failure: it sets
_fallback_final_sendand sends the missing tail. Atimeout can mean Telegram applied the edit and only the response was lost (the
send path notes the same at its own retry), in which case the tail would be
sent again on top of the complete answer. Losing formatting is the smaller
cost, so timeouts keep the existing rescue and
test_transient_network_error_keeps_the_plain_text_rescuepins that as adecision. Teaching the consumer to honour
retryableon edits looks like thereal fix and is a larger change than this one; happy to open it separately if
that is wanted.
tests/gateway/test_telegram_flood_keeps_markdown.py(new)29 cases (16 test functions, several parametrized) drive the real adapter
through a bot that replays a scripted failure sequence, and test the two
helpers directly:
retry_aftershapes, float andtimedelta[parse error, RetryAfter(1), success]keeps the degraded payload on theretry instead of reinstating the rejected markup
gets the plain-text rescue
retry_afterat all, recognised from the message,with the encoded 269s surviving into the capped result
asyncio.sleepcontrol fail closed instead of downgrading
How to Test
The test module imports
_looks_like_flood_errorand_flood_wait_seconds, thetwo helpers this PR adds, so against an unmodified
mainit fails at collection(the helpers do not exist there) rather than as individual assertion failures.
It is a fix-pinning suite: with the adapter change applied it is green, and each
behaviour it locks was mutation-checked. The captured payload that the
finalized-reply test asserts against is the bug verbatim (a finalized reply
rewritten as plain text under flood control):
Each mechanism was mutation-checked rather than assumed. Dropping the
"flood control"text clause, removing the message-parsed wait, reverting thetimedeltanormalisation, deleting the line that carries the degraded payloadinto the retry, removing the overflow guards and removing the retry's own
parse rescue each fail at least one test.
No regressions across the Telegram surface plus the streaming consumer:
Checklist
Code
Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys or N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdor N/A