fix(telegram): escalate persistent 409 Conflict to fatal via wall-clock gate - #64064
fix(telegram): escalate persistent 409 Conflict to fatal via wall-clock gate#64064xxiaoxiong wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
This PR escalates persistent 409 Conflict to fatal via wall-clock gate for Telegram. Small, targeted fix.
Please verify:
- The wall-clock gate properly detects persistent 409 conflicts
- Fatal escalation doesn't cause unintended disconnects on transient errors
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Changes
Telegram polling: wall-clock escalation gate (#63724). _polling_conflict_first_seen timestamp anchors the first conflict in a streak; if conflicts keep recurring past _CONFLICT_WALL_CLOCK_LIMIT_SECS, escalate to retryable-fatal.
Assessment
- Complements PR 64082 (same issue) with a time-based escalation gate. These are likely meant to work together.
- Well-documented with clear explanation of why a successful start_polling() does not prove the getUpdates session was released.
Reviewed by Hermes Agent
…ck gate (NousResearch#63724) When a competing long-poll holds the Telegram getUpdates session, `start_polling()` returning does NOT guarantee the conflict is resolved. The next `getUpdates` raises 409 again, the counter resets to 0 on each `start_polling()` "success", and the existing 5-attempt ladder never reaches MAX_CONFLICT_RETRIES — the bot stays silently deaf for days while the gateway reports healthy and only the missing replies reveal it. Root cause: `_polling_conflict_count = 0` at line 2568 fires on every `start_polling()` return, but the return only proves the updater started, not that this client won the Telegram server-side getUpdates session. The very next long-poll request raises 409, re-enters `_handle_polling_conflict` from attempt 1/5 again, and the ladder can never escalate. Fix: anchor an immutable `_polling_conflict_first_seen` timestamp on the FIRST conflict of a streak. If conflicts keep recurring past a 5-minute wall-clock limit (`_CONFLICT_WALL_CLOCK_LIMIT_SECS = 300.0`) regardless of intervening `start_polling()` "successes" that reset the attempt counter, the next 409 escalates directly to retryable-fatal so the supervisor can restart the gateway. The timestamp is cleared only on durable evidence of resolution — currently **never** in this patch (conservative). A future improvement could clear it on actual update consumption. Critical design note: `start_polling()` succeeds -> counter reset -> next conflict -> wall-clock first_seen already set 400s ago -> conflict_age (400s) >= limit (300s) -> `if` condition fails (no retry to schedule) -> falls through to fatal. This is exactly the fix. New regression test `test_polling_conflict_becomes_fatal_after_wall_clock_limit` mocks `time.monotonic()` to advance past the limit across two 409s where `start_polling()` "succeeds" in between — confirming the ladder would otherwise loop forever, and that the wall-clock gate correctly escalates. Existing conflict suite: 14 passed (13 existing + 1 new). No regressions in surrounding telegram tests (23 passed total). Closes NousResearch#63724.
86a61de to
ae18bf1
Compare
|
Thanks @tonydwb — confirming your two verification points:
One additional note flagged in the diff: this complements PR #64082 with a time-based ladder — they are designed to compose, not conflict. Diff unchanged from your review; happy to address any concerns. |
|
Thanks for the focused investigation and regression scenario. This is an automated hermes-sweeper review. Current
The PR's wall-clock mechanism is therefore superseded by the direct fix for the counter-reset root cause. |
Closes #63724.
Problem
A persistent Telegram
409 Conflictputs the adapter into an infiniteretry loop that never escalates to the fatal/restart path. The bot
silently stops receiving messages while the gateway reports itself
healthy. Reported real-world impact: a gateway stayed in this state for
4 days — every incoming DM queued in the Bot API and was never
handled.
root cause:
_polling_conflict_countis reset wheneverstart_polling()returns, butstart_polling()returning only provesthe updater object started — not that this client won the
server-side
getUpdatessession. If another long-poll still holds thesession, the very next
getUpdatesraises409 Conflictagain, theerror callback re-enters
_handle_polling_conflict, the counter startsfrom 1 once more, and the ladder can never reach
MAX_CONFLICT_RETRIES.The retryable-fatal path is unreachable in exactly the scenario it
exists for.
The heartbeat doesn't help either:
_heartbeat_loopprobes withbot.get_me(), which uses the general request path and stays healthyduring a
getUpdatesconflict — nothing escalates. From the outsideeverything looks fine: container healthy,
getMeOK, "connectionverified" in any UI that tests the token that way. Only the missing
replies reveal it.
Fix
Two-line idea, but with carefully designed semantics:
_polling_conflict_first_seenon the FIRST 409 of a streak._CONFLICT_WALL_CLOCK_LIMIT_SECS = 300.0(5 minutes): ifconflicts keep recurring past this window — regardless of any
intervening
start_polling()"successes" that reset the attemptcounter — the next 409 short-circuits past the retry branch and
falls through to the existing retryable-fatal escalation path so
the supervisor can restart the gateway.
To clear the timestamp, durable evidence of resolution is required
(actual update consumption) — not
start_polling()returning. Thispatch conservatively never clears it inside the conflict path; future
work can clear it from the update handler if desired.
Critical flow under the failure scenario:
The existing 5-attempt ladder still handles transient conflicts (e.g.,
gateway restart handoff) the same way it always did — it just isn't the
only safety net anymore.
Why wall-clock and not just an attempt counter ceiling
The issue suggests several alternative fixes (require observed update
drain to reset counter; bound by wall-clock; ensure no duplicate
long-poll tasks survive
updater.stop()timeout; surface conflictstate to health checks). Wall-clock is the smallest, most-rigorous
intervention: it tolerates counter resets without requiring changes to
the update-consumption path or the supervisor's health surface, it
cannot be tricked by a second competing poll task spraying 409s, and it
degrades cleanly — if
_polling_conflict_first_seenis never clearedin the conflict path (this patch), the only consequence is escalation
after 5 minutes of persistent conflict, which is exactly the desired
behavior.
Verification
New regression test
test_polling_conflict_becomes_fatal_after_wall_clock_limitmockstime.monotonic()to drive the wall-clock past the 5-minute limitacross two
_handle_polling_conflictcalls wherestart_polling()"successfully" returns and resets the counter in between — exactly the
scenario in #63724. Without the fix the test would hang in an infinite
ladder of 1/5 attempts; with the fix the second 409 escalates directly
to retryable-fatal.
Existing test suite
```
tests/gateway/test_telegram_conflict.py 14 passed (13 existing + 1 new)
```
No regressions in surrounding telegram tests.
Files changed
`_polling_conflict_first_seen` field; wall-clock gate in
`_handle_polling_conflict`.
test (`test_polling_conflict_becomes_fatal_after_wall_clock_limit`).
Notes for reviewer
cover the typical server-side session expiry (~30s) with ample margin
for repeated ladder cycles; short enough to bound the silent-deaf
window to a tolerable operator-response delay. Issue specifies
"say 5 minutes" as the example — match it.
only used transitively). It is already used elsewhere in the codebase
so this doesn't introduce a new dependency.
`_polling_conflict_count` block for locality.
remains the primary path for transient conflicts. The wall-clock
gate is a strict safety net for the stuck-forever case the ladder
cannot reach.
Happy to follow up on clearing `_polling_conflict_first_seen` from the
update-consumption path if maintainers want the timestamp reset on
genuine recovery — I left it conservative on purpose.