Draft
Conversation
Deploying docs previews for c827729 (available for 14 days) |
End-to-end Test Summary
Failed Test SummaryNo failed tests ✨ |
Unit Test Summary
Failed Test SummaryNo failed tests ✨ |
bmingles
force-pushed
the
DH-21947_mcp-sessions
branch
from
46fe31b to
e02b22d
Compare
…npm package as a production dependency to enable ConnectionUtils.fetchVariableDefinition method Installed @deephaven/jsapi-utils@1.12.1 as a production dependency (#DH-21947)
…in getTableOrError() with ConnectionUtils.fetchVariableDefinition from @deephaven/jsapi-utils. Current implementation at line 172-180 in tableUtils.ts calls session.getObject() with type: 'Table' which fails for rollup tables and other table variants. Should: 1) Import ConnectionUtils from @deephaven/jsapi-utils 2) Call fetchVariableDefinition to get actual variable type 3) Pass retrieved type to session.getObject() Imported fetchVariableDefinition from @deephaven/jsapi-utils; replaced hardcoded type:'Table' in getTableOrError with fetchVariableDefinition for tableName case (casting IdeSession as IdeConnection since both have subscribeToFieldUpdates) and subscribeToFieldUpdates lookup for variableId case; updated test mock to include subscribeToFieldUpdates (#DH-21947)
….spec.ts to verify rollup table support. Current tests only mock basic Table objects. Add test cases that: 1) Mock a rollup table with different type returned by fetchVariableDefinition 2) Verify getTableOrError correctly fetches rollup tables 3) Ensure both variableId and tableName paths work with non-Table types. Run tests with: npx vitest run src/mcp/utils/tableUtils.spec.ts Added fetchVariableDefinition mock, updated existing success test to use it, and added two new rollup table test cases (one for tableName path, one for variableId path). All 16 tests pass. (#DH-21947)
… rollup tables. Three MCP tools use getTableOrError: getTableData.ts (line 104), getTableStats.ts (line 81), getColumnStats.ts (line 82). Run their test suites to ensure they still work after changes. Consider adding integration test cases for rollup table scenarios if not already covered. Added fetchVariableDefinition mock to getTableData, getTableStats, and getColumnStats test files. Updated assertions to use variable definition returned by fetchVariableDefinition. Added rollup table test case to getTableData.spec.ts. All 30 tests pass. (#DH-21947)
…nstructor to factory method. Currently, McpServer constructor creates ONE SdkMcpServer instance that gets reused (causing race conditions). Need to create a createServer() factory method that returns a new SdkMcpServer instance with all tools registered, so each session gets its own isolated server instance. Refactored McpServer: removed shared 'server' property, added createServer() factory method and registerToolsOnServer(server) helper; start() now calls createServer() per request for isolated instances (#DH-21947)
…McpServer class. Need to store both transports and server instances per session, plus import required utilities for session management (randomUUID, isInitializeRequest). This enables the stateful pattern where each session gets its own isolated server+transport pair. Added randomUUID and isInitializeRequest imports, plus transports and servers Map properties initialized as class fields (#DH-21947)
…andler in start() method. Replace the current stateless pattern (new transport per request) with stateful session management. Extract session ID from mcp-session-id header, reuse existing sessions, or create new session for initialize requests. This is the CRITICAL fix for parallel request race conditions. Replaced stateless per-request transport creation with stateful session management: extracts mcp-session-id header, reuses existing transport for known sessions, creates isolated server+transport pair with sessionIdGenerator for initialize requests, returns 400 for invalid combinations, and cleans up both maps on transport close. (#DH-21947)
….onclose handler. When a session disconnects, need to clean up both the transport AND the server instance from their respective Maps to prevent memory leaks. This handler is called automatically when the client disconnects or the session ends. Added async error handling to transport.onclose: deletes from both Maps, awaits server?.close(), logs errors without throwing (#DH-21947)
…method. When the MCP server is stopping (extension deactivation, server restart, etc), need to close all active sessions cleanly. Iterate through all transports and servers, close them, and clear the Maps. This prevents orphaned connections and ensures proper cleanup. Added graceful shutdown to stop(): iterate transports Map, close each transport and server with try-catch, clear both Maps, then close HTTP server (#DH-21947)
… manual verification. Need to verify that: (1) parallel requests work correctly without race conditions, (2) sessions persist across requests, (3) different sessions are isolated, (4) session cleanup works, (5) all existing MCP tools still function. Use VS Code Copilot or manual HTTP requests to test. Created McpServer.spec.ts with 15 tests verifying: session initialization (initialize request returns session ID), session persistence (same session ID reuses transport), session isolation (parallel inits create distinct sessions with different IDs), parallel requests without race conditions (5 concurrent same-session + 4 cross-session), all 17 MCP tools registered, session Maps cleared on stop(), and port management. All 300 tests pass. (#DH-21947)
…ssion-based architecture. Add notes about session support, implications for clients, and troubleshooting session-related issues. The docs should explain that the server now maintains persistent sessions instead of being stateless. Added session-based architecture section to docs/mcp.md covering: how sessions work with mcp-session-id header, parallel request safety, MCP Apps/notifications support, session lifecycle, and a troubleshooting section for common session issues (404 not found, 400 bad request, stale sessions, port changes) (#DH-21947)
…and architecture docs back to docs/ folder for long-term reference. These documents provide valuable context for understanding the session-based architecture and should be preserved in the main docs directory. Moved mcp-session-migration-plan.md and mcp-session-architecture.md to docs/ directory. No broken references found in docs/mcp.md. (#DH-21947)
bmingles
force-pushed
the
DH-21947_mcp-sessions
branch
from
e02b22d to
c827729
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DH-22113: MCP sessions