diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index 65b3539794..c6cb11510a 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -64,6 +64,7 @@ import { import { maybeRequestConsentOrFail } from './extensions/consent.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; import { ExtensionStorage } from './extensions/storage.js'; +import { removeDirectoryWithRetry } from '../utils/retry.js'; import { EXTENSIONS_CONFIG_FILENAME, INSTALL_METADATA_FILENAME, @@ -495,7 +496,7 @@ Would you like to attempt to install via "git clone" instead?`, } } finally { if (tempDir) { - await fs.promises.rm(tempDir, { recursive: true, force: true }); + await removeDirectoryWithRetry(tempDir); } } return extension; @@ -564,10 +565,7 @@ Would you like to attempt to install via "git clone" instead?`, : path.basename(extension.path), ); - await fs.promises.rm(storage.getExtensionDir(), { - recursive: true, - force: true, - }); + await removeDirectoryWithRetry(storage.getExtensionDir()); // The rest of the cleanup below here is only for true uninstalls, not // uninstalls related to updates. diff --git a/packages/cli/src/config/extensions/update.ts b/packages/cli/src/config/extensions/update.ts index c4b7113530..cf3d161e94 100644 --- a/packages/cli/src/config/extensions/update.ts +++ b/packages/cli/src/config/extensions/update.ts @@ -20,6 +20,7 @@ import { import * as fs from 'node:fs'; import { copyExtension, type ExtensionManager } from '../extension-manager.js'; import { ExtensionStorage } from './storage.js'; +import { removeDirectoryWithRetry } from '../../utils/retry.js'; export interface ExtensionUpdateInfo { name: string; @@ -145,7 +146,7 @@ export async function updateExtension( await copyExtension(tempDir, extension.path); throw e; } finally { - await fs.promises.rm(tempDir, { recursive: true, force: true }); + await removeDirectoryWithRetry(tempDir); } } diff --git a/packages/cli/src/utils/retry.test.ts b/packages/cli/src/utils/retry.test.ts new file mode 100644 index 0000000000..397859c8a6 --- /dev/null +++ b/packages/cli/src/utils/retry.test.ts @@ -0,0 +1,87 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import { removeDirectoryWithRetry } from './retry.js'; + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs'); + return { + ...actual, + promises: { + ...actual.promises, + rm: vi.fn(), + }, + }; +}); + +describe('removeDirectoryWithRetry', () => { + const mockedRm = vi.mocked(fs.promises.rm); + + beforeEach(() => { + mockedRm.mockClear(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should succeed on first attempt', async () => { + mockedRm.mockResolvedValue(undefined); + + await removeDirectoryWithRetry('/test/path'); + + expect(mockedRm).toHaveBeenCalledTimes(1); + expect(mockedRm).toHaveBeenCalledWith('/test/path', { + recursive: true, + force: true, + }); + }); + + it('should retry on EBUSY error', async () => { + const ebusyError = Object.assign(new Error('EBUSY'), { code: 'EBUSY' }); + mockedRm + .mockRejectedValueOnce(ebusyError) + .mockRejectedValueOnce(ebusyError) + .mockResolvedValue(undefined); + + await removeDirectoryWithRetry('/test/path'); + + expect(mockedRm).toHaveBeenCalledTimes(3); + }); + + it('should retry on ENOTEMPTY error', async () => { + const notEmptyError = Object.assign(new Error('ENOTEMPTY'), { + code: 'ENOTEMPTY', + }); + mockedRm.mockRejectedValueOnce(notEmptyError).mockResolvedValue(undefined); + + await removeDirectoryWithRetry('/test/path'); + + expect(mockedRm).toHaveBeenCalledTimes(2); + }); + + it('should throw after max retries', async () => { + const ebusyError = Object.assign(new Error('EBUSY'), { code: 'EBUSY' }); + mockedRm.mockRejectedValue(ebusyError); + + await expect(removeDirectoryWithRetry('/test/path')).rejects.toThrow( + 'EBUSY', + ); + expect(mockedRm).toHaveBeenCalledTimes(5); + }); + + it('should throw immediately on non-retryable errors', async () => { + const otherError = new Error('Some other error'); + mockedRm.mockRejectedValue(otherError); + + await expect(removeDirectoryWithRetry('/test/path')).rejects.toThrow( + 'Some other error', + ); + expect(mockedRm).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/utils/retry.ts b/packages/cli/src/utils/retry.ts new file mode 100644 index 0000000000..7fedffef61 --- /dev/null +++ b/packages/cli/src/utils/retry.ts @@ -0,0 +1,68 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import { debugLogger } from '@google/gemini-cli-core'; + +/** + * Retries a file system operation with exponential backoff. + * Useful for Windows where file locks may cause EBUSY errors. + */ +async function retryWithBackoff( + operation: () => Promise, + maxRetries: number = 5, + initialDelayMs: number = 100, +): Promise { + let lastError: Error | undefined; + + if (maxRetries <= 0) { + throw new Error('maxRetries must be a positive number.'); + } + + for (let attempt = 0; attempt < maxRetries; attempt++) { + try { + return await operation(); + } catch (e) { + if (!(e instanceof Error)) { + throw e; + } + lastError = e; + + const code = (e as NodeJS.ErrnoException).code; + if (code !== 'EBUSY' && code !== 'ENOTEMPTY' && code !== 'EPERM') { + throw e; + } + + if (attempt < maxRetries - 1) { + const delay = initialDelayMs * Math.pow(2, attempt); + debugLogger.debug( + `Retry attempt ${attempt + 1}/${maxRetries} after ${delay}ms due to: ${lastError.message}`, + ); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + } + + throw lastError; +} + +/** + * Removes a directory recursively with retries for Windows file lock issues. + * On Windows, file handles may remain open briefly after operations like copy, + * causing EBUSY errors. This function retries with exponential backoff. + */ +export async function removeDirectoryWithRetry( + path: string, + options: { recursive?: boolean; force?: boolean } = {}, +): Promise { + await retryWithBackoff( + async () => { + await fs.promises.rm(path, { ...options, recursive: true, force: true }); + }, + 5, + 100, + ); +}