Fix connection handling #17
Conversation
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
レビュー概要
このPRは、MCPサーバーのコネクションハンドリングを改善する目的で実装されていますが、重大なリソース管理の問題があります。
🚨 重要な問題
- リソースリーク:
res.on('close')イベントに依存したリソース管理は、Lambda環境では信頼性に欠けます - エラーハンドリングの劣化: 元のコードにあった堅牢なエラーハンドリングとクリーンアップロジックが失われています
📋 主な修正要求
- 必須:
try-finallyパターンを使用した確実なリソースクリーンアップの実装 - 必須:
Promise.allSettledを使用した安全なクローズ処理の復活 - 推奨: ログ出力の一貫性確保（
console.log→logger.trace） - 推奨: 新しい依存関係
fetch-to-nodeのセキュリティ監査
💡 提案
元のコードの堅牢なリソース管理パターンを維持しつつ、新しいトランスポート実装を統合することを推奨します。現在の実装は、参考にしたコードがサーバー環境向けであるのに対し、Lambda環境では異なる考慮が必要です。
コードの品質と安全性を確保するため、上記の修正を適用してから再レビューをお願いします。
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | ||
| enableJsonResponse: true, | ||
| }); |
There was a problem hiding this comment.
🛑 リソースリーク: res.on('close') イベントハンドラーによるリソース管理に重大な問題があります。Lambda環境では、レスポンスの close イベントが適切に発火しない可能性があり、その結果 transport.close() と server.close() が呼ばれずにリソースリークが発生する恐れがあります1。
元のコードの try-finally パターンの方が確実にリソースクリーンアップを保証できます。現在の実装では、Lambda関数の実行が完了してもMCPサーバーとトランスポートが適切にクローズされない可能性があります。
Footnotes
-
Lambda環境では、レスポンスストリームの動作が通常のNode.jsサーバーと異なる場合があります ↩
| }); | ||
| try { | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） |
There was a problem hiding this comment.
ログレベルの不整合: console.log を使用していますが、他の箇所では logger インスタンスを使用しています。一貫性を保つため、ここでも logger.trace または logger.debug を使用することを推奨します。
|
|
||
| const closeResources = async (server: McpServer, transport: StreamableHTTPTransport) => { | ||
| // 両方のクローズを確実に実行（片方が失敗してももう片方を実行） | ||
| const closeResults = await Promise.allSettled([ | ||
| transport.close(), | ||
| server.close(), | ||
| ]); | ||
|
|
||
| // クローズエラーをログ出力 | ||
| closeResults.forEach((result, index) => { | ||
| if (result.status === 'rejected') { | ||
| const resourceName = index === 0 ? 'transport' : 'server'; | ||
| const error = result.reason; | ||
| const errorDetails = error instanceof Error | ||
| ? { message: error.message, stack: error.stack } | ||
| : error; | ||
| logger.error(`Error closing ${resourceName}:`, { error: errorDetails }); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| // ルートを設定 | ||
| app.post('/mcp', async (c) => { | ||
| const { req, res } = toReqRes(c.req.raw); | ||
| const server = createMcpServer(); | ||
| const transport = new StreamableHTTPTransport({ | ||
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | ||
| enableJsonResponse: true, | ||
| }); | ||
| try { | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | ||
| enableJsonResponse: true, | ||
| }); | ||
| await server.connect(transport); | ||
| try { | ||
| logger.trace('MCP リクエストを受信'); | ||
| return await transport.handleRequest(c); | ||
| } catch (error) { | ||
| return handleError(c, error, 'MCP リクエスト処理中のエラー:'); | ||
| } finally { | ||
| await closeResources(server, transport); | ||
| } | ||
| logger.trace('MCP リクエストを受信'); | ||
| await transport.handleRequest(req, res, await c.req.json()); |
There was a problem hiding this comment.
エラーハンドリングの改善が必要: 現在の実装では、transport.handleRequest() でエラーが発生した場合のリソースクリーンアップが不十分です。以下の改善を提案します：
try-catch-finallyパターンを使用してリソースの確実なクリーンアップを保証transport.close()とserver.close()の呼び出し時のエラーハンドリング- 元のコードにあった
Promise.allSettledを使用した安全なクリーンアップロジックの復活
| const closeResources = async (server: McpServer, transport: StreamableHTTPTransport) => { | |
| // 両方のクローズを確実に実行（片方が失敗してももう片方を実行） | |
| const closeResults = await Promise.allSettled([ | |
| transport.close(), | |
| server.close(), | |
| ]); | |
| // クローズエラーをログ出力 | |
| closeResults.forEach((result, index) => { | |
| if (result.status === 'rejected') { | |
| const resourceName = index === 0 ? 'transport' : 'server'; | |
| const error = result.reason; | |
| const errorDetails = error instanceof Error | |
| ? { message: error.message, stack: error.stack } | |
| : error; | |
| logger.error(`Error closing ${resourceName}:`, { error: errorDetails }); | |
| } | |
| }); | |
| }; | |
| // ルートを設定 | |
| app.post('/mcp', async (c) => { | |
| const { req, res } = toReqRes(c.req.raw); | |
| const server = createMcpServer(); | |
| const transport = new StreamableHTTPTransport({ | |
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | |
| enableJsonResponse: true, | |
| }); | |
| try { | |
| const transport = new StreamableHTTPServerTransport({ | |
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | |
| enableJsonResponse: true, | |
| }); | |
| await server.connect(transport); | |
| try { | |
| logger.trace('MCP リクエストを受信'); | |
| return await transport.handleRequest(c); | |
| } catch (error) { | |
| return handleError(c, error, 'MCP リクエスト処理中のエラー:'); | |
| } finally { | |
| await closeResources(server, transport); | |
| } | |
| logger.trace('MCP リクエストを受信'); | |
| await transport.handleRequest(req, res, await c.req.json()); | |
| const { req, res } = toReqRes(c.req.raw); | |
| const server = createMcpServer(); | |
| const transport = new StreamableHTTPServerTransport({ | |
| sessionIdGenerator: undefined, // セッションIDを生成しない（ステートレスモード） | |
| enableJsonResponse: true, | |
| }); | |
| try { | |
| await server.connect(transport); | |
| logger.trace('MCP リクエストを受信'); | |
| await transport.handleRequest(req, res, await c.req.json()); | |
| return toFetchResponse(res); | |
| } catch (error) { | |
| return handleError(c, error, 'MCP 接続中のエラー:'); | |
| } finally { | |
| // 両方のクローズを確実に実行（片方が失敗してももう片方を実行） | |
| const closeResults = await Promise.allSettled([ | |
| transport.close(), | |
| server.close(), | |
| ]); | |
| // クローズエラーをログ出力 | |
| closeResults.forEach((result, index) => { | |
| if (result.status === 'rejected') { | |
| const resourceName = index === 0 ? 'transport' : 'server'; | |
| const error = result.reason; | |
| const errorDetails = error instanceof Error | |
| ? { message: error.message, stack: error.stack } | |
| : error; | |
| logger.error(`Error closing ${resourceName}:`, { error: errorDetails }); | |
| } | |
| }); | |
| } |
| }); | ||
| }; | ||
|
|
||
| // ルートを設定 |
There was a problem hiding this comment.
トランスポートインスタンス化のタイミング: トランスポートを try ブロック内で作成することで、接続エラー時にもトランスポートのクリーンアップが必要になります。元のコードのように、トランスポートを事前に作成し、接続エラー時は適切にクリーンアップする方が安全です。
| ); | ||
| }; | ||
|
|
||
| const closeResources = async (server: McpServer, transport: StreamableHTTPTransport) => { |
There was a problem hiding this comment.
削除された重要な機能: closeResources 関数は、リソースの安全なクリーンアップを担保する重要な機能でした。この関数が提供していた以下の機能が失われています：
Promise.allSettledによる両方のリソースの確実なクリーンアップ- 個別のクローズエラーのログ出力
- 一方のクローズが失敗しても他方を実行する堅牢性
この機能を復活させることを強く推奨します。
概要
コネクションハンドリングを修正
変更点
fetch-to-nodeを追加関連Issue
確認事項
pnpm audit --fixで脆弱性を修正済みか？pnpm lint-fixでコードスタイルは修正済みか？markdownlint-2で Markdown の lint は修正済みか？特記事項