-
Notifications
You must be signed in to change notification settings - Fork 49.5k
fix(photon): prevent duplicate iMessage replies on sidecar timeout #49718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,7 @@ | |
|
|
||
| _DEFAULT_SIDECAR_PORT = 8789 | ||
| _DEFAULT_SIDECAR_BIND = "127.0.0.1" | ||
| _DEFAULT_SIDECAR_SEND_TIMEOUT = 120.0 | ||
|
|
||
| # Photon iMessage messages from the SDK side have no documented hard | ||
| # limit, but the underlying iMessage protocol limits practical message | ||
|
|
@@ -176,6 +177,11 @@ class PhotonAdapter(BasePlatformAdapter): | |
| Outbound: loopback POSTs to the sidecar's control channel. | ||
| """ | ||
|
|
||
| # iMessage has no edit API — streaming would emit a partial bubble then | ||
| # fall back to full sends for the final answer (duplicate messages). | ||
| # Match BlueBubbles: one delivery when the turn completes. | ||
| SUPPORTS_MESSAGE_EDITING = False | ||
|
|
||
| MAX_MESSAGE_LENGTH = _MAX_MESSAGE_LENGTH | ||
|
|
||
| def __init__(self, config: PlatformConfig): | ||
|
|
@@ -1245,19 +1251,45 @@ async def _send_with_retry( | |
| ) | ||
| return result | ||
|
|
||
| logger.warning( | ||
| "[photon] Send failed: %s - retrying plain-text message", | ||
| error_str, | ||
| # Do not send a second bubble. iMessage has no edit API and Photon | ||
| # markdown/plain payloads are the same text — the base adapter's | ||
| # plain-text fallback only creates duplicate replies. Timeouts and | ||
| # upstream/sidecar failures are especially dangerous: the first send | ||
| # may have been delivered while our HTTP client gave up waiting. | ||
| if self._is_ambiguous_photon_delivery_error(error_str): | ||
| logger.error( | ||
| "[photon] Send failed (not sending duplicate): %s", | ||
| error_str or "(empty error)", | ||
| ) | ||
| return result | ||
|
|
||
| logger.error( | ||
| "[photon] Send failed (not retrying duplicate bubble): %s", | ||
| error_str or "(empty error)", | ||
| ) | ||
| fallback_result = await self.send( | ||
| chat_id=chat_id, | ||
| content=text[: self.MAX_MESSAGE_LENGTH], | ||
| reply_to=reply_to, | ||
| metadata=metadata, | ||
| return result | ||
|
|
||
| @staticmethod | ||
| def _is_ambiguous_photon_delivery_error(error: Optional[str]) -> bool: | ||
| """True when a retry would likely duplicate an already-delivered bubble.""" | ||
| if not error: | ||
| # httpx.ReadTimeout often stringifies to '' — treat as ambiguous. | ||
| return True | ||
| lowered = error.lower() | ||
| return any( | ||
| pat in lowered | ||
| for pat in ( | ||
| "timed out", | ||
| "timeout", | ||
| "readtimeout", | ||
| "writetimeout", | ||
| "internal sidecar", | ||
| "upstream", | ||
| "service temporarily unavailable", | ||
| "resource_exhausted", | ||
| "concurrency limit", | ||
| ) | ||
| ) | ||
| if not fallback_result.success: | ||
| logger.error("[photon] Plain-text retry also failed: %s", fallback_result.error) | ||
| return fallback_result | ||
|
|
||
| async def _sidecar_send(self, space_id: str, text: str) -> SendResult: | ||
| if len(text) > self.MAX_MESSAGE_LENGTH: | ||
|
|
@@ -1274,7 +1306,12 @@ async def _sidecar_send(self, space_id: str, text: str) -> SendResult: | |
| try: | ||
| data = await self._sidecar_call("/send", body) | ||
| except Exception as e: | ||
| return SendResult(success=False, error=str(e)) | ||
| err = str(e) | ||
| if not err and HTTPX_AVAILABLE and isinstance(e, httpx.TimeoutException): | ||
| err = "Photon sidecar /send timed out (read timeout)" | ||
| elif not err: | ||
| err = type(e).__name__ | ||
| return SendResult(success=False, error=err, retryable=False) | ||
| self._record_sent_message(data.get("messageId")) | ||
| return SendResult(success=True, message_id=data.get("messageId")) | ||
|
|
||
|
|
@@ -1338,7 +1375,17 @@ async def _sidecar_call(self, path: str, body: Dict[str, Any]) -> Dict[str, Any] | |
| # _http_client directly — it always runs on the gateway's loop. | ||
| url = f"http://{self._sidecar_bind}:{self._sidecar_port}{path}" | ||
| headers = {"X-Hermes-Sidecar-Token": self._sidecar_token} | ||
| async with httpx.AsyncClient(timeout=30.0) as client: | ||
| # Long markdown replies can sit in spectrum-ts/upstream longer than 30s; | ||
| # timing out client-side and re-sending created duplicate iMessage bubbles. | ||
| default_send_timeout = _DEFAULT_SIDECAR_SEND_TIMEOUT | ||
| try: | ||
| default_send_timeout = float( | ||
| os.getenv("PHOTON_SIDECAR_SEND_TIMEOUT", str(_DEFAULT_SIDECAR_SEND_TIMEOUT)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This introduces a user-facing non-secret behavioral environment variable. Repository policy requires timeout/configuration knobs to use |
||
| ) | ||
| except (TypeError, ValueError): | ||
| default_send_timeout = _DEFAULT_SIDECAR_SEND_TIMEOUT | ||
| timeout = default_send_timeout if path in ("/send", "/send-attachment") else 30.0 | ||
| async with httpx.AsyncClient(timeout=timeout) as client: | ||
| resp = await client.post(url, json=body, headers=headers) | ||
| if resp.status_code != 200: | ||
| raise RuntimeError( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| """Outbound delivery tests for PhotonAdapter. | ||
|
|
||
| Guards against duplicate iMessage bubbles when the sidecar/upstream is slow | ||
| or the loopback HTTP client times out with an empty error string. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Any, Dict, List | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| import pytest | ||
|
|
||
| from gateway.config import PlatformConfig | ||
| from gateway.platforms.base import SendResult | ||
| from plugins.platforms.photon.adapter import PhotonAdapter | ||
|
|
||
|
|
||
| def _make_adapter(monkeypatch: pytest.MonkeyPatch) -> PhotonAdapter: | ||
| monkeypatch.setenv("PHOTON_PROJECT_ID", "test-project-id") | ||
| monkeypatch.setenv("PHOTON_PROJECT_SECRET", "test-project-secret") | ||
| cfg = PlatformConfig(enabled=True, token="", extra={}) | ||
| return PhotonAdapter(cfg) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "error,expected", | ||
| [ | ||
| ("", True), | ||
| ("Photon sidecar /send timed out (read timeout)", True), | ||
| ("[upstream] Service temporarily unavailable. Please retry.", True), | ||
| ("Photon sidecar /send returned 500: internal sidecar error", True), | ||
| ("permission denied for chat", False), | ||
| ], | ||
| ) | ||
| def test_is_ambiguous_photon_delivery_error( | ||
| error: str, expected: bool, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| adapter = _make_adapter(monkeypatch) | ||
| assert adapter._is_ambiguous_photon_delivery_error(error) is expected | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_send_with_retry_does_not_duplicate_on_empty_error( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| adapter = _make_adapter(monkeypatch) | ||
| send_calls: List[str] = [] | ||
|
|
||
| async def _fake_send(**kwargs: Any) -> SendResult: | ||
| send_calls.append(kwargs.get("content", "")) | ||
| return SendResult(success=False, error="") | ||
|
|
||
| adapter.send = AsyncMock(side_effect=_fake_send) # type: ignore[method-assign] | ||
|
|
||
| result = await adapter._send_with_retry("+15555550100", "hello **world**") | ||
|
|
||
| assert result.success is False | ||
| assert len(send_calls) == 1 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_send_with_retry_does_not_plaintext_fallback( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| adapter = _make_adapter(monkeypatch) | ||
| send_calls: List[str] = [] | ||
|
|
||
| async def _fake_send(**kwargs: Any) -> SendResult: | ||
| send_calls.append(str(kwargs.get("content", ""))) | ||
| return SendResult( | ||
| success=False, | ||
| error="Photon sidecar /send returned 500: internal sidecar error", | ||
| ) | ||
|
|
||
| adapter.send = AsyncMock(side_effect=_fake_send) # type: ignore[method-assign] | ||
|
|
||
| await adapter._send_with_retry("+15555550100", "long reply") | ||
|
|
||
| assert len(send_calls) == 1 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_sidecar_send_timeout_sets_non_empty_error( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| import httpx | ||
|
|
||
| adapter = _make_adapter(monkeypatch) | ||
| adapter._http_client = object() # type: ignore[assignment] | ||
|
|
||
| async def _raise_timeout(path: str, body: Dict[str, Any]) -> Dict[str, Any]: | ||
| raise httpx.ReadTimeout("") | ||
|
|
||
| adapter._sidecar_call = _raise_timeout # type: ignore[assignment] | ||
|
|
||
| result = await adapter._sidecar_send("any;-;+15555550100", "hi") | ||
|
|
||
| assert result.success is False | ||
| assert result.retryable is False | ||
| assert result.error | ||
| assert "timed out" in (result.error or "").lower() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_sidecar_call_uses_longer_timeout_for_send( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| import plugins.platforms.photon.adapter as photon_adapter | ||
|
|
||
| adapter = _make_adapter(monkeypatch) | ||
| adapter._http_client = object() # type: ignore[assignment] | ||
|
|
||
| captured: List[float] = [] | ||
|
|
||
| class _Resp: | ||
| status_code = 200 | ||
|
|
||
| @staticmethod | ||
| def json() -> Dict[str, Any]: | ||
| return {"ok": True, "messageId": "m-1"} | ||
|
|
||
| class _FakeClient: | ||
| def __init__(self, *a: Any, timeout: float = 30.0, **k: Any): | ||
| captured.append(timeout) | ||
|
|
||
| async def __aenter__(self): | ||
| return self | ||
|
|
||
| async def __aexit__(self, *a: Any): | ||
| return False | ||
|
|
||
| async def post(self, url: str, json: Dict[str, Any], headers=None): | ||
| return _Resp() | ||
|
|
||
| monkeypatch.setattr(photon_adapter.httpx, "AsyncClient", _FakeClient) | ||
| monkeypatch.delenv("PHOTON_SIDECAR_SEND_TIMEOUT", raising=False) | ||
|
|
||
| await adapter._sidecar_call("/send", {"spaceId": "x", "text": "y"}) | ||
| await adapter._sidecar_call("/typing", {"spaceId": "x", "state": "start"}) | ||
|
|
||
| assert captured == [120.0, 30.0] |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard is reached only after the retry loop above has already resent at line 1236. Current main classifies
internal sidecar errorand upstream failures as retryable, so those errors still get a second bubble before this check. Check ambiguity immediately after a failedsend()and beforeis_network/the retry loop.