From ec80cf8cd89b49e527fea0c6fde009699ece98b0 Mon Sep 17 00:00:00 2001 From: Christof Marti Date: Tue, 25 Aug 2026 09:50:11 +0200 Subject: [PATCH 1/2] Retry current user request after failure Clear failed current-user and EMU promise caches so later GitHub operations can retry instead of replaying an activation-time network error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../GitHubContactServiceProvider.ts | 2 +- src/github/credentials.ts | 38 ++++++++++-- src/github/githubRepository.ts | 2 +- src/test/github/credentials.test.ts | 58 ++++++++++++++++++- 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/src/gitProviders/GitHubContactServiceProvider.ts b/src/gitProviders/GitHubContactServiceProvider.ts index bc3973cfef..1d88f69a8e 100644 --- a/src/gitProviders/GitHubContactServiceProvider.ts +++ b/src/gitProviders/GitHubContactServiceProvider.ts @@ -125,7 +125,7 @@ export class GitHubContactServiceProvider implements ContactServiceProvider { } const origin = await this.pullRequestManager.folderManagers[0]?.getOrigin(); if (origin) { - const currentUser = origin.hub.currentUser ? await origin.hub.currentUser : undefined; + const currentUser = await origin.getAuthenticatedUser(); if (currentUser) { return currentUser.login; } diff --git a/src/github/credentials.ts b/src/github/credentials.ts index e1e9ae9ee1..aea05aaf1f 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -551,19 +551,25 @@ export class CredentialStore extends Disposable { } public async isCurrentUser(authProviderId: AuthProvider, username: string): Promise { - const api = authProviderId === AuthProvider.github ? this._githubAPI : this._githubEnterpriseAPI; - return (await api?.currentUser)?.login === username; + return (await this.getCurrentUser(authProviderId))?.login === username; } public async getIsEmu(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); + this.ensureCurrentUser(github); return !!(await github?.isEmu); } public getCurrentUser(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - const octokit = github?.octokit; - return (octokit && github?.currentUser)!; + this.ensureCurrentUser(github); + return github?.currentUser!; + } + + private ensureCurrentUser(github: GitHub | undefined): void { + if (github && (!github.currentUser || !github.isEmu)) { + this.setCurrentUser(github); + } } private setCurrentUser(github: GitHub): void { @@ -577,8 +583,28 @@ export class CredentialStore extends Disposable { reject(e); }); }); - github.currentUser = getUser.then(result => convertRESTUserToAccount(result.data)); - github.isEmu = getUser.then(result => result.data.plan?.name === 'emu_user'); + let currentUser: Promise; + let isEmu: Promise; + const clearFailedRequest = () => { + if (github.currentUser === currentUser && github.isEmu === isEmu) { + github.currentUser = undefined; + github.isEmu = undefined; + } + }; + currentUser = getUser.then(result => convertRESTUserToAccount(result.data), e => { + clearFailedRequest(); + throw e; + }); + isEmu = getUser.then(result => result.data.plan?.name === 'emu_user', e => { + clearFailedRequest(); + throw e; + }); + github.currentUser = currentUser; + github.isEmu = isEmu; + + // Both promises share the same request, but callers may only observe one of them. + void currentUser.catch(() => undefined); + void isEmu.catch(() => undefined); } private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fb6abd413d..cd825eae00 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -424,7 +424,7 @@ export class GitHubRepository extends Disposable { repo }); Logger.debug(`Fetch metadata for repo ${owner}/${repo} - done`, this.id); - const metadata = { ...result.data, currentUser: await this._hub?.currentUser }; + const metadata = { ...result.data, currentUser: await this.getAuthenticatedUser() }; return metadata; } diff --git a/src/test/github/credentials.test.ts b/src/test/github/credentials.test.ts index 14718f7e4b..5e820d8579 100644 --- a/src/test/github/credentials.test.ts +++ b/src/test/github/credentials.test.ts @@ -3,10 +3,15 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { strictEqual, deepStrictEqual } from 'assert'; +import { strictEqual, deepStrictEqual, rejects } from 'assert'; +import { Octokit } from '@octokit/rest'; +import { createSandbox, SinonSandbox } from 'sinon'; import * as vscode from 'vscode'; import { AuthProvider } from '../../common/authentication'; -import { findExistingSession } from '../../github/credentials'; +import { CredentialStore, findExistingSession, GitHub } from '../../github/credentials'; +import { LoggingApolloClient, LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; +import { MockTelemetry } from '../mocks/mockTelemetry'; const oldestScopes = ['read:user', 'user:email', 'repo']; const defaultScopes = [...oldestScopes, 'workflow']; @@ -29,6 +34,16 @@ function createSession(id: string, accountId: string, scopes: string[]): vscode. } describe('CredentialStore', function () { + let sinon: SinonSandbox; + + beforeEach(function () { + sinon = createSandbox(); + }); + + afterEach(function () { + sinon.restore(); + }); + describe('findExistingSession', function () { it('keeps broader scope lookup on the preferred account', async function () { const firstAccountAdditional = createSession('first-additional', 'first', additionalScopes); @@ -99,4 +114,43 @@ describe('CredentialStore', function () { deepStrictEqual(additionalResult?.scopes, additionalScopes); }); }); + + it('retries the current user request after a failure', async function () { + const telemetry = new MockTelemetry(); + const credentialStore = new CredentialStore(telemetry, new MockExtensionContext()); + const github: GitHub = { + octokit: new LoggingOctokit(new Octokit(), new RateLogger(telemetry, false)), + graphql: {} as LoggingApolloClient, + }; + sinon.stub(credentialStore, 'getHub').returns(github); + const getAuthenticatedUser = sinon.stub(github.octokit, 'call'); + const error = new Error('Connect Timeout Error'); + getAuthenticatedUser.onFirstCall().rejects(error); + getAuthenticatedUser.onSecondCall().resolves({ + data: { + login: 'octocat', + node_id: 'MDQ6VXNlcjE=', + html_url: 'https://github.com/octocat', + avatar_url: 'https://github.com/images/error/octocat_happy.gif', + type: 'User', + plan: { name: 'emu_user' }, + } + }); + + await rejects(credentialStore.getCurrentUser(AuthProvider.github), candidate => candidate === error); + const [currentUser, isEmu] = await Promise.all([ + credentialStore.getCurrentUser(AuthProvider.github), + credentialStore.getIsEmu(AuthProvider.github), + ]); + + deepStrictEqual({ + requests: getAuthenticatedUser.callCount, + login: currentUser.login, + isEmu, + }, { + requests: 2, + login: 'octocat', + isEmu: true, + }); + }); }); From cb0cb669f144fa6d9c3d7db9010e380979f4933a Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:34:10 +0200 Subject: [PATCH 2/2] Don't save failed promise --- src/github/credentials.ts | 76 +++++++++++++---------------- src/test/github/credentials.test.ts | 16 +++++- 2 files changed, 49 insertions(+), 43 deletions(-) diff --git a/src/github/credentials.ts b/src/github/credentials.ts index aea05aaf1f..9abe709905 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -80,8 +80,8 @@ export async function findExistingSession( export interface GitHub { octokit: LoggingOctokit; graphql: LoggingApolloClient; - currentUser?: Promise; - isEmu?: Promise; + currentUser?: IAccount; + isEmu?: boolean; } interface AuthResult { @@ -102,6 +102,7 @@ export class CredentialStore extends Disposable { private _isSamling: boolean = false; private _handlingAuthError: Map> = new Map(); private _lastAuthErrorHandledAt: Map = new Map(); + private _currentUserRequests: WeakMap> = new WeakMap(); // Cooldown long enough to absorb retries from in-flight requests that were // issued with the now-invalid token, but short enough that a token that // is invalidated again soon after re-auth will still trigger another prompt. @@ -556,55 +557,48 @@ export class CredentialStore extends Disposable { public async getIsEmu(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - this.ensureCurrentUser(github); - return !!(await github?.isEmu); + await this.ensureCurrentUser(github); + return !!github?.isEmu; } - public getCurrentUser(authProviderId: AuthProvider): Promise { + public async getCurrentUser(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - this.ensureCurrentUser(github); + await this.ensureCurrentUser(github); return github?.currentUser!; } - private ensureCurrentUser(github: GitHub | undefined): void { - if (github && (!github.currentUser || !github.isEmu)) { - this.setCurrentUser(github); + private ensureCurrentUser(github: GitHub | undefined): Promise { + if (!github || (github.currentUser && github.isEmu !== undefined)) { + return Promise.resolve(); } - } - private setCurrentUser(github: GitHub): void { - const getUser: ReturnType = new Promise((resolve, reject) => { - Logger.debug('Getting current user', CredentialStore.ID); - github.octokit.call(github.octokit.api.users.getAuthenticated, {}).then(result => { - Logger.debug(`Got current user ${result.data.login}`, CredentialStore.ID); - resolve(result); - }).catch(e => { - Logger.error(`Failed to get current user: ${e}, ${e.message}`, CredentialStore.ID); - reject(e); - }); - }); - let currentUser: Promise; - let isEmu: Promise; - const clearFailedRequest = () => { - if (github.currentUser === currentUser && github.isEmu === isEmu) { - github.currentUser = undefined; - github.isEmu = undefined; + const existingRequest = this._currentUserRequests.get(github); + if (existingRequest) { + return existingRequest; + } + + const request = this.setCurrentUser(github); + this._currentUserRequests.set(github, request); + const clearRequest = () => { + if (this._currentUserRequests.get(github) === request) { + this._currentUserRequests.delete(github); } }; - currentUser = getUser.then(result => convertRESTUserToAccount(result.data), e => { - clearFailedRequest(); - throw e; - }); - isEmu = getUser.then(result => result.data.plan?.name === 'emu_user', e => { - clearFailedRequest(); - throw e; - }); - github.currentUser = currentUser; - github.isEmu = isEmu; + void request.then(clearRequest, clearRequest); + return request; + } - // Both promises share the same request, but callers may only observe one of them. - void currentUser.catch(() => undefined); - void isEmu.catch(() => undefined); + private async setCurrentUser(github: GitHub): Promise { + Logger.debug('Getting current user', CredentialStore.ID); + try { + const result = await github.octokit.call(github.octokit.api.users.getAuthenticated, {}); + Logger.debug(`Got current user ${result.data.login}`, CredentialStore.ID); + github.currentUser = convertRESTUserToAccount(result.data); + github.isEmu = result.data.plan?.name === 'emu_user'; + } catch (e) { + Logger.error(`Failed to get current user: ${e}, ${e.message}`, CredentialStore.ID); + throw e; + } } private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { @@ -680,7 +674,7 @@ export class CredentialStore extends Disposable { octokit: new LoggingOctokit(octokit, rateLogger), graphql: new LoggingApolloClient(graphql, rateLogger), }; - this.setCurrentUser(github); + void this.ensureCurrentUser(github).catch(() => undefined); return github; } } diff --git a/src/test/github/credentials.test.ts b/src/test/github/credentials.test.ts index 5e820d8579..e6163eac31 100644 --- a/src/test/github/credentials.test.ts +++ b/src/test/github/credentials.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { strictEqual, deepStrictEqual, rejects } from 'assert'; +import { strictEqual, deepStrictEqual } from 'assert'; import { Octokit } from '@octokit/rest'; import { createSandbox, SinonSandbox } from 'sinon'; import * as vscode from 'vscode'; @@ -137,20 +137,32 @@ describe('CredentialStore', function () { } }); - await rejects(credentialStore.getCurrentUser(AuthProvider.github), candidate => candidate === error); + deepStrictEqual(await Promise.allSettled([ + credentialStore.getCurrentUser(AuthProvider.github), + credentialStore.getIsEmu(AuthProvider.github), + ]), [ + { status: 'rejected', reason: error }, + { status: 'rejected', reason: error }, + ]); + strictEqual(getAuthenticatedUser.callCount, 1); + strictEqual(github.currentUser, undefined); + strictEqual(github.isEmu, undefined); const [currentUser, isEmu] = await Promise.all([ credentialStore.getCurrentUser(AuthProvider.github), credentialStore.getIsEmu(AuthProvider.github), ]); + const cachedCurrentUser = await credentialStore.getCurrentUser(AuthProvider.github); deepStrictEqual({ requests: getAuthenticatedUser.callCount, login: currentUser.login, isEmu, + cachedLogin: cachedCurrentUser.login, }, { requests: 2, login: 'octocat', isEmu: true, + cachedLogin: 'octocat', }); }); });