Fix: Allow partial integration results when some providers have authentication failures

src/cache.ts

@@ -11,6 +11,7 @@ import type { ResourceDescriptor } from './git/models/resourceDescriptor';
 import type { GitHostIntegration } from './plus/integrations/models/gitHostIntegration';
 import type { IntegrationBase } from './plus/integrations/models/integration';
 import { isPromise } from './system/promise';
+import { CacheController } from './system/promiseCache';
 
 type Caches = {
 	defaultBranch: { key: `repo:${string}`; value: DefaultBranch };
@@ -31,7 +32,7 @@ type CacheKey<T extends Cache> = Caches[T]['key'];
 type CacheValue<T extends Cache> = Caches[T]['value'];
 type CacheResult<T> = Promise<T | undefined> | T | undefined;
 
-type Cacheable<T> = () => { value: CacheResult<T>; expiresAt?: number };
+type Cacheable<T> = (cacheable: CacheController) => { value: CacheResult<T>; expiresAt?: number };
 type Cached<T> =
 	| {
 			value: T | undefined;
@@ -46,6 +47,8 @@ type Cached<T> =
 			etag?: string;
 	  };
 
+type ExpiryOptions = { expiryOverride?: boolean | number; expireOnError?: boolean };
+
 export class CacheProvider implements Disposable {
 	private readonly _cache = new Map<`${Cache}:${CacheKey<Cache>}`, Cached<CacheResult<CacheValue<Cache>>>>();
 
@@ -65,7 +68,7 @@ export class CacheProvider implements Disposable {
 		key: CacheKey<T>,
 		etag: string | undefined,
 		cacheable: Cacheable<CacheValue<T>>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<CacheValue<T>> {
 		const item = this._cache.get(`${cache}:${key}`);
 
@@ -85,8 +88,18 @@ export class CacheProvider implements Disposable {
 			(expiry != null && expiry > 0 && expiry < Date.now()) ||
 			(item.etag != null && item.etag !== etag)
 		) {
-			const { value, expiresAt } = cacheable();
-			return this.set<T>(cache, key, value, etag, expiresAt)?.value as CacheResult<CacheValue<T>>;
+			const cacheController = new CacheController();
+			const { value, expiresAt } = cacheable(cacheController);
+			if (isPromise(value)) {
+				void value.finally(() => {
+					if (cacheController.invalidated) {
+						this.delete(cache, key);
+					}
+				});
+			}
+			return this.set<T>(cache, key, value, etag, expiresAt, options?.expireOnError)?.value as CacheResult<
+				CacheValue<T>
+			>;
 		}
 
 		return item.value as CacheResult<CacheValue<T>>;
@@ -95,7 +108,7 @@ export class CacheProvider implements Disposable {
 	getCurrentAccount(
 		integration: IntegrationBase,
 		cacheable: Cacheable<Account>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<Account> {
 		const { key, etag } = getIntegrationKeyAndEtag(integration);
 		return this.get('currentAccount', `id:${key}`, etag, cacheable, options);
@@ -117,7 +130,7 @@ export class CacheProvider implements Disposable {
 		resource: ResourceDescriptor,
 		integration: IntegrationBase | undefined,
 		cacheable: Cacheable<IssueOrPullRequest>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<IssueOrPullRequest> {
 		const { key, etag } = getResourceKeyAndEtag(resource, integration);
 
@@ -138,7 +151,7 @@ export class CacheProvider implements Disposable {
 		resource: ResourceDescriptor,
 		integration: IntegrationBase | undefined,
 		cacheable: Cacheable<Issue>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<Issue> {
 		const { key, etag } = getResourceKeyAndEtag(resource, integration);
 
@@ -165,7 +178,7 @@ export class CacheProvider implements Disposable {
 		resource: ResourceDescriptor,
 		integration: IntegrationBase | undefined,
 		cacheable: Cacheable<PullRequest>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<PullRequest> {
 		const { key, etag } = getResourceKeyAndEtag(resource, integration);
 
@@ -192,7 +205,7 @@ export class CacheProvider implements Disposable {
 		repo: ResourceDescriptor,
 		integration: GitHostIntegration | undefined,
 		cacheable: Cacheable<PullRequest>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<PullRequest> {
 		const { key, etag } = getResourceKeyAndEtag(repo, integration);
 		// Wrap the cacheable so we can also add the result to the issuesOrPrsById cache
@@ -210,7 +223,7 @@ export class CacheProvider implements Disposable {
 		repo: ResourceDescriptor,
 		integration: GitHostIntegration | undefined,
 		cacheable: Cacheable<PullRequest>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<PullRequest> {
 		const { key, etag } = getResourceKeyAndEtag(repo, integration);
 		// Wrap the cacheable so we can also add the result to the issuesOrPrsById cache
@@ -227,7 +240,7 @@ export class CacheProvider implements Disposable {
 		repo: ResourceDescriptor,
 		integration: GitHostIntegration | undefined,
 		cacheable: Cacheable<DefaultBranch>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<DefaultBranch> {
 		const { key, etag } = getResourceKeyAndEtag(repo, integration);
 		return this.get('defaultBranch', `repo:${key}`, etag, cacheable, options);
@@ -237,7 +250,7 @@ export class CacheProvider implements Disposable {
 		repo: ResourceDescriptor,
 		integration: GitHostIntegration | undefined,
 		cacheable: Cacheable<RepositoryMetadata>,
-		options?: { expiryOverride?: boolean | number },
+		options?: ExpiryOptions,
 	): CacheResult<RepositoryMetadata> {
 		const { key, etag } = getResourceKeyAndEtag(repo, integration);
 		return this.get('repoMetadata', `repo:${key}`, etag, cacheable, options);
@@ -249,17 +262,14 @@ export class CacheProvider implements Disposable {
 		value: CacheResult<CacheValue<T>>,
 		etag: string | undefined,
 		expiresAt?: number,
+		expireOnError?: boolean,
 	): Cached<CacheResult<CacheValue<T>>> {
 		let item: Cached<CacheResult<CacheValue<T>>>;
 		if (isPromise(value)) {
-			void value.then(
-				v => {
-					this.set(cache, key, v, etag, expiresAt);
-				},
-				() => {
-					this.delete(cache, key);
-				},
-			);
+			void value.then(v => this.set(cache, key, v, etag, expiresAt, expireOnError));
+			if (expireOnError !== false) {
+				void value.catch(() => this.delete(cache, key));
+			}
 
 			item = { value: value, etag: etag, cachedAt: Date.now() };
 		} else {
@@ -280,8 +290,8 @@ export class CacheProvider implements Disposable {
 		key: string,
 		etag: string | undefined,
 	): Cacheable<PullRequest> {
-		return () => {
-			const item = cacheable();
+		return cacheController => {
+			const item = cacheable(cacheController);
 			if (isPromise(item.value)) {
 				void item.value.then(v => {
 					if (v != null) {

src/plus/integrations/integrationService.ts

@@ -792,29 +792,29 @@ export class IntegrationService implements Disposable {
 		}
 
 		const results = await Promise.allSettled(promises);
-
+		const successfulResults = [
+			...flatten(
+				filterMap(results, r =>
+					r.status === 'fulfilled' && r.value?.value != null ? r.value.value : undefined,
+				),
+			),
+		];
 		const errors = [
 			...filterMap(results, r =>
 				r.status === 'fulfilled' && r.value?.error != null ? r.value.error : undefined,
 			),
 		];
-		if (errors.length) {
-			return {
-				error: errors.length === 1 ? errors[0] : new AggregateError(errors),
-				duration: Date.now() - start,
-			};
-		}
+
+		const error =
+			errors.length === 0
+				? undefined
+				: errors.length === 1
+					? errors[0]
+					: new AggregateError(errors, 'Failed to get some pull requests');
 
 		return {
-			value: [
-				...flatten(
-					filterMap(results, r =>
-						r.status === 'fulfilled' && r.value != null && r.value?.error == null
-							? r.value.value
-							: undefined,
-					),
-				),
-			],
+			value: successfulResults,
+			error: error,
 			duration: Date.now() - start,
 		};
 	}

src/plus/integrations/models/integration.ts

@@ -46,7 +46,7 @@ export type IntegrationKey<T extends IntegrationIds = IntegrationIds> = T extend
 export type IntegrationConnectedKey<T extends IntegrationIds = IntegrationIds> = `connected:${IntegrationKey<T>}`;
 
 export type IntegrationResult<T> =
-	| { value: T; duration?: number; error?: never }
+	| { value: T; duration?: number; error?: Error }
 	| { error: Error; duration?: number; value?: never }
 	| undefined;
 
@@ -600,19 +600,31 @@ export abstract class IntegrationBase<
 
 		const currentAccount = await this.container.cache.getCurrentAccount(
 			this,
-			() => ({
+			cacheable => ({
 				value: (async () => {
 					try {
 						const account = await this.getProviderCurrentAccount?.(this._session!, opts);
 						this.resetRequestExceptionCount('getCurrentAccount');
 						return account;
 					} catch (ex) {
+						if (ex instanceof CancellationError) {
+							cacheable.invalidate();
+							return undefined;
+						}
+
 						this.handleProviderException('getCurrentAccount', ex, { scope: scope });
-						return undefined;
+
+						// Invalidate the cache on error, except for auth errors
+						if (!(ex instanceof AuthenticationError)) {
+							cacheable.invalidate();
+						}
+
+						// Re-throw to the caller
+						throw ex;
 					}
 				})(),
 			}),
-			{ expiryOverride: expiryOverride },
+			{ expiryOverride: expiryOverride, expireOnError: false },
 		);
 		return currentAccount;
 	}

src/plus/launchpad/launchpad.ts

@@ -41,6 +41,7 @@ import type { IntegrationIds } from '../../constants.integrations';
 import { GitCloudHostIntegrationId, GitSelfManagedHostIntegrationId } from '../../constants.integrations';
 import type { LaunchpadTelemetryContext, Source, Sources, TelemetryEvents } from '../../constants.telemetry';
 import type { Container } from '../../container';
+import { AuthenticationError } from '../../errors';
 import type { QuickPickItemOfT } from '../../quickpicks/items/common';
 import { createQuickPickItemOfT, createQuickPickSeparator } from '../../quickpicks/items/common';
 import type { DirectiveQuickPickItem } from '../../quickpicks/items/directive';
@@ -52,6 +53,8 @@ import { openUrl } from '../../system/-webview/vscode/uris';
 import { getScopedCounter } from '../../system/counter';
 import { fromNow } from '../../system/date';
 import { some } from '../../system/iterable';
+import { Logger } from '../../system/logger';
+import { AggregateError } from '../../system/promise';
 import { interpolate, pluralize } from '../../system/string';
 import { ProviderBuildStatusState, ProviderPullRequestReviewState } from '../integrations/providers/models';
 import type { LaunchpadCategorizedResult, LaunchpadItem } from './launchpadProvider';
@@ -157,6 +160,11 @@ const instanceCounter = getScopedCounter();
 
 const defaultCollapsedGroups: LaunchpadGroup[] = ['draft', 'other', 'snoozed'];
 
+const OpenLogsQuickInputButton: QuickInputButton = {
+	iconPath: new ThemeIcon('output'),
+	tooltip: 'Open Logs',
+};
+
 export class LaunchpadCommand extends QuickCommand<State> {
 	private readonly source: Source;
 	private readonly telemetryContext: LaunchpadTelemetryContext | undefined;
@@ -565,10 +573,10 @@ export class LaunchpadCommand extends QuickCommand<State> {
 			return groupedAndSorted;
 		};
 
-		function getItemsAndQuickpickProps(isFiltering?: boolean) {
+		const getItemsAndQuickpickProps = (isFiltering?: boolean) => {
 			const result = context.inSearch ? context.searchResult : context.result;
 
-			if (result?.error != null) {
+			if (result?.error != null && !result?.items?.length) {
 				return {
 					title: `${context.title} \u00a0\u2022\u00a0 Unable to Load Items`,
 					placeholder: `Unable to load items (${
@@ -582,7 +590,7 @@ export class LaunchpadCommand extends QuickCommand<State> {
 				};
 			}
 
-			if (!result?.items.length) {
+			if (!result?.items?.length) {
 				if (context.inSearch === 'mode') {
 					return {
 						title: `Search For Pull Request \u00a0\u2022\u00a0 ${context.title}`,
@@ -616,6 +624,11 @@ export class LaunchpadCommand extends QuickCommand<State> {
 			}
 
 			const items = getLaunchpadQuickPickItems(result.items, isFiltering);
+
+			// Add error information item if there's an error but items were still loaded
+			const errorItem: DirectiveQuickPickItem | undefined =
+				result?.error != null ? this.createErrorQuickPickItem(result.error) : undefined;
+
 			const hasPicked = items.some(i => i.picked);
 			if (context.inSearch === 'mode') {
 				const offItem: ToggleSearchModeQuickPickItem = {
@@ -630,7 +643,9 @@ export class LaunchpadCommand extends QuickCommand<State> {
 				return {
 					title: `Search For Pull Request \u00a0\u2022\u00a0 ${context.title}`,
 					placeholder: 'Enter a term to search for a pull request to act on',
-					items: isFiltering ? [...items, offItem] : [offItem, ...items],
+					items: isFiltering
+						? [...(errorItem != null ? [errorItem] : []), ...items, offItem]
+						: [offItem, ...(errorItem != null ? [errorItem] : []), ...items],
 				};
 			}
 
@@ -646,10 +661,14 @@ export class LaunchpadCommand extends QuickCommand<State> {
 				title: context.title,
 				placeholder: 'Choose a pull request or paste a pull request URL to act on',
 				items: isFiltering
-					? [...items, onItem]
-					: [onItem, ...getLaunchpadQuickPickItems(result.items, isFiltering)],
+					? [...(errorItem != null ? [errorItem] : []), ...items, onItem]
+					: [
+							onItem,
+							...(errorItem != null ? [errorItem] : []),
+							...getLaunchpadQuickPickItems(result.items, isFiltering),
+						],
 			};
-		}
+		};
 
 		const updateItems = async (
 			quickpick: QuickPick<
@@ -830,6 +849,16 @@ export class LaunchpadCommand extends QuickCommand<State> {
 					return;
 				}
 
+				if (button === OpenLogsQuickInputButton) {
+					Logger.showOutputChannel();
+					return;
+				}
+
+				if (button === ConnectIntegrationButton) {
+					await this.container.integrations.manageCloudIntegrations({ source: 'launchpad' });
+					return;
+				}
+
 				if (!item) return;
 
 				switch (button) {
@@ -1403,6 +1432,25 @@ export class LaunchpadCommand extends QuickCommand<State> {
 			this.source,
 		);
 	}
+
+	private createErrorQuickPickItem(error: Error): DirectiveQuickPickItem {
+		if (error instanceof AggregateError) {
+			const firstAuthError = error.errors.find(e => e instanceof AuthenticationError);
+			error = firstAuthError ?? error.errors[0] ?? error;
+		}
+
+		const isAuthError = error instanceof AuthenticationError;
+
+		return createDirectiveQuickPickItem(Directive.Noop, false, {
+			label: isAuthError ? '$(warning) Authentication Required' : '$(warning) Unable to fully load items',
+			detail: isAuthError
+				? `${String(error)} — Click to reconnect your integration`
+				: error.name === 'HttpError' && 'status' in error && typeof error.status === 'number'
+					? `${error.status}: ${String(error)}`
+					: String(error),
+			buttons: isAuthError ? [ConnectIntegrationButton, OpenLogsQuickInputButton] : [OpenLogsQuickInputButton],
+		});
+	}
 }
 
 function getLaunchpadItemInformationRows(
@@ -1657,10 +1705,10 @@ function updateTelemetryContext(context: Context) {
 	if (context.telemetryContext == null) return;
 
 	let updatedContext: NonNullable<(typeof context)['telemetryContext']>;
-	if (context.result.error != null) {
+	if (context.result.error != null || !context.result.items) {
 		updatedContext = {
 			...context.telemetryContext,
-			'items.error': String(context.result.error),
+			'items.error': String(context.result.error ?? 'items not loaded'),
 		};
 	} else {
 		const grouped = countLaunchpadItemGroups(context.result.items);