Skip to content

Commit cfe8951

Browse files
dinwwwhdinwwwh
authored
fix(rpc,openapi): concurrent lazy router resolution, and faster route matching (#1736)
Fixes a bug where lazy routers could be permanently dropped when requests arrive concurrently, and makes route matching 1.4x to 3.7x faster. ## The bug Both matchers rebuilt their pending lazy router collection **after** awaiting the loader. With two requests in flight, the slower one's write clobbered the faster one's, and any nested lazy router discovered in between was lost for the lifetime of the process. Every route inside it returned a miss from then on. Reproduced with a 3 level lazy chain: `/p2/p3/leaf3` returned `undefined` for the concurrent requests **and for every request after them**. The same interleaving also reloaded routers. 12 concurrent requests on a 3 level chain triggered 12, then 144, then 1728 loader calls. In the OpenAPI matcher each redundant load also appended duplicate entries to the rou3 tree, so the tree grew without bound. Each lazy router now loads exactly once. ## Performance Measured with both implementations in one process, interleaved in rotating order, min of 25 rounds, against a second identical copy of the original as a noise control. | case | before | after | | |---|---|---|---| | RPC hit | 359ns | 103ns | 3.5x | | RPC hit, 1296 procedures | 362ns | 105ns | 3.4x | | RPC hit, 50 pending lazy routers | 1609ns | 224ns | 7.2x | | OpenAPI static hit | 374ns | 103ns | 3.7x | | OpenAPI dynamic param hit | 1008ns | 359ns | 2.8x | | OpenAPI hit, 1000 routes | 1032ns | 415ns | 2.5x | | OpenAPI hit, 50 pending lazy routers | 3400ns | 1381ns | 2.5x | End to end that is 1.07x on a minimal RPC request, 1.27x when the router has unresolved lazy sub routers. Construction is unchanged. `walkProcedureContractsSync` also got 1.05x to 2.8x depending on router shape, plus much lower allocation (16.7MB to 4.7MB walking a 50k procedure router) and it now tolerates deeper nesting. ## Outcome - Lazy routers are no longer lost when requests race - Each lazy router loads once instead of once per concurrent request - No more unbounded duplicate entries in the OpenAPI route tree - A `RangeError: Maximum call stack size exceeded` is gone from `walkProcedureContractsSync`, which fired once a single subtree carried ~123k lazy routers - Route matching is meaningfully faster on every shape measured ## For reviewers Three things worth a look: 1. **One intentional behaviour change.** The RPC matcher no longer loads a lazy router mounted at `/lazy` for a request to `/lazyfoo/bar`. That load could never have served the request, so results are unchanged, but the loader call count differs. Covered by a test. 2. **A trade-off from sharing in-flight loads.** If a shared load fails, every concurrent request waiting on it fails. Previously each retried on its own. The next request still retries, and this is what removes the 1728 load blowup. 3. **`decodeParams` edge cases.** rou3 stores `undefined` for an optional segment the request omitted, and a param named `__proto__` needs `defineProperty` rather than assignment. Both are covered by tests. ## Testing Full suite green (2704 tests), plus bun (59) and cloudflare (26). Type check and lint clean. Both matchers are at 100% statements, lines and functions. New tests cover concurrent matching on lazy and nested lazy routers, deep lazy chains asserting each level loads exactly once, failed and synchronously throwing loaders being retried, and the percent encoded path that only unlocks a prefixed lazy router after normalisation. Each new test was checked against the pre change code to confirm it actually fails there.
1 parent 53280cf commit cfe8951

7 files changed

Lines changed: 419 additions & 106 deletions

File tree

‎packages/cloudflare/worker-configuration.d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* eslint-disable */
22
// Generated by Wrangler by running `wrangler types` (hash: e624d76b8500cfee2091bb6c84ee404f)
3-
// Runtime types generated with workerd@1.20260714.1 2026-07-01
3+
// Runtime types generated with workerd@1.20260722.1 2026-07-01
44
interface __BaseEnv_Env {
55
RATELIMIT_3_10S: RateLimit;
66
PUBLISHER_DON: DurableObjectNamespace /* PublisherDO */;

‎packages/openapi/src/adapters/standard/openapi-matcher.test.ts‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,19 @@ describe('openAPIMatcher', () => {
127127
expect(filter.mock.calls).toContainEqual([secret, ['internal', 'secret']])
128128
})
129129

130+
it('keeps a param literally named __proto__ as an own property', async () => {
131+
const procedure = os
132+
.meta(openapi({ method: 'GET', path: '/a/{__proto__}' }))
133+
.handler(() => 'ok')
134+
135+
const matcher = new OpenAPIMatcher({ procedure })
136+
const result = await matcher.match('GET', '/a/value', undefined)
137+
138+
expect(result).toBeDefined()
139+
expect(Object.keys(result!.params!)).toEqual(['__proto__'])
140+
expect(Object.getOwnPropertyDescriptor(result!.params!, '__proto__')?.value).toBe('value')
141+
})
142+
130143
it('supports both normal and catch-all params at the same time', async () => {
131144
const procedure = os
132145
.meta(openapi({ method: 'GET', path: '/{name}/{+rest}' }))
@@ -237,6 +250,66 @@ describe('openAPIMatcher', () => {
237250
expect(loader).toHaveBeenCalledTimes(1)
238251
})
239252

253+
it('retries a lazy router whose load fails, synchronously or asynchronously', async () => {
254+
const info = os
255+
.meta(openapi({ method: 'GET', path: '/info' }))
256+
.handler(() => 'info')
257+
258+
let attempts = 0
259+
const loader = vi.fn(() => {
260+
attempts++
261+
// the first attempt throws synchronously out of `unlazy`, the second rejects
262+
if (attempts === 1) {
263+
throw new Error('sync boom')
264+
}
265+
if (attempts === 2) {
266+
return Promise.reject(new Error('async boom'))
267+
}
268+
return Promise.resolve({ default: { info } })
269+
})
270+
271+
const matcher = new OpenAPIMatcher({ lazy: os.lazy(loader as any) })
272+
273+
await expect(matcher.match('GET', '/info', undefined)).rejects.toThrowError('sync boom')
274+
await expect(matcher.match('GET', '/info', undefined)).rejects.toThrowError('async boom')
275+
276+
// a failed load must leave the router pending so a later match can still resolve it
277+
await expect(matcher.match('GET', '/info', undefined)).resolves.toEqual({
278+
path: ['lazy', 'info'],
279+
procedure: info,
280+
params: undefined,
281+
})
282+
283+
expect(loader).toHaveBeenCalledTimes(3)
284+
})
285+
286+
it('resolves a prefixed lazy router that only matches after percent-decoding', async () => {
287+
const info = os
288+
.meta(openapi({ method: 'GET', path: '/info' }))
289+
.handler(() => 'info')
290+
291+
const loader = vi.fn(async () => ({ default: { info } }))
292+
293+
const matcher = new OpenAPIMatcher({
294+
user: os.meta(openapi({ prefix: '/users' })).lazy(loader),
295+
})
296+
297+
// "%75" is "u": the prefix RegExp is tested against the raw pathname first and fails,
298+
// so the retry has to re-run lazy resolution against the normalized pathname
299+
const result = await matcher.match('GET', '/%75sers/info', undefined)
300+
301+
expect(result).toBeDefined()
302+
expect(result!.path).toEqual(['user', 'info'])
303+
expect(result!.params).toBeUndefined()
304+
expect(getOpenAPIMeta(result!.procedure)).toMatchObject({
305+
method: 'GET',
306+
path: '/info',
307+
prefix: '/users',
308+
})
309+
310+
expect(loader).toHaveBeenCalledTimes(1)
311+
})
312+
240313
it('resolves nested lazy routers added during the same match', async () => {
241314
const summary = os
242315
.meta(openapi({ method: 'GET', path: '/summary' }))
@@ -272,6 +345,65 @@ describe('openAPIMatcher', () => {
272345
expect(outerLoader).toHaveBeenCalledTimes(1)
273346
expect(projectLoader).toHaveBeenCalledTimes(1)
274347
})
348+
349+
it('resolves a deep lazy chain for concurrent matches without losing or reloading routers', async () => {
350+
const leaf1 = os.meta(openapi({ method: 'GET', path: '/leaf1' })).handler(() => '1')
351+
const leaf2 = os.meta(openapi({ method: 'GET', path: '/leaf2' })).handler(() => '2')
352+
const leaf3 = os.meta(openapi({ method: 'GET', path: '/leaf3' })).handler(() => '3')
353+
354+
const loads = { l1: 0, l2: 0, l3: 0 }
355+
356+
/** settle after N microtask turns so the three levels interleave */
357+
const settle = async (turns: number) => {
358+
for (let i = 0; i < turns; i++) {
359+
await Promise.resolve()
360+
}
361+
}
362+
363+
const l3 = os.meta(openapi({ prefix: '/p3' })).lazy(async () => {
364+
loads.l3++
365+
await settle(2)
366+
return { default: { leaf3 } }
367+
})
368+
369+
const l2 = os.meta(openapi({ prefix: '/p2' })).lazy(async () => {
370+
loads.l2++
371+
await settle(3)
372+
return { default: { leaf2, l3 } }
373+
})
374+
375+
const matcher = new OpenAPIMatcher({
376+
lazy: os.lazy(async () => {
377+
loads.l1++
378+
await settle(4)
379+
return { default: { leaf1, l2 } }
380+
}),
381+
})
382+
383+
const paths = ['/leaf1', '/p2/leaf2', '/p2/p3/leaf3'] as const
384+
const results = await Promise.all(
385+
paths.flatMap(path => [
386+
matcher.match('GET', path, undefined),
387+
matcher.match('GET', path, undefined),
388+
]),
389+
)
390+
391+
expect(results.map(result => result?.path.join('/'))).toEqual([
392+
'lazy/leaf1',
393+
'lazy/leaf1',
394+
'lazy/l2/leaf2',
395+
'lazy/l2/leaf2',
396+
'lazy/l2/l3/leaf3',
397+
'lazy/l2/l3/leaf3',
398+
])
399+
400+
// every level is loaded exactly once even though six matches raced for it
401+
expect(loads).toEqual({ l1: 1, l2: 1, l3: 1 })
402+
403+
// and the deepest route stays matchable afterwards
404+
await expect(matcher.match('GET', '/p2/p3/leaf3', undefined)).resolves.toBeDefined()
405+
expect(loads).toEqual({ l1: 1, l2: 1, l3: 1 })
406+
})
275407
})
276408

277409
describe('contract-first routers', () => {

‎packages/openapi/src/adapters/standard/openapi-matcher.ts‎

Lines changed: 56 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,16 @@ interface TreeEntry {
2525

2626
interface PendingLazyRouter extends WalkProcedureContractsLazyResult {
2727
matcher?: RegExp
28+
/** in-flight load, shared so concurrent matches never load or re-index the same router twice */
29+
loading?: Promise<void> | undefined
2830
}
2931

3032
export class OpenAPIMatcher {
3133
private readonly filter: Exclude<OpenAPIMatcherOptions['filter'], undefined>
3234
private readonly rootRouter: AnyRouter
3335

3436
private readonly tree = createRouter<TreeEntry>()
35-
36-
private pendingLazyRouters: PendingLazyRouter[] = []
37+
private readonly pendingLazyRouters: Set<PendingLazyRouter> = new Set()
3738

3839
constructor(router: AnyRouter, options: OpenAPIMatcherOptions = {}) {
3940
this.filter = options.filter ?? true
@@ -60,14 +61,14 @@ export class OpenAPIMatcher {
6061
})
6162
}, path)
6263

63-
this.pendingLazyRouters.push(...lazyResults.map((result) => {
64+
for (const result of lazyResults) {
6465
const prefix = getOpenAPIMeta(result.router)?.prefix
6566

66-
return {
67+
this.pendingLazyRouters.add({
6768
...result,
6869
matcher: prefix ? toRou3PrefixMatcher(prefix) : undefined,
69-
}
70-
}))
70+
})
71+
}
7172
}
7273

7374
async match(
@@ -101,67 +102,81 @@ export class OpenAPIMatcher {
101102
}
102103
}
103104

104-
const result = await this.matchPathname(method, pathname)
105+
// most requests `await undefined` so conditionally await it to save a microtask turn
106+
const loading = this.resolvePendingLazyRouters(pathname)
107+
if (loading !== undefined) {
108+
await loading
109+
}
110+
111+
let match = findRoute(this.tree, method, pathname)
105112

106-
if (!result && pathname.includes('%')) {
113+
if (match === undefined && pathname.includes('%')) {
107114
// Retry with a normalized path: users may percent-encode characters that
108115
// we store unencoded (e.g. "a%62c" vs "abc"), so normalization lets us
109116
// handle those requests without storing duplicate entries.
110117

111-
return this.matchPathname(method, normalizeHttpPath(pathname))
112-
}
118+
const normalizedPathname = normalizeHttpPath(pathname)
113119

114-
return result
115-
}
116-
117-
private async matchPathname(
118-
method: string,
119-
pathname: `/${string}`,
120-
): Promise<{ path: string[], procedure: AnyProcedure, params?: Record<string, string> | undefined } | undefined> {
121-
await this.resolvePendingLazyRouters(pathname)
120+
// most requests `await undefined` so conditionally await it to save a microtask turn
121+
const normalizedLoading = this.resolvePendingLazyRouters(normalizedPathname)
122+
if (normalizedLoading !== undefined) {
123+
await normalizedLoading
124+
}
122125

123-
const match = findRoute(this.tree, method, pathname)
126+
match = findRoute(this.tree, method, normalizedPathname)
127+
}
124128

125-
if (!match) {
129+
if (match === undefined) {
126130
return undefined
127131
}
128132

129-
const procedure = await this.resolveProcedure(match.data)
133+
const entry = match.data
130134

131135
return {
132-
path: match.data.path,
133-
procedure,
136+
path: entry.path,
137+
procedure: entry.procedure ?? await this.resolveProcedure(entry),
134138
params: match.params ? decodeParams(match.params) : undefined,
135139
}
136140
}
137141

138-
private async resolvePendingLazyRouters(pathname: `/${string}`): Promise<void> {
139-
if (!this.pendingLazyRouters.length) {
140-
return
142+
private resolvePendingLazyRouters(pathname: `/${string}`): Promise<void> | void {
143+
for (const pending of this.pendingLazyRouters) {
144+
if (pending.matcher === undefined || pending.matcher.test(pathname)) {
145+
return this.loadPendingLazyRouters(pathname)
146+
}
141147
}
148+
}
142149

143-
const stillPending: typeof this.pendingLazyRouters = []
144-
145-
// We need to loop over this.pendingLazyRouters because this.index can still append new lazy routers
146-
// that might need to be resolved
150+
private async loadPendingLazyRouters(pathname: `/${string}`): Promise<void> {
147151
for (const pending of this.pendingLazyRouters) {
148-
if (!pending.matcher || pending.matcher.test(pathname)) {
149-
const { default: router } = await unlazy(pending.router)
150-
this.index(router, pending.path)
151-
}
152-
else {
153-
stillPending.push(pending)
152+
if (pending.matcher === undefined || pending.matcher.test(pathname)) {
153+
await this.loadPendingLazyRouter(pending)
154154
}
155155
}
156-
157-
this.pendingLazyRouters = stillPending
158156
}
159157

160-
private async resolveProcedure(entry: TreeEntry): Promise<AnyProcedure> {
161-
if (entry.procedure) {
162-
return entry.procedure
158+
private loadPendingLazyRouter(pending: PendingLazyRouter): Promise<void> {
159+
if (pending.loading === undefined) {
160+
pending.loading = this.indexPendingLazyRouter(pending).catch((error) => {
161+
pending.loading = undefined
162+
throw error
163+
})
163164
}
164165

166+
return pending.loading
167+
}
168+
169+
private async indexPendingLazyRouter(pending: PendingLazyRouter): Promise<void> {
170+
const { default: router } = await unlazy(pending.router)
171+
172+
this.index(router, pending.path)
173+
174+
// removed only once indexed, so a concurrent match never observes this router as
175+
// neither pending nor indexed
176+
this.pendingLazyRouters.delete(pending)
177+
}
178+
179+
private async resolveProcedure(entry: TreeEntry): Promise<AnyProcedure> {
165180
const { default: maybeProcedure } = await unlazy(getRouter(this.rootRouter, entry.path))
166181

167182
if (!(maybeProcedure instanceof Procedure)) {

0 commit comments

Comments
 (0)