diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index ab000b2691..895d9ca963 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -349,6 +349,27 @@ describe('Server Config (config.ts)', () => { }), ); }); + + it('should ignore properties that are explicitly undefined and preserve existing values', () => { + const config = new Config(baseParams); + + config.setShellExecutionConfig({ + terminalWidth: 80, + showColor: true, + }); + + expect(config.getShellExecutionConfig().terminalWidth).toBe(80); + expect(config.getShellExecutionConfig().showColor).toBe(true); + + // Provide undefined for terminalWidth, which should be ignored + config.setShellExecutionConfig({ + terminalWidth: undefined, + showColor: false, + }); + + expect(config.getShellExecutionConfig().terminalWidth).toBe(80); // Should still be 80, not undefined + expect(config.getShellExecutionConfig().showColor).toBe(false); // Should be updated + }); }); beforeEach(() => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 9dbf0f8115..918b114129 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -3396,20 +3396,23 @@ export class Config implements McpContext, AgentLoopContext { return this.shellExecutionConfig; } - setShellExecutionConfig(config: ShellExecutionConfig): void { + setShellExecutionConfig(config: Partial): void { + const definedConfig: Partial = {}; + for (const [k, v] of Object.entries(config)) { + // Only merge properties explicitly provided with a concrete value. + // Filtering out `null` and `undefined` ensures existing system defaults + // are preserved when an extension doesn't want to override them. + if (v != null) { + Object.assign(definedConfig, { [k]: v }); + } + } + + // Note: This performs a shallow merge. If the incoming config provides a nested + // object (e.g., sandboxConfig), it will completely overwrite the existing + // nested object rather than merging its individual properties. this.shellExecutionConfig = { ...this.shellExecutionConfig, - terminalWidth: - config.terminalWidth ?? this.shellExecutionConfig.terminalWidth, - terminalHeight: - config.terminalHeight ?? this.shellExecutionConfig.terminalHeight, - showColor: config.showColor ?? this.shellExecutionConfig.showColor, - pager: config.pager ?? this.shellExecutionConfig.pager, - sanitizationConfig: - config.sanitizationConfig ?? - this.shellExecutionConfig.sanitizationConfig, - sandboxManager: - config.sandboxManager ?? this.shellExecutionConfig.sandboxManager, + ...definedConfig, }; } getScreenReader(): boolean { diff --git a/packages/core/src/config/projectRegistry.test.ts b/packages/core/src/config/projectRegistry.test.ts index a441de8b3e..9d59c1528d 100644 --- a/packages/core/src/config/projectRegistry.test.ts +++ b/packages/core/src/config/projectRegistry.test.ts @@ -300,4 +300,163 @@ describe('ProjectRegistry', () => { 'ProjectRegistry must be initialized before use', ); }); + + it('retries on EBUSY during save', async () => { + const registry = new ProjectRegistry(registryPath); + await registry.initialize(); + + const originalRename = fs.promises.rename; + const renameSpy = vi.spyOn(fs.promises, 'rename'); + let ebusyCount = 0; + + renameSpy.mockImplementation(async (oldPath, newPath) => { + // Only throw for the specific temporary file generated by save() + if (oldPath.toString().includes('.tmp') && ebusyCount < 2) { + ebusyCount++; + const err = Object.assign(new Error('Resource busy or locked'), { + code: 'EBUSY', + }); + throw err; + } + // On success, call the original native rename implementation + return originalRename(oldPath, newPath); + }); + + const projectPath = path.join(tempDir, 'ebusy-project'); + const shortId = await registry.getShortId(projectPath); + expect(shortId).toBe('ebusy-project'); + expect(ebusyCount).toBe(2); + + // Verify it actually saved properly after retries + const data = JSON.parse(fs.readFileSync(registryPath, 'utf8')); + expect(data.projects[normalizePath(projectPath)]).toBe('ebusy-project'); + + renameSpy.mockRestore(); + }); + + it('re-throws error if save ultimately fails after retries', async () => { + const registry = new ProjectRegistry(registryPath); + await registry.initialize(); + + const renameSpy = vi.spyOn(fs.promises, 'rename'); + const expectedError = Object.assign(new Error('Persistent EBUSY'), { + code: 'EBUSY', + }); + + // Mock rename to ALWAYS fail + renameSpy.mockRejectedValue(expectedError); + + const projectPath = path.join(tempDir, 'failing-project'); + await expect(registry.getShortId(projectPath)).rejects.toThrow( + 'Persistent EBUSY', + ); + + renameSpy.mockRestore(); + }); + + it('handles EEXIST gracefully during atomic registry creation (Race Condition Protection)', async () => { + const registry = new ProjectRegistry(registryPath); + await registry.initialize(); + + const originalWriteFile = fs.promises.writeFile; + const writeFileSpy = vi.spyOn(fs.promises, 'writeFile'); + writeFileSpy.mockImplementation(async (filePath, data, options) => { + // Simulate another instance beating us to the lockless file creation + if ( + filePath === registryPath && + (options as { flag?: string }).flag === 'wx' + ) { + const err = Object.assign(new Error('File exists'), { code: 'EEXIST' }); + throw err; + } + return originalWriteFile(filePath, data, options); + }); + + // It should NOT crash, and should successfully generate a short ID + const shortId = await registry.getShortId( + path.join(tempDir, 'race-project'), + ); + expect(shortId).toBe('race-project'); + writeFileSpy.mockRestore(); + }); + + it('performs JIT migration of un-normalized paths for backward compatibility', async () => { + const realDir = path.join(tempDir, 'real-project'); + fs.mkdirSync(realDir, { recursive: true }); + + // Simulate a legacy registry entry containing an un-normalized path. + // We use relative segments to guarantee a mismatch against the canonical path. + const unnormalizedPath = + path.join(tempDir, '.', 'real-project', '..', 'real-project') + path.sep; + + // Calculate the expected canonical path using the same rules as the registry: + // full physical resolution (realpath) and OS-specific casing. + let expectedCanonicalPath = fs.realpathSync.native(path.resolve(realDir)); + if (os.platform() === 'win32') { + expectedCanonicalPath = expectedCanonicalPath.toLowerCase(); + } + + // Seed the registry with the legacy un-normalized format. + fs.writeFileSync( + registryPath, + JSON.stringify({ + projects: { + [unnormalizedPath]: 'migrated-slug', + }, + }), + ); + + const registry = new ProjectRegistry(registryPath); + await registry.initialize(); + + // Force a disk flush by requesting a new project ID. + await registry.getShortId(path.join(tempDir, 'some-new-project')); + + const data = JSON.parse(fs.readFileSync(registryPath, 'utf8')); + + // Verify the legacy path was replaced with the canonical path. + expect(data.projects[unnormalizedPath]).toBeUndefined(); + expect(data.projects[expectedCanonicalPath]).toBe('migrated-slug'); + }); + it('protects against data destruction by throwing on EACCES instead of resetting', async () => { + // 1. Write valid registry data + fs.writeFileSync( + registryPath, + JSON.stringify({ projects: { '/foo': 'bar' } }), + ); + + const registry = new ProjectRegistry(registryPath); + + // 2. Mock readFile to throw a permissions error + const readFileSpy = vi.spyOn(fs.promises, 'readFile'); + readFileSpy.mockRejectedValue( + Object.assign(new Error('Permission denied'), { code: 'EACCES' }), + ); + + // 3. Initialization should NOT swallow the error + await expect(registry.initialize()).rejects.toThrow('Permission denied'); + + readFileSpy.mockRestore(); + }); + + it('cleans up orphaned temporary files in the background', async () => { + // 1. Create a fake orphaned tmp file from an hour ago + const oldTmpFile = path.join(tempDir, 'projects.json.1234.tmp'); + fs.writeFileSync(oldTmpFile, 'junk'); + + // Manually set mtime to 2 hours ago to ensure it gets cleaned up + const twoHoursAgo = Date.now() - 2 * 60 * 60 * 1000; + fs.utimesSync(oldTmpFile, new Date(twoHoursAgo), new Date(twoHoursAgo)); + + const registry = new ProjectRegistry(registryPath); + + // 2. Initialize (which triggers async cleanup) + await registry.initialize(); + + // Give the un-awaited background promise a tick to resolve + await new Promise((resolve) => setTimeout(resolve, 50)); + + // 3. The orphaned file should be gone + expect(fs.existsSync(oldTmpFile)).toBe(false); + }); }); diff --git a/packages/core/src/config/projectRegistry.ts b/packages/core/src/config/projectRegistry.ts index c58fb55ce8..b8e66e7245 100644 --- a/packages/core/src/config/projectRegistry.ts +++ b/packages/core/src/config/projectRegistry.ts @@ -10,6 +10,7 @@ import * as path from 'node:path'; import * as os from 'node:os'; import { lock } from 'proper-lockfile'; import { debugLogger } from '../utils/debugLogger.js'; +import { isNodeError } from '../utils/errors.js'; export interface RegistryData { projects: Record; @@ -19,6 +20,13 @@ const PROJECT_ROOT_FILE = '.project_root'; const LOCK_TIMEOUT_MS = 10000; const LOCK_RETRY_DELAY_MS = 100; +class SlugCollisionError extends Error { + constructor(message: string) { + super(message); + this.name = 'SlugCollisionError'; + } +} + /** * Manages a mapping between absolute project paths and short, human-readable identifiers. * This helps reduce context bloat and makes temporary directories easier to work with. @@ -48,29 +56,83 @@ export class ProjectRegistry { } this.data = await this.loadData(); + + // Cleanup orphaned .tmp files in the background without blocking initialization + this.cleanupOrphanedTempFiles().catch((e) => { + debugLogger.debug('Failed to cleanup orphaned temp files', e); + }); })(); return this.initPromise; } - private async loadData(): Promise { - if (!fs.existsSync(this.registryPath)) { - return { projects: {} }; - } + private async cleanupOrphanedTempFiles(): Promise { + const dir = path.dirname(this.registryPath); + const baseName = path.basename(this.registryPath); + try { + const files = await fs.promises.readdir(dir); + const now = Date.now(); + + for (const file of files) { + if (file.startsWith(`${baseName}.`) && file.endsWith('.tmp')) { + const filePath = path.join(dir, file); + try { + const stats = await fs.promises.stat(filePath); + // Delete if older than 1 hour (3,600,000 ms) to avoid deleting actively written files + if (now - stats.mtimeMs > 3600000) { + await fs.promises.unlink(filePath); + } + } catch { + // Ignore stat or unlink errors (e.g., if the file was just successfully renamed by another process) + } + } + } + } catch (e: unknown) { + // Directory might not exist yet or be unreadable. + // We log at debug level rather than swallowing silently so leaks can be investigated. + debugLogger.debug( + 'Failed to read directory for orphaned temp file cleanup:', + e, + ); + } + } + + private async loadData(): Promise { try { const content = await fs.promises.readFile(this.registryPath, 'utf8'); // eslint-disable-next-line @typescript-eslint/no-unsafe-return return JSON.parse(content); - } catch (e) { - debugLogger.debug('Failed to load registry: ', e); - // If the registry is corrupted, we'll start fresh to avoid blocking the CLI - return { projects: {} }; + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { projects: {} }; // Normal first run + } + if (error instanceof SyntaxError) { + debugLogger.warn( + 'Failed to load registry (JSON corrupted), resetting to empty: ', + error, + ); + // Ownership markers on disk will allow self-healing when short IDs are requested. + return { projects: {} }; + } + + // If it's a real filesystem error (e.g. EACCES permission denied), DO NOT swallow it. + // Swallowing read errors and overwriting the file would permanently destroy user data. + debugLogger.error('Critical failure reading project registry:', error); + throw error; } } private normalizePath(projectPath: string): string { let resolved = path.resolve(projectPath); + try { + // Resolve symlinks and Windows shortnames to get the true physical path. + // We use the sync version because this is called synchronously across the system. + resolved = fs.realpathSync.native(resolved); + } catch { + // Ignore errors if the path doesn't exist yet on disk + } + if (os.platform() === 'win32') { resolved = resolved.toLowerCase(); } @@ -79,21 +141,54 @@ export class ProjectRegistry { private async save(data: RegistryData): Promise { const dir = path.dirname(this.registryPath); - if (!fs.existsSync(dir)) { - await fs.promises.mkdir(dir, { recursive: true }); - } + // Use a randomized tmp path to avoid ENOENT crashes when save() is called concurrently + const tmpPath = this.registryPath + '.' + randomUUID() + '.tmp'; + let savedSuccessfully = false; try { + // Unconditionally ensure the directory exists; recursive ignores EEXIST. + await fs.promises.mkdir(dir, { recursive: true }); + const content = JSON.stringify(data, null, 2); - // Use a randomized tmp path to avoid ENOENT crashes when save() is called concurrently - const tmpPath = this.registryPath + '.' + randomUUID() + '.tmp'; await fs.promises.writeFile(tmpPath, content, 'utf8'); - await fs.promises.rename(tmpPath, this.registryPath); + + // Exponential backoff for OS-level file locks (EBUSY/EPERM) during rename + const maxRetries = 5; + for (let attempt = 0; attempt < maxRetries; attempt++) { + try { + await fs.promises.rename(tmpPath, this.registryPath); + savedSuccessfully = true; + break; // Success, exit the retry loop + } catch (error: unknown) { + const code = isNodeError(error) ? error.code : ''; + const isRetryable = code === 'EBUSY' || code === 'EPERM'; + + if (!isRetryable || attempt === maxRetries - 1) { + throw error; // Throw immediately on fatal error or final attempt + } + + const delayMs = Math.pow(2, attempt) * 50; + debugLogger.debug( + `Rename failed with ${code}, retrying in ${delayMs}ms (attempt ${attempt + 1}/${maxRetries})...`, + ); + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + } } catch (error) { debugLogger.error( `Failed to save project registry to ${this.registryPath}:`, error, ); + throw error; + } finally { + // Clean up the temporary file if it was left behind (e.g. if writeFile or rename failed) + if (!savedSuccessfully) { + try { + await fs.promises.unlink(tmpPath); + } catch { + // Ignore errors during cleanup + } + } } } @@ -110,15 +205,24 @@ export class ProjectRegistry { // Ensure directory exists so we can create a lock file const dir = path.dirname(this.registryPath); - if (!fs.existsSync(dir)) { - await fs.promises.mkdir(dir, { recursive: true }); - } - // Ensure the registry file exists so proper-lockfile can lock it - if (!fs.existsSync(this.registryPath)) { - await this.save({ projects: {} }); + await fs.promises.mkdir(dir, { recursive: true }); + + // Atomic creation: Prevents TOCTOU races by asking the OS to create the file + // only if it doesn't exist. If another CLI instance beats us to it, it throws EEXIST. + try { + await fs.promises.writeFile( + this.registryPath, + JSON.stringify({ projects: {} }), + { flag: 'wx' }, + ); + } catch (error: unknown) { + if (isNodeError(error) && error.code !== 'EEXIST') { + throw error; + } + // EEXIST means the file is already there, which is exactly what we want. } - // Use proper-lockfile to prevent racy updates + // Now that the file is guaranteed to exist, we can safely lock it. const release = await lock(this.registryPath, { retries: { retries: Math.floor(LOCK_TIMEOUT_MS / LOCK_RETRY_DELAY_MS), @@ -129,6 +233,18 @@ export class ProjectRegistry { try { // Re-load data under lock to get the latest state const currentData = await this.loadData(); + + // JIT Migration: Normalize all existing keys in the loaded registry. + // This ensures backward compatibility with older registries that + // stored paths before realpath or lowercase normalization was introduced. + const migratedProjects: Record = {}; + for (const [oldPath, existingSlug] of Object.entries( + currentData.projects, + )) { + const migratedPath = this.normalizePath(oldPath); + migratedProjects[migratedPath] = existingSlug; + } + currentData.projects = migratedProjects; this.data = currentData; let shortId: string | undefined = currentData.projects[normalizedPath]; @@ -137,10 +253,18 @@ export class ProjectRegistry { if (shortId) { if (await this.verifySlugOwnership(shortId, normalizedPath)) { // HEAL: If it passed verification but markers are missing (e.g. new base dir or deleted marker), recreate them. - await this.ensureOwnershipMarkers(shortId, normalizedPath); - return shortId; + try { + await this.ensureOwnershipMarkers(shortId, normalizedPath); + return shortId; + } catch (e) { + if (!(e instanceof SlugCollisionError)) { + throw e; // Bubble up true filesystem failures (EACCES, ENOSPC) + } + // If it's a collision during healing, someone else stole the slug. + // We will just fall through to delete the mapping and generate a new one. + } } - // If verification fails, it means the registry is out of sync or someone else took it. + // If verification fails or healing collides, it means the registry is out of sync. // We'll remove the mapping and find/generate a new one. delete currentData.projects[normalizedPath]; } @@ -157,7 +281,13 @@ export class ProjectRegistry { await this.save(currentData); return shortId; } finally { - await release(); + try { + await release(); + } catch (e) { + // Prevent proper-lockfile errors (e.g. if the lock dir was externally deleted) + // from masking the original error thrown inside the try block. + debugLogger.error('Failed to release project registry lock:', e); + } } } @@ -171,20 +301,19 @@ export class ProjectRegistry { for (const baseDir of this.baseDirs) { const markerPath = path.join(baseDir, slug, PROJECT_ROOT_FILE); - if (fs.existsSync(markerPath)) { - try { - const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); - if (this.normalizePath(owner) !== this.normalizePath(projectPath)) { - return false; - } - } catch (e) { - debugLogger.debug( - `Failed to read ownership marker ${markerPath}:`, - e, - ); - // If we can't read it, assume it's not ours or corrupted. + try { + const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); + if (this.normalizePath(owner) !== this.normalizePath(projectPath)) { return false; } + } catch (e: unknown) { + if (isNodeError(e) && e.code === 'ENOENT') { + // Marker doesn't exist, this is fine, we just won't fail verification + continue; + } + debugLogger.debug(`Failed to read ownership marker ${markerPath}:`, e); + // If we can't read it for other reasons (perms, corrupted), assume not ours. + return false; } } return true; @@ -193,35 +322,46 @@ export class ProjectRegistry { private async findExistingSlugForPath( projectPath: string, ): Promise { - if (this.baseDirs.length === 0) { - return undefined; - } - const normalizedTarget = this.normalizePath(projectPath); // Scan all base dirs to see if any slug already belongs to this project for (const baseDir of this.baseDirs) { - if (!fs.existsSync(baseDir)) { + let candidates: string[]; + try { + candidates = await fs.promises.readdir(baseDir); + } catch (e: unknown) { + if (isNodeError(e) && e.code === 'ENOENT') { + continue; // Base dir doesn't exist yet + } + debugLogger.debug(`Failed to scan base dir ${baseDir}:`, e); continue; } - try { - const candidates = await fs.promises.readdir(baseDir); - for (const candidate of candidates) { - const markerPath = path.join(baseDir, candidate, PROJECT_ROOT_FILE); - if (fs.existsSync(markerPath)) { - const owner = ( - await fs.promises.readFile(markerPath, 'utf8') - ).trim(); - if (this.normalizePath(owner) === normalizedTarget) { - // Found it! Ensure all base dirs have the marker + for (const candidate of candidates) { + const markerPath = path.join(baseDir, candidate, PROJECT_ROOT_FILE); + try { + const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); + if (this.normalizePath(owner) === normalizedTarget) { + // Found it! Ensure all base dirs have the marker + try { await this.ensureOwnershipMarkers(candidate, normalizedTarget); return candidate; + } catch (e) { + if (e instanceof SlugCollisionError) { + // Split-brain scenario: This candidate is valid in this baseDir, + // but collides with a different project in another baseDir. + // Abandon this corrupted candidate and keep searching. + continue; + } + throw e; } } + } catch (e: unknown) { + if (isNodeError(e) && e.code === 'ENOENT') { + continue; // No marker, not a project dir + } + debugLogger.debug(`Failed to read marker ${markerPath}:`, e); } - } catch (e) { - debugLogger.debug(`Failed to scan base dir ${baseDir}:`, e); } } @@ -237,8 +377,9 @@ export class ProjectRegistry { let counter = 0; const existingIds = new Set(Object.values(existingMappings)); + const maxAttempts = 1000; - while (true) { + while (counter < maxAttempts) { const candidate = counter === 0 ? slug : `${slug}-${counter}`; counter++; @@ -247,41 +388,23 @@ export class ProjectRegistry { continue; } - // Check if taken on disk - let diskCollision = false; - for (const baseDir of this.baseDirs) { - const markerPath = path.join(baseDir, candidate, PROJECT_ROOT_FILE); - if (fs.existsSync(markerPath)) { - try { - const owner = ( - await fs.promises.readFile(markerPath, 'utf8') - ).trim(); - if (this.normalizePath(owner) !== this.normalizePath(projectPath)) { - diskCollision = true; - break; - } - } catch { - // If we can't read it, assume it's someone else's to be safe - diskCollision = true; - break; - } - } - } - - if (diskCollision) { - continue; - } - - // Try to claim it + // Try to claim it atomically on disk across all base dirs try { await this.ensureOwnershipMarkers(candidate, projectPath); return candidate; - } catch { - // Someone might have claimed it between our check and our write. - // Try next candidate. - continue; + } catch (e) { + if (e instanceof SlugCollisionError) { + // Try the next candidate. + continue; + } + // If it's a real filesystem error (e.g. ENOSPC, EACCES), do not swallow it. + throw e; } } + + throw new Error( + `Failed to claim a unique slug for ${projectPath} after ${maxAttempts} attempts. The filesystem may be corrupted.`, + ); } private async ensureOwnershipMarkers( @@ -291,23 +414,44 @@ export class ProjectRegistry { const normalizedProject = this.normalizePath(projectPath); for (const baseDir of this.baseDirs) { const slugDir = path.join(baseDir, slug); - if (!fs.existsSync(slugDir)) { - await fs.promises.mkdir(slugDir, { recursive: true }); - } + await fs.promises.mkdir(slugDir, { recursive: true }); + const markerPath = path.join(slugDir, PROJECT_ROOT_FILE); - if (fs.existsSync(markerPath)) { - const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); - if (this.normalizePath(owner) === normalizedProject) { - continue; + + while (true) { + try { + // Use flag: 'wx' to ensure atomic creation. Fails with EEXIST if it already exists. + await fs.promises.writeFile(markerPath, normalizedProject, { + encoding: 'utf8', + flag: 'wx', + }); + break; // Created successfully + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'EEXIST') { + // It already exists. Let's see who owns it to resolve the race condition. + try { + const owner = ( + await fs.promises.readFile(markerPath, 'utf8') + ).trim(); + if (this.normalizePath(owner) === normalizedProject) { + break; // We won the race (or a previous execution of ours did) + } + // Collision! Someone else beat us to it. + throw new SlugCollisionError( + `Slug ${slug} is already owned by ${owner}`, + ); + } catch (readError: unknown) { + if (isNodeError(readError) && readError.code === 'ENOENT') { + // The file vanished between our EEXIST and readFile. + // Loop around and try creating it again. + continue; + } + throw readError; + } + } + throw error; } - // Collision! - throw new Error(`Slug ${slug} is already owned by ${owner}`); } - // Use flag: 'wx' to ensure atomic creation - await fs.promises.writeFile(markerPath, normalizedProject, { - encoding: 'utf8', - flag: 'wx', - }); } }