From afc8b0bb6ea51dd6432bd0f248a4043ab86e2311 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Thu, 2 Apr 2026 18:53:11 -0400 Subject: [PATCH] fix(image-analysis): handle installer lock contention --- src/utils/image-analysis/mcp-installer.ts | 185 +++++++++++------- .../image-analysis/mcp-installer.test.ts | 23 +++ 2 files changed, 132 insertions(+), 76 deletions(-) diff --git a/src/utils/image-analysis/mcp-installer.ts b/src/utils/image-analysis/mcp-installer.ts index c8b3e571..5dc5ed5e 100644 --- a/src/utils/image-analysis/mcp-installer.ts +++ b/src/utils/image-analysis/mcp-installer.ts @@ -121,7 +121,7 @@ function writeClaudeUserConfig(configPath: string, config: ClaudeUserConfig): bo } function withClaudeUserConfigLock(configPath: string, callback: () => T): T { - const lockTarget = fs.existsSync(configPath) ? configPath : path.dirname(configPath); + const lockTarget = path.dirname(configPath); let release: (() => void) | undefined; if (!fs.existsSync(path.dirname(configPath))) { @@ -142,6 +142,11 @@ function withClaudeUserConfigLock(configPath: string, callback: () => T): T { } } +function isLockUnavailableError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return code === 'ELOCKED' || code === 'ENOTACQUIRED'; +} + export function hasImageAnalysisMcpServerInstalled(): boolean { return ( fs.existsSync(getImageAnalysisMcpServerPath()) && @@ -183,52 +188,66 @@ function removeManagedServerConfig(configPath: string): boolean { return false; } - return withClaudeUserConfigLock(configPath, () => { - const config = readClaudeUserConfig(configPath); - if (config === null) { - if (process.env.CCS_DEBUG) { - console.error(warn(`Malformed Claude config prevents MCP cleanup: ${configPath}`)); + try { + return withClaudeUserConfigLock(configPath, () => { + const config = readClaudeUserConfig(configPath); + if (config === null) { + if (process.env.CCS_DEBUG) { + console.error(warn(`Malformed Claude config prevents MCP cleanup: ${configPath}`)); + } + return false; } - return false; - } - const existingServers = - config.mcpServers && - typeof config.mcpServers === 'object' && - !Array.isArray(config.mcpServers) - ? { ...(config.mcpServers as Record) } - : {}; + const existingServers = + config.mcpServers && + typeof config.mcpServers === 'object' && + !Array.isArray(config.mcpServers) + ? { ...(config.mcpServers as Record) } + : {}; - if (!(IMAGE_ANALYSIS_MCP_SERVER_NAME in existingServers)) { - return false; - } - - delete existingServers[IMAGE_ANALYSIS_MCP_SERVER_NAME]; - - const nextConfig: ClaudeUserConfig = { ...config }; - if (Object.keys(existingServers).length === 0) { - delete nextConfig.mcpServers; - } else { - nextConfig.mcpServers = existingServers; - } - - try { - writeClaudeUserConfig(configPath, nextConfig); - if (process.env.CCS_DEBUG) { - console.error(info(`Removed Image Analysis MCP config from ${configPath}`)); + if (!(IMAGE_ANALYSIS_MCP_SERVER_NAME in existingServers)) { + return false; } - return true; - } catch (error) { + + delete existingServers[IMAGE_ANALYSIS_MCP_SERVER_NAME]; + + const nextConfig: ClaudeUserConfig = { ...config }; + if (Object.keys(existingServers).length === 0) { + delete nextConfig.mcpServers; + } else { + nextConfig.mcpServers = existingServers; + } + + try { + writeClaudeUserConfig(configPath, nextConfig); + if (process.env.CCS_DEBUG) { + console.error(info(`Removed Image Analysis MCP config from ${configPath}`)); + } + return true; + } catch (error) { + if (process.env.CCS_DEBUG) { + console.error( + warn( + `Failed to remove Image Analysis MCP config from ${configPath}: ${(error as Error).message}` + ) + ); + } + return false; + } + }); + } catch (error) { + if (isLockUnavailableError(error)) { if (process.env.CCS_DEBUG) { console.error( warn( - `Failed to remove Image Analysis MCP config from ${configPath}: ${(error as Error).message}` + `Image Analysis MCP cleanup skipped because ${configPath} is locked by another process` ) ); } return false; } - }); + throw error; + } } export function installImageAnalysisMcpServer(): boolean { @@ -331,52 +350,66 @@ export function ensureImageAnalysisMcpConfig(): boolean { env: {}, }; - return withClaudeUserConfigLock(claudeUserConfigPath, () => { - const config = readClaudeUserConfig(claudeUserConfigPath); + try { + return withClaudeUserConfigLock(claudeUserConfigPath, () => { + const config = readClaudeUserConfig(claudeUserConfigPath); - if (config === null) { + if (config === null) { + if (process.env.CCS_DEBUG) { + console.error(warn('Malformed ~/.claude.json prevents Image Analysis MCP provisioning')); + } + return false; + } + + const existingServers = + config.mcpServers && + typeof config.mcpServers === 'object' && + !Array.isArray(config.mcpServers) + ? (config.mcpServers as Record) + : {}; + const currentConfig = existingServers[IMAGE_ANALYSIS_MCP_SERVER_NAME]; + if ( + typeof currentConfig === 'object' && + currentConfig !== null && + JSON.stringify(currentConfig) === JSON.stringify(desiredServerConfig) + ) { + return true; + } + + const nextConfig: ClaudeUserConfig = { + ...config, + mcpServers: { + ...existingServers, + [IMAGE_ANALYSIS_MCP_SERVER_NAME]: desiredServerConfig, + }, + }; + + try { + writeClaudeUserConfig(claudeUserConfigPath, nextConfig); + if (process.env.CCS_DEBUG) { + console.error(info(`Ensured Image Analysis MCP config in ${claudeUserConfigPath}`)); + } + return true; + } catch (error) { + if (process.env.CCS_DEBUG) { + console.error(warn(`Failed to update ~/.claude.json: ${(error as Error).message}`)); + } + return false; + } + }); + } catch (error) { + if (isLockUnavailableError(error)) { if (process.env.CCS_DEBUG) { - console.error(warn('Malformed ~/.claude.json prevents Image Analysis MCP provisioning')); + console.error( + warn( + `Image Analysis MCP provisioning skipped because ${claudeUserConfigPath} is locked by another process` + ) + ); } return false; } - - const existingServers = - config.mcpServers && - typeof config.mcpServers === 'object' && - !Array.isArray(config.mcpServers) - ? (config.mcpServers as Record) - : {}; - const currentConfig = existingServers[IMAGE_ANALYSIS_MCP_SERVER_NAME]; - if ( - typeof currentConfig === 'object' && - currentConfig !== null && - JSON.stringify(currentConfig) === JSON.stringify(desiredServerConfig) - ) { - return true; - } - - const nextConfig: ClaudeUserConfig = { - ...config, - mcpServers: { - ...existingServers, - [IMAGE_ANALYSIS_MCP_SERVER_NAME]: desiredServerConfig, - }, - }; - - try { - writeClaudeUserConfig(claudeUserConfigPath, nextConfig); - if (process.env.CCS_DEBUG) { - console.error(info(`Ensured Image Analysis MCP config in ${claudeUserConfigPath}`)); - } - return true; - } catch (error) { - if (process.env.CCS_DEBUG) { - console.error(warn(`Failed to update ~/.claude.json: ${(error as Error).message}`)); - } - return false; - } - }); + throw error; + } } export function ensureImageAnalysisMcp(): boolean { diff --git a/tests/unit/utils/image-analysis/mcp-installer.test.ts b/tests/unit/utils/image-analysis/mcp-installer.test.ts index cae4076a..cc82e884 100644 --- a/tests/unit/utils/image-analysis/mcp-installer.test.ts +++ b/tests/unit/utils/image-analysis/mcp-installer.test.ts @@ -184,10 +184,33 @@ describe('ensureImageAnalysisMcp', () => { it('serializes ~/.claude.json updates with a file lock', () => { setupTempHome(); writeEnabledConfig(); + const claudeUserConfigPath = path.join(tempHome as string, '.claude.json'); + fs.writeFileSync(claudeUserConfigPath, '{}\n', 'utf8'); const lockSpy = spyOn(lockfile, 'lockSync'); expect(ensureImageAnalysisMcp()).toBe(true); expect(lockSpy).toHaveBeenCalled(); + expect(lockSpy.mock.calls[0]?.[0]).toBe(tempHome as string); + }); + + it('returns false instead of throwing when ~/.claude.json is already locked', () => { + setupTempHome(); + writeEnabledConfig(); + + const claudeUserConfigPath = path.join(tempHome as string, '.claude.json'); + fs.writeFileSync(claudeUserConfigPath, '{}\n', 'utf8'); + + const release = lockfile.lockSync(tempHome as string, { stale: 10000 }) as () => void; + + try { + let result: boolean | undefined; + expect(() => { + result = ensureImageAnalysisMcp(); + }).not.toThrow(); + expect(result).toBe(false); + } finally { + release(); + } }); });