Skip to content

fix(photon): prevent duplicate iMessage replies on sidecar timeout - #49718

Closed
tsale wants to merge 1 commit into
NousResearch:mainfrom
tsale:fix/photon-imessage-duplicate-send
Closed

fix(photon): prevent duplicate iMessage replies on sidecar timeout#49718
tsale wants to merge 1 commit into
NousResearch:mainfrom
tsale:fix/photon-imessage-duplicate-send

Conversation

@tsale

@tsale tsale commented Jun 20, 2026

Copy link
Copy Markdown

Problem

Users on the Photon (iMessage) channel sometimes receive two identical agent replies for a single turn, especially on longer responses.

Root cause

  1. Gateway calls sidecar POST /send; sidecar awaits space.send() via spectrum-ts/upstream.
  2. Python httpx client used a 30s timeout on all sidecar calls.
  3. On timeout, str(httpx.ReadTimeout("")) is often empty, so _is_timeout_error() did not apply.
  4. PhotonAdapter._send_with_retry then ran the plain-text fallback, which re-sent the same text as a second iMessage bubble.

Typical log pattern (~30s after Sending response):

[Photon] Sending response (NNNN chars) to <chat>
[photon] Send failed:  - retrying plain-text message
photon-sidecar: handler error: ConnectionError: [upstream] Service temporarily unavailable

Fix

  • No second bubble on Photon: remove plain-text fallback (iMessage cannot edit bubbles; markdown/plain are the same content).
  • _is_ambiguous_photon_delivery_error: empty errors, timeouts, upstream/sidecar failures → do not resend.
  • PHOTON_SIDECAR_SEND_TIMEOUT (default 120s) for /send and /send-attachment.
  • Normalize timeout exceptions to a non-empty error with retryable=False.
  • SUPPORTS_MESSAGE_EDITING = False on Photon (parity with BlueBubbles; avoids streaming partial + final duplicate).

Tradeoff

When delivery is ambiguous (timeout/upstream), we fail closed (no duplicate) rather than risk a second bubble. The user may occasionally see no message if the first attempt never delivered; that is preferable to duplicate replies on a personal channel.

Test plan

pytest tests/plugins/platforms/photon/test_send_delivery.py \
       tests/plugins/platforms/photon/test_markdown.py::test_supports_message_editing_is_false -q

Long outbound replies could hit the 30s loopback HTTP timeout while the
sidecar was still waiting on spectrum-ts/upstream. httpx.ReadTimeout often
stringifies to an empty error, which bypassed timeout handling and triggered
the plain-text fallback — sending the same bubble twice.

- Remove Photon plain-text resend (same payload as markdown path)
- Treat empty/timeout/upstream errors as ambiguous delivery
- Raise default /send sidecar HTTP timeout to 120s (PHOTON_SIDECAR_SEND_TIMEOUT)
- Disable streaming edits on Photon (SUPPORTS_MESSAGE_EDITING=False)
- Add regression tests
@tsale
tsale marked this pull request as ready for review June 20, 2026 16:45
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins labels Jun 20, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the duplicate-send path and adding focused regression coverage. The current-main premise is real: plugins/platforms/photon/adapter.py:1426-1469 still turns a blank sidecar error into a second send.

Problems

  • The new ambiguity check is after the retry loop (plugins/platforms/photon/adapter.py:1228-1259 in this PR). Current main classifies Photon upstream/internal-sidecar failures as retryable (plugins/platforms/photon/adapter.py:97-105, merged as 2a4542333), so an ambiguous upstream failure is resent at PR line 1236 before the guard runs.
  • PHOTON_SIDECAR_SEND_TIMEOUT is a new user-facing behavioral environment variable (plugins/platforms/photon/adapter.py:1383), but repository policy requires behavioral settings in config.yaml. It is also not added to plugin.yaml or canonical Photon docs.
  • Standalone cron delivery retains AsyncClient(timeout=30.0) at PR line 1528, so the documented send timeout does not apply there.

Suggested changes

  • Gate ambiguous delivery before retrying, and test the current retryable upstream classification.
  • Move the setting to config and share it with standalone delivery.

Automated hermes-sweeper review.

# 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):

Copy link
Copy Markdown
Contributor

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 error and upstream failures as retryable, so those errors still get a second bubble before this check. Check ambiguity immediately after a failed send() and before is_network/the retry loop.

default_send_timeout = _DEFAULT_SIDECAR_SEND_TIMEOUT
try:
default_send_timeout = float(
os.getenv("PHOTON_SIDECAR_SEND_TIMEOUT", str(_DEFAULT_SIDECAR_SEND_TIMEOUT))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 config.yaml; please resolve the value through the platform configuration and keep any environment bridge internal.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing — the double-send-on-ambiguous-error premise was real and is fixed by #73563's structured error classes (auth/config classes now return before the fallback send in both send paths). The env-var toggle here conflicted with config policy. Thanks for the diagnosis; the fix landed via the error-classification route.

@teknium1 teknium1 closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants