DEV-1478: generous SLayer MCP startup timeout - #7
Merged
Conversation
The slayer stdio MCP server runs `--ingest-on-startup`, whose cost is a datasource schema RE-REFLECTION + semantic-layer rebuild (~30-50s for a large schema like alien). Under multi-actor CPU contention that blew the prior 300s MCP `initialize()` handshake budget, so every large-schema task timed out before any LLM call. Embeddings are NOT the cost — they're prebuilt and hash-skipped on startup. Add a shared, documented `SLAYER_MCP_STARTUP_TIMEOUT_S = 1800` in harness.py and use it at all three MCPServerStdio sites (pydantic_ai, otf_encode, recursive); ~30-50x margin so contention can't trip it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds SLAYER_MCP_STARTUP_TIMEOUT_S = 1800 in harness.py and uses it for MCPServerStdio startup timeouts in agent modules; threads a shared TokenUsage accumulator through setup-encoder runs, exposes it on run_one, persists per-DB _setup_usage.json during reference build finalization; tests updated/added. ChangesMCP Startup Timeout Consolidation
Setup-encoder usage instrumentation & persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py (1)
10-10: 💤 Low valueConsider updating the stale timeout reference.
The comment mentions "Slayer MCP startup is up to 300s" but the timeout is now 1800s. While the comment's main point (explaining why one server is shared per task) remains valid, the specific timing reference is outdated.
📝 Suggested update
-* One ``MCPServerStdio`` per task, shared across root → every sub-agent - → query-constructor. Slayer MCP startup is up to 300s with - ``--ingest-on-startup``; per-agent spawn would dominate wall time. +* One ``MCPServerStdio`` per task, shared across root → every sub-agent + → query-constructor. Slayer MCP startup can take significant time with + ``--ingest-on-startup`` (especially under CPU contention); per-agent + spawn would dominate wall time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py` at line 10, Update the stale timeout reference in the comment that mentions "Slayer MCP startup is up to 300s" (near the "query-constructor" comment) to the current 1800s value and adjust the wording to reflect the longer startup window while preserving the explanation for sharing one server per task; search for the "query-constructor" marker or the "Slayer MCP" text in src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py and replace "300s" with "1800s" (or rephrase to "up to 1800s / 30 minutes") so the comment is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_mcp_startup_timeout.py`:
- Around line 26-41: Add a test that verifies the pydantic_ai adapter uses the
shared startup timeout by calling _build_slayer_agent from
bird_interact_agents.agents.pydantic_ai.agent with test arguments (e.g., model,
slayer_storage_dir using tmp_path, strict_value=False), then inspect the
returned agent's toolsets (agent._toolsets) to locate the constructed slayer
server (first toolset) and assert its timeout equals
SLAYER_MCP_STARTUP_TIMEOUT_S; ensure the test mirrors existing tests' style and
imports and includes an assertion that agent._toolsets is non-empty before
checking slayer_server.timeout.
---
Nitpick comments:
In `@src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py`:
- Line 10: Update the stale timeout reference in the comment that mentions
"Slayer MCP startup is up to 300s" (near the "query-constructor" comment) to the
current 1800s value and adjust the wording to reflect the longer startup window
while preserving the explanation for sharing one server per task; search for the
"query-constructor" marker or the "Slayer MCP" text in
src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py and replace
"300s" with "1800s" (or rephrase to "up to 1800s / 30 minutes") so the comment
is accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7cb93e8-8d4f-4925-bf13-90537f2a96ff
📒 Files selected for processing (5)
src/bird_interact_agents/agents/pydantic_ai/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/harness.pytests/test_mcp_startup_timeout.py
The one-time per-DB reference-build encode (setup_encoder) was uninstrumented — its Opus spend appeared in no usage_json or artifact, so the OTF "incl encoders" cost was unrecoverable. Capture each KB-encode's usage under scope "setup_encoder" into a per-DB TokenUsage (exposed on run_one.usage) and persist it to `_setup_usage.json` next to the reference, carried back by the cloud merge. The scope is isolated from the per-task agent/user_sim subtotals so it stays separately summable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bird_interact_agents/agents/pydantic_ai_otf_encode/setup_encoder.py`:
- Around line 214-215: When the call to agent_run.usage() fails in the except
Exception block (currently logging via logger.debug with kb_id), mark the usage
accumulator as partial so downstream cost consumers can distinguish
unknown/incomplete from zero; specifically, set usage.partial = True in that
except branch (where logger.debug("setup-encode usage capture failed for
kb_id=%s", kb_id) is called) while keeping the existing log and continuation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd27f041-a0e7-4c34-8ca2-92c2aa048ce0
📒 Files selected for processing (8)
src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/setup_encoder.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/slayer_otf/reference_build.pytests/test_pydantic_ai_otf_encode_setup_encoder.pytests/test_slayer_otf_reference_build.pytests/test_usage.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py
- src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py
- Cover the 3rd MCP-server site: assert the base pydantic_ai adapter's MCPServerStdio (built inline in _build_slayer_agent) uses the shared SLAYER_MCP_STARTUP_TIMEOUT_S (Codex + CodeRabbit). - setup_encoder: flag the per-DB usage accumulator partial=True when an agent_run.usage() capture fails, so cost consumers distinguish $0 from unmeasured (CodeRabbit) + a focused test. - Reword the stale "up to 300s" comment in the recursive adapter docstring (now 1800s; CodeRabbit nitpick). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t (DEV-1478) PR #7 review (Codex): the pydantic_ai-adapter timeout assertion read the derived `agent.toolsets` view; switch to `agent._user_toolsets` (the raw user-supplied list, repo convention per test_root_clarifier_no_pinning.py) so it can't break if pydantic-ai changes what `.toolsets` composes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--ingest-on-startup, whose cost is a datasource schema re-reflection + semantic-layer rebuild (~30–50s for a large schema likealien, 30+ models). Under multi-actor CPU contention this blew the prior 300s MCPinitialize()handshake budget, so every large-schema task timed out before any LLM call (DEV-1478 cloud run: all 10alien, bothcold_chain,exchange_traded_funds, and 4credittasks died at the handshake withTimeoutError).embeddings.db, copied into each task variant, and hash-skipped on startup (verified: re-run leavesembeddings.dbuntouched).SLAYER_MCP_STARTUP_TIMEOUT_S = 1800inharness.py, wired into all threeMCPServerStdiosites (pydantic_ai,pydantic_ai_otf_encode,pydantic_ai_recursive). ~30–50x margin over the uncontended cost so contention can't trip it; a truly hung handshake is still bounded by the run's max-runtime / no-progress deadline.Test plan
tests/test_mcp_startup_timeout.py— asserts the budget is generous (≥1800) and that each adapter's MCP server is constructed with the shared value.e2-standard-8, 2 actors/worker) to confirm no MCP-startup timeouts.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests