mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-08-31 16:21:50 +03:00
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).
This commit is contained in:
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user