Skip to content

Fix/feishu reconnect and shutdown - #5500

Closed
jtuki wants to merge 5 commits into
NousResearch:mainfrom
jtuki:fix/feishu-reconnect-and-shutdown-v2
Closed

Fix/feishu reconnect and shutdown#5500
jtuki wants to merge 5 commits into
NousResearch:mainfrom
jtuki:fix/feishu-reconnect-and-shutdown-v2

Conversation

@jtuki

@jtuki jtuki commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR fixes several Feishu websocket reliability issues that affected gateway behavior in production.

The main problems were:

  • inbound Feishu messages could be dropped after reconnects because the adapter reused stale event handlers tied to an old loop
  • websocket shutdown could hang because the SDK client thread was not cleaned up aggressively enough
  • webhook dispatch did not fully follow the same adapter-loop path as websocket mode
  • user-configured reconnect/ping tuning did not reliably take effect because the official Feishu SDK refreshed its runtime client config and overwrote local values

This approach keeps the scope tightly focused on gateway/platforms/feishu.py and its targeted tests. It fixes the runtime lifecycle problems at the actual integration boundary with the Feishu SDK, instead of adding outer-layer workarounds.

Related Issue

Fixes
#5499

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Rebuild the Feishu event handler on each connect in gateway/platforms/feishu.py so reconnects do not reuse stale callbacks.
  • Clean up the websocket thread loop and pending tasks more reliably during disconnect in gateway/platforms/feishu.py.
  • Route webhook message and card-action dispatch through the same adapter-loop path used by websocket mode in gateway/platforms/feishu.py.
  • Add Feishu-specific websocket tuning support for reconnect nonce, reconnect interval, ping interval, and ping timeout in gateway/platforms/feishu.py.
  • Reapply local websocket tuning after the official Feishu SDK refreshes runtime client config, so configured overrides remain effective in gateway/platforms/feishu.py.
  • Add and update focused regression tests in tests/gateway/test_feishu.py.
  • Keep the follow-up refactor limited to helper/test deduplication inside the same Feishu scope, with no functional change.

How to Test

  1. Run uv run pytest -o addopts='' tests/gateway/test_feishu.py -q.
  2. Start the gateway with Feishu websocket mode enabled and verify that disconnect/shutdown exits cleanly.
  3. Configure custom Feishu websocket reconnect/ping settings and verify they still take effect after reconnects and SDK client-config refreshes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux (WSL2), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Relevant symptoms before the fix included repeated warnings such as:

WARNING gateway.platforms.feishu: [Feishu] Dropping inbound message before adapter loop is ready

and delayed reconnect / keepalive-related websocket failures during runtime.

jtuki and others added 5 commits April 6, 2026 15:15
This commit fixes two critical bugs in the Feishu adapter that affect
message reliability and process lifecycle.

**Bug Fix 1: Intermittent Message Drops**

Root cause: Event handler was created once in __init__ and reused across
reconnects, causing callbacks to capture stale loop references. When the
adapter disconnected and reconnected, old callbacks continued firing with
invalid loop references, resulting in dropped messages with warnings:
"[Feishu] Dropping inbound message before adapter loop is ready"

Fix:
- Rebuild event handler on each connect (websocket/webhook)
- Clear handler on disconnect
- Ensure callbacks always capture current valid loop
- Add defensive loop.is_closed() checks with getattr for test compatibility
- Unify webhook dispatch path to use same loop checks as websocket mode

**Bug Fix 2: Process Hangs on Ctrl+C / SIGTERM**

Root cause: Feishu SDK's websocket client runs in a background thread with
an infinite _select() loop that never exits naturally. The thread was never
properly joined on disconnect, causing processes to hang indefinitely after
Ctrl+C or gateway stop commands.

Fix:
- Store reference to thread-local event loop (_ws_thread_loop)
- On disconnect, cancel all tasks in thread loop and stop it gracefully
  via call_soon_threadsafe()
- Await thread future with 10s timeout
- Clean up pending tasks in thread's finally block before closing loop
- Add detailed debug logging for disconnect flow

**Additional Improvements:**
- Add regression tests for disconnect cleanup and webhook dispatch
- Ensure all event callbacks check loop readiness before dispatching

Tested on Linux with websocket mode. All Feishu tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allow users to configure websocket reconnect behavior via platform extra
config to reduce reconnect latency in production environments.

The official Feishu SDK defaults to:
- First reconnect: random jitter 0-30 seconds
- Subsequent retries: 120 second intervals

This can cause 20-30 second delays before reconnection after network
interruptions. This commit makes these values configurable while keeping
the SDK defaults for backward compatibility.

Configuration via ~/.hermes/config.yaml:
```yaml
platforms:
  feishu:
    extra:
      ws_reconnect_nonce: 0        # Disable first-reconnect jitter (default: 30)
      ws_reconnect_interval: 3     # Retry every 3 seconds (default: 120)
```

Invalid values (negative numbers, non-integers) fall back to SDK defaults.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allow Feishu websocket keepalive timing to be configured via platform
extra config so disconnects can be detected faster in unstable networks.

New optional extra settings:
- ws_ping_interval
- ws_ping_timeout

These values are applied only when explicitly configured. Invalid values
fall back to the websocket library defaults by leaving the options unset.

This complements the reconnect timing settings added previously and helps
reduce total recovery time after network interruptions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reapply local reconnect and ping settings after the Feishu SDK refreshes its client config so user-provided websocket tuning actually takes effect.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consolidate coercion functions, extract loop readiness check, and deduplicate test mock setup to improve maintainability without changing behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@teknium1

teknium1 commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #5665. All 5 commits were cherry-picked with authorship preserved. Thanks @jtuki!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants