Skip to content

fix: close MCP toolset sessions#346

Open
pragnyanramtha wants to merge 12 commits into
google:mainfrom
pragnyanramtha:codex/close-mcp-toolset-sessions
Open

fix: close MCP toolset sessions#346
pragnyanramtha wants to merge 12 commits into
google:mainfrom
pragnyanramtha:codex/close-mcp-toolset-sessions

Conversation

@pragnyanramtha
Copy link
Copy Markdown

Summary

  • track MCP clients created by MCPSessionManager
  • close tracked clients from MCPSessionManager.close() and remove successfully closed clients
  • delegate MCPToolset.close() to its session manager
  • add focused MCP session/toolset lifecycle regressions

Root cause

MCPToolset.close() was a no-op even though MCPToolset creates MCP clients when discovering tools and its MCPTool wrappers can create additional sessions when invoked. Those clients were not tracked for lifecycle cleanup, so callers had no way to release MCP transports through the toolset close path.

Validation

  • npm install
  • npm run build --workspace core
  • npx vitest run --project unit:core core/test/tools/mcp/mcp_session_manager_test.ts core/test/tools/mcp/mcp_toolset_test.ts
  • npx prettier --check core/src/tools/mcp/mcp_session_manager.ts core/src/tools/mcp/mcp_toolset.ts core/test/tools/mcp/mcp_session_manager_test.ts core/test/tools/mcp/mcp_toolset_test.ts
  • npx eslint core/src/tools/mcp/mcp_session_manager.ts core/src/tools/mcp/mcp_toolset.ts core/test/tools/mcp/mcp_session_manager_test.ts core/test/tools/mcp/mcp_toolset_test.ts
  • git diff --check

Note: npm install reports existing dependency audit findings (26 total: 9 low, 4 moderate, 13 high); no dependency changes are included in this PR.

@pragnyanramtha pragnyanramtha marked this pull request as ready for review May 17, 2026 04:10
@kalenkevich
Copy link
Copy Markdown
Collaborator

Related to: #377

Comment thread dev/src/cli_entrypoint.ts Outdated
console.error(e);
}
// Async CLI actions can briefly await before opening a server handle.
const keepAliveUntilCommandStarts = setInterval(() => {}, 2 ** 31 - 1);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert that change

@kalenkevich
Copy link
Copy Markdown
Collaborator

Please revert the dev/src/cli_entrypoint.ts file change.

@pragnyanramtha
Copy link
Copy Markdown
Author

Updated the branch to follow the maintainer feedback and keep the PR scoped to MCP session handling only.\n\nChanges in this push:\n- Reverted the CLI entrypoint change requested by @kalenkevich.\n- Removed the extra CLI stdin workaround as well, so the PR diff no longer touches CLI files.\n- Merged current main and resolved the remaining conflict in the MCP toolset test only.\n- Kept the remaining diff limited to MCP session manager/toolset code and tests.\n\nLocal verification:\n- git diff --check\n- git merge-tree --write-tree --messages HEAD origin/main\n- npm run build\n- npx vitest run --project unit:core core/test/tools/mcp/mcp_session_manager_test.ts core/test/tools/mcp/mcp_toolset_test.ts -> 17 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants