-
Notifications
You must be signed in to change notification settings - Fork 10
fix(mcp-server): add diagnostic logging for 500 errors #1456
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
Conversation
1 new issue
|
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (1)
🛟 Help
|
03e3119 to
2b97a07
Compare
f205005 to
dad6ffb
Compare
packages/mcp-server/src/server.ts
Outdated
| res.status(500).json({ | ||
| error: 'internal_server_error', | ||
| error_description: err.message, | ||
| error_description: 'Internal server error', |
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.
The global Express error handler was returning err.message directly in the HTTP response (error_description: err.message). This could leak internal details (file paths, database errors, stack traces embedded
in messages) to external clients. The detailed error is still logged server-side via logErrorWithStack, but the response now returns a generic "Internal server error" message, consistent with how the agent's
error-handling.ts sanitizes errors in production.
| const mcpMethod = req.body?.method || 'unknown'; | ||
| const err = error instanceof Error ? error : new Error(String(error)); | ||
| logErrorWithStack( | ||
| this.logger, | ||
| `MCP Error on method '${mcpMethod}': ${err.message || error}`, | ||
| err, | ||
| ); | ||
|
|
||
| if (!res.headersSent) { | ||
| res.status(500).json({ | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: -32603, | ||
| message: (error as Error)?.message || 'Internal server error', | ||
| message: err.message || 'Internal server error', | ||
| }, | ||
| id: null, | ||
| }); | ||
| } else { | ||
| this.logger( | ||
| 'Warn', | ||
| `Cannot send error response (headers already sent) for method '${mcpMethod}'`, | ||
| ); |
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.
why don't we use here the error handler defined below ?
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.
We can't use the global error handler here because next is not available in this .catch() callback — we're inside a Promise chain, not an Express middleware. Even if we could call next(err), the global handler would do the same thing (log + check headersSent), but with less context (no mcpMethod).
Replace hardcoded port numbers with dynamically assigned ports to avoid EADDRINUSE errors in CI environments where ports may already be in use. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Improve logging to diagnose production 500 errors (0ms response, no useful info):
- Replace fragile res.end wrapping with res.on('finish') in logging middleware
- Log JSON-RPC method name in handleMcpRequest
- Add stack traces to MCP error catch and global error handler
- Log request method/path in global error handler
- Use Error level for 5xx, Warn for 4xx in response logging
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssing tests - Log a warning when error response cannot be sent because headers were already sent - Add test for Warn log level on 4xx responses - Add test for MCP error handler logging method name and stack trace Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…anch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ror" - Catch tool handler errors and return them as isError results instead of throwing, because the MCP SDK replaces thrown messages with generic text - Expose real error messages in dev mode (isProduction: false) in the agent error handler, keeping "Unexpected error" in production for security Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove incorrect claim that MCP SDK replaces thrown error messages - Normalize non-Error values in catch handler to avoid undefined message - Fix misleading comment about arrow function context - Stop leaking error details in global error handler response Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2cd4749 to
ffc0dd1
Compare
| return 'Info'; | ||
| } | ||
|
|
||
| function logErrorWithStack(logger: Logger, message: string, err: Error): void { |
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.
why taking an error and a message ? Message can be retrieved from the error 🤔
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.
The message param adds context that err.message alone doesn't have — e.g. "MCP Error on method 'tools/call': <err.message>" or "Unhandled error on POST /mcp: <err.message>". The helper then appends the stack trace on a separate log line. So it's context + error message on line 1, stack on line 2.
Revert out-of-scope changes (isError tool handling, agent dev mode error messages) — these will be in a separate PR. Remove global error handler headersSent else branch per review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## @forestadmin/mcp-server [1.7.7](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/mcp-server@1.7.6...@forestadmin/mcp-server@1.7.7) (2026-02-10) ### Bug Fixes * **mcp-server:** add diagnostic logging for 500 errors ([#1456](#1456)) ([1e61fd7](1e61fd7)) * **mcp-server:** use random available ports in server tests ([#1457](#1457)) ([1fce1ee](1fce1ee))

Summary
res.endwrapping withres.on('finish')in logging middleware — avoids conflicts withinterceptResponseForErrorLoggingwhich also wrapsres.end/res.writeinitialize,tools/list,tools/call, etc.) inhandleMcpRequestto distinguish MCP operations in logs/mcp.catch()block and global Express error handlerErrorlevel for 5xx,Warnfor 4xx,Infofor 2xx/3xx in response loggingContext
A client gets a 500 error on the second
POST /mcp(0ms response time) with no useful info in the logs. The current logging was too sparse to diagnose production issues — the global error handler (which catchesrequireBearerAuthrejections) only loggedUnhandled error: <message>with no path, no stack.Test plan
yarn workspace @forestadmin/mcp-server test— 483 tests passyarn workspace @forestadmin/mcp-server build— compiles cleanlyyarn workspace @forestadmin/mcp-server lint— 0 errors🤖 Generated with Claude Code