feat: Multi-Durable Object Support#181
Open
Abdullah-Azbah wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds namespace-aware Durable Object handling to the Cloudflare adapter via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/2.adapters/cloudflare.md`:
- Around line 83-90: The documentation paragraph about Durable Objects has
several grammar and clarity issues; rewrite the section to fix typos and improve
flow by changing "design" to "designed", "There two scenarios" to "There are two
scenarios", and correcting phrases like "which DO handle" to "which DO handles
the request" and "their its" if present; make the two scenarios explicit and
accurate: scenario 1 — enabling useNamespaceAsId only will choose a DO based on
URL().pathname, and scenario 2 — using useNamespaceAsId plus the upgrade() hook
to return an object with a namespace property gives you full control over the DO
instance id; ensure references to useNamespaceAsId, upgrade(), URL().pathname,
and the namespace property remain intact and clearly described.
- Line 82: The heading "Durable Objects" is using ### which jumps from a
higher-level # heading and fails markdown linting; change the heading token for
"Durable Objects" from "### Durable Objects" to "## Durable Objects" so heading
levels increment correctly and satisfy the linter.
In `@src/adapters/cloudflare.ts`:
- Around line 88-93: The publish path crashes because resolveDurableStub calls
hooks.upgrade with _req even when publish() passes undefined; modify
resolveDurableStub to first check opts.useNamespaceAsId && _req is defined
before calling hooks.upgrade, and only attempt to extract namespace and set
instanceName when _req exists; if _req is undefined, skip the upgrade call (or
use a safe fallback) so hooks.upgrade (and any request.url access) is never
invoked with an undefined request.
- Line 57: Update the JSDoc note that currently references the wrong option
name: replace `useNamespaceAsStubId` with the correct option `useNamespaceAsId`
in the comment block alongside `bindingName` and `instanceName` (look for the
JSDoc/notice in src/adapters/cloudflare.ts that mentions those three options to
locate the exact comment).
- Around line 89-93: The code calls hooks.upgrade(_req, { cf: { runtime:
"worker" } }) and only reads namespace, which ignores an early termination
signal; change the logic in the resolver so you do not drop upgrade() rejections
or endResponse values: call hooks.upgrade and (a) wrap it in try/catch to let
any thrown Response/Errors bubble up (rethrow Response objects), and (b) inspect
the returned object for an endResponse flag/value and propagate/return that
immediately instead of continuing; only set instanceName from namespace when no
endResponse/exception occurred. Use the symbols hooks.upgrade, instanceName, and
_req to locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6318912a-0fee-43ef-9d32-e51baca516a0
📒 Files selected for processing (3)
docs/2.adapters/cloudflare.mdsrc/adapters/cloudflare.tssrc/hooks.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/2.adapters/cloudflare.md (1)
83-93:⚠️ Potential issue | 🟡 MinorClarify the Durable Objects section wording and scenario descriptions.
This block still has multiple grammar issues and scenario 2 currently says “You only enable
useNamespaceAsIdand use the upgrade hook…”, which is contradictory. Please tighten the prose and make the two scenarios explicit and distinct.✍️ Suggested rewrite
-By default, the cloudflare adapter uses a single Durable Object (DO) to handle ALL requests. This behavior will create a bottleneck since DOs are design to scale horizontally and only handle about 1000 requests/s see [DO message throughput limits](https://developers.cloudflare.com/durable-objects/best-practices/rules-of-durable-objects/#message-throughput-limits). To address this, you can use the `useNamespaceAsId` option along with the [`upgrade() hook`](/guide/hooks) to control which DO handle which request. +By default, the Cloudflare adapter uses a single Durable Object (DO) to handle all requests. This can become a bottleneck, since DOs are designed to scale horizontally and each object handles about 1000 requests/s. See [DO message throughput limits](https://developers.cloudflare.com/durable-objects/best-practices/rules-of-durable-objects/#message-throughput-limits). To address this, use `useNamespaceAsId` with the [`upgrade()` hook](/guide/hooks) to control which DO handles each request. There are two scenarios for this: 1. You only enable `useNamespaceAsId`. In this case, a DO will be created to handle the request based on the `URL().pathname`. -2. You only enable `useNamespaceAsId` and use the upgrade hook in your route to return an object with a `namespace` property. Here you gain full control over the DO creation. The namespace to get the DO instance id. +2. You enable `useNamespaceAsId` and use the `upgrade()` hook in your route to return an object with a `namespace` property. This gives you full control over DO instance selection via namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/2.adapters/cloudflare.md` around lines 83 - 93, Tighten and clarify the Durable Object section so the two scenarios are explicit and non-contradictory: describe scenario A as "enable useNamespaceAsId only — the Durable Object id is derived from URL().pathname so a DO is created per path", and scenario B as "enable useNamespaceAsId and return a namespace property from your upgrade() hook in the route — this gives you full control over which DO id is used"; also fix the wording around upgrade() running twice to say it runs once on the worker and once on the DO and mention you can use the upgrade() second argument (upgrade context) to distinguish the runs, and ensure you reference useNamespaceAsId, upgrade(), URL().pathname, and the namespace property in the updated prose.src/adapters/cloudflare.ts (1)
88-93:⚠️ Potential issue | 🔴 CriticalGuard request-less namespace resolution and propagate worker-phase upgrade termination.
resolveDurableStubstill callshooks.upgradeeven when_reqis undefined (publish path), and it dropsendResponsefrom the worker-phase upgrade result. This can crashpublish()and can bypass worker-side early termination logic.🛠️ Suggested fix
- if (opts.useNamespaceAsId) { - const { namespace } = await hooks.upgrade( - _req as unknown as Request, - { cf: { runtime: "worker" } }, - ); - if (namespace) instanceName = namespace; + if (opts.useNamespaceAsId && _req) { + const { namespace, endResponse } = await hooks.upgrade( + _req as unknown as Request, + { cf: { runtime: "worker" } }, + ); + if (endResponse) { + throw { response: endResponse }; + } + if (namespace) instanceName = namespace; }- const stub = await resolveDurableStub( - request as CF.Request, - cfEnv, - cfCtx, - ); + let stub; + try { + stub = await resolveDurableStub(request as CF.Request, cfEnv, cfCtx); + } catch (error) { + const endResponse = (error as { response?: Response }).response; + if (endResponse instanceof Response) { + return endResponse; + } + throw error; + }Also applies to: 104-115, 219-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 88 - 93, The resolveDurableStub path calls hooks.upgrade even when _req is undefined and currently drops the worker-phase endResponse; change the code guarded by opts.useNamespaceAsId to only call hooks.upgrade when _req is present, capture the full upgrade result (including endResponse), and propagate/return that endResponse from resolveDurableStub so worker-phase early termination is honored; make the same guard-and-propagation change for the other occurrences of hooks.upgrade in the file (the blocks around the other uses of opts.useNamespaceAsId and the occurrence near symbol names at the later block) so publish paths don’t invoke request-bound upgrade logic and worker-phase termination isn’t lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/2.adapters/cloudflare.md`:
- Around line 83-93: Tighten and clarify the Durable Object section so the two
scenarios are explicit and non-contradictory: describe scenario A as "enable
useNamespaceAsId only — the Durable Object id is derived from URL().pathname so
a DO is created per path", and scenario B as "enable useNamespaceAsId and return
a namespace property from your upgrade() hook in the route — this gives you full
control over which DO id is used"; also fix the wording around upgrade() running
twice to say it runs once on the worker and once on the DO and mention you can
use the upgrade() second argument (upgrade context) to distinguish the runs, and
ensure you reference useNamespaceAsId, upgrade(), URL().pathname, and the
namespace property in the updated prose.
In `@src/adapters/cloudflare.ts`:
- Around line 88-93: The resolveDurableStub path calls hooks.upgrade even when
_req is undefined and currently drops the worker-phase endResponse; change the
code guarded by opts.useNamespaceAsId to only call hooks.upgrade when _req is
present, capture the full upgrade result (including endResponse), and
propagate/return that endResponse from resolveDurableStub so worker-phase early
termination is honored; make the same guard-and-propagation change for the other
occurrences of hooks.upgrade in the file (the blocks around the other uses of
opts.useNamespaceAsId and the occurrence near symbol names at the later block)
so publish paths don’t invoke request-bound upgrade logic and worker-phase
termination isn’t lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dd97db7-3626-4441-9975-982aa58537df
📒 Files selected for processing (2)
docs/2.adapters/cloudflare.mdsrc/adapters/cloudflare.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/adapters/cloudflare.ts (1)
95-99:⚠️ Potential issue | 🟠 MajorStill surface worker-phase
upgrade()rejections before fetching the DO.
AdapterHookable.upgrade()returns a short-circuit response viaendResponse, but this path still reads onlynamespaceand continues intostub.fetch(). WithuseNamespaceAsIdenabled, a hook that intentionally blocks only in the worker runtime can be bypassed here.src/hooks.ts:44-98is the cross-file evidence for that behavior.Possible fix
- const { namespace } = await hooks.upgrade(req as unknown as Request, { - cf: { runtime: "worker" }, - }); + const { namespace, endResponse } = await hooks.upgrade( + req as unknown as Request, + { cf: { runtime: "worker" } }, + ); + if (endResponse) { + throw { response: endResponse }; + } if (namespace) { instanceName = namespace; }- const stub = await resolveDurableStub( - request as CF.Request, - cfEnv, - cfCtx, - ); + let stub: WSDurableObjectStub | undefined; + try { + stub = await resolveDurableStub(request as CF.Request, cfEnv, cfCtx); + } catch (error) { + const response = (error as { response?: Response }).response; + if (response instanceof Response) { + return response; + } + throw error; + }Also applies to: 116-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 95 - 99, The hook upgrade() call can return a short-circuit response via endResponse which must be surfaced immediately instead of continuing into stub.fetch; modify the logic around the hooks.upgrade(req, ...) invocation in the cloudflare adapter (the block using explicitNamespace, opts.useNamespaceAsId and req) to detect if the returned value is an HTTP Response/short-circuit indicator and return it to the caller immediately (i.e., if hooks.upgrade() indicates an endResponse, do not fall through to reading namespace or calling stub.fetch), and apply the same guard to the other similar block (the 116-124 region) so worker-phase rejections are not bypassed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapters/cloudflare.ts`:
- Around line 93-104: The upgrade flow sets instanceName to explicitNamespace or
"crossws" and then optionally overwrites it with the namespace from
hooks.upgrade (symbols: instanceName, explicitNamespace, opts.useNamespaceAsId,
hooks.upgrade), but publishing and fan-out logic treat namespaces inconsistently
(symbols: publish, peer.publish, peer.peers, state.n), causing cross-namespace
leakage or missed peers; fix by making namespace handling deterministic: when
opts.useNamespaceAsId is true use the upgraded namespace as the binding id (call
binding.idFromName with that namespace) and persist/compare the serialized
namespace in connection state (state.n) so publish/fan-out filters by state.n,
and when opts.useNamespaceAsId is false ensure upgrades do not override
instanceName (keep "crossws" or explicitNamespace) so
binding.get(binding.idFromName(instanceName)) and all publish/peer code
consistently derive and compare the same namespace identifier.
- Around line 255-257: The current check treats state.t === undefined as a topic
match, causing unsubscriptions to receive topic messages; update the
topic-filter condition in the loop that uses getAttachedState(ws) so it
continues when a topic is provided but the socket has no subscription set
(state.t is undefined) or does not contain the topic; specifically change the
conditional that currently reads "if (topic && state.t && !state.t.has(topic))
continue" to explicitly handle missing state.t (e.g., continue when topic &&
(!state.t || !state.t.has(topic))) so handleDurablePublish() only sends to
sockets that actually subscribed.
---
Duplicate comments:
In `@src/adapters/cloudflare.ts`:
- Around line 95-99: The hook upgrade() call can return a short-circuit response
via endResponse which must be surfaced immediately instead of continuing into
stub.fetch; modify the logic around the hooks.upgrade(req, ...) invocation in
the cloudflare adapter (the block using explicitNamespace, opts.useNamespaceAsId
and req) to detect if the returned value is an HTTP Response/short-circuit
indicator and return it to the caller immediately (i.e., if hooks.upgrade()
indicates an endResponse, do not fall through to reading namespace or calling
stub.fetch), and apply the same guard to the other similar block (the 116-124
region) so worker-phase rejections are not bypassed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89ffc694-88f5-483d-b3c0-c0ad49cd2913
📒 Files selected for processing (1)
src/adapters/cloudflare.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/adapters/cloudflare.ts (4)
95-99:⚠️ Potential issue | 🟠 MajorDo not drop worker-phase
upgrade()termination signals.At Line 96, only
namespaceis read fromhooks.upgrade(...). IfendResponseis returned, that worker-side decision is ignored and processing continues.Suggested fix
- const { namespace } = await hooks.upgrade(req as unknown as Request, { + const { namespace, endResponse } = await hooks.upgrade( + req as unknown as Request, + { cf: { runtime: "worker" }, - }); + }, + ); + if (endResponse) { + throw endResponse; + } if (namespace) { instanceName = namespace; }- const stub = await resolveDurableStub( - request as CF.Request, - cfEnv, - cfCtx, - ); + let stub: WSDurableObjectStub | undefined; + try { + stub = await resolveDurableStub(request as CF.Request, cfEnv, cfCtx); + } catch (error) { + if (error instanceof Response) { + return error; + } + throw error; + }Also applies to: 116-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 95 - 99, The current code calls hooks.upgrade(req, {cf:{runtime:"worker"}}) but only destructures namespace, which ignores a possible endResponse termination signal returned by hooks.upgrade; update both call sites (the block using explicitNamespace && opts.useNamespaceAsId && req and the similar block around the 116-125 region) to capture the full result (e.g., const { namespace, endResponse } = await hooks.upgrade(...)), check if endResponse is present and, if so, return or handle it immediately instead of continuing, otherwise proceed to use namespace; ensure you reference hooks.upgrade, namespace, and endResponse when making the change.
259-261:⚠️ Potential issue | 🟠 MajorTopic filter currently treats missing subscriptions as a match.
At Line 260, sockets with
state.t === undefinedare not filtered out for topic-scoped publish, so unsubscribed peers can receive messages.Suggested fix
- if (topic && state.t && !state.t.has(topic)) { + if (topic && !state.t?.has(topic)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 259 - 261, The topic filter currently treats missing subscription sets as a match because it only checks state.t.has(topic); update the check in the publish loop that uses getAttachedState(ws) so sockets with state.t === undefined are excluded for topic-scoped publishes: change the condition that currently reads "if (topic && state.t && !state.t.has(topic)) { continue; }" to explicitly reject when state.t is falsy or when it doesn't contain the topic (i.e., if topic && (!state.t || !state.t.has(topic)) { continue; }), referencing getAttachedState, state.t and topic to locate the code to change.
310-316:⚠️ Potential issue | 🟠 MajorEnforce namespace isolation in
peer.publish()fan-out.At Lines 310-316, fan-out checks id and topic but not namespace. In a shared DO, this can publish across namespaces.
Suggested fix
for (const ws of websockets) { const state = getAttachedState(ws); + if (state.n !== this.namespace) { + continue; + } if (state.i === this.id) { continue; } if (state.t?.has(topic)) { ws.send(dataBuff);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 310 - 316, The fan-out loop in peer.publish() only checks peer id and topic membership (getAttachedState(ws), state.i vs this.id, state.t?.has(topic)) but misses namespace isolation; update the loop to skip sockets whose attached state's namespace does not match the publisher's namespace (e.g., compare state.ns (or state.namespace) to this.namespace) before calling ws.send(dataBuff), so only clients in the same namespace receive the message.
93-104:⚠️ Potential issue | 🟠 MajorKeep Durable Object ID resolution gated by
useNamespaceAsId.At Line 93,
explicitNamespaceis always used as the DO id. In shared-object mode (useNamespaceAsId === false), this misroutespublish(..., { namespace })to a different DO than upgrades.Suggested fix
- let instanceName = explicitNamespace || opts.instanceName || "crossws"; + let instanceName = opts.instanceName || "crossws"; + + if (opts.useNamespaceAsId && explicitNamespace) { + instanceName = explicitNamespace; + } - if (!explicitNamespace && opts.useNamespaceAsId && req) { + if (!explicitNamespace && opts.useNamespaceAsId && req) { const { namespace } = await hooks.upgrade(req as unknown as Request, { cf: { runtime: "worker" }, }); if (namespace) { instanceName = namespace; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 93 - 104, The code always prefers explicitNamespace as the Durable Object id even when opts.useNamespaceAsId is false, causing publish(..., { namespace }) to target a different DO than upgrades; change the selection so the explicitNamespace (and the namespace returned from hooks.upgrade) are only used when opts.useNamespaceAsId is true. Concretely, update the logic around instanceName/explicitNamespace and the hooks.upgrade block (symbols: instanceName, explicitNamespace, opts.useNamespaceAsId, hooks.upgrade, binding.get, binding.idFromName) so that when opts.useNamespaceAsId is false you never assign instanceName from explicitNamespace or upgrade.namespace and instead fall back to opts.instanceName or "crossws"; only assign instanceName = namespace or explicitNamespace when opts.useNamespaceAsId === true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapters/cloudflare.ts`:
- Around line 215-221: The durable publish path dereferences opts.namespace
unconditionally which can throw when opts is undefined and also incorrectly
drops recipients when namespace is intentionally omitted; update the loop that
calls getDurablePeers(_obj as DurableObjectPub, topic) to guard access to opts
(e.g., check if opts && opts.namespace !== undefined) and only apply the
namespace filter when opts.namespace is provided, otherwise call peer.send(data)
for all peers; reference getDurablePeers, DurableObjectPub, peer.namespace and
opts.namespace when making the change so the conditional becomes "if (opts &&
opts.namespace !== undefined && peer.namespace !== opts.namespace) continue;".
---
Duplicate comments:
In `@src/adapters/cloudflare.ts`:
- Around line 95-99: The current code calls hooks.upgrade(req,
{cf:{runtime:"worker"}}) but only destructures namespace, which ignores a
possible endResponse termination signal returned by hooks.upgrade; update both
call sites (the block using explicitNamespace && opts.useNamespaceAsId && req
and the similar block around the 116-125 region) to capture the full result
(e.g., const { namespace, endResponse } = await hooks.upgrade(...)), check if
endResponse is present and, if so, return or handle it immediately instead of
continuing, otherwise proceed to use namespace; ensure you reference
hooks.upgrade, namespace, and endResponse when making the change.
- Around line 259-261: The topic filter currently treats missing subscription
sets as a match because it only checks state.t.has(topic); update the check in
the publish loop that uses getAttachedState(ws) so sockets with state.t ===
undefined are excluded for topic-scoped publishes: change the condition that
currently reads "if (topic && state.t && !state.t.has(topic)) { continue; }" to
explicitly reject when state.t is falsy or when it doesn't contain the topic
(i.e., if topic && (!state.t || !state.t.has(topic)) { continue; }), referencing
getAttachedState, state.t and topic to locate the code to change.
- Around line 310-316: The fan-out loop in peer.publish() only checks peer id
and topic membership (getAttachedState(ws), state.i vs this.id,
state.t?.has(topic)) but misses namespace isolation; update the loop to skip
sockets whose attached state's namespace does not match the publisher's
namespace (e.g., compare state.ns (or state.namespace) to this.namespace) before
calling ws.send(dataBuff), so only clients in the same namespace receive the
message.
- Around line 93-104: The code always prefers explicitNamespace as the Durable
Object id even when opts.useNamespaceAsId is false, causing publish(..., {
namespace }) to target a different DO than upgrades; change the selection so the
explicitNamespace (and the namespace returned from hooks.upgrade) are only used
when opts.useNamespaceAsId is true. Concretely, update the logic around
instanceName/explicitNamespace and the hooks.upgrade block (symbols:
instanceName, explicitNamespace, opts.useNamespaceAsId, hooks.upgrade,
binding.get, binding.idFromName) so that when opts.useNamespaceAsId is false you
never assign instanceName from explicitNamespace or upgrade.namespace and
instead fall back to opts.instanceName or "crossws"; only assign instanceName =
namespace or explicitNamespace when opts.useNamespaceAsId === true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb19f96a-6a12-4b6b-a31c-8923f3d59bc7
📒 Files selected for processing (1)
src/adapters/cloudflare.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/adapters/cloudflare.ts (4)
215-223:⚠️ Potential issue | 🔴 CriticalGuard optional
optsand only namespace-filter when provided.Line 219 dereferences
opts.namespaceunconditionally. Ifoptsis undefined, this path can throw; when namespace is omitted, current logic also drops all recipients.Suggested fix
handleDurablePublish: async (_obj, topic, data, opts) => { + const targetNamespace = opts?.namespace; const peers = getDurablePeers(_obj as DurableObjectPub, topic); for (const peer of peers) { - // single Durable Object with multiple namespaces - if (peer.namespace !== opts.namespace) { + if (targetNamespace && peer.namespace !== targetNamespace) { continue; } peer.send(data); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 215 - 223, The handler handleDurablePublish unconditionally accesses opts.namespace and thus can throw or incorrectly filter peers when opts is undefined; change the logic in handleDurablePublish (the loop over peers returned by getDurablePeers for DurableObjectPub) to first check whether opts exists and opts.namespace is defined before applying the namespace filter (i.e., only skip peers based on peer.namespace if opts && opts.namespace is provided); otherwise, deliver to all peers by calling peer.send(data).
95-101:⚠️ Potential issue | 🟠 MajorPropagate worker-phase
upgrade()early termination.
hooks.upgrade()can returnendResponse, but Line 96 currently ignores it and continues resolving the DO stub. This can bypass worker-side gating logic.Suggested fix
- if (!explicitNamespace && opts.useNamespaceAsId && req) { - const { namespace } = await hooks.upgrade(req as unknown as Request, { + if (!explicitNamespace && opts.useNamespaceAsId && req) { + const { namespace, endResponse } = await hooks.upgrade( + req as unknown as Request, + { cf: { runtime: "worker" }, - }); + }, + ); + if (endResponse) { + throw { response: endResponse }; + } if (namespace) { instanceName = namespace; } }- const stub = await resolveDurableStub( - request as CF.Request, - cfEnv, - cfCtx, - ); + let stub: WSDurableObjectStub | undefined; + try { + stub = await resolveDurableStub(request as CF.Request, cfEnv, cfCtx); + } catch (error) { + const errResponse = (error as { response?: Response }).response; + if (errResponse instanceof Response) { + return errResponse as unknown as Response; + } + throw error; + }Also applies to: 115-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 95 - 101, The code calls hooks.upgrade(...) and only reads { namespace } but ignores a possible endResponse that should short-circuit; update both call sites that call hooks.upgrade (the block that sets instanceName and the similar block later) to inspect the full result, detect if result.endResponse is present, and propagate/return that endResponse immediately instead of continuing; if not, continue to use result.namespace to set instanceName. Make sure to reference hooks.upgrade, the result object (including endResponse and namespace), instanceName, and the enclosing function’s return path so the early-termination is propagated to the caller.
261-262:⚠️ Potential issue | 🟠 MajorDo not treat missing subscriptions as a topic match.
With the current condition, sockets with
state.t === undefinedstill pass whentopicis set.Suggested fix
- if (topic && state.t && !state.t.has(topic)) { + if (topic && !state.t?.has(topic)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 261 - 262, The current check allows sockets with undefined subscriptions (state.t) to match a non-empty topic because it only tests !state.t.has(topic); update the condition in the topic-filtering logic (the block referencing topic and state.t) to treat missing subscriptions as non-matches by checking for the existence of state.t first and continuing when topic is set and either state.t is falsy or state.t does not contain the topic; adjust the condition that currently reads "if (topic && state.t && !state.t.has(topic)) continue" to explicitly handle state.t being undefined so sockets without a subscription set are skipped when topic is present.
93-104:⚠️ Potential issue | 🟠 MajorNamespace isolation is still inconsistent in shared-DO mode.
When
useNamespaceAsIdis off,explicitNamespacestill influences DO id selection (Line 93), whilepeer.peers/peer.publishare not namespace-scoped inside a shared DO. This can cause missed publishes and cross-namespace leakage.Suggested fix
- let instanceName = explicitNamespace || opts.instanceName || "crossws"; + let instanceName = + opts.useNamespaceAsId && explicitNamespace + ? explicitNamespace + : opts.instanceName || "crossws";override get peers() { return new Set( - this.#getwebsockets().map((ws) => - CloudflareDurablePeer._restore(this._internal.durable, ws), - ), + this.#getwebsockets() + .filter((ws) => getAttachedState(ws).n === this.namespace) + .map((ws) => CloudflareDurablePeer._restore(this._internal.durable, ws)), ); }for (const ws of websockets) { const state = getAttachedState(ws); + if (state.n !== this.namespace) { + continue; + } if (state.i === this.id) { continue; } if (state.t?.has(topic)) { ws.send(dataBuff); } }Also applies to: 278-283, 311-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cloudflare.ts` around lines 93 - 104, The code incorrectly uses explicitNamespace to select the DO id even when opts.useNamespaceAsId is false, causing namespace leakage; update the selection so instanceName is derived only from opts.instanceName or the default ("crossws") unless opts.useNamespaceAsId is true — only then call hooks.upgrade(req) to read namespace and override instanceName. Concretely, change the logic around instanceName, explicitNamespace, opts.useNamespaceAsId and the hooks.upgrade call in this function (and apply the same fix to the analogous blocks at the other two locations) so explicitNamespace does not influence binding.idFromName when useNamespaceAsId is disabled and ensure peer.peers/peer.publish remain namespace-scoped only when useNamespaceAsId is true.docs/2.adapters/cloudflare.md (1)
83-93:⚠️ Potential issue | 🟡 MinorClarify the Durable Objects behavior text and fix grammar.
This section still has multiple wording/grammar issues and one contradictory phrase in scenario 2 (
only enable+and use). Please tighten this block for accuracy.Suggested rewrite
-By default, the cloudflare adapter uses a single Durable Object (DO) to handle ALL requests. This behavior will create a bottleneck since DOs are design to scale horizontally and only handle about 1000 requests/s see [DO message throughput limits](https://developers.cloudflare.com/durable-objects/best-practices/rules-of-durable-objects/#message-throughput-limits). To address this, you can use the `useNamespaceAsId` option along with the [`upgrade() hook`](/guide/hooks) to control which DO handle which request. +By default, the Cloudflare adapter uses a single Durable Object (DO) to handle all requests. This can become a bottleneck because DOs are designed to scale horizontally, and each object handles roughly 1000 requests/s. See [DO message throughput limits](https://developers.cloudflare.com/durable-objects/best-practices/rules-of-durable-objects/#message-throughput-limits). To address this, use `useNamespaceAsId` with the [`upgrade()` hook](/guide/hooks) to control which DO handles each request. @@ -2. You only enable `useNamespaceAsId` and use the upgrade hook in your route to return an object with a `namespace` property. Here you gain full control over the DO creation. The namespace to get the DO instance id. +2. Enable `useNamespaceAsId` and use the `upgrade()` hook to return `{ namespace }`. This gives you full control over DO instance selection. @@ -> When you enable the `useNamespaceAsId` option, your `upgrade()` hook will run twice!. First it will run on the worker to check if you have returned a `namespace`. Then it will run on the DO once the connection is passed to it. You can use the second argument of the `upgrade()` hook which contains the upgrade context. +> When `useNamespaceAsId` is enabled, the `upgrade()` hook can run twice: first in the Worker to derive namespace routing, then in the DO after handoff. Use the second `upgrade()` argument to inspect runtime context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/2.adapters/cloudflare.md` around lines 83 - 93, Rewrite the paragraph to fix grammar, remove the contradiction in scenario 2, and clarify DO behavior: state that by default one Durable Object handles all requests (causing a bottleneck because DOs have ~1000 req/s limits), explain that enabling the useNamespaceAsId option lets you shard DOs (by URL().pathname by default), and that using useNamespaceAsId together with the upgrade() hook (returning an object with a namespace property) gives full control over which DO instance handles a request; also correct the note to say upgrade() runs twice (once in the worker, once in the DO) and that the hook’s second argument contains the upgrade context. Include references to useNamespaceAsId, upgrade(), and URL().pathname so readers can locate relevant docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/2.adapters/cloudflare.md`:
- Line 100: Remove the stray blank line inside the blockquote labeled
"code-note" (the blockquote containing the example/note) so the quoted lines are
continuous and MD028 is resolved; edit the blockquote in
docs/2.adapters/cloudflare.md to merge the separated lines into a single
uninterrupted blockquote paragraph.
- Line 113: The documentation line for resolveDurableStub is incomplete: update
the note for resolveDurableStub to state that overriding it also overrides
useNamespaceAsId in addition to bindingName and instanceName; mention the
symbols resolveDurableStub, bindingName, instanceName, and useNamespaceAsId so
readers know all three options are affected when providing a custom
resolveDurableStub implementation.
---
Duplicate comments:
In `@docs/2.adapters/cloudflare.md`:
- Around line 83-93: Rewrite the paragraph to fix grammar, remove the
contradiction in scenario 2, and clarify DO behavior: state that by default one
Durable Object handles all requests (causing a bottleneck because DOs have ~1000
req/s limits), explain that enabling the useNamespaceAsId option lets you shard
DOs (by URL().pathname by default), and that using useNamespaceAsId together
with the upgrade() hook (returning an object with a namespace property) gives
full control over which DO instance handles a request; also correct the note to
say upgrade() runs twice (once in the worker, once in the DO) and that the
hook’s second argument contains the upgrade context. Include references to
useNamespaceAsId, upgrade(), and URL().pathname so readers can locate relevant
docs.
In `@src/adapters/cloudflare.ts`:
- Around line 215-223: The handler handleDurablePublish unconditionally accesses
opts.namespace and thus can throw or incorrectly filter peers when opts is
undefined; change the logic in handleDurablePublish (the loop over peers
returned by getDurablePeers for DurableObjectPub) to first check whether opts
exists and opts.namespace is defined before applying the namespace filter (i.e.,
only skip peers based on peer.namespace if opts && opts.namespace is provided);
otherwise, deliver to all peers by calling peer.send(data).
- Around line 95-101: The code calls hooks.upgrade(...) and only reads {
namespace } but ignores a possible endResponse that should short-circuit; update
both call sites that call hooks.upgrade (the block that sets instanceName and
the similar block later) to inspect the full result, detect if
result.endResponse is present, and propagate/return that endResponse immediately
instead of continuing; if not, continue to use result.namespace to set
instanceName. Make sure to reference hooks.upgrade, the result object (including
endResponse and namespace), instanceName, and the enclosing function’s return
path so the early-termination is propagated to the caller.
- Around line 261-262: The current check allows sockets with undefined
subscriptions (state.t) to match a non-empty topic because it only tests
!state.t.has(topic); update the condition in the topic-filtering logic (the
block referencing topic and state.t) to treat missing subscriptions as
non-matches by checking for the existence of state.t first and continuing when
topic is set and either state.t is falsy or state.t does not contain the topic;
adjust the condition that currently reads "if (topic && state.t &&
!state.t.has(topic)) continue" to explicitly handle state.t being undefined so
sockets without a subscription set are skipped when topic is present.
- Around line 93-104: The code incorrectly uses explicitNamespace to select the
DO id even when opts.useNamespaceAsId is false, causing namespace leakage;
update the selection so instanceName is derived only from opts.instanceName or
the default ("crossws") unless opts.useNamespaceAsId is true — only then call
hooks.upgrade(req) to read namespace and override instanceName. Concretely,
change the logic around instanceName, explicitNamespace, opts.useNamespaceAsId
and the hooks.upgrade call in this function (and apply the same fix to the
analogous blocks at the other two locations) so explicitNamespace does not
influence binding.idFromName when useNamespaceAsId is disabled and ensure
peer.peers/peer.publish remain namespace-scoped only when useNamespaceAsId is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bad83f2a-c156-4bc8-aa6f-92658153c362
📒 Files selected for processing (2)
docs/2.adapters/cloudflare.mdsrc/adapters/cloudflare.ts
# Conflicts: # src/adapters/cloudflare.ts # src/hooks.ts
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.
Introduction
This PR introduces a new configuration flag to the Cloudflare Durable Object adapter. It allows users to resolve a DO stub using the peer namespace as the identifier. This is especially useful when used with other frameworks like Nitro and Nuxt. In Nuxt, for example, all connections will be sent to the same durable object with no clean way to change this behavior.
Work done
useNamespaceAsIdwas added to the Cloudflare adapter. Enabling this option causes the DO stub to be resolved based on the peer namespace. As such, developers can now use theupgrade()hook to have full control over which DO handles which request.upgrade()hook needs to be called twice whenuseNamespaceAsIdis enabled. First to resolve the namespace on the Worker (if provided) and again once the request reaches the DO instance. To allow differentiating between the two runs, theupgrade()hook now optionally accepts a secondUpgradeContextargument indicating which runtime ("worker"or"DO") invoked the hook.getDurablePeers()helper enumerates peers safely viactx.getWebSockets()and is the supported way to list a DO's peers.Breaking changes
Durable Object peers are no longer exposed via
crossws.peers.Holding peer references across requests / Durable Objects on Cloudflare triggers an
I/O on behalf of a different requesterror (and is empty across isolates anyway), so the adapter-levelpeersmap no longer contains Durable Object peers. To list peers from inside a DO, use the newgetDurablePeers()helper, optionally filtered by topic:The in-Worker fallback path (no Durable Object) is unaffected —
crossws.peersandpeer.peerscontinue to work as before.peer.peersinside a Durable Object also continues to work.Review fixes
The following were addressed during review:
getDurablePeers()is now declared on the publicCloudflareDurableAdaptertype (was returned but untyped).handleDurablePublishdropping messages whenoptswas provided without anamespace(e.g.publish(topic, data, { compress: true })); the namespace filter now only applies when a namespace is actually given.peers/peer.peersworking (no API change for that path)./peersroute now demonstratesgetDurablePeers()via a DO RPC).This PR also addresses the following PR in Nitro: #3875, in a much cleaner way.
Thank you for maintaining such a great ecosystem! I've been enjoying using Nuxt and am happy to contribute this improvement to crossws. I'm looking forward to your review and feedback on this implementation!
Summary by CodeRabbit
New Features
Improvements
Documentation