fix(gateway): don't retry ambiguous HTTP 5xx sends (Signal group-reply duplicates)

[exiao]

Problem

Users get duplicate replies in Signal group chats. Root cause is a misclassified retry.

When a Signal group send hits an intermittent server-side HTTP 500, signal-cli surfaces it as:

org.signal.network.exceptions.NonSuccessfulResponseCodeException: [500] Bad response: 500 : {"code":500,...}

_is_retryable_error substring-matches an error against a pattern list that includes "network". The Java exception's package name — org.signal.network.exceptions — matches it, so the 500 is treated as a transient network error and _send_with_retry re-sends the identical message up to 2 more times (≈2s, ≈4s backoff).

A 5xx is ambiguous: Signal frequently accepts and delivers the message and then returns 500. So every retry lands as a duplicate in the group. This is the exact hazard the existing timeout guard already guards against ("message may have been delivered") — but a 500 isn't a timeout, so nothing protected it.

Evidence (from a live gateway)

• 43 send-500s over ~18h were 100% group sends, 0 on 1:1.
• Observed duplicate pairs to those same groups were 3–5s apart — matching the 2s/4s retry backoff.
• Confirmed the classifier returns is_retryable=True on the real error string (matched pattern: network); the timeout guard returns False.

Fix

Add _is_ambiguous_delivery_error (HTTP 5xx detection) and use it in two places:

1. _is_retryable_error — return False for a 5xx before the substring match. This runs first so genuine connection-level failures in the same package (e.g. PushNetworkException, request never reached the server) are still retried.
2. _send_with_retry — extend the existing ambiguous-failure guard so a 5xx skips both the retry loop and the plain-text fallback, returning the failure as-is.

Notes:

• SendResult.retryable=True still wins — a platform that knows its 5xx is safe to retry can opt in.
• 4xx is deliberately excluded (outright rejection, nothing delivered) and remains eligible for the plain-text fallback.

Tests

Regression tests use the real observed Signal 500 error string:

• _is_retryable_error: 500 not retryable despite network in the package; genuine PushNetworkException still retryable.
• new _is_ambiguous_delivery_error unit tests (5xx yes, 4xx no, no-status no).
• _send_with_retry: a 500 produces exactly one send (no retry, no fallback → no duplicate); still retries when the platform sets retryable=True.

tests/gateway/test_send_retry.py (50) and tests/gateway/test_send_error_classification.py (22) pass.

Scope

• Fixes duplicate replies (local classifier bug).
• Does not address the underlying Signal-side group-send 500s themselves — those are upstream/transient on Signal's infrastructure and out of scope here.

🤖 Generated with Claude Code

[claude]

Claude finished @exiao's task in 3m 1s —— View job

---

PR Review — fix(gateway): don't retry ambiguous HTTP 5xx sends

• Read the diff (base.py, signal.py, tests) against origin/live-config
• Trace the retry/fallback classification paths
• Verify the Signal send() → _rpc() error-preservation path
• Check regexes, tests, and review-focus invariants

LGTM. The ambiguous-5xx fix is correct and complete, and the prior review rounds have been addressed cleanly.

What I verified:

• Classifier order is right. _is_ambiguous_delivery_error runs before the substring match in _is_retryable_error, so Signal's org.signal.network.exceptions...[500] no longer misretries on the "network" substring, while genuine PushNetworkException (no HTTP status) stays retryable.
• 4xx short-circuit is sound. The explicit [4xx] / bad response: 4xx / http 4xx / error '4xx guard only demotes errors that also carry a rejection marker — genuine connection errors don't match, so no retryable class regresses. 4xx stays fallback-eligible.
• Regex covers the real formats: Signal [500], bad response: 500, WhatsApp/Graph HTTP 500 / (HTTP 503), and httpx raise_for_status Server error '500 …' all match; URLs like http://host:5000 don't (no whitespace after http).
• Both guard sites covered. Initial result (base.py:4375) and the mid-retry break path (base.py:4409-4411) both return result on timeout/5xx instead of dropping to the duplicating plain-text fallback. result.retryable=True opt-in still wins.
• Signal error text now reaches the classifier. send() calls _rpc(..., raise_on_error=True) and converts SignalRPCError into SendResult.error, so JSON-RPC error dicts (str(err) contains [500]) and httpx transport errors both surface the real string. SignalRateLimitError precedence is preserved.
• Tests are behavior contracts, not snapshots: real observed error strings, assert exact send counts (1 send for 500, 2 for opt-in/400-fallback), no-sleep assertions, and drive the real send()→_rpc()→raise_for_status path.

Review-focus invariants: no system-prompt/toolset mutation (cache-safe), no role-alternation impact, no new core tool, no new HERMES_* env var — none apply. I could not execute the suite here (test runner needs approval in this sandbox), but the author reports 248 passing and the logic checks out by inspection.

0 blocking, 0 minor.
· branch fix/signal-500-ambiguous-retry-dupes

[claude]

LGTM — the 5xx ambiguous-delivery guard correctly stops the retry-and-fallback duplication for Signal group 500s, keeps genuine PushNetworkException retryable, and preserves the retryable=True opt-in. Tests use the real error string and assert single-send behavior. One minor non-blocking note left inline: the mid-retry break path (L4401) still falls through to the duplicating plain-text fallback if a 500 appears after an initial transient failure.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 015b038c56

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[exiao]

Addressed the mid-retry ambiguity: after a transient failure, an HTTP 5xx or delivery-ambiguous timeout now returns the failed result instead of falling through to the plain-text fallback. Added a regression covering transient → Signal 500 (two sends only). Verified with scripts/run_tests.sh tests/gateway/test_send_retry.py -q (51 passed).

[claude]

Reviewed the ambiguous-5xx delivery guard. Core Signal fix is correct and complete; Codex #2 (mid-retry 5xx) is already resolved in the second commit. One valid minor finding: _is_ambiguous_delivery_error misses the WhatsApp Cloud 5xx format (HTTP 500: / graph error 500 (HTTP 500)), so a Meta Graph 500 still drops to the duplicating plain-text fallback. Suggest broadening the regex to also match http\s*5\d\d. Details in the PR comment. 0 blocking, 1 minor.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c61f0bacd4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Reviewed the ambiguous-5xx delivery guard. Core Signal fix is correct and complete; both Codex points (WhatsApp Graph 5xx format, mid-retry 5xx) are resolved. One minor non-blocking finding: _is_ambiguous_delivery_error misses the httpx raise_for_status() format (Server error '500 ...' for url ...) that httpx-based adapters like bluebubbles surface (base.py:601/618), so those 5xx sends still hit the duplicating plain-text fallback — suggest adding an error '5\d\d alternative to the regex. Details in the PR comment. 0 blocking, 1 minor.

[claude]

LGTM — the ambiguous-5xx guard correctly stops the retry-and-fallback duplication for Signal group 500s: the classifier runs before the substring match so network-package 500s are no longer misretried, genuine PushNetworkException stays retryable, the mid-retry break path returns instead of falling through, and SignalAdapter.send() now preserves the real error text so the guard actually sees it. One minor non-blocking note posted inline: _is_ambiguous_delivery_error doesn't match the httpx raise_for_status format (Server error '500 ...') that BlueBubbles/weixin/yuanbao surface, so those 5xx sends still drop to the duplicating plain-text fallback — broadening the regex closes the whole class. 0 blocking, 1 minor.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a31285196

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

LGTM — the ambiguous-5xx guard correctly stops the retry-and-fallback duplication for Signal group 500s: the classifier runs before the substring match so network-package 500s are no longer misretried, PushNetworkException stays retryable, the mid-retry break path returns instead of falling through, and SignalAdapter.send() now preserves the real error text so the guard sees it. 1 minor non-blocking note in the PR comment: _is_ambiguous_delivery_error still misses the httpx raise_for_status format (Server error '500 …') that BlueBubbles/weixin/yuanbao and Signal's own transport surface, so those 5xx sends still drop to the duplicating plain-text fallback — broadening the regex with an error '5\d\d alternative closes the whole class. 0 blocking, 1 minor.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f485e36264

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

One real regression on the Signal 4xx path (details in the Claude review comment): the newly-preserved org.signal.network.exceptions text now matches the "network" retryable substring for 4xx errors, so _is_retryable_error returns True, the rejected payload is retried 2x, and the plain-text fallback the docstring promises for 4xx never runs. Fix: short-circuit any non-5xx HTTP response-code error to non-retryable (but fallback-eligible) in _is_retryable_error, plus a Signal-400 regression. Core Signal-500 duplicate fix is correct and well-tested — 1 minor, non-blocking.

[claude]

LGTM — reviewed the ambiguous-5xx guard end-to-end. The classifier runs before the substring match so Signal's network-package 500s are no longer misretried, genuine PushNetworkException stays retryable, 4xx short-circuits to fallback, both the initial and mid-retry break paths return the failure instead of falling through to the duplicating plain-text fallback, and SignalAdapter.send() now preserves the real error text (JSON-RPC dict + httpx transport) so the guard actually sees it. Regex covers Signal / Graph / httpx 5xx formats without false-positiving on URLs. Tests are real-error-string behavior contracts asserting exact send counts. 0 blocking, 0 minor.