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 14 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}
d081c44 to
a362f65
Compare
|
bugbot run |
8869481 to
0ba7c00
Compare
Adds tests to check if `enableDedupes` is really disabled for workflows original trigger: #23151 (comment) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
0ba7c00 to
7a1cdd3
Compare
|
bugbot run |
|
bugbot approve |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 71b3f5f. Configure here.
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
This seems like a lot of extra, hard-to-maintain code that's working around the way client flushing works in our SDKs. Are you sure the perf gains are worth it? I'm a bit concerned, but your call.
| * scope, which is shared by every invocation in the isolate. | ||
| */ | ||
| export function setInvocationState(scope: Scope, state: InvocationState): void { | ||
| (scope as ScopeWithInvocationState)[INVOCATION_STATE] = state; |
There was a problem hiding this comment.
m: Let's add a check here to only set this if scope !== getDefaultIsolationScope(), wdyt?
| options: CloudflareOptions, | ||
| getDefaultIntegrationsImpl: (options: CloudflareOptions) => Integration[], | ||
| ): CloudflareClient | undefined { | ||
| const cacheEnabled = options.cacheClient !== false && Boolean(options.dsn); |
There was a problem hiding this comment.
Uff that was a left over from before where I cached multiple clients per isolate. This should be removed
|
|
||
| // If no more pending spans, resolve the completion promise | ||
| if (this._pendingSpans.size === 0 && this._resolveSpanCompletion) { | ||
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, resolving promise'); |
There was a problem hiding this comment.
l: This doesn't seem to add much value to the user, what promise is resolving? I think we prob don't need to log here, wdyt?
There was a problem hiding this comment.
Before, per client we needed a way to know WHEN to flush, as there is no timer. The only way to know when to flush is when potentially no spans are open anymore (this was super hacky, but worked somehow - but ofc not for all usecases - this is why we don't rely on it anymore).
There was a problem hiding this comment.
Sure, but I mean the debug log itself. What would I do with this as a user? 🤔
There was a problem hiding this comment.
Ah true - I can refine that a little. I change it to
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, resolving promise'); | |
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, preparing to flush'); |
ed8d910 to
51d50d8
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
👋 @mydea, @nicohrubec — Please review this PR when you get a chance! |
|
👋 @logaretm, @andreiborza, @getsentry/team-javascript-sdks, @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
|
👋 @mydea, @nicohrubec — Please review this PR when you get a chance! |
|
👋 @logaretm, @andreiborza, @getsentry/team-javascript-sdks, @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This is a pretty big set of changes. I haven't checked the stated perf gains (some kind of benchmark would maybe be good for that?) but the correctness wins in fixing the reported issues would make it worthwhile even if it's a slight perf regression, imo, so as long as it's not making things worse, that's fine.
Re @andreiborza's comment, it is definitely a lot of extra code. The cacheClient: false escape hatch guarantees both delivery pathways live in the tree indefinitely: the flush lock, the span-tracking promise machinery, dispose(), and their tests all stay, and now a second set maintained alongside them. Nothing exercises the false path, so that can potentially rot.
We can delete a lot of the new machinery, though. Some parts are redundant or collapse if the eager drains are gated on flushPointReached the same way span delivery already is. That is imo the highest-value simplification.
Maybe we could either drop the cacheClient: false option now, or file the v12 removal issue and link it from the option's JSDoc so the dual pathway has an expiration date.
We could also push the concept into core, not Cloudflare. The issue is that Client has no notion of an invocation lifetime when it outlives a single request. Vercel Edge and Deno Deploy need the same thing, so a core "delivery scope" could replace InvocationState, the symbol-on-scope trick, and the narrow flushTraceSpans hook. That might be too big for now, but could be a good follow-up PR to simplify things further.
|
|
||
| const client = initAndBind(CloudflareClient, clientOptions) as CloudflareClient; | ||
|
|
||
| if (cacheEnabled && client && options.dsn) { |
There was a problem hiding this comment.
If we don't have a DSN, then it looks like this will leak a client and a global console.log instrumentation.
Because we set cacheClient unconditionally on line 120, if options.dsn is falsey here (eg, preview deploys or just a missing env), then:
- builds a fresh client (nothing is cached),
- gets
isCachedClient === true, sopackages/cloudflare/src/flush.tsline 146 skipsdispose(), - and
dispose()is what runs the cleanup callbacks registered byconsoleIntegration(packages/core/src/integrations/console.tsline 56), which pushes into the module-local handler list inpackages/core/src/instrument/console.tsline 28.
I think we can fix it by either removing the options.dsn check here, or making line 120 be cacheClient: cacheEnabled && !!options.dsn, so that it's setting based on whether the client will actually be cached here.
There was a problem hiding this comment.
Seems like this is also an artifact from my previous cache to have multiple clients per isolate. Now this is trimmed to 1. I'll remove that options.dsn check entirely. Done in: 486b480aee53e538a4c40bc840c1f39008dd8d1b
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>
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
34f7242 to
8b39b70
Compare
| this._inBoundaryFlush = true; | ||
| try { | ||
| return await super.flush(timeout); | ||
| } finally { | ||
| this._inBoundaryFlush = false; |
There was a problem hiding this comment.
Bug: Concurrent calls to flush() can cause a race condition with the shared _inBoundaryFlush flag, leading to incorrect eager envelope delivery.
Severity: MEDIUM
Suggested Fix
Replace the boolean _inBoundaryFlush flag with a more robust concurrency control mechanism, such as a counter that is incremented upon entering the flush operation and decremented in the finally block. The check should then be based on whether the counter is greater than zero.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cloudflare/src/client.ts#L146-L150
Potential issue: A race condition exists in the `flush()` method due to the shared
`_inBoundaryFlush` flag. In a concurrent environment, two requests can call `flush()` on
the same client instance. One request can complete its `await super.flush(timeout)` and
execute its `finally` block, setting `_inBoundaryFlush` to `false`. This can happen
while a second request is still awaiting its own `super.flush()`. If an `afterEnvelope`
event fires for the second request during this time, it will incorrectly see
`_inBoundaryFlush` as `false` and trigger eager envelope delivery when it should be
suppressed, as a flush is still in progress for that request's context.
And that was true, a lot of extra machinery is gone now thanks to you 👍
I'll create a ticket once this lands. Goal is for now to just have an escape hatch in case something goes totally south.
Is that true for Vercel Edge and Deno Deploy? As everything which happens here is mostly because timers are usually 0 - otherwise we could use the client almost as is (and ofc the |
| // life. Mirrors the same reset in the Node client. | ||
| _INTERNAL_clearAiProviderSkips(); | ||
| super._setupIntegrations(); |
There was a problem hiding this comment.
Bug: With client caching, AI provider skips from one request incorrectly persist and affect subsequent requests, as the _INTERNAL_clearAiProviderSkips() cleanup function is no longer called per-request.
Severity: HIGH
Suggested Fix
Ensure that the _INTERNAL_clearAiProviderSkips() function is called at the beginning of each request, even when a cached client is being used. This might involve invoking it from a different part of the request lifecycle that runs for every request, regardless of client caching.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cloudflare/src/client.ts#L186-L188
Potential issue: With client caching enabled by default (`cacheClient: true`), the
`init()` and `_setupIntegrations()` methods are not called when a cached client is
reused. This prevents `_INTERNAL_clearAiProviderSkips()` from running on subsequent
requests. As a result, if a higher-level integration marks an AI provider to be skipped
in one request, that provider will remain skipped for all future requests in the same
isolate that use the cached client. This leads to the incorrect suppression of
`env.AI.run` spans, causing a loss of monitoring data.
Oh, I might've overstated this then, because I'm not sure about their timer behavior. I think that might be a cf-only behavior, actually. Probably not worth doing now, in any event, but might be good to keep in mind as a possible DRY fix if we do need to duplicate the logic in those runtimes. |
3 participants
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.