microsoft / microsoft/vscode-pull-request-github
Transient connect timeout on GET /user (or repo metadata) permanently breaks PR overview until reload: rejected promises are cached (setCurrentUser / getMetadata)
- Lingua principale
- TypeScript
- Stelle
- 2.6k
- Fork
- 795
- Merge medio
- 1g 4h
- PR unite (30g)
- 46
Descrizione
Summary
A transient network error (e.g. an Undici/Octokit UND_ERR_CONNECT_TIMEOUT on GET /user) that occurs once during authentication/current-user or repository-metadata initialization becomes a permanent failure until the extension host (or window) is reloaded, even after connectivity is restored. This is because several code paths cache the rejected promise itself as the "result", and every later caller reuses that already-rejected promise instead of retrying.
This was observed while opening a PR overview: the underlying failing request was GET https://api.github.com/user (from users.getAuthenticated), which failed once with a nested UND_ERR_CONNECT_TIMEOUT. After that single failure, connectivity was fine, but every subsequent attempt to open a PR overview kept failing with the generic message Error updating pull request description: ... until the window was reloaded. Manually replacing the cached currentUser/isEmu promises and retrying (without reload) fixed it immediately, confirming the promises — not the network — were the persistent problem.
I'm not claiming to know what originally caused the one-off connect timeout (that's environment/network dependent); the bug being reported is that the extension has no recovery path once such a transient rejection is cached.
Root cause 1 — CredentialStore.setCurrentUser caches a rejected getUser promise forever
private setCurrentUser(github: GitHub): void {
const getUser: ReturnType<typeof github.octokit.api.users.getAuthenticated> = 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);
});
});
github.currentUser = getUser.then(result => convertRESTUserToAccount(result.data));
github.isEmu = getUser.then(result => result.data.plan?.name === 'emu_user');
}
setCurrentUser is called exactly once, from createHub, when a GitHub hub is created:
const github: GitHub = {
octokit: new LoggingOctokit(octokit, rateLogger),
graphql: new LoggingApolloClient(graphql, rateLogger),
};
this.setCurrentUser(github);
return github;
github.currentUser and github.isEmu are derived (.then(...)) from the single getUser promise, so if the one underlying GET /user request rejects (any reason — here a transient UND_ERR_CONNECT_TIMEOUT), both github.currentUser and github.isEmu become permanently rejected promises for the lifetime of that GitHub hub instance.
CredentialStore.getCurrentUser / getIsEmu simply return the same cached (possibly rejected) promises every time, with no re-fetch or retry:
public async getIsEmu(authProviderId: AuthProvider): Promise<boolean> {
const github = this.getHub(authProviderId);
return !!(await github?.isEmu);
}
public getCurrentUser(authProviderId: AuthProvider): Promise<IAccount> {
const github = this.getHub(authProviderId);
const octokit = github?.octokit;
return (octokit && github?.currentUser)!;
}
FolderRepositoryManager.getCurrentUser delegates straight to CredentialStore.getCurrentUser, so every later consumer (PR overview, issue overview, comment controllers, assignment quick-picks, etc.) keeps awaiting and re-rejecting on the same stale promise:
https://github.com/microsoft/vscode-pull-request-github/blob/849821a34981608ca9705439e458ffe5527fe480/src/github/folderRepositoryManager.ts (see getCurrentUser)
Root cause 2 — GitHubRepository.getMetadata has the same "cache the rejection" pattern
async getMetadata(): Promise<IMetadata> {
if (this._metadata) {
const metadata = await this._metadata;
Logger.debug(`Using cached metadata ${metadata.owner?.login}/${metadata.name}`, this.id);
return metadata;
}
Logger.debug(`Fetch metadata - enter`, this.id);
const { remote } = await this.ensure();
this._metadata = this.getMetadataForRepo(remote.owner, remote.repositoryName).catch(e => {
if ((getErrorCode(e) === '404') && !isSamlError(e) && !this._isInaccessible) {
this._isInaccessible = true;
Logger.warn(`Repository ${remote.owner}/${remote.repositoryName} from remote ${remote.remoteName} in workspace folder ${this.rootUri.fsPath} returned HTTP 404 and will be skipped for this session.`, this.id);
}
throw e;
});
Logger.debug(`Fetch metadata ${remote.owner}/${remote.repositoryName} - done`, this.id);
return this._metadata;
}
this._metadata (declared at https://github.com/microsoft/vscode-pull-request-github/blob/849821a34981608ca9705439e458ffe5527fe480/src/github/githubRepository.ts#L184) is assigned the promise before it settles, and the if (this._metadata) guard is a truthiness check on the field, not on whether it resolved. If the underlying repos.get call rejects once (same class of transient network error), this._metadata permanently holds a rejected promise and every future getMetadata() call re-awaits (and rethrows from) that same rejection — there is no retry and no way to clear the field.
This directly breaks the two main consumers, which each call getMetadata() and fail every time afterwards:
getDefaultBranch()— https://github.com/microsoft/vscode-pull-request-github/blob/849821a34981608ca9705439e458ffe5527fe480/src/github/githubRepository.ts#L508-L523getRepoAccessAndMergeMethods()— https://github.com/microsoft/vscode-pull-request-github/blob/849821a34981608ca9705439e458ffe5527fe480/src/github/githubRepository.ts#L556-L580
Both catch the error locally (falling back to 'master', or logging a warning), but neither resets this._metadata, so the cached rejection keeps being reused on every subsequent call for the lifetime of the GitHubRepository instance.
Compounding issue — PullRequestOverview.updateItem masks which dependency failed
updateItem awaits a single Promise.all([...17 entries...]) (lines 346–368), including this._folderRepositoryManager.getCurrentUser(pullRequestModel.githubRepository) (line 358) and this._folderRepositoryManager.getPullRequestRepositoryDefaultBranch(pullRequestModel) / getPullRequestRepositoryAccessAndMergeMethods(pullRequestModel) (lines 353, 356, which internally call getMetadata()), all wrapped in one try { ... } catch (e) { vscode.window.showErrorMessage(...) } block:
} catch (e) {
vscode.window.showErrorMessage(`Error updating pull request description: ${formatError(e)}`);
}
This generic message (line 485) is shown regardless of which of the 17 concurrent operations actually failed, so the surfaced error looks like a PR-description-specific problem when the true cause is the unrelated, permanently-cached currentUser (or _metadata) rejection from root causes 1/2.
Verified repro
- Have the extension hit a transient connect timeout on
GET https://api.github.com/userduring hub creation (this call happens once per authenticatedGitHubhub, viaCredentialStore.createHub→setCurrentUser), or onrepos.getduringGitHubRepository.getMetadata(). In our case, the caught error's underlying cause wasUND_ERR_CONNECT_TIMEOUTnested under the Octokit request error forGET /user. - Connectivity recovers immediately afterward (verified other requests to the same host succeed).
- Open (or re-open) a pull request overview.
updateItemfails every time withError updating pull request description: ..., becausegetCurrentUser/getMetadatakeep returning the same cached rejected promise from step 1. - Reloading the window (which recreates
CredentialStore/GitHubRepositoryinstances and thus the cached promises) fixes it — confirming the cache, not the network, is the persistent problem. - As a workaround we confirmed in-session: manually replacing the
GitHubhub'scurrentUser/isEmufields with freshly retried promises (without reloading) also immediately fixed subsequent PR overview loads.
Suggested fixes
CredentialStore/setCurrentUser: make the current-user fetch single-flight but rejection-safe — e.g. store the in-flight promise, and on rejection clear the stored reference only if it still points at the failing promise (to avoid a race with a newer, concurrently-started fetch), so the next caller triggers a freshgetAuthenticatedrequest instead of re-awaiting the stale rejection. CentralizecurrentUserandisEmuso both are derived from one retryable/refreshable "get current user" operation rather than two independent.then()chains hanging off a single one-shot promise.GitHubRepository.getMetadata/_metadata: same pattern — only treat_metadataas a valid cache once it has resolved; on rejection, clear_metadata(guarding against clearing a newer in-flight fetch) so the nextgetDefaultBranch()/getRepoAccessAndMergeMethods()(or any other caller) triggers a real retry instead of reusing the cached failure.PullRequestOverview.updateItem: narrow or tag the error surfaced from the combinedPromise.all(e.g. wrap each entry with context, or reporte?.message/the failing operation name) so failures unrelated to the PR description (e.g. current-user or repo-metadata fetch failures) aren't reported asError updating pull request description, which currently makes the actual root cause hard to diagnose from the visible error alone.
Environment
- Repro is against the current
mainbranch source, commit849821a34981608ca9705439e458ffe5527fe480. - No local machine details, tokens, or private repository paths are included above; the affected endpoint is the public
GET https://api.github.com/usercall made byusers.getAuthenticated.
Guida per i contributori
Nessuna guida per i contributori indicizzata per questo repository
Come iniziare
- Leggi tutta la issue e poi la guida ai contributi del progetto.
- Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
- Fai un fork del repository e lavora su un branch.
- Apri una pull request che faccia riferimento al numero della issue.
Valutazione
Questa issue non è ancora stata valutata.