fix(relay): rc.4 relay transport + inbound fixes — dedupe replays, fail pending on drop, fail fast mid-redial, WAN keepalive - #89584
Merged
benbarclay merged 12 commits intoAug 20, 2026
Conversation
The reader is the only thing that can resolve a pending outbound_result
future. When the socket dropped unexpectedly (vs a deliberate
disconnect(), which already failed pending), every in-flight
_request_response waiter blocked the full _outbound_timeout_s (~30s) on
a future no reader could resolve.
Coatue incident 2026-08-18: a 1011 keepalive ping timeout close left
final-send and error-notification calls wedged against the dead socket,
holding sessions active while the connector replayed inbound backlog.
Fail every not-done pending future from _read_loop's finally with the
dict shape callers expect ({success: False, error: connection lost}) —
never an exception on the outbound path — then clear the map. list()
snapshot avoids mutation-during-iteration from woken waiters' finally
pops.
Between an unexpected close and the supervisor's successful re-dial,
self._ws still points at the DEAD socket, so the existing None-check
does not cover the backoff window: a send registered a future no reader
could resolve and blocked the caller for _outbound_timeout_s.
A live supervisor task is exactly the redial window (the loop returns
when a dial succeeds and its fresh reader takes over), so no new state
is needed: when self._supervisor is not done, return the error dict
immediately ({success: False, error: reconnecting}). Callers already
handle failed sends; a fast, honest failure beats a 30s wedge
(Coatue 2026-08-18).
…eout=60) Customer gateways cross WAN paths to the connector; the websockets library defaults (ping 20s, pong deadline 20s) close the socket with 1011 keepalive ping timeout under transient latency or event-loop stalls — the trigger of the Coatue 2026-08-18 incident. 60s tolerates stalls while still detecting a genuinely dead link within ~90s worst case. Set explicitly at both connect() call sites (with and without auth headers) so the tuning is visible and pinned by test rather than inherited from library defaults.
alt-glitch
added
type/bug
Collaborator
Related to #82238, which addresses the stale dead-socket result path. This PR also fails pending futures, blocks sends during redial, and tunes keepalives; maintainers should choose or consolidate the overlapping relay fixes. |
…age_id) Live-canary finding NousResearch#3 (Alice, staging): the relay inbound leg is at-least-once. On WS re-handshake the connector replays its durable per-instance buffer; a long multi-tool turn (60-100s) straddling a quiet socket drop got its ORIGINAL inbound replayed after the turn finished, re-running the entire turn — the user saw the final answer posted 2-5x (each a separate execution, hence slightly different texts). Receipts: same msg text at history=0 in back-to-back sessions 121647/121840, no Slack-side retry on the connector (envelope dedupe never fired). Consumer-side idempotency: bounded FIFO seen-set (512) keyed by platform message identity; events without a message_id never dedupe (fail-open — dropping a real message is worse than rerunning one). No wire change; contract v1 untouched. Transplanted-from: victor-fork/feat/relay-slack-live-cards@73ce04ae75 (extracted for the rc.4 relay-fixes train; tests moved to a standalone file with no live-cards dependencies)
victor-kyriazakos
changed the title
…er platform The dedupe key read event.chat_id — a field MessageEvent does not have (chat identity lives on event.source.chat_id; see how every other read in this adapter resolves it). getattr defaulted to None, so _inbound_dedupe_key returned None for EVERY production event: the fail-open branch always taken, the seen-set permanently empty, and a replayed inbound still re-ran the whole turn. The tests passed because their SimpleNamespace events carried a top-level chat_id no production code path produces. Read the chat id from event.source, and join the underlying platform into the key: one relay adapter fronts several platforms (Phase 1.5 multiplex), and two platforms' numeric chat/message ids must not collide into one replay identity. The test event factory now builds real MessageEvent/SessionSource objects in the wire decoder's shape, so the replay and bounded-set tests fail against the broken key instead of green-lighting it.
The dedupe tests validated hand-built events only, so a key that read a field the production event type doesn't have still went green — and the 'all 7 new tests fail against base' mutation claim didn't hold either (the fail-open and distinct-message tests pass against base because a no-op dedupe trivially satisfies both). Add a wire-level class that decodes a connector frame with _event_from_wire and dispatches it through _on_inbound — the exact production path — asserting: a decoded event yields a dedupe key at all, a re-delivered frame is dropped (fresh decode each time, so identity must come from the key, not the object), and identical chat/message ids on two different platforms are NOT conflated (Phase 1.5 multiplex). Mutation-checked: with the previous event-shape key reinstated, the wire tests fail (3 failed); with the fix, all 7 pass.
Two reader-exit paths arm NO reconnect supervisor: a terminal 4401 revocation (deliberately never re-dials) and reconnect=False transports. On both, _ws kept pointing at the dead socket after the reader unwound, so the 'is None' liveness guard reported connected and a send registered a future nothing could ever resolve — the full _outbound_timeout_s (~30s) wedge this PR exists to eliminate. The revocation case is the sharpest: the fatal-error notification that path emits is itself an outbound send, so it ate the stall. Null the handle in the reader's finally, identity-guarded (only if _ws still points at the socket THIS reader served) so a supervisor re-dial that already installed a fresh socket is never clobbered, and gated on not _closing so disconnect() keeps sole ownership of teardown. This also makes _ws the single honest liveness signal for the redial window itself — groundwork for retiring the supervisor-state send guard, which misreads that window from both directions. Regression tests cover both uncovered paths, asserting _ws is cleared and a post-drop send fails fast; both wedge (fail) with the finally reverted.
The mid-redial fail-fast guard used 'supervisor task not done' as the definition of the redial window, and that signal is wrong from both directions: - Too narrow: the wedge it fixed also occurs on reader exits that arm NO supervisor (terminal 4401 revocation, reconnect=False) — those stayed wedged. - Too broad: _reconnect_loop -> _dial_and_start installs the fresh socket and starts its reader, THEN awaits one hello send per fronted identity before the supervisor unwinds. Through those awaits the transport is fully live, yet the guard rejected every send as 'reconnecting' — refusing real traffic on a healthy socket. With the previous commit the reader clears _ws on unexpected exit, so the existing 'is None' check now covers the entire outage window honestly: _ws is the single liveness signal. Drop the supervisor-state guard. The redial-window test now drives the real sequence (reader exit arms the supervisor and clears _ws) instead of hand-crafting a stale-_ws state the transport can no longer reach, and a new test pins the post-dial window: a send issued while the supervisor is still unwinding past a live socket must reach that socket (fails with the guard reinstated).
…f asserting _read_loop opened with 'assert self._ws is not None' — an exit that escaped BEFORE the finally that fails pending futures, contradicting the 'fails pending on ANY exit path' invariant the hardening commit established. Production currently assigns _ws before scheduling the reader, so this was latent, but any future lifecycle change hitting it would strand every in-flight waiter for the full outbound timeout with only an AssertionError in the logs. Turn it into a guarded early-return INSIDE the try: the reader logs the lifecycle bug and unwinds through the same finally as every other exit, settling all waiters. Regression test drives _read_loop with _ws=None against a registered pending future.
The key derived its platform component with getattr(platform, 'value', ''), which handles the Platform enum the wire decoder always produces but collapses a plain-string platform — or a missing one — to the same empty string. Two DIFFERENT string platforms would then share one key component (cross-platform id collisions conflate), and enum vs string spellings of the SAME platform would produce two keys (a replay decoded differently would not dedupe). Inert on today's wire path (the decoder canonicalizes unknowns to Platform.RELAY), but alternate event constructors carry strings, so normalize at the key: enum value when present, the string itself otherwise, empty only when there is genuinely no platform. Missing platform intentionally still yields a key — fail-open on identity is reserved for missing message/chat ids. Tests pin all three properties; the spelling-invariance pair fails against the previous expression.
…ception
The socket can die BETWEEN _request_response's 'is None' liveness guard
and the actual write: the reader's finally hasn't cleared _ws yet, so
_send raises ConnectionClosed straight into callers whose contract is a
result dict (RelayAdapter.send consumes it with no try — only the
cosmetic typing lanes wrap the call). No liveness check can close this
window; it has to be caught at the write.
Convert the raise to {'success': False, 'error': ...} like every other
failed send, log the traceback at debug (the returned string alone
can't distinguish an ordinary dead socket from a defect in the frame
building above), and rely on the existing finally to drop the pending
entry. CancelledError is a BaseException, so cancellation still
propagates.
Same disposition as the equivalent guard in PR NousResearch#82238; regression test
drives a socket whose write raises while the reader is still parked,
and fails without the except clause.
NousResearch#85796 (live-cards gateway half) landed on main and touches the same two files. Resolutions: - adapter.py __init__: union — the dedupe seen-set and the live-cards draft/seal caches are independent sibling attributes. - ws_transport.py _request_response: keep main's ambiguous-timeout contract AND this branch's raising-write catch, composed: a raise from the WRITE itself means the frame never reached the wire (definite non-delivery, no flag), while a failure surfaced after the frame was sent carries ambiguous=True like the timeout — tracked via a frame_sent marker.
prmartinow
pushed a commit
to prmartinow/hermes-agent
that referenced
this pull request
Aug 26, 2026
…-hardening fix(relay): rc.4 relay transport + inbound fixes — dedupe replays, fail pending on drop, fail fast mid-redial, WAN keepalive
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
The complete relay-robustness fix set for the rc.4 expedite train, one commit per fix. Driven by a customer incident (2026-08-18, release 2026.8.13): a
1011 keepalive ping timeoutclose mid-turn produced BOTH wedged sends and an agent that appeared stuck re-running previous turns after/stop//new.Standalone by design: no live-cards dependencies, cherry-picks cleanly onto release lineages as a hotfix. The inbound dedupe commit was extracted from #85796 (its finding #3) and transplanted here so relay fixes ship without waiting for the task-card/live-cards review; #85796 no longer carries it.
The incident, fully decomposed
1011close. → keepalive commit_wsstill points at the dead socket). → fail-fast commitCommits
d2975f4219_read_loopfails all in-flight_pendingfutures on ANY exit path with the dict shape callers expecta7946f6502_request_responsefails fast during the redial window (a live supervisor task IS the window; no new state)534f8d4fb0ping_interval=30, ping_timeout=60at both connect sites; library defaults (20/20) are too aggressive for customer WAN pathsb5f95d0890Validation
tests/gateway/relay/test_ws_transport_hardening.py(3 tests) +tests/gateway/relay/test_relay_inbound_dedupe.py(4 tests: replay dropped, distinct handled, fail-open without id, seen-set bounded)Relationship to #85796
Now fully disjoint: #85796 carries only streaming/live-cards work (its branch was rebased to drop the dedupe commit; verified the branch diff is exactly the 105 transplanted lines). Merge order between the two no longer matters.
Review updates (2026-08-20)
Two independent review rounds (plus a follow-up) were run against this PR; 8 commits were added to resolve the findings. Merge order context: #85796 landed on main mid-review, so this branch now carries a merge of main with a composed resolution.
Added commits
22dfdc1c56event.chat_id— a fieldMessageEventdoes not have (chat identity lives atevent.source.chat_id), so the key wasNonefor every production event and the replay dedupe was a complete no-op; the original tests passed only because theirSimpleNamespaceevents carried a top-levelchat_idno production path produces. Now readsevent.source.chat_id, keyed per platform (Phase 1.5 multiplex-safe).8772702503_event_from_wire → _on_inbound— the real production path — so an event-shape mismatch can't green-light again. Includes cross-platform non-conflation coverage.52f9979a6c_wson unexpected exit (identity-guarded, teardown-owned paths excluded). Fixes the ~30s send wedge on the two reader exits that arm NO reconnect supervisor: terminal 4401 revocation (whose own fatal-error notification ate the stall) andreconnect=Falsetransports.ca3438d395_dial_and_start()was still awaiting hello sends. With_wscleared by the reader, the existingis Nonecheck covers the outage window honestly.2f77ca1993assertbefore it.c60a525380bb0a4193d1ConnectionClosedintoRelayAdapter.send.141af4febfadapter.py__init__: union of independent sibling caches._request_response: composes #85796's ambiguous-result contract with the raising-write catch via aframe_sentmarker — a raise from the write itself is definite non-delivery (no flag); a failure surfaced after the frame was sent carriesambiguous: Truelike the timeout.Validation (corrected)
scripts/run_tests.sh(the canonical runner)ambiguousabsent; post-send failure (disconnect mid-flight) →ambiguous: TrueRelationship to #82238 (updated)
This PR now covers the pending-future/dead-socket core that overlapped with #82238. That PR's remaining unique value: per-frame exception isolation in the read loop, the non-dict frame guard in
_handle_frame, and thego_dormantstale-handle fix.