Skip to content

fix(telegram): use wall deadline for init timeout - #58250

Closed
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-telegram-init-wall-timeout
Closed

tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-telegram-init-wall-timeout

Conversation

@tianma-if

Copy link
Copy Markdown
Contributor

Summary

  • replace Telegram bootstrap initialize() wait_for with a daemon-thread wall-clock deadline
  • abandon a shielded initialize task after timeout so the retry ladder can advance instead of staying on attempt 1/8 forever
  • add a regression test that proves a timed-out initialize attempt backs off and retries

Fixes #58236

Tests

  • .venv/bin/python -m pytest tests/gateway/test_telegram_init_deadline.py -q
  • .venv/bin/python -m pytest tests/gateway/test_telegram_init_deadline.py tests/gateway/test_telegram_closewait_limits_31599.py tests/gateway/test_telegram_network.py -q
  • .venv/bin/python -m pytest tests/gateway/test_telegram_network_reconnect.py tests/gateway/test_telegram_send_path_health.py tests/gateway/test_telegram_init_deadline.py -q
  • .venv/bin/python -m py_compile plugins/platforms/telegram/adapter.py tests/gateway/test_telegram_init_deadline.py
  • .venv/bin/python -m ruff check plugins/platforms/telegram/adapter.py tests/gateway/test_telegram_init_deadline.py
  • git diff --check

Note: the reconnect/send-path pytest batch passed but emitted an existing RuntimeWarning from a mocked _handle_polling_network_error coroutine in that test set; the new focused test and closewait/network batch run cleanly.

@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 sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 4, 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: Comment (clean fix, well-scoped)

Summary

This PR replaces asyncio.wait_for timeout with a wall-clock deadline using threading.Timer for Telegram adapter initialization. The change prevents PTB/httpcore initialization from being blocked inside cancellation-shielded anyio scopes.

Looks Good

  • Well-documented helper function with clear docstring explaining the problem and solution
  • Proper resource cleanup: timer.cancel() in finally block
  • Detached task exception consumption to avoid noisy loop logs
  • Clean backport: 2 files, 137 additions, focused single concern
  • Tests added for the new helper

Minor Note

  • logger.debug call on line 23 is intentional exception logging (not debug artifact)

Reviewed by Hermes Agent

@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: Comment (LGTM — token read-only, formal approval deferred to maintainer)

Fix for Telegram init timeout handling: replaces asyncio.wait_for with _await_with_thread_deadline which uses a threading.Timer for wall-clock deadline instead of the event loop's timer mechanism. This prevents init hangs when PTB/httpcore sits inside cancellation-shielded anyio scopes that never propagate cancellation back to the event loop. Test simulates the wedged initialize on attempt 1 and verifies retry with wall-deadline fires on attempt 2.

Looks Good

  • Clear rationale in comment and docstring explaining why asyncio.wait_for is insufficient
  • Proper cleanup: timer cancellation in finally block
  • _consume_abandoned_task callback prevents noisy loop error logs from orphaned tasks

Reviewed by Hermes Agent

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Merged via #58293 — your commit was cherry-picked with authorship preserved (d50aae0e3, on main in merge 5daa5a0f). Thanks for the fix and the sharp #58236 diagnosis (especially confirming that a naive Timer + task.cancel() still hangs because it awaits cancellation — that's exactly why the abandon-don't-await approach is the right one).

Your wall-deadline _await_with_thread_deadline landed as-is. The salvage added a follow-up on top:

  • Leak fix: the abandoned initialize() left the half-built PTB app's httpx connection pool open (up to 8× across the retry ladder). Added best-effort cleanup on abandonment, run detached + exception-swallowed so it can't re-block the ladder.
  • One subtlety worth noting: Application.shutdown()/Bot.shutdown() are gated on _initialized, which a wedged initialize() never sets — so the cleanup falls back to closing the bot._request transports directly (HTTPXRequest.shutdown() gates only on client.is_closed), releasing the pool regardless of PTB init state.
  • Added direct tests for the helper (the original test had monkeypatched it out) + AUTHOR_MAP entry.

Full credit to you. 🙏

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: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.

Gateway hangs indefinitely on Telegram connect (attempt 1/8) — asyncio.wait_for never fires under s6 supervision

4 participants