Repository navigation
fix(slack): list mcp servers off the event loop for the home tab - #24154
KiroCrewCaptain merged 1 commit into
Conversation
_publish_home_tab called mcp_discovery.list_servers() inline on the event loop. The listing reads the agent config, <data home>/mcp.json and every provider's global config (including ~/.claude.json, which is routinely megabytes), so each time a user opened the bot's Home tab the gateway loop stalled for every session. The next line already lists skills through asyncio.to_thread, and every other async caller of list_servers offloads it the same way. Run list_servers through asyncio.to_thread and keep the disabled filter on its result. Add a thread-recording list_servers stub test to test/test_slack_home_tab.py that pins the listing off the loop thread and that disabled servers are still not advertised.
github-actions
Bot
added
fork
UX Review (Opus 5.5, fork) — ⏭️ skippedRevision |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. The fix is one thread hop at the mechanism. The other blocking What this change shipsInventory (2 items) — 2 justifiedIntent: let someone open the bot's Slack Home tab without stalling every other gateway session (a FIX, closes #24153).
[FIRST-PRINCIPLES-REVIEWED] 63b0ad6 Description read[DESCRIPTION-READ] fc2ce0ca3208de9f5e2d632b523d68c8d4d1608e5be007048fe9e17512e451e1 That sha256 names the PR description this verdict read. Recompute it with |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: it moves one blocking listing call into a worker thread, the same way the skills listing on the next line already runs, and it applies the same filter to the result. [DESIGN-REVIEWED] 63b0ad6 Description read[DESCRIPTION-READ] a666091ccae644539c32b78f9772aa83958975137bd7a02e68388265e021a29a That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: action required
KiroCrewCaptain
left a comment
KiroCrewCaptain
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, security checklist all-NO, no reviewer-lane findings, readiness signal fresh for this head SHA, declared category matches the diff. Category: the Slack Home tab lists MCP servers in a worker thread instead of on the event loop, with a test that the disabled filter still applies.
github-actions
Bot
removed
the
readiness: passed
Problem / Motivation
Goal: Opening the bot's Slack App Home tab no longer blocks the gateway event loop while it lists the MCP servers.
_publish_home_tab(src/kiro_crew/slack/events_home_tab.py) calledlist_servers()inline on the event loop to build the Capabilities section.mcp_discovery_config.list_serversreads the agent config,<data home>/mcp.jsonand every provider's global config, including~/.claude.json, which is routinely megabytes, and then merges the probe caches.Why it matters
It runs each time any user opens the bot's Home tab. While it reads those files, every gateway session stalls; on a large
~/.claude.jsonor a slow data home that is a visible pause for everyone. This is the stall class behind #3057 and #1572.Not a goal
list_servers()calls in async code off the loop (see Sibling sweep).What changed (motivation → approach → change)
The Home tab now lists MCP servers in a worker thread, the same way the line after it already lists skills and the way
api_mcp_probe_cached, the Mochi routes andconnections/ownership.pyalready calllist_servers. The disabled filter applies to the result as before, so the tab shows the same servers.The lexical sync-io gate does not count this site (the blocking IO is inside the imported helper), so no baseline line changes.
Backwards compatibility
Compatible: the same listing runs with the same arguments and the same filter; only the thread it runs on changes, and an exception from it still reaches the section's existing
except.Removes nothing: the diff moves one call into
asyncio.to_threadand adds a test.Tests
test_home_tab_mcp_server_listing_runs_off_loopintest/test_slack_home_tab.pystubslist_serverswith a thread-recording function. It pins that the listing runs on a thread other than the loop's, runs once, and that a disabled server is still left out of the published view.Red on the unfixed parent, for the right reason:
assert threads[0] != loop_threadfailed withassert 140331221796672 != 140331221796672(the listing ran on the loop thread). Green on the fix, with all 32 tests in the file passing.Proof: PROVEN test/test_slack_home_tab.py::test_home_tab_mcp_server_listing_runs_off_loop (head 63b0ad6)
Determinism: 20/20 runs of the new test at
-n0; 10/10 runs at-n 4 --dist loadgroup --max-worker-restart=2withtest/test_slack_events_coverage.pyandtest/test_slack_sessions_view.pyas co-tenants (258 passed each); the whole file in a shuffled order (seed 14252, 32 passed).Local gates: black, isort, flake8, mypy, sync-io, subprocess-encoding, comment-history, brand, docs-lint and the flake-pattern ratchet all pass.
scripts/local-gate.pybackend related set: 232 passed.Manual verification
N/A — unit coverage sufficient: the change is one thread hop around a pure listing call, and the test drives the real
_publish_home_tab.Related Issues
Closes #24153
Sibling sweep
Swept: git grep -nE "\blist_servers(" -- src/ (then each call site's enclosing function checked for async def)
Sibling: src/kiro_crew/slack/events_home_tab.py:121 -- changed here
Sibling: src/kiro_crew/dashboard/handlers/mcp_probe.py -- not affected -- by this diff; api_mcp_servers (line 426) and api_mcp_active (line 605) call list_servers inline, left for their own PR because open PR #24093 touches mcp_probe.py
Sibling: src/kiro_crew/mcp_discovery.py:141 -- not affected -- by this diff; probe_all calls list_servers inline, left for its own PR because open PRs #10175 and #22965 touch mcp_discovery.py
Pattern harvest
Rule candidate: lint
Pattern: an async def calling an imported sync helper that does filesystem IO; the lexical sync-io gate sees only direct fs calls, so a cross-module scan (async caller -> imported sync function -> fs within a few hops) would catch this class.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)