fix(telegram): recover exhausted request pool by helix4u · Pull Request #98094 · NousResearch/hermes-agent

helix4u · 2026-08-29T19:18:25Z

What does this PR do?

Telegram polling recovery currently resets only PTB's dedicated getUpdates request pool. When the triggering error is a confirmed pool timeout, that is insufficient: start_polling() performs Bot API bootstrap calls through PTB's separate general request pool before polling begins, so an exhausted general pool causes every reconnect attempt to fail before it can reach getUpdates.

This change resets the general request pool only for classifier-confirmed pool timeouts, then continues through the existing polling-pool drain and restart path. Generic network errors remain polling-only so ordinary recovery does not interrupt concurrent sends. General-pool shutdown and initialization are bounded with the adapter's existing wall-clock deadline helper so a cancellation-resistant transport cannot freeze the reconnect ladder.

This salvages #68983 by @AideYu onto current main; the original commit authorship is preserved.

Related Issue

Supersedes #68983, which no longer applies cleanly to current main.

Type of Change

🐛 Bug fix (non-breaking change that fixes an issue)
✨ New feature (non-breaking change that adds functionality)
🔒 Security fix
📝 Documentation update
✅ Tests (adding or improving test coverage)
♻️ Refactor (no behavior change)
🎯 New skill (bundled or hub)

Changes Made

Reset PTB's general Bot API request pool before polling recovery when the error is a confirmed pool timeout.
Keep generic polling-network recovery scoped to the getUpdates pool.
Bound general-pool shutdown and initialization with the current thread-deadline helper.
Add behavioral coverage for pool-timeout recovery, generic-error isolation, and a hung general-pool drain.

How to Test

Run scripts/run_tests.sh tests/gateway/test_telegram_network_reconnect.py -q -j 4.
Confirm a pool-timeout recovery resets both PTB request pools and reaches start_polling().
Confirm a generic network error leaves the general request pool untouched, and a hung general-pool close cannot pin recovery indefinitely.

Local execution was not performed under the workstation test policy. Static verification completed: both changed Python files parse, git diff --check passes, and scripts/check-windows-footguns.py --diff origin/main passes. Focused execution is left to CI.

Checklist

Code

I've read the Contributing Guide
My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
I searched for existing PRs to make sure this isn't a duplicate
My PR contains only changes related to this fix/feature (no unrelated commits)
I've run pytest tests/ -q and all tests pass
I've added tests for my changes (required for bug fixes, strongly encouraged for features)
I've tested on my platform

Documentation & Housekeeping

I've updated relevant documentation (README, docs/, docstrings) — or N/A
I've updated cli-config.yaml.example if I added/changed config keys — or N/A
I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A. The regression is covered with mocked PTB request pools and lifecycle behavior.

AideYu · 2026-08-30T00:21:17Z

Hi helix4u, this is @AideYu, author of the original #68983. Reviewed the diff — this is a faithful rebase onto current main, and the generalization to also drain the general request pool on classifier-confirmed timeouts (_await_with_thread_deadline-bounded) is a solid improvement over my initial version. Appreciate you salvaging it; happy to see it merged. Thanks!

_drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch.

_looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the #98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for.

_drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (NousResearch#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (NousResearch#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch.

_looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the NousResearch#98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for.

* chore: sync fork with upstream through a9c783f * fix(dashboard-auth): url-encode the PKCE cookie value so strict proxy hops stop dropping it (NousResearch#99176) The PKCE payload is a flat 'provider=...;state=...;verifier=...;next=...' string. A raw ';' is a cookie-attribute terminator, so Python's http.cookies emits the value in RFC 6265 quoted form with each ';' escaped as the backslash-octal '\073'. Mainstream browsers echo that form back verbatim and Python parsers decode it — the browser round trip is fine. But '"' and '\' are outside the plain cookie-octet set, and non-Python hops that re-serialize the Cookie header reject the value and drop the cookie entirely: Go's net/http (Traefik middleware, Authentik outposts, other gateways) refuses any cookie value containing a backslash. The OIDC callback then 400s with "Missing PKCE state cookie" even though the browser sent the cookie. Field reproduction: support thread "Still unable to use Authentik for signin with traefik" — devtools showed the browser sending the intact quoted \073 cookie on /auth/callback while Hermes logged missing_pkce_cookie behind a Traefik+Authentik chain. Fix: URL-encode the whole payload in set_pkce_cookie (quote(payload, safe='') — ';' becomes '%3B') so the wire value contains only cookie-octets and no parser in the chain has anything to reject, and decode through a single shared inverse, cookies.parse_pkce_payload(), in BOTH readers: the OAuth /auth/callback and the native password-login path (routes.login_submit), whose broker/provider binding check would otherwise parse zero segments from the newly-encoded value and silently disable itself. Regression coverage: the wire-shape test pins the full cookie-octet set (the '"'/'\' assertions are the ones a Go-parser hop fails pre-fix), the round-trip tests drive the real /auth/login → /auth/callback path, and the next= test pins the exact post-login redirect byte shape. Native-flow broker assertions updated to decode through parse_pkce_payload instead of substring-matching the raw wire value. Salvaged from NousResearch#84065 (rebased onto current main, which gained the SameSite=None PKCE attrs and the RFC 8252 native password flow since the PR branched): kept main's _pkce_attrs cookie shape, extended the fix to the login_submit reader the original PR predated, and reframed the rationale — browsers do NOT truncate at the first ';' (there is no literal ';' on the wire in the quoted form); the failing hop is a strict middlebox cookie parser. Closes NousResearch#83832 Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com> * fix(telegram): bound polling drain with wall-clock deadline _drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (NousResearch#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (NousResearch#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch. * test: settle generation verifier in shielded-close drain test * refactor(telegram): share exception-graph walk across classifiers _looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the NousResearch#98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for. * test: stabilize Telegram deadline assertion on Windows * fix: address hosted room review findings * fix: close hosted room publication races * fix(relay): resolve fresh-final unfurl decision per chat, not per primary identity (NousResearch#99206) The stream consumer called prefers_fresh_final_streaming(text, metadata=...) only, and no metadata producer stamps a platform key — so RelayAdapter's hook always fell back to the PRIMARY descriptor's platform (the scalar-vs-per-chat capability seam, third occurrence). Two failure directions on multiplexed relays with platforms.relay.extra.slack.unfurl_links/media: true (NousResearch#97957): - Slack primary fronting Telegram/Discord: every link-bearing streamed final on the non-Slack chats finalized as a fresh send with no delete op advertised -> the answer delivered TWICE (orphaned preview). - Non-Slack primary fronting Slack: the hook returned False, leaving the force-on unfurl feature dark on exactly the chats it shipped for. Pass chat_id=self.chat_id from the consumer; the relay hook already accepted it and resolves via _platform_by_chat + the per-platform negotiated descriptor. Graduated TypeError fallback keeps the single-platform hook signatures (Telegram, base class) and legacy test doubles working unchanged. Both regression tests verified RED against the unfixed consumer, GREEN with the fix; single-platform relays are unaffected (NousResearch#97957's own 30 tests unchanged-green). * fix(bot-mode): preserve UTF-8 local DM delivery on Windows * fix(hosted-rooms): close remaining lifecycle races * fix(bot-mode): use subprocess env factory for peer delivery * test: accept asynchronous stop settlement * test: replace fixed waits with lifecycle conditions * fix(groups): close hosted room review races * fix: close hosted room authority review gaps * fix: close hosted room retry review gaps * fix(groups): refresh demotion stop fence on retry * fix(groups): defer deleted profiles before admission * fix(tui): recover long hosted room ids for prompt fence * test(groups): assert fresh stop per demotion attempt * fix(groups): fence profiles deleted after admission * fix(groups): close hosted room review races * fix(groups): account authority loss storage * fix(groups): keep cross-process stops pending * fix(telegram): quarantine abandoned polling shutdown * test(groups): align demotion fixture with process owner * fix(hosted-rooms): harden stop ownership and terminal recovery * fix(groups): close hosted-room concurrency invariants * fix(groups): close terminal recovery races * fix(groups): close remaining hosted room review gaps * fix(groups): preserve concurrent disband and legacy approvals --------- Co-authored-by: Ben Barclay <ben@nousresearch.com> Co-authored-by: Kailigithub <12250313+Kailigithub@users.noreply.github.com> Co-authored-by: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>

_drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (NousResearch#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (NousResearch#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch.

_looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the NousResearch#98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for.

_drain_polling_connections still bounded its shutdown()/initialize() with asyncio.wait_for (NousResearch#66377), while its sibling the general-pool drain moved to _await_with_thread_deadline (NousResearch#98094). httpcore's pool close runs under AsyncShieldCancellation, so a cancellation-resistant close keeps wait_for pending forever even after its timeout fires — the tracked _polling_error_task wedges and every escalation gate behind it stalls. Use the same wall-clock deadline helper (cancel + abandon, no cancel-await) on both polling-drain awaits, and add a regression test whose close swallows cancellation — the shape the existing cancellable-hang test cannot catch.

_looks_like_connect_timeout and _looks_like_pool_timeout carried two copies of the same 15-line DFS skeleton (seen-set, stack, __cause__/ __context__ descent) differing only in the one-line match predicate — follow-up to the NousResearch#98094 review. Extract _iter_exception_graph() and collapse both classifiers onto it. Behavior is byte-identical (subprocess parity vs origin/main on real PTB error fixtures: 6/6 identical), and the two classifiers gain direct unit tests for the first time, including the cycle/diamond chain shapes the inline copies had no coverage for.

AideYu and others added 2 commits August 29, 2026 13:17

fix(telegram): recover exhausted request pool

8727eae

chore: map contributor email

c8b0ca4

alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 29, 2026

chore: retrigger flaky CI

91e91c6

helix4u marked this pull request as ready for review August 29, 2026 19:29

AideYu mentioned this pull request Aug 30, 2026

fix(telegram): recover exhausted request pool #68983

Closed

teknium1 merged commit cd1c321 into NousResearch:main Aug 30, 2026
35 checks passed

This was referenced Aug 31, 2026

fix(telegram): bound polling drain with wall-clock deadline #99192

Merged

refactor(telegram): share exception-graph walk across classifiers #99193

Merged

Navigation Menu

fix(telegram): recover exhausted request pool - #98094

fix(telegram): recover exhausted request pool#98094
teknium1 merged 3 commits into
NousResearch:mainfrom
helix4u:fix/telegram-general-pool-recovery

helix4u commented Aug 29, 2026

Uh oh!

AideYu commented Aug 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

helix4u commented Aug 29, 2026

What does this PR do?

Related Issue

Type of Change

Changes Made

How to Test

Checklist

Code

Documentation & Housekeeping

Screenshots / Logs

Uh oh!

AideYu commented Aug 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants