Repository navigation
STOR-5615: Replace @retryable with retryable() - #7617
Conversation
|
Since last review: 0 resolved, 0 still open, 1 new. Replaces the retryable decorator with a variadic function API. Not re-run: api-compat, tests, docs, compat-flags, design-simplicity, jsg-gc, kj-style, memory-safety (no author changes in their files since the last review) Reviewed commit: 4be0c016 · github run |
0eed613 to
dde933d
Compare
dde933d to
95dd385
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7617 +/- ##
==========================================
+ Coverage 38.51% 38.60% +0.08%
==========================================
Files 868 868
Lines 268035 268492 +457
Branches 25329 25428 +99
==========================================
+ Hits 103241 103654 +413
Misses 150619 150619
- Partials 14175 14219 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think retryPolicy is attached at the wrong layer. Durable Objects are configured as exports; namespace bindings should only reference those exports.
This is observable with ctx.exports: Global::DurableActorNamespace receives the binding’s userDefinedRetryPolicy, while Global::LoopbackDurableActorNamespace explicitly passes kj::none. Consequently, calls through ctx.exports.MyDO always use the system defaults.
Could we store the policy on the durable actor/export configuration and propagate it when constructing both regular and loopback namespaces? That would give the DO consistent retry behavior regardless of whether it is accessed through env or ctx.exports, without duplicating policy across bindings.
@jamesopstad do we have any data on the number of namespaces/accounts using exports? As my only concern with this approach is it requires them to be using exports, and I am not sure those have propagated very far yet. My assumption would be the vast majority of people are using the top-level binding, so limiting it to exports is going to hinder its use, even if moving to exports is easy. Unless I am misunderstanding how exports/top-level bindings interact, but this is my assumption. |
In It's really about the ownership model too. I think DOs, Workflows, WorkerEntrypoints etc. should be configured by the Worker that owns them. Bindings should not contain configuration. |
Discussed a little internally, I think it's best to support both for now. exports is great, but it's still relatively new and most, I would wager, are not using it or have migrated yet. Putting "migrate to exports to use this" in the changelog is not really great, even if it's easy enough to do. I think we can support both, however, with exports being preferred if set, else it uses the binding's config, else the system defaults. |
|
@jamesopstad can we also untangle these two things also? This one is purely the replacement of the decorator, which has no bearing on the bindings/exports. I can then open another workerd PR to handle any changes to the retry policy (+ update the Workers SDK PR) and EWC (tweaks needed to map for handling both bindings + exports) |
Yes, I've approved this PR. I originally came back and unapproved it when I understood what was happening in the workers-sdk PR but you're right that this PR is separate to the issues we're discussing can be merged as is. |
The @retryable decorator from cloudflare:durable-objects needs a build step: V8 does not parse decorators, and Vite's Oxc transform does not lower them. retryable() is now a plain function that marks methods on the class prototype, usually from a static block: class Counter extends DurableObject { static { retryable(this.prototype.reset); } async reset() { ... } } It takes any number of functions, sets the V8 private key the decorator set, and returns undefined. If any argument is not a function, it throws a TypeError and marks nothing. The decorator's context checks and its warning about later decorators are gone. The mark is on the function object, so the claim check, method lookup and DURABLE_OBJECT_RETRIES_USERLAND gate are unchanged. An inherited method stays marked; an override, bound copy or wrapper does not. Methods stay on the prototype, and RPC still refuses own instance properties. cloudflare:durable-objects still exports only retryable, now with a different signature. The decorator form shipped in v1.20260930.1 but is undocumented and its effect is gated off, so we accept the risk of breaking a Worker that uses it.
95dd385 to
4be0c01
Compare
retryable()fromcloudflare:durable-objectsis now a plain function instead of a method decorator. It marks Durable Object methods as safe to run more than once, usually from a static block next to the method:The decorator needed a build step. V8 doesn't parse decorators, and Vite's Oxc transform doesn't lower them, so Vite users couldn't use it. An earlier version of this PR used a
"use retry"directive instead, with the runtime scanning the start of the method's source. A function call needs no source parsing, marks the exact function, and fails loudly on a typo, so I went with that.retryable()takes any number of functions, sets the V8 private key the decorator set, and returnsundefined. If any argument isn't a function, it throws aTypeErrorand marks nothing. The decorator's context checks and its warning about later decorators are gone.The mark is on the function object, so the claim check, the method lookup, and the
DURABLE_OBJECT_RETRIES_USERLANDgate don't change. An inherited method stays marked. An override, a bound copy, or a wrapper doesn't. Methods must be on the prototype: RPC still refuses own instance properties, so a class field likereset = retryable(...)isn't supported.cloudflare:durable-objectsstill exports onlyretryable, now with a different signature. The decorator form shipped in v1.20260930.1, but it's undocumented and the autogate keeps its effect off, so we accept the risk of breaking a Worker that uses it.