various fixes

- removed some unnecessary code
- fix some comments
- if heartbeats fails, stop sending future heartbeats, and remove the existing one from the state so it cannot
  be seen as a crash.
- increase the retries from 12 seconds total to 25 seconds total (1 extra interval)
- switch a vscode fs mkdir call to the node one since it looks like it may be flaky based on telemetry data
- retry when trying to load the extension state from disk since it seems to fail sometimes due to temporary issues

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

Author: nkomonen-amazon

packages/core/src/shared/crashMonitoring.ts

@@ -54,8 +54,6 @@ const className = 'CrashMonitoring'
  *   - We cannot persist crash information on computer restart
  */
 export class CrashMonitoring {
-    private isStarted: boolean = false
-
     protected heartbeat: Heartbeat | undefined
     protected crashChecker: CrashChecker | undefined
 
@@ -67,20 +65,21 @@ export class CrashMonitoring {
         private readonly devLogger: Logger | undefined
     ) {}
 
-    static #didTryInstance = false
+    static #didTryCreate = false
     static #instance: CrashMonitoring | undefined
+    /** Returns an instance of this class or undefined if any initial validation fails. */
     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) {
+        if (this.#didTryCreate === true) {
             return this.#instance
         }
 
         try {
-            this.#didTryInstance = true
+            this.#didTryCreate = true
             const isDevMode = getIsDevMode()
             const devModeLogger: Logger | undefined = isDevMode ? getLogger() : undefined
-            const state = await crashMonitoringStateFactory()
+            const state = await crashMonitoringStateFactory() // can throw
             return (this.#instance ??= new CrashMonitoring(
                 state,
                 DevSettings.instance.get('crashCheckInterval', 1000 * 60 * 10), // check every 10 minutes
@@ -89,7 +88,6 @@ export class CrashMonitoring {
                 devModeLogger
             ))
         } catch (error) {
-            // Creating the crash monitoring state can throw/fail
             emitFailure({ functionName: 'instance', error })
             return undefined
         }
@@ -113,10 +111,10 @@ export class CrashMonitoring {
 
             await this.heartbeat.start()
             await this.crashChecker.start()
-
-            this.isStarted = true
         } catch (error) {
             emitFailure({ functionName: 'start', error })
+            this.cleanup()
+
             // In development this gives us a useful stacktrace
             if (this.isDevMode) {
                 throw error
@@ -126,10 +124,6 @@ export class CrashMonitoring {
 
     /** Stop the Crash Monitoring process, signifying a graceful shutdown */
     public async stop() {
-        if (!this.isStarted) {
-            return
-        }
-
         // Dont throw since this feature is not critical and shouldn't prevent extension shutdown
         try {
             this.crashChecker?.stop()
@@ -151,13 +145,9 @@ export class CrashMonitoring {
      * Mimic a crash of the extension, or can just be used as cleanup.
      * Only use this for tests.
      */
-    protected crash() {
-        if (!this.isStarted) {
-            return
-        }
-
+    protected cleanup() {
         this.crashChecker?.stop()
-        this.heartbeat?.crash()
+        this.heartbeat?.stopInterval()
     }
 }
 
@@ -166,8 +156,6 @@ export class CrashMonitoring {
  * {@link CrashChecker} listens for these.
  */
 class Heartbeat {
-    private didEmitFailure: boolean = false
-    private isRunning: boolean = false
     private intervalRef: NodeJS.Timer | undefined
     constructor(
         private readonly state: FileSystemState,
@@ -187,33 +175,25 @@ class Heartbeat {
             try {
                 await this.state.sendHeartbeat()
             } catch (e) {
-                // only emit a failure once so we do not spam telemetry on repeated errors
-                if (!this.didEmitFailure) {
-                    this.didEmitFailure = true
-                    emitFailure({ functionName: 'sendHeartbeat', error: e })
-                }
+                this.stopInterval()
+                emitFailure({ functionName: 'sendHeartbeat', error: e })
 
                 // During development we are fine with impacting extension execution, so throw
                 if (this.isDevMode) {
                     throw e
                 }
             }
         }, heartbeatInterval)
-
-        this.isRunning = true
     }
 
+    /** Stops everything, signifying a graceful shutdown */
     public async stop() {
-        // non-happy path where heartbeats were never started.
-        if (!this.isRunning) {
-            return
-        }
-
-        globals.clock.clearInterval(this.intervalRef)
+        this.stopInterval()
         return this.state.indicateGracefulShutdown()
     }
 
-    public crash() {
+    /** Stops the heartbeat interval */
+    public stopInterval() {
         globals.clock.clearInterval(this.intervalRef)
     }
 }
@@ -258,7 +238,7 @@ class CrashChecker {
                 } catch (e) {
                     emitFailure({ functionName: 'checkCrashInterval', error: e })
 
-                    // Since there was an error we want to stop crash monitoring since it is pointless.
+                    // Since there was an error there is point to try checking for crashed instances.
                     // We will need to monitor telemetry to see if we can determine widespread issues.
                     this.stop()
 
@@ -379,7 +359,7 @@ function getDefaultDependencies(): MementoStateDependencies {
  * 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
+ * @throws if the filesystem state cannot be confirmed to be stable, i.e flaky fs operations
  */
 export async function crashMonitoringStateFactory(deps = getDefaultDependencies()): Promise<FileSystemState> {
     const state: FileSystemState = new FileSystemState(deps)
@@ -409,15 +389,15 @@ 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
+     * @throws if the filesystem state cannot be confirmed to be stable, i.e flaky fs operations
      */
     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))
+            await withFailCtx('validateFileSystemStability', () => throwOnUnstableFileSystem())
         })
 
         // Clear the state if the user did something like a computer restart
@@ -428,21 +408,29 @@ export class FileSystemState {
 
     // ------------------ Heartbeat methods ------------------
     public async sendHeartbeat() {
-        const _sendHeartbeat = () =>
-            withFailCtx('sendHeartbeatState', async () => {
+        const extId = this.createExtId(this.ext)
+
+        try {
+            const func = async () => {
                 const dir = await this.runningExtsDir()
-                const extId = this.createExtId(this.ext)
                 await fs.writeFile(
                     path.join(dir, extId),
                     JSON.stringify({ ...this.ext, lastHeartbeat: this.deps.now() }, undefined, 4)
                 )
                 this.deps.devLogger?.debug(
                     `crashMonitoring: HEARTBEAT pid ${this.deps.pid} + sessionId: ${this.deps.sessionId.slice(0, 8)}-...`
                 )
-            })
-        // retry a failed heartbeat to avoid incorrectly being reported as a crash
-        const _sendHeartbeatWithRetries = withRetries(_sendHeartbeat, { maxRetries: 7, delay: 100, backoff: 2 })
-        return _sendHeartbeatWithRetries
+            }
+            const funcWithCtx = () => withFailCtx('sendHeartbeatState', func)
+            const funcWithRetries = withRetries(funcWithCtx, { maxRetries: 8, delay: 100, backoff: 2 })
+
+            // retry a failed heartbeat to avoid incorrectly being reported as a crash
+            return await funcWithRetries
+        } catch (e) {
+            // delete this ext from the state to avoid an incorrectly reported crash since we could not send a new heartbeat
+            await withFailCtx('sendHeartbeatStateCleanup', () => this.deleteRunningFile(extId))
+            throw e
+        }
     }
 
     /**
@@ -488,13 +476,17 @@ export class FileSystemState {
             await opts.crash()
         }
 
+        // Clean up the running extension file since it no longer exists
+        await this.deleteRunningFile(extId)
+    }
+    private async deleteRunningFile(extId: ExtInstanceId) {
         // Clean up the running extension file since it is no longer exists
         const dir = await this.runningExtsDir()
         // Retry on failure since failing to delete this file will result in incorrectly reported crashes.
         // The common errors we were seeing were windows EPERM/EBUSY errors. There may be a relation
         // to this https://github.com/aws/aws-toolkit-vscode/pull/5335
         await withRetries(() => withFailCtx('deleteStaleRunningFile', () => fs.delete(path.join(dir, extId))), {
-            maxRetries: 7,
+            maxRetries: 8,
             delay: 100,
             backoff: 2,
         })
@@ -525,7 +517,7 @@ export class FileSystemState {
     private async runningExtsDir(): Promise<string> {
         const p = path.join(this.stateDirPath, crashMonitoringDirNames.running)
         // ensure the dir exists
-        await withFailCtx('ensureRunningExtsDir', () => fs.mkdir(p))
+        await withFailCtx('ensureRunningExtsDir', () => nodeFs.mkdir(p, { recursive: true }))
         return p
     }
     private async shutdownExtsDir() {
@@ -551,17 +543,22 @@ export class FileSystemState {
             const allExts = allExtIds.map<Promise<ExtInstanceHeartbeat | undefined>>(async (extId: string) => {
                 // Due to a race condition, a separate extension instance may have removed this file by this point. It is okay since
                 // we will assume that other instance handled its termination appropriately.
-                const ext = await withFailCtx('parseRunningExtFile', async () =>
-                    ignoreBadFileError(async () => {
-                        const text = await fs.readFileText(path.join(await this.runningExtsDir(), extId))
-
-                        if (!text) {
-                            return undefined
-                        }
-
-                        // This was sometimes throwing SyntaxError
-                        return JSON.parse(text) as ExtInstanceHeartbeat
-                    })
+                // NOTE: On Windows we were failing on EBUSY, so we retry on failure.
+                const ext: ExtInstanceHeartbeat | undefined = await withRetries(
+                    () =>
+                        withFailCtx('parseRunningExtFile', async () =>
+                            ignoreBadFileError(async () => {
+                                const text = await fs.readFileText(path.join(await this.runningExtsDir(), extId))
+
+                                if (!text) {
+                                    return undefined
+                                }
+
+                                // This was sometimes throwing SyntaxError
+                                return JSON.parse(text) as ExtInstanceHeartbeat
+                            })
+                        ),
+                    { maxRetries: 6, delay: 100, backoff: 2 }
                 )
 
                 if (ext === undefined) {

packages/core/src/shared/filesystemUtilities.ts

@@ -12,6 +12,7 @@ import * as pathutils from './utilities/pathUtils'
 import globals from '../shared/extensionGlobals'
 import fs from '../shared/fs/fs'
 import { ToolkitError } from './errors'
+import * as nodeFs from 'fs/promises'
 
 export const tempDirPath = path.join(
     // https://github.com/aws/aws-toolkit-vscode/issues/240
@@ -261,7 +262,7 @@ const FileSystemStabilityException = ToolkitError.named(FileSystemStabilityExcep
  *
  * @throws a {@link FileSystemStabilityException} which wraps the underlying failed fs operation error.
  */
-export async function throwOnUnstableFileSystem(tmpRoot: string) {
+export async function throwOnUnstableFileSystem(tmpRoot: string = tempDirPath) {
     const tmpFolder = path.join(tmpRoot, `validateStableFS-${crypto.randomBytes(4).toString('hex')}`)
     const tmpFile = path.join(tmpFolder, 'file.txt')
 
@@ -270,8 +271,14 @@ export async function throwOnUnstableFileSystem(tmpRoot: string) {
         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 subfolder creation. Based on telemetry it looks like the vsc mkdir may be flaky
+        // when creating subfolders. Hopefully this gives us some useful information.
+        const subfolderRoot = path.join(tmpFolder, 'a')
+        const subfolderPath = path.join(subfolderRoot, 'b/c/d/e')
+        await withFailCtx('mkdirSubfolderNode', () => nodeFs.mkdir(subfolderPath, { recursive: true }))
+        await withFailCtx('rmdirInitialForNode', () => fs.delete(subfolderRoot, { recursive: true }))
+        await withFailCtx('mkdirSubfolderVsc', () => fs.mkdir(subfolderPath))
+        await withFailCtx('rmdirInitialForVsc', () => fs.delete(subfolderRoot, { recursive: true }))
 
         // test basic file operations
         await withFailCtx('mkdirForFileOpsTest', () => fs.mkdir(tmpFolder))
@@ -293,6 +300,8 @@ export async function throwOnUnstableFileSystem(tmpRoot: string) {
                 throw new Error(`Unexpected file contents after multiple writes: "${text}"`)
             }
         })
+        // write a large file, ensuring we are not near a space limit
+        await withFailCtx('writeFileLarge', () => fs.writeFile(tmpFile, 'a'.repeat(1000)))
         // test concurrent reads on a file
         await withFailCtx('writeFileConcurrencyTest', () => fs.writeFile(tmpFile, 'concurrencyTest'))
         const result = await Promise.all([

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

@@ -14,8 +14,9 @@ class TestCrashMonitoring extends CrashMonitoring {
     public constructor(...deps: ConstructorParameters<typeof CrashMonitoring>) {
         super(...deps)
     }
-    public override crash() {
-        super.crash()
+    /** Immitates an extension crash */
+    public crash() {
+        super.cleanup()
     }
 }