Skip to content

fix: prevent disabled MCP servers from starting processes and show correct status #6084

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/core/webview/webviewMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,13 @@ export const webviewMessageHandler = async (
case "mcpEnabled":
const mcpEnabled = message.bool ?? true
await updateGlobalState("mcpEnabled", mcpEnabled)

// Delegate MCP enable/disable logic to McpHub
const mcpHubInstance = provider.getMcpHub()
if (mcpHubInstance) {
await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)
}

await provider.postStateToWebview()
break
case "enableMcpServerCreation":
Expand Down
225 changes: 209 additions & 16 deletions src/services/mcp/McpHub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,29 @@ import { fileExistsAtPath } from "../../utils/fs"
import { arePathsEqual } from "../../utils/path"
import { injectVariables } from "../../utils/config"

export type McpConnection = {
// Discriminated union for connection states
export type ConnectedMcpConnection = {
type: "connected"
server: McpServer
client: Client
transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport
}

export type DisconnectedMcpConnection = {
type: "disconnected"
server: McpServer
client: null
transport: null
}

export type McpConnection = ConnectedMcpConnection | DisconnectedMcpConnection

// Enum for disable reasons
export enum DisableReason {
MCP_DISABLED = "mcpDisabled",
SERVER_DISABLED = "serverDisabled",
}

// Base configuration schema for common settings
const BaseConfigSchema = z.object({
disabled: z.boolean().optional(),
Expand Down Expand Up @@ -497,6 +514,7 @@ export class McpHub {
const result = McpSettingsSchema.safeParse(config)

if (result.success) {
// Pass all servers including disabled ones - they'll be handled in updateServerConnections
await this.updateServerConnections(result.data.mcpServers || {}, source, false)
} else {
const errorMessages = result.error.errors
Expand Down Expand Up @@ -552,6 +570,49 @@ export class McpHub {
await this.initializeMcpServers("project")
}

/**
* Creates a placeholder connection for disabled servers or when MCP is globally disabled
* @param name The server name
* @param config The server configuration
* @param source The source of the server (global or project)
* @param reason The reason for creating a placeholder (mcpDisabled or serverDisabled)
* @returns A placeholder DisconnectedMcpConnection object
*/
private createPlaceholderConnection(
name: string,
config: z.infer<typeof ServerConfigSchema>,
source: "global" | "project",
reason: DisableReason,
): DisconnectedMcpConnection {
return {
type: "disconnected",
server: {
name,
config: JSON.stringify(config),
status: "disconnected",
disabled: reason === DisableReason.SERVER_DISABLED ? true : config.disabled,
source,
projectPath: source === "project" ? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath : undefined,
errorHistory: [],
},
client: null,
transport: null,
}
}

/**
* Checks if MCP is globally enabled
* @returns Promise<boolean> indicating if MCP is enabled
*/
private async isMcpEnabled(): Promise<boolean> {
const provider = this.providerRef.deref()
if (!provider) {
return true // Default to enabled if provider is not available
}
const state = await provider.getState()
return state.mcpEnabled ?? true
}

private async connectToServer(
name: string,
config: z.infer<typeof ServerConfigSchema>,
Expand All @@ -560,6 +621,26 @@ export class McpHub {
// Remove existing connection if it exists with the same source
await this.deleteConnection(name, source)

// Check if MCP is globally enabled
const mcpEnabled = await this.isMcpEnabled()
if (!mcpEnabled) {
// Still create a connection object to track the server, but don't actually connect
const connection = this.createPlaceholderConnection(name, config, source, DisableReason.MCP_DISABLED)
this.connections.push(connection)
return
}

// Skip connecting to disabled servers
if (config.disabled) {
// Still create a connection object to track the server, but don't actually connect
const connection = this.createPlaceholderConnection(name, config, source, DisableReason.SERVER_DISABLED)
this.connections.push(connection)
return
}

// Set up file watchers for enabled servers
this.setupFileWatcher(name, config, source)

try {
const client = new Client(
{
Expand Down Expand Up @@ -733,7 +814,9 @@ export class McpHub {
transport.start = async () => {}
}

const connection: McpConnection = {
// Create a connected connection
const connection: ConnectedMcpConnection = {
type: "connected",
server: {
name,
config: JSON.stringify(configInjected),
Expand Down Expand Up @@ -826,8 +909,8 @@ export class McpHub {
// Use the helper method to find the connection
const connection = this.findConnection(serverName, source)

if (!connection) {
throw new Error(`Server ${serverName} not found`)
if (!connection || connection.type !== "connected") {
return []
}

const response = await connection.client.request({ method: "tools/list" }, ListToolsResultSchema)
Expand Down Expand Up @@ -881,7 +964,7 @@ export class McpHub {
private async fetchResourcesList(serverName: string, source?: "global" | "project"): Promise<McpResource[]> {
try {
const connection = this.findConnection(serverName, source)
if (!connection) {
if (!connection || connection.type !== "connected") {
return []
}
const response = await connection.client.request({ method: "resources/list" }, ListResourcesResultSchema)
Expand All @@ -898,7 +981,7 @@ export class McpHub {
): Promise<McpResourceTemplate[]> {
try {
const connection = this.findConnection(serverName, source)
if (!connection) {
if (!connection || connection.type !== "connected") {
return []
}
const response = await connection.client.request(
Expand All @@ -913,15 +996,20 @@ export class McpHub {
}

async deleteConnection(name: string, source?: "global" | "project"): Promise<void> {
// Clean up file watchers for this server
this.removeFileWatchersForServer(name)

// If source is provided, only delete connections from that source
const connections = source
? this.connections.filter((conn) => conn.server.name === name && conn.server.source === source)
: this.connections.filter((conn) => conn.server.name === name)

for (const connection of connections) {
try {
await connection.transport.close()
await connection.client.close()
if (connection.type === "connected") {
await connection.transport.close()
await connection.client.close()
}
} catch (error) {
console.error(`Failed to close transport for ${name}:`, error)
}
Expand Down Expand Up @@ -975,15 +1063,21 @@ export class McpHub {
if (!currentConnection) {
// New server
try {
this.setupFileWatcher(name, validatedConfig, source)
// Only setup file watcher for enabled servers
if (!validatedConfig.disabled) {
this.setupFileWatcher(name, validatedConfig, source)
}
await this.connectToServer(name, validatedConfig, source)
} catch (error) {
this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
}
} else if (!deepEqual(JSON.parse(currentConnection.server.config), config)) {
// Existing server with changed config
try {
this.setupFileWatcher(name, validatedConfig, source)
// Only setup file watcher for enabled servers
if (!validatedConfig.disabled) {
this.setupFileWatcher(name, validatedConfig, source)
}
await this.deleteConnection(name, source)
await this.connectToServer(name, validatedConfig, source)
} catch (error) {
Expand Down Expand Up @@ -1066,10 +1160,21 @@ export class McpHub {
this.fileWatchers.clear()
}

private removeFileWatchersForServer(serverName: string) {
const watchers = this.fileWatchers.get(serverName)
if (watchers) {
watchers.forEach((watcher) => watcher.close())
this.fileWatchers.delete(serverName)
}
}

async restartConnection(serverName: string, source?: "global" | "project"): Promise<void> {
this.isConnecting = true
const provider = this.providerRef.deref()
if (!provider) {

// Check if MCP is globally enabled
const mcpEnabled = await this.isMcpEnabled()
if (!mcpEnabled) {
this.isConnecting = false
return
}

Expand Down Expand Up @@ -1111,6 +1216,23 @@ export class McpHub {
return
}

// Check if MCP is globally enabled
const mcpEnabled = await this.isMcpEnabled()
if (!mcpEnabled) {
// Clear all existing connections
const existingConnections = [...this.connections]
for (const conn of existingConnections) {
await this.deleteConnection(conn.server.name, conn.server.source)
}

// Still initialize servers to track them, but they won't connect
await this.initializeMcpServers("global")
await this.initializeMcpServers("project")

await this.notifyWebviewOfServerChanges()
return
}

this.isConnecting = true
vscode.window.showInformationMessage(t("mcp:info.refreshing_all"))

Expand Down Expand Up @@ -1257,8 +1379,21 @@ export class McpHub {
try {
connection.server.disabled = disabled

// Only refresh capabilities if connected
if (connection.server.status === "connected") {
// If disabling a connected server, disconnect it
if (disabled && connection.server.status === "connected") {
// Clean up file watchers when disabling
this.removeFileWatchersForServer(serverName)
await this.deleteConnection(serverName, serverSource)
// Re-add as a disabled connection
await this.connectToServer(serverName, JSON.parse(connection.server.config), serverSource)
} else if (!disabled && connection.server.status === "disconnected") {
// If enabling a disabled server, connect it
const config = JSON.parse(connection.server.config)
await this.deleteConnection(serverName, serverSource)
// When re-enabling, file watchers will be set up in connectToServer
await this.connectToServer(serverName, config, serverSource)
} else if (connection.server.status === "connected") {
// Only refresh capabilities if connected
connection.server.tools = await this.fetchToolsList(serverName, serverSource)
connection.server.resources = await this.fetchResourcesList(serverName, serverSource)
connection.server.resourceTemplates = await this.fetchResourceTemplatesList(
Expand Down Expand Up @@ -1439,7 +1574,7 @@ export class McpHub {

async readResource(serverName: string, uri: string, source?: "global" | "project"): Promise<McpResourceResponse> {
const connection = this.findConnection(serverName, source)
if (!connection) {
if (!connection || connection.type !== "connected") {
throw new Error(`No connection found for server: ${serverName}${source ? ` with source ${source}` : ""}`)
}
if (connection.server.disabled) {
Expand All @@ -1463,7 +1598,7 @@ export class McpHub {
source?: "global" | "project",
): Promise<McpToolCallResponse> {
const connection = this.findConnection(serverName, source)
if (!connection) {
if (!connection || connection.type !== "connected") {
throw new Error(
`No connection found for server: ${serverName}${source ? ` with source ${source}` : ""}. Please make sure to use MCP servers available under 'Connected MCP Servers'.`,
)
Expand Down Expand Up @@ -1609,6 +1744,64 @@ export class McpHub {
}
}

/**
* Handles enabling/disabling MCP globally
* @param enabled Whether MCP should be enabled or disabled
* @returns Promise<void>
*/
async handleMcpEnabledChange(enabled: boolean): Promise<void> {
Copy link

Choose a reason for hiding this comment

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

The handleMcpEnabledChange method cleanly disconnects all servers when MCP is globally disabled and then refreshes connections. However, inline fallback strings (using the || operator) in translation calls should be removed to adhere to i18n best practices.

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

if (!enabled) {
// If MCP is being disabled, disconnect all servers with error handling
const existingConnections = [...this.connections]
const disconnectionErrors: Array<{ serverName: string; error: string }> = []

for (const conn of existingConnections) {
try {
await this.deleteConnection(conn.server.name, conn.server.source)
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error)
disconnectionErrors.push({
serverName: conn.server.name,
error: errorMessage,
})
console.error(`Failed to disconnect MCP server ${conn.server.name}: ${errorMessage}`)
}
}

// If there were errors, notify the user
if (disconnectionErrors.length > 0) {
const errorSummary = disconnectionErrors.map((e) => `${e.serverName}: ${e.error}`).join("\n")
vscode.window.showWarningMessage(
t("mcp:errors.disconnect_servers_partial", {
count: disconnectionErrors.length,
errors: errorSummary,
}) ||
`Failed to disconnect ${disconnectionErrors.length} MCP server(s). Check the output for details.`,
)
}

// Re-initialize servers to track them in disconnected state
try {
await this.refreshAllConnections()
} catch (error) {
console.error(`Failed to refresh MCP connections after disabling: ${error}`)
vscode.window.showErrorMessage(
t("mcp:errors.refresh_after_disable") || "Failed to refresh MCP connections after disabling",
)
}
} else {
// If MCP is being enabled, reconnect all servers
try {
await this.refreshAllConnections()
} catch (error) {
console.error(`Failed to refresh MCP connections after enabling: ${error}`)
vscode.window.showErrorMessage(
t("mcp:errors.refresh_after_enable") || "Failed to refresh MCP connections after enabling",
)
}
}
}

async dispose(): Promise<void> {
// Prevent multiple disposals
if (this.isDisposed) {
Expand Down
Loading