Skip to content

fix: race condition and JSON format issues in MCP server management #319

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.

Sign up for GitHub

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

Closed
juniorcammel wants to merge 4 commits into from
Closed

fix: race condition and JSON format issues in MCP server management #319

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f493d2
docs: document MCP server bug fixes and patches
junior-cammel Dec 29, 2025
3fbe662
Merge branch 'main' of https://github.com/juniorcammel/automaker
junior-cammel Dec 29, 2025
1a11405
fix: race condition and JSON format issues in MCP server management
junior-cammel Dec 29, 2025
628ccc9
chore: remove fork-specific documentation and patches
junior-cammel Dec 30, 2025
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
282 changes: 207 additions & 75 deletions apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,15 @@ export function useMCPServers() {
// If editing an existing server, save directly (user already approved it)
if (editingServer) {
updateMCPServer(editingServer.id, serverData);

const syncSuccess = await syncSettingsToServer();

if (!syncSuccess) {
toast.error('Failed to save MCP server to disk');
return;
}

toast.success('MCP server updated');
await syncSettingsToServer();
handleCloseDialog();
return;
}
Comment on lines 277 to 290
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

State inconsistency on sync failure.

When syncSettingsToServer() fails, the local Zustand store already contains the update from updateMCPServer(), but the disk state doesn't. The user sees an error toast, yet the UI shows the updated server. On next app restart, the changes will be lost.

Consider rolling back the local state on sync failure, or at minimum, inform the user that the change exists only locally.

🔎 Proposed fix with rollback 
     if (editingServer) {
+      // Capture previous state for potential rollback
+      const previousState = mcpServers.find(s => s.id === editingServer.id);
       updateMCPServer(editingServer.id, serverData);

       const syncSuccess = await syncSettingsToServer();

       if (!syncSuccess) {
+        // Rollback local state
+        if (previousState) {
+          updateMCPServer(editingServer.id, previousState);
+        }
         toast.error('Failed to save MCP server to disk');
         return;
       }

Expand All @@ -303,14 +310,28 @@ export function useMCPServers() {

if (pendingServerData.type === 'add' && pendingServerData.serverData) {
addMCPServer(pendingServerData.serverData);

const syncSuccess = await syncSettingsToServer();

if (!syncSuccess) {
toast.error('Failed to save MCP server to disk');
return;
}

toast.success('MCP server added');
await syncSettingsToServer();
handleCloseDialog();
} else if (pendingServerData.type === 'import' && pendingServerData.importServers) {
for (const serverData of pendingServerData.importServers) {
addMCPServer(serverData);
}
await syncSettingsToServer();

const syncSuccess = await syncSettingsToServer();

if (!syncSuccess) {
toast.error('Failed to save MCP servers to disk');
return;
}

const count = pendingServerData.importServers.length;
toast.success(`Imported ${count} MCP server${count > 1 ? 's' : ''}`);
setIsImportDialogOpen(false);
Expand All @@ -323,13 +344,27 @@ export function useMCPServers() {

const handleToggleEnabled = async (server: MCPServerConfig) => {
updateMCPServer(server.id, { enabled: !server.enabled });
await syncSettingsToServer();

const syncSuccess = await syncSettingsToServer();

if (!syncSuccess) {
toast.error('Failed to save MCP server to disk');
return;
}

toast.success(server.enabled ? 'Server disabled' : 'Server enabled');
};

const handleDelete = async (id: string) => {
removeMCPServer(id);
await syncSettingsToServer();

const syncSuccess = await syncSettingsToServer();

if (!syncSuccess) {
toast.error('Failed to save changes to disk');
return;
}

setDeleteConfirmId(null);
toast.success('MCP server removed');
};
Comment on lines 358 to 370
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Deleted server reappears after failed sync.

If syncSettingsToServer() fails, the server is removed from UI state but persists on disk. On app restart, the "deleted" server will reappear, confusing the user.

🤖 Prompt for AI Agents 
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 358 to 370, the code removes the server from local state before
persisting changes so if syncSettingsToServer() fails the deleted server remains
on disk and will reappear; change the flow to perform the persistent sync first
and only remove from state after sync succeeds (or keep the optimistic removal
but immediately restore the server if sync fails). Specifically: attempt to sync
the updated settings to server before calling removeMCPServer (or save the
removed server data to a temp variable so you can call addMCPServer to rollback
on failure), show error toast and do not clear delete confirmation when sync
fails, and only clear setDeleteConfirmId and show success toast after successful
sync and state update.

Expand Down Expand Up @@ -569,11 +604,11 @@ export function useMCPServers() {
};

const handleOpenGlobalJsonEdit = () => {
// Build the full mcpServers config object
const exportData: Record<string, Record<string, unknown>> = {};

for (const server of mcpServers) {
// Build array of servers with IDs preserved
const serversArray = mcpServers.map((server) => {
const serverConfig: Record<string, unknown> = {
id: server.id, // Preserve ID
name: server.name, // Preserve name
type: server.type || 'stdio',
};

Expand All @@ -596,102 +631,199 @@ export function useMCPServers() {
}
}

exportData[server.name] = serverConfig;
}
return serverConfig;
});

setGlobalJsonValue(JSON.stringify({ mcpServers: exportData }, null, 2));
setGlobalJsonValue(JSON.stringify({ mcpServers: serversArray }, null, 2));
setIsGlobalJsonEditOpen(true);
};

const handleSaveGlobalJsonEdit = async () => {
try {
const parsed = JSON.parse(globalJsonValue);

// Support both formats
// Support both formats: array and object
const servers = parsed.mcpServers || parsed;

if (typeof servers !== 'object' || Array.isArray(servers)) {
toast.error('Invalid format: expected object with server configurations');
if (Array.isArray(servers)) {
// Array format (new format with IDs)
await handleSaveGlobalJsonArray(servers);
} else if (typeof servers === 'object' && servers !== null) {
// Object format (legacy Claude Desktop format)
await handleSaveGlobalJsonObject(servers);
} else {
toast.error('Invalid format: expected array or object with server configurations');
return;
}

// Validate all servers first
for (const [name, config] of Object.entries(servers)) {
if (typeof config !== 'object' || config === null) {
toast.error(`Invalid config for "${name}"`);
return;
}
const syncSuccess = await syncSettingsToServer();

const serverConfig = config as Record<string, unknown>;
const serverType = (serverConfig.type as string) || 'stdio';
if (!syncSuccess) {
toast.error('Failed to save MCP servers to disk');
return;
}

if (serverType === 'stdio') {
if (!serverConfig.command || typeof serverConfig.command !== 'string') {
toast.error(`Command is required for "${name}" (stdio)`);
return;
}
} else if (serverType === 'sse' || serverType === 'http') {
if (!serverConfig.url || typeof serverConfig.url !== 'string') {
toast.error(`URL is required for "${name}" (${serverType})`);
return;
}
toast.success('MCP servers configuration updated');
setIsGlobalJsonEditOpen(false);
setGlobalJsonValue('');
} catch (error) {
toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error'));
}
Comment on lines +669 to +671
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 29, 2025

Choose a reason for hiding this comment

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

medium

The current error handling prefixes all errors with 'Invalid JSON:', which can be misleading for data validation errors (e.g., a missing server name). It's better to distinguish between JSON syntax errors and other validation errors to provide clearer feedback to the user.

    } catch (error) {
      if (error instanceof SyntaxError) {
        toast.error('Invalid JSON: ' + error.message);
      } else if (error instanceof Error) {
        toast.error(error.message);
      } else {
        toast.error('An unknown error occurred while saving.');
      }
    }

};

// Helper: Process array format (with IDs)
const handleSaveGlobalJsonArray = async (serversArray: unknown[]) => {
// Validate all servers first
for (const config of serversArray) {
if (typeof config !== 'object' || config === null) {
toast.error('Invalid server config in array');
throw new Error('Invalid server config');
}
Comment on lines +678 to +681
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 29, 2025

Choose a reason for hiding this comment

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

medium

To avoid showing duplicate error toasts, the toast.error call should be removed from this helper function. The calling function handleSaveGlobalJsonEdit will now handle displaying the error toast. Please apply this change to all validation checks in both handleSaveGlobalJsonArray and handleSaveGlobalJsonObject, and ensure the Error constructor receives the user-friendly message.

      if (typeof config !== 'object' || config === null) {
        throw new Error('Invalid server config in array');
      }


const serverConfig = config as Record<string, unknown>;
const name = serverConfig.name as string;
const serverType = (serverConfig.type as string) || 'stdio';

if (!name || typeof name !== 'string') {
toast.error('Server name is required');
throw new Error('Server name required');
}

if (serverType === 'stdio') {
if (!serverConfig.command || typeof serverConfig.command !== 'string') {
toast.error(`Command is required for "${name}" (stdio)`);
throw new Error('Command required');
}
} else if (serverType === 'sse' || serverType === 'http') {
if (!serverConfig.url || typeof serverConfig.url !== 'string') {
toast.error(`URL is required for "${name}" (${serverType})`);
throw new Error('URL required');
}
}
}

// Create a map of existing servers by name for updating
const existingByName = new Map(mcpServers.map((s) => [s.name, s]));
const processedNames = new Set<string>();
// Create maps of existing servers by ID and name
const existingById = new Map(mcpServers.map((s) => [s.id, s]));
const existingByName = new Map(mcpServers.map((s) => [s.name, s]));
const processedIds = new Set<string>();

// Update or add servers
for (const [name, config] of Object.entries(servers)) {
const serverConfig = config as Record<string, unknown>;
const serverType = (serverConfig.type as ServerType) || 'stdio';
// Update or add servers
for (const config of serversArray) {
const serverConfig = config as Record<string, unknown>;
const id = serverConfig.id as string | undefined;
const name = serverConfig.name as string;
const serverType = (serverConfig.type as ServerType) || 'stdio';

const serverData: Omit<MCPServerConfig, 'id'> = {
name,
type: serverType,
description: (serverConfig.description as string) || undefined,
enabled: serverConfig.enabled !== false,
};
const serverData: Omit<MCPServerConfig, 'id'> = {
name,
type: serverType,
description: (serverConfig.description as string) || undefined,
enabled: serverConfig.enabled !== false,
};

if (serverType === 'stdio') {
serverData.command = serverConfig.command as string;
if (Array.isArray(serverConfig.args)) {
serverData.args = serverConfig.args as string[];
}
if (typeof serverConfig.env === 'object' && serverConfig.env !== null) {
serverData.env = serverConfig.env as Record<string, string>;
}
} else {
serverData.url = serverConfig.url as string;
if (typeof serverConfig.headers === 'object' && serverConfig.headers !== null) {
serverData.headers = serverConfig.headers as Record<string, string>;
}
if (serverType === 'stdio') {
serverData.command = serverConfig.command as string;
if (Array.isArray(serverConfig.args)) {
serverData.args = serverConfig.args as string[];
}
if (typeof serverConfig.env === 'object' && serverConfig.env !== null) {
serverData.env = serverConfig.env as Record<string, string>;
}
} else {
serverData.url = serverConfig.url as string;
if (typeof serverConfig.headers === 'object' && serverConfig.headers !== null) {
serverData.headers = serverConfig.headers as Record<string, string>;
}
}

const existing = existingByName.get(name);
if (existing) {
updateMCPServer(existing.id, serverData);
} else {
addMCPServer(serverData);
// Try to match by ID first, then by name
const existing = id ? existingById.get(id) : existingByName.get(name);
if (existing) {
updateMCPServer(existing.id, serverData);
processedIds.add(existing.id);
} else {
addMCPServer(serverData);
}
}

// Remove servers that are no longer in the JSON
for (const server of mcpServers) {
if (!processedIds.has(server.id)) {
removeMCPServer(server.id);
}
}
};

// Helper: Process object format (legacy Claude Desktop format)
const handleSaveGlobalJsonObject = async (servers: Record<string, unknown>) => {
// Validate all servers first
for (const [name, config] of Object.entries(servers)) {
if (typeof config !== 'object' || config === null) {
toast.error(`Invalid config for "${name}"`);
throw new Error('Invalid config');
}

const serverConfig = config as Record<string, unknown>;
const serverType = (serverConfig.type as string) || 'stdio';

if (serverType === 'stdio') {
if (!serverConfig.command || typeof serverConfig.command !== 'string') {
toast.error(`Command is required for "${name}" (stdio)`);
throw new Error('Command required');
}
} else if (serverType === 'sse' || serverType === 'http') {
if (!serverConfig.url || typeof serverConfig.url !== 'string') {
toast.error(`URL is required for "${name}" (${serverType})`);
throw new Error('URL required');
}
processedNames.add(name);
}
}

// Create a map of existing servers by name for updating
const existingByName = new Map(mcpServers.map((s) => [s.name, s]));
const processedNames = new Set<string>();

// Remove servers that are no longer in the JSON
for (const server of mcpServers) {
if (!processedNames.has(server.name)) {
removeMCPServer(server.id);
// Update or add servers
for (const [name, config] of Object.entries(servers)) {
const serverConfig = config as Record<string, unknown>;
const serverType = (serverConfig.type as ServerType) || 'stdio';

const serverData: Omit<MCPServerConfig, 'id'> = {
name,
type: serverType,
description: (serverConfig.description as string) || undefined,
enabled: serverConfig.enabled !== false,
};

if (serverType === 'stdio') {
serverData.command = serverConfig.command as string;
if (Array.isArray(serverConfig.args)) {
serverData.args = serverConfig.args as string[];
}
if (typeof serverConfig.env === 'object' && serverConfig.env !== null) {
serverData.env = serverConfig.env as Record<string, string>;
}
} else {
serverData.url = serverConfig.url as string;
if (typeof serverConfig.headers === 'object' && serverConfig.headers !== null) {
serverData.headers = serverConfig.headers as Record<string, string>;
}
}

await syncSettingsToServer();
const existing = existingByName.get(name);
if (existing) {
updateMCPServer(existing.id, serverData);
} else {
addMCPServer(serverData);
}
processedNames.add(name);
}

toast.success('MCP servers configuration updated');
setIsGlobalJsonEditOpen(false);
setGlobalJsonValue('');
} catch (error) {
toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error'));
// Remove servers that are no longer in the JSON
for (const server of mcpServers) {
if (!processedNames.has(server.name)) {
removeMCPServer(server.id);
}
}
};

Expand Down