Repository navigation
fix(server): deliver in-flight tool call responses during graceful shutdown #402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -966,14 +966,19 @@ export function createServer( | |
| localAgentProviders, | ||
| close: () => { | ||
| closePromise ??= (async () => { | ||
| await toolActivities.waitForIdle(); | ||
| // Tool handlers resolving only starts the JSON-RPC response chain; | ||
| // it settles several microtasks later. Yield to the macrotask queue | ||
| // so in-flight responses are delivered before mcpHandler.close() | ||
| // rejects their still-open exchanges (HTTP 499). | ||
| await new Promise<void>((resolve) => setImmediate(resolve)); | ||
|
Comment on lines
+969
to
+974
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An already-accepted request can enter a tool during the new This must be fixed before merging. Prevent further tool entry before declaring the tracker drained, or account for accepted requests that can still enter. Drain admitted tools and their responses before closing the handler and process sessions, and add a regression for this late-entry case. Knowledge Base Used: CLI commands and server startup ArtifactsFull executable HTTP late-entry validation script
Base behavior for an accepted request released during shutdown
HEAD behavior showing lost response and process interruption
|
||
| try { | ||
| await mcpHandler.close(); | ||
|
Comment on lines
+972
to
976
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '65,125p' src/server.ts
sed -n '900,990p' src/server.ts
sed -n '595,660p' src/server.test.ts
rg -n 'mcpNodeHandler|mcpHandler|trackToolActivity|waitForIdle|setImmediate|response' src/server.ts src/server.test.tsRepository: Waishnav/devspace Length of output: 9024 🏁 Script executed: set -o pipefail
printf '%s\n' '--- handler construction and tracking ---'
sed -n '740,875p' src/server.ts
printf '%s\n' '--- package and lockfile versions ---'
rg -n -F --glob 'package.json' --glob 'pnpm-lock.yaml' -e '"@modelcontextprotocol/node"' -e '"@modelcontextprotocol/sdk"' -e '"@modelcontextprotocol/ext-apps"' package.json pnpm-lock.yaml || true
printf '%s\n' '--- dependency source candidates ---'
find . -path '*/node_modules/@modelcontextprotocol/*' -type f \( -name '*node*handler*' -o -name '*transport*' -o -name 'index.js' -o -name 'index.ts' \) -print 2>/dev/null | head -80
printf '%s\n' '--- shutdown and MCP lifecycle references ---'
rg -n -F --glob '*.ts' -e 'createMcpHandler' -e 'toNodeHandler' -e 'mcpHandler.close' -e 'close()' src test tests 2>/dev/null || true
printf '%s\n' '--- dependency/cache source references ---'
find . -maxdepth 4 -type f \( -name 'package.json' -o -name 'node.js' -o -name 'node.mjs' -o -name '*.d.ts' \) -path '*mcp*' -print 2>/dev/null | head -100Repository: Waishnav/devspace Length of output: 15695 🌐 Web query:
💡 Result: 🏁 Script executed: printf '%s\n' '--- MCP request helpers and shutdown test setup ---'
rg -n -F --glob 'src/server.test.ts' -e 'function postModernMcp' -e 'postModernMcp(' -e '"_meta"' -e 'shutdown waits for an active MCP tool call' src/server.test.ts
sed -n '500,655p' src/server.test.ts
printf '%s\n' '--- package manager and dependency declarations ---'
sed -n '1,85p' package.json
find . -maxdepth 2 -type f \( -name 'pnpm-lock.yaml' -o -name 'package-lock.json' -o -name 'yarn.lock' \) -printRepository: Waishnav/devspace Length of output: 8160 🌐 Web query:
💡 Result: 🏁 Script executed: printf '%s\n' '--- postModernMcp helper ---'
sed -n '900,940p' src/server.test.ts
printf '%s\n' '--- lockfile resolved MCP packages ---'
rg -n -A8 -B2 -F -e '@modelcontextprotocol/server@' -e '@modelcontextprotocol/node@' pnpm-lock.yaml | head -80Repository: Waishnav/devspace Length of output: 3465 🌐 Web query:
💡 Result: Wait for the MCP response promise before closing the handler. The tracker observes only the tool handler. The modern MCP exchange remains in flight while Suggested fix const toolActivities = new ToolActivityTracker();
+const mcpRequests = new ToolActivityTracker();
...
try {
- await mcpNodeHandler(req, res, req.body);
+ await mcpRequests.track(() => mcpNodeHandler(req, res, req.body));
} catch (error) {
...
await toolActivities.waitForIdle();
- await new Promise<void>((resolve) => setImmediate(resolve));
+ await mcpRequests.waitForIdle();
try {🤖 Prompt for AI Agents |
||
| } catch (error) { | ||
| logEvent(config.logging, "warn", "mcp_handler_close_failed", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| await toolActivities.waitForIdle(); | ||
| processSessions.shutdown(); | ||
| oauthProvider.close(); | ||
| workspaceStore.close?.(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 7683
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 41883
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 980
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 719
🌐 Web query:
site:nodejs.org/api/http.html server.close existing keep-alive connection stops accepting new connections💡 Result:
🌐 Web query:
site:github.com/nodejs/node server.close keep-alive existing connection accepts another request after close _http_server.js💡 Result:
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 17747
🏁 Script executed:
Repository: Waishnav/devspace
Length of output: 11384
Gate new
/mcprequests before draining tool activities.httpServer.close()stops new connections, but it does not reject requests on an active connection whose current response is unfinished. Node can dispatch a pipelined request on that connection afterwaitForIdle()returns. The request can reachmcpNodeHandler, thenmcpHandler.close()can reject its exchange with HTTP 499.Set the closing gate before waiting and reject new
/mcprequests.Suggested fix
🤖 Prompt for AI Agents