diff --git a/src/utils/browser/mcp-installer.ts b/src/utils/browser/mcp-installer.ts index e5e8552c..082b62d5 100644 --- a/src/utils/browser/mcp-installer.ts +++ b/src/utils/browser/mcp-installer.ts @@ -4,10 +4,13 @@ import * as fs from 'fs'; import * as path from 'path'; -import * as lockfile from 'proper-lockfile'; import { InstanceManager } from '../../management/instance-manager'; import { getClaudeUserConfigPath } from '../claude-config-path'; import { getCcsDir } from '../config-manager'; +import { + isClaudeUserConfigLockUnavailableError as isLockUnavailableError, + withClaudeUserConfigLock, +} from '../claude-user-config-lock'; const BROWSER_MCP_SERVER = 'ccs-browser-server.cjs'; const BROWSER_MCP_SERVER_NAME = 'ccs-browser'; @@ -133,38 +136,6 @@ function writeClaudeUserConfig(configPath: string, config: ClaudeUserConfig): bo } } -function withClaudeUserConfigLock(configPath: string, callback: () => T): T { - const configDir = path.dirname(configPath); - const lockTarget = path.join(configDir, `${path.basename(configPath)}.ccs-lock`); - let release: (() => void) | undefined; - - if (!fs.existsSync(configDir)) { - fs.mkdirSync(configDir, { recursive: true, mode: 0o700 }); - } - - if (!fs.existsSync(lockTarget)) { - fs.writeFileSync(lockTarget, '', { encoding: 'utf8', mode: 0o600 }); - } - - try { - release = lockfile.lockSync(lockTarget, { stale: 10000 }) as () => void; - return callback(); - } finally { - if (release) { - try { - release(); - } catch { - // Best-effort release. - } - } - } -} - -function isLockUnavailableError(error: unknown): boolean { - const code = (error as NodeJS.ErrnoException | undefined)?.code; - return code === 'ELOCKED' || code === 'ENOTACQUIRED'; -} - function removeManagedServerConfig(configPath: string): boolean { if (!fs.existsSync(configPath)) { return false; diff --git a/src/utils/claude-user-config-lock.ts b/src/utils/claude-user-config-lock.ts new file mode 100644 index 00000000..df3da9ba --- /dev/null +++ b/src/utils/claude-user-config-lock.ts @@ -0,0 +1,102 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as lockfile from 'proper-lockfile'; + +const DEFAULT_LOCK_STALE_MS = 10000; +const DEFAULT_LOCK_RETRY_DELAY_MS = 200; +const DEFAULT_LOCK_RETRY_TIMEOUT_MS = 10000; + +interface LockOptions { + staleMs?: number; + retryDelayMs?: number; + retryTimeoutMs?: number; +} + +export function withClaudeUserConfigLock( + configPath: string, + callback: () => T, + options: LockOptions = {} +): T { + const configDir = path.dirname(configPath); + const lockTarget = path.join(configDir, `${path.basename(configPath)}.ccs-lock`); + const staleMs = options.staleMs ?? DEFAULT_LOCK_STALE_MS; + const retryDelayMs = options.retryDelayMs ?? DEFAULT_LOCK_RETRY_DELAY_MS; + const retryTimeoutMs = options.retryTimeoutMs ?? DEFAULT_LOCK_RETRY_TIMEOUT_MS; + let release: (() => void) | undefined; + + if (!fs.existsSync(configDir)) { + fs.mkdirSync(configDir, { recursive: true, mode: 0o700 }); + } + + if (!fs.existsSync(lockTarget)) { + fs.writeFileSync(lockTarget, '', { encoding: 'utf8', mode: 0o600 }); + } + + try { + release = acquireClaudeUserConfigLock( + lockTarget, + configPath, + staleMs, + retryDelayMs, + retryTimeoutMs + ); + return callback(); + } finally { + if (release) { + try { + release(); + } catch {} + } + } +} + +export function isClaudeUserConfigLockUnavailableError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return code === 'ELOCKED' || code === 'ENOTACQUIRED'; +} + +function acquireClaudeUserConfigLock( + lockTarget: string, + configPath: string, + staleMs: number, + retryDelayMs: number, + retryTimeoutMs: number +): () => void { + const startedAt = Date.now(); + let lastError: unknown; + + for (;;) { + try { + return lockfile.lockSync(lockTarget, { stale: staleMs }) as () => void; + } catch (error) { + if (!isClaudeUserConfigLockUnavailableError(error)) { + throw error; + } + lastError = error; + if (Date.now() - startedAt >= retryTimeoutMs) { + throw buildLockTimeoutError(configPath, retryTimeoutMs, lastError); + } + sleepSync(retryDelayMs); + } + } +} + +function sleepSync(ms: number): void { + if (ms <= 0) { + return; + } + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +function buildLockTimeoutError( + configPath: string, + timeoutMs: number, + cause: unknown +): NodeJS.ErrnoException { + const causeCode = (cause as NodeJS.ErrnoException | undefined)?.code; + const error = new Error( + `Failed to acquire Claude user config lock for ${configPath} after ${timeoutMs}ms; another CCS process may be starting CLIProxy` + ) as NodeJS.ErrnoException; + error.code = causeCode === 'ENOTACQUIRED' ? 'ENOTACQUIRED' : 'ELOCKED'; + return error; +} diff --git a/src/utils/image-analysis/mcp-installer.ts b/src/utils/image-analysis/mcp-installer.ts index 64bd9e35..ce9d106d 100644 --- a/src/utils/image-analysis/mcp-installer.ts +++ b/src/utils/image-analysis/mcp-installer.ts @@ -4,7 +4,6 @@ import * as fs from 'fs'; import * as path from 'path'; -import * as lockfile from 'proper-lockfile'; import { getCcsDir } from '../config-manager'; import { getClaudeUserConfigPath } from '../claude-config-path'; @@ -12,6 +11,10 @@ import { info, warn } from '../ui'; import { InstanceManager } from '../../management/instance-manager'; import { installImageAnalysisPrompts } from './hook-installer'; import { getImageAnalysisConfig } from '../../config/config-loader-facade'; +import { + isClaudeUserConfigLockUnavailableError as isLockUnavailableError, + withClaudeUserConfigLock, +} from '../claude-user-config-lock'; const IMAGE_ANALYSIS_MCP_SERVER = 'ccs-image-analysis-server.cjs'; const IMAGE_ANALYSIS_MCP_RUNTIME = 'image-analysis-runtime.cjs'; @@ -121,38 +124,6 @@ function writeClaudeUserConfig(configPath: string, config: ClaudeUserConfig): bo } } -function withClaudeUserConfigLock(configPath: string, callback: () => T): T { - const configDir = path.dirname(configPath); - const lockTarget = path.join(configDir, `${path.basename(configPath)}.ccs-lock`); - let release: (() => void) | undefined; - - if (!fs.existsSync(configDir)) { - fs.mkdirSync(configDir, { recursive: true, mode: 0o700 }); - } - - if (!fs.existsSync(lockTarget)) { - fs.writeFileSync(lockTarget, '', { encoding: 'utf8', mode: 0o600 }); - } - - try { - release = lockfile.lockSync(lockTarget, { stale: 10000 }) as () => void; - return callback(); - } finally { - if (release) { - try { - release(); - } catch { - // Best-effort release. - } - } - } -} - -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()) && diff --git a/src/utils/websearch/mcp-installer.ts b/src/utils/websearch/mcp-installer.ts index 967860ba..64ad1bea 100644 --- a/src/utils/websearch/mcp-installer.ts +++ b/src/utils/websearch/mcp-installer.ts @@ -12,6 +12,10 @@ import { InstanceManager } from '../../management/instance-manager'; import { installWebSearchHook } from './hook-installer'; import { appendWebSearchTrace } from './trace'; import { getWebSearchConfig } from '../../config/config-loader-facade'; +import { + isClaudeUserConfigLockUnavailableError as isLockUnavailableError, + withClaudeUserConfigLock, +} from '../claude-user-config-lock'; const WEBSEARCH_MCP_SERVER = 'ccs-websearch-server.cjs'; const WEBSEARCH_MCP_SERVER_NAME = 'ccs-websearch'; @@ -118,47 +122,63 @@ function removeManagedServerConfig(configPath: string): boolean { return false; } - const config = readClaudeUserConfig(configPath); - if (config === null) { - if (process.env.CCS_DEBUG) { - console.error(warn(`Malformed Claude config prevents MCP cleanup: ${configPath}`)); - } - return false; - } - - const existingServers = - config.mcpServers && typeof config.mcpServers === 'object' && !Array.isArray(config.mcpServers) - ? { ...(config.mcpServers as Record) } - : {}; - - if (!(WEBSEARCH_MCP_SERVER_NAME in existingServers)) { - return false; - } - - delete existingServers[WEBSEARCH_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 WebSearch MCP config from ${configPath}`)); - } - return true; + 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; + } + + const existingServers = + config.mcpServers && + typeof config.mcpServers === 'object' && + !Array.isArray(config.mcpServers) + ? { ...(config.mcpServers as Record) } + : {}; + + if (!(WEBSEARCH_MCP_SERVER_NAME in existingServers)) { + return false; + } + + delete existingServers[WEBSEARCH_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 WebSearch MCP config from ${configPath}`)); + } + return true; + } catch (error) { + if (process.env.CCS_DEBUG) { + console.error( + warn( + `Failed to remove WebSearch MCP config from ${configPath}: ${(error as Error).message}` + ) + ); + } + return false; + } + }); } catch (error) { - if (process.env.CCS_DEBUG) { - console.error( - warn( - `Failed to remove WebSearch MCP config from ${configPath}: ${(error as Error).message}` - ) - ); + if (isLockUnavailableError(error)) { + appendWebSearchTrace('websearch_mcp_config_remove_skipped', { + reason: 'user_config_locked', + configPath, + error: (error as Error).message, + }); + return false; } - return false; + throw error; } } @@ -243,67 +263,86 @@ export function ensureWebSearchMcpConfig(): boolean { } const claudeUserConfigPath = getClaudeUserConfigPath(); - const claudeUserConfigDir = path.dirname(claudeUserConfigPath); - const config = readClaudeUserConfig(claudeUserConfigPath); - - if (config === null) { - appendWebSearchTrace('websearch_mcp_config_failed', { reason: 'malformed_user_config' }); - if (process.env.CCS_DEBUG) { - console.error(warn('Malformed ~/.claude.json prevents WebSearch MCP provisioning')); - } - return false; - } - - if (!fs.existsSync(claudeUserConfigDir)) { - fs.mkdirSync(claudeUserConfigDir, { recursive: true, mode: 0o700 }); - } - - const existingServers = - config.mcpServers && typeof config.mcpServers === 'object' && !Array.isArray(config.mcpServers) - ? (config.mcpServers as Record) - : {}; - const desiredServerConfig: ManagedWebSearchMcpConfig = { - type: 'stdio', - command: 'node', - args: [getWebSearchMcpServerPath()], - env: {}, - }; - - const currentConfig = existingServers[WEBSEARCH_MCP_SERVER_NAME]; - if ( - typeof currentConfig === 'object' && - currentConfig !== null && - JSON.stringify(currentConfig) === JSON.stringify(desiredServerConfig) - ) { - appendWebSearchTrace('websearch_mcp_config_ready', { configPath: claudeUserConfigPath }); - return true; - } - - const nextConfig: ClaudeUserConfig = { - ...config, - mcpServers: { - ...existingServers, - [WEBSEARCH_MCP_SERVER_NAME]: desiredServerConfig, - }, - }; try { - writeClaudeUserConfig(claudeUserConfigPath, nextConfig); - appendWebSearchTrace('websearch_mcp_config_ready', { configPath: claudeUserConfigPath }); - if (process.env.CCS_DEBUG) { - console.error(info(`Ensured WebSearch MCP config in ${claudeUserConfigPath}`)); - } - return true; - } catch (error) { - appendWebSearchTrace('websearch_mcp_config_failed', { - reason: 'write_failed', - configPath: claudeUserConfigPath, - error: (error as Error).message, + return withClaudeUserConfigLock(claudeUserConfigPath, () => { + const config = readClaudeUserConfig(claudeUserConfigPath); + + if (config === null) { + appendWebSearchTrace('websearch_mcp_config_failed', { reason: 'malformed_user_config' }); + if (process.env.CCS_DEBUG) { + console.error(warn('Malformed ~/.claude.json prevents WebSearch MCP provisioning')); + } + return false; + } + + const existingServers = + config.mcpServers && + typeof config.mcpServers === 'object' && + !Array.isArray(config.mcpServers) + ? (config.mcpServers as Record) + : {}; + const desiredServerConfig: ManagedWebSearchMcpConfig = { + type: 'stdio', + command: 'node', + args: [getWebSearchMcpServerPath()], + env: {}, + }; + + const currentConfig = existingServers[WEBSEARCH_MCP_SERVER_NAME]; + if ( + typeof currentConfig === 'object' && + currentConfig !== null && + JSON.stringify(currentConfig) === JSON.stringify(desiredServerConfig) + ) { + appendWebSearchTrace('websearch_mcp_config_ready', { configPath: claudeUserConfigPath }); + return true; + } + + const nextConfig: ClaudeUserConfig = { + ...config, + mcpServers: { + ...existingServers, + [WEBSEARCH_MCP_SERVER_NAME]: desiredServerConfig, + }, + }; + + try { + writeClaudeUserConfig(claudeUserConfigPath, nextConfig); + appendWebSearchTrace('websearch_mcp_config_ready', { configPath: claudeUserConfigPath }); + if (process.env.CCS_DEBUG) { + console.error(info(`Ensured WebSearch MCP config in ${claudeUserConfigPath}`)); + } + return true; + } catch (error) { + appendWebSearchTrace('websearch_mcp_config_failed', { + reason: 'write_failed', + configPath: claudeUserConfigPath, + error: (error as Error).message, + }); + if (process.env.CCS_DEBUG) { + console.error(warn(`Failed to update ~/.claude.json: ${(error as Error).message}`)); + } + return false; + } }); - if (process.env.CCS_DEBUG) { - console.error(warn(`Failed to update ~/.claude.json: ${(error as Error).message}`)); + } catch (error) { + if (isLockUnavailableError(error)) { + appendWebSearchTrace('websearch_mcp_config_failed', { + reason: 'user_config_locked', + configPath: claudeUserConfigPath, + error: (error as Error).message, + }); + if (process.env.CCS_DEBUG) { + console.error( + warn( + `WebSearch MCP config skipped because ${claudeUserConfigPath} is locked by another process` + ) + ); + } + return false; } - return false; + throw error; } } diff --git a/tests/unit/utils/image-analysis/mcp-installer.test.ts b/tests/unit/utils/image-analysis/mcp-installer.test.ts index 40b617f5..f3a9032a 100644 --- a/tests/unit/utils/image-analysis/mcp-installer.test.ts +++ b/tests/unit/utils/image-analysis/mcp-installer.test.ts @@ -194,26 +194,54 @@ describe('ensureImageAnalysisMcp', () => { expect(lockSpy.mock.calls[0]?.[0]).toBe(path.join(tempHome as string, '.claude.json.ccs-lock')); }); - it('returns false instead of throwing when ~/.claude.json is already locked', () => { + it('retries ~/.claude.json lock acquisition before provisioning', () => { + setupTempHome(); + writeEnabledConfig(); + const claudeUserConfigPath = path.join(tempHome as string, '.claude.json'); + fs.writeFileSync(claudeUserConfigPath, '{}\n', 'utf8'); + + let calls = 0; + spyOn(lockfile, 'lockSync').mockImplementation(() => { + calls++; + if (calls === 1) { + const error = new Error('Lock file is already being held') as NodeJS.ErrnoException; + error.code = 'ELOCKED'; + throw error; + } + return (() => undefined) as ReturnType; + }); + + expect(ensureImageAnalysisMcp()).toBe(true); + expect(calls).toBe(2); + + const config = JSON.parse(fs.readFileSync(claudeUserConfigPath, 'utf8')) as { + mcpServers: Record; + }; + expect(config.mcpServers[getImageAnalysisMcpServerName()]).toEqual(getManagedConfig()); + }); + + it('returns false instead of throwing when ~/.claude.json stays locked', () => { setupTempHome(); writeEnabledConfig(); const claudeUserConfigPath = path.join(tempHome as string, '.claude.json'); fs.writeFileSync(claudeUserConfigPath, '{}\n', 'utf8'); - fs.writeFileSync(path.join(tempHome as string, '.claude.json.ccs-lock'), '', 'utf8'); - const release = lockfile.lockSync(path.join(tempHome as string, '.claude.json.ccs-lock'), { - stale: 10000, - }) as () => void; + let now = 0; + spyOn(Date, 'now').mockImplementation(() => { + now += 10001; + return now; + }); + spyOn(lockfile, 'lockSync').mockImplementation(() => { + const error = new Error('Lock file is already being held') as NodeJS.ErrnoException; + error.code = 'ELOCKED'; + throw error; + }); - try { - let result: boolean | undefined; - expect(() => { - result = ensureImageAnalysisMcp(); - }).not.toThrow(); - expect(result).toBe(false); - } finally { - release(); - } + let result: boolean | undefined; + expect(() => { + result = ensureImageAnalysisMcp(); + }).not.toThrow(); + expect(result).toBe(false); }); }); diff --git a/tests/unit/utils/websearch/mcp-installer.test.ts b/tests/unit/utils/websearch/mcp-installer.test.ts index 5fb07d34..03de5847 100644 --- a/tests/unit/utils/websearch/mcp-installer.test.ts +++ b/tests/unit/utils/websearch/mcp-installer.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, mock, spyOn } from 'bun:test'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; +import * as lockfile from 'proper-lockfile'; import { getHookPath } from '../../../../src/utils/websearch/hook-config'; import { ensureWebSearchMcp, @@ -33,7 +34,15 @@ describe('ensureWebSearchMcp', () => { fs.mkdirSync(ccsDir, { recursive: true }); fs.writeFileSync( path.join(ccsDir, 'config.yaml'), - ['version: 12', 'websearch:', ' enabled: true', ' providers:', ' duckduckgo:', ' enabled: true', ''].join('\n'), + [ + 'version: 12', + 'websearch:', + ' enabled: true', + ' providers:', + ' duckduckgo:', + ' enabled: true', + '', + ].join('\n'), 'utf8' ); } @@ -95,6 +104,19 @@ describe('ensureWebSearchMcp', () => { expect(config.mcpServers[getWebSearchMcpServerName()]).toEqual(getManagedConfig()); }); + 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(ensureWebSearchMcp()).toBe(true); + expect(lockSpy).toHaveBeenCalled(); + expect(lockSpy.mock.calls[0]?.[0]).toBe(path.join(tempHome as string, '.claude.json.ccs-lock')); + }); + it('preserves the existing ~/.claude.json permissions when provisioning WebSearch MCP', () => { setupTempHome(); writeEnabledConfig(); @@ -170,7 +192,11 @@ describe('ensureWebSearchMcp', () => { 'utf8' ); + const lockSpy = spyOn(lockfile, 'lockSync'); + expect(uninstallWebSearchMcp()).toBe(true); + expect(lockSpy).toHaveBeenCalled(); + expect(lockSpy.mock.calls[0]?.[0]).toBe(path.join(tempHome as string, '.claude.json.ccs-lock')); expect(fs.existsSync(getWebSearchMcpServerPath())).toBe(false); const globalConfig = JSON.parse(fs.readFileSync(claudeUserConfigPath, 'utf8')) as {