Repository navigation
Conversation
size-limit report 📦Compared: 1274d44 (PR test merge using develop at dcc7e5e) Against: 3963e60 (develop) · base run #65264
|
Optional E2E testsSome optional checks were not successful. They do not block the merge. 7 not successful, 50 successful. See the run.
Successful checks
|
29b761b to
0fa5a24
Compare
79c5b03 to
ee6cc70
Compare
ee6cc70 to
dcc387e
Compare
dcc387e to
735789f
Compare
dc67ef5 to
02913e0
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
d0f5ad8 to
0ebb300
Compare
0ebb300 to
36182d9
Compare
36182d9 to
72c67bf
Compare
0b90045 to
4272ef6
Compare
4272ef6 to
a6ea10f
Compare
| plugins: | ||
| command !== 'dev' | ||
| ? [sentryCloudflareNodeWarningPlugin(), sentryCloudflareVitePlugin()] | ||
| : [sentryCloudflareNodeWarningPlugin()], |
There was a problem hiding this comment.
Pages wrap drops tunnel client
High Severity
Production Pages builds now wrap the worker with withSentry(() => undefined), which inits a Cloudflare client that does not include serveTunnelIntegration. handleServedTunnelRequest and Astro middleware both read that client via getClient(), so managed tunnel requests are no longer answered. Pages previously relied on middleware and the server-init client, which this wrap replaces.
Reviewed by Cursor Bugbot for commit a6ea10f. Configure here.
There was a problem hiding this comment.
Ah, yeah, this is valid, missed in my review. The Pages withSentry wrap replaces the user's client, so the tunnel stops answering (and events likely stop too).
The failure on Pages:
- The wrap is
withSentry(() => undefined, ...), sogetFinalOptionstakes its options from Wranglerenvonly. On every request,wrapRequestHandlerWithInitcallsinitBaseSdkwith them. That builds aCloudflareClientwith noserveTunnelIntegration, and usually no DSN. initBaseSdkcaches the first client of the isolate (packages/cloudflare/src/baseSdk.tsline 90). On each later request it callssetCurrentClient(cached), and the wrapper callsisolationScope.setClient(client).- On Pages,
@sentry/astro's server entry is@sentry/node. The user'ssentry.server.configrunsSentry.initonce, when the middleware module loads, and that is whereserveTunnelIntegration()lives. From then on,getClient()returns the Cloudflare client. This is true both inhandleServedTunnelRequestinside the wrapper and inpackages/astro/src/server/middleware.tsline 93. Neither finds a served tunnel, so the request goes to the app.
The first request of an isolate may still work, because the middleware's init can bind the Node client after the wrapper let the request through. A quick manual check could miss the bug.
The effect is probably wider than the tunnel. On those later requests, events captured in the middleware and the pages also go to the env-only client. With no SENTRY_DSN binding, that client sends nothing, so a Pages app could stop reporting after the first request of each isolate. I did not confirm this at runtime.
Workers uses the same wrap, but under a different contract. dev-packages/e2e-tests/test-applications/astro-5-cf-workers sets SENTRY_DSN in wrangler.jsonc, and its sentry.server.config.js is empty on purpose ("The Worker wrapper initializes Sentry from Wrangler bindings"). Pages apps configure the SDK in sentry.server.config. Moving them onto the Workers wrap without that contract breaks them.
Suggestion: revert the Pages wrap in this PR, and leave the Pages tunnel to the Astro middleware. If Pages has to get the wrap, do it in its own PR. That PR has to pass the user's options to the wrap (or reuse the client from sentry.server.config, not an env-only one), or it has to move Pages to the Workers contract with a migration note. Add an Astro Pages e2e app (or a variant of astro-5). It should check the tunnel answer and an error event with the right request data on the second and later requests of an isolate, not only the first, and one server span per request.
There was a problem hiding this comment.
So all forms of middlewares are getting cleaned up in the next PR, I don't want us to have both runtime-level interception and then app-level interception, so relying on the middleware isn't ideal.
I tried to verify this but looks like we have a blocker, the current CF pages sentry.server.config loads the client bundle not the server's. So the tunnel integration and logic doesn't even exist there. Will PR a fix and revisit this issue to see if we can verify it.
|
One or more gzipped bundles increased by more than 500 bytes. If this increase is intentional, add the Accept Bundlesize Increase label to this PR to rerun and accept the check. |
The Cloudflare request wrapper answers tunnel requests before the Worker's handler. Nuxt's Cloudflare plugin passes the request with its body, and Astro wraps Pages builds with `withSentry` like Workers builds.
a6ea10f to
e611657
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This change is small and fits the stack. 👍
I'd probably revert the Nuxt v2 body copy unless there is a demonstrated gap in the h3-layer tunnel.
…unnel requests Every POST body went into the request the wrapper reads, so the SDK attached app request bodies to events.
…apper Nitro v3 sends internal `$fetch` calls through `fetch` too, so each one during server rendering ran the Cloudflare wrapper again, with another `init`, isolation scope and `http.server` span. Requests without the Worker's context now go straight to the app. Adds a `nuxt-5-cloudflare` e2e app.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 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 9c444ce. Configure here.
| headers: event.headers, | ||
| // Nitro read the body before calling `localFetch`. Only the managed tunnel answers from this request, so | ||
| // only its requests get the body, and other requests' bodies don't end up on events. | ||
| body: isServedTunnelRequest({ method: event.method, url: pathname }) ? event.body : undefined, |
There was a problem hiding this comment.
Tunnel body skipped before SDK init
High Severity
isServedTunnelRequest runs before wrapRequestHandler creates the Cloudflare client, and it is called without a client, so it falls back to getClient(). On this Cloudflare path the SDK is not initialized yet, so the check is always false and the tunnel POST body is omitted. handleServedTunnelRequest then treats a request with no body as not a tunnel request and never answers it. Previously every non-GET body was copied, so the wrapper could still serve the tunnel after init.
Reviewed by Cursor Bugbot for commit 9c444ce. Configure here.
Adds the tunnel handling logic to our CF workers wrapper, and re-wires astro and other CF capable frameworks.
Note for reviewers: if you are lacking context on this PR, check #25997