Skip to content

fix(telegram): honor fallback disable during connect - #82626

Closed
worlldz wants to merge 1 commit into
NousResearch:mainfrom
worlldz:fix-telegram-connect-fallback-disable
Closed

fix(telegram): honor fallback disable during connect#82626
worlldz wants to merge 1 commit into
NousResearch:mainfrom
worlldz:fix-telegram-connect-fallback-disable

Conversation

@worlldz

@worlldz worlldz commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Addresses #80632
Related to #82558

Telegram cold connect could still enter or be affected by fallback-IP preparation before reaching the plain PTB connect path.

This patch makes fallback preparation bounded and fail-open:

  • HERMES_TELEGRAM_DISABLE_FALLBACK_IPS=true now skips DoH fallback discovery entirely, not only fallback transport construction.
  • When fallback is disabled, configured fallback IPs are also removed from downstream proxy target resolution, so disabled IPs cannot affect NO_PROXY matching.
  • Auto-discovery is bounded by HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT, defaulting to 5 seconds.
  • The discovery timeout uses the existing finite/clamped env parser, so NaN/Inf values fall back to the safe default instead of defeating the bound.
  • If discovery times out or fails, the adapter logs the failure and continues with the plain api.telegram.org HTTPXRequest path.
  • Fallback transport is still used when fallback IPs are configured or successfully discovered and fallback is not disabled.
  • The new operator timeout is documented in the Telegram guide.

Regression coverage:

  • fallback disabled skips discover_fallback_ips() during connect
  • stuck fallback discovery does not block connect
  • non-finite HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT still uses a finite default
  • configured fallback IPs are excluded from proxy target resolution while fallback is disabled
  • fallback-disabled and discovery-timeout paths continue through plain polling request construction without a fallback transport
  • existing init-deadline and Telegram network tests still pass

Validation:

python3 -m py_compile plugins/platforms/telegram/adapter.py
uv run --extra dev pytest tests/gateway/test_telegram_polling_progress.py tests/gateway/test_telegram_init_deadline.py tests/gateway/test_telegram_network.py -q
git diff --check

Result:

34 passed
git diff --check clean

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

@Riccardo-Vecchi Riccardo-Vecchi 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.

I reproduced two edge cases that still undermine the fail-open/kill-switch contract:

  1. The new discovery timeout is read through the local _env_float, which accepts non-finite values. With HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT=nan and a discovery coroutine waiting forever, _await_with_thread_deadline does not release connect(); an outer 200 ms deadline fires instead, and the watchdog emits a false "event loop BLOCKED" dump. (inf has the same unsafe shape.) This is the exact startup-hang class the PR is trying to bound. Could this use the existing finite/clamped parser, e.g. self._env_float_clamped(..., min_value=0.0), with a regression case for non-finite input?

  2. When fallback is disabled but config.extra.fallback_ips is populated, those disabled IPs are still included in proxy_targets. resolve_proxy_url() bypasses the proxy when NO_PROXY matches any target. Reproduction: fallback IP 149.154.167.220, HERMES_TELEGRAM_DISABLE_FALLBACK_IPS=true, TELEGRAM_PROXY=http://127.0.0.1:8080, NO_PROXY=149.154.167.220; connect() succeeds but the constructed polling request has proxy=None. Since the only actual target is now api.telegram.org, this can suppress the required proxy and break bootstrap. Clearing disabled fallback IPs (or excluding them from proxy_targets) would make the kill switch apply to all downstream decisions.

Both were exercised against head 010175c1 as focused async integration tests in test_telegram_polling_progress.py; result: 2 failed.

@worlldz
worlldz force-pushed the fix-telegram-connect-fallback-disable branch from 010175c to 76f4a93 Compare August 9, 2026 18:43
@worlldz

worlldz commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

I reproduced two edge cases that still undermine the fail-open/kill-switch contract:

  1. The new discovery timeout is read through the local _env_float, which accepts non-finite values. With HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT=nan and a discovery coroutine waiting forever, _await_with_thread_deadline does not release connect(); an outer 200 ms deadline fires instead, and the watchdog emits a false "event loop BLOCKED" dump. (inf has the same unsafe shape.) This is the exact startup-hang class the PR is trying to bound. Could this use the existing finite/clamped parser, e.g. self._env_float_clamped(..., min_value=0.0), with a regression case for non-finite input?
  2. When fallback is disabled but config.extra.fallback_ips is populated, those disabled IPs are still included in proxy_targets. resolve_proxy_url() bypasses the proxy when NO_PROXY matches any target. Reproduction: fallback IP 149.154.167.220, HERMES_TELEGRAM_DISABLE_FALLBACK_IPS=true, TELEGRAM_PROXY=http://127.0.0.1:8080, NO_PROXY=149.154.167.220; connect() succeeds but the constructed polling request has proxy=None. Since the only actual target is now api.telegram.org, this can suppress the required proxy and break bootstrap. Clearing disabled fallback IPs (or excluding them from proxy_targets) would make the kill switch apply to all downstream decisions.

Both were exercised against head 010175c1 as focused async integration tests in test_telegram_polling_progress.py; result: 2 failed.

Thanks, both edge cases were valid.

I updated the patch so the fallback discovery timeout uses the existing finite/clamped parser, which makes NaN/Inf fall back to the default 5s deadline instead of defeating the bound.

I also clear configured fallback IPs when HERMES_TELEGRAM_DISABLE_FALLBACK_IPS=true, so disabled fallback hosts no longer participate in proxy target resolution or NO_PROXY matching. With the kill switch set, the only proxy target is api.telegram.org.

Added focused regressions for both cases:

  • non-finite HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT still uses a finite default
  • configured fallback IPs are excluded from proxy target resolution while fallback is disabled

Validation:
uv run --extra dev pytest tests/gateway/test_telegram_polling_progress.py tests/gateway/test_telegram_init_deadline.py tests/gateway/test_telegram_network.py -q

Result:
34 passed

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #86676 — your commit was cherry-picked onto current main with your authorship preserved in git log, including your non-finite-timeout hardening and the four regression tests. Thank you for the thorough fail-open design and for addressing the review edge cases. Closing this PR as merged-via #86676.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter 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