Repository navigation
STOR-5615: Move the Durable Object retry policy to the namespace - #7672
Open
apeacock1991 wants to merge 1 commit into
Open
apeacock1991 wants to merge 1 commit into
apeacock1991 wants to merge 1 commit into
Conversation
#7383 put retryPolicy on DurableObjectNamespaceDesignator, so each binding carried its own policy. ctx.exports namespaces have no binding config and always passed kj::none, so calls through ctx.exports.MyDO used the defaults even when env.MY_DO did not. The policy belongs to the Worker that exports the Durable Object, not to whoever calls it. retryPolicy now lives on Worker.DurableObjectNamespace and is stored in Server::Durable. Every namespace built from that config uses it: env bindings in the owning Worker, bindings from other services through serviceName, and ctx.exports. Callers can no longer choose their own limits. The limit checks move with it and now report the namespace instead of the binding. An ephemeralLocal namespace with a retry policy is a config error, since ephemeral namespaces never retry through it. The binding field shipped in v1.20260925.2, but nothing sets it yet; the Miniflare change that would has not merged. Cap'n Proto needs the ordinal kept, so it becomes obsolete2 and the runtime ignores it.
Contributor
|
Review: 2 findings (1 blocking, 1 warning). Moves Durable Object retry policy configuration from bindings to namespaces and routes it through bindings and Reviewed commit: 54a66cab · github run |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| .enableSql = ns.getEnableSql(), | ||
| .containerOptions = ns.hasContainer() ? kj::Maybe(ns.getContainer()) : kj::none}); | ||
| .containerOptions = ns.hasContainer() ? kj::Maybe(ns.getContainer()) : kj::none, | ||
| .retryPolicy = readRetryPolicy(name, ns)}); |
Contributor
There was a problem hiding this comment.
[WARNING] The new tests cover only rejected configuration. They never enable durable-object-retries-userland and invoke a namespace with a valid policy, so dropping either this assignment or the analogous ctx.exports assignment at line 6202 would still pass. Add a server-level retry test using a failing actor and maxAttempts = 0 or 1, and exercise both the binding and ctx.exports namespace paths.
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
1 participant
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.
The retry policy from #7383 now belongs to the Durable Object namespace, not to each binding.
With the policy on
DurableObjectNamespaceDesignator, every binding carried its own limits.ctx.exportsnamespaces have no binding config, so they always passedkj::none. A Worker could set a policy onenv.MY_DOand still get the defaults throughctx.exports.MyDO, for the same object.The Worker that exports a Durable Object should decide how hard calls to it retry, not each caller. So
retryPolicymoves toWorker.DurableObjectNamespace:Server::Durablestores it, and every namespace built from that config uses it:envbindings in the owning WorkerserviceName, which already resolve to the owner'sDurablectx.exportsCallers can't pick their own limits any more. The limit checks moved with the field and now name the namespace in the error. A retry policy on an
ephemeralLocalnamespace is a config error.The binding field shipped in v1.20260925.2, but nothing sets it. The Miniflare change that would (cloudflare/workers-sdk#15872) hasn't merged and is being reworked to send the policy per class. Cap'n Proto won't let us drop the ordinal, so the field is now
obsolete2 @2 :AnyPointerand the runtime ignores it.Edgeworker needs no change. It reads the policy from each actor namespace global, and its control plane will fill those in from the namespace.