diff --git a/src/config/__tests__/config-loader-facade.test.ts b/src/config/__tests__/config-loader-facade.test.ts index f780bcc8..aff186a5 100644 --- a/src/config/__tests__/config-loader-facade.test.ts +++ b/src/config/__tests__/config-loader-facade.test.ts @@ -116,14 +116,15 @@ describe('config-loader-facade', () => { }); describe('memoization', () => { - it('getCachedConfig returns same object on repeated calls', async () => { + it('getCachedConfig returns equivalent config on repeated calls', async () => { const facade = await importFacade(); const first = facade.getCachedConfig(); const second = facade.getCachedConfig(); - // Same reference (cached, not re-read) - expect(first).toBe(second); + // Deep copies — different references but same content (no re-read from disk) + expect(first).not.toBe(second); + expect(first.version).toBe(second.version); }); it('invalidateConfigCache forces re-read on next getCachedConfig', async () => { @@ -139,16 +140,21 @@ describe('config-loader-facade', () => { expect(first.version).toBe(second.version); }); - it('saveConfig updates cache and does not invalidate', async () => { + it('saveConfig persists to disk and updates cache', async () => { const facade = await importFacade(); const config = facade.getCachedConfig(); config.default = 'test-profile'; facade.saveConfig(config); + // Verify disk content reflects the save + const configPath = path.join(tempHome, '.ccs', 'config.yaml'); + const diskContent = fs.readFileSync(configPath, 'utf8'); + const diskConfig = yaml.load(diskContent) as Record; + expect(diskConfig.default).toBe('test-profile'); + const cached = facade.getCachedConfig(); - // Cache should hold the just-saved config (no re-read) - expect(cached).toBe(config); + // Cache holds a copy of the saved config expect(cached.default).toBe('test-profile'); }); diff --git a/src/config/config-loader-facade.ts b/src/config/config-loader-facade.ts index 40b526fc..b3fee310 100644 --- a/src/config/config-loader-facade.ts +++ b/src/config/config-loader-facade.ts @@ -62,7 +62,11 @@ let _configCache: UnifiedConfig | null = null; /** * Get the unified config with in-memory caching. - * First call reads from disk; subsequent calls return the cached object. + * First call reads from disk; subsequent calls return a deep copy. + * Returns a copy to prevent callers from silently mutating the cache. + * + * NOTE: `loadOrCreateUnifiedConfig` (re-exported above) is NOT cached. + * Use `getCachedConfig()` for cached reads, or the direct import for uncached. * * Call invalidateConfigCache() or use mutateConfig()/updateConfig() * to force a re-read from disk. @@ -71,7 +75,7 @@ export function getCachedConfig(): UnifiedConfig { if (!_configCache) { _configCache = _loadOrCreateUnifiedConfig(); } - return _configCache; + return structuredClone(_configCache); } /** @@ -83,17 +87,17 @@ export function invalidateConfigCache(): void { } /** - * Save config to disk and update the cache to the given object. - * Does NOT invalidate — the provided config IS the new cache value. + * Save config to disk and update the cache. + * Stores a deep copy to break the reference alias. */ export function saveConfig(config: UnifiedConfig): void { _saveUnifiedConfig(config); - _configCache = config; + _configCache = structuredClone(config); } /** * Atomically mutate config (read-modify-write with lock) and invalidate cache. - * After mutation, the next getCachedConfig() call will re-read from disk. + * Invalidated AFTER _mutateUnifiedConfig returns — if that throws, cache stays valid. */ export function mutateConfig(mutator: (config: UnifiedConfig) => void): UnifiedConfig { const result = _mutateUnifiedConfig(mutator); diff --git a/src/errors/__tests__/error-types.test.ts b/src/errors/__tests__/error-types.test.ts index 0a28b240..b17298d6 100644 --- a/src/errors/__tests__/error-types.test.ts +++ b/src/errors/__tests__/error-types.test.ts @@ -29,10 +29,10 @@ describe('RetryableError', () => { expect(err.message).toBe('something went wrong'); }); - it('accepts an optional cause', () => { - const cause = new Error('original'); - const err = new RetryableError('wrapped', cause); - expect(err.cause).toBe(cause); + it('accepts an optional originalError', () => { + const original = new Error('original'); + const err = new RetryableError('wrapped', original); + expect(err.originalError).toBe(original); }); it('accepts an optional retryAfter (ms)', () => { diff --git a/src/errors/error-types.ts b/src/errors/error-types.ts index c0b8a9b2..2c288426 100644 --- a/src/errors/error-types.ts +++ b/src/errors/error-types.ts @@ -176,7 +176,7 @@ export class ValidationError extends CCSError { export class RetryableError extends CCSError { constructor( message: string, - public readonly cause?: Error, + public readonly originalError?: Error, public readonly retryAfter?: number // ms until next attempt ) { super(message, ExitCode.GENERAL_ERROR, true); diff --git a/src/utils/__tests__/retry-strategy.test.ts b/src/utils/__tests__/retry-strategy.test.ts index e22693ef..372babf5 100644 --- a/src/utils/__tests__/retry-strategy.test.ts +++ b/src/utils/__tests__/retry-strategy.test.ts @@ -95,11 +95,11 @@ describe('withRetry', () => { maxDelayMs: 200, }); - // Verify setTimeout was called with delay <= maxDelayMs (200ms) + jitter buffer - // Jitter adds 0-20% of delay, so max possible is 240ms + // Verify setTimeout was called with delay <= maxDelayMs (200ms) + // Jitter is capped, so delay must never exceed 200ms for (const call of sleepSpy.mock.calls) { const delay = call[1] as number; - expect(delay).toBeLessThanOrEqual(250); // allow small jitter overhead + expect(delay).toBeLessThanOrEqual(200); } sleepSpy.mockRestore(); }); @@ -158,4 +158,30 @@ describe('withRetry', () => { await expect(withRetry(fn, { maxRetries: 0, baseDelayMs: 1 })).rejects.toThrow('fail'); expect(fn).toHaveBeenCalledTimes(1); }); + + it('throws on negative maxRetries', async () => { + const fn = mock(() => Promise.resolve('ok')); + await expect(withRetry(fn, { maxRetries: -1, baseDelayMs: 1 })).rejects.toThrow( + 'maxRetries must be >= 0' + ); + expect(fn).not.toHaveBeenCalled(); + }); + + it('respects retryAfter from RetryableError', async () => { + const sleepSpy = spyOn(globalThis, 'setTimeout'); + let attempt = 0; + const fn = mock(() => { + attempt++; + if (attempt < 2) { + return Promise.reject(new RetryableError('rate limited', undefined, 500)); + } + return Promise.resolve('ok'); + }); + + await withRetry(fn, { maxRetries: 3, baseDelayMs: 10, maxDelayMs: 1000 }); + // retryAfter=500 should override the computed backoff (~10ms) since 500 > 10 + const delay = sleepSpy.mock.calls[0][1] as number; + expect(delay).toBeGreaterThanOrEqual(500); + sleepSpy.mockRestore(); + }); }); diff --git a/src/utils/retry-strategy.ts b/src/utils/retry-strategy.ts index 988f9216..6e778862 100644 --- a/src/utils/retry-strategy.ts +++ b/src/utils/retry-strategy.ts @@ -46,16 +46,22 @@ function defaultRetryableCheck(error: unknown): boolean { /** * Compute backoff delay: base * multiplier^attempt + jitter, capped at maxDelay. + * Jitter is applied before the final cap to ensure the result never exceeds maxDelayMs. */ function computeDelay( attempt: number, baseDelayMs: number, maxDelayMs: number, - multiplier: number + multiplier: number, + retryAfter?: number ): number { const exponentialDelay = Math.min(baseDelayMs * Math.pow(multiplier, attempt), maxDelayMs); const jitter = exponentialDelay * JITTER_RATIO * Math.random(); - return exponentialDelay + jitter; + const delay = Math.min(exponentialDelay + jitter, maxDelayMs); + if (retryAfter !== undefined && retryAfter > 0) { + return Math.max(delay, retryAfter); + } + return delay; } /** @@ -86,6 +92,10 @@ export async function withRetry(fn: () => Promise, options: RetryOptions): onRetry, } = options; + if (maxRetries < 0) { + throw new Error('withRetry: maxRetries must be >= 0'); + } + const isRetryable = retryableCheck ?? defaultRetryableCheck; let lastError: unknown; @@ -108,7 +118,8 @@ export async function withRetry(fn: () => Promise, options: RetryOptions): const err = error instanceof Error ? error : new Error(String(error)); onRetry?.(err, attempt + 1); - const delay = computeDelay(attempt, baseDelayMs, maxDelayMs, backoffMultiplier); + const retryAfter = error instanceof RetryableError ? error.retryAfter : undefined; + const delay = computeDelay(attempt, baseDelayMs, maxDelayMs, backoffMultiplier, retryAfter); await sleep(delay); } }