storage - do not backup if we detect a corrupted DB during runtime

src/vs/base/node/storage.ts

@@ -325,6 +325,8 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 	private name: string;
 	private logger: SQLiteStorageDatabaseLogger;
 
+	private isCorrupt: boolean;
+
 	private whenOpened: Promise<IOpenDatabaseResult>;
 
 	constructor(path: string, options: ISQLiteStorageDatabaseOptions = Object.create(null)) {
@@ -411,15 +413,16 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 			return new Promise((resolve, reject) => {
 				result.db.close(error => {
 					if (error) {
-						this.logger.error(`[storage ${this.name}] close(): ${error}`);
+						this.handleSQLiteError(error, `[storage ${this.name}] close(): ${error}`);
 
 						return reject(error);
 					}
 
 					// If the DB closed successfully and we are not running in-memory
-					// make a backup of the DB so that we can use it as fallback in
-					// case the actual DB becomes corrupt.
-					if (result.path !== SQLiteStorageDatabase.IN_MEMORY_PATH) {
+					// and the DB did not get corrupted during runtime, make a backup
+					// of the DB so that we can use it as fallback in case the actual
+					// DB becomes corrupt.
+					if (result.path !== SQLiteStorageDatabase.IN_MEMORY_PATH && !this.isCorrupt) {
 						return this.backup(result).then(resolve, error => {
 							this.logger.error(`[storage ${this.name}] backup(): ${error}`);
 
@@ -462,7 +465,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 
 		return new Promise((resolve, reject) => {
 			const fallbackToInMemoryDatabase = (error: Error) => {
-				this.logger.error(`[storage ${this.name}] open(): Error (open DB): ${error}. Falling back to in-memory DB`);
+				this.handleSQLiteError(error, `[storage ${this.name}] open(): Error (open DB): ${error}. Falling back to in-memory DB`);
 
 				// In case of any error to open the DB, use an in-memory
 				// DB so that we always have a valid DB to talk to.
@@ -477,7 +480,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 				// that the previous connection was not properly closed while the new connection is
 				// already established.
 				if (error.code === 'SQLITE_BUSY') {
-					return this.handleSQLiteBusy(path).then(resolve, fallbackToInMemoryDatabase);
+					return this.handleSQLiteBusy(path, error).then(resolve, fallbackToInMemoryDatabase);
 				}
 
 				// This error code indicates that even though the DB file exists,
@@ -492,15 +495,15 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		});
 	}
 
-	private handleSQLiteBusy(path: string): Promise<IOpenDatabaseResult> {
-		this.logger.error(`[storage ${this.name}] open(): Retrying after ${SQLiteStorageDatabase.BUSY_OPEN_TIMEOUT}ms due to SQLITE_BUSY`);
+	private handleSQLiteBusy(path: string, error: Error & { code?: string }): Promise<IOpenDatabaseResult> {
+		this.handleSQLiteError(error, `[storage ${this.name}] open(): Retrying after ${SQLiteStorageDatabase.BUSY_OPEN_TIMEOUT}ms due to SQLITE_BUSY`);
 
 		// Retry after some time if the DB is busy
 		return timeout(SQLiteStorageDatabase.BUSY_OPEN_TIMEOUT).then(() => this.doOpen(path));
 	}
 
-	private handleSQLiteCorrupt(path: string, error: any): Promise<IOpenDatabaseResult> {
-		this.logger.error(`[storage ${this.name}] open(): Unable to open DB due to ${error.code}`);
+	private handleSQLiteCorrupt(path: string, error: Error & { code?: string }): Promise<IOpenDatabaseResult> {
+		this.handleSQLiteError(error, `[storage ${this.name}] open(): Unable to open DB due to ${error.code}`);
 
 		// Move corrupt DB to a different filename and try to load from backup
 		// If that fails, a new empty DB is being created automatically
@@ -516,6 +519,10 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 	}
 
 	private doOpen(path: string): Promise<IOpenDatabaseResult> {
+
+		// Reset flags when we open a DB
+		this.isCorrupt = false;
+
 		// TODO@Ben clean up performance markers
 		return new Promise((resolve, reject) => {
 			let measureRequireDuration = false;
@@ -554,7 +561,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 				});
 
 				// Errors
-				db.on('error', error => this.logger.error(`[storage ${this.name}] Error (event): ${error}`));
+				db.on('error', error => this.handleSQLiteError(error, `[storage ${this.name}] Error (event): ${error}`));
 
 				// Tracing
 				if (this.logger.isTracing) {
@@ -568,7 +575,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		return new Promise((resolve, reject) => {
 			db.exec(sql, error => {
 				if (error) {
-					this.logger.error(`[storage ${this.name}] exec(): ${error}`);
+					this.handleSQLiteError(error, `[storage ${this.name}] exec(): ${error}`);
 
 					return reject(error);
 				}
@@ -582,7 +589,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		return new Promise((resolve, reject) => {
 			db.get(sql, (error, row) => {
 				if (error) {
-					this.logger.error(`[storage ${this.name}] get(): ${error}`);
+					this.handleSQLiteError(error, `[storage ${this.name}] get(): ${error}`);
 
 					return reject(error);
 				}
@@ -596,7 +603,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		return new Promise((resolve, reject) => {
 			db.all(sql, (error, rows) => {
 				if (error) {
-					this.logger.error(`[storage ${this.name}] all(): ${error}`);
+					this.handleSQLiteError(error, `[storage ${this.name}] all(): ${error}`);
 
 					return reject(error);
 				}
@@ -615,7 +622,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 
 				db.run('END TRANSACTION', error => {
 					if (error) {
-						this.logger.error(`[storage ${this.name}] transaction(): ${error}`);
+						this.handleSQLiteError(error, `[storage ${this.name}] transaction(): ${error}`);
 
 						return reject(error);
 					}
@@ -630,7 +637,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		const stmt = db.prepare(sql);
 
 		const statementErrorListener = error => {
-			this.logger.error(`[storage ${this.name}] prepare(): ${error} (${sql}). Details: ${errorDetails()}`);
+			this.handleSQLiteError(error, `[storage ${this.name}] prepare(): ${error} (${sql}). Details: ${errorDetails()}`);
 		};
 
 		stmt.on('error', statementErrorListener);
@@ -645,6 +652,14 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 			stmt.removeListener('error', statementErrorListener);
 		});
 	}
+
+	private handleSQLiteError(error: Error & { code?: string }, msg: string): void {
+		if (error.code === 'SQLITE_CORRUPT' || error.code === 'SQLITE_NOTADB') {
+			this.isCorrupt = true;
+		}
+
+		this.logger.error(msg);
+	}
 }
 
 class SQLiteStorageDatabaseLogger {

src/vs/base/test/node/storage/storage.test.ts

@@ -11,6 +11,7 @@ import { equal, ok } from 'assert';
 import { mkdirp, del, writeFile } from 'vs/base/node/pfs';
 import { timeout } from 'vs/base/common/async';
 import { Event, Emitter } from 'vs/base/common/event';
+import { isWindows } from 'vs/base/common/platform';
 
 suite('Storage Library', () => {
 
@@ -426,6 +427,54 @@ suite('SQLite Storage Library', () => {
 		await del(storageDir, tmpdir());
 	});
 
+	test('basics (corrupt DB does not backup)', async () => {
+		if (isWindows) {
+			await Promise.resolve(); // Windows will fail to write to open DB due to locking
+
+			return;
+		}
+
+		const storageDir = uniqueStorageDir();
+
+		await mkdirp(storageDir);
+
+		const storagePath = join(storageDir, 'storage.db');
+		let storage = new SQLiteStorageDatabase(storagePath);
+
+		const items = new Map<string, string>();
+		items.set('foo', 'bar');
+		items.set('some/foo/path', 'some/bar/path');
+		items.set(JSON.stringify({ foo: 'bar' }), JSON.stringify({ bar: 'foo' }));
+
+		await storage.updateItems({ insert: items });
+		await storage.close();
+
+		storage = new SQLiteStorageDatabase(storagePath);
+		await storage.getItems();
+
+		await writeFile(storagePath, 'This is now a broken DB');
+
+		// we still need to trigger a check to the DB so that we get to know that
+		// the DB is corrupt. We have no extra code on shutdown that checks for the
+		// health of the DB. This is an optimization to not perform too many tasks
+		// on shutdown.
+		await storage.checkIntegrity(true).then(null, error => { } /* error is expected here but we do not want to fail */);
+
+		await storage.close();
+
+		storage = new SQLiteStorageDatabase(storagePath);
+
+		const storedItems = await storage.getItems();
+		equal(storedItems.size, items.size);
+		equal(storedItems.get('foo'), 'bar');
+		equal(storedItems.get('some/foo/path'), 'some/bar/path');
+		equal(storedItems.get(JSON.stringify({ foo: 'bar' })), JSON.stringify({ bar: 'foo' }));
+
+		await storage.close();
+
+		await del(storageDir, tmpdir());
+	});
+
 	test('real world example', async () => {
 		const storageDir = uniqueStorageDir();