Skip to content

Sync cloud integration on Authorization problems #4324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 16, 2025
Merged

Sync cloud integration on Authorization problems #4324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37cfe1a
Syncs cloud integration on Authorizatin problems
sergeibbb May 21, 2025
87141b4
Stores auth problem flag per usecase to avoid continuous refreshing
chivorotkiv Jun 5, 2025
2ecfca5
Avoids continuous refreshing when GKDev cannot renew expired session
chivorotkiv Jun 5, 2025
4d05594
Refactors exception handling for integrations
sergeibbb Jun 19, 2025
ff0c424
Suppresses warning message on Launchpad load failure
sergeibbb Jun 19, 2025
aaa9497
Resets request exception count after token refresh
sergeibbb Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 33 additions & 18 deletions src/plus/integrations/models/gitHostIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Account | undefined>(ex, scope, undefined);
this.handleProviderException('getAccountForEmail', ex, { scope: scope });
return undefined;
}
}

Expand All @@ -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<Account | undefined>(ex, scope, undefined);
this.handleProviderException('getAccountForCommit', ex, { scope: scope });
return undefined;
}
}

Expand Down Expand Up @@ -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<DefaultBranch | undefined>(ex, scope, undefined);
this.handleProviderException('getDefaultBranch', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand Down Expand Up @@ -160,10 +163,11 @@ export abstract class GitHostIntegration<
repo,
options?.cancellation,
);
this.resetRequestExceptionCount();
this.resetRequestExceptionCount('getRepositoryMetadata');
return result;
} catch (ex) {
return this.handleProviderException<RepositoryMetadata | undefined>(ex, scope, undefined);
this.handleProviderException('getRepositoryMetadata', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand All @@ -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<boolean>(ex, scope, false);
this.handleProviderException('mergePullRequest', ex, { scope: scope });
return false;
}
}

Expand Down Expand Up @@ -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<PullRequest | undefined>(ex, scope, undefined);
this.handleProviderException('getPullRequestForBranch', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand Down Expand Up @@ -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<PullRequest | undefined>(ex, scope, undefined);
this.handleProviderException('getPullRequestForCommit', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand Down Expand Up @@ -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,
};
}
}

Expand Down Expand Up @@ -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<PullRequest[] | undefined>(ex, scope, undefined);
this.handleProviderException('searchPullRequests', ex, { scope: scope });
return undefined;
}
}

Expand Down
127 changes: 101 additions & 26 deletions src/plus/integrations/models/integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,29 @@ export type IntegrationResult<T> =
| { 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,
Expand Down Expand Up @@ -174,7 +197,7 @@ export abstract class IntegrationBase<
void authProvider.deleteSession(this.authProviderDescriptor);
}

this.resetRequestExceptionCount();
this.resetRequestExceptionCount('all');
this._session = null;

if (connected) {
Expand Down Expand Up @@ -207,9 +230,30 @@ export abstract class IntegrationBase<
void this.ensureSession({ createIfNeeded: false });
}

private _syncRequestsPerFailedUsecase = new Set<SyncReqUsecase>();
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;
}

Expand All @@ -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);
Expand All @@ -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<T>(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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to note here that once we have a "session sync request" for a particular case like "getIssuesForProject", we will not pass this check again until either:

  1. that specific function has had a chance to run successfully, or
  2. the integration disconnects
    So for a three hour session, if the token lasts an hour and we run into this once for some specific query, we will go back to the default non-sync failures for other queries if that specific query is not used again after syncing the first time.

Not a big deal I suppose, and not sure there is a perfect solution here - just making sure we are aware of the limitations even though this is better than the previous version,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@axosoft-ramint

we will not pass this check again until either:

What do you think about this: add09cc

It should work like this:

  • we detect an auth error on getIssuesForProject
  • we expire it locally in our memory to force it resync
  • Then one of the following happens:
    • if it's not actually expired GK.dev returns us the same token, so we don't reset the flags
    • if GK.dev refreshes token and returns a new one, we reset all the flags, to be able to see how it works.

I haven't tested it thoroughly yet, but if you approve the approach I go ahead with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach sounds ok. Once you've tested it, if it seems like it works, let me know.

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;
Expand Down Expand Up @@ -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 });
}
}
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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<IssueShape[] | undefined>(ex, scope, undefined);
this.handleProviderException('searchMyIssues', ex, { scope: scope });
return undefined;
}
}

Expand Down Expand Up @@ -454,10 +525,11 @@ export abstract class IntegrationBase<
id,
options?.type,
);
this.resetRequestExceptionCount();
this.resetRequestExceptionCount('getIssueOrPullRequest');
return result;
} catch (ex) {
return this.handleProviderException<IssueOrPullRequest | undefined>(ex, scope, undefined);
this.handleProviderException('getIssueOrPullRequest', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand Down Expand Up @@ -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<Issue | undefined>(ex, scope, undefined);
this.handleProviderException('getIssue', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand Down Expand Up @@ -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<Account | undefined>(ex, scope, undefined);
this.handleProviderException('getCurrentAccount', ex, { scope: scope });
return undefined;
}
})(),
}),
Expand All @@ -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<PullRequest | undefined>(ex, scope, undefined);
this.handleProviderException('getPullRequest', ex, { scope: scope });
return undefined;
}
})(),
}));
Expand Down
Loading