fix(telegram): stop typing refresh before final delivery - #64719
fix(telegram): stop typing refresh before final delivery#64719seewilds wants to merge 2 commits into
Conversation
Related: this enters the Telegram typing-indicator-race cluster. The most comprehensive open approach is #29172 (per-chat |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Fixes a Telegram UX bug: the typing indicator was being refreshed (reset) immediately before the final message delivery, causing a race where the indicator disappears before the message lands.
Assessment
- Correctness: Stops the pre-delivery refresh — prevents the race condition cleanly.
- Security: No security-sensitive changes.
- Debug artifacts: None found.
Summary
Small, targeted fix. LGTM.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the final-delivery typing race and adding a deterministic ordering regression. The race is present on current main: gateway/platforms/base.py:4908 starts _keep_typing, while the existing stop remains in final cleanup at gateway/platforms/base.py:5303, after delivery.
Problems
gateway/platforms/base.py:5036adds a pre-delivery await of_stop_typing_task(), but current_stop_typing_refresh()catchesCancelledErroratgateway/platforms/base.py:3945without distinguishing caller cancellation from the expected child-task cancellation. This can let a cancelled/stale processor continue into final delivery.tests/gateway/test_typing_indicator_toggle.py:104verifies ordering only; it does not cover cancellation while the newly pre-delivery typing shutdown is in progress.
Suggested changes
- Preserve and re-raise caller cancellation after typing cleanup, while swallowing only the cancelled refresh task. Related PR #29172 contains a focused implementation and regression coverage for this distinction.
- Add a cancellation regression proving no final text or attachment send starts after cancellation at this boundary.
Automated hermes-sweeper review.
| # Stop the refresh loop before the first final delivery. Some | ||
| # platforms clear typing when a message arrives, so a concurrent | ||
| # refresh can otherwise race the send and re-arm the indicator. | ||
| await _stop_typing_task() |
There was a problem hiding this comment.
This newly pre-delivery await makes caller cancellation safety load-bearing, but _stop_typing_refresh() currently catches CancelledError from the shielded wait without distinguishing cancellation of this processing task from expected child-task cancellation (gateway/platforms/base.py:3941-3948 on main). Preserve and re-raise caller cancellation here so a stale run cannot continue into final delivery.
There was a problem hiding this comment.
Thanks — addressed in commit 2b6593d. After the typing stop returns, the processor now checks its own cancellation state and re-raises CancelledError before TTS, text, or attachments can be delivered. Keeping the guard at the delivery boundary preserves the shared helper’s existing child-cancellation cleanup behavior; the new regression uses a text + PDF response and asserts neither send starts after cancellation.
Summary
_keep_typingrefresh task before the first final user-visible deliveryWhy
Commit 565b7c8 fixed #48678 by preventing
TelegramAdapter.send()from directly callingsend_typing()whenmetadata["notify"]marks a final response. That guard is necessary but insufficient:BasePlatformAdapter._process_message_background()independently starts a generic_keep_typingtask with the unmarked thread metadata.That task runs concurrently with final delivery. Telegram clears the current typing action when
sendMessagearrives, but the refresh loop can race the delivery and issue anothersendChatActionafterward, re-arming the indicator after the final reply.The refresh task is now stopped after any auto-TTS generation but before
play_tts, text, image, media, or file-only delivery. Intermediate and progress messages retain typing because the loop remains active until this final-delivery boundary. The existing outerfinallycleanup remains in place as an idempotent safety net.The new test starts a fast refresh loop, waits until typing is active, makes final delivery deliberately slow, and asserts that no typing calls occur once delivery begins. It fails on current
mainand passes with this fix.Tests
scripts/run_tests.sh tests/gateway/test_typing_indicator_toggle.py tests/gateway/test_telegram_format.py -q— 112 passedscripts/run_tests.sh tests/gateway/test_keep_typing_timeout.py tests/gateway/test_tts_media_routing.py tests/gateway/test_media_metadata_contract.py tests/gateway/test_tool_response_drop_recovery.py -q— 38 passeduv run ruff check gateway/platforms/base.py tests/gateway/test_typing_indicator_toggle.py— passedProduction verification
The fix was verified against a live Telegram gateway after restart; the phantom typing indicator disappeared after final replies.