fix(cloudflare/workers): resolve zone inference with exact name lookups - #1092
Open
aryasaatvik wants to merge 4 commits into
Open
fix(cloudflare/workers): resolve zone inference with exact name lookups#1092aryasaatvik wants to merge 4 commits into
aryasaatvik wants to merge 4 commits into
Conversation
inferZoneIdForHostname listed the account's zones without pagination, so any zone past the first page of 20 failed to resolve. Walk the hostname's label hierarchy with account-scoped ?name= lookups via resolveZoneId instead, keeping the per-run cache. Hoisted to module scope so the walk, the cache and the exhausted-hierarchy defect are covered by unit tests against a stubbed zones API.
sam-goodwin
reviewed
Aug 4, 2026
Comment on lines
+1026
to
+1059
| /** | ||
| * Infer the Cloudflare Zone ID for a given hostname by walking up the DNS | ||
| * label hierarchy with exact, account-scoped `?name=` lookups | ||
| * (`resolveZoneId`). Listing the account's zones and matching locally is a | ||
| * trap: the list endpoint paginates (20 zones per page), so a hostname whose | ||
| * zone sits on a later page silently fails to resolve. | ||
| * | ||
| * `zoneCache` is the caller's per-run memo — one map per reconcile pass, so a | ||
| * Worker with several domains or routes in the same zone looks it up once. | ||
| */ | ||
| export const inferZoneIdForHostname = ( | ||
| hostname: string, | ||
| zoneCache: Map<string, string>, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| const cached = zoneCache.get(hostname); | ||
| if (cached) return cached; | ||
|
|
||
| const { accountId } = yield* yield* CloudflareEnvironment; | ||
| const zoneId = yield* resolveZoneId({ | ||
| accountId, | ||
| zone: undefined, | ||
| hostname, | ||
| }).pipe( | ||
| Effect.catch(() => | ||
| Effect.die( | ||
| `Could not infer Cloudflare Zone for hostname "${hostname}". ` + | ||
| "Ensure the parent zone exists in this account.", | ||
| ), | ||
| ), | ||
| ); | ||
| zoneCache.set(hostname, zoneId); | ||
| return zoneId; | ||
| }); |
Contributor
There was a problem hiding this comment.
Does something like this exist already in Zone.ts? I also think it probably belongs there instead of WorkerProvider?
Contributor
Author
There was a problem hiding this comment.
Yes — the walk itself is resolveZoneId in Zone/lookup.ts, and this PR routes through it; what was left here was just the per-run cache + env resolution + the die message. Moved that wrapper into Zone/lookup.ts next to resolveZoneId and moved the tests with it.
The wrapper is not Worker-specific — it is `resolveZoneId` against the ambient account with a per-run memo. Move it (and its tests) into Zone/lookup.ts next to the walk it delegates to.
sam-goodwin
reviewed
Aug 4, 2026
Comment on lines
+66
to
+85
| zoneCache: Map<string, string>, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| const cached = zoneCache.get(hostname); | ||
| if (cached) return cached; | ||
|
|
||
| const { accountId } = yield* yield* CloudflareEnvironment; | ||
| const zoneId = yield* resolveZoneId({ | ||
| accountId, | ||
| zone: undefined, | ||
| hostname, | ||
| }).pipe( | ||
| Effect.catch(() => | ||
| Effect.die( | ||
| `Could not infer Cloudflare Zone for hostname "${hostname}". ` + | ||
| "Ensure the parent zone exists in this account.", | ||
| ), | ||
| ), | ||
| ); | ||
| zoneCache.set(hostname, zoneId); |
Contributor
There was a problem hiding this comment.
This is not atomic. Should probably store an Effect in the Map and set it to zoneCache.set(hostname, yield* Effect.cached(resolve...)
That way it's only ever computed once.
Contributor
Author
There was a problem hiding this comment.
Done. the map now stores Effect.cached lookups built synchronously inside Effect.suspend (the Action.ts shape) — get-and-set is atomic and the lookup runs once per hostname. Added a concurrency test pinning exactly one fetch for N concurrent resolutions.
The zone cache held resolved ids, so two fibers inferring the same hostname could both miss and each run the full lookup. Store the lookup Effect instead, built with Effect.cached for single-flight and installed inside Effect.suspend so get/construct/set has no yield point between it.
sam-goodwin
reviewed
Aug 4, 2026
Comment on lines
+85
to
+106
| let lookup = zoneCache.get(hostname); | ||
| if (!lookup) { | ||
| lookup = Effect.runSync( | ||
| Effect.cached( | ||
| Effect.gen(function* () { | ||
| const { accountId } = yield* yield* CloudflareEnvironment; | ||
| return yield* resolveZoneId({ | ||
| accountId, | ||
| zone: undefined, | ||
| hostname, | ||
| }).pipe( | ||
| Effect.catch(() => | ||
| Effect.die( | ||
| `Could not infer Cloudflare Zone for hostname "${hostname}". ` + | ||
| "Ensure the parent zone exists in this account.", | ||
| ), | ||
| ), | ||
| ); | ||
| }), | ||
| ), | ||
| ); | ||
| zoneCache.set(hostname, lookup); |
Contributor
There was a problem hiding this comment.
Like this:
if (!zoneCache.has(hostname)) {
const eff = yield* Effect.cached(..)
zoneCache.set(eff)
}
return yield* zoneCache.get(eff)Drop the Effect.suspend + Effect.runSync install in favour of yielding Effect.cached directly. The first suspension inside the lookup is the fetch, which happens after the map is populated, so the single-flight guarantee holds in practice; the residual window costs at most one duplicate GET.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
inferZoneIdForHostnamelisted the account's zones with a single unpaginated call — 20 zones per page, so a Worker domain or route on any zone past the first page failed to resolve.It now walks the hostname's label hierarchy with account-scoped exact-name lookups instead:
Lookups go through
resolveZoneIdand keep the per-run cache; exhausting the hierarchy dies with the same no-zone message as before. The helper moved to module scope so the walk, the cache and the exhausted-hierarchy defect are covered by unit tests against a stubbed zones API.