Skip to content

Commit 05599f4

Browse files
committed
STOR-5489: Exempt actor fetch replays from subrequest limits
Count the initial Durable Object fetch against the per-invocation subrequest budget, but bypass the check for physical replay attempts. This preserves one budget unit per logical fetch even when delivery is retried.
1 parent 12537e8 commit 05599f4

7 files changed

Lines changed: 149 additions & 37 deletions

File tree

src/workerd/api/actor-fetch-retry-test.c++

Lines changed: 81 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ class RetryMetadataOutgoingFactory final: public Fetcher::OutgoingFactory {
7777
}
7878

7979
kj::Own<WorkerInterface> newSingleUseClientWithActorRetryMetadata(kj::Maybe<kj::String>,
80-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) override {
80+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
81+
IsActorFetchRetryAttempt isRetryAttempt) override {
8182
capturedMetadata = kj::mv(actorRetryRequestMetadata);
8283
return kj::heap<MockFetchTarget>();
8384
}
@@ -100,6 +101,7 @@ struct ReplayState {
100101
bool acceptWebSocket = false;
101102
kj::Maybe<kj::Own<kj::WebSocket>> acceptedWebSocket;
102103
kj::Vector<IoChannelFactory::ActorRetryRequestMetadata> metadata;
104+
kj::Vector<IsActorFetchRetryAttempt> retryAttempts;
103105
kj::Vector<kj::Array<kj::byte>> requestBodies;
104106
uint requestCount = 0;
105107
uint webSocketRequestCount = 0;
@@ -192,8 +194,10 @@ class ReplayOutgoingFactory final: public Fetcher::OutgoingFactory {
192194
}
193195

194196
kj::Own<WorkerInterface> newSingleUseClientWithActorRetryMetadata(kj::Maybe<kj::String>,
195-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) override {
197+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
198+
IsActorFetchRetryAttempt isRetryAttempt) override {
196199
state.metadata.add(KJ_REQUIRE_NONNULL(actorRetryRequestMetadata));
200+
state.retryAttempts.add(isRetryAttempt);
197201
return kj::heap<ReplayFetchTarget>(state);
198202
}
199203

@@ -314,6 +318,28 @@ class RecordingActorChannel final: public IoChannelFactory::ActorChannel {
314318
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata>& capturedMetadata;
315319
};
316320

321+
class ReplayActorChannel final: public IoChannelFactory::ActorChannel {
322+
public:
323+
ReplayActorChannel(ReplayState& state): state(state) {}
324+
325+
kj::Own<WorkerInterface> startRequest(IoChannelFactory::SubrequestMetadata metadata) override {
326+
state.metadata.add(KJ_REQUIRE_NONNULL(metadata.actorRetryRequestMetadata));
327+
return kj::heap<ReplayFetchTarget>(state);
328+
}
329+
330+
void requireAllowsTransfer() override {
331+
KJ_UNIMPLEMENTED("not used in this test");
332+
}
333+
334+
kj::OneOf<kj::Array<byte>, kj::Promise<kj::Array<byte>>> getTokenMaybeSync(
335+
IoChannelFactory::ChannelTokenUsage) override {
336+
KJ_UNIMPLEMENTED("not used in this test");
337+
}
338+
339+
private:
340+
ReplayState& state;
341+
};
342+
317343
struct ActorIoChannelFactory final: public TestFixture::DummyIoChannelFactory {
318344
ActorIoChannelFactory(TimerChannel& timer,
319345
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata>& capturedMetadata,
@@ -438,6 +464,11 @@ KJ_TEST("actor fetch updates retry metadata and rewinds the body") {
438464
KJ_EXPECT(state.metadata[1].isRetry == IsActorRetry::NO);
439465
KJ_EXPECT(state.metadata[2].isRetry == IsActorRetry::YES);
440466
KJ_EXPECT(state.metadata[3].isRetry == IsActorRetry::YES);
467+
KJ_ASSERT(state.retryAttempts.size() == 4);
468+
KJ_EXPECT(state.retryAttempts[0] == IsActorFetchRetryAttempt::NO);
469+
KJ_EXPECT(state.retryAttempts[1] == IsActorFetchRetryAttempt::YES);
470+
KJ_EXPECT(state.retryAttempts[2] == IsActorFetchRetryAttempt::YES);
471+
KJ_EXPECT(state.retryAttempts[3] == IsActorFetchRetryAttempt::YES);
441472
KJ_ASSERT(state.requestBodies.size() == 4);
442473
for (auto& body: state.requestBodies) {
443474
KJ_EXPECT(body == "request body"_kj.asBytes());
@@ -553,10 +584,39 @@ KJ_TEST("actor fetch stops when the retry budget expires") {
553584
KJ_EXPECT(state.retryCount == 1);
554585
}
555586

556-
// Global actor factories must place the caller's metadata on the actor subrequest.
557-
KJ_TEST("GlobalActorOutgoingFactory places actor retry metadata on the actor subrequest") {
587+
KJ_TEST("actor fetch replay consumes one subrequest limit unit") {
588+
ReplayState state{.failures = kj::arr(ReplayFailure::NOT_DELIVERED)};
589+
uint checkedSubrequestCount = 0;
590+
TestFixture fixture(TestFixture::SetupParams{
591+
.useRealTimers = true,
592+
.checkedSubrequestCount = checkedSubrequestCount,
593+
});
594+
util::Autogate::initAutogateNamesForTest(
595+
{"durable-object-retries-fetch"_kj,
596+
"durable-object-retries-fetch-retry-requests"_kj},
597+
util::IgnoreAllAutogatesEnv::YES);
598+
599+
fixture.runInIoContext([&](const TestFixture::Environment& env) {
600+
auto fetcher = env.js.alloc<Fetcher>(
601+
env.context.addObject<Fetcher::OutgoingFactory>(kj::heap<ReplicaActorOutgoingFactory>(
602+
kj::refcounted<ReplayActorChannel>(state), kj::str("actor-id"))),
603+
Fetcher::RequiresHostAndProtocol::YES);
604+
auto promise = fetcher->fetch(env.js, kj::str("http://example.com"), kj::none);
605+
return env.context.awaitJs(env.js, kj::mv(promise)).ignoreResult().attach(kj::mv(fetcher));
606+
});
607+
608+
KJ_EXPECT(state.requestCount == 2);
609+
KJ_ASSERT(state.metadata.size() == 2);
610+
KJ_EXPECT(state.metadata[0].nonce != state.metadata[1].nonce);
611+
KJ_EXPECT(state.metadata[0].isRetry == IsActorRetry::NO);
612+
KJ_EXPECT(state.metadata[1].isRetry == IsActorRetry::NO);
613+
KJ_EXPECT(checkedSubrequestCount == 1);
614+
}
615+
616+
KJ_TEST("GlobalActorOutgoingFactory forwards metadata without counting retries against limits") {
558617
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> capturedMetadata;
559618
uint channelCount = 0;
619+
uint checkedSubrequestCount = 0;
560620
kj::Vector<kj::String> locationHints;
561621
kj::Vector<kj::String> cohorts;
562622
TestFixture fixture(TestFixture::SetupParams{
@@ -566,6 +626,7 @@ KJ_TEST("GlobalActorOutgoingFactory places actor retry metadata on the actor sub
566626
return kj::rc<ActorIoChannelFactory>(
567627
timer, capturedMetadata, channelCount, locationHints, cohorts);
568628
}),
629+
.checkedSubrequestCount = checkedSubrequestCount,
569630
});
570631

571632
fixture.runInIoContext([&](const TestFixture::Environment& env) {
@@ -581,7 +642,8 @@ KJ_TEST("GlobalActorOutgoingFactory places actor retry metadata on the actor sub
581642
.nonce = 0x123456789abcdef0,
582643
.createdAt = kj::UNIX_EPOCH + 123 * kj::MILLISECONDS,
583644
.isRetry = IsActorRetry::YES,
584-
});
645+
},
646+
IsActorFetchRetryAttempt::NO);
585647

586648
KJ_IF_SOME(metadata, capturedMetadata) {
587649
KJ_EXPECT(metadata.nonce == 0x123456789abcdef0);
@@ -597,7 +659,9 @@ KJ_TEST("GlobalActorOutgoingFactory places actor retry metadata on the actor sub
597659
.nonce = 0xfedcba9876543210,
598660
.createdAt = kj::UNIX_EPOCH + 456 * kj::MILLISECONDS,
599661
.isRetry = IsActorRetry::YES,
600-
});
662+
},
663+
IsActorFetchRetryAttempt::YES);
664+
KJ_EXPECT(checkedSubrequestCount == 1);
601665
KJ_EXPECT(channelCount == 2);
602666
KJ_ASSERT(locationHints.size() == 2);
603667
KJ_EXPECT(locationHints[0] == "location");
@@ -608,9 +672,13 @@ KJ_TEST("GlobalActorOutgoingFactory places actor retry metadata on the actor sub
608672
});
609673
}
610674

611-
KJ_TEST("ReplicaActorOutgoingFactory places actor retry metadata on the actor subrequest") {
675+
KJ_TEST("ReplicaActorOutgoingFactory forwards metadata without counting retries against limits") {
612676
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> capturedMetadata;
613-
TestFixture fixture;
677+
uint checkedSubrequestCount = 0;
678+
TestFixture fixture(TestFixture::SetupParams{
679+
.useRealTimers = false,
680+
.checkedSubrequestCount = checkedSubrequestCount,
681+
});
614682

615683
fixture.runInIoContext([&](const TestFixture::Environment& env) {
616684
ReplicaActorOutgoingFactory factory(
@@ -622,7 +690,8 @@ KJ_TEST("ReplicaActorOutgoingFactory places actor retry metadata on the actor su
622690
.nonce = 0x123456789abcdef0,
623691
.createdAt = kj::UNIX_EPOCH + 123 * kj::MILLISECONDS,
624692
.isRetry = IsActorRetry::YES,
625-
});
693+
},
694+
IsActorFetchRetryAttempt::NO);
626695

627696
KJ_IF_SOME(metadata, capturedMetadata) {
628697
KJ_EXPECT(metadata.nonce == 0x123456789abcdef0);
@@ -638,7 +707,9 @@ KJ_TEST("ReplicaActorOutgoingFactory places actor retry metadata on the actor su
638707
.nonce = 0xfedcba9876543210,
639708
.createdAt = kj::UNIX_EPOCH + 456 * kj::MILLISECONDS,
640709
.isRetry = IsActorRetry::YES,
641-
});
710+
},
711+
IsActorFetchRetryAttempt::YES);
712+
KJ_EXPECT(checkedSubrequestCount == 1);
642713
KJ_IF_SOME(metadata, capturedMetadata) {
643714
KJ_EXPECT(metadata.nonce == 0xfedcba9876543210);
644715
} else {

src/workerd/api/actor.c++

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,19 @@ namespace {
2323
// accumulate.
2424
constexpr size_t ESTIMATED_EXTERNAL_MEMORY_PER_ACTOR_CHANNEL = 32768;
2525

26+
template <typename StartRequest>
27+
kj::Own<WorkerInterface> startActorSubrequest(IoContext& context,
28+
IsActorFetchRetryAttempt isRetryAttempt,
29+
StartRequest& startRequest) {
30+
auto options = IoContext::SubrequestOptions{.inHouse = true,
31+
.wrapMetrics = true,
32+
.operationName = kj::ConstString("durable_object_subrequest"_kjc)};
33+
auto client = isRetryAttempt
34+
? context.getSubrequestNoChecks(startRequest, kj::mv(options))
35+
: context.getSubrequest(startRequest, kj::mv(options));
36+
return context.getMetrics().wrapActorSubrequestClient(kj::mv(client));
37+
}
38+
2639
} // namespace
2740

2841
IoChannelFactory::ActorChannel& LocalActorOutgoingFactory::getOrCreateActorChannel(
@@ -100,27 +113,26 @@ void GlobalActorOutgoingFactory::onActorFetchRetry() {
100113

101114
kj::Own<WorkerInterface> GlobalActorOutgoingFactory::newSingleUseClient(
102115
kj::Maybe<kj::String> cfStr) {
103-
return newSingleUseClientWithActorRetryMetadata(kj::mv(cfStr), kj::none);
116+
return newSingleUseClientWithActorRetryMetadata(
117+
kj::mv(cfStr), kj::none, IsActorFetchRetryAttempt::NO);
104118
}
105119

106120
kj::Own<WorkerInterface> GlobalActorOutgoingFactory::newSingleUseClientWithActorRetryMetadata(
107121
kj::Maybe<kj::String> cfStr,
108-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) {
122+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
123+
IsActorFetchRetryAttempt isRetryAttempt) {
109124
auto& context = IoContext::current();
110125

111-
return context.getMetrics().wrapActorSubrequestClient(context.getSubrequest(
112-
[&](TraceContext& tracing, IoChannelFactory& ioChannelFactory) {
126+
auto startRequest = [&](TraceContext& tracing, IoChannelFactory& ioChannelFactory) {
113127
tracing.setTag("objectId"_kjc, id->toString());
114128

115129
return getOrCreateActorChannel(context, tracing.getInternalSpanParent())
116130
.startRequest({.cfBlobJson = kj::mv(cfStr),
117131
.parentSpan = tracing.getInternalSpanParent(),
118132
.userSpanParent = tracing.getUserSpanParent(),
119133
.actorRetryRequestMetadata = kj::mv(actorRetryRequestMetadata)});
120-
},
121-
{.inHouse = true,
122-
.wrapMetrics = true,
123-
.operationName = kj::ConstString("durable_object_subrequest"_kjc)}));
134+
};
135+
return startActorSubrequest(context, isRetryAttempt, startRequest);
124136
}
125137

126138
kj::Own<IoChannelFactory::SubrequestChannel> GlobalActorOutgoingFactory::getSubrequestChannel() {
@@ -130,16 +142,17 @@ kj::Own<IoChannelFactory::SubrequestChannel> GlobalActorOutgoingFactory::getSubr
130142

131143
kj::Own<WorkerInterface> ReplicaActorOutgoingFactory::newSingleUseClient(
132144
kj::Maybe<kj::String> cfStr) {
133-
return newSingleUseClientWithActorRetryMetadata(kj::mv(cfStr), kj::none);
145+
return newSingleUseClientWithActorRetryMetadata(
146+
kj::mv(cfStr), kj::none, IsActorFetchRetryAttempt::NO);
134147
}
135148

136149
kj::Own<WorkerInterface> ReplicaActorOutgoingFactory::newSingleUseClientWithActorRetryMetadata(
137150
kj::Maybe<kj::String> cfStr,
138-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) {
151+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
152+
IsActorFetchRetryAttempt isRetryAttempt) {
139153
auto& context = IoContext::current();
140154

141-
return context.getMetrics().wrapActorSubrequestClient(context.getSubrequest(
142-
[&](TraceContext& tracing, IoChannelFactory& ioChannelFactory) {
155+
auto startRequest = [&](TraceContext& tracing, IoChannelFactory& ioChannelFactory) {
143156
tracing.setTag("objectId"_kjc, actorId.asPtr());
144157

145158
// Unlike in `GlobalActorOutgoingFactory`, we do not create this lazily, since our channel was
@@ -148,10 +161,8 @@ kj::Own<WorkerInterface> ReplicaActorOutgoingFactory::newSingleUseClientWithActo
148161
.parentSpan = tracing.getInternalSpanParent(),
149162
.userSpanParent = tracing.getUserSpanParent(),
150163
.actorRetryRequestMetadata = kj::mv(actorRetryRequestMetadata)});
151-
},
152-
{.inHouse = true,
153-
.wrapMetrics = true,
154-
.operationName = kj::ConstString("durable_object_subrequest"_kjc)}));
164+
};
165+
return startActorSubrequest(context, isRetryAttempt, startRequest);
155166
}
156167

157168
kj::Own<IoChannelFactory::SubrequestChannel> ReplicaActorOutgoingFactory::getSubrequestChannel() {

src/workerd/api/actor.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,8 @@ class GlobalActorOutgoingFactory final: public Fetcher::OutgoingFactory {
352352
}
353353
void onActorFetchRetry() override;
354354
kj::Own<WorkerInterface> newSingleUseClientWithActorRetryMetadata(kj::Maybe<kj::String> cfStr,
355-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) override;
355+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
356+
IsActorFetchRetryAttempt isRetryAttempt) override;
356357
kj::Own<IoChannelFactory::SubrequestChannel> getSubrequestChannel() override;
357358

358359
private:
@@ -417,7 +418,8 @@ class ReplicaActorOutgoingFactory final: public Fetcher::OutgoingFactory {
417418
}
418419
void onActorFetchRetry() override {}
419420
kj::Own<WorkerInterface> newSingleUseClientWithActorRetryMetadata(kj::Maybe<kj::String> cfStr,
420-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) override;
421+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
422+
IsActorFetchRetryAttempt isRetryAttempt) override;
421423
kj::Own<IoChannelFactory::SubrequestChannel> getSubrequestChannel() override;
422424

423425
private:

src/workerd/api/http.c++

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1484,6 +1484,10 @@ class ActorFetchRetryState {
14841484
return deadline != kj::none;
14851485
}
14861486

1487+
IsActorFetchRetryAttempt isRetryAttempt() const {
1488+
return IsActorFetchRetryAttempt(attemptCount > 1);
1489+
}
1490+
14871491
kj::Maybe<kj::Exception> checkDeadline();
14881492
kj::OneOf<kj::Duration, kj::Exception> prepareRetry(kj::Exception exception);
14891493

@@ -1762,14 +1766,16 @@ jsg::Promise<jsg::Ref<Response>> fetchImplNoOutputLockAttempt(jsg::Lock& js,
17621766
ioContext.getMetrics().setNextSubrequestBodyRewindable(SubrequestBodyRewindable(bodyRewindable));
17631767

17641768
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata;
1769+
auto isRetryAttempt = IsActorFetchRetryAttempt::NO;
17651770
KJ_IF_SOME(state, retryState) {
17661771
actorRetryRequestMetadata = state.getMetadata();
1772+
isRetryAttempt = state.isRetryAttempt();
17671773
}
17681774

17691775
// Get client and trace context (if needed) in one clean call
17701776
auto clientWithTracing = fetcher->getClientWithTracing(ioContext,
17711777
jsRequest->serializeCfBlobJson(js), "fetch"_kjc,
1772-
kj::mv(actorRetryRequestMetadata));
1778+
kj::mv(actorRetryRequestMetadata), isRetryAttempt);
17731779
auto traceContext = kj::mv(clientWithTracing.traceContext);
17741780

17751781
// TODO(cleanup): Don't convert to HttpClient. Use the HttpService interface instead. This
@@ -2755,23 +2761,24 @@ jsg::Promise<Fetcher::ScheduledResult> Fetcher::scheduled(
27552761
kj::Own<WorkerInterface> Fetcher::getClient(
27562762
IoContext& ioContext, kj::Maybe<kj::String> cfStr, kj::ConstString operationName) {
27572763
auto clientWithTracing = getClientWithTracing(
2758-
ioContext, kj::mv(cfStr), kj::mv(operationName), kj::none);
2764+
ioContext, kj::mv(cfStr), kj::mv(operationName), kj::none, IsActorFetchRetryAttempt::NO);
27592765
return clientWithTracing.client.attach(kj::mv(clientWithTracing.traceContext));
27602766
}
27612767

27622768
Fetcher::ClientWithTracing Fetcher::getClientWithTracing(
27632769
IoContext& ioContext,
27642770
kj::Maybe<kj::String> cfStr,
27652771
kj::ConstString operationName,
2766-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) {
2772+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
2773+
IsActorFetchRetryAttempt isRetryAttempt) {
27672774
KJ_IF_SOME(metadata, actorRetryRequestMetadata) {
27682775
auto& outgoingFactory = KJ_REQUIRE_NONNULL(
27692776
channelOrClientFactory.tryGet<IoOwn<OutgoingFactory>>(),
27702777
"actor retry metadata supplied to an unsupported Fetcher");
27712778
KJ_REQUIRE(outgoingFactory->supportsActorFetchRetries(),
27722779
"actor retry metadata supplied to an unsupported Fetcher");
27732780
auto client = outgoingFactory->newSingleUseClientWithActorRetryMetadata(
2774-
kj::mv(cfStr), kj::mv(metadata));
2781+
kj::mv(cfStr), kj::mv(metadata), isRetryAttempt);
27752782
return ClientWithTracing{kj::mv(client), kj::none};
27762783
}
27772784

src/workerd/api/http.h

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@ namespace workerd::api {
2727

2828
WD_STRONG_BOOL(IsHyperdrive);
2929

30+
// Identifies a physical replay for local subrequest accounting. This cannot be derived from
31+
// `ActorRetryRequestMetadata::isRetry`: when the previous attempt was known not to have been
32+
// delivered, the replay uses a fresh logical token whose `isRetry` remains `NO`.
33+
WD_STRONG_BOOL(IsActorFetchRetryAttempt);
34+
3035
// Whether a Fetcher is exempt from the `fetcher_rpc` compatibility gate that Fetcher::getRpcMethod
3136
// applies, so its JSRPC wildcard resolves regardless of the worker's compatibility date.
3237

@@ -248,7 +253,8 @@ class Fetcher: public JsRpcClientProvider {
248253
// metadata rather than silently starting a new logical call.
249254
virtual kj::Own<WorkerInterface> newSingleUseClientWithActorRetryMetadata(
250255
kj::Maybe<kj::String> cfStr,
251-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata) {
256+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
257+
IsActorFetchRetryAttempt isRetryAttempt) {
252258
KJ_FAIL_REQUIRE("actor retry metadata supplied to an unsupported Fetcher");
253259
}
254260

@@ -310,7 +316,8 @@ class Fetcher: public JsRpcClientProvider {
310316
ClientWithTracing getClientWithTracing(IoContext& ioContext,
311317
kj::Maybe<kj::String> cfStr,
312318
kj::ConstString operationName,
313-
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata);
319+
kj::Maybe<IoChannelFactory::ActorRetryRequestMetadata> actorRetryRequestMetadata,
320+
IsActorFetchRetryAttempt isRetryAttempt);
314321

315322
bool supportsActorFetchRetries();
316323
void onActorFetchRetry();

0 commit comments

Comments
 (0)