Skip to content

fix(gateway): release orphaned Telegram Application on failed connect (#14210) - #46313

Closed
tamio0800 wants to merge 1 commit into
NousResearch:mainfrom
tamio0800:fix/telegram-connect-fd-leak
Closed

fix(gateway): release orphaned Telegram Application on failed connect (#14210)#46313
tamio0800 wants to merge 1 commit into
NousResearch:mainfrom
tamio0800:fix/telegram-connect-fd-leak

Conversation

@tamio0800

Copy link
Copy Markdown

What does this PR do?

TelegramAdapter.connect() builds a PTB Application (holding two httpx pools), then if a later step fails, the except handler returns False without tearing the Application down. The supervisor retries with a fresh adapter and GCs the old one — but GC never closes httpx pools, so their sockets leak in CLOSE_WAIT until the process dies. After days of reconnect retries this exhausts the fd limit (EMFILE).

Fix: run the same teardown disconnect() already does, guarded so a cleanup error can't mask the original failure.

Complementary to #39336 (which raises the launchd fd cap and fixes steady-state pool sizing) — this stops the failure-path leak that accumulates regardless of the cap.

Related Issue

Refs #14210 (and its duplicate #14209)

Type of Change

  • 🐛 Bug fix

Changes Made

  • gateway/platforms/telegram.pyconnect() except handler now tears down self._app (stop updater → stop app → shutdown()) before releasing the lock, wrapped in try/except; _app/_bot reset in finally. Original error handling unchanged.
  • tests/gateway/test_telegram_connect_cleanup.py — 3 tests: cleanup runs on failure; safely skipped when _app never built; cleanup error doesn't mask the original exception.

How to Test

  1. pytest tests/gateway/test_telegram_connect_cleanup.py -q → 3 passed
  2. pytest tests/gateway/test_telegram_network_reconnect.py tests/gateway/test_telegram_network.py -q → 61 passed (no regression)
  3. Manual: force connect() to fail mid-startup; confirm Telegram CLOSE_WAIT sockets no longer accumulate across retries (lsof -p <pid> | grep CLOSE_WAIT)

Checklist

Code

  • Read the Contributing Guide
  • Conventional Commits
  • Searched existing PRs
  • Only changes related to this fix
  • Tests pass
  • Added tests
  • Tested on macOS 26.4 (Apple Silicon)

Documentation & Housekeeping

  • N/A — docstrings inline, no user-facing docs
  • N/A — no config keys
  • N/A — no architecture change
  • Leak/fix are platform-agnostic; EMFILE just surfaces fastest under macOS's 256 fd cap
  • N/A — no tool behavior change

@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Code Review: Clean

Reviewed the diff (gateway/platforms/telegram.py + test file). The fix correctly addresses the fd-leak described in #14210:

  1. Cleanup scope — The except handler checks self._app is not None before attempting cleanup, avoiding NPE when the failure occurs before the Application is built.
  2. Orderingupdater.stop()app.stop()app.shutdown() follows the correct teardown sequence. The updater.running and app.running guards prevent redundant stop calls.
  3. Exception safety — Inner try/except catches cleanup errors and logs them without masking the original connect failure. The finally block ensures _app = None and _bot = None regardless of cleanup outcome.
  4. Test coverage — Three tests cover: (a) full cleanup on failure, (b) no-op when app not built, (c) cleanup error doesn't replace the original exception in _set_fatal_error.

No issues found. The fix prevents CLOSE_WAIT socket accumulation that would eventually exhaust the fd limit on repeated gateway supervisor retries.

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter labels Jun 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Automated hermes-sweeper review: this cleanup is already covered on current main.

Evidence:

  • Current main disposes adapters that fail during startup: gateway/run.py:5641 calls _safe_adapter_disconnect(adapter, platform) when adapter.connect() returns false.
  • Current main disposes adapters that fail during reconnect: gateway/run.py:6422, gateway/run.py:6458, and gateway/run.py:6474 call _dispose_unused_adapter(adapter) across the non-retryable, retryable, and exception paths.
  • _dispose_unused_adapter() is defined at gateway/run.py:2404 and calls await adapter.disconnect() while suppressing cleanup errors.
  • The current Telegram adapter teardown path already stops the updater, stops the app, runs shutdown(), and clears _app / _bot in plugins/platforms/telegram/adapter.py:2448.
  • The covering implementation landed in 4b06c98fe4f4686f89873933ac616cb6e76b298d (fix(gateway): close ResponseStore + dispose unowned adapter on reconnect failure), with regression coverage in tests/gateway/test_platform_reconnect_fd_leak.py:139.

The prior review comment here was useful; the final mainline fix moved the cleanup to the gateway ownership boundary rather than duplicating it inside the old Telegram adapter path.

@teknium1 teknium1 closed this Jun 21, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 21, 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:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants