Repository navigation
fix(astro): Initialize the server SDK on the first endpoint request - #25731
AyeshaWaseemAhmad wants to merge 2 commits into
Conversation
The server init was only imported through injectScript('page-ssr'),
which Astro adds to page modules but not to API endpoints. On a server
that had not rendered a page yet, getClient() was undefined, so the
request middleware skipped the request and errors thrown by endpoints
were dropped.
The middleware is now registered through a virtual module that imports
the server init before re-exporting onRequest from
@sentry/astro/middleware. With a server init file, the init is that
file, which the page-ssr injection imports as well. With the default
generated snippet, the snippet lives in a virtual module that both the
page-ssr injection and the middleware import. Either way the module is
shared, so Sentry.init() still runs once and still runs ahead of a
page's own imports.
Cloudflare Workers is unchanged, because the Worker wrapper already
initializes the SDK for every request. Netlify is unchanged too: its
optional edge middleware is bundled separately and cannot be detected
here, and running the init there fails on Deno.
Fixes getsentry#25729
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } finally { | ||
| wranglerPagesConfig = undefined; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Conditional assertion in parameterized test
Low Severity
The Cloudflare Pages it.each still picks the expected page-ssr script with a ternary on serverInitPath. Nearby Netlify cases already put that expected script in the table. This was flagged because the review rules ask to split conditional paths instead of branching inside one test.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit aa825c3. Configure here.
|
This follows a similar approach to #25419, I think we can merge this then rebase the other one. Did you verify this last bit you mentioned, which error is being thrown here?
|
With `edgeMiddleware` enabled, Vercel and Netlify bundle the middleware into a separate edge function. Since the middleware now imports the server init, the Node SDK would be initialized in the edge runtime, where it can't run. Return early from `init()` there so the middleware stays a no-op, as before. This also drops the Netlify adapter exclusion, so default (non-edge) Netlify setups initialize the SDK on the first endpoint request too. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
There was a problem hiding this comment.
Thanks for opening this PR @AyeshaWaseemAhmad! This looked mostly good to me but i wasn't too happy about handling Netlify Edge and Vercel Edge differently. Since AFAIK, there's no way to detect if edge or node runtimes are used on both platforms, we can early return on server side init. I made that change on the branch.
We would need to look at edge runtime support differently long-term, similarly to how we support Vercel Edge in NextJS. But this is not on our priority list at the moment.
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.
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 d9801aa. Configure here.
| // server init) into a separate edge function, where the Node SDK can't run. | ||
| if (isEdgeRuntime()) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Edge guard runs after Node imports
Medium Severity
isEdgeRuntime() only skips the init() call, but this module already imports @sentry/node at load time. With Netlify now using the init middleware, edgeMiddleware bundles that graph into the Deno/Edge function, so Node built-ins can throw before the guard and fail every request. That also lets SDK init take down the host app on an unsupported runtime.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit d9801aa. Configure here.
The server init was only imported through
injectScript('page-ssr', ...), which Astro adds to page modules but not to API endpoints. On a server that has not rendered a page yet,getClient()is undefined, so the request middleware skips the request and errors thrown by endpoints are dropped.The middleware is now registered through a virtual module that imports the server init before re-exporting
onRequestfrom@sentry/astro/middleware. With a server init file, the init is that file, whichpage-ssrimports as well. With the default generated snippet, the snippet lives in a virtual module that both thepage-ssrinjection and the middleware import. Either way the module is shared, soSentry.init()still runs once, ahead of a page's own imports.On a fresh Node-adapter build the first request to an endpoint now runs the init and the error is captured, on Astro 4, 5, 6 and 7 (also in
astro devon 7, on@astrojs/vercelserverless and on Cloudflare Pages). Before, nothing was captured until a page had rendered.Cloudflare Workers is unchanged, because the Worker wrapper already initializes the SDK for every request. Netlify is unchanged too: its optional edge middleware is bundled into a separate Deno function and can't be detected here, and running the init there throws. Not covered: Netlify,
autoInstrumentation.requestHandler: false, and errors thrown before the first request. An init file that throws at import time now fails endpoint requests as well.Fixes #25729