fix: "Failed to access settings" when restarting multiple vscodes #4474

Problem:
Shutdown-and-restart VS Code with multiple windows opened, causes AWS
Toolkit to show a "Failed to access settings" error. #4453

Steps to reproduce:
1. open a bunch of different vscode instances/workspaces.
2. close all vscode instances.
3. start vscode (which resumes all the previously-closed instances).

Solution:
Verifying settings on startup causes too many problems, so drop it in
favor of verifying just-in-time.

Author: justinmk3

.changes/next-release/Bug Fix-ae8ef2c1-9969-45f9-bda9-0249ee124c46.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "\"Failed to access settings\" error when restarting multiple vscode instances"
+}

packages/core/src/extension.ts

@@ -46,11 +46,10 @@ import { EcsCredentialsProvider } from './auth/providers/ecsCredentialsProvider'
 import { SchemaService } from './shared/schemas'
 import { AwsResourceManager } from './dynamicResources/awsResourceManager'
 import globals from './shared/extensionGlobals'
-import { Experiments, Settings } from './shared/settings'
+import { Experiments, Settings, showSettingsFailedMsg } from './shared/settings'
 import { isReleaseVersion } from './shared/vscode/env'
 import { telemetry } from './shared/telemetry/telemetry'
 import { Auth } from './auth/auth'
-import { showViewLogsMessage } from './shared/utilities/messages'
 import { initializeNetworkAgent } from './codewhisperer/client/agent'
 import { Timeout } from './shared/utilities/timeoutUtils'
 import { submitFeedback } from './feedback/vue/submitFeedback'
@@ -180,7 +179,10 @@ export async function activate(context: vscode.ExtensionContext) {
 
         showWelcomeMessage(context)
 
-        const settingsValid = await checkSettingsHealth(settings)
+        const settingsValid = await settings.isReadable()
+        if (!settingsValid) {
+            void showSettingsFailedMsg('read')
+        }
         recordToolkitInitialization(activationStartedOn, settingsValid, getLogger())
 
         if (!isReleaseVersion()) {
@@ -238,38 +240,13 @@ function recordToolkitInitialization(activationStartedOn: number, settingsValid:
         if (settingsValid) {
             telemetry.toolkit_init.emit({ duration, result: 'Succeeded' })
         } else {
-            telemetry.toolkit_init.emit({ duration, result: 'Failed', reason: 'UserSettings' })
+            telemetry.toolkit_init.emit({ duration, result: 'Failed', reason: 'UserSettingsRead' })
         }
     } catch (err) {
         logger?.error(err as Error)
     }
 }
 
-async function checkSettingsHealth(settings: Settings): Promise<boolean> {
-    const r = await settings.isValid()
-    switch (r) {
-        case 'invalid': {
-            const msg = 'Failed to access settings. Check settings.json for syntax errors.'
-            const openSettingsItem = 'Open settings.json'
-            void showViewLogsMessage(msg, 'error', [openSettingsItem]).then(async resp => {
-                if (resp === openSettingsItem) {
-                    await vscode.commands.executeCommand('workbench.action.openSettingsJson')
-                }
-            })
-            return false
-        }
-        // Don't show a message for 'nowrite' because:
-        //  - settings.json may intentionally be readonly. #4043
-        //  - vscode will show its own error if settings.json cannot be written.
-        //
-        // Note: isValid() already logged a warning.
-        case 'nowrite':
-        case 'ok':
-        default:
-            return true
-    }
-}
-
 // Unique extension entrypoint names, so that they can be obtained from the webpack bundle
 export const awsToolkitActivate = activate
 export const awsToolkitDeactivate = deactivate

packages/core/src/shared/sam/cli/samCliDetection.ts

@@ -37,14 +37,12 @@ export async function detectSamCli(args: { passive: boolean; showMessage: boolea
     // conflicts with VSCode "remote": each VSCode instance will update the
     // setting based on its local environment, but the user settings are
     // shared across VSCode instances...
-    if (!args.passive && sam.autoDetected && sam.path) {
-        await config.update('location', sam.path)
-    }
+    const update = !args.passive && sam.autoDetected && sam.path && (await config.update('location', sam.path))
 
     if (args.showMessage !== false || !found) {
         if (!found) {
             notifyUserSamCliNotDetected(config)
-        } else if (args.showMessage === true) {
+        } else if (update && args.showMessage === true) {
             void vscode.window.showInformationMessage(getSettingsUpdatedMessage(sam.path ?? '?'))
         }
     }

packages/core/src/shared/settings.ts

@@ -7,18 +7,50 @@ import * as vscode from 'vscode'
 import * as packageJson from '../../package.json'
 import * as codecatalyst from './clients/codecatalystClient'
 import * as codewhisperer from '../codewhisperer/client/codewhisperer'
-import { getLogger } from './logger'
+import { getLogger, showLogOutputChannel } from './logger'
 import { cast, FromDescriptor, Record, TypeConstructor, TypeDescriptor } from './utilities/typeConstructors'
 import { assertHasProps, ClassToInterfaceType, keys } from './utilities/tsUtils'
 import { toRecord } from './utilities/collectionUtils'
 import { isNameMangled } from './vscode/env'
 import { once, onceChanged } from './utilities/functionUtils'
 import { ToolkitError } from './errors'
+import { telemetry } from './telemetry/telemetry'
 
 type Workspace = Pick<typeof vscode.workspace, 'getConfiguration' | 'onDidChangeConfiguration'>
 
-/** Used by isValid(). Must be something that's defined in our package.json. */
-const testSetting = 'aws.samcli.lambdaTimeout'
+/** Used by isReadable(). Must be something that's defined in our package.json. */
+export const testSetting = 'aws.samcli.lambdaTimeout'
+
+export async function showSettingsFailedMsg(kind: 'read' | 'update', key?: string) {
+    const keyMsg = key ? ` (key: "${key}")` : ''
+    const msg = `Failed to ${kind} settings${keyMsg}. Check settings.json for syntax errors or insufficient permissions.`
+    const openSettingsItem = 'Open settings.json'
+    const logsItem = 'View Logs...'
+
+    const items = [openSettingsItem, logsItem]
+    const p = vscode.window.showErrorMessage(msg, {}, ...items)
+    return p.then<string | undefined>(async selection => {
+        if (selection === logsItem) {
+            showLogOutputChannel()
+        } else if (selection === openSettingsItem) {
+            await vscode.commands.executeCommand('workbench.action.openSettingsJson')
+        }
+        return selection
+    })
+}
+
+/**
+ * Shows an error message if we couldn't update settings, unless the last message was for the same `key`.
+ */
+const showSettingsUpdateFailedMsgOnce = onceChanged(key => {
+    // Edge cases:
+    //  - settings.json may intentionally be readonly. #4043
+    //  - settings.json may be open in multiple vscodes. #4453
+    //  - vscode will show its own error if settings.json cannot be written.
+    void showSettingsFailedMsg('update', key)
+
+    telemetry.aws_modifySetting.emit({ result: 'Failed', reason: 'UserSettingsWrite', settingId: key })
+})
 
 /**
  * A class for manipulating VS Code user settings (from all extensions).
@@ -78,58 +110,43 @@ export class Settings {
      * `vscode.ConfigurationTarget.Workspace` target requires a workspace).
      */
     public async update(key: string, value: unknown): Promise<boolean> {
+        const config = this.getConfig()
         try {
-            await this.getConfig().update(key, value, this.updateTarget)
+            await config.update(key, value, this.updateTarget)
 
             return true
         } catch (e) {
-            getLogger().warn('settings: failed to update "%s": %s', key, (e as Error).message)
+            const fullKey = config.inspect(key)?.key ?? key
+            getLogger().warn('settings: failed to update "%s": %s', fullKey, (e as Error).message)
+            showSettingsUpdateFailedMsgOnce(fullKey)
 
             return false
         }
     }
 
     /**
-     * Checks that user `settings.json` actually works. #3910
+     * Checks that user `settings.json` is readable. #3910
      *
-     * Note: This checks that we can actually "roundtrip" (read and write) settings. vscode notifies
-     * the user if settings.json is complete nonsense, but silently fails if there are only
-     * "recoverable" JSON syntax errors.
+     * Note: Does NOT check that we can "roundtrip" (read-and-write) settings. vscode notifies the
+     * user if settings.json is complete nonsense, but silently fails if there are only
+     * "recoverable" JSON syntax errors. We can't test for "roundtrip" on _startup_ because it causes
+     * race conditions if multiple VSCode instances start simultaneously. #4453
+     * Instead we handle that in {@link Settings#update()}.
      */
-    public async isValid(): Promise<'ok' | 'invalid' | 'nowrite'> {
+    public async isReadable(): Promise<boolean> {
         const key = testSetting
         const config = this.getConfig()
-        const tempValOld = 1234 // Legacy temp value we are migrating from.
-        const tempVal = 91234 // Temp value used to check that read/write works.
-        const defaultVal = settingsProps[key].default
 
         try {
-            const userVal = config.get<number>(key)
-            // Try to write a temporary "sentinel" value to settings.json.
-            await config.update(key, tempVal, this.updateTarget)
-            if (userVal === undefined || [defaultVal, tempValOld, tempVal].includes(userVal)) {
-                // Avoid polluting the user's settings.json.
-                await config.update(key, undefined, this.updateTarget)
-            } else {
-                // Restore the user's actual setting value.
-                await config.update(key, userVal, this.updateTarget)
-            }
+            config.get<number>(key)
 
-            return 'ok'
+            return true
         } catch (e) {
             const err = e as Error
-            // If anything tries to update an unwritable settings.json, vscode will thereafter treat
-            // it as an "unsaved" file. #4043
-            if (err.message.includes('EACCES') || err.message.includes('the file has unsaved changes')) {
-                const logMsg = 'settings: unwritable settings.json: %s'
-                getLogger().warn(logMsg, err.message)
-                return 'nowrite'
-            }
-
             const logMsg = 'settings: invalid settings.json: %s'
             getLogger().error(logMsg, err.message)
 
-            return 'invalid'
+            return false
         }
     }
 
@@ -321,6 +338,8 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
 
                 return true
             } catch (e) {
+                const fullKey = `${section}.${key}`
+                showSettingsUpdateFailedMsgOnce(fullKey)
                 this._log('failed to update "%s": %s', key, (e as Error).message)
 
                 return false
@@ -333,6 +352,8 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
 
                 return true
             } catch (e) {
+                const fullKey = `${section}.${key}`
+                showSettingsUpdateFailedMsgOnce(fullKey)
                 this._log('failed to delete "%s": %s', key, (e as Error).message)
 
                 return false
@@ -394,9 +415,7 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
                 }
 
                 for (const key of props.filter(isDifferent)) {
-                    if (`${section}.${key}` !== testSetting) {
-                        this._log('key "%s" changed', key)
-                    }
+                    this._log('key "%s" changed', key)
                     emitter.fire({ key })
                 }
             })

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

@@ -6,11 +6,19 @@
 import assert from 'assert'
 import * as sinon from 'sinon'
 import * as vscode from 'vscode'
-import { DevSettings, Experiments, fromExtensionManifest, PromptSettings, Settings } from '../../shared/settings'
+import {
+    DevSettings,
+    Experiments,
+    fromExtensionManifest,
+    PromptSettings,
+    Settings,
+    testSetting,
+} from '../../shared/settings'
 import { TestSettings } from '../utilities/testSettingsConfiguration'
 import { ClassToInterfaceType } from '../../shared/utilities/tsUtils'
 import { Optional } from '../../shared/utilities/typeConstructors'
 import { ToolkitError } from '../../shared/errors'
+import { getTestWindow } from './vscode/window'
 
 const settingsTarget = vscode.ConfigurationTarget.Workspace
 
@@ -46,10 +54,13 @@ describe('Settings', function () {
         // Note: we don't test undefined because retrieval returns the package.json configured default value, if there is one
     ]
 
-    it('isValid()', async () => {
+    it('isReadable()', async () => {
         // TODO: could avoid sinon if we can force vscode to use a dummy settings.json file.
         const fake = {
-            get: async () => {
+            get: (key: string) => {
+                if (key === testSetting) {
+                    throw Error()
+                }
                 return 'setting-value'
             },
             update: async () => {
@@ -58,17 +69,7 @@ describe('Settings', function () {
         } as unknown as vscode.WorkspaceConfiguration
         sinon.stub(vscode.workspace, 'getConfiguration').returns(fake)
 
-        assert.deepStrictEqual(await sut.isValid(), 'invalid')
-
-        fake.update = async () => {
-            throw Error('EACCES')
-        }
-        assert.deepStrictEqual(await sut.isValid(), 'nowrite')
-
-        fake.update = async () => {
-            throw Error('xxx the file has unsaved changes xxx')
-        }
-        assert.deepStrictEqual(await sut.isValid(), 'nowrite')
+        assert.deepStrictEqual(await sut.isReadable(), false)
     })
 
     describe('get', function () {
@@ -122,6 +123,43 @@ describe('Settings', function () {
                 assert.deepStrictEqual(savedValue, scenario.testValue)
             })
         })
+
+        it('shows message on failure', async () => {
+            // TODO: could avoid sinon if we can force vscode to use a dummy settings.json file.
+            // const fakeSettings = {}
+            const fake = {
+                lastValue: 'x',
+                get: (key: string) => {
+                    return `${key}-value`
+                },
+                update: async (key: string, val: any) => {
+                    // Do nothing (success).
+                    // (fakeSettings as any)[key] = val
+                },
+                inspect: (key: string) => {
+                    return {}
+                },
+            } as unknown as vscode.WorkspaceConfiguration
+            sinon.stub(vscode.workspace, 'getConfiguration').returns(fake)
+
+            assert.deepStrictEqual(await sut.update(testSetting, 91234), true)
+            // No errors.
+            assert.deepStrictEqual(getTestWindow().shownMessages.length, 0)
+
+            fake.update = async () => {
+                throw Error('EACCES')
+            }
+            assert.deepStrictEqual(await sut.update(testSetting, 91234), false)
+            getTestWindow()
+                .getFirstMessage()
+                .assertError(
+                    `Failed to update settings (key: "${testSetting}"). Check settings.json for syntax errors or insufficient permissions.`
+                )
+
+            // Message skipped for duplicate error.
+            assert.deepStrictEqual(await sut.update(testSetting, 91234), false)
+            assert.deepStrictEqual(getTestWindow().shownMessages.length, 1)
+        })
     })
 
     describe('isSet', function () {

packages/core/src/test/utilities/testSettingsConfiguration.ts

@@ -48,8 +48,8 @@ export class TestSettings implements ClassToInterfaceType<Settings> {
         return !type || value === undefined ? value : cast(value, type)
     }
 
-    public async isValid(): Promise<'ok' | 'invalid' | 'nowrite'> {
-        return 'ok'
+    public async isReadable(): Promise<boolean> {
+        return true
     }
 
     public async update(key: string, value: unknown): Promise<boolean> {