From 16510dc7656b5363688e6fee3ed4b6c3f26f6479 Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Tue, 14 Apr 2026 05:32:07 +0000 Subject: [PATCH 1/3] fix(core): fix ShellExecutionConfig spread and add ProjectRegistry save backoff --- packages/core/src/config/config.test.ts | 21 ++ packages/core/src/config/config.ts | 27 +- .../core/src/config/projectRegistry.test.ts | 159 ++++++++ packages/core/src/config/projectRegistry.ts | 340 +++++++++++++----- 4 files changed, 437 insertions(+), 110 deletions(-) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index ab000b2691f..895d9ca963a 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 9dbf0f8115d..918b1141298 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 a441de8b3e2..9d59c1528d7 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 c58fb55ce84..b8e66e7245a 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', - }); } } From d44ed7f53e7338515dd1829dc274be23483fc6df Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Wed, 15 Apr 2026 17:09:45 +0000 Subject: [PATCH 2/3] chore(core): reduce ProjectRegistry PR scope by removing JIT migration and background cleanup --- .../core/src/config/projectRegistry.test.ts | 85 ------ packages/core/src/config/projectRegistry.ts | 243 ++++++------------ 2 files changed, 75 insertions(+), 253 deletions(-) diff --git a/packages/core/src/config/projectRegistry.test.ts b/packages/core/src/config/projectRegistry.test.ts index 9d59c1528d7..e7e532bb2bc 100644 --- a/packages/core/src/config/projectRegistry.test.ts +++ b/packages/core/src/config/projectRegistry.test.ts @@ -354,70 +354,6 @@ describe('ProjectRegistry', () => { 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( @@ -438,25 +374,4 @@ describe('ProjectRegistry', () => { 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 b8e66e7245a..e04605336bc 100644 --- a/packages/core/src/config/projectRegistry.ts +++ b/packages/core/src/config/projectRegistry.ts @@ -20,13 +20,6 @@ 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. @@ -56,48 +49,11 @@ 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 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'); @@ -125,14 +81,6 @@ export class ProjectRegistry { 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(); } @@ -141,6 +89,9 @@ 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; @@ -205,24 +156,15 @@ export class ProjectRegistry { // Ensure directory exists so we can create a lock file const dir = path.dirname(this.registryPath); - 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. + 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: {} }); } - // Now that the file is guaranteed to exist, we can safely lock it. + // Use proper-lockfile to prevent racy updates const release = await lock(this.registryPath, { retries: { retries: Math.floor(LOCK_TIMEOUT_MS / LOCK_RETRY_DELAY_MS), @@ -233,18 +175,6 @@ 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]; @@ -253,18 +183,10 @@ 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. - 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. - } + await this.ensureOwnershipMarkers(shortId, normalizedPath); + return shortId; } - // If verification fails or healing collides, it means the registry is out of sync. + // If verification fails, it means the registry is out of sync or someone else took it. // We'll remove the mapping and find/generate a new one. delete currentData.projects[normalizedPath]; } @@ -322,46 +244,35 @@ 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) { - 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); + if (!fs.existsSync(baseDir)) { continue; } - 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 { + 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 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); } } @@ -377,9 +288,8 @@ export class ProjectRegistry { let counter = 0; const existingIds = new Set(Object.values(existingMappings)); - const maxAttempts = 1000; - while (counter < maxAttempts) { + while (true) { const candidate = counter === 0 ? slug : `${slug}-${counter}`; counter++; @@ -388,23 +298,41 @@ export class ProjectRegistry { continue; } - // Try to claim it atomically on disk across all base dirs + // 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 { await this.ensureOwnershipMarkers(candidate, projectPath); return candidate; - } 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; + } catch { + // Someone might have claimed it between our check and our write. + // Try next candidate. + continue; } } - - throw new Error( - `Failed to claim a unique slug for ${projectPath} after ${maxAttempts} attempts. The filesystem may be corrupted.`, - ); } private async ensureOwnershipMarkers( @@ -414,44 +342,23 @@ export class ProjectRegistry { const normalizedProject = this.normalizePath(projectPath); for (const baseDir of this.baseDirs) { const slugDir = path.join(baseDir, slug); - await fs.promises.mkdir(slugDir, { recursive: true }); - + if (!fs.existsSync(slugDir)) { + await fs.promises.mkdir(slugDir, { recursive: true }); + } const markerPath = path.join(slugDir, PROJECT_ROOT_FILE); - - 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; + if (fs.existsSync(markerPath)) { + const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); + if (this.normalizePath(owner) === normalizedProject) { + continue; } + // 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', + }); } } From 4587aa76842c1ad6f530e160fc75fa867655d192 Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Fri, 17 Apr 2026 08:20:58 +0000 Subject: [PATCH 3/3] fix(core): prevent infinite loop in ProjectRegistry slug claiming --- packages/core/src/config/projectRegistry.ts | 58 +++++++++++++++++---- 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/core/src/config/projectRegistry.ts b/packages/core/src/config/projectRegistry.ts index e04605336bc..1aec0b7ad28 100644 --- a/packages/core/src/config/projectRegistry.ts +++ b/packages/core/src/config/projectRegistry.ts @@ -159,9 +159,18 @@ export class ProjectRegistry { if (!fs.existsSync(dir)) { await fs.promises.mkdir(dir, { recursive: true }); } - // Ensure the registry file exists so proper-lockfile can lock it + // Ensure the registry file exists so proper-lockfile can lock it. + // If it doesn't exist, we try to create it. If someone else creates it + // between our check and our write, we just continue. if (!fs.existsSync(this.registryPath)) { - await this.save({ projects: {} }); + try { + await this.save({ projects: {} }); + } catch (e: unknown) { + if (!fs.existsSync(this.registryPath)) { + throw e; // File still doesn't exist and save failed, this is a real error. + } + // Someone else created it while we were trying to save. Continue to locking. + } } // Use proper-lockfile to prevent racy updates @@ -327,10 +336,22 @@ export class ProjectRegistry { 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 (error: unknown) { + // Only retry if it was a collision (someone else took the slug) + // or a race condition during marker creation. + const code = isNodeError(error) ? error.code : ''; + const isCollision = + code === 'EEXIST' || + (error instanceof Error && + error.message.includes('already owned by')); + + if (isCollision) { + debugLogger.debug(`Slug collision for ${candidate}, trying next...`); + continue; + } + + // Fatal error (Permission denied, Disk full, etc.) + throw error; } } } @@ -352,13 +373,28 @@ export class ProjectRegistry { continue; } // Collision! - throw new Error(`Slug ${slug} is already owned by ${owner}`); + const error = Object.assign( + new Error(`Slug ${slug} is already owned by ${owner}`), + { code: 'EEXIST' }, + ); + throw error; } // Use flag: 'wx' to ensure atomic creation - await fs.promises.writeFile(markerPath, normalizedProject, { - encoding: 'utf8', - flag: 'wx', - }); + try { + await fs.promises.writeFile(markerPath, normalizedProject, { + encoding: 'utf8', + flag: 'wx', + }); + } catch (e: unknown) { + if (isNodeError(e) && e.code === 'EEXIST') { + // Re-verify ownership in case we just lost a race + const owner = (await fs.promises.readFile(markerPath, 'utf8')).trim(); + if (this.normalizePath(owner) === normalizedProject) { + continue; + } + } + throw e; + } } }