From 91c4c6f9d230cc5a6b41a8aee778511017125641 Mon Sep 17 00:00:00 2001 From: SeoYeonKim <28585885+westkite1201@users.noreply.github.com> Date: Tue, 23 Jun 2026 13:21:47 +0900 Subject: [PATCH] Preserve Photon sidecar retry semantics Photon's Node sidecar intentionally hides raw handler exceptions, but the Python adapter still needs a safe failure class and retryability bit so delivery retries do not collapse into an opaque generic 500. Constraint: Sidecar responses must not leak raw stack traces or private exception text Rejected: Retry every internal sidecar error | masks permanent auth/config failures Confidence: high Scope-risk: narrow Directive: Keep sidecar error text generic; extend safe error classes instead of exposing raw SDK failures Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon/test_overflow_recovery.py -q Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon -q Tested: uv run ruff check plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: python3 -m py_compile plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: node --check plugins/platforms/photon/sidecar/index.mjs Tested: git diff --check Tested: python3 scripts/check-windows-footguns.py --diff origin/main Not-tested: Live Photon/Spectrum delivery against a real iMessage account Related: #50971 --- plugins/platforms/photon/adapter.py | 85 ++++++++++- plugins/platforms/photon/sidecar/index.mjs | 53 ++++++- .../photon/test_overflow_recovery.py | 132 ++++++++++++++++++ 3 files changed, 259 insertions(+), 11 deletions(-) diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index 4d49ccbdacfd..97c82f6aa4b6 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -223,6 +223,53 @@ _DEFAULT_MENTION_PATTERNS = [ # --------------------------------------------------------------------------- # Module-level helpers — also used by check_fn / standalone send +class PhotonSidecarError(RuntimeError): + """Structured failure returned by the supervised Photon sidecar.""" + + def __init__( + self, + *, + path: str, + status_code: int, + error: str, + error_class: str = "sidecar_error", + retryable: bool = False, + ) -> None: + self.path = path + self.status_code = status_code + self.error = error + self.error_class = error_class + self.retryable = retryable + super().__init__( + f"Photon sidecar {path} returned {status_code} " + f"({error_class}, retryable={retryable}): {error}" + ) + + +def _sidecar_error_from_response( + path: str, + status_code: int, + text: str, + data: Optional[Dict[str, Any]] = None, +) -> PhotonSidecarError: + if data is None: + try: + parsed = json.loads(text) + except Exception: + parsed = {} + data = parsed if isinstance(parsed, dict) else {} + + error = str(data.get("error") or text[:200] or "sidecar error") + error_class = str(data.get("error_class") or "sidecar_error") + retryable = bool(data.get("retryable")) + return PhotonSidecarError( + path=path, + status_code=status_code, + error=error, + error_class=error_class, + retryable=retryable, + ) + def _coerce_port(value: Any, default: int) -> int: try: return int(value) @@ -1701,6 +1748,8 @@ class PhotonAdapter(BasePlatformAdapter): if not error: return False lowered = error.lower() + if "retryable=false" in lowered or "auth_or_config" in lowered: + return False return any(pat in lowered for pat in _PHOTON_RETRYABLE_PATTERNS) async def _send_with_retry( @@ -1728,6 +1777,14 @@ class PhotonAdapter(BasePlatformAdapter): if result.success: return result + sidecar_failure = result.raw_response + if ( + isinstance(sidecar_failure, dict) + and sidecar_failure.get("error_class") == "auth_or_config" + and sidecar_failure.get("retryable") is False + ): + return result + error_str = result.error or "" is_network = result.retryable or self._is_retryable_error(error_str) if not is_network and self._is_timeout_error(error_str): @@ -1787,6 +1844,16 @@ class PhotonAdapter(BasePlatformAdapter): body["format"] = "markdown" try: data = await self._sidecar_call("/send", body) + except PhotonSidecarError as e: + return SendResult( + success=False, + error=str(e), + raw_response={ + "error_class": e.error_class, + "retryable": e.retryable, + }, + retryable=e.retryable, + ) except Exception as e: return SendResult(success=False, error=str(e)) self._record_sent_message(data.get("messageId")) @@ -1836,6 +1903,16 @@ class PhotonAdapter(BasePlatformAdapter): body["caption"] = caption try: data = await self._sidecar_call("/send-attachment", body) + except PhotonSidecarError as e: + return SendResult( + success=False, + error=str(e), + raw_response={ + "error_class": e.error_class, + "retryable": e.retryable, + }, + retryable=e.retryable, + ) except Exception as e: return SendResult(success=False, error=str(e)) self._record_sent_message(data.get("messageId")) @@ -1855,14 +1932,10 @@ class PhotonAdapter(BasePlatformAdapter): async with httpx.AsyncClient(timeout=30.0, trust_env=False) as client: resp = await client.post(url, json=body, headers=headers) if resp.status_code != 200: - raise RuntimeError( - f"Photon sidecar {path} returned {resp.status_code}: {resp.text[:200]}" - ) + raise _sidecar_error_from_response(path, resp.status_code, resp.text) data = resp.json() or {} if not data.get("ok"): - raise RuntimeError( - f"Photon sidecar {path} reported error: {data.get('error')}" - ) + raise _sidecar_error_from_response(path, resp.status_code, resp.text, data) return data diff --git a/plugins/platforms/photon/sidecar/index.mjs b/plugins/platforms/photon/sidecar/index.mjs index 6fcc51170993..811afbea6b25 100644 --- a/plugins/platforms/photon/sidecar/index.mjs +++ b/plugins/platforms/photon/sidecar/index.mjs @@ -596,13 +596,56 @@ function badRequest(res, msg) { res.end(JSON.stringify({ ok: false, error: msg })); } -function serverError(res) { +function classifySidecarError(err) { + const message = String(err && err.message ? err.message : err || ""); + const lowered = message.toLowerCase(); + + if ( + lowered.includes("unauthorized") || + lowered.includes("forbidden") || + lowered.includes("permission") || + lowered.includes("401") || + lowered.includes("403") || + lowered.includes("invalid token") || + lowered.includes("project secret") || + lowered.includes("unable to resolve space id") + ) { + return { errorClass: "auth_or_config", retryable: false }; + } + + if ( + lowered.includes("timeout") || + lowered.includes("timed out") || + lowered.includes("econnreset") || + lowered.includes("econnrefused") || + lowered.includes("socket hang up") || + lowered.includes("fetch failed") || + lowered.includes("unavailable") || + lowered.includes("upstream connect") || + lowered.includes("reset reason") || + lowered.includes("overflow") || + lowered.includes("temporarily") || + lowered.includes("429") + ) { + return { errorClass: "upstream_transient", retryable: true }; + } + + return { errorClass: "sidecar_internal", retryable: false }; +} + +function serverError(res, err) { res.statusCode = 500; res.setHeader("Content-Type", "application/json"); + const classified = classifySidecarError(err); // Don't leak stack traces or raw exception text to the caller — even - // though we listen on loopback, the supervisor logs the real error - // and the client only needs a generic failure signal. - res.end(JSON.stringify({ ok: false, error: "internal sidecar error" })); + // though we listen on loopback, the supervisor logs the real error. The + // Python adapter only needs a safe class plus retryability. + res.end(JSON.stringify({ + ok: false, + error: "internal sidecar error", + error_class: classified.errorClass, + retryable: classified.retryable, + })); } function ok(res, data) { @@ -847,7 +890,7 @@ const server = http.createServer(async (req, res) => { ); // serverError() intentionally returns a generic message — see its // body for the rationale. - return serverError(res); + return serverError(res, e); } }); diff --git a/tests/plugins/platforms/photon/test_overflow_recovery.py b/tests/plugins/platforms/photon/test_overflow_recovery.py index 42db80cc4a96..abef5a97aebe 100644 --- a/tests/plugins/platforms/photon/test_overflow_recovery.py +++ b/tests/plugins/platforms/photon/test_overflow_recovery.py @@ -25,6 +25,8 @@ from typing import Any, Dict import pytest from gateway.config import PlatformConfig +from gateway.platforms.base import SendResult +from plugins.platforms.photon import adapter as photon_adapter from plugins.platforms.photon.adapter import PhotonAdapter @@ -63,6 +65,136 @@ def test_base_network_patterns_still_match() -> None: assert PhotonAdapter._is_retryable_error("ConnectError: connection refused") is True +def test_structured_non_retryable_sidecar_error_not_legacy_retried() -> None: + error = str( + photon_adapter.PhotonSidecarError( + path="/send", + status_code=500, + error="internal sidecar error", + error_class="auth_or_config", + retryable=False, + ) + ) + + assert PhotonAdapter._is_retryable_error(error) is False + + +@pytest.mark.asyncio +async def test_structured_sidecar_retryable_error_preserved( + monkeypatch: pytest.MonkeyPatch, +) -> None: + adapter = _make_adapter(monkeypatch) + adapter._http_client = object() + adapter._sidecar_bind = "127.0.0.1" + adapter._sidecar_port = 43210 + adapter._sidecar_token = "token" + + class _Resp: + status_code = 500 + text = ( + '{"ok":false,"error":"temporary upstream failure",' + '"error_class":"upstream_transient","retryable":true}' + ) + + @staticmethod + def json() -> Dict[str, Any]: + return { + "ok": False, + "error": "temporary upstream failure", + "error_class": "upstream_transient", + "retryable": True, + } + + class _FakeClient: + def __init__(self, *a: Any, **k: Any) -> None: + pass + + async def __aenter__(self) -> "_FakeClient": + return self + + async def __aexit__(self, *a: Any) -> bool: + return False + + async def post(self, *a: Any, **k: Any) -> _Resp: + return _Resp() + + monkeypatch.setattr(photon_adapter.httpx, "AsyncClient", _FakeClient) + + result = await adapter._sidecar_send("space-1", "hello") + + assert result.success is False + assert result.retryable is True + assert "upstream_transient" in (result.error or "") + assert "temporary upstream failure" in (result.error or "") + + +@pytest.mark.asyncio +async def test_send_with_retry_uses_structured_retryable_flag( + monkeypatch: pytest.MonkeyPatch, +) -> None: + adapter = _make_adapter(monkeypatch) + calls = 0 + sleeps: list[float] = [] + + async def _fake_sleep(delay: float) -> None: + sleeps.append(delay) + + async def _fake_sidecar_call(path: str, body: Dict[str, Any]) -> Dict[str, Any]: + nonlocal calls + calls += 1 + if calls == 1: + raise photon_adapter.PhotonSidecarError( + path=path, + status_code=500, + error="temporary upstream failure", + error_class="upstream_transient", + retryable=True, + ) + return {"ok": True, "messageId": "m-2"} + + monkeypatch.setattr(photon_adapter.asyncio, "sleep", _fake_sleep) + monkeypatch.setattr(adapter, "_sidecar_call", _fake_sidecar_call) + + result = await adapter._send_with_retry( + "space-1", "hello", max_retries=1, base_delay=0.25 + ) + + assert result.success is True + assert result.message_id == "m-2" + assert calls == 2 + assert sleeps == [0.25] + + +@pytest.mark.asyncio +async def test_send_with_retry_does_not_fallback_after_auth_or_config_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + adapter = _make_adapter(monkeypatch) + calls = 0 + + async def _permanent_failure(**kwargs: Any) -> SendResult: + nonlocal calls + calls += 1 + return SendResult( + success=False, + error="Photon sidecar /send returned 500 (auth_or_config)", + raw_response={ + "error_class": "auth_or_config", + "retryable": False, + }, + retryable=False, + ) + + monkeypatch.setattr(adapter, "send", _permanent_failure) + + result = await adapter._send_with_retry( + "space-1", "hello", max_retries=2, base_delay=0 + ) + + assert result.success is False + assert calls == 1 + + # -- Gap 2: typing-indicator cooldown --------------------------------------- @pytest.mark.asyncio