Repository navigation
fix(telegram): retain loop cleanup across bounded stops - #6772
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
Verified against head 6cec431: the shield/generation design matches the ChannelService retain-and-retry contract (instance retained on ChannelStopTimeout/CancelledError, popped only after stop() completes), and I confirmed the regression tests are doing real work — all three new tests fail against the base revision's telegram.py and pass at head, the Telegram suites pass (42 passed), and ruff check/format are clean. One minor, non-blocking finding inline about the dead-loop branch never releasing an abandoned cleanup generation; happy path and retry convergence look correct.
| if cleanup_future is not None and not telegram_loop_running: | ||
| # Its owner loop is gone, so nothing can finish this cleanup. | ||
| logger.warning("[Telegram] Telegram loop stopped before its resource cleanup finished; the download bot's HTTPX clients may not have been closed") | ||
| cleanup_future = None |
There was a problem hiding this comment.
Minor/nit, non-blocking: this branch clears only the local cleanup_future, so a retained cleanup generation whose owner loop died before the cleanup could run is never released. The final guard at the bottom (if self._shutdown_resources_future is cleanup_future:) compares the stored future against None and never matches, leaving the abandoned future — and its never-started coroutine, which references the channel — attached to the retained instance, and every retried stop() re-walks this warning path. The instance is retained and heading for discard anyway, so impact is a stale object reference plus repeated warnings, but releasing the generation here keeps the 'exactly one cleanup generation' invariant airtight:
if cleanup_future is not None and not telegram_loop_running:
# Its owner loop is gone, so nothing can finish this cleanup.
logger.warning("[Telegram] Telegram loop stopped before its resource cleanup finished; the download bot's HTTPX clients may not have been closed")
with self._shutdown_resources_lock:
if self._shutdown_resources_future is cleanup_future:
self._shutdown_resources_future = None
cleanup_future = None
Why
TelegramChannel.stop()runs under the Gateway's bounded five-secondshutdown hook (
asyncio.wait_for(channel.stop(), ...)), and it schedules itsreal teardown — cancelling in-flight inbound-file bridge tasks and calling
Bot.shutdown()to close the download Bot's HTTPX clients — onto the Telegrampolling loop via
run_coroutine_threadsafe, because python-telegram-bot's HTTPXclients may only be closed on the loop that created them.
The previous implementation awaited that cleanup with a bare
asyncio.wait_for(asyncio.wrap_future(drain_future), ...)and, on timeout orcaller cancellation, called
drain_future.cancel()and then unconditionallytelegram_loop.stop()in afinallyblock. Both the deadline and a cancelledstop()therefore tore down the Telegram-loop-owned cleanup mid-flight:leak), and in-flight bridge tasks could be abandoned rather than cancelled;
stop()could join — a secondstop()would schedule a fresh_shutdown_telegram_resources()racing theabandoned one.
This is the same "an outer deadline/cancellation severs owner-held cleanup"
class fixed for the subagent batch service in #6552, here adapted to Telegram's
cross-thread event loop.
What changed
TelegramChannel.stop()now owns a single, retryable cleanup generation:stop()schedules_shutdown_telegram_resources()on theTelegram loop once and records the resulting future
(
_shutdown_resources_future, guarded by a lock so concurrent stops cannotschedule two).
asyncio.wait_for(asyncio.shield(...)), so theGateway deadline or a cancelled
stop()interrupts only the wait, never thecleanup the Telegram loop is running.
stop()raisesChannelStopTimeoutwhile leaving the loop,worker thread, application, and cleanup future owned by the instance — exactly
the retain-and-retry contract
ChannelServicealready applies to Feishu,DingTalk, Slack, and Buzz.
_thread/_application/_shutdown_resources_futurecleared only after the cleanup future hasresolved. A retried
stop()joins that same future and converges, withBot.shutdown()running exactly once.No behavior change for a channel that stops within budget.
Surface area
backend/app/channelsBug fix verification
backend/tests/test_channels.py::TestTelegramInboundMessagestest_shutdown_timeout_keeps_download_bot_cleanup_runningtest_cancelled_stop_keeps_owned_cleanup_and_retry_convergestest_concurrent_stops_share_one_telegram_cleanup_generationmainand green on this branch? yes. Onmainthedeadline/cancellation cancels the owned cleanup (the download Bot's
shutdown()never completes and the retry cannot join it); on this branch thecleanup survives, a retried
stop()joins the same generation, andBot.shutdown()runs exactly once.Validation
Run from
backend/:PYTHONPATH=.:packages/harness:packages/extension-api python -m pytest tests/test_channels.py::TestTelegramInboundMessages tests/test_telegram_channel_connections.py -q→ 42 passedmake lint→ruff check+ruff format --checkcleanmake test-blocking-io→ 245 passed, 1 skippedmake test(4 offline shards) → all Telegram/channel suites green. Twofailures surfaced that are unrelated to this change and reproduce independently
of it:
test_agent_guidance_checkflagsfrontend/AGENTS.mdas over its sizebudget on
origin/mainalready (this PR touches no frontend file), andtest_skills_custom_multiprocessis a multi-process worker-startup timeoutunder full-suite load that passes when run in isolation.
AI assistance
Tool(s) used: Claude Code
How you used it: AI assisted with diagnosis, the implementation, and the
regression tests; I reviewed every line.
🤖 Generated with Claude Code