fix(gateway): do not resend a message the platform rate-limited - #79043
Open
s905060 wants to merge 1 commit into
Open
fix(gateway): do not resend a message the platform rate-limited#79043s905060 wants to merge 1 commit into
s905060 wants to merge 1 commit into
Conversation
A 429 with a server-requested retry_after is not proof of non-delivery.
Telegram commonly ACCEPTS the message and rate-limits the acknowledgement,
so _send_with_retry re-sending the whole payload put a second full copy in
the chat.
Observed in a live gateway log, one user turn:
sendRichMessage transient failure (no legacy resend): Flood control
exceeded. Retry in 13 seconds
Send failed (attempt 1/2, retrying in 13.7s)
Send succeeded on retry 1
One 1700-char response, two identical messages delivered.
The Telegram adapter already refuses its own internal legacy-resend on this
exact error for this exact reason, then returned retryable=True and handed
the same duplicate risk to the shared retry layer. Treat retry_after as
delivery-ambiguous the way a timeout already is, in both the pre-loop and
in-loop checks, so the one copy the platform holds stays the only one.
This makes the first-send server_retry_after pacing branch unreachable, so
it is removed rather than left as dead code.
Fixed in the shared base adapter, not in Telegram: every platform reporting
retry_after routes through here.
Contributor
|
This was generated by AI during triage. A defect in the change: The guard keyed on Problems:
Suggested changes:
Checked against |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
429with a server-requestedretry_afteris not proof of non-delivery. Telegram commonly ACCEPTS the message and rate-limits the acknowledgement, so_send_with_retryre-sending the whole payload puts a second full copy in the chat.Observed in a live gateway log, one user turn:
One
Sending response (1700 chars), two identical messages delivered to the chat.Cause
The Telegram adapter already refuses its own internal legacy-resend on this exact error, for this exact reason:
It then returns
retryable=Truewith the parsedretry_after, which hands the same duplicate risk to the shared retry layer ingateway/platforms/base.py— which re-sends the whole message. The adapter closed the door and the base layer opened it again.Change
Treat
retry_afteras delivery-ambiguous, the way a timeout already is._send_with_retryreturns the failure instead of resending, in both the pre-loop check and the in-loop check (a 429 can arrive on a later attempt after a genuineConnectError).That makes the first-send
server_retry_afterpacing branch unreachable, so it is removed rather than left as dead code.Fixed in the shared base adapter rather than in Telegram: every platform that reports
retry_afterroutes through this one path.Ordinary transient errors (
ConnectErrorand friends) retry exactly as before — the guard is scoped to a server-requested backoff.Tests
TestSendWithRetryAfterpreviously asserted thatretry_aftermerely paced a retry, which is the bug encoded as a contract, so those two cases are rewritten to assert send-count instead:test_retry_after_is_not_resent— 429 sends exactly oncetest_retry_after_on_a_later_attempt_stops_resending—ConnectErrorstill retries, the 429 on that retry stops it (no third copy)test_plain_network_error_still_retries— new, pins that the guard did not disable ordinary retriesBoth rewritten tests were confirmed to fail on unpatched code before the fix was trusted, reproducing the duplicate (
assert not result.success/AssertionError: assert not True).The suite also drops 32.13s → 1.16s, because the old tests were really sleeping through the flood waits they asserted on.
Not run: the full
tests/gatewaysuite is red at baseline in this environment (213 failed / 1389 errors on an unmodified checkout), so its totals carry no signal. Threetest_telegram_thread_fallbackcases fail when the_send_with_retryfiles are run as one batch — they fail identically on unpatched code and pass in isolation, so that is a pre-existing cross-test state leak, not this change.