perf(dispatch): cache route resolution and survive a stale team list - #39
Conversation
`resolveScriptName` ran on EVERY request with no cache, against a single-region D1 (creek-db, WNAM, read replication disabled). Every request from every colo — static assets and 304s included — therefore paid a long-haul round trip before anything else could happen. Measured from Taipei: ~0.33s to a `cf-cache-status: HIT` asset, against ~0.15s for the same object from a US colo. The gap is the ocean, and it was the tenant-visible "every page transition is slow" symptom. It was also the platform's largest failure surface — one mandatory cross-Pacific dependency per request is how a tenant came to see ~0.47% of invocations fail (Error 1101 report, 2026-07-30). #34 stopped those from reaching the visitor as 1101s; this removes most of the exposure that produced them. Two changes: 1. Cache hostname → {scriptName, plan, isCustomDomain} for 60s. The lookup precedes hostname parsing, so a hit costs zero D1 work — not even the team-list read — and an already-seen host keeps serving while D1 is unreachable. Safe because production/branch script names are deterministic and a redeploy updates the WfP script IN PLACE rather than renaming it, so a cached entry stays correct across deploys; the query only answers "does this exist". Negatives are NOT cached — a project created seconds ago must not stay 404 for a minute. Bounded at 2000 entries with oldest-first eviction, since hostnames are attacker-suppliable. `isCustomDomain` is carried in the entry rather than re-derived: it decides whether `Set-Cookie` gets its `Domain=` narrowed, and it cannot be recomputed from the hostname alone because "no team slug matched" is one of the ways a hostname becomes custom. Known trade-off: moving a custom domain between tenants takes up to the TTL to be reflected. 2. `getTeams` now serves its previous list when a refresh fails. Slugs change rarely; refusing every request in the isolate for as long as D1 is unreachable is far worse. `teamsCacheTime` is deliberately not advanced, so the next request retries. Also isolates cache-passthrough.test.ts with a fresh module per test. Its tests share one hostname, so a module-level route cache would have let the first test populate it and every later test skip the resolution path — quietly no longer exercising the cross-tenant cookie invariants that suite exists to lock. Each part is independently covered: removing the cache, dropping `isCustomDomain` on a hit, caching negatives, and removing the stale- list fallback each fail a distinct set of assertions.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated tests contain TypeScript type-only import misuse (import type used with typeof ...) that should be corrected to avoid type-check/IDE breakage, and the new stale-team warning behavior may need an agreed logging strategy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Improves dispatch-worker request latency and reliability by caching hostname → route resolution results in-memory, reducing per-request D1 lookups (especially impactful given single-region D1), and by allowing routing to continue using a stale team list when team-list refreshes fail.
Changes:
- Add a bounded (max 2000) 60s TTL route cache for
hostname → { scriptName, plan, isCustomDomain }, with no negative caching. - Update
getTeams()to fall back to a previously-loaded (stale) team list when refresh fails, instead of failing the request. - Add/adjust tests to cover cache hits, TTL expiry, negative non-caching, custom-domain behavior, and D1-down resilience; isolate module-level cache state per test where needed.
File summaries
| File | Description |
|---|---|
| packages/dispatch-worker/src/index.ts | Adds route cache and stale-team-list fallback to reduce D1 latency and avoid routing outages. |
| packages/dispatch-worker/src/route-cache.test.ts | New test suite validating caching behavior, TTL, non-cached 404s, cookie isolation, and bounded cache size. |
| packages/dispatch-worker/src/cache-passthrough.test.ts | Re-imports worker per test to avoid module-level caches masking intended invariants. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // the route cache for `site-acme.bycreek.com` and every later test would | ||
| // then skip the real resolution path — quietly no longer exercising the | ||
| // invariants this suite exists to lock. Re-import per test instead. | ||
| let worker: typeof workerModule; |
There was a problem hiding this comment.
Declining: typeof on an import type binding is valid TypeScript — import type restricts the name to type positions, and typeof is one. Verified rather than assumed: tsc --noEmit --listFiles confirms this package type-checks its test files (all four are in the file list), and it reports no errors. To rule out a false negative I injected const x: number = "str" into route-cache.test.ts and tsc caught it at that exact line, so the file is genuinely being checked. Changing to workerModule["fetch"] would also be wrong here: the default export is a value, so its type is only reachable through typeof.
| console.warn( | ||
| `[dispatch] team list refresh failed, serving stale: ${ | ||
| e instanceof Error ? e.message : String(e) | ||
| }`, | ||
| ); |
There was a problem hiding this comment.
Adopted in 43b447b0b1 — the stale-list warning is now throttled to once a minute per isolate. You're right that an unthrottled line per request would flood Workers Logs for the duration of an outage, and bill for it. The retry cadence is deliberately untouched: throttling the log must not throttle recovery. Covered by a test that fires 25 requests through a downed team-list read and asserts a single warning, plus one asserting it warns again after the interval.
| } as unknown as Parameters<typeof workerModule.fetch>[1]; | ||
| } | ||
|
|
||
| let worker: typeof workerModule; |
There was a problem hiding this comment.
Declining: typeof on an import type binding is valid TypeScript — import type restricts the name to type positions, and typeof is one. Verified rather than assumed: tsc --noEmit --listFiles confirms this package type-checks its test files (all four are in the file list), and it reports no errors. To rule out a false negative I injected const x: number = "str" into route-cache.test.ts and tsc caught it at that exact line, so the file is genuinely being checked. Changing to workerModule["fetch"] would also be wrong here: the default export is a value, so its type is only reachable through typeof.
`teamsCacheTime` is deliberately not advanced when a refresh fails, so every request retries — which also meant every request warned. During a sustained D1 outage that floods Workers Logs at production request rates, exactly when an operator is trying to read them, and bills for the volume. Warn at most once a minute per isolate. The retry cadence is untouched: throttling the log must not throttle recovery. Raised in Copilot review of this PR.
There was a problem hiding this comment.
🟡 Human review recommended
It changes production routing/caching behavior and security-relevant cookie handling, so a human should validate rollout risk and edge cases despite the strong test coverage.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The dominant cause of the
nii-course-systemslowness report (2026-07-24), and most of the failure surface behind the Error 1101 report (2026-07-30). One change addresses both.Problem
resolveScriptNameran on every request with no cache, against a single-region D1 —creek-dbis in WNAM withread_replication.mode = disabled(verified viawrangler d1 info). Every request from every colo, static assets and 304s included, paid a long-haul round trip before anything else could happen.Measured from Taipei against the reporting tenant:
cf-cache-status: HITstatic chunk, from TaipeiThe gap is the ocean. The tenant blamed
run_worker_first; it was never enabled in production. This was the toll.It was also the platform's largest failure surface. One mandatory cross-Pacific dependency per request is how a tenant came to see ~0.47% of invocations fail. #34 stopped those from surfacing as full-page 1101s; this removes most of what produced them.
Change
1. Route cache —
hostname → {scriptName, plan, isCustomDomain}, 60s TTL.The lookup precedes hostname parsing, so a hit costs zero D1 work — not even the team-list read — and an already-seen host keeps serving while D1 is unreachable.
Why it's safe: production/branch script names are deterministic (
{project}-{team}), and a redeploy updates the WfP script in place rather than renaming it, so a cached entry stays correct across deploys. The query only ever answered "does this exist".isCustomDomainis carried in the entry, not re-derived. It decides whetherSet-Cookiegets itsDomain=narrowed, and it cannot be recomputed from the hostname alone — "no team slug matched" is one of the ways a hostname becomes custom, and that needs the team list.2.
getTeamsserves a stale list on refresh failure. Slugs change rarely; refusing every request in the isolate for as long as D1 is unreachable is far worse.teamsCacheTimeis deliberately not advanced, so the next request retries rather than pinning the stale list for a full TTL.Known trade-off — worth a second opinion
Moving a custom domain between tenants takes up to 60s to be reflected; until then it keeps routing to the previous tenant. Shortening the TTL trades away most of the benefit, since custom domains are the case that costs two round trips. Flagging explicitly rather than burying it in a comment.
Testing
10 new tests. Verified by mutation — four independent breakages, four distinct failure sets:
isCustomDomaindropped on a hitAlso isolates
cache-passthrough.test.tswith a fresh module per test. Its tests share one hostname, so a module-level route cache would have let the first test populate it and every later test skip the resolution path — quietly no longer exercising the cross-tenant cookie invariants that suite exists to lock. Without this the suite would still have been green while testing less.tsc --noEmit,oxlint,pnpm format:checkclean. Full suite green except the pre-existingpackages/cli/src/dev/worker-runner.test.tsfailures that reproduce on a cleanmain.Still outstanding
D1 read replication is not enabled by this PR. That's a production database configuration change, not code. It would cut the remaining cache-miss latency globally and is the natural companion — worth doing, but it should be a deliberate, separately-approved change.