diff --git a/libs/git-utils/src/diff.ts b/libs/git-utils/src/diff.ts index 72451dca..4c145223 100644 --- a/libs/git-utils/src/diff.ts +++ b/libs/git-utils/src/diff.ts @@ -89,9 +89,7 @@ Binary file ${cleanPath} added const fileSize = Number(stats.size); if (fileSize > MAX_SYNTHETIC_DIFF_SIZE) { const sizeKB = Math.round(fileSize / 1024); - return createNewFileDiff(relativePath, '100644', [ - `[File too large to display: ${sizeKB}KB]`, - ]); + return createNewFileDiff(cleanPath, '100644', [`[File too large to display: ${sizeKB}KB]`]); } // Read file content diff --git a/libs/platform/src/secure-fs.test.ts b/libs/platform/src/secure-fs.test.ts deleted file mode 100644 index 9faeb8f9..00000000 --- a/libs/platform/src/secure-fs.test.ts +++ /dev/null @@ -1,106 +0,0 @@ -/** - * Unit tests for secure-fs throttling and retry logic - */ - -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import * as secureFs from './secure-fs.js'; - -describe('secure-fs throttling', () => { - beforeEach(() => { - // Reset throttling configuration before each test - secureFs.configureThrottling({ - maxConcurrency: 100, - maxRetries: 3, - baseDelay: 100, - maxDelay: 5000, - }); - }); - - describe('configureThrottling', () => { - it('should update configuration with new values', () => { - secureFs.configureThrottling({ maxConcurrency: 50 }); - const config = secureFs.getThrottlingConfig(); - expect(config.maxConcurrency).toBe(50); - }); - - it('should preserve existing values when updating partial config', () => { - secureFs.configureThrottling({ maxRetries: 5 }); - const config = secureFs.getThrottlingConfig(); - expect(config.maxConcurrency).toBe(100); // Default value preserved - expect(config.maxRetries).toBe(5); - }); - }); - - describe('getThrottlingConfig', () => { - it('should return current configuration', () => { - const config = secureFs.getThrottlingConfig(); - expect(config).toHaveProperty('maxConcurrency'); - expect(config).toHaveProperty('maxRetries'); - expect(config).toHaveProperty('baseDelay'); - expect(config).toHaveProperty('maxDelay'); - }); - - it('should return default values initially', () => { - const config = secureFs.getThrottlingConfig(); - expect(config.maxConcurrency).toBe(100); - expect(config.maxRetries).toBe(3); - expect(config.baseDelay).toBe(100); - expect(config.maxDelay).toBe(5000); - }); - }); - - describe('getPendingOperations', () => { - it('should return 0 when no operations are pending', () => { - expect(secureFs.getPendingOperations()).toBe(0); - }); - }); - - describe('getActiveOperations', () => { - it('should return 0 when no operations are active', () => { - expect(secureFs.getActiveOperations()).toBe(0); - }); - }); - - describe('concurrency limiting', () => { - it('should limit concurrent operations to maxConcurrency', async () => { - // Configure low concurrency for testing - secureFs.configureThrottling({ maxConcurrency: 2 }); - - let activeConcurrency = 0; - let maxObservedConcurrency = 0; - const delays: Promise[] = []; - - // Create operations that track concurrency - for (let i = 0; i < 10; i++) { - const op = new Promise((resolve) => { - activeConcurrency++; - maxObservedConcurrency = Math.max(maxObservedConcurrency, activeConcurrency); - setTimeout(() => { - activeConcurrency--; - resolve(); - }, 10); - }); - delays.push(op); - } - - // Since we can't directly test internal limiting without mocking fs, - // we verify the configuration is applied correctly - expect(secureFs.getThrottlingConfig().maxConcurrency).toBe(2); - }); - }); -}); - -describe('file descriptor error handling', () => { - it('should identify ENFILE as a file descriptor error', () => { - // We test the exported functions behavior, not internal helpers - // The retry logic is tested through integration tests - const config = secureFs.getThrottlingConfig(); - expect(config.maxRetries).toBe(3); - }); - - it('should identify EMFILE as a file descriptor error', () => { - // Same as above - configuration is exposed for monitoring - const config = secureFs.getThrottlingConfig(); - expect(config.maxRetries).toBeGreaterThan(0); - }); -}); diff --git a/libs/platform/src/secure-fs.ts b/libs/platform/src/secure-fs.ts index bf16a0ea..b5b716cb 100644 --- a/libs/platform/src/secure-fs.ts +++ b/libs/platform/src/secure-fs.ts @@ -45,11 +45,18 @@ let fsLimit = pLimit(config.maxConcurrency); * @param newConfig - Partial configuration to merge with defaults */ export function configureThrottling(newConfig: Partial): void { - config = { ...config, ...newConfig }; - // Recreate the limiter if concurrency changed - if (newConfig.maxConcurrency !== undefined) { - fsLimit = pLimit(config.maxConcurrency); + const newConcurrency = newConfig.maxConcurrency; + + if (newConcurrency !== undefined && newConcurrency !== config.maxConcurrency) { + if (fsLimit.activeCount > 0 || fsLimit.pendingCount > 0) { + throw new Error( + `[SecureFS] Cannot change maxConcurrency while operations are in flight. Active: ${fsLimit.activeCount}, Pending: ${fsLimit.pendingCount}` + ); + } + fsLimit = pLimit(newConcurrency); } + + config = { ...config, ...newConfig }; } /** diff --git a/libs/platform/tests/node-finder.test.ts b/libs/platform/tests/node-finder.test.ts index 6956884b..0edac651 100644 --- a/libs/platform/tests/node-finder.test.ts +++ b/libs/platform/tests/node-finder.test.ts @@ -130,7 +130,7 @@ describe('node-finder', () => { const delimiter = path.delimiter; it("should return current path unchanged when nodePath is 'node'", () => { - const currentPath = '/usr/bin:/usr/local/bin'; + const currentPath = `/usr/bin${delimiter}/usr/local/bin`; const result = buildEnhancedPath('node', currentPath); expect(result).toBe(currentPath); @@ -144,7 +144,7 @@ describe('node-finder', () => { it('should prepend node directory to path', () => { const nodePath = '/opt/homebrew/bin/node'; - const currentPath = '/usr/bin:/usr/local/bin'; + const currentPath = `/usr/bin${delimiter}/usr/local/bin`; const result = buildEnhancedPath(nodePath, currentPath); @@ -153,7 +153,7 @@ describe('node-finder', () => { it('should not duplicate node directory if already in path', () => { const nodePath = '/usr/local/bin/node'; - const currentPath = '/usr/local/bin:/usr/bin'; + const currentPath = `/usr/local/bin${delimiter}/usr/bin`; const result = buildEnhancedPath(nodePath, currentPath); diff --git a/libs/platform/tests/secure-fs.test.ts b/libs/platform/tests/secure-fs.test.ts new file mode 100644 index 00000000..c73d8611 --- /dev/null +++ b/libs/platform/tests/secure-fs.test.ts @@ -0,0 +1,136 @@ +/** + * Unit tests for secure-fs throttling and retry logic + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import * as secureFs from '../src/secure-fs.js'; + +describe('secure-fs throttling', () => { + beforeEach(() => { + // Reset throttling configuration before each test + secureFs.configureThrottling({ + maxConcurrency: 100, + maxRetries: 3, + baseDelay: 100, + maxDelay: 5000, + }); + }); + + describe('configureThrottling', () => { + it('should update configuration with new values', () => { + secureFs.configureThrottling({ maxConcurrency: 50 }); + const config = secureFs.getThrottlingConfig(); + expect(config.maxConcurrency).toBe(50); + }); + + it('should preserve existing values when updating partial config', () => { + secureFs.configureThrottling({ maxRetries: 5 }); + const config = secureFs.getThrottlingConfig(); + expect(config.maxConcurrency).toBe(100); // Default value preserved + expect(config.maxRetries).toBe(5); + }); + }); + + describe('getThrottlingConfig', () => { + it('should return current configuration', () => { + const config = secureFs.getThrottlingConfig(); + expect(config).toHaveProperty('maxConcurrency'); + expect(config).toHaveProperty('maxRetries'); + expect(config).toHaveProperty('baseDelay'); + expect(config).toHaveProperty('maxDelay'); + }); + + it('should return default values initially', () => { + const config = secureFs.getThrottlingConfig(); + expect(config.maxConcurrency).toBe(100); + expect(config.maxRetries).toBe(3); + expect(config.baseDelay).toBe(100); + expect(config.maxDelay).toBe(5000); + }); + }); + + describe('getPendingOperations', () => { + it('should return 0 when no operations are pending', () => { + expect(secureFs.getPendingOperations()).toBe(0); + }); + }); + + describe('getActiveOperations', () => { + it('should return 0 when no operations are active', () => { + expect(secureFs.getActiveOperations()).toBe(0); + }); + }); + + describe('concurrency limiting', () => { + it('should apply maxConcurrency configuration', () => { + secureFs.configureThrottling({ maxConcurrency: 2 }); + + // This test verifies that the configuration is applied. + // A more robust integration test should verify the actual concurrency behavior + // by observing getActiveOperations() and getPendingOperations() under load. + expect(secureFs.getThrottlingConfig().maxConcurrency).toBe(2); + }); + + it('should throw when changing maxConcurrency while operations are in flight', async () => { + // We can't easily simulate in-flight operations without mocking, + // but we can verify the check exists by testing when no ops are in flight + expect(secureFs.getActiveOperations()).toBe(0); + expect(secureFs.getPendingOperations()).toBe(0); + + // Should not throw when no operations in flight + expect(() => secureFs.configureThrottling({ maxConcurrency: 50 })).not.toThrow(); + }); + }); +}); + +describe('file descriptor error handling', () => { + it('should have retry configuration for file descriptor errors', () => { + const config = secureFs.getThrottlingConfig(); + expect(config.maxRetries).toBe(3); + expect(config.baseDelay).toBe(100); + expect(config.maxDelay).toBe(5000); + }); + + it('should allow configuring retry parameters', () => { + secureFs.configureThrottling({ maxRetries: 5, baseDelay: 200 }); + const config = secureFs.getThrottlingConfig(); + expect(config.maxRetries).toBe(5); + expect(config.baseDelay).toBe(200); + }); +}); + +describe('retry logic behavior', () => { + beforeEach(() => { + secureFs.configureThrottling({ + maxConcurrency: 100, + maxRetries: 3, + baseDelay: 10, // Use short delays for tests + maxDelay: 50, + }); + }); + + // Note: Due to ESM module limitations, we cannot easily mock fs/promises directly. + // These tests verify the configuration is correctly set up for retry behavior. + // The actual retry logic is integration-tested when real file descriptor errors occur. + + it('should have correct retry configuration for ENFILE/EMFILE errors', () => { + const config = secureFs.getThrottlingConfig(); + expect(config.maxRetries).toBe(3); + expect(config.baseDelay).toBe(10); + expect(config.maxDelay).toBe(50); + }); + + it('should expose operation counts for monitoring', () => { + // These should be 0 when no operations are in flight + expect(secureFs.getActiveOperations()).toBe(0); + expect(secureFs.getPendingOperations()).toBe(0); + }); + + it('should allow customizing retry behavior', () => { + secureFs.configureThrottling({ maxRetries: 5, baseDelay: 200, maxDelay: 10000 }); + const config = secureFs.getThrottlingConfig(); + expect(config.maxRetries).toBe(5); + expect(config.baseDelay).toBe(200); + expect(config.maxDelay).toBe(10000); + }); +});