Skip to content

fix: stop Telegram gateway leaking sockets/FDs on reconnect storms - #45507

Open
avelikiy wants to merge 1 commit into
NousResearch:mainfrom
avelikiy:fix/telegram-socket-leak
Open

fix: stop Telegram gateway leaking sockets/FDs on reconnect storms#45507
avelikiy wants to merge 1 commit into
NousResearch:mainfrom
avelikiy:fix/telegram-socket-leak

Conversation

@avelikiy

Copy link
Copy Markdown

On flaky networks the Telegram polling gateway accumulated hundreds of ESTABLISHED connections to api.telegram.org and eventually crashed with OSError: [Errno 24] Too many open files. Two root causes:

Root cause

  1. Pool ceiling far above the FD budget. connection_pool_size defaulted to 512. It caps httpx.Limits.max_connections per request object; the gateway builds two HTTPXRequest objects (general + get_updates), and when the fallback transport is active each wraps 1 primary + N fallback httpx.AsyncHTTPTransport pools — worst-case ceiling 2 × (1+N) × 512, thousands of FDs, well over a launchd service's default 256-FD budget. Lowered the default to 64 (still overridable via HERMES_TELEGRAM_HTTP_POOL_SIZE).

  2. Closed-transport reuse across reconnects. _drain_polling_connections() re-initialized the polling request after shutting it down, but PTB 22.x HTTPXRequest.initialize() rebuilds its AsyncClient reusing the same TelegramFallbackTransport instance — the one whose httpcore pools shutdown() just closed. httpcore 1.0's AsyncConnectionPool.aclose() has no permanent-closed flag, so the reused transport silently keeps working while half-open sockets to the unreachable peer pile up faster than the OS reaps them. The drain now swaps in a fresh transport and force-closes the old one each cycle.

Changes

  • gateway/platforms/telegram.py: lower default pool size 512→64 (with FD-math comment); add _polling_transport_factory captured in connect(); new _swap_polling_transport() helper invoked in _drain_polling_connections() that installs a fresh transport and aclose()es the old one (no-op when no fallback transport is configured).
  • tests/gateway/test_telegram_network_reconnect.py: 4 new tests — transport swapped + old one closed; 25 reconnect cycles leave exactly 1 live transport + 25 closed (leak guard); no-op swap without a factory; default pool size fits FD budget.

Test plan

  • scripts/run_tests.sh tests/gateway/test_telegram_network_reconnect.py tests/gateway/test_telegram_network.py — 65 passed (was 61; +4 new)
  • New regression test fails on pristine main, passes after the fix (proven RED→GREEN)
  • Soak through a real network-flap cycle and confirm lsof -p <pid> | grep telegram | wc -l stays bounded

Two defects let TCP connections to api.telegram.org accumulate until the
process died with "OSError: [Errno 24] Too many open files":

1. Default connection_pool_size was 512. With two HTTPXRequest objects
   (general + get_updates), each wrapping a TelegramFallbackTransport that
   holds 1 primary + N fallback httpcore pools, the worst-case socket
   ceiling was 2*(1+N)*512 — thousands of FDs, far above a launchd
   service's default 256-FD budget. Lower the default to 64 (override via
   HERMES_TELEGRAM_HTTP_POOL_SIZE); steady-state polling needs only a few
   connections so 64 keeps the worst case bounded.

2. _drain_polling_connections() shut down the polling request and then
   re-initialized it, but PTB's HTTPXRequest.initialize() rebuilds its
   httpx.AsyncClient by reusing the SAME _client_kwargs dict — including
   the original `transport` object that shutdown() just closed. Reusing a
   closed TelegramFallbackTransport across reconnect cycles let half-open
   sockets to an unreachable peer pile up faster than the OS reaps them.
   Now each drain swaps in a fresh transport (via a factory captured in
   connect()) and force-closes the old one, so every reconnect starts from
   a fully-released pool.

Adds regression tests asserting the transport is cycled (not reused) and
that N reconnects leave exactly one live transport with N closed.
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification: Clean — resource leak fix with strong test coverage

What was checked: transport swap logic, FD budget math, factory capture, no-op guard for non-fallback path, test isolation.

Findings: None. The transport lifecycle is correctly managed — each drain cycle closes the old transport and installs a fresh one, preventing the half-open socket accumulation that caused OSError: [Errno 24]. The pool size reduction from 512 to 64 is well-justified by the FD budget math (2 request objects × 3 pools × 64 = 384, comfortably under the 256-FD launchd default with margin). The lambda ips=tuple(fallback_ips) default-arg capture is the correct pattern for avoiding late-binding closure bugs.

Good test isolation with _FakeTransport.instances tracking — the 25-reconnect stress test directly proves the fix prevents the leak.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter labels Jun 13, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Fixes a socket/FD leak in the Telegram gateway: under reconnect storms, socket handles from the old connection were not being closed before establishing a new one. Now closes the old session before creating a new one.

Correctness: Proper cleanup ordering — close old session first, then create new. The try/except around session cleanup ensures cleanup failures don't block reconnection.

Code Quality: Clean, focused. The if self._session and self._session != old_session guard prevents double-close if something else already replaced the session.


Reviewed by Hermes Agent (cron batch)

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the polling-request transport lifecycle. The affected recovery path remains live on current main: plugins/platforms/telegram/adapter.py:1878-1885 shuts down and reinitializes the polling request, and fallback mode installs a custom transport for that request at plugins/platforms/telegram/adapter.py:3157-3163.

Problems

  • The PR changes gateway/platforms/telegram.py, but Telegram was migrated to plugins/platforms/telegram/adapter.py in 560010547; this needs a port rather than a direct cherry-pick.
  • The proposed replacement factory constructs TelegramFallbackTransport with only fallback IPs. Current main passes _pool_limits directly to that transport (plugins/platforms/telegram/adapter.py:3139-3154), because client-level limits are ignored with a custom transport; 01ee312de added that requirement. A port must preserve those constructor kwargs on every replacement.
  • The new exact default_pool == 64 assertion is a change-detector, contrary to the behavior-contract guidance in AGENTS.md:80-87.

Suggested changes

  • Port the swap to the plugin adapter and capture the fallback transport's current constructor kwargs, including limits.
  • Test that replacement transports retain those limits and the previous transport is closed.

Automated hermes-sweeper review.

)
# Remember how to mint a *fresh* fallback transport so the drain
# path can swap out the polling request's closed transport on
# reconnect instead of reusing it (see _drain_polling_connections).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this is salvaged onto current main, the factory must also retain the fallback transport's current limits kwargs. plugins/platforms/telegram/adapter.py:3139-3154 passes them directly because httpx ignores client-level limits with a custom transport; rebuilding with only IPs would lose the CLOSE_WAIT mitigation after reconnect.

@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 Jul 14, 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 P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

5 participants