fix(nextjs): Prevent false API resolved without sending a response …

…warning (#4139)

This prevents a false positive warning from nextjs which arises from the interaction of our API route wrapper `withSentry()` with nextjs's native API route handling.

In dev, nextjs checks that API route handlers return a response to the client before they resolve, and it throws a warning[1] if this hasn't happened. Meanwhile, in `withSentry()`, we wrap the `res.end()` method to ensure that events are flushed before the request/response lifecycle finishes.

As a result, there are cases where the handler resolves before the response is finished, while flushing is still in progress. This triggers the warning mentioned above, but it's a false alarm - the response may not have ended _yet_, but it will end. This suppresses the warning by temporarily marking the response finished, and then restoring it to unfinished before the original `res.end()` is called.

The fix here is simple, but the async-i-ness of it all leads to some complex interactions in sequencing between the SDK, nextjs, and Node itself. I've tried to lay it out as clearly as I can in comments.

Fixes #4007
Fixes #3852

[1] https://github.com/vercel/next.js/blob/e1464ae5a5061ae83ad015018d4afe41f91978b6/packages/next/server/api-utils.ts#L106-L118

packages/nextjs/src/utils/withSentry.ts

@@ -74,7 +74,57 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
       }
 
       try {
-        return await origHandler(req, res);
+        const handlerResult = await origHandler(req, res);
+
+        // Temporarily mark the response as finished, as a hack to get nextjs to not complain that we're coming back
+        // from the handler successfully without `res.end()` having completed its work.  This is necessary (and we know
+        // we can do it safely) for a few reasons:
+        //
+        // - Normally, `res.end()` is sync and completes before the request handler returns, as part of the handler
+        //   sending data back to the client. As soon as the handler is done, nextjs checks to make sure that the
+        //   response is indeed finished. (`res.end()` signals this by setting `res.finished` to `true`.) If it isn't,
+        //   nextjs complains. ("Warning: API resolved without sending a response for <url>.")
+        //
+        // - In order to prevent the lambda running the route handler from shutting down before we can send events to
+        //   Sentry, we monkeypatch `res.end()` so that we can call `flush()`, wait for it to finish, and only then
+        //   allow the response to be marked complete. This turns the normally-sync `res.end()` into an async function,
+        //   which isn't awaited because it's assumed to still be sync. As a result, nextjs runs the aforementioned
+        //   check before the now-async `res.end()` has had a chance to set `res.finished = false`, and therefore thinks
+        //   there's a problem when there's not.
+        //
+        // - In order to trick nextjs into not complaining, we can set `res.finished` to `true` before exiting the
+        //   handler. If we do that, though, `res.end()` gets mad because it thinks *it* should be the one to get to
+        //   mark the response complete. We therefore need to flip it back to `false` 1) after nextjs's check but 2)
+        //   before the original `res.end()` is called.
+        //
+        // - The second part is easy - we control when the original `res.end()` is called, so we can do the flipping
+        //   right beforehand and `res.end()` will be none the wiser.
+        //
+        // - The first part isn't as obvious. How do we know we won't end up with a race condition, such that the
+        //   flipping to `false` might happen before the check, negating the entire purpose of this hack? Fortunately,
+        //   before it's done, our async `res.end()` wrapper has to await a `setImmediate()` callback, guaranteeing its
+        //   run lasts at least until the next event loop. The check, on the other hand, happens synchronously,
+        //   immediately after the request handler (so in the same event loop). So as long as we wait to flip
+        //   `res.finished` back to `false` until after the `setImmediate` callback has run, we know we'll be safely in
+        //   the next event loop when we do so.
+        //
+        // And with that, everybody's happy: Nextjs doesn't complain about an unfinished response, `res.end()` doesn’t
+        // complain about an already-finished response, and we have time to make sure events are flushed to Sentry.
+        //
+        // One final note: It might seem like making `res.end()` an awaited async function would run the danger of
+        // having the lambda close before it's done its thing, meaning we *still* might not get events sent to Sentry.
+        // Fortunately, even though it's called `res.end()`, and even though it's normally sync, a) it's far from the
+        // end of the request process, so there's other stuff which needs to happen before the lambda can close in any
+        // case, and b) that other stuff isn't triggered until `res.end()` emits a `prefinished` event, so even though
+        // it's not technically awaited, it's still the case that the process can't go on until it's done.
+        //
+        // See
+        // https://github.com/vercel/next.js/blob/e1464ae5a5061ae83ad015018d4afe41f91978b6/packages/next/server/api-utils.ts#L106-L118
+        // and
+        // https://github.com/nodejs/node/blob/d8f1823d5fca5e3c00b19530fb15343fdd3c8bf5/lib/_http_outgoing.js#L833-L911.
+        res.finished = true;
+
+        return handlerResult;
       } catch (e) {
         // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can
         // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced
@@ -112,6 +162,16 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
 type ResponseEndMethod = AugmentedResponse['end'];
 type WrappedResponseEndMethod = AugmentedResponse['end'];
 
+/**
+ * Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish.
+ *
+ * Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping
+ * things in the right order, in this case it's safe', as explained in detail in the long comment in the main
+ * `withSentry()` function.
+ *
+ * @param origEnd The original `res.end()` method
+ * @returns The wrapped version
+ */
 function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
   return async function newEnd(this: AugmentedResponse, ...args: unknown[]) {
     const transaction = this.__sentryTransaction;
@@ -140,6 +200,11 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
       logger.log(`Error while flushing events:\n${e}`);
     }
 
+    // If the request didn't error, we will have temporarily marked the response finished to avoid a nextjs warning
+    // message. (See long note above.) Now we need to flip `finished` back to `false` so that the real `res.end()`
+    // method doesn't throw `ERR_STREAM_WRITE_AFTER_END` (which it will if presented with an already-finished response).
+    this.finished = false;
+
     return origEnd.call(this, ...args);
   };
 }