MCP gateway startup: degrade on optional server failures, fail only on required/no-success cases#46919
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add support in frontmatter to specify a MCP tool as required:false (default true) |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46919 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Makes MCP gateway readiness tolerant of optional server failures while retaining fatal handling for required or universally failing servers.
Changes:
- Adds optional/required failure classification and warning summaries.
- Adds mock-server tests for optional and required failures.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/check_mcp_servers.sh |
Implements revised readiness policy. |
actions/setup/sh/check_mcp_servers_test.sh |
Adds targeted connectivity tests. |
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: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — 2 correctness bugs + 2 test reliability issues
The optional/required split is the right approach, but the implementation has a logic ordering bug that will produce wrong error messages in production, plus test-infrastructure issues that can leave orphaned processes and leaked temp dirs.
Issues found
1. Branch order inverts error semantics (high)
SERVERS_SUCCEEDED == 0 is checked before REQUIRED_SERVERS_FAILED > 0. When all servers are required and all fail, the script exits with No
🔎 Code quality review by PR Code Quality Reviewer · 41.8 AIC · ⌖ 4.82 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/sh/check_mcp_servers.sh:67
Wrong branch order hides required-server failures when zero servers succeed: the SERVERS_SUCCEEDED == 0 branch fires before REQUIRED_SERVERS_FAILED > 0, so if every server is required and all fail, the user sees "No HTTP servers were successfully checked / maybe all stdio-type" — not "required server(s) failed". The actual root cause is invisible in CI logs.
<details>
<summary>💡 Suggested fix</summary>
Swap the branch order so required-failure is checked first:
if [ $REQUIRE…
</details>
<details><summary>actions/setup/sh/check_mcp_servers.sh:60</summary>
**Misleading error message when all optional servers fail**: the `SERVERS_SUCCEEDED == 0` error text lists only two causes — "No HTTP-type MCP servers configured" or "all servers are stdio-type" — but there is now a third reachable cause: all configured HTTP servers failed connectivity but were all optional. A user hitting this branch after a total optional-server outage will see a stdio-type hint and waste time chasing a non-existent config problem.
<details>
<summary>💡 Suggested fix</summar…
</details>
<details><summary>actions/setup/sh/check_mcp_servers_test.sh:38</summary>
**`start_mock_mcp_server` echoes the shell PID, not the Node process PID**: `node - ... <<'NODE' &` backgrounds a shell that in turn execs Node; `$!` is the shell wrapper's PID. Calling `kill "$server_pid"` later kills that wrapper but leaves the Node process orphaned — it keeps the ephemeral port open and can cause port conflicts when tests are re-run back-to-back or in parallel.
<details>
<summary>💡 Suggested fix</summary>
Write the Node PID from inside the JS before the shell exits, or us…
</details>
<details><summary>actions/setup/sh/check_mcp_servers_test.sh:231</summary>
**`tmpdir` leaked when mock server fails to start**: both new tests return early (via `return`) on mock-server startup failure without calling `rm -rf "$tmpdir"`, leaking the temp directory on every test-environment failure.
<details>
<summary>💡 Suggested fix</summary>
Add cleanup before the early return:
```sh
if ! server_pid=$(start_and_validate_mock_server "$port_file" "$tmpdir/mock.log"); then
rm -rf "$tmpdir"
print_result "Mock MCP server failed to start" "FAIL"
return
fiOr…
There was a problem hiding this comment.
Review: MCP gateway startup degradation
The overall design is sound — tolerating optional server failures while preserving hard-fail semantics for required servers is the right approach, and the test coverage is thorough.
One blocking correctness issue found (see inline comment):
Branch-ordering bug at the summary block — when a required: true server fails and is the only server, SERVERS_SUCCEEDED is 0 and the new SERVERS_SUCCEEDED -eq 0 guard fires first, printing a misleading "no HTTP servers configured" error instead of the intended "required server failed" message. The fix is to add && [ $REQUIRED_SERVERS_FAILED -eq 0 ] to the first condition (or reorder so required-failure is checked first).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 30.2 AIC · ⌖ 4.49 AIC · ⊞ 5K
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
placeholder
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit The
Example: mcp-servers:
github:
type: http
url: "http://127.0.0.1:8080/mcp/github"
datadog:
type: http
url: "http://127.0.0.1:8080/mcp/datadog"
required: false # startup continues with a warning if this server fails |
|
@copilot please run the Unresolved review threads to close out:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All four threads have been addressed in code and replied to:
Threads 1–3 are |
|
@copilot please run the Unresolved review threads to close out:
Run: https://github.com/github/gh-aw/actions/runs/29788028039 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All four threads are already fully addressed in code and have replies:
The PR Sous Chef run ( |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29789600762 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All 4 review threads are now resolved. PR Sous Chef run
Ready for merge. |
🔍 PR Triage — Run §29791496271
Rationale: MCP Inspector deterministically failing when Datadog returns 403. Graceful degradation fix with tests. Ready for review.
|
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Actions taken: None in this run — all prior work was already complete. Next concrete maintainer action: Wait for the current CI run ( |
|
🎉 This pull request is included in a new release. Release: |
Start MCP Gatewaywas deterministically failing the MCP Inspector job when a single MCP server (Datadog) returned403duringinitialize. This change makes gateway readiness tolerant of optional server failures while preserving strict failure behavior for startup-critical servers.Startup failure policy in MCP readiness checks
actions/setup/sh/check_mcp_servers.shto treat per-server failures as:required: truerequiredis omitted/falseTargeted coverage for new behavior
actions/setup/sh/check_mcp_servers_test.shwith scenarios for:403oninitialize) => startup check succeeds with warningExample of the new server-level control:
{ "mcpServers": { "github": { "type": "http", "url": "http://127.0.0.1:8080/mcp/github" }, "datadog": { "type": "http", "url": "http://127.0.0.1:8080/mcp/datadog" } } }With the above, Datadog failure degrades to warning if GitHub connects. Marking Datadog as
"required": truerestores fail-fast behavior for that server.403 Forbidden#46916pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/29783168867
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
branch update requested from https://github.com/github/gh-aw/actions/runs/29788028039
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.