Skip to content

Commit 26546e3

Browse files
alistair3149claude
andcommitted
Fail reliably when the conformance server dies
The cleanup path awaited a child exit event that could already have fired, hanging the promise chain and letting Node exit 0 after a server crash — a crashed HTTP transport passed the check. The exit promise is now armed at spawn so cleanup always resolves, a pre-spawn probe rejects a port already occupied by another process instead of running conformance against it, and the doctor's spawned server is pinned to the stdio transport so an exported MCP_TRANSPORT cannot skew it. Also caps the mcp-checks CI job at 15 minutes and documents the conformance port and its PORT override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 406aa4e commit 26546e3

3 files changed

Lines changed: 27 additions & 9 deletions

File tree

.github/workflows/ci.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ jobs:
2525

2626
mcp-checks:
2727
runs-on: ubuntu-24.04-arm
28+
timeout-minutes: 15
2829
steps:
2930
- uses: actions/checkout@v7
3031

docs/testing.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ Token-free structural checks against the built server: a stdio doctor sweep plus
7878
npm run check:mcp
7979
```
8080

81-
Both use the minimal wiki configuration in `scripts/mcp-checks.config.json`, so results do not depend on your local `config.json`. CI runs the same script on every pull request, and additionally diffs the tool surface against the PR base — a removed or renamed tool fails the job, while description changes only show up in the report.
81+
Both use the minimal wiki configuration in `scripts/mcp-checks.config.json`, so results do not depend on your local `config.json`. The conformance run binds `127.0.0.1:3117`; set `PORT` to use a different port. CI runs the same script on every pull request, and additionally diffs the tool surface against the PR base — a removed or renamed tool fails the job, while description changes only show up in the report.
8282

8383
## MCP Inspector CLI (integration tests)
8484

scripts/mcp-checks.cjs

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ function runDoctor() {
3737
REPO_ROOT,
3838
'--env',
3939
`CONFIG=${CONFIG_PATH}`,
40+
'--env',
41+
'MCP_TRANSPORT=stdio',
4042
'--no-telemetry',
4143
'--quiet',
4244
],
@@ -78,25 +80,37 @@ function runDoctor() {
7880
return true;
7981
}
8082

83+
async function respondsOverHttp() {
84+
try {
85+
// Any HTTP response, including an error status, means something
86+
// is accepting connections on the port.
87+
await fetch(SERVER_URL, { method: 'GET', signal: AbortSignal.timeout(2000) });
88+
return true;
89+
} catch {
90+
return false;
91+
}
92+
}
93+
8194
async function waitForServer(child) {
8295
const deadline = Date.now() + STARTUP_TIMEOUT_MS;
8396
while (Date.now() < deadline) {
8497
if (child.exitCode !== null) {
8598
return false;
8699
}
87-
try {
88-
// Any HTTP response, including an error status, means the
89-
// transport is accepting connections.
90-
await fetch(SERVER_URL, { method: 'GET' });
100+
if (await respondsOverHttp()) {
91101
return true;
92-
} catch {
93-
await new Promise((resolve) => setTimeout(resolve, 250));
94102
}
103+
await new Promise((resolve) => setTimeout(resolve, 250));
95104
}
96105
return false;
97106
}
98107

99108
async function runConformance() {
109+
if (await respondsOverHttp()) {
110+
console.error(`Port ${PORT} is already in use by another process — set PORT to a free port.`);
111+
return false;
112+
}
113+
100114
console.log(`\nStarting HTTP transport on port ${PORT}...`);
101115
const serverLog = [];
102116
const child = spawn(process.execPath, [DIST_ENTRY], {
@@ -111,9 +125,12 @@ async function runConformance() {
111125
});
112126
child.stdout.on('data', (chunk) => serverLog.push(chunk));
113127
child.stderr.on('data', (chunk) => serverLog.push(chunk));
128+
// Armed before the child can possibly exit, so the finally block's
129+
// await resolves even when the child is already dead by then.
130+
const exited = new Promise((resolve) => child.once('exit', resolve));
114131

115132
try {
116-
if (!(await waitForServer(child))) {
133+
if (!(await waitForServer(child)) || child.exitCode !== null) {
117134
console.error('HTTP transport did not become reachable:');
118135
console.error(serverLog.join(''));
119136
return false;
@@ -142,7 +159,7 @@ async function runConformance() {
142159
return true;
143160
} finally {
144161
child.kill('SIGTERM');
145-
await new Promise((resolve) => child.once('exit', resolve));
162+
await exited;
146163
}
147164
}
148165

0 commit comments

Comments
 (0)