fix(photon): harden standalone send retries - #53831
Conversation
alt-glitch
added
type/bug
Related to the Photon outbound-recovery cluster — #51075 (iMessage outbound recovery), #50974 (degraded sidecar streams), #51897 (shared-line outbound). This PR hardens the standalone out-of-process send path specifically (a different facet), so not a duplicate; reviewers may want to land these together. |
40298e4 to
2c55fa1
Compare
|
Landing proof for maintainer merge:
No live Hermes runtime mutation, service restart, live config mutation, credential access, message send, or homelab mirror update was performed as part of this PR landing. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the standalone Photon delivery path. The current-main premise is valid: plugins/platforms/photon/adapter.py:1699-1706 returns the first failed standalone /send result without retrying.
Problems
- PR-head
plugins/platforms/photon/adapter.py:1641letshttpx.RequestErrorescapepost_sidecar(), bypassing the retry loop at:1664-1679and falling through to the outer error handler at:1724. This misses safe connection failures; shared policy classifies connection errors as retryable ingateway/platforms/base.py:2149-2166. - The new retry controls at PR-head
adapter.py:187-202are not added toplugins/platforms/photon/plugin.yamlor the README table, despiteREADME.md:108-110stating all env vars are documented there. - The unrelated inbound dedup timing change at PR-head
adapter.py:530-539has no focused test intests/plugins/platforms/photon/test_inbound.py.
Suggested changes
- Handle safe transport failures inside the retry path and add a connection-error retry test; preserve the existing no-retry treatment for ambiguous read/write timeouts.
- Document or re-home the retry settings, and separate or test the inbound dedup semantic change.
Automated hermes-sweeper review.
| path: str, | ||
| body: Dict[str, Any], | ||
| ) -> tuple[Optional[Dict[str, Any]], Optional[str]]: | ||
| resp = await client.post(f"{base}{path}", json=body, headers=headers) |
There was a problem hiding this comment.
httpx.RequestError from this call escapes post_sidecar() and therefore skips post_text_with_retry() entirely. Normalize safe connection failures here so they are evaluated by the retry loop; do not blindly retry read/write timeouts because those may already have delivered the message.
| } | ||
|
|
||
|
|
||
| def _standalone_retry_count() -> int: |
There was a problem hiding this comment.
These new runtime controls need documentation and setup metadata. plugins/platforms/photon/README.md:108-110 says all Photon env vars are documented in plugin.yaml, but neither new key is added there or to the README table.
| logger.exception("[photon] inbound dispatch failed") | ||
| return | ||
| if msg_id: | ||
| self._mark_seen(msg_id) |
There was a problem hiding this comment.
This changes inbound retransmission semantics: an event whose dispatch raises is no longer marked seen. Please add a focused failure-then-redelivery test in tests/plugins/platforms/photon/test_inbound.py, or split this unrelated inbound change from the standalone-send retry fix.
teknium1
added
sweeper:risk-compatibility
Summary
PHOTON_STANDALONE_SEND_RETRIESandPHOTON_STANDALONE_RETRY_BASE_DELAY_SECONDSContext
Standalone sends are used by out-of-process delivery paths that reuse an already-running Photon sidecar. A transient sidecar or upstream send failure could previously fail the whole delivery immediately. This adds the same kind of bounded retry behavior expected from connected sends, while keeping markdown delivery as the preferred path.
Tests
.venv/bin/python -m pytest tests/plugins/platforms/photon/test_markdown.py -q-> 8 passed.venv/bin/python -m pytest tests/plugins/platforms/photon/test_markdown.py tests/plugins/platforms/photon/test_outbound_media.py tests/plugins/platforms/photon/test_sidecar_lifecycle.py -q-> 21 passed