refactor(auth): redo DiskCacheError with proper context (#5558)

## Problem:

The original implementation of DiskCacheError can be simplified and
still achieve it's original purpose.

## Solution:

During token refresh we eventually write to the disk cache. Sometimes
this process fails due to filesystem errors.

We dont need to care about the actual file system error, but only that
the error came during a DiskCache operation (i.e when it attempts to
save the token to disk)

So instead, at the root of where the disk cache errors ocurr, we will
wrap caught errors in a DiskCacheError.
This adds context to the error and allows anything upstream to
specifically check for that context, instead of needing to parse the
error to see if it was filesystem related.

Additionally not all file system errors were guaranteed to come from the
disk cache, so this change allows us to target the right errors.

Signed-off-by: Nikolas Komonen <[email protected]>

---

<!--- REMINDER: Ensure that your PR meets the guidelines in
CONTRIBUTING.md -->

License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

Signed-off-by: Nikolas Komonen <[email protected]>

Author: nkomonen-amazon

packages/core/src/auth/auth.ts

@@ -13,7 +13,7 @@ import * as localizedText from '../shared/localizedText'
 import { Credentials } from '@aws-sdk/types'
 import { SsoAccessTokenProvider } from './sso/ssoAccessTokenProvider'
 import { Timeout } from '../shared/utilities/timeoutUtils'
-import { DiskCacheError, errorCode, isAwsError, isNetworkError, ToolkitError, UnknownError } from '../shared/errors'
+import { errorCode, isAwsError, isNetworkError, ToolkitError, UnknownError } from '../shared/errors'
 import { getCache } from './sso/cache'
 import { isNonNullable, Mutable } from '../shared/utilities/tsUtils'
 import { builderIdStartUrl, SsoToken, truncateStartUrl } from './sso/model'
@@ -64,6 +64,7 @@ import { telemetry } from '../shared/telemetry/telemetry'
 import { randomUUID } from '../shared/crypto'
 import { asStringifiedStack } from '../shared/telemetry/spans'
 import { withTelemetryContext } from '../shared/telemetry/util'
+import { DiskCacheError } from '../shared/utilities/cacheUtils'
 
 interface AuthService {
     /**
@@ -854,11 +855,10 @@ export class Auth implements AuthService, ConnectionManager {
             })
         }
 
-        const possibleCacheError = DiskCacheError.instanceIf(e)
-        if (possibleCacheError instanceof DiskCacheError) {
+        if (e instanceof DiskCacheError) {
             throw new ToolkitError('Failed to update connection due to file system operation failures', {
-                cause: possibleCacheError,
-                code: possibleCacheError.code,
+                cause: e,
+                code: e.code,
             })
         }
     }

packages/core/src/auth/sso/ssoAccessTokenProvider.ts

@@ -18,9 +18,8 @@ import {
 import { getCache } from './cache'
 import { hasProps, hasStringProps, RequiredProps, selectFrom } from '../../shared/utilities/tsUtils'
 import { OidcClient } from './clients'
-import { loadOr } from '../../shared/utilities/cacheUtils'
+import { DiskCacheError, loadOr } from '../../shared/utilities/cacheUtils'
 import {
-    DiskCacheError,
     ToolkitError,
     getErrorMsg,
     getRequestId,
@@ -192,26 +191,21 @@ export abstract class SsoAccessTokenProvider {
 
             return refreshed
         } catch (err) {
-            const possibleCacheError = DiskCacheError.instanceIf(err)
-            if (possibleCacheError instanceof DiskCacheError) {
+            if (err instanceof DiskCacheError) {
                 /**
                  * Background:
-                 * - During token refresh when saving to our filesystem/cache there are sometimes file system
-                 *   related errors.
+                 * - During token refresh the cache sometimes fails due to a file system error.
                  * - When these errors ocurr it will cause the token refresh process to fail, and the users SSO
                  *   connection to become invalid.
-                 * - Because these filesystem errors do not indicate the SSO session is actually stale,
-                 *   we want to catch these filesystem errors and not invalidate the users SSO connection since a
+                 * - Because these cache errors do not indicate the SSO session is actually stale,
+                 *   we want to catch these errors and not invalidate the users SSO connection since a
                  *   subsequent attempt to refresh may succeed.
                  * - To give the user a chance to resolve their filesystem related issue, we want to point them
                  *   to the logs where the error was logged. Hopefully they can use this information to fix the issue,
                  *   or at least hint for them to provide the logs in a bug report.
                  */
-                void DiskCacheErrorMessage.instance.showMessageThrottled(possibleCacheError)
-                throw possibleCacheError
-            }
-
-            if (!isNetworkError(err)) {
+                void DiskCacheErrorMessage.instance.showMessageThrottled(err)
+            } else if (!isNetworkError(err)) {
                 const reason = getTelemetryReason(err)
                 telemetry.aws_refreshCredentials.emit({
                     result: getTelemetryResult(err),
@@ -786,13 +780,13 @@ type ReAuthStateValue = {
 }
 
 /**
- * Singleton class that manages showing the user a message during
- * failures that ocurr during SSO disk cache operations.
+ * Singleton class that manages showing the user a message during {@link DiskCacheError} errors.
  *
  * Background:
  * - We need this {@link DiskCacheErrorMessage} specifically as a singleton since we want to ensure
- *   that only 1 instance of this message appears at a time. There are cases where it shows multiple messages
- *   if not used as a singleton.
+ *   that only 1 instance of this message appears at a time. The current implementation creates a new
+ *   {@link SsoAccessTokenProvider} instance each time a token is requested, and this can happen multiple
+ *   times in rapid succession.
  */
 class DiskCacheErrorMessage {
     static #instance: DiskCacheErrorMessage
@@ -803,6 +797,8 @@ class DiskCacheErrorMessage {
     /**
      * Show a `"don't show again"`-able message which tells the user about a file system related error
      * with the sso cache.
+     *
+     * This message is throttled so we do not spam the user every time something requests a token.
      */
     public showMessageThrottled(error: Error) {
         return this._showMessageThrottled(error)
@@ -827,7 +823,7 @@ class DiskCacheErrorMessage {
 
         function showMessage() {
             return showViewLogsMessage(
-                `File system related error during SSO cache operation:\n"${getErrorMsg(error, true)}"`,
+                `Features using SSO will not work due to:\n"${getErrorMsg(error, true)}"`,
                 'error',
                 [dontShow]
             )

packages/core/src/shared/errors.ts

@@ -1003,65 +1003,6 @@ export class AwsClientResponseError extends Error {
     }
 }
 
-/**
- * Represents a generalized error that happened during a disk cache operation.
- *
- * For example, when SSO refreshes a token a disk cache error can occur when it
- * attempts to read/write the disk cache. These errors can be recoverable and do not
- * imply that the SSO session is stale. So by wrapping those errors in an instance of
- * this class it will help to distinguish them.
- */
-export class DiskCacheError extends Error {
-    #code: string | undefined
-
-    /** Use {@link DiskCacheError.instanceIf()} to create an instance */
-    protected constructor(message: string, options?: { code?: string }) {
-        super(message)
-        this.#code = options?.code
-    }
-
-    /**
-     * We are seeing these errors in telemetry, but they have no error message attached.
-     * The best we can do is assume these are filesystem errors in the context that we see them.
-     */
-    private static fileSystemErrorsWithoutMessage = ['EACCES', 'EBADF']
-
-    /**
-     * Return an instance of {@link DiskCacheError} that consists of the properties from the
-     * given error IF certain conditions are met. Otherwise, the original error is returned.
-     *
-     * - Also returns the original error if it is already a {@link DiskCacheError}.
-     */
-    public static instanceIf<T>(err: T): DiskCacheError | T {
-        if (!(err instanceof Error) || err instanceof DiskCacheError) {
-            return err
-        }
-
-        const errorId = (err as any).code ?? err.name
-
-        if (DiskCacheError.fileSystemErrorsWithoutMessage.includes(errorId)) {
-            // The error message will probably be blank
-            const message = err.message ? err.message : 'No msg'
-            return new DiskCacheError(message, { code: errorId })
-        }
-
-        // These are errors we were seeing in telemetry
-        if (
-            isError(err, 'ENOSPC', 'no space left on device') ||
-            isError(err, 'EPERM', 'operation not permitted') ||
-            isError(err, 'EBUSY', 'resource busy or locked')
-        ) {
-            return new DiskCacheError(err.message, { code: errorId })
-        }
-
-        return err // the error does no meet the conditions to be a DiskCacheError
-    }
-
-    public get code() {
-        return this.#code
-    }
-}
-
 /**
  * Run a function and swallow any errors that are not specified by `shouldThrow`
  */

packages/core/src/shared/utilities/cacheUtils.ts

@@ -5,7 +5,7 @@
 
 import * as vscode from 'vscode'
 import { dirname } from 'path'
-import { ToolkitError, isFileNotFoundError } from '../errors'
+import { ErrorInformation, ToolkitError, isFileNotFoundError } from '../errors'
 import fs from '../../shared/fs/fs'
 import { isWeb } from '../extensionGlobals'
 import type { MapSync } from './map'
@@ -121,10 +121,7 @@ export function createDiskCache<V, K>(
                     return
                 }
                 log(`read failed ${error}`, key)
-                throw ToolkitError.chain(error, `Failed to read from "${target}"`, {
-                    code: 'FSReadFailed',
-                    details: { key },
-                })
+                throw createDiskCacheError(error, 'LOAD', target, key)
             }
         },
         save: async (key, data) => {
@@ -142,10 +139,7 @@ export function createDiskCache<V, K>(
                     await fs.writeFile(target, JSON.stringify(data), { mode: 0o600, atomic: true })
                 }
             } catch (error) {
-                throw ToolkitError.chain(error, `Failed to save "${target}"`, {
-                    code: 'FSWriteFailed',
-                    details: { key },
-                })
+                throw createDiskCacheError(error, 'SAVE', target, key)
             }
 
             log('saved', key)
@@ -160,15 +154,34 @@ export function createDiskCache<V, K>(
                     return log('file not found', key)
                 }
 
-                throw ToolkitError.chain(error, `Failed to delete "${target}"`, {
-                    code: 'FSDeleteFailed',
-                    details: { key },
-                })
+                throw createDiskCacheError(error, 'CLEAR', target, key)
             }
 
             log(`deleted (reason: ${reason})`, key)
         },
     }
+
+    /** Helper to make a disk cache error */
+    function createDiskCacheError(error: unknown, operation: 'LOAD' | 'SAVE' | 'CLEAR', target: string, key: K) {
+        return DiskCacheError.chain(error, `${operation} failed for '${target}'`, {
+            details: { key },
+        })
+    }
+}
+
+/**
+ * Represents a generalized error that happened during a disk cache operation.
+ *
+ * For example, when SSO refreshes a token a disk cache error can occur when it
+ * attempts to read/write the disk cache. These errors can be recoverable and do not
+ * imply that the SSO session is stale. So by creating a context specific instance it
+ * will help to distinguish them when we need to decide if the SSO session is actually
+ * stale.
+ */
+export class DiskCacheError extends ToolkitError.named('DiskCacheError') {
+    public constructor(message: string, info?: Omit<ErrorInformation, 'code'>) {
+        super(message, { ...info, code: 'DiskCacheError' })
+    }
 }
 
 export function createSecretsCache(

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

@@ -19,7 +19,6 @@ import {
     ToolkitError,
     tryRun,
     UnknownError,
-    DiskCacheError,
     getErrorId,
 } from '../../shared/errors'
 import { CancellationError } from '../../shared/utilities/timeoutUtils'
@@ -28,6 +27,7 @@ import { AWSError } from 'aws-sdk'
 import { AccessDeniedException } from '@aws-sdk/client-sso-oidc'
 import { OidcClient } from '../../auth/sso/clients'
 import { SamCliError } from '../../shared/sam/cli/samCliInvokerUtils'
+import { DiskCacheError } from '../../shared/utilities/cacheUtils'
 
 class TestAwsError extends Error implements AWSError {
     constructor(
@@ -258,7 +258,7 @@ describe('ToolkitError', function () {
         it('maintains the prototype chain with a constructor', function () {
             const OopsieError = class extends ToolkitError.named('OopsieError') {
                 public constructor() {
-                    super('oopsies')
+                    super('oopsies', { code: 'OopsieErrorCode' })
                 }
             }
 
@@ -267,6 +267,7 @@ describe('ToolkitError', function () {
             assert.ok(error instanceof OopsieError)
             assert.strictEqual(error.cause, undefined)
             assert.strictEqual(error.message, 'oopsies')
+            assert.strictEqual(error.code, 'OopsieErrorCode')
         })
 
         it('maintains the prototype chain without a constructor', function () {
@@ -284,6 +285,15 @@ describe('ToolkitError', function () {
             assert.strictEqual(error.cause?.message, 'oops')
         })
     })
+
+    describe(`${DiskCacheError.name}`, function () {
+        it(`subclasses ${ToolkitError.named.name}()`, function () {
+            const dce = new DiskCacheError('foo')
+            assert.strictEqual(dce instanceof ToolkitError, true)
+            assert.strictEqual(dce.code, 'DiskCacheError')
+            assert.strictEqual(dce.name, 'DiskCacheError')
+        })
+    })
 })
 
 describe('Telemetry', function () {
@@ -618,76 +628,6 @@ describe('util', function () {
         })
     })
 
-    describe(`${DiskCacheError.name}`, function () {
-        function assertIsDiskCacheError(err: unknown, expected: { message: string; code: string }) {
-            const cacheError = DiskCacheError.instanceIf(err)
-            assert(cacheError instanceof DiskCacheError)
-            assert.deepStrictEqual(cacheError.message, expected.message)
-            assert.deepStrictEqual(cacheError.code, expected.code)
-        }
-
-        it('returns the original error on non-disk cache errors', function () {
-            const nonDiskCacheError = new Error('I am not a disk cache error')
-            nonDiskCacheError.name = 'NotADiskCacheError'
-
-            const result = DiskCacheError.instanceIf(nonDiskCacheError)
-
-            assert.deepStrictEqual(result instanceof DiskCacheError, false)
-            assert.deepStrictEqual(result, nonDiskCacheError)
-        })
-
-        it('errors without a message', function () {
-            const eacces = new Error()
-            eacces.name = 'EACCES'
-            assertIsDiskCacheError(eacces, { message: 'No msg', code: 'EACCES' })
-
-            const ebadf = new Error()
-            ebadf.name = 'EBADF'
-            assertIsDiskCacheError(ebadf, { message: 'No msg', code: 'EBADF' })
-        })
-
-        /**
-         * The error message is part of the heuristic to determine if certain errors
-         * can be a {@link DiskCacheError}. This asserts that if the message changes, the
-         * error can no longer be a {@link DiskCacheError}.
-         */
-        function assertExpectedMessageRequired(error: Error) {
-            error.message = 'Not the expected message'
-            assert.deepStrictEqual(DiskCacheError.instanceIf(error) instanceof DiskCacheError, false)
-            assert.deepStrictEqual(DiskCacheError.instanceIf(error), error)
-        }
-
-        it('ENOSPC', function () {
-            const error = new Error('Blah blah no space left on device blah blah')
-            error.name = 'ENOSPC'
-            assertIsDiskCacheError(error, {
-                message: 'Blah blah no space left on device blah blah',
-                code: 'ENOSPC',
-            })
-            assertExpectedMessageRequired(error)
-        })
-
-        it('EPERM', function () {
-            const error = new Error('Blah blah operation not permitted blah blah')
-            error.name = 'EPERM'
-            assertIsDiskCacheError(error, {
-                message: 'Blah blah operation not permitted blah blah',
-                code: 'EPERM',
-            })
-            assertExpectedMessageRequired(error)
-        })
-
-        it('EBUSY', function () {
-            const error = new Error('Blah blah resource busy or locked blah blah')
-            error.name = 'EBUSY'
-            assertIsDiskCacheError(error, {
-                message: 'Blah blah resource busy or locked blah blah',
-                code: 'EBUSY',
-            })
-            assertExpectedMessageRequired(error)
-        })
-    })
-
     it('scrubNames()', async function () {
         const fakeUser = 'jdoe123'
         assert.deepStrictEqual(scrubNames('', fakeUser), '')