do not crash monitor on unstable fs

Signed-off-by: nkomonen-amazon <[email protected]>

Author: nkomonen-amazon

packages/amazonq/src/extensionNode.ts

@@ -32,7 +32,7 @@ export async function activate(context: vscode.ExtensionContext) {
  * the code compatible with web and move it to {@link activateAmazonQCommon}.
  */
 async function activateAmazonQNode(context: vscode.ExtensionContext) {
-    await (await CrashMonitoring.instance()).start()
+    await (await CrashMonitoring.instance())?.start()
 
     const extContext = {
         extensionContext: context,
@@ -96,5 +96,5 @@ async function setupDevMode(context: vscode.ExtensionContext) {
 
 export async function deactivate() {
     // Run concurrently to speed up execution. stop() does not throw so it is safe
-    await Promise.all([(await CrashMonitoring.instance()).stop(), deactivateCommon()])
+    await Promise.all([(await CrashMonitoring.instance())?.stop(), deactivateCommon()])
 }

packages/core/src/extensionNode.ts

@@ -78,7 +78,7 @@ export async function activate(context: vscode.ExtensionContext) {
         // IMPORTANT: If you are doing setup that should also work in web mode (browser), it should be done in the function below
         const extContext = await activateCommon(context, contextPrefix, false)
 
-        await (await CrashMonitoring.instance()).start()
+        await (await CrashMonitoring.instance())?.start()
 
         initializeCredentialsProviderManager()
 
@@ -254,7 +254,7 @@ export async function activate(context: vscode.ExtensionContext) {
 
 export async function deactivate() {
     // Run concurrently to speed up execution. stop() does not throw so it is safe
-    await Promise.all([await (await CrashMonitoring.instance()).stop(), deactivateCommon()])
+    await Promise.all([await (await CrashMonitoring.instance())?.stop(), deactivateCommon()])
     await globals.resourceManager.dispose()
 }
 

packages/core/src/shared/crashMonitoring.ts

@@ -16,6 +16,7 @@ import nodeFs from 'fs/promises'
 import fs from './fs/fs'
 import { getLogger } from './logger/logger'
 import { crashMonitoringDirNames } from './constants'
+import { throwOnUnstableFileSystem } from './filesystemUtilities'
 
 const className = 'CrashMonitoring'
 
@@ -65,17 +66,32 @@ export class CrashMonitoring {
         private readonly devLogger: Logger | undefined
     ) {}
 
+    static #didTryInstance = false
     static #instance: CrashMonitoring | undefined
-    public static async instance(): Promise<CrashMonitoring> {
-        const isDevMode = getIsDevMode()
-        const devModeLogger: Logger | undefined = isDevMode ? getLogger() : undefined
-        return (this.#instance ??= new CrashMonitoring(
-            await crashMonitoringStateFactory(),
-            DevSettings.instance.get('crashCheckInterval', 1000 * 60 * 3),
-            isDevMode,
-            isAutomation(),
-            devModeLogger
-        ))
+    public static async instance(): Promise<CrashMonitoring | undefined> {
+        // Since the first attempt to create an instance may have failed, we do not
+        // attempt to create an instance again and return whatever we have
+        if (this.#didTryInstance === true) {
+            return this.#instance
+        }
+
+        try {
+            this.#didTryInstance = true
+            const isDevMode = getIsDevMode()
+            const devModeLogger: Logger | undefined = isDevMode ? getLogger() : undefined
+            const state = await crashMonitoringStateFactory()
+            return (this.#instance ??= new CrashMonitoring(
+                state,
+                DevSettings.instance.get('crashCheckInterval', 1000 * 60 * 3),
+                isDevMode,
+                isAutomation(),
+                devModeLogger
+            ))
+        } catch (error) {
+            // Creating the crash monitoring state can throw/fail
+            emitFailure({ functionName: 'instance', error })
+            return undefined
+        }
     }
 
     /** Start the Crash Monitoring process */
@@ -358,6 +374,12 @@ function getDefaultDependencies(): MementoStateDependencies {
         devLogger: getIsDevMode() ? getLogger() : undefined,
     }
 }
+/**
+ * Factory to create an instance of the state. Do not create an instance of the
+ * class manually since there are required setup steps.
+ *
+ * @throws if the filesystem state cannot get in to a good state
+ */
 export async function crashMonitoringStateFactory(deps = getDefaultDependencies()): Promise<FileSystemState> {
     const state: FileSystemState = new FileSystemState(deps)
     await state.init()
@@ -385,8 +407,18 @@ export class FileSystemState {
     /**
      * Does the required initialization steps, this must always be run after
      * creation of the instance.
+     *
+     * @throws if the filesystem state cannot get in to a good state
      */
     public async init() {
+        // IMPORTANT: do not run crash reporting on unstable filesystem to reduce invalid crash data
+        // NOTE: We want to get a diff between clients that succeeded vs failed this check,
+        // so emit a metric to track that
+        await telemetry.function_call.run(async (span) => {
+            span.record({ className, functionName: 'FileSystemStateValidation' })
+            await withFailCtx('validateFileSystemStability', () => throwOnUnstableFileSystem(this.deps.workDirPath))
+        })
+
         // Clear the state if the user did something like a computer restart
         if (await this.deps.isStateStale()) {
             await this.clearState()
@@ -595,7 +627,7 @@ async function withFailCtx<T>(ctx: string, fn: () => Promise<T>): Promise<T> {
         // make sure we await the function so it actually executes within the try/catch
         return await fn()
     } catch (err) {
-        throw CrashMonitoringError.chain(err, `Failed "${ctx}"`, { code: className })
+        throw CrashMonitoringError.chain(err, `Context: "${ctx}"`, { code: className })
     }
 }
 

packages/core/src/shared/filesystemUtilities.ts

@@ -11,6 +11,7 @@ import { getLogger } from './logger'
 import * as pathutils from './utilities/pathUtils'
 import globals from '../shared/extensionGlobals'
 import fs from '../shared/fs/fs'
+import { ToolkitError } from './errors'
 
 export const tempDirPath = path.join(
     // https://github.com/aws/aws-toolkit-vscode/issues/240
@@ -246,3 +247,117 @@ export async function setDefaultDownloadPath(downloadPath: string) {
         getLogger().error('Error while setting "aws.downloadPath"', err as Error)
     }
 }
+
+const FileSystemStabilityExceptionId = 'FileSystemStabilityException'
+const FileSystemStabilityException = ToolkitError.named(FileSystemStabilityExceptionId)
+
+/**
+ * Run this function to validate common file system calls/flows, ensuring they work
+ * as expected.
+ *
+ * The intent of this function is to catch potential fs issues early,
+ * testing common cases of fs usage. We need this since each machine can behave
+ * differently depending on things like OS, permissions, disk speed, ...
+ *
+ * @throws a {@link FileSystemStabilityException} which wraps the underlying failed fs operation error.
+ */
+export async function throwOnUnstableFileSystem(tmpRoot: string) {
+    const tmpFolder = path.join(tmpRoot, `validateStableFS-${crypto.randomBytes(4).toString('hex')}`)
+    const tmpFile = path.join(tmpFolder, 'file.txt')
+
+    try {
+        // test basic folder operations
+        await withFailCtx('mkdirInitial', () => fs.mkdir(tmpFolder))
+        // Verifies that we do not throw if the dir exists and we try to make it again
+        await withFailCtx('mkdirButAlreadyExists', () => fs.mkdir(tmpFolder))
+        await withFailCtx('mkdirSubfolder', () => fs.mkdir(path.join(tmpFolder, 'subfolder')))
+        await withFailCtx('rmdirInitial', () => fs.delete(tmpFolder, { recursive: true }))
+
+        // test basic file operations
+        await withFailCtx('mkdirForFileOpsTest', () => fs.mkdir(tmpFolder))
+        await withFailCtx('writeFile1', () => fs.writeFile(tmpFile, 'test1'))
+        await withFailCtx('readFile1', async () => {
+            const text = await fs.readFileText(tmpFile)
+            if (text !== 'test1') {
+                throw new Error(`Unexpected file contents: "${text}"`)
+            }
+        })
+        // overwrite the file content multiple times
+        await withFailCtx('writeFile2', () => fs.writeFile(tmpFile, 'test2'))
+        await withFailCtx('writeFile3', () => fs.writeFile(tmpFile, 'test3'))
+        await withFailCtx('writeFile4', () => fs.writeFile(tmpFile, 'test4'))
+        await withFailCtx('writeFile5', () => fs.writeFile(tmpFile, 'test5'))
+        await withFailCtx('readFile5', async () => {
+            const text = await fs.readFileText(tmpFile)
+            if (text !== 'test5') {
+                throw new Error(`Unexpected file contents after multiple writes: "${text}"`)
+            }
+        })
+        // test concurrent reads on a file
+        await withFailCtx('writeFileConcurrencyTest', () => fs.writeFile(tmpFile, 'concurrencyTest'))
+        const result = await Promise.all([
+            withFailCtx('readFileConcurrent1', () => fs.readFileText(tmpFile)),
+            withFailCtx('readFileConcurrent2', () => fs.readFileText(tmpFile)),
+            withFailCtx('readFileConcurrent3', () => fs.readFileText(tmpFile)),
+            withFailCtx('readFileConcurrent4', () => fs.readFileText(tmpFile)),
+            withFailCtx('readFileConcurrent5', () => fs.readFileText(tmpFile)),
+        ])
+        if (result.some((text) => text !== 'concurrencyTest')) {
+            throw new Error(`Unexpected concurrent file reads: ${result}`)
+        }
+        // test deleting a file
+        await withFailCtx('deleteFileInitial', () => fs.delete(tmpFile))
+        await withFailCtx('writeFileAfterDelete', () => fs.writeFile(tmpFile, 'afterDelete'))
+        await withFailCtx('readNewFileAfterDelete', async () => {
+            const text = await fs.readFileText(tmpFile)
+            if (text !== 'afterDelete') {
+                throw new Error(`Unexpected file content after writing to deleted file: "${text}"`)
+            }
+        })
+        await withFailCtx('deleteFileFully', () => fs.delete(tmpFile))
+        await withFailCtx('notExistsFileAfterDelete', async () => {
+            const res = await fs.exists(tmpFile)
+            if (res) {
+                throw new Error(`Expected file to NOT exist: "${tmpFile}"`)
+            }
+        })
+
+        // test rename
+        await withFailCtx('writeFileForRename', () => fs.writeFile(tmpFile, 'TestingRename'))
+        const tmpFileRenamed = tmpFile + '.renamed'
+        await withFailCtx('renameFile', () => fs.rename(tmpFile, tmpFileRenamed))
+        await withFailCtx('existsRenamedFile', async () => {
+            const res = await fs.exists(tmpFileRenamed)
+            if (!res) {
+                throw new Error(`Expected RENAMED file to exist: "${tmpFileRenamed}"`)
+            }
+        })
+        await withFailCtx('writeToRenamedFile', async () => fs.writeFile(tmpFileRenamed, 'hello'))
+        await withFailCtx('readFromRenamedFile', async () => {
+            const res = await fs.readFileText(tmpFileRenamed)
+            if (res !== 'hello') {
+                throw new Error(`Expected RENAMED file to be writable: "${tmpFileRenamed}"`)
+            }
+        })
+        await withFailCtx('renameFileReset', () => fs.rename(tmpFileRenamed, tmpFile))
+        await withFailCtx('renameFileResetExists', async () => {
+            const res = await fs.exists(tmpFile)
+            if (!res) {
+                throw new Error(`Expected reverted renamed file to exist: "${tmpFile}"`)
+            }
+        })
+    } finally {
+        await fs.delete(tmpFolder, { recursive: true, force: true })
+    }
+
+    async function withFailCtx<T>(ctx: string, fn: () => Promise<T>): Promise<T> {
+        try {
+            return await fn()
+        } catch (e) {
+            if (!(e instanceof Error)) {
+                throw e
+            }
+            throw FileSystemStabilityException.chain(e, `context: "${ctx}"`, { code: FileSystemStabilityExceptionId })
+        }
+    }
+}

packages/core/src/test/shared/filesystemUtilities.test.ts

@@ -13,8 +13,10 @@ import {
     isInDirectory,
     makeTemporaryToolkitFolder,
     tempDirPath,
+    throwOnUnstableFileSystem,
 } from '../../shared/filesystemUtilities'
 import { fs } from '../../shared'
+import { TestFolder } from '../testUtil'
 
 describe('filesystemUtilities', function () {
     const targetFilename = 'findThisFile12345.txt'
@@ -191,4 +193,11 @@ describe('filesystemUtilities', function () {
             assert.strictEqual(actual, 3)
         })
     })
+
+    describe('throwOnUnstableFileSystem', async function () {
+        it('does not throw on stable filesystem', async function () {
+            const testFolder = await TestFolder.create()
+            await throwOnUnstableFileSystem(testFolder.path)
+        })
+    })
 })

packages/core/src/test/testUtil.ts

@@ -75,7 +75,7 @@ export function getWorkspaceFolder(dir: string): vscode.WorkspaceFolder {
  * But if the day comes that we need it for web, we should be able to add some agnostic FS methods in here.
  */
 export class TestFolder {
-    protected constructor(private readonly rootFolder: string) {}
+    protected constructor(public readonly path: string) {}
 
     /** Creates a folder that deletes itself once all tests are done running. */
     static async create() {
@@ -118,10 +118,6 @@ export class TestFolder {
     pathFrom(relativePath: string): string {
         return path.join(this.path, relativePath)
     }
-
-    get path(): string {
-        return path.join(this.rootFolder)
-    }
 }
 
 /**