Web: do not disable backups when auto save is enabled (fix #108789)

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

@@ -5,22 +5,53 @@
 
 import { IBackupFileService } from 'vs/workbench/services/backup/common/backup';
 import { IWorkbenchContribution } from 'vs/workbench/common/contributions';
-import { IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService';
-import { IWorkingCopyService } from 'vs/workbench/services/workingCopy/common/workingCopyService';
+import { AutoSaveMode, IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService';
+import { IWorkingCopy, IWorkingCopyService, WorkingCopyCapabilities } from 'vs/workbench/services/workingCopy/common/workingCopyService';
 import { ILifecycleService, ShutdownReason } from 'vs/platform/lifecycle/common/lifecycle';
 import { ILogService } from 'vs/platform/log/common/log';
 import { BackupTracker } from 'vs/workbench/contrib/backup/common/backupTracker';
 
 export class BrowserBackupTracker extends BackupTracker implements IWorkbenchContribution {
 
+	// Delay creation of backups when content changes to avoid too much
+	// load on the backup service when the user is typing into the editor
+	// Since we always schedule a backup, even when auto save is on (web
+	// only), we have different scheduling delays based on auto save. This
+	// helps to avoid a race between saving (after 1s per default) and making
+	// a backup of the working copy.
+	private static readonly BACKUP_SCHEDULE_DELAYS = {
+		[AutoSaveMode.OFF]: 1000,
+		[AutoSaveMode.ON_FOCUS_CHANGE]: 1000,
+		[AutoSaveMode.ON_WINDOW_CHANGE]: 1000,
+		[AutoSaveMode.AFTER_SHORT_DELAY]: 2000, // explicitly higher to prevent races
+		[AutoSaveMode.AFTER_LONG_DELAY]: 1000
+	};
+
 	constructor(
 		@IBackupFileService backupFileService: IBackupFileService,
-		@IFilesConfigurationService filesConfigurationService: IFilesConfigurationService,
+		@IFilesConfigurationService private readonly filesConfigurationService: IFilesConfigurationService,
 		@IWorkingCopyService workingCopyService: IWorkingCopyService,
 		@ILifecycleService lifecycleService: ILifecycleService,
 		@ILogService logService: ILogService
 	) {
-		super(backupFileService, filesConfigurationService, workingCopyService, logService, lifecycleService);
+		super(backupFileService, workingCopyService, logService, lifecycleService);
+	}
+
+	protected shouldScheduleBackup(workingCopy: IWorkingCopy): boolean {
+		// Web: we always want to schedule a backup, even if auto save
+		// is enabled because in web there is no handler on shutdown
+		// to trigger saving so there is a higher chance of dataloss.
+		// See https://github.com/microsoft/vscode/issues/108789
+		return true;
+	}
+
+	protected getBackupScheduleDelay(workingCopy: IWorkingCopy): number {
+		let autoSaveMode = this.filesConfigurationService.getAutoSaveMode();
+		if (workingCopy.capabilities & WorkingCopyCapabilities.Untitled) {
+			autoSaveMode = AutoSaveMode.OFF; // auto-save is never on for untitled working copies
+		}
+
+		return BrowserBackupTracker.BACKUP_SCHEDULE_DELAYS[autoSaveMode];
 	}
 
 	protected onBeforeShutdown(reason: ShutdownReason): boolean | Promise<boolean> {

src/vs/workbench/contrib/backup/common/backupTracker.ts

@@ -5,47 +5,28 @@
 
 import { IBackupFileService } from 'vs/workbench/services/backup/common/backup';
 import { Disposable, IDisposable, dispose, toDisposable } from 'vs/base/common/lifecycle';
-import { IFilesConfigurationService, IAutoSaveConfiguration } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService';
-import { IWorkingCopyService, IWorkingCopy, WorkingCopyCapabilities } from 'vs/workbench/services/workingCopy/common/workingCopyService';
+import { IWorkingCopyService, IWorkingCopy } from 'vs/workbench/services/workingCopy/common/workingCopyService';
 import { ILogService } from 'vs/platform/log/common/log';
 import { ShutdownReason, ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle';
 
 export abstract class BackupTracker extends Disposable {
 
-	// Disable backup for when a short auto-save delay is configured with
-	// the rationale that the auto save will trigger a save periodically
-	// anway and thus creating frequent backups is not useful
-	//
-	// This will only apply to working copies that are not untitled where
-	// auto save is actually saving.
-	private static DISABLE_BACKUP_AUTO_SAVE_THRESHOLD = 1500;
-
-	// Delay creation of backups when content changes to avoid too much
-	// load on the backup service when the user is typing into the editor
-	protected static BACKUP_FROM_CONTENT_CHANGE_DELAY = 1000;
-
 	// A map from working copy to a version ID we compute on each content
 	// change. This version ID allows to e.g. ask if a backup for a specific
 	// content has been made before closing.
 	private readonly mapWorkingCopyToContentVersion = new Map<IWorkingCopy, number>();
 
-	private backupsDisabledForAutoSaveables = false;
-
 	// A map of scheduled pending backups for working copies
 	private readonly pendingBackups = new Map<IWorkingCopy, IDisposable>();
 
 	constructor(
 		protected readonly backupFileService: IBackupFileService,
-		protected readonly filesConfigurationService: IFilesConfigurationService,
 		protected readonly workingCopyService: IWorkingCopyService,
 		protected readonly logService: ILogService,
 		protected readonly lifecycleService: ILifecycleService
 	) {
 		super();
 
-		// Figure out initial auto save config
-		this.onAutoSaveConfigurationChange(filesConfigurationService.getAutoSaveConfiguration());
-
 		// Fill in initial dirty working copies
 		this.workingCopyService.dirtyWorkingCopies.forEach(workingCopy => this.onDidRegister(workingCopy));
 
@@ -60,9 +41,6 @@ export abstract class BackupTracker extends Disposable {
 		this._register(this.workingCopyService.onDidChangeDirty(workingCopy => this.onDidChangeDirty(workingCopy)));
 		this._register(this.workingCopyService.onDidChangeContent(workingCopy => this.onDidChangeContent(workingCopy)));
 
-		// Listen to auto save config changes
-		this._register(this.filesConfigurationService.onAutoSaveConfigurationChange(c => this.onAutoSaveConfigurationChange(c)));
-
 		// Lifecycle (handled in subclasses)
 		this.lifecycleService.onBeforeShutdown(event => event.veto(this.onBeforeShutdown(event.reason)));
 	}
@@ -105,13 +83,21 @@ export abstract class BackupTracker extends Disposable {
 		}
 	}
 
-	private onAutoSaveConfigurationChange(configuration: IAutoSaveConfiguration): void {
-		this.backupsDisabledForAutoSaveables = typeof configuration.autoSaveDelay === 'number' && configuration.autoSaveDelay < BackupTracker.DISABLE_BACKUP_AUTO_SAVE_THRESHOLD;
-	}
+	/**
+	 * Allows subclasses to conditionally opt-out of doing a backup, e.g. if
+	 * auto save is enabled.
+	 */
+	protected abstract shouldScheduleBackup(workingCopy: IWorkingCopy): boolean;
+
+	/**
+	 * Allows subclasses to control the delay before performing a backup from
+	 * working copy content changes.
+	 */
+	protected abstract getBackupScheduleDelay(workingCopy: IWorkingCopy): number;
 
 	private scheduleBackup(workingCopy: IWorkingCopy): void {
-		if (this.backupsDisabledForAutoSaveables && !(workingCopy.capabilities & WorkingCopyCapabilities.Untitled)) {
-			return; // skip if auto save is enabled with a short delay
+		if (!this.shouldScheduleBackup(workingCopy)) {
+			return; // subclass prevented backup for working copy
 		}
 
 		// Clear any running backup operation
@@ -137,7 +123,7 @@ export abstract class BackupTracker extends Disposable {
 					this.logService.error(error);
 				}
 			}
-		}, BackupTracker.BACKUP_FROM_CONTENT_CHANGE_DELAY);
+		}, this.getBackupScheduleDelay(workingCopy));
 
 		// Keep in map for disposal as needed
 		this.pendingBackups.set(workingCopy, toDisposable(() => {

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

@@ -23,9 +23,21 @@ import { IEnvironmentService } from 'vs/platform/environment/common/environment'
 
 export class NativeBackupTracker extends BackupTracker implements IWorkbenchContribution {
 
+	// Delay creation of backups when working copy changes to avoid too much
+	// load on the backup service when the user is typing into the editor
+	private static readonly BACKUP_SCHEDULE_DELAY = 1000;
+
+	// Disable backup for when a short auto-save delay is configured with
+	// the rationale that the auto save will trigger a save periodically
+	// anway and thus creating frequent backups is not useful
+	//
+	// This will only apply to working copies that are not untitled where
+	// auto save is actually saving.
+	private static readonly DISABLE_BACKUP_AUTO_SAVE_THRESHOLD = 1500;
+
 	constructor(
 		@IBackupFileService backupFileService: IBackupFileService,
-		@IFilesConfigurationService filesConfigurationService: IFilesConfigurationService,
+		@IFilesConfigurationService private readonly filesConfigurationService: IFilesConfigurationService,
 		@IWorkingCopyService workingCopyService: IWorkingCopyService,
 		@ILifecycleService lifecycleService: ILifecycleService,
 		@IFileDialogService private readonly fileDialogService: IFileDialogService,
@@ -36,7 +48,24 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont
 		@IEditorService private readonly editorService: IEditorService,
 		@IEnvironmentService private readonly environmentService: IEnvironmentService
 	) {
-		super(backupFileService, filesConfigurationService, workingCopyService, logService, lifecycleService);
+		super(backupFileService, workingCopyService, logService, lifecycleService);
+	}
+
+	protected shouldScheduleBackup(workingCopy: IWorkingCopy): boolean {
+		if (workingCopy.capabilities & WorkingCopyCapabilities.Untitled) {
+			return true; // always backup untitled
+		}
+
+		const autoSaveConfiguration = this.filesConfigurationService.getAutoSaveConfiguration();
+		if (typeof autoSaveConfiguration.autoSaveDelay === 'number' && autoSaveConfiguration.autoSaveDelay < NativeBackupTracker.DISABLE_BACKUP_AUTO_SAVE_THRESHOLD) {
+			return false; // skip backup when auto save is already enabled with a low delay
+		}
+
+		return true;
+	}
+
+	protected getBackupScheduleDelay(): number {
+		return NativeBackupTracker.BACKUP_SCHEDULE_DELAY;
 	}
 
 	protected onBeforeShutdown(reason: ShutdownReason): boolean | Promise<boolean> {

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

@@ -69,9 +69,10 @@ class TestBackupTracker extends NativeBackupTracker {
 		@IEnvironmentService environmentService: IEnvironmentService
 	) {
 		super(backupFileService, filesConfigurationService, workingCopyService, lifecycleService, fileDialogService, dialogService, contextService, nativeHostService, logService, editorService, environmentService);
+	}
 
-		// Reduce timeout for tests
-		BackupTracker.BACKUP_FROM_CONTENT_CHANGE_DELAY = 10;
+	protected getBackupScheduleDelay(): number {
+		return 10; // Reduce timeout for tests
 	}
 }