fix(core): fix ShellExecutionConfig spread and add ProjectRegistry save backoff

This commit is contained in:
Mahima Shanware
2026-04-14 05:32:07 +00:00
parent 22fb83320e
commit 6128ab982f
4 changed files with 438 additions and 111 deletions
+21
View File
@@ -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(() => {
+15 -12
View File
@@ -3396,20 +3396,23 @@ export class Config implements McpContext, AgentLoopContext {
return this.shellExecutionConfig;
}
setShellExecutionConfig(config: ShellExecutionConfig): void {
setShellExecutionConfig(config: Partial<ShellExecutionConfig>): void {
const definedConfig: Partial<ShellExecutionConfig> = {};
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 {
@@ -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);
});
});
+243 -99
View File
@@ -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<string, string>;
@@ -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<RegistryData> {
if (!fs.existsSync(this.registryPath)) {
return { projects: {} };
}
private async cleanupOrphanedTempFiles(): Promise<void> {
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<RegistryData> {
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<void> {
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<string, string> = {};
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<string | undefined> {
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',
});
}
}