fix(telegram): bound and fail-open fallback-IP discovery during cold connect (#80632) - #86676
Conversation
૮ >ﻌ< ა ci reviewrunning on 0eef820 — fix(telegram): honor fallback disable during connect Still running 1 job:
|
trevorgordon981
left a comment
There was a problem hiding this comment.
Correct intent, well-implemented. The primary mechanism — bounded, fail-open discovery, kill switch actually bypassing discovery, disabled IPs excluded from proxy targets — is sound and uses the finite/clamped env helper and shield-proof abandon helper properly. The 4 tests directly cover the four advertised behaviors. One moderate finding before merge.
1. Thread leak on abandon (moderate — worth a deliberate decision)
discover_fallback_ips() spawns asyncio.ensure_future(asyncio.to_thread(_resolve_system_dns)). When the outer deadline fires, _await_with_thread_deadline cancels the parent coroutine but never awaits it (by design, to stay shield-proof) — and cancelling the coroutine does not interrupt the to_thread worker already running getaddrinfo, which the code itself notes can block "minutes." Every timeout-triggered cold connect leaks one such ThreadPoolExecutor worker while the OS resolver is wedged. Under repeated reconnect attempts those accumulate.
The fix neutralizes the hang (good), but in exactly the broken-DNS/VPN scenario this PR targets, it trades the hang for a background-thread leak. The stale result is only consumed for a log line, so it's functionally benign — but the thread-pool hygiene gap should be consciously accepted or addressed (e.g. bounding the to_thread pool, or documenting that the reaper is trusted).
2. Double-timeout shadowing the operator knob
Discovery already bounds its DoH legs to _DOH_TIMEOUT = 4.0s internally, so the outer 5.0s default only meaningfully fires if the inner bounds regress — defensible defense-in-depth. But an operator setting HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT below ~4s gets no real effect on the DoH network time, and the docs don't say so. If the knob is meant to shave cold-connect latency, this interaction should be documented.
3. Fail-open silently drops the seed fallback IPs
On timeout or error, fallback_ips = [] and it goes plain — discarding _SEED_FALLBACK_IPS (149.154.166.110 / 149.154.167.220) that a clean no-answer from discovery would otherwise have returned. Arguably the intended fail-open semantics, but there's no log line distinguishing "seed fallback skipped due to timeout" from a clean no-answer. Worth an explicit log so operators debugging DoH-blocked networks can tell what happened.
Tests
Strong on the four advertised behaviors, importantly including an end-to-end fail-open-on-timeout through the real _await_with_thread_deadline (stubbed discovery hangs, asserts connect() returns true). Gaps: no test exercises the real discover_fallback_ips (DoH providers, system-DNS bound, seed fallback), the non-timeout except Exception fail-open branch is only implicit, and the thread-leak behavior (finding #1) is unverified.
Addresses #80632 and #82558 (adapter-init family; salvage of #82626 by @worlldz with authorship preserved).
The bug
Telegram cold connect could enter — or be affected by — fallback-IP preparation before ever reaching the plain PTB connect path. The DoH fallback discovery (
discover_fallback_ips()) ran unbounded and outside any deadline: a stuck DoH lookup wedgesconnect()before the_await_with_thread_deadlineretry ladder even starts, which is exactly the "hangs at attempt 1/8 with no retry line and no watchdog dump" signature reported in the adapter-init hang family (#80632, #82558; the sibling macOS repro #82627 shows the same never-engaged deadline machinery).Also,
HERMES_TELEGRAM_DISABLE_FALLBACK_IPS=trueonly disabled fallback transport construction — discovery still ran, and configured fallback IPs still leaked into proxy target resolution (affectingNO_PROXYmatching) even when disabled.The fix (bounded + fail-open)
HERMES_TELEGRAM_FALLBACK_DISCOVERY_TIMEOUT(default 5s), parsed through the existing finite/clamped env parser sonan/inffall back to the safe default instead of defeating the bound (review finding, fixed on the PR branch).api.telegram.orgHTTPXRequest path — fail-open, never fail-hung.Verification
Companion to #86673 (parallel startup connects + capped Telegram cold-start budget): that PR bounds the gateway-side wait; this one removes the adapter-side unbounded pre-connect step that produced the wedge in the first place.
Infographic