fix(gateway): parallel platform connects + capped Telegram cold-start budget (#85993, #83791) - #86673
Conversation
) GatewayRunner.start() previously awaited each platform's connect() (with its own timeout) in a serial for-loop. A single slow/failing platform (e.g. Telegram behind a dead proxy) delayed every later platform's connect by a full timeout window, cascading one platform's failure onto WeChat/QQ/etc. Now the slow connect() calls run concurrently via asyncio.gather while the serial pre-filter (checks, adapter creation, handler wiring) and the single-threaded result aggregation (shared-state mutation, error handling) are unchanged. A failing platform no longer blocks the others. Adds regression tests proving connect() calls overlap and that one failing platform leaves the others connected.
The previous concurrency assertion (slow_start < fast_end) was true under BOTH the serial and parallel implementations, so it proved nothing -- it even passed against the old serial code on main. The only assertion that distinguishes the two is that the fast platform finishes before the slow one (fast_end before slow_end), which is only possible when the connects overlap. Switch the test to record connect start/end events in arrival order (clock-resolution independent) and assert fast_end precedes slow_end. This also fixes the Windows failure @zuowen7 reported: time.monotonic() has only ~15 ms resolution there, so two parallel connects could land on the same tick and defeat any wall-clock comparison -- event ordering cannot. Verified the new test fails against origin/main (serial) and passes against this branch (parallel).
…ing fast (#85993) The initial (pre-running) connect awaited during gateway startup now uses a capped 45s budget for Telegram instead of the full 180s (#67498) budget. On timeout the platform is queued for the reconnect watcher, which retries with the full budget and is_reconnect=True (preserving the offline update queue, #46621). Combined with the parallel startup connects, an unreachable Telegram no longer holds the whole gateway out of the running state.
alt-glitch
added
type/bug
૮ >ﻌ< ა ci reviewran on bf5e2a4 — fix(gateway): abort-aware parallel startup connects (restart
|
trevorgordon981
left a comment
trevorgordon981
left a comment
There was a problem hiding this comment.
The core refactor is right-shaped: serial pre-filter + concurrency + single-threaded aggregation preserves serial state semantics, and the 45s Telegram cap is cleanly scoped via initial= without disturbing the reconnect budget. Two things to resolve before merge.
1. Detached wedged connect leaks and invites a double-connect race
The cap is enforced via the existing detach-on-timeout pattern: the underlying adapter.connect() task is abandoned rather than cancelled. The 45s cap makes detach 4× more likely during startup than the old 180s, and the PR's own E2E enshrines the residue: test_initial_connect_times_out_at_cap_and_queues_retry shrinks the cap to 0.2s against a _WedgedAdapter whose connect() sleeps 3600s. On timeout that 3600s task is detached and never cancelled; Telegram is queued to _failed_platforms with next_retry = now+30; when the watcher fires it calls connect() again on the same adapter while the first wedged connect is still running → two concurrent connect()s on one adapter.
The test only asserts Platform.TELEGRAM in runner._failed_platforms and not in runner.adapters — it passes while the leaked task and latent double-connect live on. Partly pre-existing in serial mode, but the cap makes it more likely and this test stabilizes the scenario without cancelling or asserting on the detached task. The test should account for the leaked task, and the reconnect path should cancel a preceding detached connect before re-connecting.
2. Confirm the abort helpers exist on the base
The new code calls _abort_startup_if_shutdown_requested(adp, p) and _startup_should_abort(), but the diff doesn't define them and they're absent from my checkout (older main). Presumably they're on the PR base — verify before merge, since if they don't exist the abort path raises.
3. The "byte-identical shared-state" claim is imprecise
_update_platform_runtime_status(...,"connecting") mutates shared runtime state from inside the parallel tasks, outside the aggregation loop. Harmless today (keyed per-platform, asyncio is single-threaded), but the claim that all shared mutation is deferred to the single-threaded loop is technically overstated — worth correcting so it doesn't mask a future real race.
4. Minor
- 0.05s abort-poll granularity adds latency to honoring a shutdown mid-connect (bounded, not a correctness break).
- Abort teardown matches the closure's tuple contract via
_t.result()[3] == "ok"string comparisons — a fragile coupling between the closure layout and the abort path. - Aggregation order is deterministic (dict insertion = config order), good.
Tests
Strong on the two headline features: the concurrency proof is event-order-based (correct — wall-clock is a no-op on Windows), plus budget-cap, env-override, other-platforms-unchanged, and a wedged E2E. Gaps: the abort-mid-connect path, the detached-task leak / double-connect (finding #1), the "exception"-escape path, and non-deterministic multi-failure ordering.
4 participants
Fixes #85993. Fixes #83791.
The bug
Gateway startup connected messaging platforms sequentially — each platform's
connect()(with its own timeout) was awaited before the next began (gateway/run.pystartup loop). Two deliberate changes conflicted:getUpdatesreadinessResult (#85993): when Telegram is unreachable, its 180s budget becomes a 180s wall-clock stall imposed on every other platform, and the gateway doesn't reach
runninguntil Telegram gives up.The fix (two halves)
1. Parallel startup connects — salvaged from #83809 by @EvanProgramming (cherry-picked with authorship preserved, incl. the follow-up commit that makes the concurrency test genuinely distinguish serial from parallel via event ordering after @zuowen7's Windows field-testing found the original wall-clock assertion was a no-op). The serial pre-filter (cheap checks, adapter creation, handler wiring) stays sequential; the slow
connect()calls run concurrently viaasyncio.gather(return_exceptions=True); result aggregation is single-threaded so all shared-state mutation (self.adapters,self._failed_platforms, error lists,connected_count) is byte-identical to the old serial loop's behavior.2. Capped Telegram cold-start budget — the residual gap called out in the #85993 thread: for a Telegram-only gateway the parallel change alone is behaviorally identical to the serial await, and even multi-platform gateways still wait the full 180s before
running. The initial (pre-running) connect now uses a capped 45s budget (_TELEGRAM_INITIAL_CONNECT_TIMEOUT_SECS_DEFAULT); on timeout the platform enters the existing reconnect watcher, which retries with the full 180s budget andis_reconnect=True— preserving #67498's cold-readiness proof on the retry path and #46621's offline update queue.HERMES_GATEWAY_PLATFORM_CONNECT_TIMEOUTstill overrides everything. Not a Telegram skip: the platform still connects, just off the critical startup path when it's slow.Verification
Related: #82627/#80632 (adapter-level init deadlock — separate diagnosis in flight via #82626); #13602 (older competing parallel-startup PR, superseded by this).
Infographic