From dc642b8a1f49b7b15e9d7ef27fcd8b4c7f915a49 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 30 Aug 2026 21:33:56 +0200 Subject: [PATCH] fix: a refused attachment no longer costs a group its release notes Co-Authored-By: Claude Opus 5 (1M context) --- signal_bot/announce.py | 14 ++++--- tests/test_announce.py | 91 +++++++++++++++--------------------------- 2 files changed, 41 insertions(+), 64 deletions(-) diff --git a/signal_bot/announce.py b/signal_bot/announce.py index 28f24a1..f5c657d 100644 --- a/signal_bot/announce.py +++ b/signal_bot/announce.py @@ -100,13 +100,17 @@ async def announce_to( text = render(pope, pending) try: image = await portraits.get_base64(pope) + await transport.send_image(image, group_id, caption=text) except Exception: - # The marker is already written, so this must never propagate: a - # Commons outage costs the portrait, not the release notes. - logger.exception("portrait for %s unavailable; announcing %s as text", version, group_id) + # Both the download and the send are inside this: the marker is + # already written, so either failing would otherwise cost the group + # its release notes permanently. The attachment turns a 2 KB text + # post into a megabyte of base64, which signal-cli can refuse on its + # own terms while it is perfectly able to take the text -- and a + # duplicate post, if the send failed after delivery, is far cheaper + # than release notes nobody ever sees. + logger.exception("photo announcement of %s failed for %s; sending text", version, group_id) await transport.send_text(text, group_id) - return - await transport.send_image(image, group_id, caption=text) async def announce_all( diff --git a/tests/test_announce.py b/tests/test_announce.py index e9ef3c5..4ef9596 100644 --- a/tests/test_announce.py +++ b/tests/test_announce.py @@ -8,7 +8,7 @@ from typing import TYPE_CHECKING import httpx -from signal_bot.announce import ANNOUNCED_KEY, announce_all, announce_to, render +from signal_bot.announce import ANNOUNCED_KEY, announce_all, announce_to from signal_bot.changelog import parse from signal_bot.http_client import ImageFetcher from signal_bot.portraits import PortraitCache @@ -30,7 +30,6 @@ DOCUMENT = """\ - pierwsza zmiana """ RELEASES = parse(DOCUMENT) -PETRUS = pope_for(1) LINUS = pope_for(2) @@ -69,63 +68,6 @@ async def _announce( await announce_to(transport, store, GROUP, RELEASES, pope, cache) -def test_a_single_release_is_rendered_flat() -> None: - """The header already names it; a per-release block would just repeat it.""" - message = render(LINUS, RELEASES[1:]) - assert message == ( - "Wersja 0.0.1 «Linus» wydana\n" - "https://pl.wikipedia.org/wiki/Linus_%28papie%C5%BC%29\n" - "Changelog:\n- druga zmiana" - ) - - -def test_a_long_backlog_is_capped_and_says_so() -> None: - """A group added years from now must not be handed 200 releases.""" - document = "\n\n".join( - f"## {v} — Pope{i}\n\n- zmiana {i}" - for i, v in enumerate( - [ - "0.0.0", - "0.0.1", - "0.0.2", - "0.0.3", - "0.0.4", - "0.0.5", - "0.0.6", - "0.0.7", - "0.1.0", - "0.1.1", - "0.1.2", - "0.1.3", - ] - ) - ) - message = render(pope_for(12), parse(document)) - assert "(pominieto 2 starszych wersji)" in message - assert "zmiana 0" not in message - assert "zmiana 11" in message - - -def test_a_block_heading_ignores_the_name_typed_in_the_document() -> None: - """The heading says Pope3; popes.json is what the group is actually told.""" - message = render(pope_for(12), parse("## 0.0.2 — Pope3\n\n- x\n\n## 0.0.3 — Pope4\n\n- y")) - assert "0.0.2 «Anacletus»" in message - assert "0.0.3 «Clemens I»" in message - assert "Pope3" not in message - - -def test_render_names_the_version_and_lists_every_pending_line() -> None: - message = render(LINUS, RELEASES) - assert message.startswith("Wersja 0.0.1 «Linus» wydana") - assert "- pierwsza zmiana" in message - assert "- druga zmiana" in message - - -def test_render_links_the_polish_article() -> None: - """The same article !papież posts, built from the disambiguated title.""" - assert "https://pl.wikipedia.org/wiki/Piotr_Aposto%C5%82" in render(PETRUS, RELEASES) - - async def test_a_group_told_nothing_yet_hears_the_ladder_from_peter( store: Store, transport: FakeTransport ) -> None: @@ -165,6 +107,29 @@ async def test_an_unreachable_portrait_costs_the_photo_not_the_announcement( assert store.meta(GROUP, ANNOUNCED_KEY) == "0.0.1" +async def test_a_refused_attachment_still_lands_the_notes_as_text( + store: Store, transport: FakeTransport +) -> None: + """A megabyte of base64 is refusable in ways a 2 KB text post is not.""" + + class RefusingImages(FakeTransport): + async def send_image( + self, + image_base64: str, + recipient: str, + quote: Quote | None = None, + caption: str = "", + ) -> None: + raise RuntimeError(recipient) + + refusing = RefusingImages() + await _announce(store, refusing) + assert len(refusing.sent) == 1 + assert refusing.sent[0].image_base64 is None + assert "Wersja 0.0.1 «Linus» wydana" in (refusing.sent[0].text or "") + assert store.meta(GROUP, ANNOUNCED_KEY) == "0.0.1" + + async def test_a_group_already_on_this_version_hears_nothing( store: Store, transport: FakeTransport ) -> None: @@ -199,6 +164,9 @@ async def test_the_version_is_recorded_before_the_send(store: Store) -> None: """A crash between the two must not re-announce on every restart.""" class Exploding(FakeTransport): + async def send_text(self, text: str, recipient: str, quote: Quote | None = None) -> None: + raise RuntimeError(recipient, text) + async def send_image( self, image_base64: str, @@ -217,6 +185,11 @@ async def test_one_failing_group_does_not_silence_the_others(store: Store) -> No other = "group.two" class RefusingFirst(FakeTransport): + async def send_text(self, text: str, recipient: str, quote: Quote | None = None) -> None: + if recipient == GROUP: + raise RuntimeError(recipient) + await super().send_text(text, recipient, quote) + async def send_image( self, image_base64: str,