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.

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

Conversation

sergeibbb
Copy link
Member

@sergeibbb sergeibbb commented May 21, 2025

Description

Here we sync the cloud connected integration with GKDev when we face an authorization problem hoping that during the sync they respond us with the actual token.

If we see that we keep facing the same error we don't try to sync again. Instead we disconnect the integration soon letting user know that they need to take an action to reconnect.

Related to:

Follow-ups:

Verification steps

  • Simple stop syncing
    2. Connect GitHub-com integration on GKDev
    3. Reject the OAuth applicaiton on GitHub's side, by keeping it on GKDev
    4. Open launchpad and refresh its content
    5. Check that GitLens stops trying to refresh the invalid token (stops auto-syncing)
  • Keeps being stopped after manual sync when token has not been refreshed
    6. Continue the previous scenario
    7. Manually sync the integrations, make sure GitHub is still highlighted as connected
    8. Open launchpad and refresh its content
    9. Check that GitLens does not try sync GitHub integration
  • Manual sync recovers the integration when it is fixed on GKDev
    9. Continue the previous scenario
    10. Go to GKDev and re-connect GitHub integration
    11. Got to GitLens and sync integrations manually
    12. Open launchpad and refresh its content
    13. Check that GitHub integration started working
  • Tries to sync once after it was blocked and then recovered
    14. Continue the previous scenario
    15. Go to GitHub settings and reject the application again
    16. Go to GKDev and re-connect: so GKDev has a new token and GitLens yet has the old token.
    17. Open launchpad and refresh its content twice
    18. Check that the GitLens has refreshed the token and shows the Launchpad content
  • Lack of permissions: it does not sync continuously when some requests succeed and others fail.
    • ❓ I wanted to connect a self-hosted integrations (e.g. GitHub Enterprise) with lack of permissions . But I failed. With whatever limitations of the token, I was able to get public PRs. I tested on GitHub Enterprise, GitLab Server, Bitbucket Server. If you find the exact way to create such token, then please let me know.
    • Another way to check it is to make a change in code to alwasy fail PRs and check that it does not try to sync again and again (that it stops syncing)
  • We do not message user due to errors in Launchpad (if the user errors too many times, and folks whose integration was having issues with Launchpad, due to too many PRs, large/complex repos, etc.)

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@sergeibbb sergeibbb changed the title Sync cloud integration on Authorizatin problems Sync cloud integration on Authorization problems May 21, 2025
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from eb468fa to fd28d05 Compare May 21, 2025 18:00
sergeibbb added a commit that referenced this pull request May 21, 2025
@sergeibbb sergeibbb requested a review from eamodio May 21, 2025 18:00
sergeibbb added a commit that referenced this pull request May 22, 2025
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from fd28d05 to 500523d Compare May 22, 2025 11:27
sergeibbb added a commit that referenced this pull request May 23, 2025
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 500523d to 3ce2c47 Compare May 23, 2025 10:21
@sergeibbb sergeibbb requested a review from axosoft-ramint June 3, 2025 16:22
sergeibbb added a commit that referenced this pull request Jun 3, 2025
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 3ce2c47 to c962bd0 Compare June 3, 2025 16:56
@sergeibbb sergeibbb added this to the 17.2 milestone Jun 3, 2025
@sergeibbb sergeibbb self-assigned this Jun 3, 2025
Comment on lines 287 to 334
this.syncCountDueToRequestException++;
this._session = {
...this._session,
expiresAt: new Date(Date.now() - 1),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a hypothetical scenario I want to check with you here.

Let's say I have a GitHub token which has the required scopes to get my current account but not the scopes to get PRs.

So, every time the Launchpad automatically fetches PRs (let's say, every 5 minutes), it first gets the current account, which succeeds (and also resets the sync count) and then it tries to get PRs and fails because of the scope issue, throws an AuthenticationError, and we force it to sync.

This would cause us to sync every time the Launchpad fetches PRs which would hit our backend pretty hard and probably is not the intention, but..is it possible? If we can confirm that I am worrying for nothing, I think we are ok to merge.

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

That makes sense. It's hard to reproduce, probably should be reproduced on a self-hosted server where the integration is connected using PAT.

Anyway, I've implemented the fix here: 4e1b411 when a success in one use-case doesn't cancel failure in another case, so we avoid continuous syncing.

Also I've noticed that when GKDev keeps returning expired key we keep refreshing it in refreshSessionIfExpired(), so I added another fix: 3c06400

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

better solution for unsuccessful syncing of expired tokens: b178a03

@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from a1bdad6 to 9bcdaad Compare June 5, 2025 14:41
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 9bcdaad to 3c06400 Compare June 5, 2025 14:43
@sergeibbb sergeibbb requested a review from axosoft-ramint June 5, 2025 14:52
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 3c06400 to 3e099e9 Compare June 5, 2025 16:13
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 3e099e9 to b178a03 Compare June 5, 2025 16:15
return { value: pullRequests, duration: Date.now() - start };
} catch (ex) {
Logger.error(ex, scope);
return { error: ex, duration: Date.now() - start };
return this.handleProviderException('searchMyPullRequests', ex, scope, {
Copy link
Contributor

@axosoft-ramint axosoft-ramint Jun 6, 2025

Choose a reason for hiding this comment

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

Sorry I'm catching this a bit late, but: we moved searchMyPullRequests off of handleProviderException on purpose, specifically because it results in an error notification in VS Code if the user errors too many times, and folks whose integration was having issues with Launchpad (due to too many PRs, large/complex repos, etc.) were seeing the notification all the time since Launchpad query automatically runs in an interval.

We need to make sure we do not send a message in this case.

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

RE: we moved searchMyPullRequests off of handleProviderException on purpose, specifically because it results in an error notification in VS Code if the user errors too many times, and folks whose integration was having issues with Launchpad (due to too many PRs, large/complex repos, etc.)

What do you think about adding silent flag: b4db45d ?
(It's done after a little refactoring: 07116b4)

if (ex instanceof CancellationError) return defaultValue;

Logger.error(ex, scope);

if (ex instanceof AuthenticationError || ex instanceof RequestClientError) {
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.

@d13 d13 modified the milestones: 17.2, 17.2-patch Jun 12, 2025
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 19f289a to c0205c9 Compare July 15, 2025 13:21
sergeibbb added a commit that referenced this pull request Jul 15, 2025
sergeibbb added a commit that referenced this pull request Jul 15, 2025
- The `handleProviderException` method in `IntegrationBase` is updated to accept an options object instead of individual parameters.
- The method now only returns void, requiring the calling functions to return the default value themselves.

(#4324)
sergeibbb added a commit that referenced this pull request Jul 15, 2025
Resets the request exception count when a new access token is obtained after refreshing the session.
This prevents the integration from being prematurely disconnected due to request errors associated with the old token.

(#4324)
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from c0205c9 to 00091f9 Compare July 15, 2025 13:22
@sergeibbb sergeibbb added the needs-verification Request for verification label Jul 15, 2025
sergeibbb and others added 6 commits July 16, 2025 15:37
- The `handleProviderException` method in `IntegrationBase` is updated to accept an options object instead of individual parameters.
- The method now only returns void, requiring the calling functions to return the default value themselves.

(#4324)
Resets the request exception count when a new access token is obtained after refreshing the session.
This prevents the integration from being prematurely disconnected due to request errors associated with the old token.

(#4324)
@sergeibbb sergeibbb force-pushed the sync-cloud-integration-on-auth-problems branch from 00091f9 to aaa9497 Compare July 16, 2025 13:40
@sergeibbb sergeibbb merged commit ba39cd8 into main Jul 16, 2025
3 checks passed
sergeibbb added a commit that referenced this pull request Jul 16, 2025
sergeibbb added a commit that referenced this pull request Jul 16, 2025
- The `handleProviderException` method in `IntegrationBase` is updated to accept an options object instead of individual parameters.
- The method now only returns void, requiring the calling functions to return the default value themselves.

(#4324)
@sergeibbb sergeibbb deleted the sync-cloud-integration-on-auth-problems branch July 16, 2025 13:57
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
- The `handleProviderException` method in `IntegrationBase` is updated to accept an options object instead of individual parameters.
- The method now only returns void, requiring the calling functions to return the default value themselves.

(gitkraken#4324)
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
wlnxing pushed a commit to wlnxing/vscode-gitlens that referenced this pull request Aug 19, 2025
Resets the request exception count when a new access token is obtained after refreshing the session.
This prevents the integration from being prematurely disconnected due to request errors associated with the old token.

(gitkraken#4324)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues or features related to integrations needs-verification Request for verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants