diff --git a/src/plus/integrations/models/gitHostIntegration.ts b/src/plus/integrations/models/gitHostIntegration.ts index e64511a7cdb99..211e7255835ed 100644 --- a/src/plus/integrations/models/gitHostIntegration.ts +++ b/src/plus/integrations/models/gitHostIntegration.ts @@ -54,10 +54,11 @@ export abstract class GitHostIntegration< try { const author = await this.getProviderAccountForEmail(this._session!, repo, email, options); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getAccountForEmail'); return author; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getAccountForEmail', ex, { scope: scope }); + return undefined; } } @@ -84,10 +85,11 @@ export abstract class GitHostIntegration< try { const author = await this.getProviderAccountForCommit(this._session!, repo, rev, options); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getAccountForCommit'); return author; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getAccountForCommit', ex, { scope: scope }); + return undefined; } } @@ -117,10 +119,11 @@ export abstract class GitHostIntegration< value: (async () => { try { const result = await this.getProviderDefaultBranch(this._session!, repo, options?.cancellation); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getDefaultBranch'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getDefaultBranch', ex, { scope: scope }); + return undefined; } })(), }), @@ -160,10 +163,11 @@ export abstract class GitHostIntegration< repo, options?.cancellation, ); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getRepositoryMetadata'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getRepositoryMetadata', ex, { scope: scope }); + return undefined; } })(), }), @@ -188,10 +192,11 @@ export abstract class GitHostIntegration< try { const result = await this.mergeProviderPullRequest(this._session!, pr, options); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('mergePullRequest'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, false); + this.handleProviderException('mergePullRequest', ex, { scope: scope }); + return false; } } @@ -224,10 +229,11 @@ export abstract class GitHostIntegration< value: (async () => { try { const result = await this.getProviderPullRequestForBranch(this._session!, repo, branch, opts); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getPullRequestForBranch'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getPullRequestForBranch', ex, { scope: scope }); + return undefined; } })(), }), @@ -264,10 +270,11 @@ export abstract class GitHostIntegration< value: (async () => { try { const result = await this.getProviderPullRequestForCommit(this._session!, repo, rev); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getPullRequestForCommit'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getPullRequestForCommit', ex, { scope: scope }); + return undefined; } })(), }), @@ -663,10 +670,17 @@ export abstract class GitHostIntegration< cancellation, silent, ); + this.resetRequestExceptionCount('searchMyPullRequests'); return { value: pullRequests, duration: Date.now() - start }; } catch (ex) { - Logger.error(ex, scope); - return { error: ex, duration: Date.now() - start }; + this.handleProviderException('searchMyPullRequests', ex, { + scope: scope, + silent: true, + }); + return { + error: ex, + duration: Date.now() - start, + }; } } @@ -706,10 +720,11 @@ export abstract class GitHostIntegration< repos != null ? (Array.isArray(repos) ? repos : [repos]) : undefined, cancellation, ); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('searchPullRequests'); return prs; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('searchPullRequests', ex, { scope: scope }); + return undefined; } } diff --git a/src/plus/integrations/models/integration.ts b/src/plus/integrations/models/integration.ts index 3252bc34cae34..6d9b0515c4389 100644 --- a/src/plus/integrations/models/integration.ts +++ b/src/plus/integrations/models/integration.ts @@ -50,6 +50,29 @@ export type IntegrationResult = | { error: Error; duration?: number; value?: never } | undefined; +type SyncReqUsecase = Exclude< + | 'getAccountForCommit' + | 'getAccountForEmail' + | 'getAccountForResource' + | 'getCurrentAccount' + | 'getDefaultBranch' + | 'getIssue' + | 'getIssueOrPullRequest' + | 'getIssuesForProject' + | 'getProjectsForResources' + | 'getPullRequest' + | 'getPullRequestForBranch' + | 'getPullRequestForCommit' + | 'getRepositoryMetadata' + | 'getResourcesForUser' + | 'mergePullRequest' + | 'searchMyIssues' + | 'searchMyPullRequests' + | 'searchPullRequests', + // excluding to show explicitly that we don't want to add 'all' key occasionally + 'all' +>; + export abstract class IntegrationBase< ID extends IntegrationIds = IntegrationIds, T extends ResourceDescriptor = ResourceDescriptor, @@ -174,7 +197,7 @@ export abstract class IntegrationBase< void authProvider.deleteSession(this.authProviderDescriptor); } - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('all'); this._session = null; if (connected) { @@ -207,9 +230,30 @@ export abstract class IntegrationBase< void this.ensureSession({ createIfNeeded: false }); } + private _syncRequestsPerFailedUsecase = new Set(); + hasSessionSyncRequests(): boolean { + return this._syncRequestsPerFailedUsecase.size > 0; + } + requestSessionSyncForUsecase(syncReqUsecase: SyncReqUsecase): void { + this._syncRequestsPerFailedUsecase.add(syncReqUsecase); + } + private static readonly requestExceptionLimit = 5; private requestExceptionCount = 0; - resetRequestExceptionCount(): void { + resetRequestExceptionCount(syncReqUsecase: SyncReqUsecase | 'all'): void { + this.requestExceptionCount = 0; + if (syncReqUsecase === 'all') { + this._syncRequestsPerFailedUsecase.clear(); + } else { + this._syncRequestsPerFailedUsecase.delete(syncReqUsecase); + } + } + + /** + * Resets request exceptions without resetting the amount of syncs + */ + smoothifyRequestExceptionCount(): void { + // On resync we reset exception count only to avoid infinitive syncs on failure this.requestExceptionCount = 0; } @@ -234,7 +278,8 @@ export abstract class IntegrationBase< } switch (state) { - case 'connected': + case 'connected': { + const oldSession = this._session; if (forceSync) { // Reset our stored session so that we get a new one from the cloud const authProvider = await this.authenticationService.get(this.authProvider.id); @@ -257,23 +302,42 @@ export abstract class IntegrationBase< // sync option, rather than createIfNeeded, makes sure we don't call connectCloudIntegrations and open a gkdev window // if there was no session or some problem fetching/refreshing the existing session from the cloud api - await this.ensureSession({ sync: forceSync }); + const newSession = await this.ensureSession({ sync: forceSync }); + + if (oldSession && newSession && newSession.accessToken !== oldSession.accessToken) { + this.resetRequestExceptionCount('all'); + } + break; + } case 'disconnected': await this.disconnect({ silent: true }); break; } } - protected handleProviderException(ex: Error, scope: LogScope | undefined, defaultValue: T): T { - if (ex instanceof CancellationError) return defaultValue; - - Logger.error(ex, scope); - - if (ex instanceof AuthenticationError || ex instanceof RequestClientError) { - this.trackRequestException(); + protected handleProviderException( + syncReqUsecase: SyncReqUsecase, + ex: Error, + options?: { scope?: LogScope | undefined; silent?: boolean }, + ): void { + if (ex instanceof CancellationError) return; + + Logger.error(ex, options?.scope); + + if (ex instanceof AuthenticationError && this._session?.cloud) { + if (!this.hasSessionSyncRequests()) { + this.requestSessionSyncForUsecase(syncReqUsecase); + this._session = { + ...this._session, + expiresAt: new Date(Date.now() - 1), + }; + } else { + this.trackRequestException(options); + } + } else if (ex instanceof AuthenticationError || ex instanceof RequestClientError) { + this.trackRequestException(options); } - return defaultValue; } private missingExpirityReported = false; @@ -301,11 +365,13 @@ export abstract class IntegrationBase< } @debug() - trackRequestException(): void { + trackRequestException(options?: { silent?: boolean }): void { this.requestExceptionCount++; - if (this.requestExceptionCount >= 5 && this._session !== null) { - void showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name); + if (this.requestExceptionCount >= IntegrationBase.requestExceptionLimit && this._session !== null) { + if (!options?.silent) { + void showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name); + } void this.disconnect({ currentSessionOnly: true }); } } @@ -355,6 +421,10 @@ export abstract class IntegrationBase< source: source, }, ); + + if (session?.expiresAt != null && session.expiresAt < new Date()) { + session = null; + } } catch (ex) { await this.container.storage.deleteWorkspace(this.connectedKey); @@ -370,7 +440,7 @@ export abstract class IntegrationBase< } this._session = session ?? null; - this.resetRequestExceptionCount(); + this.smoothifyRequestExceptionCount(); if (session != null) { await this.container.storage.storeWorkspace(this.connectedKey, true); @@ -414,10 +484,11 @@ export abstract class IntegrationBase< resources != null ? (Array.isArray(resources) ? resources : [resources]) : undefined, cancellation, ); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('searchMyIssues'); return issues; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('searchMyIssues', ex, { scope: scope }); + return undefined; } } @@ -454,10 +525,11 @@ export abstract class IntegrationBase< id, options?.type, ); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getIssueOrPullRequest'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getIssueOrPullRequest', ex, { scope: scope }); + return undefined; } })(), }), @@ -494,10 +566,11 @@ export abstract class IntegrationBase< value: (async () => { try { const result = await this.getProviderIssue(this._session!, resource, id); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getIssue'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getIssue', ex, { scope: scope }); + return undefined; } })(), }), @@ -531,10 +604,11 @@ export abstract class IntegrationBase< value: (async () => { try { const account = await this.getProviderCurrentAccount?.(this._session!, opts); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getCurrentAccount'); return account; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getCurrentAccount', ex, { scope: scope }); + return undefined; } })(), }), @@ -561,10 +635,11 @@ export abstract class IntegrationBase< value: (async () => { try { const result = await this.getProviderPullRequest?.(this._session!, resource, id); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getPullRequest'); return result; } catch (ex) { - return this.handleProviderException(ex, scope, undefined); + this.handleProviderException('getPullRequest', ex, { scope: scope }); + return undefined; } })(), })); diff --git a/src/plus/integrations/models/issuesIntegration.ts b/src/plus/integrations/models/issuesIntegration.ts index 0ae4904b01324..717ac582eafc0 100644 --- a/src/plus/integrations/models/issuesIntegration.ts +++ b/src/plus/integrations/models/issuesIntegration.ts @@ -32,10 +32,11 @@ export abstract class IssuesIntegration< try { const account = await this.getProviderAccountForResource(this._session!, resource); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getAccountForResource'); return account; } catch (ex) { - return this.handleProviderException(ex, undefined, undefined); + this.handleProviderException('getAccountForResource', ex); + return undefined; } } @@ -55,10 +56,11 @@ export abstract class IssuesIntegration< try { const resources = await this.getProviderResourcesForUser(this._session!); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getResourcesForUser'); return resources; } catch (ex) { - return this.handleProviderException(ex, undefined, undefined); + this.handleProviderException('getResourcesForUser', ex); + return undefined; } } @@ -74,10 +76,11 @@ export abstract class IssuesIntegration< try { const projects = await this.getProviderProjectsForResources(this._session!, resources); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getProjectsForResources'); return projects; } catch (ex) { - return this.handleProviderException(ex, undefined, undefined); + this.handleProviderException('getProjectsForResources', ex); + return undefined; } } @@ -106,10 +109,11 @@ export abstract class IssuesIntegration< try { const issues = await this.getProviderIssuesForProject(this._session!, project, options); - this.resetRequestExceptionCount(); + this.resetRequestExceptionCount('getIssuesForProject'); return issues; } catch (ex) { - return this.handleProviderException(ex, undefined, undefined); + this.handleProviderException('getIssuesForProject', ex); + return undefined; } }