Skip to content

fix(feishu,multiplex): WS thread-safety + IP failover + secret scope fallback (re-submission of #53691) - #64247

Closed
zmlgit wants to merge 4 commits into
NousResearch:mainfrom
zmlgit:fix/feishu-ws-multiplex-hardening
Closed

fix(feishu,multiplex): WS thread-safety + IP failover + secret scope fallback (re-submission of #53691)#64247
zmlgit wants to merge 4 commits into
NousResearch:mainfrom
zmlgit:fix/feishu-ws-multiplex-hardening

Conversation

@zmlgit

@zmlgit zmlgit commented Jul 14, 2026

Copy link
Copy Markdown

Summary

Re-submission of #53691 (closed prematurely). The close reason claimed "upstream solved the Feishu WS thread-safety issue differently" — verification against current origin/main shows that is not the case for 3 of the 4 fixes here. Only _ThreadLocalLoopProxy could theoretically be replaced by upstream's per-thread asyncio.set_event_loop, but that does NOT actually fix the underlying bug because the Lark SDK reads ws_client_module.loop (module global) directly, not asyncio.get_event_loop(). So all 4 fixes are still needed upstream.

Verification that upstream still has the bugs

Fix in this PR Upstream state (verified on 226e8de82)
_ThreadLocalLoopProxy ws_client_module.loop = loop (line 1304) — still bare module-global, no thread-local proxy
per-instance _hermes_loop + monkey-patch SDK methods Upstream has no _hermes_loop, SDK methods still read module global
_connect_ws_with_ip_failover for DNS black-holes Upstream has no getaddrinfo / IP rotation; single connect with default timeout
_resolve_custom_provider_key via secret_scope with UnscopedSecretError fallback Upstream _fallback_entry_api_key still calls bare os.getenv(key_env, "")

What this PR fixes

Multiplex feishu gateways (gateway.multiplex_profiles: true) currently break in 4 distinct ways, each addressed by one commit:

1. _ThreadLocalLoopProxy (commit 1)

The Lark SDK stores its asyncio event loop in a module-level global (lark_oapi.ws.client.loop) and reads it via module-global lookup inside Client.start() / _connect() / _receive_message_loop(). With 2+ feishu WS adapters in one process, the second worker thread overwrites the loop the first one registered, and the first adapter crashes with:

Task ... got Future ... attached to a different loop

This commit replaces lark_oapi.ws.client.loop with a _ThreadLocalLoopProxy instance that forwards attribute access to the calling thread's registered loop.

2. per-instance _hermes_loop (commit 2)

Even with the proxy, three SDK methods (_connect, _receive_message_loop, _start) cache the loop reference at method entry, so a later reconnect on a wrong-thread client still misroutes. This commit stashes the per-thread loop on ws_client._hermes_loop and monkey-patches those three methods to read self._hermes_loop instead of the module global. Belt-and-suspenders with commit 1.

3. WS IP failover (commit 3)

msg-frontier.feishu.cn DNS pool returns ~12 IPs, ~3 of which are TCP/TLS black-holes (no RST, just indefinite hang). websockets.connect() tries resolved IPs sequentially under one open_timeout — if the first IP is a black-hole the entire connect times out. This commit resolves the hostname via getaddrinfo, tries each unique IP with a 4s per-IP timeout, returns the first successful TLS+WS handshake.

Upgrade path: replace with happy-eyeballs (RFC 8305) if websockets adds native support, or remove entirely if Feishu cleans their DNS pool.

4. Multiplex secret_scope fallback (commit 4)

_fallback_entry_api_key reads custom-provider API keys via os.getenv(key_env, ""). In multiplex mode without an active profile scope, agent.secret_scope.get_secret() raises UnscopedSecretError. At startup / prewarm no scope exists yet, so the call fails. This commit adds a _resolve_custom_provider_key helper that tries get_secret() first and falls back to os.getenv() on UnscopedSecretError, then routes both call sites through it.

Tests

Local: 217/218 pass on this branch. The single failure (test_websocket_sdk_accepts_channel_ua_tag) is pre-existing on pristine origin/main and unrelated to these changes.

Backward compatibility

  • Single-profile feishu configurations are byte-identical: the proxy install is a no-op when only one thread ever registers a loop; the IP failover falls through to a single connect when only one IP is returned; the secret_scope fallback returns the same value as os.getenv when no scope is active.
  • All four commits are additive — no public API change.

Why this PR was re-opened

#53691 was closed with the reasoning "upstream solved the Feishu WS thread-safety issue differently". That is inaccurate: upstream uses asyncio.set_event_loop(loop) per-thread, but the Lark SDK reads ws_client_module.loop directly (not via asyncio.get_event_loop()), so upstream's approach does not address the module-global clobbering. The other three fixes were not addressed upstream at all.

Re-submitting as a fresh PR rather than re-opening #53691 to keep the diff context current against latest main (the original PR was rebased onto a much older base).

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint platform/feishu Feishu / Lark adapter area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 14, 2026
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
张满良 added 4 commits July 23, 2026 08:17
…tion

Without the thread-local loop proxy, each feishu adapter overwrites the
Lark SDK's module-global event loop, causing 'Future attached to a
different loop' crashes when multiplexing multiple profile adapters.
Restored from 58ca5f7.
… instead of module global

The lark_oapi SDK's ws/client.py references a module-level loop global for
loop.create_task(...) calls inside _connect and _receive_message_loop.
In multiplex mode several adapter threads start near-simultaneously and each
overwrites that global, so a later reconnect on an earlier client schedules
coroutines on the wrong loop ('Task got Future attached to a different loop').

Previously _install_thread_local_ws_loop_proxy() made loop a thread-local, but
the SDK still used the module global. This patch monkey-patches the three SDK
methods that use the global (start, _connect, _receive_message_loop) to reach
for self._hermes_loop instead.
…ith UnscopedSecretError fallback

When the multiplexing gateway starts, resolve_provider_client() prewarms
auxiliary clients before any per-turn profile secret scope is installed. Named
custom providers (e.g. 9Router configured via config.yaml providers.ninerouter
with key_env: NINEROUTER_API_KEY) were still using os.getenv() directly, which
in multiplex mode either reads the wrong profile's value or fails to resolve
at all.

Add _resolve_custom_provider_key() helper that tries get_secret() first (for
per-turn scope isolation), falling back to os.getenv() when UnscopedSecretError
is raised (startup/prewarm path). Apply to both _fallback_entry_api_key and
the resolve_provider_client named custom provider branch.

Copy link
Copy Markdown

The latest public reproduction added to #31367 supports the event-loop isolation part of this PR.

For mergeability, could that change be split into a focused current-plugin-path patch with a multiplex regression test? The IP failover and secret-scope changes solve different problems and make the root-cause fix harder to review. The silent-worker recovery should remain in the gateway-owned supervisor path (#73202 / #53508), rather than adding another retry policy here.

@zmlgit

zmlgit commented Aug 19, 2026

Copy link
Copy Markdown
Author

Split per review (@Seekers2001's ask) — closing in favor of focused follow-ups:

The secret-scope fallback part of this PR turned out to be already implemented on current main (_scoped_key_env in agent/auxiliary_client.py, #76573 / Slack pattern #59739) — verified both call sites (resolve_provider_client named-custom branch and _fallback_entry_api_keyresolve_entry_api_key) now route through it, so no replacement PR is needed for that piece.

The adapter-side silent-worker retry is dropped per review — the gateway-owned supervisor path (#73202 / #53508) stays the reconnect authority.

@zmlgit zmlgit closed this Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants