From 3e4a5d68a6b3aa80562add2f7200d9d3f913b88c Mon Sep 17 00:00:00 2001 From: Maxim Hayes Date: Tue, 19 Nov 2024 17:44:46 -0500 Subject: [PATCH] fix(notifications): extension activation hangs if fetch fails Problem: If notifications fail to fetch, it will retry multiple times with 30 second waits. Activation is awaiting on this though, so it may take forever to finish. Solution: don't await on notification activation Also, update logging statements. --- packages/amazonq/src/extensionNode.ts | 2 +- packages/core/src/extensionNode.ts | 2 +- packages/core/src/notifications/activation.ts | 28 ++++++----- packages/core/src/notifications/controller.ts | 48 ++++++++++--------- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/packages/amazonq/src/extensionNode.ts b/packages/amazonq/src/extensionNode.ts index efb118e570c..2997b709348 100644 --- a/packages/amazonq/src/extensionNode.ts +++ b/packages/amazonq/src/extensionNode.ts @@ -75,7 +75,7 @@ async function activateAmazonQNode(context: vscode.ExtensionContext) { ...authState, }) - await activateNotifications(context, authState, getAuthState) + void activateNotifications(context, authState, getAuthState) } async function getAuthState(): Promise> { diff --git a/packages/core/src/extensionNode.ts b/packages/core/src/extensionNode.ts index cef34e522df..fecbec41d8d 100644 --- a/packages/core/src/extensionNode.ts +++ b/packages/core/src/extensionNode.ts @@ -246,7 +246,7 @@ export async function activate(context: vscode.ExtensionContext) { ...authState, }) - await activateNotifications(context, authState, getAuthState) + void activateNotifications(context, authState, getAuthState) } catch (error) { const stacktrace = (error as Error).stack?.split('\n') // truncate if the stacktrace is unusually long diff --git a/packages/core/src/notifications/activation.ts b/packages/core/src/notifications/activation.ts index cd732909d16..dcf07cf2b8f 100644 --- a/packages/core/src/notifications/activation.ts +++ b/packages/core/src/notifications/activation.ts @@ -13,6 +13,8 @@ import { AuthState } from './types' import { getLogger } from '../shared/logger/logger' import { oneMinute } from '../shared/datetime' +const logger = getLogger('notifications') + /** Time in MS to poll for emergency notifications */ const emergencyPollTime = oneMinute * 10 @@ -33,19 +35,23 @@ export async function activate( return } - const panelNode = NotificationsNode.instance - panelNode.registerView(context) + try { + const panelNode = NotificationsNode.instance + panelNode.registerView(context) - const controller = new NotificationsController(panelNode) - const engine = new RuleEngine(await getRuleContext(context, initialState)) + const controller = new NotificationsController(panelNode) + const engine = new RuleEngine(await getRuleContext(context, initialState)) - await controller.pollForStartUp(engine) - await controller.pollForEmergencies(engine) + await controller.pollForStartUp(engine) + await controller.pollForEmergencies(engine) - globals.clock.setInterval(async () => { - const ruleContext = await getRuleContext(context, await authStateFn()) - await controller.pollForEmergencies(new RuleEngine(ruleContext)) - }, emergencyPollTime) + globals.clock.setInterval(async () => { + const ruleContext = await getRuleContext(context, await authStateFn()) + await controller.pollForEmergencies(new RuleEngine(ruleContext)) + }, emergencyPollTime) - getLogger('notifications').debug('Activated in-IDE notifications polling module') + logger.debug('Activated in-IDE notifications polling module') + } catch (err) { + logger.error('Failed to activate in-IDE notifications module.') + } } diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts index 30aa8ef3570..5a2c17c729f 100644 --- a/packages/core/src/notifications/controller.ts +++ b/packages/core/src/notifications/controller.ts @@ -26,6 +26,8 @@ import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetch import { isAmazonQ } from '../shared/extensionUtilities' import { telemetry } from '../shared/telemetry/telemetry' +const logger = getLogger('notifications') + /** * Handles fetching and maintaining the state of in-IDE notifications. * Notifications are constantly polled from a known endpoint and then stored in global state. @@ -75,7 +77,7 @@ export class NotificationsController { this.readState() await this.fetchNotifications(category) } catch (err: any) { - getLogger('notifications').error(`Unable to fetch %s notifications: %s`, category, err) + logger.error(`Unable to fetch %s notifications: %s`, category, err) } await this.displayNotifications(ruleEngine) @@ -121,7 +123,7 @@ export class NotificationsController { * hide all notifications. */ public async dismissNotification(notificationId: string) { - getLogger('notifications').debug('Dismissing notification: %s', notificationId) + logger.debug('Dismissing notification: %s', notificationId) this.state.dismissed.push(notificationId) await this.writeState() @@ -134,7 +136,7 @@ export class NotificationsController { private async fetchNotifications(category: NotificationType) { const response = await this.fetcher.fetch(category, this.state[category].eTag) if (!response.content) { - getLogger('notifications').verbose('No new notifications for category: %s', category) + logger.verbose('No new notifications for category: %s', category) return } // Parse the notifications @@ -149,7 +151,7 @@ export class NotificationsController { const addedNotifications = newNotifications.filter((n: any) => !currentNotificationIds.has(n.id)) if (addedNotifications.length > 0) { - getLogger('notifications').verbose( + logger.verbose( 'New notifications received for category %s, ids: %s', category, addedNotifications.map((n: any) => n.id).join(', ') @@ -161,7 +163,7 @@ export class NotificationsController { this.state[category].eTag = response.eTag await this.writeState() - getLogger('notifications').verbose( + logger.verbose( "Fetched notifications JSON for category '%s' with schema version: %s. There were %d notifications.", category, this.state[category].payload?.schemaVersion, @@ -173,7 +175,7 @@ export class NotificationsController { * Write the latest memory state to global state. */ private async writeState() { - getLogger('notifications').debug('NotificationsController: Updating notifications state at %s', this.storageKey) + logger.debug('NotificationsController: Updating notifications state at %s', this.storageKey) // Clean out anything in 'dismissed' that doesn't exist anymore. const notifications = new Set( @@ -239,7 +241,7 @@ function registerDismissCommand() { await NotificationsController.instance.dismissNotification(notification.id) }) } else { - getLogger('notifications').error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`) + logger.error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`) } }) ) @@ -277,17 +279,23 @@ export class RemoteFetcher implements NotificationFetcher { const fetcher = new HttpResourceFetcher(endpoint, { showUrl: true, }) - getLogger('notifications').verbose( - 'Attempting to fetch notifications for category: %s at endpoint: %s', - category, - endpoint + logger.verbose('Attempting to fetch notifications for category: %s at endpoint: %s', category, endpoint) + + return withRetries( + async () => { + try { + return await fetcher.getNewETagContent(versionTag) + } catch (err) { + logger.error('Failed to fetch at endpoint: %s, err: %s', endpoint, err) + throw err + } + }, + { + maxRetries: RemoteFetcher.retryNumber, + delay: RemoteFetcher.retryIntervalMs, + // No exponential backoff - necessary? + } ) - - return withRetries(async () => await fetcher.getNewETagContent(versionTag), { - maxRetries: RemoteFetcher.retryNumber, - delay: RemoteFetcher.retryIntervalMs, - // No exponential backoff - necessary? - }) } } @@ -309,11 +317,7 @@ export class LocalFetcher implements NotificationFetcher { async fetch(category: NotificationType, versionTag?: string): Promise { const uri = category === 'startUp' ? this.startUpLocalPath : this.emergencyLocalPath - getLogger('notifications').verbose( - 'Attempting to fetch notifications locally for category: %s at path: %s', - category, - uri - ) + logger.verbose('Attempting to fetch notifications locally for category: %s at path: %s', category, uri) return { content: await new FileResourceFetcher(globals.context.asAbsolutePath(uri)).get(),