Skip to content

Commit f09212f

Browse files
wangsijiecursoragent
wangsijie
and
cursoragent
authored
fix: cache OIDC discovery for server clients (#1113)
* fix: cache OIDC discovery for server clients Fixes #1112 Co-authored-by: wangsijie <sijiewg@gmail.com> * docs: explain adapter cache dedupe * chore: limit changeset to packages with direct code changes Co-authored-by: wangsijie <sijiewg@gmail.com> * fix(client): catch rejected in-flight cache getters Wrap await runningGetter in try/catch so concurrent waiters can fall back to their own getter when the shared in-flight population fails, as intended by the cache dedupe logic. * fix(client): populate cache on in-flight getter fallback Re-enter getWithCache after a rejected in-flight population so recovery writes to shared cache and later callers can reuse the result. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 parent 5a65065 commit f09212f

11 files changed

Lines changed: 320 additions & 25 deletions

File tree

‎.changeset/cache-oidc-discovery.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@logto/client': patch
3+
'@logto/node': patch
4+
---
5+
6+
cache OIDC discovery metadata across server-side client instances and normalize non-JSON request failures

‎packages/client/src/adapter/index.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,56 @@ describe('ClientAdapterInstance', () => {
4747
expect(spy).toHaveBeenCalledTimes(2);
4848
expect(getter).toHaveBeenCalledTimes(1);
4949
});
50+
51+
it('should fall back to the current caller getter when the in-flight getter rejects', async () => {
52+
const adapters = createAdapters(true);
53+
const firstAdapterInstance = new ClientAdapterInstance(adapters);
54+
const secondAdapterInstance = new ClientAdapterInstance(adapters);
55+
const firstGetter = vi.fn(async () => {
56+
await new Promise((resolve) => {
57+
setTimeout(resolve, 50);
58+
});
59+
throw new Error('transient failure');
60+
});
61+
const secondGetter = vi.fn().mockResolvedValue({ test: 'recovered' });
62+
63+
const firstCall = firstAdapterInstance.getWithCache(CacheKey.OpenidConfig, firstGetter);
64+
const secondCall = secondAdapterInstance.getWithCache(CacheKey.OpenidConfig, secondGetter);
65+
66+
await expect(firstCall).rejects.toThrow('transient failure');
67+
await expect(secondCall).resolves.toEqual({ test: 'recovered' });
68+
expect(secondGetter).toHaveBeenCalledTimes(1);
69+
expect(await adapters.unstable_cache?.getItem(CacheKey.OpenidConfig)).toBe(
70+
JSON.stringify({ test: 'recovered' })
71+
);
72+
73+
const thirdGetter = vi.fn().mockResolvedValue({ test: 'should not run' });
74+
const thirdAdapterInstance = new ClientAdapterInstance(adapters);
75+
76+
expect(await thirdAdapterInstance.getWithCache(CacheKey.OpenidConfig, thirdGetter)).toEqual({
77+
test: 'recovered',
78+
});
79+
expect(thirdGetter).not.toHaveBeenCalled();
80+
});
81+
82+
it('should deduplicate concurrent cache misses for the same cache storage', async () => {
83+
const adapters = createAdapters(true);
84+
const firstAdapterInstance = new ClientAdapterInstance(adapters);
85+
const secondAdapterInstance = new ClientAdapterInstance(adapters);
86+
const getter = vi.fn(async () => {
87+
await new Promise((resolve) => {
88+
setTimeout(resolve, 10);
89+
});
90+
91+
return { test: 'test' };
92+
});
93+
94+
await expect(
95+
Promise.all([
96+
firstAdapterInstance.getWithCache(CacheKey.OpenidConfig, getter),
97+
secondAdapterInstance.getWithCache(CacheKey.OpenidConfig, getter),
98+
])
99+
).resolves.toEqual([{ test: 'test' }, { test: 'test' }]);
100+
expect(getter).toHaveBeenCalledTimes(1);
101+
});
50102
});

‎packages/client/src/adapter/index.ts‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,22 @@ import {
1111
type InferStorageKey,
1212
} from './types.js';
1313

14+
// Track in-flight cache writes per cache storage instance. A WeakMap keeps this helper from
15+
// extending the lifetime of adapter-provided cache stores.
16+
const runningCacheGetters = new WeakMap<Storage<CacheKey>, Map<CacheKey, Promise<unknown>>>();
17+
18+
const getRunningCacheGetterMap = (cache: Storage<CacheKey>) => {
19+
const runningGetterMap = runningCacheGetters.get(cache);
20+
21+
if (runningGetterMap) {
22+
return runningGetterMap;
23+
}
24+
25+
const newRunningGetterMap = new Map<CacheKey, Promise<unknown>>();
26+
runningCacheGetters.set(cache, newRunningGetterMap);
27+
return newRunningGetterMap;
28+
};
29+
1430
export class ClientAdapterInstance implements ClientAdapter {
1531
/*
1632
* Implement `ClientAdapter`. Its properties are assigned by
@@ -73,9 +89,48 @@ export class ClientAdapterInstance implements ClientAdapter {
7389
return cached;
7490
}
7591

76-
const result = await getter();
77-
await this.unstable_cache?.setItem(key, JSON.stringify(result));
78-
return result;
92+
const { unstable_cache: cache } = this;
93+
94+
if (!cache) {
95+
return getter();
96+
}
97+
98+
const runningGetterMap = getRunningCacheGetterMap(cache);
99+
const runningGetter = runningGetterMap.get(key);
100+
101+
if (runningGetter) {
102+
// Another client sharing the same cache storage is already populating this key. Wait for it
103+
// instead of issuing a duplicate discovery request.
104+
try {
105+
await runningGetter;
106+
} catch {
107+
// The in-flight getter rejected before writing to cache. Fall through to the cache check
108+
// and the current caller's getter below.
109+
}
110+
111+
const cachedResult = await this.getCachedObject<T>(key);
112+
113+
if (cachedResult) {
114+
return cachedResult;
115+
}
116+
117+
// The in-flight getter may fail before writing to cache. Retry through the same population
118+
// path so a successful recovery is stored for later callers.
119+
return this.getWithCache(key, getter);
120+
}
121+
122+
const newRunningGetter = (async () => {
123+
const result = await getter();
124+
await cache.setItem(key, JSON.stringify(result));
125+
return result;
126+
})();
127+
runningGetterMap.set(key, newRunningGetter);
128+
129+
try {
130+
return await newRunningGetter;
131+
} finally {
132+
runningGetterMap.delete(key);
133+
}
79134
}
80135
}
81136

‎packages/client/src/utils/requester.test.ts‎

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,28 +20,35 @@ describe('createRequester', () => {
2020
const message = 'some error message';
2121

2222
test('failing response json with code and message should throw LogtoRequestError with same code and message', async () => {
23-
const fetchFunction = vi
24-
.fn()
25-
.mockResolvedValue(new Response(JSON.stringify({ code, message }), { status: 400 }));
23+
const fetchFunction = vi.fn().mockResolvedValue(
24+
new Response(JSON.stringify({ code, message }), {
25+
status: 400,
26+
headers: { 'content-type': 'application/json' },
27+
})
28+
);
2629
const requester = createRequester(fetchFunction);
2730
await expect(requester('foo')).rejects.toMatchObject(new LogtoRequestError(code, message));
2831
});
2932

3033
test('failing response json with more than code and message should throw LogtoRequestError with same code and message', async () => {
31-
const fetchFunction = vi
32-
.fn()
33-
.mockResolvedValue(
34-
new Response(JSON.stringify({ code, message, foo: 'bar' }), { status: 400 })
35-
);
34+
const fetchFunction = vi.fn().mockResolvedValue(
35+
new Response(JSON.stringify({ code, message, foo: 'bar' }), {
36+
status: 400,
37+
headers: { 'content-type': 'application/json' },
38+
})
39+
);
3640
const requester = createRequester(fetchFunction);
3741
await expect(requester('foo')).rejects.toMatchObject(new LogtoRequestError(code, message));
3842
});
3943

4044
test('failing response json with only code should throw LogtoError', async () => {
4145
const json = { code };
42-
const fetchFunction = vi
43-
.fn()
44-
.mockResolvedValue(new Response(JSON.stringify(json), { status: 400 }));
46+
const fetchFunction = vi.fn().mockResolvedValue(
47+
new Response(JSON.stringify(json), {
48+
status: 400,
49+
headers: { 'content-type': 'application/json' },
50+
})
51+
);
4552
const requester = createRequester(fetchFunction);
4653
await expect(requester('foo')).rejects.toMatchObject(
4754
new LogtoError('unexpected_response_error', json)
@@ -50,9 +57,12 @@ describe('createRequester', () => {
5057

5158
test('failing response json with only message should throw LogtoError', async () => {
5259
const json = { message };
53-
const fetchFunction = vi
54-
.fn()
55-
.mockResolvedValue(new Response(JSON.stringify(json), { status: 400 }));
60+
const fetchFunction = vi.fn().mockResolvedValue(
61+
new Response(JSON.stringify(json), {
62+
status: 400,
63+
headers: { 'content-type': 'application/json' },
64+
})
65+
);
5666
const requester = createRequester(fetchFunction);
5767
await expect(requester('foo')).rejects.toMatchObject(
5868
new LogtoError('unexpected_response_error', json)
@@ -61,23 +71,41 @@ describe('createRequester', () => {
6171

6272
test('failing response json without code and message should throw LogtoError', async () => {
6373
const json = {};
64-
const fetchFunction = vi
65-
.fn()
66-
.mockResolvedValue(new Response(JSON.stringify(json), { status: 400 }));
74+
const fetchFunction = vi.fn().mockResolvedValue(
75+
new Response(JSON.stringify(json), {
76+
status: 400,
77+
headers: { 'content-type': 'application/json' },
78+
})
79+
);
6780
const requester = createRequester(fetchFunction);
6881
await expect(requester('foo')).rejects.toMatchObject(
6982
new LogtoError('unexpected_response_error', json)
7083
);
7184
});
7285

73-
test('failing response with non-json text should throw TypeError', async () => {
86+
test('failing response with non-json text should throw LogtoRequestError', async () => {
7487
const fetchFunction = vi.fn().mockResolvedValue(
7588
new Response('not json content', {
7689
status: 400,
7790
})
7891
);
7992
const requester = createRequester(fetchFunction);
80-
await expect(requester('foo')).rejects.toThrowError(SyntaxError);
93+
await expect(requester('foo')).rejects.toMatchObject(
94+
new LogtoRequestError('http_error_400', 'not json content')
95+
);
96+
});
97+
98+
test('rate limited response with non-json text should throw LogtoRequestError', async () => {
99+
const fetchFunction = vi.fn().mockResolvedValue(
100+
new Response('Too many requests', {
101+
status: 429,
102+
headers: { 'content-type': 'text/plain' },
103+
})
104+
);
105+
const requester = createRequester(fetchFunction);
106+
await expect(requester('foo')).rejects.toMatchObject(
107+
new LogtoRequestError('rate_limited', 'Too many requests')
108+
);
81109
});
82110
});
83111
});

‎packages/client/src/utils/requester.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,19 @@ export const createRequester = (fetchFunction: typeof fetch): Requester => {
1414

1515
if (!response.ok) {
1616
const cloned = response.clone();
17-
const responseJson = await response.json();
17+
const responseJson: unknown = await response
18+
.clone()
19+
.json()
20+
.catch(async () => {
21+
const responseText = await response.text();
22+
console.error(`Logto requester error: [status=${response.status}]`, responseText);
23+
throw new LogtoRequestError(
24+
response.status === 429 ? 'rate_limited' : `http_error_${response.status}`,
25+
responseText || response.statusText,
26+
cloned
27+
);
28+
});
29+
1830
console.error(`Logto requester error: [status=${response.status}]`, responseJson);
1931

2032
if (!isLogtoRequestErrorJson(responseJson)) {

‎packages/node/edge/index.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import BaseClient, { type ClientAdapter } from '@logto/client';
2+
3+
import LogtoClient from './index.js';
4+
5+
const appId = 'app_id_value';
6+
const endpoint = 'https://logto.dev';
7+
8+
const navigate = vi.fn();
9+
const storage = {
10+
setItem: vi.fn(),
11+
getItem: vi.fn(),
12+
removeItem: vi.fn(),
13+
};
14+
15+
vi.mock('@logto/client', () => ({
16+
__esModule: true,
17+
default: vi.fn(),
18+
createRequester: vi.fn(),
19+
}));
20+
21+
const getLatestBaseClientAdapter = (): ClientAdapter => {
22+
const { calls } = vi.mocked(BaseClient).mock;
23+
const [, adapter] = calls.at(-1)!;
24+
return adapter;
25+
};
26+
27+
describe('LogtoClient (edge)', () => {
28+
beforeEach(() => {
29+
vi.mocked(BaseClient).mockClear();
30+
});
31+
32+
it('should provide endpoint-scoped cache storage by default', () => {
33+
expect(
34+
new LogtoClient({ endpoint: `${endpoint}/`, appId }, { navigate, storage })
35+
).toBeDefined();
36+
const cache = getLatestBaseClientAdapter().unstable_cache;
37+
38+
expect(new LogtoClient({ endpoint, appId }, { navigate, storage })).toBeDefined();
39+
expect(getLatestBaseClientAdapter().unstable_cache).toBe(cache);
40+
41+
expect(
42+
new LogtoClient({ endpoint: 'https://another.logto.dev', appId }, { navigate, storage })
43+
).toBeDefined();
44+
expect(getLatestBaseClientAdapter().unstable_cache).not.toBe(cache);
45+
});
46+
});

‎packages/node/edge/index.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import type { LogtoConfig, ClientAdapter } from '@logto/client';
22
import { createRequester } from '@logto/client';
3+
import { encode } from 'js-base64';
34

45
import BaseClient from '../src/client.js';
6+
import { createMemoryCache } from '../src/utils/cache.js';
57

68
import { generateCodeChallenge, generateCodeVerifier, generateState } from './generators.js';
79

@@ -17,8 +19,7 @@ export default class LogtoClient extends BaseClient {
1719
? async (...args: Parameters<typeof fetch>) => {
1820
const [input, init] = args;
1921

20-
// Encode to base64 using btoa
21-
const base64Credentials = btoa(`${config.appId}:${config.appSecret ?? ''}`);
22+
const base64Credentials = encode(`${config.appId}:${config.appSecret ?? ''}`);
2223

2324
return fetch(input, {
2425
...init,
@@ -33,6 +34,7 @@ export default class LogtoClient extends BaseClient {
3334
generateCodeChallenge,
3435
generateCodeVerifier,
3536
generateState,
37+
unstable_cache: createMemoryCache(config.endpoint),
3638
});
3739
}
3840
}

0 commit comments

Comments
 (0)