From 8379099e85537a5e510ec6505a518f26121388d2 Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Fri, 17 Apr 2026 12:49:44 -0400 Subject: [PATCH] fix(core): fix ShellExecutionConfig spread and add ProjectRegistry save backoff (#25382) --- packages/core/src/config/config.test.ts | 21 +++ packages/core/src/config/config.ts | 27 +-- .../core/src/config/projectRegistry.test.ts | 74 +++++++++ packages/core/src/config/projectRegistry.ts | 157 ++++++++++++++---- 4 files changed, 232 insertions(+), 47 deletions(-) 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..e7e532bb2b 100644 --- a/packages/core/src/config/projectRegistry.test.ts +++ b/packages/core/src/config/projectRegistry.test.ts @@ -300,4 +300,78 @@ 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('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(); + }); }); diff --git a/packages/core/src/config/projectRegistry.ts b/packages/core/src/config/projectRegistry.ts index c58fb55ce8..1aec0b7ad2 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; @@ -54,18 +55,27 @@ export class ProjectRegistry { } private async loadData(): Promise { - if (!fs.existsSync(this.registryPath)) { - return { projects: {} }; - } - 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; } } @@ -82,18 +92,54 @@ export class ProjectRegistry { 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 + } + } } } @@ -113,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 @@ -157,7 +212,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 +232,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; @@ -276,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; } } } @@ -301,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; + } } }