fs - retry rename on Windows and adopt (microsoft#188899)

bpasero · web-flow · commit a1c783c20cce · 2023-07-28T17:17:50.000+02:00
* fs - retry `rename` on Windows and adopt

* return early if target is not a file

* proper throw error

* align move and rename

* fix compile

* tests
diff --git a/src/bootstrap.js b/src/bootstrap.js
@@ -23,6 +23,7 @@
 	const Module = typeof require === 'function' ? require('module') : undefined;
 	const path = typeof require === 'function' ? require('path') : undefined;
 	const fs = typeof require === 'function' ? require('fs') : undefined;
+	const util = typeof require === 'function' ? require('util') : undefined;
 
 	//#region global bootstrapping
 
@@ -222,8 +223,8 @@
 			return ipcRenderer.invoke('vscode:readNlsFile', ...pathSegments);
 		}
 
-		if (fs && path) {
-			return (await fs.promises.readFile(path.join(...pathSegments))).toString();
+		if (fs && path && util) {
+			return (await util.promisify(fs.readFile)(path.join(...pathSegments))).toString();
 		}
 
 		throw new Error('Unsupported operation (read NLS files)');
@@ -240,8 +241,8 @@
 			return ipcRenderer.invoke('vscode:writeNlsFile', path, content);
 		}
 
-		if (fs) {
-			return fs.promises.writeFile(path, content);
+		if (fs && util) {
+			return util.promisify(fs.writeFile)(path, content);
 		}
 
 		throw new Error('Unsupported operation (write NLS files)');
diff --git a/src/vs/base/node/pfs.ts b/src/vs/base/node/pfs.ts
@@ -6,7 +6,7 @@
 import * as fs from 'fs';
 import { tmpdir } from 'os';
 import { promisify } from 'util';
-import { ResourceQueue } from 'vs/base/common/async';
+import { ResourceQueue, timeout } from 'vs/base/common/async';
 import { isEqualOrParent, isRootOrDriveLetter, randomPath } from 'vs/base/common/extpath';
 import { normalizeNFC } from 'vs/base/common/normalization';
 import { join } from 'vs/base/common/path';
@@ -491,16 +491,24 @@ function ensureWriteOptions(options?: IWriteFileOptions): IEnsuredWriteFileOptio
 /**
  * A drop-in replacement for `fs.rename` that:
  * - allows to move across multiple disks
+ * - attempts to retry the operation for certain error codes on Windows
  */
-async function move(source: string, target: string): Promise<void> {
+async function rename(source: string, target: string, windowsRetryTimeout: number | false = 60000 /* matches graceful-fs */): Promise<void> {
 	if (source === target) {
 		return;  // simulate node.js behaviour here and do a no-op if paths match
 	}
 
 	try {
-		await Promises.rename(source, target);
+		if (isWindows && typeof windowsRetryTimeout === 'number') {
+			// On Windows, a rename can fail when either source or target
+			// is locked by AV software. We do leverage graceful-fs to iron
+			// out these issues, however in case the target file exists,
+			// graceful-fs will immediately return without retry for fs.rename().
+			await renameWithRetry(source, target, Date.now(), windowsRetryTimeout);
+		} else {
+			await promisify(fs.rename)(source, target);
+		}
 	} catch (error) {
-
 		// In two cases we fallback to classic copy and delete:
 		//
 		// 1.) The EXDEV error indicates that source and target are on different devices
@@ -518,6 +526,44 @@ async function move(source: string, target: string): Promise<void> {
 	}
 }
 
+async function renameWithRetry(source: string, target: string, startTime: number, retryTimeout: number, attempt = 0): Promise<void> {
+	try {
+		return await promisify(fs.rename)(source, target);
+	} catch (error) {
+		if (error.code !== 'EACCES' && error.code !== 'EPERM' && error.code !== 'EBUSY') {
+			throw error; // only for errors we think are temporary
+		}
+
+		if (Date.now() - startTime >= retryTimeout) {
+			console.error(`[node.js fs] rename failed after ${attempt} retries with error: ${error}`);
+
+			throw error; // give up after configurable timeout
+		}
+
+		if (attempt === 0) {
+			let abortRetry = false;
+			try {
+				const { stat } = await SymlinkSupport.stat(target);
+				if (!stat.isFile()) {
+					abortRetry = true; // if target is not a file, EPERM error may be raised and we should not attempt to retry
+				}
+			} catch (error) {
+				// Ignore
+			}
+
+			if (abortRetry) {
+				throw error;
+			}
+		}
+
+		// Delay with incremental backoff up to 100ms
+		await timeout(Math.min(100, attempt * 10));
+
+		// Attempt again
+		return renameWithRetry(source, target, startTime, retryTimeout, attempt + 1);
+	}
+}
+
 interface ICopyPayload {
 	readonly root: { source: string; target: string };
 	readonly options: { preserveSymlinks: boolean };
@@ -694,7 +740,6 @@ export const Promises = new class {
 	get fdatasync() { return promisify(fs.fdatasync); }
 	get truncate() { return promisify(fs.truncate); }
 
-	get rename() { return promisify(fs.rename); }
 	get copyFile() { return promisify(fs.copyFile); }
 
 	get open() { return promisify(fs.open); }
@@ -733,7 +778,7 @@ export const Promises = new class {
 
 	get rm() { return rimraf; }
 
-	get move() { return move; }
+	get rename() { return rename; }
 	get copy() { return copy; }
 
 	//#endregion
diff --git a/src/vs/base/parts/storage/node/storage.ts b/src/vs/base/parts/storage/node/storage.ts
@@ -274,7 +274,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 			try {
 				await Promises.unlink(path);
 				try {
-					await Promises.rename(this.toBackupPath(path), path);
+					await Promises.rename(this.toBackupPath(path), path, false /* no retry */);
 				} catch (error) {
 					// ignore
 				}
diff --git a/src/vs/base/test/node/pfs/pfs.test.ts b/src/vs/base/test/node/pfs/pfs.test.ts
@@ -173,7 +173,7 @@ flakySuite('PFS', function () {
 		assert.ok(!fs.existsSync(testDir));
 	});
 
-	test('copy, move and delete', async () => {
+	test('copy, rename and delete', async () => {
 		const sourceDir = FileAccess.asFileUri('vs/base/test/node/pfs/fixtures').fsPath;
 		const parentDir = join(tmpdir(), 'vsctests', 'pfs');
 		const targetDir = randomPath(parentDir);
@@ -188,7 +188,7 @@ flakySuite('PFS', function () {
 		assert.ok(fs.statSync(join(targetDir, 'examples')).isDirectory());
 		assert.ok(fs.existsSync(join(targetDir, 'examples', 'small.jxs')));
 
-		await Promises.move(targetDir, targetDir2);
+		await Promises.rename(targetDir, targetDir2);
 
 		assert.ok(!fs.existsSync(targetDir));
 		assert.ok(fs.existsSync(targetDir2));
@@ -198,7 +198,34 @@ flakySuite('PFS', function () {
 		assert.ok(fs.statSync(join(targetDir2, 'examples')).isDirectory());
 		assert.ok(fs.existsSync(join(targetDir2, 'examples', 'small.jxs')));
 
-		await Promises.move(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'));
+		await Promises.rename(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'));
+
+		assert.ok(!fs.existsSync(join(targetDir2, 'index.html')));
+		assert.ok(fs.existsSync(join(targetDir2, 'index_moved.html')));
+
+		await Promises.rm(parentDir);
+
+		assert.ok(!fs.existsSync(parentDir));
+	});
+
+	test('rename without retry', async () => {
+		const sourceDir = FileAccess.asFileUri('vs/base/test/node/pfs/fixtures').fsPath;
+		const parentDir = join(tmpdir(), 'vsctests', 'pfs');
+		const targetDir = randomPath(parentDir);
+		const targetDir2 = randomPath(parentDir);
+
+		await Promises.copy(sourceDir, targetDir, { preserveSymlinks: true });
+		await Promises.rename(targetDir, targetDir2, false);
+
+		assert.ok(!fs.existsSync(targetDir));
+		assert.ok(fs.existsSync(targetDir2));
+		assert.ok(fs.existsSync(join(targetDir2, 'index.html')));
+		assert.ok(fs.existsSync(join(targetDir2, 'site.css')));
+		assert.ok(fs.existsSync(join(targetDir2, 'examples')));
+		assert.ok(fs.statSync(join(targetDir2, 'examples')).isDirectory());
+		assert.ok(fs.existsSync(join(targetDir2, 'examples', 'small.jxs')));
+
+		await Promises.rename(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'), false);
 
 		assert.ok(!fs.existsSync(join(targetDir2, 'index.html')));
 		assert.ok(fs.existsSync(join(targetDir2, 'index_moved.html')));
diff --git a/src/vs/platform/backup/electron-main/backupMainService.ts b/src/vs/platform/backup/electron-main/backupMainService.ts
@@ -130,7 +130,7 @@ export class BackupMainService implements IBackupMainService {
 		// When we have data to migrate from, move it over to the target location
 		if (await Promises.exists(moveFromPath)) {
 			try {
-				await Promises.rename(moveFromPath, backupPath);
+				await Promises.rename(moveFromPath, backupPath, false /* no retry */);
 			} catch (error) {
 				this.logService.error(`Backup: Could not move backup folder to new location: ${error.toString()}`);
 			}
@@ -285,7 +285,7 @@ export class BackupMainService implements IBackupMainService {
 		// Rename backupPath to new empty window backup path
 		const newEmptyWindowBackupPath = join(this.backupHome, newEmptyWindowBackupInfo.backupFolder);
 		try {
-			await Promises.rename(backupPath, newEmptyWindowBackupPath);
+			await Promises.rename(backupPath, newEmptyWindowBackupPath, false /* no retry */);
 		} catch (error) {
 			this.logService.error(`Backup: Could not rename backup folder: ${error.toString()}`);
 			return false;
diff --git a/src/vs/platform/extensionManagement/node/extensionDownloader.ts b/src/vs/platform/extensionManagement/node/extensionDownloader.ts
@@ -7,7 +7,6 @@ import { Promises } from 'vs/base/common/async';
 import { getErrorMessage } from 'vs/base/common/errors';
 import { Disposable } from 'vs/base/common/lifecycle';
 import { Schemas } from 'vs/base/common/network';
-import { isWindows } from 'vs/base/common/platform';
 import { joinPath } from 'vs/base/common/resources';
 import * as semver from 'vs/base/common/semver/semver';
 import { isBoolean } from 'vs/base/common/types';
@@ -129,7 +128,7 @@ export class ExtensionsDownloader extends Disposable {
 
 		try {
 			// Rename temp location to original
-			await this.rename(tempLocation, location, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
+			await FSPromises.rename(tempLocation.fsPath, location.fsPath, 2 * 60 * 1000 /* Retry for 2 minutes */);
 		} catch (error) {
 			try {
 				await this.fileService.del(tempLocation);
@@ -148,18 +147,6 @@ export class ExtensionsDownloader extends Disposable {
 		await this.fileService.del(location);
 	}
 
-	private async rename(from: URI, to: URI, retryUntil: number): Promise<void> {
-		try {
-			await FSPromises.rename(from.fsPath, to.fsPath);
-		} catch (error) {
-			if (isWindows && error && error.code === 'EPERM' && Date.now() < retryUntil) {
-				this.logService.info(`Failed renaming ${from} to ${to} with 'EPERM' error. Trying again...`);
-				return this.rename(from, to, retryUntil);
-			}
-			throw error;
-		}
-	}
-
 	private async cleanUp(): Promise<void> {
 		try {
 			if (!(await this.fileService.exists(this.extensionsDownloadDir))) {
diff --git a/src/vs/platform/extensionManagement/node/extensionManagementService.ts b/src/vs/platform/extensionManagement/node/extensionManagementService.ts
@@ -15,7 +15,6 @@ import { Disposable } from 'vs/base/common/lifecycle';
 import { ResourceSet } from 'vs/base/common/map';
 import { Schemas } from 'vs/base/common/network';
 import * as path from 'vs/base/common/path';
-import { isWindows } from 'vs/base/common/platform';
 import { joinPath } from 'vs/base/common/resources';
 import * as semver from 'vs/base/common/semver/semver';
 import { isBoolean, isUndefined } from 'vs/base/common/types';
@@ -508,7 +507,7 @@ export class ExtensionsScanner extends Disposable {
 				// Rename
 				try {
 					this.logService.trace(`Started renaming the extension from ${tempLocation.fsPath} to ${extensionLocation.fsPath}`);
-					await this.rename(extensionKey, tempLocation.fsPath, extensionLocation.fsPath, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
+					await this.rename(tempLocation.fsPath, extensionLocation.fsPath);
 					this.logService.info('Renamed to', extensionLocation.fsPath);
 				} catch (error) {
 					if (error.code === 'ENOTEMPTY') {
@@ -606,7 +605,7 @@ export class ExtensionsScanner extends Disposable {
 	private async deleteExtensionFromLocation(id: string, location: URI, type: string): Promise<void> {
 		this.logService.trace(`Deleting ${type} extension from disk`, id, location.fsPath);
 		const renamedLocation = this.uriIdentityService.extUri.joinPath(this.uriIdentityService.extUri.dirname(location), `${this.uriIdentityService.extUri.basename(location)}.${hash(generateUuid()).toString(16)}${DELETED_FOLDER_POSTFIX}`);
-		await this.rename({ id }, location.fsPath, renamedLocation.fsPath, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
+		await this.rename(location.fsPath, renamedLocation.fsPath);
 		await this.fileService.del(renamedLocation, { recursive: true });
 		this.logService.info(`Deleted ${type} extension from disk`, id, location.fsPath);
 	}
@@ -643,14 +642,10 @@ export class ExtensionsScanner extends Disposable {
 		});
 	}
 
-	private async rename(identifier: IExtensionIdentifier, extractPath: string, renamePath: string, retryUntil: number): Promise<void> {
+	private async rename(extractPath: string, renamePath: string): Promise<void> {
 		try {
-			await pfs.Promises.rename(extractPath, renamePath);
+			await pfs.Promises.rename(extractPath, renamePath, 2 * 60 * 1000 /* Retry for 2 minutes */);
 		} catch (error) {
-			if (isWindows && error && error.code === 'EPERM' && Date.now() < retryUntil) {
-				this.logService.info(`Failed renaming ${extractPath} to ${renamePath} with 'EPERM' error. Trying again...`, identifier.id);
-				return this.rename(identifier, extractPath, renamePath, retryUntil);
-			}
 			throw new ExtensionManagementError(error.message || nls.localize('renameError', "Unknown error while renaming {0} to {1}", extractPath, renamePath), error.code || ExtensionManagementErrorCode.Rename);
 		}
 	}
diff --git a/src/vs/platform/files/node/diskFileSystemProvider.ts b/src/vs/platform/files/node/diskFileSystemProvider.ts
@@ -640,8 +640,8 @@ export class DiskFileSystemProvider extends AbstractDiskFileSystemProvider imple
 			// Validate the move operation can perform
 			await this.validateMoveCopy(from, to, 'move', opts.overwrite);
 
-			// Move
-			await Promises.move(fromFilePath, toFilePath);
+			// Rename
+			await Promises.rename(fromFilePath, toFilePath);
 		} catch (error) {
 
 			// Rewrite some typical errors that can happen especially around symlinks
diff --git a/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts b/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts
@@ -252,7 +252,7 @@ import { FileAccess } from 'vs/base/common/network';
 
 		// Move file
 		changeFuture = awaitEvent(watcher, filePath, FileChangeType.DELETED);
-		await Promises.move(filePath, `${filePath}-moved`);
+		await Promises.rename(filePath, `${filePath}-moved`);
 		await changeFuture;
 	});
 
diff --git a/src/vs/platform/remote/node/wsl.ts b/src/vs/platform/remote/node/wsl.ts
@@ -5,7 +5,7 @@
 
 import * as os from 'os';
 import * as cp from 'child_process';
-import { promises as fs } from 'fs';
+import { Promises } from 'vs/base/node/pfs';
 import * as path from 'path';
 
 let hasWSLFeaturePromise: Promise<boolean> | undefined;
@@ -33,7 +33,7 @@ async function testWSLFeatureInstalled(): Promise<boolean> {
 		const dllPath = getLxssManagerDllPath();
 		if (dllPath) {
 			try {
-				if ((await fs.stat(dllPath)).isFile()) {
+				if ((await Promises.stat(dllPath)).isFile()) {
 					return true;
 				}
 			} catch (e) {
diff --git a/src/vs/platform/terminal/node/terminalProcess.ts b/src/vs/platform/terminal/node/terminalProcess.ts
@@ -4,7 +4,6 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { exec } from 'child_process';
-import { promises as fs } from 'fs';
 import { timeout } from 'vs/base/common/async';
 import { Emitter, Event } from 'vs/base/common/event';
 import { Disposable } from 'vs/base/common/lifecycle';
@@ -213,9 +212,9 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
 				}
 				if (injection.filesToCopy) {
 					for (const f of injection.filesToCopy) {
-						await fs.mkdir(path.dirname(f.dest), { recursive: true });
+						await Promises.mkdir(path.dirname(f.dest), { recursive: true });
 						try {
-							await fs.copyFile(f.source, f.dest);
+							await Promises.copyFile(f.source, f.dest);
 						} catch {
 							// Swallow error, this should only happen when multiple users are on the same
 							// machine. Since the shell integration scripts rarely change, plus the other user
diff --git a/src/vs/platform/update/electron-main/updateService.win32.ts b/src/vs/platform/update/electron-main/updateService.win32.ts
@@ -137,7 +137,7 @@ export class Win32UpdateService extends AbstractUpdateService {
 							return this.requestService.request({ url }, CancellationToken.None)
 								.then(context => this.fileService.writeFile(URI.file(downloadPath), context.stream))
 								.then(hash ? () => checksum(downloadPath, update.hash) : () => undefined)
-								.then(() => pfs.Promises.rename(downloadPath, updatePackagePath))
+								.then(() => pfs.Promises.rename(downloadPath, updatePackagePath, false /* no retry */))
 								.then(() => updatePackagePath);
 						});
 					}).then(packagePath => {
diff --git a/src/vs/server/node/webClientServer.ts b/src/vs/server/node/webClientServer.ts

Original file line number	Diff line number	Diff line change
`@@ -23,6 +23,7 @@`
`23`	`23`	`const Module = typeof require === 'function' ? require('module') : undefined;`
`24`	`24`	`const path = typeof require === 'function' ? require('path') : undefined;`
`25`	`25`	`const fs = typeof require === 'function' ? require('fs') : undefined;`
	`26`	`+ const util = typeof require === 'function' ? require('util') : undefined;`
`26`	`27`
`27`	`28`	`//#region global bootstrapping`
`28`	`29`
`@@ -222,8 +223,8 @@`
`222`	`223`	`return ipcRenderer.invoke('vscode:readNlsFile', ...pathSegments);`
`223`	`224`	`}`
`224`	`225`
`225`		`- if (fs && path) {`
`226`		`- return (await fs.promises.readFile(path.join(...pathSegments))).toString();`
	`226`	`+ if (fs && path && util) {`
	`227`	`+ return (await util.promisify(fs.readFile)(path.join(...pathSegments))).toString();`
`227`	`228`	`}`
`228`	`229`
`229`	`230`	`throw new Error('Unsupported operation (read NLS files)');`
`@@ -240,8 +241,8 @@`
`240`	`241`	`return ipcRenderer.invoke('vscode:writeNlsFile', path, content);`
`241`	`242`	`}`
`242`	`243`
`243`		`- if (fs) {`
`244`		`- return fs.promises.writeFile(path, content);`
	`244`	`+ if (fs && util) {`
	`245`	`+ return util.promisify(fs.writeFile)(path, content);`
`245`	`246`	`}`
`246`	`247`
`247`	`248`	`throw new Error('Unsupported operation (write NLS files)');`
Original file line number	Diff line number	Diff line change
`@@ -274,7 +274,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {`
`274`	`274`	`try {`
`275`	`275`	`await Promises.unlink(path);`
`276`	`276`	`try {`
`277`		`- await Promises.rename(this.toBackupPath(path), path);`
	`277`	`+ await Promises.rename(this.toBackupPath(path), path, false /* no retry */);`
`278`	`278`	`} catch (error) {`
`279`	`279`	`// ignore`
`280`	`280`	`}`