Skip to content

Commit

Permalink
fix(notifications): extension activation hangs if fetch fails
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hayemaxi committed Nov 19, 2024
1 parent 7389d24 commit 4ea6d98
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 37 deletions.
2 changes: 1 addition & 1 deletion packages/amazonq/src/extensionNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Omit<AuthUserState, 'source'>> {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/extensionNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 17 additions & 11 deletions packages/core/src/notifications/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.')
}
}
52 changes: 28 additions & 24 deletions packages/core/src/notifications/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -39,8 +41,6 @@ import { telemetry } from '../shared/telemetry/telemetry'
* Emergency notifications - fetched at a regular interval.
*/
export class NotificationsController {
public static readonly suggestedPollIntervalMs = 1000 * 60 * 10 // 10 minutes

/** Internal memory state that is written to global state upon modification. */
private readonly state: NotificationsState

Expand Down Expand Up @@ -75,7 +75,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)
Expand Down Expand Up @@ -121,7 +121,9 @@ 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.readState() // Don't overwrite dismissals from other windows
this.state.dismissed.push(notificationId)
await this.writeState()

Expand All @@ -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
Expand All @@ -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(', ')
Expand All @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -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`)
}
})
)
Expand Down Expand Up @@ -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?
})
}
}

Expand All @@ -309,11 +317,7 @@ export class LocalFetcher implements NotificationFetcher {

async fetch(category: NotificationType, versionTag?: string): Promise<ResourceResponse> {
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(),
Expand Down

0 comments on commit 4ea6d98

Please sign in to comment.