Skip to content

fix(feishu): notify gateway reconnect watcher on WS thread death - #73202

Open
zhaomengfan wants to merge 2 commits into
NousResearch:mainfrom
zhaomengfan:fix/feishu-ws-reconnect-on-thread-death
Open

fix(feishu): notify gateway reconnect watcher on WS thread death#73202
zhaomengfan wants to merge 2 commits into
NousResearch:mainfrom
zhaomengfan:fix/feishu-ws-reconnect-on-thread-death

Conversation

@zhaomengfan

@zhaomengfan zhaomengfan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

When Feishu's hosted WebSocket receive loop dies (~every 30 min), the lark-oapi SDK's ws_client.start() returns silently and the executor future completes with no observer. Without this fix, the adapter never learns the WS died, and when all platform connections drop simultaneously (multiplex with 5 profiles), the gateway's stranded-detection path triggers a full gateway restart — disconnecting every other platform and pausing all cron jobs.

This change adds a _on_ws_thread_exit done-callback on _ws_future that detects unexpected WS thread exit (distinct from clean disconnect where _running=False and _ws_client is cleared). On unexpected death, it calls _set_fatal_error(retryable=True) + _notify_fatal_error(), routing through the gateway's existing per-platform reconnect watcher — no custom retry loop, as requested in the #31367 review.

Changes

  • plugins/platforms/feishu/adapter.py: +41 lines
    • _on_ws_thread_exit() — done-callback that filters clean-disconnect from unexpected death
    • _handle_ws_unexpected_exit() — cleanup + fatal-error notification
    • Wired into _connect_websocket() via add_done_callback

How to Test

Reproduce: run multiplex gateway with 5 Feishu profiles, wait ~30 min for WS receive-loop death.

Expected behaviour after fix:

[Feishu] WebSocket thread exited unexpectedly — notifying gateway reconnect watcher instead of escalating to full gateway restart.
[Feishu] Connected in websocket mode (feishu)

Other platforms stay up, cron jobs not paused, active sessions not interrupted.

Related

Fixes #31367
Supersedes/ports #31386 (original fix by @xxxigm; this ports it to the plugin adapter location and routes through the existing gateway watcher per @teknium1's review)

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages needs-decision Awaiting maintainer decision before any implementation labels Jul 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #31386 repairs the same Feishu WebSocket exit path with bounded adapter-level retries before escalation. This patch immediately notifies the gateway watcher instead; please choose one recovery policy rather than treating the implementations as duplicates.

@zhaomengfan

zhaomengfan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@alt-glitch This PR intentionally takes the "notify watcher immediately" approach rather than bounded adapter-level retries, following @teknium1's review guidance on #31367:

"Route unexpected completion through _set_fatal_error(..., retryable=True) plus _notify_fatal_error() so the existing per-platform supervisor owns retry/backoff."

This PR also targets the current plugin adapter location (plugins/platforms/feishu/adapter.py) rather than the old gateway/platforms/feishu.py path. #31367 can be closed in favor of this one.

@zhaomengfan

Copy link
Copy Markdown
Contributor Author

Why this approach subsumes #31386

@alt-glitch

1. Reviewer already decided this

@teknium1's review of #31386 (May 2026) is explicit:

"Route unexpected completion through _set_fatal_error(..., retryable=True) plus _notify_fatal_error() so the existing per-platform supervisor owns retry/backoff."

This PR is the implementation of that directive. #31386 has not been updated since that review — it still carries a 5-retry exponential-backoff ladder (~31s) before escalating to the same supervisor.

2. No duplicate retry policy

The gateway supervisor (gateway/run.py) already retries retryable adapters with backoff. Adding a second retry ladder inside the adapter:

  • Duplicates recovery logic — two layers fighting over the same reconnect
  • Delays escalation — worst case ~31s before the supervisor even learns of the failure
  • Loses observability — if adapter retries succeed, the supervisor never records the event

3. Correct file location

#31386 targets gateway/platforms/feishu.py — deprecated since 560010547 moved Feishu to plugins/platforms/feishu/adapter.py. @teknium1 flagged this. This PR targets the current location.

4. Code surface

+41 lines here vs +449 lines (+13 tests) in #31386 for the same failure detection. Simpler fix, fewer edge cases.

5. New production evidence (#73779)

pittosporum-seu's report demonstrates the failure is a two-layer problem:

Both are needed. This PR closes the recovery gap with the approach the reviewer explicitly requested.


Summary: If the team prefers bounded retries over immediate watcher escalation, that's a policy decision for the supervisor — not a reason to maintain two competing adapter-level implementations. This PR is the minimal, reviewer-aligned fix for the recovery gap.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for keeping recovery in the gateway-owned reconnect path. The premise is present on current main: plugins/platforms/feishu/adapter.py:1351-1353 suppresses errors from ws_client.start(), and :4906-4911 retains the executor future with no completion observer. The proposed retryable fatal-error route fits the existing supervisor, which queues retryable failures at gateway/run.py:6960-6976.

Problems

  • There is no regression coverage for the newly added completion callback. Current Feishu WebSocket tests cover clean disconnect (tests/gateway/test_feishu.py:132-182) and construction (:189-242), not an unexpected completed executor future.

Suggested changes

  • Add an async test that completes a live adapter's _ws_future and verifies the retryable fatal notification reaches its registered handler, plus a clean-disconnect no-op test.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@alt-glitch alt-glitch added duplicate This issue or pull request already exists and removed needs-decision Awaiting maintainer decision before any implementation labels Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #53508 — both current patches add the same WebSocket-future completion callback and hand unexpected exits to the gateway reconnect watcher.

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Correction: related to #53508, not a duplicate. Current #73202 is the focused plugin-path supervisor-notification repair; #53508 bundles a related callback with unrelated Feishu behavior changes. #31386 uses a distinct adapter-level retry policy on the retired path.

@zhaomengfan
zhaomengfan force-pushed the fix/feishu-ws-reconnect-on-thread-death branch from 0a92b64 to fbe1a82 Compare August 4, 2026 02:55
@zhaomengfan
zhaomengfan force-pushed the fix/feishu-ws-reconnect-on-thread-death branch from fbe1a82 to 560e6e5 Compare August 13, 2026 03:38
@zhaomengfan

Copy link
Copy Markdown
Contributor Author

@teknium1 rebased onto latest main (the branch had drifted 1300+ commits behind and mergeable_state was stuck unknown). No conflicts — mergeable: True. Local tests pass:

pytest tests/gateway/test_feishu_ws_thread_death.py -q
# 5 passed

This implements the recovery policy requested on #31386/#31367: unexpected WS thread exit routes through _set_fatal_error(..., retryable=True) + _notify_fatal_error() so the gateway-owned reconnect supervisor owns retry/backoff — no custom retry loop. Ready for another look when you have a chance.

Copy link
Copy Markdown

The latest public reproduction added to #31367 supports the recovery gap this PR addresses.

This should remain the focused recovery-layer patch; #64247 addresses the multiplex event-loop root cause. The current PR metadata reports it as non-mergeable against current main, so it needs another rebase. Please preserve the focused worker-exit and clean-disconnect regression tests when rebasing.

When Feishu's hosted WebSocket receive loop dies (~every 30 min),
the lark-oapi SDK's ws_client.start() returns silently and the
executor future completes with no observer. Without this fix,
the adapter never learns the WS died, and when all platform
connections drop simultaneously (multiplex with 5 profiles),
the gateway's stranded-detection path triggers a full gateway
restart — disconnecting every other platform and pausing all
cron jobs.

This change adds a done-callback on _ws_future that detects
unexpected WS thread exit (distinct from clean disconnect where
_running=False and _ws_client is cleared). On unexpected death,
it calls _set_fatal_error(retryable=True) + _notify_fatal_error(),
routing through the gateway's existing per-platform reconnect
watcher instead of implementing its own retry loop.

Fixes: NousResearch#31386
Refs: PR NousResearch#31367 (original fix, blocked by plugin-migration review)
Add 5 tests covering _handle_ws_unexpected_exit and _on_ws_thread_exit:
- _handle_ws_unexpected_exit notifies fatal handler (retryable)
- Gate: no-op when _running=False
- Gate: no-op when _ws_client=None
- _on_ws_thread_exit gate: clean-disconnect no-ops
- _on_ws_thread_exit dispatches to loop
@zhaomengfan
zhaomengfan force-pushed the fix/feishu-ws-reconnect-on-thread-death branch from 560e6e5 to 6c85b83 Compare August 21, 2026 04:16
@Seekers2001

Copy link
Copy Markdown

Thanks for rebasing and adding the requested tests. This now covers the recovery layer described in #31367: unexpected WS thread death is reported as retryable to the per-platform supervisor, while clean disconnect remains a no-op. The current CI checks are green and the branch is mergeable.

Small metadata correction: the Related section currently reverses the two references:

Could you update it to:

This keeps attribution for the problem report and the original implementation distinct.

@zhaomengfan

Copy link
Copy Markdown
Contributor Author

@Seekers2001 thanks for catching that — updated. The Related section now reads:

Appreciate the review.

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 P3 Low — cosmetic, nice to have platform/feishu Feishu / Lark adapter 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.

Feishu WebSocket disconnect causes full gateway restart every ~30min

4 participants