Skip to content

fix(telegram): drain send connection pool on sustained pool timeouts - #38812

Closed
E-R-Butch wants to merge 2 commits into
NousResearch:mainfrom
E-R-Butch:fix/telegram-send-pool-drain
Closed

fix(telegram): drain send connection pool on sustained pool timeouts#38812
E-R-Butch wants to merge 2 commits into
NousResearch:mainfrom
E-R-Butch:fix/telegram-send-pool-drain

Conversation

@E-R-Butch

Copy link
Copy Markdown

Problem

The httpx connection pool used for send_message / edit_message (_request[1], the general pool) had no recovery mechanism. When proxy-level connection leaks (half-closed TCP connections through Clash, etc.) filled all 512 pool slots, every subsequent send/edit failed with:

Pool timeout: All connections in the connection pool are occupied.
Request was *not* sent to Telegram.

Retrying doesn't help — stuck connections must be torn down. The only recovery was a full gateway restart.

The polling pool (_request[0]) already had _drain_polling_connections(), but the send pool had no equivalent.

Fix

  • Added _drain_send_connections() — mirrors _drain_polling_connections() but cycles _request[1] (general request) instead of _request[0] (polling)
  • Added _send_pool_timeout_count counter to track consecutive pool timeouts
  • In the send() retry loop: after 3 consecutive pool timeout errors, drain the send pool before the next retry attempt
  • Counter resets on successful sends

Each pool is drained independently — this fix does not touch the polling pool, same as the existing polling drain does not touch the send pool.

Tests

7 new tests in test_telegram_network_reconnect.py:

  • Pool cycles only _request[1], not _request[0]
  • No-op when _app or _app.bot is None
  • Continues if shutdown/initialize fails
  • Counter starts at 0 and resets on drain

The httpx connection pool used for send_message / edit_message
(_request[1], the general pool) had no recovery path. When proxy-level
connection leaks filled all 512 pool slots, every send/edit failed with
"Pool timeout: all connections occupied" and retrying was useless.

Add _drain_send_connections() that cycles _request[1] (shutdown →
initialize), mirroring _drain_polling_connections() which already
exists for the polling pool (_request[0]). Wire it into the send()
retry loop: after 3 consecutive pool timeout errors, drain the pool
before the next retry attempt. Reset the counter on successful sends.

The polling pool is intentionally left untouched by this drain —
same separation guarantee the existing polling drain makes for sends.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter P1 High — major feature broken, no workaround labels Jun 4, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

I found one issue worth fixing before merge.

tests/gateway/test_telegram_network_reconnect.py — the test test_send_pool_drain_resets_counter does not verify what its name and docstring claim.

async def test_send_pool_drain_resets_counter():
    adapter._send_pool_timeout_count = 3
    await adapter._drain_send_connections()
    adapter._send_pool_timeout_count = 0  # ← manual reset
    assert adapter._send_pool_timeout_count == 0  # ← always passes

The manual adapter._send_pool_timeout_count = 0 on the line before the assertion means the test passes regardless of whether _drain_send_connections() actually resets the counter. Looking at the implementation, _drain_send_connections() does NOT reset the counter — the reset happens in the caller (send() method, lines 77 and 90). So the test name "drain_resets_counter" is misleading.

Suggested fix: either remove the manual reset and assert directly (if the intent is to test that the caller resets), or rename the test to reflect what it actually verifies (e.g., test_drain_send_connections_completes_without_error):

async def test_send_pool_drain_resets_counter():
    adapter._send_pool_timeout_count = 3
    await adapter._drain_send_connections()
    # _drain_send_connections itself doesn't reset the counter;
    # the send() caller does. This test verifies drain completes cleanly.
    assert adapter._send_pool_timeout_count == 3  # unchanged by drain

This is a minor test quality issue — the production code is correct (the send() method properly resets the counter after drain and on success).

- test_send_pool_drain_resets_counter: manual =0 before assert made it a no-op.
  Renamed to test_drain_send_does_not_reset_counter, correctly asserts counter
  stays at 3 after drain (reset is caller's responsibility in send()).

- Replaced inconsistent hasattr/getattr patterns with uniform getattr default
  for defensive read; use direct assignment for writes where counter is
  guaranteed to exist.

- Added test_send_pool_timeout_counter_full_lifecycle for counter lifecycle.

Co-authored-by: liuhao1024 (code review)
@E-R-Butch

Copy link
Copy Markdown
Author

Thanks for catching this — the manual = 0 on the line before the assert made the test a complete no-op.

Fixed in the latest push:

  • Renamed to test_drain_send_does_not_reset_counter, correctly asserts counter stays at 3 after drain (since _drain_send_connections doesn't reset — that's the send() caller's job)
  • Added test_send_pool_timeout_counter_full_lifecycle for the full counter lifecycle
  • Also cleaned up the inconsistent hasattr/getattr patterns in the production code

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Automated hermes-sweeper review: this Telegram send-pool recovery fix is now implemented on main.

Evidence:

  • plugins/platforms/telegram/adapter.py:1670 defines _drain_general_connections_after_pool_timeout(), which resets PTB's general Bot API request pool (_request[1]) with shutdown() then initialize().
  • plugins/platforms/telegram/adapter.py:3185 calls that drain helper from the send() retry loop whenever _looks_like_pool_timeout(send_err) is true.
  • tests/gateway/test_telegram_thread_fallback.py:1343 covers the behavior: a pool timeout drains only the general request pool, retries successfully, and leaves the polling pool untouched.
  • The implementation landed in 7e2ca7f68da63a29e1695b5e24901cc57e32f2ac (fix(telegram): reset send pool after pool timeouts).

Thanks to @E-R-Butch for the original fix and to @liuhao1024 for catching the dead-test issue in review; the same behavioral guarantee is now covered on main.

@teknium1 teknium1 closed this Jun 29, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants