fix(channels): prevent retry amplification and silent message loss across channels
Audited all channel implementations for overly broad exception handling that causes retry amplification or silent message loss during network errors. This is the same class of bug as #3050 (Telegram _send_text). Fixes by channel: Telegram (send_delta): - _stream_end path used except Exception for HTML edit fallback - Network errors (TimedOut, NetworkError) triggered redundant plain text edit, doubling connection demand during pool exhaustion - Changed to except BadRequest, matching the _send_text fix Discord: - send() caught all exceptions without re-raising - ChannelManager._send_with_retry() saw successful return, never retried - Messages silently dropped on any send failure - Added raise after error logging DingTalk: - _send_batch_message() returned False on all exceptions including network errors — no retry, fallback text sent unnecessarily - _read_media_bytes() and _upload_media() swallowed transport errors, causing _send_media_ref() to cascade through doomed fallback attempts - Added except httpx.TransportError handlers that re-raise immediately WeChat: - Media send failure triggered text fallback even for network errors - During network issues: 3×(media + text) = 6 API calls per message - Added specific catches: TimeoutException/TransportError re-raise, 5xx HTTPStatusError re-raises, 4xx falls back to text QQ: - _send_media() returned False on all exceptions - Network errors triggered fallback text instead of retry - Added except (aiohttp.ClientError, OSError) that re-raises Tests: 331 passed (283 existing + 48 new across 5 channel test files) Fixes: #3054 Related: #3050, #3053
This commit is contained in:
@@ -867,3 +867,100 @@ async def test_start_no_proxy_auth_when_only_password(monkeypatch) -> None:
|
||||
assert channel.is_running is False
|
||||
assert _FakeDiscordClient.instances[0].proxy == "http://127.0.0.1:7890"
|
||||
assert _FakeDiscordClient.instances[0].proxy_auth is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for the send() exception propagation fix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_re_raises_network_error() -> None:
|
||||
"""Network errors during send must propagate so ChannelManager can retry."""
|
||||
channel = DiscordChannel(DiscordConfig(enabled=True, allow_from=["*"]), MessageBus())
|
||||
client = _FakeDiscordClient(channel, intents=None)
|
||||
channel._client = client
|
||||
channel._running = True
|
||||
|
||||
async def _failing_send_outbound(msg: OutboundMessage) -> None:
|
||||
raise ConnectionError("network unreachable")
|
||||
|
||||
client.send_outbound = _failing_send_outbound # type: ignore[method-assign]
|
||||
|
||||
with pytest.raises(ConnectionError, match="network unreachable"):
|
||||
await channel.send(OutboundMessage(channel="discord", chat_id="123", content="hello"))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_re_raises_generic_exception() -> None:
|
||||
"""Any exception from send_outbound must propagate, not be swallowed."""
|
||||
channel = DiscordChannel(DiscordConfig(enabled=True, allow_from=["*"]), MessageBus())
|
||||
client = _FakeDiscordClient(channel, intents=None)
|
||||
channel._client = client
|
||||
channel._running = True
|
||||
|
||||
async def _failing_send_outbound(msg: OutboundMessage) -> None:
|
||||
raise RuntimeError("discord API failure")
|
||||
|
||||
client.send_outbound = _failing_send_outbound # type: ignore[method-assign]
|
||||
|
||||
with pytest.raises(RuntimeError, match="discord API failure"):
|
||||
await channel.send(OutboundMessage(channel="discord", chat_id="123", content="hello"))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_still_stops_typing_on_error() -> None:
|
||||
"""Typing cleanup must still run in the finally block even when send raises."""
|
||||
channel = DiscordChannel(DiscordConfig(enabled=True, allow_from=["*"]), MessageBus())
|
||||
client = _FakeDiscordClient(channel, intents=None)
|
||||
channel._client = client
|
||||
channel._running = True
|
||||
|
||||
# Start a typing task so we can verify it gets cleaned up
|
||||
start = asyncio.Event()
|
||||
release = asyncio.Event()
|
||||
|
||||
async def slow_typing() -> None:
|
||||
start.set()
|
||||
await release.wait()
|
||||
|
||||
typing_channel = _FakeChannel(channel_id=123)
|
||||
typing_channel.typing_enter_hook = slow_typing
|
||||
await channel._start_typing(typing_channel)
|
||||
await asyncio.wait_for(start.wait(), timeout=1.0)
|
||||
|
||||
async def _failing_send_outbound(msg: OutboundMessage) -> None:
|
||||
raise ConnectionError("timeout")
|
||||
|
||||
client.send_outbound = _failing_send_outbound # type: ignore[method-assign]
|
||||
|
||||
with pytest.raises(ConnectionError, match="timeout"):
|
||||
await channel.send(OutboundMessage(channel="discord", chat_id="123", content="hello"))
|
||||
|
||||
release.set()
|
||||
await asyncio.sleep(0)
|
||||
|
||||
# Typing should have been cleaned up by the finally block
|
||||
assert channel._typing_tasks == {}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_succeeds_normally() -> None:
|
||||
"""Successful sends should work without raising."""
|
||||
channel = DiscordChannel(DiscordConfig(enabled=True, allow_from=["*"]), MessageBus())
|
||||
client = _FakeDiscordClient(channel, intents=None)
|
||||
channel._client = client
|
||||
channel._running = True
|
||||
|
||||
sent_messages: list[OutboundMessage] = []
|
||||
|
||||
async def _capture_send_outbound(msg: OutboundMessage) -> None:
|
||||
sent_messages.append(msg)
|
||||
|
||||
client.send_outbound = _capture_send_outbound # type: ignore[method-assign]
|
||||
|
||||
msg = OutboundMessage(channel="discord", chat_id="123", content="hello world")
|
||||
await channel.send(msg)
|
||||
|
||||
assert len(sent_messages) == 1
|
||||
assert sent_messages[0].content == "hello world"
|
||||
assert sent_messages[0].chat_id == "123"
|
||||
|
||||
Reference in New Issue
Block a user