Conversation
|
bugbot run |
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 78c4faa. Configure here.
JPeer264
left a comment
There was a problem hiding this comment.
Nice change. Got some questions throughout. The Sentry bot comment might be worth to check out
|
|
||
| describe('BrowserProfilingIntegration', () => { | ||
| beforeEach(() => { | ||
| getCurrentScope().setClient(undefined); |
There was a problem hiding this comment.
q: Just for my understanding. We have to do this now, as otherwise we would just reuse the client from before, right?
There was a problem hiding this comment.
Not quite. The beforeEach only stops the new warning, which fires because the previous test's client is still bound. So the test runs the same either way, and the reset just keeps the output clean.
The reset will matter more at the next major, though. Once "first wins" lands, a test that skips the reset will get the previous test's client back, with the previous test's options. So this prepares these tests for that change.
| // A closed client drops all data, so the next `init()` must not reuse it. | ||
| client.on('close', () => { | ||
| if (getCachedClient() === client) { | ||
| _clearGlobalClientCache(); |
There was a problem hiding this comment.
q: When would a Sentry.close() be called? At least in Cloudflare that would be only when a user calls it manually, right? Maybe I'm missing something
There was a problem hiding this comment.
Right, the SDK never closes a cached client itself (flushAndDispose only disposes per-invocation clients), so this only fires on a manual Sentry.close() / client.close().
The case I had in mind is someone calling await Sentry.close() at the end of a handler, thinking it means "flush". Without this, the first request in the isolate reports, and every later request gets the closed, disabled client back from the cache and drops everything silently until the isolate is recycled. With it, close() works as the new rule says: the next init() sets up a fresh client.
| Runtime packages (`node`, `cloudflare`, ...) re-export from | ||
| `@sentry/server-utils` rather than defining their own. | ||
|
|
||
| - `Sentry.init()` follows one rule for repeated calls: the first call wins. See |
There was a problem hiding this comment.
q: You think it is worth it to also add a note in write-tests/SKILLS.md to remove the client in a beforeEach or afterEach when writing unit tests?
There was a problem hiding this comment.
Ah, yes, good idea!
Looking at it, the "Test isolation" example there is already stale (Scope#clear() and clearGlobalScope() don't exist anymore), so I'll replace it with the reset patterns we actually use (ie, getCurrentScope().setClient(undefined) before/after init()), plus a note on asserting the warning via originalConsoleMethods.warn and a link to docs/repeated-init.md.
| const client = initAndBind(CloudflareClient, clientOptions) as CloudflareClient; | ||
|
|
||
| if (cacheEnabled && client) { | ||
| cacheClient(client); |
There was a problem hiding this comment.
Just thinking loud, no action item now. I wonder if the cacheClient strategy within Cloudflare could be simplified, given that we reuse clients now per definition. We might run into the warnings though 🤔
… `close()` A repeated `Sentry.init()` call was mostly undefined behavior, and each SDK handled it in its own way. Most SDKs built a new client and replaced the old one without a warning. Nothing closed the old client, so its buffers, timers, and hooks stayed alive, and `setupOnce` kept the settings of the first call. The goal is one rule for all SDKs: the first `init()` wins, a later call returns the active client, and `close()` lets you start over. Most SDKs cannot switch to "first wins" before a major version, so this change adds the parts that are safe now: - `initAndBind`, Node's `_init`, and Vercel Edge's `init` print a warning (with or without `debug`) when a client is already bound. They still replace the client for now. Wrappers that expect a repeated call (Next.js server, Remix server, Nuxt server, Hono) keep their own guard and return early, so they do not warn. Cloudflare's `cacheClient: false` asks for a new client on each call, so it unbinds the old one first and does not warn. The Next.js client drops its own warning, which used a flag that never reset and so also fired after `close()`. Its config-file hint moves to the docs. - `Sentry.close()` unbinds the client after it closes it. Before, a guard based on `getClient()` treated the closed client as active, so `close(); init()` returned the closed client. Nuxt's server guard now also checks for a bound client, for the same reason. Cloudflare also caches its client for the isolate, and the cache handed the closed client back to every later `init()`, so the isolate sent nothing until it was recycled. Closing the cached client now clears the cache. - Next.js server and Remix server return the active client from a repeated call, not `undefined`. A caller could not tell "already initialized" from "failed". `docs/repeated-init.md` records the rule, the current behavior of each SDK, and the plan for the next major. The warning text should point apps that share a page to the isolated client helper from PR #24883 once that helper has a final name. The Next.js and Nuxt server `init()` added their event processors to the global scope. Since `close()` now unbinds the client, a later `init()` would run the full setup again, and each cycle would add another copy of each processor to the global scope. So instead, add them to the client, so that they go away with a closed client, and a later `init()` uses its own options. Client processors run before all scope processors, so these now also run before any global scope processor that user code added before `init()`. They only drop events or fix stack frames, so the earlier position does not change which events are sent. Fixes #24960 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
9208e17 to
eb60271
Compare

A repeated
Sentry.init()call was mostly undefined behavior, and each SDK handled it in its own way. Most SDKs built a new client and replaced the old one without a warning. Nothing closed the old client, so its buffers, timers, and hooks stayed alive, andsetupOncekept the settings of the first call.The goal is one rule for all SDKs: the first
init()wins, a later call returns the active client, andclose()lets you start over. Most SDKs cannot switch to "first wins" before a major version, so this change adds the parts that are safe now:initAndBind, Node's_init, and Vercel Edge'sinitprint a warning (with or withoutdebug) when a client is already bound. They still replace the client for now. Wrappers that expect a repeated call (Next.js server, Remix server, Nuxt server, Hono) keep their own guard and return early, so they do not warn. Cloudflare'scacheClient: falseasks for a new client on each call, so it unbinds the old one first and does not warn. The Next.js client drops its own warning, which used a flag that never reset and so also fired afterclose(). Its config-file hint moves to the docs.Sentry.close()unbinds the client after it closes it. Before, a guard based ongetClient()treated the closed client as active, soclose(); init()returned the closed client. Nuxt's server guard now also checks for a bound client, for the same reason. Cloudflare also caches its client for the isolate, and the cache handed the closed client back to every laterinit(), so the isolate sent nothing until it was recycled. Closing the cached client now clears the cache.undefined. A caller could not tell "already initialized" from "failed".The Next.js and Nuxt server
init()added their event processors to the global scope. Sinceclose()now unbinds the client, a laterinit()would run the full setup again, and each cycle would add another copy of each processor to the global scope. So instead, add them to the client, so that they go away with a closed client, and a laterinit()uses its own options. Client processors run before all scope processors, so these now also run before any global scope processor that user code added beforeinit(). They only drop events or fix stack frames, so the earlier position does not change which events are sent.docs/repeated-init.mdrecords the rule, the current behavior of each SDK, and the plan for the next major. The warning text should point apps that share a page to the isolated client helper from #24883 once that helper has a final name.Fixes #24960