Support locking cache file when writing and handles syntax error when reading (#21469)

extensions/azurecore/package.json

@@ -364,6 +364,7 @@
     "@azure/storage-blob": "^12.6.0",
     "axios": "^0.27.2",
     "crypto": "^1.0.1",
+    "lockfile": "1.0.4",
     "msal": "^1.4.16",
     "node-fetch": "^2.6.7",
     "qs": "^6.9.1",
@@ -375,6 +376,7 @@
     "@microsoft/azdata-test": "^2.0.3",
     "@microsoft/vscodetestcover": "^1.2.1",
     "@types/keytar": "4.4.0",
+    "@types/lockfile": "^1.0.2",
     "@types/mocha": "^7.0.2",
     "@types/node": "^12.20.55",
     "@types/qs": "^6.9.1",

extensions/azurecore/src/account-provider/utils/msalCachePlugin.ts

@@ -4,42 +4,62 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { ICachePlugin, TokenCacheContext } from '@azure/msal-node';
-import { constants, promises as fsPromises } from 'fs';
+import { promises as fsPromises } from 'fs';
+
+import * as lockFile from 'lockfile';
 import * as path from 'path';
+import { AccountsClearTokenCacheCommand } from '../../constants';
 import { Logger } from '../../utils/Logger';
 
 export class MsalCachePluginProvider {
 	constructor(
 		private readonly _serviceName: string,
-		private readonly _msalFilePath: string
+		private readonly _msalFilePath: string,
 	) {
 		this._msalFilePath = path.join(this._msalFilePath, this._serviceName);
 		this._serviceName = this._serviceName.replace(/-/, '_');
 		Logger.verbose(`MsalCachePluginProvider: Using cache path ${_msalFilePath} and serviceName ${_serviceName}`);
 	}
 
+	private _lockTaken: boolean = false;
+
+	private getLockfilePath(): string {
+		return this._msalFilePath + '.lock';
+	}
+
 	public getCachePlugin(): ICachePlugin {
+		const lockFilePath = this.getLockfilePath();
 		const beforeCacheAccess = async (cacheContext: TokenCacheContext): Promise<void> => {
-			let exists = true;
+			await this.waitAndLock(lockFilePath);
 			try {
-				await fsPromises.access(this._msalFilePath, constants.R_OK | constants.W_OK);
-			} catch {
-				exists = false;
-			}
-			if (exists) {
+				const cache = await fsPromises.readFile(this._msalFilePath, { encoding: 'utf8' });
 				try {
-					const cache = await fsPromises.readFile(this._msalFilePath, { encoding: 'utf8' });
 					cacheContext.tokenCache.deserialize(cache);
-					Logger.verbose(`MsalCachePlugin: Token read from cache successfully.`);
 				} catch (e) {
-					Logger.error(`MsalCachePlugin: Failed to read from cache file. ${e}`);
+					// Handle deserialization error in cache file in case file gets corrupted.
+					// Clearing cache here will ensure account is marked stale so re-authentication can be triggered.
+					Logger.verbose(`MsalCachePlugin: Error occurred when trying to read cache file, file contents will be cleared: ${e.message}`);
+					await fsPromises.writeFile(this._msalFilePath, '', { encoding: 'utf8' });
+				}
+				Logger.verbose(`MsalCachePlugin: Token read from cache successfully.`);
+			} catch (e) {
+				if (e.code === 'ENOENT') {
+					// File doesn't exist, log and continue
+					Logger.verbose(`MsalCachePlugin: Cache file not found on disk: ${e.code}`);
+				}
+				else {
+					Logger.error(`MsalCachePlugin: Failed to read from cache file: ${e}`);
 					throw e;
 				}
+			} finally {
+				lockFile.unlockSync(lockFilePath);
+				this._lockTaken = false;
 			}
-		};
+		}
 
 		const afterCacheAccess = async (cacheContext: TokenCacheContext): Promise<void> => {
 			if (cacheContext.cacheHasChanged) {
+				await this.waitAndLock(lockFilePath);
 				try {
 					const data = cacheContext.tokenCache.serialize();
 					await fsPromises.writeFile(this._msalFilePath, data, { encoding: 'utf8' });
@@ -47,6 +67,9 @@ export class MsalCachePluginProvider {
 				} catch (e) {
 					Logger.error(`MsalCachePlugin: Failed to write to cache file. ${e}`);
 					throw e;
+				} finally {
+					lockFile.unlockSync(lockFilePath);
+					this._lockTaken = false;
 				}
 			}
 		};
@@ -61,4 +84,36 @@ export class MsalCachePluginProvider {
 			afterCacheAccess,
 		};
 	}
+
+	private async waitAndLock(lockFilePath: string): Promise<void> {
+		// Make 500 retry attempts with 100ms wait time between each attempt to allow enough time for the lock to be released.
+		const retries = 500;
+		const retryWait = 100;
+
+		// We cannot rely on lockfile.lockSync() to clear stale lockfile,
+		// so we check if the lockfile exists and if it does, calling unlockSync() will clear it.
+		if (lockFile.checkSync(lockFilePath) && !this._lockTaken) {
+			lockFile.unlockSync(lockFilePath);
+			Logger.verbose(`MsalCachePlugin: Stale lockfile found and has been removed.`);
+		}
+
+		let retryAttempt = 0;
+		while (retryAttempt <= retries) {
+			try {
+				// Use lockfile.lockSync() to ensure only one process is accessing the cache at a time.
+				// lockfile.lock() does not wait for async callback promise to resolve.
+				lockFile.lockSync(lockFilePath);
+				this._lockTaken = true;
+				break;
+			} catch (e) {
+				if (retryAttempt === retries) {
+					Logger.error(`MsalCachePlugin: Failed to acquire lock on cache file after ${retries} attempts.`);
+					throw new Error(`Failed to acquire lock on cache file after ${retries} attempts. Please attempt command: '${AccountsClearTokenCacheCommand}' to clear access token cache.`);
+				}
+				retryAttempt++;
+				Logger.verbose(`MsalCachePlugin: Failed to acquire lock on cache file. Retrying in ${retryWait} ms.`);
+				await new Promise(resolve => setTimeout(resolve, retryWait));
+			}
+		}
+	}
 }

extensions/azurecore/yarn.lock

@@ -413,6 +413,11 @@
   resolved "https://registry.yarnpkg.com/@types/keytar/-/keytar-4.4.0.tgz#ca24e6ee6d0df10c003aafe26e93113b8faf0d8e"
   integrity sha512-cq/NkUUy6rpWD8n7PweNQQBpw2o0cf5v6fbkUVEpOB9VzzIvyPvSEId1/goIj+MciW2v1Lw5mRimKO01XgE9EA==
 
+"@types/lockfile@^1.0.2":
+  version "1.0.2"
+  resolved "https://registry.yarnpkg.com/@types/lockfile/-/lockfile-1.0.2.tgz#3f77e84171a2b7e3198bd5717c7547a54393baf8"
+  integrity sha512-jD5VbvhfMhaYN4M3qPJuhMVUg3Dfc4tvPvLEAXn6GXbs/ajDFtCQahX37GIE65ipTI3I+hEvNaXS3MYAn9Ce3Q==
+
 "@types/mocha@^7.0.2":
   version "7.0.2"
   resolved "https://registry.yarnpkg.com/@types/mocha/-/mocha-7.0.2.tgz#b17f16cf933597e10d6d78eae3251e692ce8b0ce"
@@ -1334,6 +1339,13 @@ locate-path@^3.0.0:
     p-locate "^3.0.0"
     path-exists "^3.0.0"
 
+lockfile@1.0.4:
+  version "1.0.4"
+  resolved "https://registry.yarnpkg.com/lockfile/-/lockfile-1.0.4.tgz#07f819d25ae48f87e538e6578b6964a4981a5609"
+  integrity sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==
+  dependencies:
+    signal-exit "^3.0.2"
+
 lodash.get@^4.4.2:
   version "4.4.2"
   resolved "https://registry.yarnpkg.com/lodash.get/-/lodash.get-4.4.2.tgz#2d177f652fa31e939b4438d5341499dfa3825e99"
@@ -1774,6 +1786,11 @@ side-channel@^1.0.4:
     get-intrinsic "^1.0.2"
     object-inspect "^1.9.0"
 
+signal-exit@^3.0.2:
+  version "3.0.7"
+  resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.7.tgz#a9a1767f8af84155114eaabd73f99273c8f59ad9"
+  integrity sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ==
+
 sinon@^9.0.2:
   version "9.0.2"
   resolved "https://registry.yarnpkg.com/sinon/-/sinon-9.0.2.tgz#b9017e24633f4b1c98dfb6e784a5f0509f5fd85d"