From 195e4c281d2631f0fd2fbd4ec6c9d5835eeb0b82 Mon Sep 17 00:00:00 2001 From: Nolan Date: Mon, 31 Aug 2026 16:53:57 +0800 Subject: [PATCH] fix(telegram): preserve final-edit retry contract in rich stream upgrade Address review feedback on the rich edit error classification: - Transport, rate-limit, and unexpected errors now propagate instead of returning False. Returning False made send_delta fall through to an immediate legacy edit_message_text, which under connection-pool exhaustion doubled demand and discarded the buffered retry state that ChannelManager relies on. - 'Message is not modified' is treated as success: when the rich edit is applied server-side but its response times out, the retry inside _call_with_retry reports the edit as already applied, and the previous BadRequest branch would have let the legacy edit overwrite the successful rich result. - False is now returned only for capability errors (pre-10.1 servers, which still trip the rich latch) and content-shaped rejections, where the legacy HTML path is the intended fallback. Adds regression coverage for a direct NetworkError (propagates, buffer kept for manager retry) and TimedOut followed by Message-is-not-modified (treated as success, no legacy overwrite). --- nanobot/channels/telegram/runtime.py | 37 ++++++++++---- .../telegram/tests/test_telegram_channel.py | 51 +++++++++++++++++++ 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/nanobot/channels/telegram/runtime.py b/nanobot/channels/telegram/runtime.py index 4dbfec2b1..090a38914 100644 --- a/nanobot/channels/telegram/runtime.py +++ b/nanobot/channels/telegram/runtime.py @@ -902,9 +902,16 @@ class TelegramChannel(BaseChannel): Editing in place keeps the message identity, so the streaming preview is upgraded without the delete-and-resend pattern that caused flickering and - dropped line breaks (issue #4470). Returns True on success; on capability - errors (server older than Bot API 10.1) the rich latch is tripped so the - legacy HTML path is used from then on. + dropped line breaks (issue #4470). + + Returns True when the rich edit is in place (including the ambiguous + "message is not modified" retry outcome after a response timeout). + Returns False only when the legacy HTML path should take over: + capability errors (server older than Bot API 10.1, which also trip the + rich latch) and content-shaped BadRequest rejections. Transport, + rate-limit, and unexpected errors propagate so the final-edit retry + contract is preserved — ChannelManager retries the buffered send + instead of an immediate legacy edit doubling connection demand. """ if not self._app: return False @@ -924,19 +931,27 @@ class TelegramChannel(BaseChannel): ) return True except BadRequest as exc: + if self._is_not_modified_error(exc): + # Ambiguous success: the rich edit was applied server-side but + # its response timed out, so the retry hit "message is not + # modified". Treat it as done rather than letting the legacy + # edit overwrite the already-successful rich result. + self.logger.debug("Rich stream edit already applied for {}", chat_id) + return True if self._is_rich_capability_error(exc): self.logger.debug("editMessageText rich_message not available, disabling") self._rich_send_disabled = True - else: - self.logger.debug("editMessageText rich_message rejected: {}", exc) - return False - except Exception as exc: - err_str = str(exc).lower() - if "timed out" in err_str or isinstance(exc, TimedOut): - self.logger.debug("editMessageText rich_message timeout, falling back to legacy path") return False - self.logger.debug("editMessageText rich_message failed: {}", exc) + # Content-shaped rejections (invalid markdown, unsupported media in + # the rich payload, …) fall back to the legacy HTML edit. + self.logger.debug("editMessageText rich_message rejected: {}", exc) return False + except Exception: + # Transport, rate-limit, and unexpected errors propagate so the + # final-edit retry contract stays intact: ChannelManager retries + # the buffered send instead of this handler doubling connection + # demand with an immediate legacy edit. + raise async def send(self, msg: OutboundMessage) -> None: """Send a message through Telegram.""" diff --git a/nanobot/channels/telegram/tests/test_telegram_channel.py b/nanobot/channels/telegram/tests/test_telegram_channel.py index 2c10ae119..d4240c4f9 100644 --- a/nanobot/channels/telegram/tests/test_telegram_channel.py +++ b/nanobot/channels/telegram/tests/test_telegram_channel.py @@ -2808,3 +2808,54 @@ async def test_send_delta_stream_end_rich_disabled_uses_legacy_html() -> None: channel._app.bot.do_api_request.assert_not_called() channel._app.bot.edit_message_text.assert_awaited_once() assert "123" not in channel._stream_bufs + + +@pytest.mark.asyncio +async def test_send_delta_stream_end_rich_network_error_propagates_for_retry() -> None: + """A transport failure on the rich edit must propagate so ChannelManager + retries the buffered send — not fall through to an immediate legacy edit + that doubles connection demand during pool exhaustion.""" + from telegram.error import NetworkError + + channel = TelegramChannel( + TelegramConfig(enabled=True, token="123:abc", allow_from=["*"], rich_messages=True), + MessageBus(), + ) + _install_ready_app(channel) + channel._app.bot.do_api_request = AsyncMock(side_effect=NetworkError("pool exhausted")) + channel._app.bot.edit_message_text = AsyncMock() + channel._stream_bufs["123"] = _StreamBuf(text="hello", message_id=7, last_edit=0.0) + + with pytest.raises(NetworkError): + await channel.send_delta("123", "", stream_end=True) + + # No legacy fallback edit: the buffered state stays for the manager retry. + channel._app.bot.edit_message_text.assert_not_awaited() + assert "123" in channel._stream_bufs + + +@pytest.mark.asyncio +async def test_send_delta_stream_end_rich_not_modified_after_timeout_is_success() -> None: + """Ambiguous success: the rich edit applied server-side but its response + timed out, so the retry hit "message is not modified". That is a completed + rich upgrade — the legacy edit must not overwrite it.""" + from telegram.error import BadRequest, TimedOut + + channel = TelegramChannel( + TelegramConfig(enabled=True, token="123:abc", allow_from=["*"], rich_messages=True), + MessageBus(), + ) + _install_ready_app(channel) + # First attempt (inside _call_with_retry) times out, retry reports the + # edit as already applied. + channel._app.bot.do_api_request = AsyncMock( + side_effect=[TimedOut(), BadRequest("Message is not modified")] + ) + channel._app.bot.edit_message_text = AsyncMock(side_effect=AssertionError("must not overwrite rich result")) + channel._stream_bufs["123"] = _StreamBuf(text="hello", message_id=7, last_edit=0.0) + + await channel.send_delta("123", "", stream_end=True) + + assert channel._app.bot.do_api_request.await_count == 2 + channel._app.bot.edit_message_text.assert_not_awaited() + assert "123" not in channel._stream_bufs