Repository navigation
STOR-5615: Set the Durable Object retry policy per stub - #7672
Conversation
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: api-compat, tests, docs, compat-flags, design-simplicity, jsg-gc, kj-style, memory-safety, rust-first (no author changes in their files since the last review) Reviewed commit: 01bb2e6d · github run |
54a66ca to
c93799f
Compare
c93799f to
0e92560
Compare
0e92560 to
92896c8
Compare
<title>502 Bad Gateway</title>
502 Bad Gatewaynginx |
|
@apeacock1991 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
2 similar comments
|
@apeacock1991 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
@apeacock1991 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
7c45ce4 to
cb865fa
Compare
maxmcd
left a comment
There was a problem hiding this comment.
If the stub is serialized, is the retry policy dropped? Does that matter?
64c72be to
f220985
Compare
b325f92 to
c0394c2
Compare
c0394c2 to
9565a40
Compare
18e72d1 to
91a11d3
Compare
Add a `retry` option to DurableObjectNamespace's get(), getByName()
and getExisting():
env.MY_DO.get(id, { retry: { maxRetries: 5, timeoutMs: 10_000 } })
The policy belongs to the returned stub, so each call site chooses
its own, and ctx.exports namespaces accept it the same way as env
bindings. maxRetries must be an integer from 0 to 10 and timeoutMs
an integer from 500 to 60000. Both fields reject non-number values,
including null, with a TypeError rather than coercing them. Other
numbers outside those limits, including fractions, throw a RangeError.
Omitted fields use the runtime's defaults. Passing `retry: null` is
equivalent to `retry: {}` and uses the runtime's defaults.
The option is experimental: without the `experimental` compatibility
flag it is absent from the public types and never read. Workers with
the flag get separate get(), getByName() and getExisting()
registrations whose options dictionary has the `retry` field; other
workers keep the dictionary without it, so neither a bad value nor a
throwing getter can affect them. With the flag, the option is always
validated, but the policy applies only while the
DURABLE_OBJECT_RETRIES_USERLAND autogate is on, so turning the gate
on changes retry behaviour without changing which inputs throw.
maxRetries counts retries after the initial attempt, so the
UserDefinedRetryPolicy field and its constants are renamed from
maxAttempts to match. ActorRetryPolicy::maxAttempts() keeps its name,
since it counts every attempt.
This replaces the binding-level retryPolicy from #7383, so that field,
its copy in compiled-bindings.capnp and the namespace constructor
parameter that carried it are removed. Cap'n Proto needs the ordinals
kept, so the fields become obsolete2 and obsolete28.
91a11d3 to
01bb2e6
Compare
🤖 AI code review💬 Approved with comments — No blocking or warning findings. The only remaining item is a SUGGESTION to rename a stale test and its comment, which still describe the removed binding-level policy. Findings: 1 suggestion The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality: 💬 1 commentThe per-stub Recommendation: 💬 Optional Nits
View full review
Checked with no issues found:
✅ All other checks passed🔒 Security: ✅ LGTMNo security concerns. Input values are validated, and the option only takes effect with the 📖 AGENTS.md: ✅ LGTMThis PR doesn't change any AGENTS.md file, and no guidance needs updating. A suggestion about the length of the root AGENTS.md was dropped because it has nothing to do with this change. ⚡ Performance: ✅ LGTMValidation is O(1) per stub creation, and copying the options is trivial. 📝 Documentation: ✅ LGTMNo documentation issues found. 📘 How the review decision works
Reviewed commit: |
Adds a
retryoption when creating a stub on aDurableObjectNamespace, replacing the binding-levelretryPolicyfrom #7383.The policy belongs to the returned stub, so each call site chooses its own.
envbindings andctx.exportsbehave the same.maxRetries: an integer from 0 to 10, counting retries after the initial attempt.maxRetries: 4means five attempts in total.timeoutMs: an integer from 500 to 60000.maxRetriesandtimeoutMsreject non-number values, includingnull, with aTypeErrorrather than coercing them. Other numbers outside the limits, including fractions, throw aRangeError. Omitted fields use the runtime's defaults.retry: nullis equivalent toretry: {}and uses the runtime's defaults.experimentalcompatibility flag it is ignored and absent from the public types. Workers with the flag get separateget()/getByName()/getExisting()registrations whose options dictionary hasretry; other workers keep the dictionary without it, so the property is never read for them and neither a bad value nor a throwing getter can affect them.DURABLE_OBJECT_RETRIES_USERLANDautogate is on. Turning the gate on changes retry behaviour, not which inputs throw.The stub already carried its own policy (
GlobalActorOutgoingFactory), sogetImpl()now fills it from the options instead of from the namespace.UserDefinedRetryPolicy::maxAttemptsand its constants are renamed tomaxRetriesto match the option.ActorRetryPolicy::maxAttempts()keeps its name, since it counts every attempt. The binding field, its copy incompiled-bindings.capnpand the namespace constructor parameter are removed. Embedders that pass a policy to that constructor need updating. Cap'n Proto needs the ordinals kept, so the fields becomeobsolete2 @2andobsolete28 @28(both:AnyPointer). Nothing set it: the Miniflare change that would have (cloudflare/workers-sdk#15872) has been closed.