Skip to content

fix(feishu): defer the lark_oapi import off the startup path (salvage #57657) - #77514

Merged
kshitijk4poor merged 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-57657-feishu-lazy
Aug 3, 2026
Merged

fix(feishu): defer the lark_oapi import off the startup path (salvage #57657)#77514
kshitijk4poor merged 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-57657-feishu-lazy

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvages #57657 by @baau — ported onto the plugin layout with authorship preserved (the adapter moved from gateway/platforms/feishu.py to plugins/platforms/feishu/adapter.py since the PR's base, ~4,500 commits back, so this is a port rather than a cherry-pick).

Context — what this fixes, for whom

Every gateway user, whether or not they use Feishu: lark_oapi is a heavy SDK that takes seconds to import and holds the GIL while doing it, and the adapter imported it at module level — so every gateway boot paid that cost during startup's most GIL-sensitive window. This was the surviving canonical of a duplicate pair (#68849 was closed in its favor): 23cb26c fixed the desktop-boot symptom, but the eager import itself remained.

What the fix does (from #57657, kept intact through the port)

  • _load_lark_oapi() binds the SDK globals on first use, with a threading lock + double-checked locking; connect() and _standalone_send() invoke it via asyncio.to_thread so the event loop never blocks on the import
  • check_feishu_requirements() becomes install-only (lazy_deps.ensure) — no import, no global rebinding

Review findings folded in (the port's two additions)

  1. probe_bot() SDK path preserved: under the PR as authored, FEISHU_AVAILABLE stays False until first connect, so probe_bot() would have silently degraded to its HTTP fallback. It now calls _load_lark_oapi() first (sync context — it's a blocking helper already), keeping the SDK probe path. This was the prior triage's Medium finding; call-site enumeration confirmed probe_bot was the only consumer left stranded.
  2. Test-suite adaptation: test_feishu.py's existing tests inject fake clients into the module globals, which the PR's install-only check no longer binds — a setUpModule binds them eagerly for the test process (7 failures otherwise).

Verification

  • tests/gateway/test_feishu.py + test_feishu_lazy_import.py: 77 passed, 1 skipped on current main
  • Mutation check: revert adapter.py to main → both lazy-import tests fail; restore → pass
  • ruff clean

Closes #57657 (superseded by this salvage — original author credited via commit authorship).

Salvage of NousResearch#57657, ported onto the plugin layout (the adapter moved
from gateway/platforms/feishu.py to plugins/platforms/feishu/adapter.py
since the PR's base). lark_oapi takes seconds to import and holds the
GIL doing it; the module-level import made every gateway boot pay that
cost even with Feishu unconfigured.

- _load_lark_oapi() with double-checked locking binds the SDK globals
  on first use; connect() and _standalone_send() call it via
  asyncio.to_thread so the loop never blocks on the import.
- probe_bot() also calls _load_lark_oapi() (sync context) so the SDK
  probe path is preserved rather than silently degrading to the HTTP
  fallback before a first connect.
- check_feishu_requirements() is install-only and no longer rebinds
  globals; test_feishu.py gets a setUpModule that binds them eagerly
  for tests that inject fake clients.

Includes the dedicated lazy-import test file (check-does-not-import,
connect-loads-on-worker-thread).
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 3, 2026 08:51
@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter labels Aug 3, 2026
CI exposed the whole class: feishu tests across MANY files (thread
routing, text batching, sdk executor, ...) inject a mock _client and
skip connect(), so the deferred import leaves the request-builder
globals None. Replace the single-file setUpModule with a session-scoped
autouse conftest fixture that binds the globals once when lark_oapi is
installed; when it isn't, the affected tests already skip via their own
skipUnless guards. Full tests/gateway run: zero failures beyond main's
pre-existing baseline (sorted failure-diff).
The no-SDK fallback guards check '"Name" in globals()' — correct on
main where a failed module-level import leaves those names undefined,
but the deferred-import port pre-binds every SDK name to None, so the
guard was always true and the fallback paths called .builder() on None
(AttributeError) wherever lark_oapi isn't installed. Local runs passed
because lark IS installed here; CI's default env has no feishu extra.
Rewrote all 14 guards to 'is not None', which is correct under both
conditions. Verified by simulating CI with a lark-blocking meta_path
hook: 74 passed, 18 skipped (the skipUnless set), zero failures.
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 type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants