Skip to content

fix(gateway): bound adapter disconnect in fatal-error handler - #54041

Open
srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/gateway-bound-fatal-error-disconnect
Open

fix(gateway): bound adapter disconnect in fatal-error handler#54041
srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/gateway-bound-fatal-error-disconnect

Conversation

@srojk34

@srojk34 srojk34 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • _handle_adapter_fatal_error() called raw adapter.disconnect() with no timeout — a half-dead adapter (e.g. a wedged Feishu/Lark WebSocket) blocked the handler indefinitely, preventing the platform from being popped and queued for reconnection
  • Replace with _safe_adapter_disconnect(), which wraps the call in HERMES_GATEWAY_ADAPTER_DISCONNECT_TIMEOUT — the same bounded-teardown pattern applied to _stop_impl() in fix(gateway): Gateway shutdown hangs causing 'PID file race lost' on restart #14128
  • Add a regression test: a wedged adapter's fatal-error handler must complete within the timeout budget

Test plan

  • test_fatal_error_bounds_wedged_disconnect — wedged disconnect() times out, adapter is removed, platform is queued for reconnection
  • All 13 existing TestRuntimeDisconnectQueuing + test_bounded_adapter_teardown tests pass

_handle_adapter_fatal_error() called raw adapter.disconnect() with no
timeout.  A half-dead adapter (e.g. a wedged Feishu/Lark WebSocket)
could block the handler indefinitely, preventing the platform from
being popped from the adapter map and queued for reconnection.

Replace with the existing _safe_adapter_disconnect() helper, which
wraps the call in HERMES_GATEWAY_ADAPTER_DISCONNECT_TIMEOUT — the
same bounded-teardown pattern applied to _stop_impl() in NousResearch#14128.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jun 28, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Bounds the adapter disconnect in the fatal-error handler to prevent hangs. When _handle_adapter_fatal_error calls adapter.disconnect(), a wedged disconnect (e.g. WebSocket hang) could block the entire handler indefinitely. The fix wraps the disconnect in a timeout via _safe_adapter_disconnect. Test verifies a wedged adapter completes within the timeout and is properly cleaned up.


Reviewed by Hermes Agent

@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 identifying the unbounded fatal-error teardown. The defect remains on current main: gateway/run.py:4038 still awaits adapter.disconnect() directly, while _safe_adapter_disconnect() already bounds that operation at gateway/run.py:3347-3359.

Problems

  • The PR predates commit a682091044955167c9a728f9641ff279c96a73a7, which intentionally changed the handler to remove the adapter before awaiting teardown (gateway/run.py:4030-4038) so concurrent fatal notifications cannot disconnect it twice. Do not restore the older try/finally ordering from the PR base.

Suggested changes

  • Preserve the current pre-await pop() and delivery-router update, and replace only the raw await at gateway/run.py:4038 with _safe_adapter_disconnect(adapter, adapter.platform).
  • Carry over the wedged-disconnect regression test to verify the handler reaches reconnection queuing after the timeout.

This is an automated hermes-sweeper review.

Comment thread gateway/run.py
@@ -3633,7 +3633,7 @@ async def _handle_adapter_fatal_error(self, adapter: BasePlatformAdapter) -> Non
existing = self.adapters.get(adapter.platform)

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.

When salvaging this change, retain current main's later ordering that removes the adapter before the awaited teardown (gateway/run.py:4036-4038, commit a682091044955167c9a728f9641ff279c96a73a7). Replace only the current raw await with this helper; restoring the PR base's surrounding try/finally ordering would reopen the concurrent-fatal-notification race.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

4 participants