fix(telegram): cancel delayed deliveries on disconnect - #73
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
This PR fixes Telegram delayed delivery drops but introduces a race condition in _cancel_pending_delivery_tasks that can orphan a polling error task, silently degrading outbound sends after reconnect. Files Reviewed (2 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟡 Medium (45/100) — 1 medium finding · 119 LOC across 2 files
Summary
A focused PR modifying two files: plugins/platforms/telegram/adapter.py (Telegram adapter delayed-delivery fix) and scripts/release.py (changelog author attribution).
Key Finding
Race condition in disconnect teardown (medium severity)
In plugins/platforms/telegram/adapter.py, the _cancel_pending_delivery_tasks() method (lines 2940-2985) contains a race window: after cancelling flush tasks, asyncio.gather() at line 2976 yields to the event loop while the PTB updater is still running (stopped later at line 3019). If a polling network error fires during this gather window, _polling_error_callback creates a new _polling_error_task, but lines 2984-2985 unconditionally set _polling_error_task = None, orphaning it.
The orphaned _handle_polling_network_error task sets _send_path_degraded=True, which persists across disconnect/reconnect and blocks all outbound message sends until _verify_polling_after_reconnect resets it ~60 seconds later.
Fix: Capture the task reference before gather and only nullify if unchanged after.
Other observations
Several sub-threshold findings were noted but not flagged: inconsistent CancelledError handling across flush methods, stale batch state dicts when gateway disconnect times out during gather, and an accidental removal of contributor ypwcharles from the AUTHOR_MAP in scripts/release.py.
| if self._polling_error_task is not current_task: | ||
| self._polling_error_task = None |
There was a problem hiding this comment.
🟡 Race condition: _polling_error_task orphaned during disconnect can degrade outbound sends after reconnect (bug)
In _cancel_pending_delivery_tasks() (lines 2940-2985), after collecting and cancelling all pending flush tasks, asyncio.gather() at line 2976 yields to the event loop. During this yield, the PTB polling updater is still running (stopped later at line 3019). If a polling network error occurs during the gather window, _polling_error_callback (line 2793) fires and creates a new _polling_error_task via loop.create_task() (line 2808). After gather completes, lines 2984-2985 unconditionally set self._polling_error_task = None, discarding the reference to this newly created recovery task. The orphaned _handle_polling_network_error task continues running and will: (1) set _send_path_degraded=True (line 1756), which blocks all outbound message sends (line 3064 returns send_path_degraded); this flag persists across disconnect/reconnect cycles because _verify_polling_after_reconnect only resets it ~60s after reconnect; (2) attempt recovery operations (start_polling()) on an app that disconnect() is actively shutting down, risking a start/stop race inside PTB.
💡 Suggestion: Capture the _polling_error_task reference before the gather and only nullify it if the reference hasn't changed. For example: prev = self._polling_error_task; ... gather ...; if self._polling_error_task is prev: self._polling_error_task = None. This preserves any newly-created task that arrived during the gather window. Additionally, consider checking _should_drop_delayed_delivery() in the _polling_error_callback to suppress error recovery tasks during teardown.
📋 Prompt for AI Agents
In plugins/platforms/telegram/adapter.py in _cancel_pending_delivery_tasks(), capture prev = self._polling_error_task after the collect call and before the gather. After the gather, change lines 2984-2985 to only nullify if the reference still matches: if self._polling_error_task is prev: self._polling_error_task = None. This prevents orphaning a new error-recovery task created by the PTB callback during the gather window. Additionally, consider adding a _should_drop_delayed_delivery() guard at the top of _polling_error_callback (around line 2793) to suppress recovery task creation during teardown.
1 participant
Summary
Telegram no longer dispatches buffered messages into a torn-down session after disconnect. Salvage of NousResearch#17082 (@CRWuTJ), re-targeted onto current
mainwhere the adapter now lives atplugins/platforms/telegram/adapter.py.Root cause: text/photo/media-group flushes and the polling-error recovery task sit behind an
asyncio.sleep().disconnect()only cancelled the media-group and photo-batch tasks — the text-batch tasks and polling-error task leaked, and no flush had a drop guard, so any flush already past itssleep()still calledhandle_message()after teardown, spawning an agent on a dead session (stale / duplicate deliveries).Changes
plugins/platforms/telegram/adapter.py:_drop_delayed_deliveriesflag set by_mark_disconnected/_set_fatal_error, cleared by_mark_connected; checked in all 3 enqueue + 3 flush paths so a flush that loses the race to teardown drops instead of dispatching, and late update handlers don't schedule new delayed tasks during teardown._cancel_pending_delivery_tasks()cancels + clears all four task maps (media-group, photo, text, polling-error), skipping the current task; awaits only real awaitables.disconnect()calls_mark_disconnected()first, then the shared cancel helper (removes the two ad-hoc cancellation blocks).finallyblock guarded so a cancelled stale flush can't erase a replacement task handle.tests/gateway/test_telegram_text_batching.py: 9 regression tests (disconnect drops pending text/photo/media flushes, late enqueue dropped, stale media flush doesn't clear newer task, cancel helper skips current task, full disconnect clears all maps).scripts/release.py: AUTHOR_MAP entry for @CRWuTJ.Validation
-k telegram)test_telegram_text_batching.pyInfographic
Nous Research
Mirror-of: NousResearch#55971
NousResearch#55971