sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

[@sentry/nextjs] Errored API routes trigger "API resolved without sending a response"

Open adarnon opened this issue 2 years ago • 26 comments

  • [x] Review the documentation: https://docs.sentry.io/
  • [x] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
  • [x] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
  • [x] Provide a link to the affected event from your Sentry account

Package + Version

  • [x] @sentry/nextjs

Version:

6.10.0

Description

Describe your issue in detail, ideally, you have a reproducible demo that you can show.

The withSentry() API route wrapper monkeypatches res.end() function to await for flush before closing a request. While this is certainly useful for serverless environments, it could cause a false positive warning on deployments with a custom server (e.g., Express).

Since normal Next API route code does not await for res.end() to finish, the handler's promise returns before the response has ended properly. This causes Next to detect the API route as having finished without a response, even though the response is sent a few moments later. Here is an example:

imageedit_3_8075703349

A workaround is to wrap all API routes with another callback that always performs await res.end() before returning.

adarnon avatar Jul 30 '21 02:07 adarnon

We're getting these false positives as well. I haven't put in the time to look at testing this but I think that the only way to workaround this issue is in the wrapper, since there are transitive calls to res.end in express (e.g. res.send or res.json). I think that attaching a promise to the context would be sufficient, like this: https://github.com/getsentry/sentry-javascript/compare/master...etrepum:resolve-with-response-gh-3852?expand=1&w=1

etrepum avatar Aug 08 '21 18:08 etrepum

It does this for all my requests as well, even though I do this:

return res.status(200).json({ success: true });

I hope this will be fixed :)

jordymeow avatar Aug 25 '21 07:08 jordymeow

Any plan on this one? I'm currently doing something like this to get rid of the message

import * as Sentry from '@sentry/nextjs';

// ISSUE: https://github.com/getsentry/sentry-javascript/issues/3852
// API resolved without sending a response for *, this may result in stalled requests.
const withSentry = (fn) => async (req, res) => {
  await Sentry.withSentry(fn)(req, res);
  await res.end();
};

export default withSentry;

KieraDOG avatar Aug 31 '21 01:08 KieraDOG

I am having the same issue in development. I have not deployed it to production to verify yet.

This gets logged in the server log:

API resolved without sending a response for /api/graphql, this may result in stalled requests.

And no error is logged in sentry.

If I have the barebone code from the example, it works fine. However, my actual code does a call to server and uses await to wait for the response. If there is error in server, then I don't get any report in sentry.

adikari avatar Sep 02 '21 06:09 adikari

You can set externalResolver to true in api route config since it is handled by sentry (external) https://nextjs.org/docs/api-routes/api-middlewares#custom-config

kaykdm avatar Sep 14 '21 05:09 kaykdm

@kaykdm Thanks for this workaround! Is there a way to set a default config for all API routes ? From the docs it seems that there is no option to do this in next.config.js, which means this config has to be added to all API routes individually.

@KieraDOG the workaround in this comment did not work for me. This resulted in the response being ended before the actual content is sent, which means stalling requests (especially in a development environment).

benjamintd avatar Sep 14 '21 08:09 benjamintd

Why are you guys monkey patching .end() ?

https://github.com/getsentry/sentry-javascript/blob/master/packages/nextjs/src/utils/withSentry.ts#L22

Can you not just return a promise to Next?

maccman avatar Sep 25 '21 12:09 maccman

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

github-actions[bot] avatar Oct 16 '21 12:10 github-actions[bot]

Please don't close - this is still an issue that should be resolved

adarnon avatar Oct 17 '21 03:10 adarnon

Is this fixed?

arclogos132 avatar Nov 06 '21 11:11 arclogos132

I have upgraded to latest version and I am still getting this error

image

adikari avatar Nov 12 '21 06:11 adikari

@adikari This is a different error

adarnon avatar Nov 12 '21 19:11 adarnon

@adarnon my bad. I Mixed up different issues :)

adikari avatar Nov 12 '21 22:11 adikari

Why is this error closed? With the last version I still see the error, there is a warning but the problem is still there.

federico-moretti avatar Mar 01 '22 09:03 federico-moretti

Hi there, putting my comment here.

After #4516 I'm getting a massive amount of logs (I'm using next-auth + swr)

image

Thank you for your hard work :)

Fedeorlandau avatar Mar 09 '22 13:03 Fedeorlandau

Why is this error closed?

Because it was fixed... in a way which turned out to break other things, and had to be reverted. I'll reopen.

As for finding a new solution, I'm open to ideas. We've been focusing on trying to ship our next major version, so I haven't been able to dive back into this at a deep level, but I will admit that even coming up with my turned-out-to-cause-other-problems hack took quite a bit of poking and prodding and digging through not only nextjs source code but node source code as well, and so I don't expect finding a new approach is going to be a trivial problem.

In the meantime, you can suppress next's warning yourself by using the "external resolver" option in your API route (docs here). I've also just pushed https://github.com/getsentry/sentry-javascript/pull/4706, to allow our warning to be suppressed as well.

lobsterkatie avatar Mar 11 '22 05:03 lobsterkatie

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

github-actions[bot] avatar Apr 02 '22 00:04 github-actions[bot]

Please do not close

adarnon avatar Apr 02 '22 00:04 adarnon

You can set externalResolver to true in api route config since it is handled by sentry (external) https://nextjs.org/docs/api-routes/api-middlewares#custom-config

I am running a tRPC backend through NextJS with a Sentry wrapper, and this is by far the nicest solution imo. It worked like a charm.

edit: grammar

maxzaleski avatar Apr 15 '22 22:04 maxzaleski

Setting externalResolver to true has no effect on this for me. Adding a wrapper around withSentry to await it, then response.end() doesn't work either. I don't see a resolution that works anywhere here with latest NextJS and latest Sentry.

This is my API code to test - very trivial

const handler = (request: NextApiRequest, response: NextApiResponse) => {
  response.status(200);
  response.end();
};

export default withSentry(handler);

EDIT: if I remove the withSentryConfig wrapper from my next.config.js, THEN the externalResolver has an effect. This is such a bad experience because of Sentry. Why is the withSentryConfig overwriting the option that I need to fix the Sentry-created error?

dgattey avatar May 06 '22 19:05 dgattey

You can set externalResolver to true in api route config since it is handled by sentry (external) https://nextjs.org/docs/api-routes/api-middlewares#custom-config

I use this config and the messages at the console stop but still does not log the errors in Sentry Dashboard. I change the withSentry hoc in my api for Sentry.captureException(error) and it works. Just need to create a personal hoc to avoid writing this every time. I don't know if it's the best approach but for now it's all i got.

rafaelnogueira1 avatar Jun 08 '22 13:06 rafaelnogueira1

We ever fixing this or what? Annoying to have logs spammed.

leerobert avatar Jun 10 '22 14:06 leerobert

Ok I made a quick little hack around until this issue is resolved... it's not the cleanest but at the end of each api/endpoint.ts file I put the following:

async function handler(req: NextApiRequest, res: NextApiResponse) {
....
}

export const config = {
  api: {
    externalResolver: true,
  },
};

export default withSentry(handler);

Annoying but works. next.config.js gets overwritten by withSentryConfig so you gotta do this to each route.

leerobert avatar Jun 10 '22 14:06 leerobert

This is the way that is in the Sentry docs. But for me it doesn't show anything at Sentry dashboard :/.

rafaelnogueira1 avatar Jun 14 '22 00:06 rafaelnogueira1

It seems like the SENTRY_IGNORE_API_RESOLUTION_ERROR environment variable is not being respected anymore either. This is especially ironic since we also added the externalResolver API prop on every handler, so we get no logging from NextJS but we get Sentry's warning that NextJS might log for every request. Is that variable still working for anyone?

Edit: I don't even see anything in the repo that'd be looking for such a variable, but maybe it's buried in the CLI itself? In my case it's being provided in next.config.js alongside other env variables such as SENTRY_URL, SENTRY_PROJECT, etc, and all of those are working fine.

bsplosion avatar Aug 08 '22 13:08 bsplosion

Same boat as @bsplosion, externalResolver workaround does silence the next.js warning, but the sentry warnings that next.js may be hammering us with warnings ironically persists and hammers us with warnings 😮‍💨. SENTRY_IGNORE_API_RESOLUTION_ERROR doesn't seem to do anything

Edit: I lie. The SENTRY_IGNORE_API_RESOLUTION_ERROR flag does work when set in your local env variables. I dug around in the source code for the withSentry method and can see it referencing it before executing the console.warning. Don't know why it didn't work when I tried added the env var before or why searching the repo for that string doesn't hit that reference, but at least i've got the log squashed 😆

reuben-pct avatar Aug 09 '22 04:08 reuben-pct

Resolved by https://github.com/getsentry/sentry-javascript/pull/5778, which is published with 7.14.0.

AbhiPrasad avatar Oct 28 '22 10:10 AbhiPrasad

For anyone who still is getting the log messages, 7.14.0 doesn't resolve the logs on its own. You actually have to go through and strip out all the workaround code you've put in, including the withSentry calls in each API endpoint and potentially the externalResolver configs as well.

Thanks for getting a solution in place, @lobsterkatie. The logging is much better now!

bsplosion avatar Dec 05 '22 16:12 bsplosion

I'm still seeing the messages on 7.24.2, I have removed all of my withSentry wrappers and I never added the externalResolver change.

MisterJimson avatar Dec 08 '22 14:12 MisterJimson

I can confirm this is an issue on 7.32.1 but the same pattern was working fine on 7.31.1.

I didn't see anything in the diff which would indicate the breaking change, but the constant logging has definitely returned. Downgrading to 7.31.1 resolves again.

bsplosion avatar Jan 24 '23 13:01 bsplosion