Skip to content

Commit ec80cf8

Browse files
chrmartiCopilot
andcommitted
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>
1 parent 849821a commit ec80cf8

4 files changed

Lines changed: 90 additions & 10 deletions

File tree

src/gitProviders/GitHubContactServiceProvider.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ export class GitHubContactServiceProvider implements ContactServiceProvider {
125125
}
126126
const origin = await this.pullRequestManager.folderManagers[0]?.getOrigin();
127127
if (origin) {
128-
const currentUser = origin.hub.currentUser ? await origin.hub.currentUser : undefined;
128+
const currentUser = await origin.getAuthenticatedUser();
129129
if (currentUser) {
130130
return currentUser.login;
131131
}

src/github/credentials.ts

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -551,19 +551,25 @@ export class CredentialStore extends Disposable {
551551
}
552552

553553
public async isCurrentUser(authProviderId: AuthProvider, username: string): Promise<boolean> {
554-
const api = authProviderId === AuthProvider.github ? this._githubAPI : this._githubEnterpriseAPI;
555-
return (await api?.currentUser)?.login === username;
554+
return (await this.getCurrentUser(authProviderId))?.login === username;
556555
}
557556

558557
public async getIsEmu(authProviderId: AuthProvider): Promise<boolean> {
559558
const github = this.getHub(authProviderId);
559+
this.ensureCurrentUser(github);
560560
return !!(await github?.isEmu);
561561
}
562562

563563
public getCurrentUser(authProviderId: AuthProvider): Promise<IAccount> {
564564
const github = this.getHub(authProviderId);
565-
const octokit = github?.octokit;
566-
return (octokit && github?.currentUser)!;
565+
this.ensureCurrentUser(github);
566+
return github?.currentUser!;
567+
}
568+
569+
private ensureCurrentUser(github: GitHub | undefined): void {
570+
if (github && (!github.currentUser || !github.isEmu)) {
571+
this.setCurrentUser(github);
572+
}
567573
}
568574

569575
private setCurrentUser(github: GitHub): void {
@@ -577,8 +583,28 @@ export class CredentialStore extends Disposable {
577583
reject(e);
578584
});
579585
});
580-
github.currentUser = getUser.then(result => convertRESTUserToAccount(result.data));
581-
github.isEmu = getUser.then(result => result.data.plan?.name === 'emu_user');
586+
let currentUser: Promise<IAccount>;
587+
let isEmu: Promise<boolean>;
588+
const clearFailedRequest = () => {
589+
if (github.currentUser === currentUser && github.isEmu === isEmu) {
590+
github.currentUser = undefined;
591+
github.isEmu = undefined;
592+
}
593+
};
594+
currentUser = getUser.then(result => convertRESTUserToAccount(result.data), e => {
595+
clearFailedRequest();
596+
throw e;
597+
});
598+
isEmu = getUser.then(result => result.data.plan?.name === 'emu_user', e => {
599+
clearFailedRequest();
600+
throw e;
601+
});
602+
github.currentUser = currentUser;
603+
github.isEmu = isEmu;
604+
605+
// Both promises share the same request, but callers may only observe one of them.
606+
void currentUser.catch(() => undefined);
607+
void isEmu.catch(() => undefined);
582608
}
583609

584610
private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> {

src/github/githubRepository.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,7 @@ export class GitHubRepository extends Disposable {
424424
repo
425425
});
426426
Logger.debug(`Fetch metadata for repo ${owner}/${repo} - done`, this.id);
427-
const metadata = { ...result.data, currentUser: await this._hub?.currentUser };
427+
const metadata = { ...result.data, currentUser: await this.getAuthenticatedUser() };
428428
return metadata;
429429
}
430430

src/test/github/credentials.test.ts

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,15 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { strictEqual, deepStrictEqual } from 'assert';
6+
import { strictEqual, deepStrictEqual, rejects } from 'assert';
7+
import { Octokit } from '@octokit/rest';
8+
import { createSandbox, SinonSandbox } from 'sinon';
79
import * as vscode from 'vscode';
810
import { AuthProvider } from '../../common/authentication';
9-
import { findExistingSession } from '../../github/credentials';
11+
import { CredentialStore, findExistingSession, GitHub } from '../../github/credentials';
12+
import { LoggingApolloClient, LoggingOctokit, RateLogger } from '../../github/loggingOctokit';
13+
import { MockExtensionContext } from '../mocks/mockExtensionContext';
14+
import { MockTelemetry } from '../mocks/mockTelemetry';
1015

1116
const oldestScopes = ['read:user', 'user:email', 'repo'];
1217
const defaultScopes = [...oldestScopes, 'workflow'];
@@ -29,6 +34,16 @@ function createSession(id: string, accountId: string, scopes: string[]): vscode.
2934
}
3035

3136
describe('CredentialStore', function () {
37+
let sinon: SinonSandbox;
38+
39+
beforeEach(function () {
40+
sinon = createSandbox();
41+
});
42+
43+
afterEach(function () {
44+
sinon.restore();
45+
});
46+
3247
describe('findExistingSession', function () {
3348
it('keeps broader scope lookup on the preferred account', async function () {
3449
const firstAccountAdditional = createSession('first-additional', 'first', additionalScopes);
@@ -99,4 +114,43 @@ describe('CredentialStore', function () {
99114
deepStrictEqual(additionalResult?.scopes, additionalScopes);
100115
});
101116
});
117+
118+
it('retries the current user request after a failure', async function () {
119+
const telemetry = new MockTelemetry();
120+
const credentialStore = new CredentialStore(telemetry, new MockExtensionContext());
121+
const github: GitHub = {
122+
octokit: new LoggingOctokit(new Octokit(), new RateLogger(telemetry, false)),
123+
graphql: {} as LoggingApolloClient,
124+
};
125+
sinon.stub(credentialStore, 'getHub').returns(github);
126+
const getAuthenticatedUser = sinon.stub(github.octokit, 'call');
127+
const error = new Error('Connect Timeout Error');
128+
getAuthenticatedUser.onFirstCall().rejects(error);
129+
getAuthenticatedUser.onSecondCall().resolves({
130+
data: {
131+
login: 'octocat',
132+
node_id: 'MDQ6VXNlcjE=',
133+
html_url: 'https://github.com/octocat',
134+
avatar_url: 'https://github.com/images/error/octocat_happy.gif',
135+
type: 'User',
136+
plan: { name: 'emu_user' },
137+
}
138+
});
139+
140+
await rejects(credentialStore.getCurrentUser(AuthProvider.github), candidate => candidate === error);
141+
const [currentUser, isEmu] = await Promise.all([
142+
credentialStore.getCurrentUser(AuthProvider.github),
143+
credentialStore.getIsEmu(AuthProvider.github),
144+
]);
145+
146+
deepStrictEqual({
147+
requests: getAuthenticatedUser.callCount,
148+
login: currentUser.login,
149+
isEmu,
150+
}, {
151+
requests: 2,
152+
login: 'octocat',
153+
isEmu: true,
154+
});
155+
});
102156
});

0 commit comments

Comments
 (0)