Skip to content

Commit 811c35b

Browse files
sergeibbbsergeibbb
committed
Controls cache invalidation or persistance on errors
(#4942, #4944)
1 parent 84ba8fb commit 811c35b

File tree

3 files changed

+48
-25
lines changed

3 files changed

+48
-25
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
2727
- Fixes an issue where the GitKraken MCP installation could fail or conflict across remote environments like WSL, SSH, or containers ([#4918](https://github.com/gitkraken/vscode-gitlens/issues/4918))
2828
- Fixes an issue where diffing untracked files in the _Commit Composer_ could trigger unwanted file system events ([#4917](https://github.com/gitkraken/vscode-gitlens/issues/4917))
2929
- Fixes an issue where commit messages become invisible during interactive rebase in some themes ([#4886](https://github.com/gitkraken/vscode-gitlens/issues/4886))
30+
- Fixes an issue where authentication errors were not properly displayed in _Launchpad_ when GitLab integration credentials were revoked ([#4944](https://github.com/gitkraken/vscode-gitlens/issues/4944))
3031
- Fixes issue in the _Commit Graph_ minimap where it only shows a spinner when the repo has no commits ([#4741](https://github.com/gitkraken/vscode-gitlens/issues/4741))
3132
- Fixes an inline markdown rendering issue in the _Interactive Rebase Editor_ ([#4914](https://github.com/gitkraken/vscode-gitlens/issues/4914))
3233
- Fixes an issue where opening a deep link to create a PR worktree would incorrectly prompt to add a remote that already exists ([#4926](https://github.com/gitkraken/vscode-gitlens/issues/4926))

‎src/cache.ts‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type { ResourceDescriptor } from './git/models/resourceDescriptor.js';
1111
import type { GitHostIntegration } from './plus/integrations/models/gitHostIntegration.js';
1212
import type { IntegrationBase } from './plus/integrations/models/integration.js';
1313
import { isPromise } from './system/promise.js';
14+
import { CacheController } from './system/promiseCache.js';
1415

1516
type Caches = {
1617
defaultBranch: { key: `repo:${string}`; value: DefaultBranch };
@@ -31,7 +32,7 @@ type CacheKey<T extends Cache> = Caches[T]['key'];
3132
type CacheValue<T extends Cache> = Caches[T]['value'];
3233
type CacheResult<T> = Promise<T | undefined> | T | undefined;
3334

34-
type Cacheable<T> = () => { value: CacheResult<T>; expiresAt?: number };
35+
type Cacheable<T> = (cacheable: CacheController) => { value: CacheResult<T>; expiresAt?: number };
3536
type Cached<T> =
3637
| {
3738
value: T | undefined;
@@ -46,6 +47,8 @@ type Cached<T> =
4647
etag?: string;
4748
};
4849

50+
type ExpiryOptions = { expiryOverride?: boolean | number; expireOnError?: boolean };
51+
4952
export class CacheProvider implements Disposable {
5053
private readonly _cache = new Map<`${Cache}:${CacheKey<Cache>}`, Cached<CacheResult<CacheValue<Cache>>>>();
5154

@@ -65,7 +68,7 @@ export class CacheProvider implements Disposable {
6568
key: CacheKey<T>,
6669
etag: string | undefined,
6770
cacheable: Cacheable<CacheValue<T>>,
68-
options?: { expiryOverride?: boolean | number },
71+
options?: ExpiryOptions,
6972
): CacheResult<CacheValue<T>> {
7073
const item = this._cache.get(`${cache}:${key}`);
7174

@@ -85,8 +88,18 @@ export class CacheProvider implements Disposable {
8588
(expiry != null && expiry > 0 && expiry < Date.now()) ||
8689
(item.etag != null && item.etag !== etag)
8790
) {
88-
const { value, expiresAt } = cacheable();
89-
return this.set<T>(cache, key, value, etag, expiresAt)?.value as CacheResult<CacheValue<T>>;
91+
const cacheController = new CacheController();
92+
const { value, expiresAt } = cacheable(cacheController);
93+
if (isPromise(value)) {
94+
void value.finally(() => {
95+
if (cacheController.invalidated) {
96+
this.delete(cache, key);
97+
}
98+
});
99+
}
100+
return this.set<T>(cache, key, value, etag, expiresAt, options?.expireOnError)?.value as CacheResult<
101+
CacheValue<T>
102+
>;
90103
}
91104

92105
return item.value as CacheResult<CacheValue<T>>;
@@ -95,7 +108,7 @@ export class CacheProvider implements Disposable {
95108
getCurrentAccount(
96109
integration: IntegrationBase,
97110
cacheable: Cacheable<Account>,
98-
options?: { expiryOverride?: boolean | number },
111+
options?: ExpiryOptions,
99112
): CacheResult<Account> {
100113
const { key, etag } = getIntegrationKeyAndEtag(integration);
101114
return this.get('currentAccount', `id:${key}`, etag, cacheable, options);
@@ -117,7 +130,7 @@ export class CacheProvider implements Disposable {
117130
resource: ResourceDescriptor,
118131
integration: IntegrationBase | undefined,
119132
cacheable: Cacheable<IssueOrPullRequest>,
120-
options?: { expiryOverride?: boolean | number },
133+
options?: ExpiryOptions,
121134
): CacheResult<IssueOrPullRequest> {
122135
const { key, etag } = getResourceKeyAndEtag(resource, integration);
123136

@@ -138,7 +151,7 @@ export class CacheProvider implements Disposable {
138151
resource: ResourceDescriptor,
139152
integration: IntegrationBase | undefined,
140153
cacheable: Cacheable<Issue>,
141-
options?: { expiryOverride?: boolean | number },
154+
options?: ExpiryOptions,
142155
): CacheResult<Issue> {
143156
const { key, etag } = getResourceKeyAndEtag(resource, integration);
144157

@@ -165,7 +178,7 @@ export class CacheProvider implements Disposable {
165178
resource: ResourceDescriptor,
166179
integration: IntegrationBase | undefined,
167180
cacheable: Cacheable<PullRequest>,
168-
options?: { expiryOverride?: boolean | number },
181+
options?: ExpiryOptions,
169182
): CacheResult<PullRequest> {
170183
const { key, etag } = getResourceKeyAndEtag(resource, integration);
171184

@@ -192,7 +205,7 @@ export class CacheProvider implements Disposable {
192205
repo: ResourceDescriptor,
193206
integration: GitHostIntegration | undefined,
194207
cacheable: Cacheable<PullRequest>,
195-
options?: { expiryOverride?: boolean | number },
208+
options?: ExpiryOptions,
196209
): CacheResult<PullRequest> {
197210
const { key, etag } = getResourceKeyAndEtag(repo, integration);
198211
// Wrap the cacheable so we can also add the result to the issuesOrPrsById cache
@@ -210,7 +223,7 @@ export class CacheProvider implements Disposable {
210223
repo: ResourceDescriptor,
211224
integration: GitHostIntegration | undefined,
212225
cacheable: Cacheable<PullRequest>,
213-
options?: { expiryOverride?: boolean | number },
226+
options?: ExpiryOptions,
214227
): CacheResult<PullRequest> {
215228
const { key, etag } = getResourceKeyAndEtag(repo, integration);
216229
// Wrap the cacheable so we can also add the result to the issuesOrPrsById cache
@@ -227,7 +240,7 @@ export class CacheProvider implements Disposable {
227240
repo: ResourceDescriptor,
228241
integration: GitHostIntegration | undefined,
229242
cacheable: Cacheable<DefaultBranch>,
230-
options?: { expiryOverride?: boolean | number },
243+
options?: ExpiryOptions,
231244
): CacheResult<DefaultBranch> {
232245
const { key, etag } = getResourceKeyAndEtag(repo, integration);
233246
return this.get('defaultBranch', `repo:${key}`, etag, cacheable, options);
@@ -237,7 +250,7 @@ export class CacheProvider implements Disposable {
237250
repo: ResourceDescriptor,
238251
integration: GitHostIntegration | undefined,
239252
cacheable: Cacheable<RepositoryMetadata>,
240-
options?: { expiryOverride?: boolean | number },
253+
options?: ExpiryOptions,
241254
): CacheResult<RepositoryMetadata> {
242255
const { key, etag } = getResourceKeyAndEtag(repo, integration);
243256
return this.get('repoMetadata', `repo:${key}`, etag, cacheable, options);
@@ -249,17 +262,14 @@ export class CacheProvider implements Disposable {
249262
value: CacheResult<CacheValue<T>>,
250263
etag: string | undefined,
251264
expiresAt?: number,
265+
expireOnError?: boolean,
252266
): Cached<CacheResult<CacheValue<T>>> {
253267
let item: Cached<CacheResult<CacheValue<T>>>;
254268
if (isPromise(value)) {
255-
void value.then(
256-
v => {
257-
this.set(cache, key, v, etag, expiresAt);
258-
},
259-
() => {
260-
this.delete(cache, key);
261-
},
262-
);
269+
void value.then(v => this.set(cache, key, v, etag, expiresAt, expireOnError));
270+
if (expireOnError !== false) {
271+
void value.catch(() => this.delete(cache, key));
272+
}
263273

264274
item = { value: value, etag: etag, cachedAt: Date.now() };
265275
} else {
@@ -280,8 +290,8 @@ export class CacheProvider implements Disposable {
280290
key: string,
281291
etag: string | undefined,
282292
): Cacheable<PullRequest> {
283-
return () => {
284-
const item = cacheable();
293+
return cacheController => {
294+
const item = cacheable(cacheController);
285295
if (isPromise(item.value)) {
286296
void item.value.then(v => {
287297
if (v != null) {

‎src/plus/integrations/models/integration.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -603,19 +603,31 @@ export abstract class IntegrationBase<
603603

604604
const currentAccount = await this.container.cache.getCurrentAccount(
605605
this,
606-
() => ({
606+
cacheable => ({
607607
value: (async () => {
608608
try {
609609
const account = await this.getProviderCurrentAccount?.(this._session!, opts);
610610
this.resetRequestExceptionCount('getCurrentAccount');
611611
return account;
612612
} catch (ex) {
613+
if (ex instanceof CancellationError) {
614+
cacheable.invalidate();
615+
return undefined;
616+
}
617+
613618
this.handleProviderException('getCurrentAccount', ex, { scope: scope });
614-
return undefined;
619+
620+
// Invalidate the cache on error, except for auth errors
621+
if (!(ex instanceof AuthenticationError)) {
622+
cacheable.invalidate();
623+
}
624+
625+
// Re-throw to the caller
626+
throw ex;
615627
}
616628
})(),
617629
}),
618-
{ expiryOverride: expiryOverride },
630+
{ expiryOverride: expiryOverride, expireOnError: false },
619631
);
620632
return currentAccount;
621633
}

0 commit comments

Comments
 (0)