backup - improve discardAllBackups() semantics

src/vs/workbench/contrib/backup/electron-browser/backupTracker.ts

@@ -53,7 +53,7 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 						return this.handleDirtyBeforeShutdown(remainingDirtyWorkingCopies, reason);
 					}
 
-					return this.noVeto({ dicardAllBackups: false }); // no veto and no backup cleanup (since there are no dirty working copies)
+					return this.noVeto({ dicardAllBackups: false }); // no veto (there are no remaining dirty working copies)
 				});
 			}
 
@@ -61,8 +61,7 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 			return this.handleDirtyBeforeShutdown(dirtyWorkingCopies, reason);
 		}
 
-		// No dirty working copies: no veto
-		return this.noVeto({ dicardAllBackups: true });
+		return this.noVeto({ dicardAllBackups: true }); // no veto (no dirty working copies)
 	}
 
 	private async handleDirtyBeforeShutdown(workingCopies: IWorkingCopy[], reason: ShutdownReason): Promise<boolean> {
@@ -79,8 +78,14 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 		}
 
 		if (!didBackup) {
-			// since a backup did not happen, we have to confirm for the dirty working copies now
-			return this.confirmBeforeShutdown();
+			try {
+				// since a backup did not happen, we have to confirm for the dirty working copies now
+				return await this.confirmBeforeShutdown();
+			} catch (error) {
+				this.notificationService.error(localize('backupTrackerConfirmFailed', "Working copies that are dirty could not be saved or reverted (Error: {0}). Try saving your editors first and then exit.", error.message));
+
+				return true; // veto (save or revert failed)
+			}
 		}
 
 		if (backupError || workingCopies.some(workingCopy => !this.backupFileService.hasBackupSync(workingCopy.resource, this.getContentVersion(workingCopy)))) {
@@ -92,11 +97,10 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 				this.notificationService.error(localize('backupTrackerBackupIncomplete', "Some working copies that are dirty could not be backed up. Try saving your editors first and then exit."));
 			}
 
-			return true; // veto, the backups failed
+			return true; // veto (the backups failed)
 		}
 
-		// no veto and no backup cleanup (since backup was successful)
-		return this.noVeto({ dicardAllBackups: false });
+		return this.noVeto({ dicardAllBackups: false }); // no veto (backup was successful)
 	}
 
 	private async backupBeforeShutdown(workingCopies: IWorkingCopy[], reason: ShutdownReason): Promise<boolean> {
@@ -160,49 +164,47 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 		if (confirm === ConfirmResult.SAVE) {
 			await this.doSaveAllBeforeShutdown(dirtyWorkingCopies, true /* includeUntitled */);
 
-			return this.noVeto({ dicardAllBackups: true });
+			return this.noVeto({ dicardAllBackups: true }); // no veto (dirty saved)
 		}
 
 		// Don't Save
 		else if (confirm === ConfirmResult.DONT_SAVE) {
 			await this.doRevertAllBeforeShutdown(dirtyWorkingCopies);
 
-			return this.noVeto({ dicardAllBackups: true });
+			return this.noVeto({ dicardAllBackups: true }); // no veto (dirty reverted)
 		}
 
 		// Cancel
-		else if (confirm === ConfirmResult.CANCEL) {
-			return true; // veto
-		}
-
-		return false;
+		return true; // veto (user canceled)
 	}
 
-	private doSaveAllBeforeShutdown(workingCopies: IWorkingCopy[], includeUntitled: boolean): Promise<boolean[]> {
-		return Promise.all(workingCopies.map(async workingCopy => {
-			if (workingCopy.isDirty() && (includeUntitled || !(workingCopy.capabilities & WorkingCopyCapabilities.Untitled))) {
-				return workingCopy.save({ skipSaveParticipants: true }); // skip save participants on shutdown for performance reasons
+	private async doSaveAllBeforeShutdown(dirtyWorkingCopies: IWorkingCopy[], includeUntitled: boolean): Promise<void> {
+		await Promise.all(dirtyWorkingCopies.map(async workingCopy => {
+			if (!includeUntitled && (workingCopy.capabilities & WorkingCopyCapabilities.Untitled)) {
+				return; // skip untitled unless explicitly included
 			}
 
-			return false;
+			return workingCopy.save({ skipSaveParticipants: true }); // skip save participants on shutdown for performance reasons
 		}));
 	}
 
-	private doRevertAllBeforeShutdown(workingCopies: IWorkingCopy[]): Promise<boolean[]> {
-		return Promise.all(workingCopies.map(workingCopy => workingCopy.revert({ soft: true }))); // soft revert is good enough on shutdown
+	private async doRevertAllBeforeShutdown(dirtyWorkingCopies: IWorkingCopy[]): Promise<void> {
+		await Promise.all(dirtyWorkingCopies.map(workingCopy => workingCopy.revert({ soft: true }))); // soft revert is good enough on shutdown
 	}
 
 	private noVeto(options: { dicardAllBackups: boolean }): boolean | Promise<boolean> {
-		let dicardAllBackups = options.dicardAllBackups;
+		if (!options.dicardAllBackups) {
+			return false;
+		}
 
 		if (this.lifecycleService.phase < LifecyclePhase.Restored) {
-			dicardAllBackups = false; // if editors have not restored, we are not up to speed with backups and thus should not discard them
+			return false; // if editors have not restored, we are not up to speed with backups and thus should not discard them
 		}
 
 		if (this.environmentService.isExtensionDevelopment) {
-			dicardAllBackups = false; // extension development does not track any backups
+			return false; // extension development does not track any backups
 		}
 
-		return this.backupFileService.shutdown({ dicardAllBackups }).then(() => false, () => false);
+		return this.backupFileService.discardAllBackups().then(() => false, () => false);
 	}
 }

src/vs/workbench/contrib/backup/test/electron-browser/backupTracker.test.ts

@@ -279,13 +279,13 @@ suite('BackupTracker', () => {
 
 		let veto = event.value;
 		if (typeof veto === 'boolean') {
-			assert.ok(accessor.backupFileService.didDiscardAllWorkspaceBackups);
+			assert.ok(accessor.backupFileService.didDiscardAllBackups);
 			assert.ok(!veto);
 			return;
 		}
 
 		veto = await veto;
-		assert.ok(accessor.backupFileService.didDiscardAllWorkspaceBackups);
+		assert.ok(accessor.backupFileService.didDiscardAllBackups);
 		assert.ok(!veto);
 
 		part.dispose();
@@ -453,7 +453,7 @@ suite('BackupTracker', () => {
 			accessor.lifecycleService.fireWillShutdown(event);
 
 			const veto = await (<Promise<boolean>>event.value);
-			assert.ok(!accessor.backupFileService.didDiscardAllWorkspaceBackups); // When hot exit is set, backups should never be cleaned since the confirm result is cancel
+			assert.ok(!accessor.backupFileService.didDiscardAllBackups); // When hot exit is set, backups should never be cleaned since the confirm result is cancel
 			assert.equal(veto, shouldVeto);
 
 			part.dispose();

src/vs/workbench/services/backup/common/backup.ts

@@ -66,7 +66,7 @@ export interface IBackupFileService {
 	discardBackup(resource: URI): Promise<void>;
 
 	/**
-	 * Shutdown the service and optionally discard any backups.
+	 * Discards all backups that exist.
 	 */
-	shutdown(options?: { dicardAllBackups: boolean }): Promise<void>;
+	discardAllBackups(): Promise<void>;
 }

src/vs/workbench/services/backup/common/backupFileService.ts

@@ -19,6 +19,7 @@ import { Schemas } from 'vs/base/common/network';
 import { IWorkbenchEnvironmentService } from 'vs/workbench/services/environment/common/environmentService';
 import { VSBuffer } from 'vs/base/common/buffer';
 import { TextSnapshotReadable, stringToSnapshot } from 'vs/workbench/services/textfile/common/textfiles';
+import { Disposable } from 'vs/base/common/lifecycle';
 
 export interface IBackupFilesModel {
 	resolve(backupRoot: URI): Promise<IBackupFilesModel>;
@@ -162,8 +163,8 @@ export class BackupFileService implements IBackupFileService {
 		return this.impl.discardBackup(resource);
 	}
 
-	shutdown(options?: { dicardAllBackups: boolean }): Promise<void> {
-		return this.impl.shutdown(options);
+	discardAllBackups(): Promise<void> {
+		return this.impl.discardAllBackups();
 	}
 
 	getBackups(): Promise<URI[]> {
@@ -179,7 +180,7 @@ export class BackupFileService implements IBackupFileService {
 	}
 }
 
-class BackupFileServiceImpl implements IBackupFileService {
+class BackupFileServiceImpl extends Disposable implements IBackupFileService {
 
 	private static readonly PREAMBLE_END_MARKER = '\n';
 	private static readonly PREAMBLE_META_SEPARATOR = ' '; // using a character that is know to be escaped in a URI as separator
@@ -189,8 +190,7 @@ class BackupFileServiceImpl implements IBackupFileService {
 
 	private backupWorkspacePath!: URI;
 
-	private readonly ioOperationQueues = new ResourceQueue(); // queue IO operations to ensure write order
-	private isShutdown = false; // a flag to set indicating that we no longer backup any resources
+	private readonly ioOperationQueues = this._register(new ResourceQueue()); // queue IO operations to ensure write/delete file order
 
 	private ready!: Promise<IBackupFilesModel>;
 	private model!: IBackupFilesModel;
@@ -200,6 +200,8 @@ class BackupFileServiceImpl implements IBackupFileService {
 		private readonly hashPath: (resource: URI) => string,
 		@IFileService private readonly fileService: IFileService
 	) {
+		super();
+
 		this.initialize(backupWorkspaceResource);
 	}
 
@@ -228,10 +230,6 @@ class BackupFileServiceImpl implements IBackupFileService {
 	}
 
 	async backup<T extends object>(resource: URI, content?: ITextSnapshot, versionId?: number, meta?: T): Promise<void> {
-		if (this.isShutdown) {
-			return;
-		}
-
 		const model = await this.ready;
 
 		const backupResource = this.toBackupResource(resource);
@@ -263,13 +261,14 @@ class BackupFileServiceImpl implements IBackupFileService {
 		});
 	}
 
-	async discardBackup(resource: URI): Promise<void> {
-		if (this.isShutdown) {
-			return;
-		}
+	discardBackup(resource: URI): Promise<void> {
+		const backupResource = this.toBackupResource(resource);
 
+		return this.doDiscardBackup(backupResource);
+	}
+
+	private async doDiscardBackup(backupResource: URI): Promise<void> {
 		const model = await this.ready;
-		const backupResource = this.toBackupResource(resource);
 
 		return this.ioOperationQueues.queueFor(backupResource).queue(async () => {
 			await this.deleteIgnoreFileNotFound(backupResource);
@@ -278,22 +277,10 @@ class BackupFileServiceImpl implements IBackupFileService {
 		});
 	}
 
-	async shutdown(options?: { dicardAllBackups: boolean }): Promise<void> {
-
-		// Signal that no more backups should happen from this point
-		this.isShutdown = true;
-		this.ioOperationQueues.dispose();
-
-		// Optionally discard backups
-		if (options?.dicardAllBackups) {
-
-			// Clear model
-			const model = await this.ready;
-			model.clear();
+	async discardAllBackups(): Promise<void> {
+		const model = await this.ready;
 
-			// Delete the backup home for this workspace
-			await this.deleteIgnoreFileNotFound(this.backupWorkspacePath);
-		}
+		await Promise.all(model.get().map(backupResource => this.doDiscardBackup(backupResource)));
 	}
 
 	private async deleteIgnoreFileNotFound(resource: URI): Promise<void> {
@@ -309,8 +296,8 @@ class BackupFileServiceImpl implements IBackupFileService {
 	async getBackups(): Promise<URI[]> {
 		const model = await this.ready;
 
-		const backups = await Promise.all(model.get().map(async fileBackup => {
-			const backupPreamble = await this.readToMatchingString(fileBackup, BackupFileServiceImpl.PREAMBLE_END_MARKER, BackupFileServiceImpl.PREAMBLE_MAX_LENGTH);
+		const backups = await Promise.all(model.get().map(async backupResource => {
+			const backupPreamble = await this.readToMatchingString(backupResource, BackupFileServiceImpl.PREAMBLE_END_MARKER, BackupFileServiceImpl.PREAMBLE_MAX_LENGTH);
 			if (!backupPreamble) {
 				return undefined;
 			}
@@ -447,7 +434,7 @@ export class InMemoryBackupFileService implements IBackupFileService {
 		this.backups.delete(this.toBackupResource(resource).toString());
 	}
 
-	async shutdown(options?: { dicardAllBackups: boolean }): Promise<void> {
+	async discardAllBackups(): Promise<void> {
 		this.backups.clear();
 	}
 

src/vs/workbench/services/backup/test/electron-browser/backupFileService.test.ts

@@ -72,7 +72,7 @@ export class NodeTestBackupFileService extends BackupFileService {
 		this.fileService = fileService;
 		this.backupResourceJoiners = [];
 		this.discardBackupJoiners = [];
-		this.didDiscardAllWorkspaceBackups = false;
+		this.didDiscardAllBackups = false;
 	}
 
 	joinBackupResource(): Promise<void> {
@@ -99,12 +99,12 @@ export class NodeTestBackupFileService extends BackupFileService {
 		}
 	}
 
-	didDiscardAllWorkspaceBackups: boolean;
+	didDiscardAllBackups: boolean;
 
-	shutdown(options?: { dicardAllBackups: boolean }): Promise<void> {
-		this.didDiscardAllWorkspaceBackups = !!options?.dicardAllBackups;
+	discardAllBackups(): Promise<void> {
+		this.didDiscardAllBackups = true;
 
-		return super.shutdown(options);
+		return super.discardAllBackups();
 	}
 }
 
@@ -283,30 +283,22 @@ suite('BackupFileService', () => {
 		});
 	});
 
-	suite('discardBackups', () => {
+	suite('discardAllBackups', () => {
 		test('text file', async () => {
 			await service.backup(fooFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false));
 			assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'file')).length, 1);
 			await service.backup(barFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false));
 			assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'file')).length, 2);
-			await service.shutdown({ dicardAllBackups: true });
+			await service.discardAllBackups();
 			assert.equal(fs.existsSync(fooBackupPath), false);
 			assert.equal(fs.existsSync(barBackupPath), false);
-			assert.equal(fs.existsSync(path.join(workspaceBackupPath, 'file')), false);
 		});
 
 		test('untitled file', async () => {
 			await service.backup(untitledFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false));
 			assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'untitled')).length, 1);
-			await service.shutdown({ dicardAllBackups: true });
+			await service.discardAllBackups();
 			assert.equal(fs.existsSync(untitledBackupPath), false);
-			assert.equal(fs.existsSync(path.join(workspaceBackupPath, 'untitled')), false);
-		});
-
-		test('should disable further backups', async () => {
-			await service.shutdown({ dicardAllBackups: true });
-			await service.backup(untitledFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false));
-			assert.equal(fs.existsSync(workspaceBackupPath), false);
 		});
 	});
 

src/vs/workbench/test/workbenchTestServices.ts

@@ -1198,7 +1198,7 @@ export class TestBackupFileService implements IBackupFileService {
 		return Promise.resolve();
 	}
 
-	shutdown(options?: { dicardAllBackups: boolean }): Promise<void> {
+	discardAllBackups(): Promise<void> {
 		return Promise.resolve();
 	}
 }