fix(mount_mcp_as_cli): retry tools/list on empty response to handle backend schema-build race - #49759
Conversation
…tion When the MCP gateway reports a backend as "running", tools/list can still return 0 tools if the backend hasn't finished building its internal tool schema yet. This is a race condition that becomes more likely with large dispatch-workflow configs (e.g., 44 workflows each with rich input schemas) because schema construction takes long enough to be visible. Add `fetchMCPToolsWithRetry` which retries tools/list up to `TOOLS_EMPTY_MAX_RETRIES` (5) times with `TOOLS_EMPTY_RETRY_DELAY_MS` (1000ms) delay between attempts when an empty result is returned. The production `main()` path now uses this instead of the bare `fetchMCPTools` call. A genuinely broken backend still fails fast (after at most 5 extra seconds), while a slow-but-healthy backend gets a chance to finish registering its tools. Closes #49695 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (default_business_additions=0). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds bounded retries for empty MCP tools/list responses to mitigate backend schema-build races.
Changes:
- Adds configurable empty-response retry logic.
- Adds unit tests for success, exhaustion, warnings, and delay behavior.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/mount_mcp_as_cli.cjs |
Implements and applies retry logic. |
actions/setup/js/mount_mcp_as_cli.test.cjs |
Tests retry behavior and constants. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| const doSleep = sleep ?? (ms => new Promise(resolve => setTimeout(resolve, ms))); | ||
| const doFetch = fetchFn ?? ((url, key, c) => fetchMCPTools(url, key, c)); | ||
| let tools = await doFetch(serverUrl, apiKey, core); | ||
| for (let attempt = 1; attempt <= TOOLS_EMPTY_MAX_RETRIES && tools.length === 0; attempt++) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The retry logic is well-implemented and thoroughly tested. The injectable fetchFn/sleep options make the tests deterministic without real HTTP or timer dependencies. Retry count matches the documented invariant (1 initial + 5 retries = 6 calls). No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.6 AIC · ⌖ 11.9 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report
📊 Metrics (5 tests)
Test Coverage Highlights✅ All 5 tests verify design invariants:
✅ Edge/error scenarios covered:
✅ No violations detected:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor improvements suggested; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Silent exhaustion path — after retries are exhausted, the function returns
[]without a final diagnostic warning, making downstream failures harder to trace. - Incomplete warning coverage in tests — the exhaustion test asserts
callCountbut not the number of warnings emitted, leaving the observability path under-tested.
Positive Highlights
- ✅ Root cause clearly identified and addressed (race between gateway readiness and backend schema construction)
- ✅ Injectable
fetchFn/sleepmake tests fully deterministic without HTTP or timer stubs - ✅ Configurable constants (
TOOLS_EMPTY_MAX_RETRIES,TOOLS_EMPTY_RETRY_DELAY_MS) exported for reuse - ✅ Good test structure: covers immediate success, retry-then-success, exhaustion, sleep timing, and constant shape
- ✅
main()call-site comment explains the retry rationale inline
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.7 AIC · ⌖ 8.02 AIC · ⊞ 7.1K
Comment /matt to run again
| await doSleep(TOOLS_EMPTY_RETRY_DELAY_MS); | ||
| tools = await doFetch(serverUrl, apiKey, core); | ||
| } | ||
| return tools; |
There was a problem hiding this comment.
[/diagnosing-bugs] After exhausting all retries, the function silently returns [] with no final diagnostic — the retry warnings stop but there is no closing signal when the loop ends with tools.length === 0.
💡 Suggested fix
Add a final warning after the loop so that a slow-but-broken backend leaves a clear trace:
if (tools.length === 0) {
core.warning(` tools/list still returned 0 tools for '${serverName}' after ${TOOLS_EMPTY_MAX_RETRIES} retries — backend may be unavailable`);
}
return tools;Without this, the next failure (e.g. from SERVER_VALIDATORS) will reference an empty tools array with no breadcrumb explaining why.
@copilot please address this.
| ); | ||
| expect(result).toEqual([]); | ||
| // 1 initial attempt + TOOLS_EMPTY_MAX_RETRIES retries | ||
| expect(callCount).toBe(1 + TOOLS_EMPTY_MAX_RETRIES); |
There was a problem hiding this comment.
[/tdd] The exhaustion test verifies callCount but doesn't assert on the number of warnings emitted — making it possible to silently remove all retry-warning calls without a failing test.
💡 Suggested addition
const warnings = [];
// pass { warning: msg => warnings.push(msg) } to core
// ...
expect(warnings).toHaveLength(TOOLS_EMPTY_MAX_RETRIES);This pins the warning behaviour alongside the call-count assertion so that both the retry loop and its observability signal are covered.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Comment (non-blocking)
Sound, well-tested fix for a real race condition, but the documented latency bound is inaccurate and each retry leaks a fresh unclosed MCP session.
������ Themes
- Retry loop reuses
fetchMCPToolswholesale, so worst-case latency is much higher than the "≤5s extra" claimed in the PR description (each attempt can itself take up to ~30-40s on a hung backend). - No session cleanup between retry attempts — 5 separate MCP sessions may be opened for one server before giving up.
- Test coverage for the new function itself is good (injectable fetch/sleep, exhaustion, success paths all covered).
🔎 Code quality review by PR Code Quality Reviewer · auto · 43.1 AIC · ⌖ 4.46 AIC · ⊞ 7.8K
Comment /review to run again
| async function fetchMCPToolsWithRetry(serverUrl, apiKey, serverName, core, { sleep = undefined, fetchFn = undefined } = {}) { | ||
| const doSleep = sleep ?? (ms => new Promise(resolve => setTimeout(resolve, ms))); | ||
| const doFetch = fetchFn ?? ((url, key, c) => fetchMCPTools(url, key, c)); | ||
| let tools = await doFetch(serverUrl, apiKey, core); | ||
| for (let attempt = 1; attempt <= TOOLS_EMPTY_MAX_RETRIES && tools.length === 0; attempt++) { | ||
| core.warning(` tools/list returned 0 tools for '${serverName}', retrying in ${TOOLS_EMPTY_RETRY_DELAY_MS}ms (attempt ${attempt}/${TOOLS_EMPTY_MAX_RETRIES})...`); | ||
| await doSleep(TOOLS_EMPTY_RETRY_DELAY_MS); | ||
| tools = await doFetch(serverUrl, apiKey, core); |
There was a problem hiding this comment.
Worst-case retry latency can massively exceed the documented "≤ 5s extra" claim.
������ details
Each retry calls fetchMCPTools, which internally makes up to 3 sequential HTTP calls (initialize, notifications/initialized, tools/list), each with its own DEFAULT_HTTP_TIMEOUT_MS (15s). A single retry attempt against a hung/slow backend can therefore take up to ~30-40s, not a fraction of a second. With 5 retries plus the initial call, worst-case total latency before failing could exceed 3+ minutes, not the "≤5s extra" described in the PR description. Consider a shorter per-attempt timeout for the retry path, or clarify/adjust the stated latency bound.
| async function fetchMCPToolsWithRetry(serverUrl, apiKey, serverName, core, { sleep = undefined, fetchFn = undefined } = {}) { | ||
| const doSleep = sleep ?? (ms => new Promise(resolve => setTimeout(resolve, ms))); | ||
| const doFetch = fetchFn ?? ((url, key, c) => fetchMCPTools(url, key, c)); | ||
| let tools = await doFetch(serverUrl, apiKey, core); |
There was a problem hiding this comment.
Each retry re-runs the full initialize + notifications/initialized handshake, creating a new MCP session per attempt that is never closed.
|
@copilot Please address the remaining review feedback on this PR, then run the pr-finisher skill. Outstanding review items observed:
Branch refresh was requested for this PR. After that, run the pr-finisher skill.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented in d7641d7. The retry path now distinguishes successful-empty tools/list responses from fetch/transport/protocol failures, retries only the successful-empty case, emits a final exhaustion warning when empty retries are exhausted, and includes tighter unit coverage for exhaustion warning emission and failure short-circuit behavior. |
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "batch_review",
"category": "bug",
"pr_number": 49759,
"risk": "medium"
}
|
The MCP gateway reports a backend as
"running"once its process accepts connections, not once it finishes building its tool schema. Forsafeoutputswith a largedispatch-workflowconfig (e.g. 44 workflows with richworkflow_dispatchinput schemas), schema construction takes long enough that a single unretriedtools/listcall immediately after the health check passes can still return 0 tools — triggering an unrecoverable failure.Changes
mount_mcp_as_cli.cjs— addsfetchMCPToolsWithRetrywhich retriestools/listup toTOOLS_EMPTY_MAX_RETRIES(5) times withTOOLS_EMPTY_RETRY_DELAY_MS(1000 ms) between attempts when the result is empty.main()now uses this instead of the barefetchMCPTools. A genuinely broken backend still fails fast (≤5 s extra); a slow-but-healthy backend gets a chance to finish registering tools.mount_mcp_as_cli.test.cjs— adds 5 tests covering immediate success, retry-then-success, retry exhaustion, sleep invocation, and exported constant shape.fetchFnandsleepare injectable via an options argument for synchronous testing without real HTTP or timer delays.