feat(cloudflare): Add cacheClient to reuse the client across invocations - #23151
feat(cloudflare): Add cacheClient to reuse the client across invocations#23151JPeer264 wants to merge 3 commits into
Conversation
926afe8 to
04d255e
Compare
|
bugbot run |
size-limit report 📦
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Second and third invocations capture the same error, but dedupe drops them. | ||
| await runner.makeRequest('get', '/cache/dedupe?id=dedupe-shared'); | ||
| await runner.makeRequest('get', '/cache/dedupe?id=dedupe-shared'); | ||
| }); |
There was a problem hiding this comment.
Dedupe test asserts nothing about dropped duplicates
Medium Severity
The second and third requests are fired without any assertion, and the runner silently ignores envelopes that match no registered expectation, so the test passes identically whether dedupe drops the duplicates or reports all three. The claimed cross-invocation dedupe behaviour of the shared client is not actually covered.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 04d255e. Configure here.
d081c44 to
a362f65
Compare
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
There are 5 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a362f65. Configure here.
| cached.setExecutionContext(options.ctx); | ||
| return cached; | ||
| } | ||
| } |
There was a problem hiding this comment.
Cached client ignores workflow dedupe opt-out
Medium Severity
init() returns the isolate's first client regardless of the options passed later, but instrumentWorkflowWithSentry relies on init({ ..., enableDedupe: false }) to keep every workflow step failure. When another entrypoint in the same isolate (an ExportedHandler or WorkerEntrypoint, which a WorkflowEntrypoint shares an isolate with) created the client first, the workflow silently runs with dedupe enabled and repeated step errors are dropped — and the reverse ordering disables dedupe for the request handler.
Reviewed by Cursor Bugbot for commit a362f65. Configure here.
There was a problem hiding this comment.
That is indeed true. I just wonder why we didn't have tests for this scenario 🤔
| } catch { | ||
| // The owning invocation already ended; the send races isolate teardown either way. | ||
| } | ||
| } |
There was a problem hiding this comment.
Workflows lack invocation state for eager sends
Medium Severity
Every other entrypoint attaches invocation state to its forked isolation scope, but the workflow wrapper still uses a plain withIsolationScope and never calls setInvocationState. Eager envelope sends from a workflow therefore fall back to the client-global _invocationContext, which the most recent init() overwrites. With concurrent workflow runs (or a request handler running alongside), a run's envelopes get registered on another invocation's waitUntil, so they can be suspended when that unrelated invocation ends.
Reviewed by Cursor Bugbot for commit a362f65. Configure here.
| await runner.makeRequest('get', '/cache/handler-error?id=instance-1', { expectError: true }); | ||
| await runner.makeRequest('get', '/cache/handler-error?id=instance-2', { expectError: true }); | ||
| await runner.completed(); | ||
| }); |
There was a problem hiding this comment.
Duplicate integration test spawns another worker
Low Severity
The "multiple DO instances share the same client" test is identical to the earlier "cacheClient: true - DO handler error is captured" test: same expectations, same two requests, same instance ids. It adds no coverage while starting another wrangler dev process, which is the expensive part of this suite.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a362f65. Configure here.
Building and disposing a client per invocation costs real time on every request, and in a Durable Object it also loses data: there is no `waitUntil` boundary that dependably extends execution, so anything captured after the handler returned went to a client that had already been disposed. Enabled by default, this caches one client per isolate. The first initialization wins for the isolate's lifetime: a later init with different options reuses that client, and a new deployment always starts fresh isolates, so clients are always built from the current version's options. A cached client is flushed but not disposed at an invocation boundary, and it is re-bound to the current scope on every invocation — otherwise `initialScope` would apply only to an isolate's first invocation, and a client disposed by a competing init would keep being handed out. A cached client whose transport is gone is evicted rather than returned. Because a reused client never reaches an end-of-invocation flush, delivery is eager: the new `afterEnvelope` hook on the core client drains the transport buffer as soon as an envelope has been accepted, and logs and metrics drain on a debounced hook so they are batched rather than sent one at a time. Spans that end after the invocation's flush point are delivered through core's `flushTraceSpans` hook, which flushes only that trace's bucket from the span streaming buffer. The per-invocation flush lock and span tracking are skipped, since binding a client that outlives the invocation to one invocation's lock would make later flushes wait on that invocation's work forever. A shared client also shares integration state, so dedupe works across invocations: the same error raised by two separate requests is reported only once. Uncached behavior is unchanged; pass `cacheClient: false` to restore it. Co-authored-by: Cursor <cursoragent@cursor.com>
a362f65 to
8869481
Compare
8869481 to
0ba7c00
Compare
1 participant
closes #23083
closes #22545
closes #21950
What
This PR is reusing the client, instead of creating a new one. This is also only possible because of #22969 (as now we have the correct isolation scopes per request).
To still have an escape hatch and keep the old behavior there is the
cacheClient: falseoption, that just creates a new client per request, as before.Why
There are more and more issues coming in, that
.disposeis leading to errors, which makes sense as in DurableObjects data can flow in after a request happened and it stays alive. Since we created a new client on each request, we also had to clean it up - the best point in time was after a request, which was too early for e.g. #22545. Since there is not a perfect time to dispose the client the only option is to reuse the client and not dispose at all (this is then also aligned with how other SDK machinery works).Issues and how they're solved
Timing
In CF, timers are usually 0 and therefore our 5 second auto flush wouldn't work. In order to still retrieve all the data we need to have point in times (hooks) where it is safe to flush. In our case we have a request and flush after a request, like before. Events that come in a later point in time are then captured with the hooks added in #23136 (most important one is the
afterEnvelope).We start listening to the hooks once
flushPointReachedis set totrue- which is AFTER a request, the time where we have no control anymore about flushing manually otherwise.waitUntil
Keeping the correct
waitUntilis important, as each request needs its ownwaitUntilto properly flush. To still keep the correctwaitUntilthis is now bound onto thescopedirectly. I tried usingsetSDKProcessingMetadataon the scope, but it just didn't work properly on deployed workers. Instead this is bound onto the scope directly with a Symbol - that works like a charm. This is theINVOCATION_STATE#namingishardflushLock
The flush lock would wait for all spans to be finished and then flush. This would just not work, as we only have one client. So we skip this entirely and get rid of that hack. We keep this in order to have the escape hatch
cacheClient: falsein case something goes sideways.Bonuses
Bonus 1
Because we are now reusing the client we are saving valuable CPU cycles per request. With
cacheClient: truewe gain up to ~14-21% per request, which is loads. Also on top of the CPU wins we also retrieve more events, which would have been dropped before.Bonus 2
In v12 (or any other major) we could get rid of all the
flushLockhacks and the rest of hacks we didBonus 3
Dedupe integration works now as intended. Because we created a new client and the
dedupeIntegrationonly deduplicated per integration, which was a new one on every client, we only deduped it per request, not for all requests.Sidenotes
During the implementation I thought about having multiple clients, which are cached in one global map - in case the isolations would get reused from other deployments or other bindings. After some excessive tests it seems that new deployments are getting a fresh isolate, different bindings have their own isolate, only
ExportedHandlers andWorkerEntrypoints share one isolate, which makes sense to some degree, as they're isolated within each request anyways (they're getting a fresh isolate on a new deployment though). Because this is the case only one client is being created instead of checking if the config differs between clients.Clanker description
Building and disposing a client per invocation costs real time on every request, and in a Durable Object it also loses data: there is no
waitUntilboundary that dependably extends execution, so anything captured after the handler returned went to a client that had already been disposed.Enabled by default, this caches one client per isolate. The first initialization wins for the isolate's lifetime: a later init with different options reuses that client, and a new deployment always starts fresh isolates, so clients are always built from the
current version's options. A cached client is flushed but not disposed at an invocation boundary, and it is re-bound to the current scope on every invocation — otherwise
initialScopewould apply only to an isolate's first invocation, and a client disposed by a competing init would keep being handed out. A cached client whose transport is gone is evicted rather than returned.Because a reused client never reaches an end-of-invocation flush, delivery is eager: the new
afterEnvelopehook on the core client drains the transport buffer as soon as an envelope has been accepted, and logs and metrics drain on a debounced hook so they are batched rather than sent one at a time. Spans that end after the invocation's flush point are delivered through core'sflushTraceSpanshook, which flushes only that trace's bucket from the span streaming buffer. The per-invocation flush lock and span tracking are skipped, since binding a client that outlives the invocation to one invocation's lock would make later flushes wait on that invocation's work forever.A shared client also shares integration state, so dedupe works across invocations: the same error raised by two separate requests is reported only once.