Conditional GET requests don't appear to be handled correctly

[mdlawson]

When etags are enabled in Next.js, conditional GET requests to an OpenNext lambda with an etag matching the page generated/cached by the server and considered fresh seem to result in a status 200 response with an empty body rather than the expected 304.

As far as I can tell, the key interaction is with this behaviour in the Next server when sending the 304 response:

function sendEtagResponse(req, res, etag) {
  ...
  if (fresh(req.headers, { etag })) {
    res.statusCode = 304
    res.end()
    return true
  }
  ...
}

and this block which captures the status code before the response is sent, and restores it afterwards.

    const originalStatus = res.statusCode
    ...
    if (!res.sent) {
      await this.sendRenderResult(req, res, {...}) // calls through to sendEtagResponse 
      res.statusCode = originalStatus
    }

The commit that added this (vercel/next.js@74153e1) is explicitly aiming to make sure that the 304 status code is hidden from the rest of the server so that it doesn't itself get cached.

However the response object supplied by OpenNext is not behaving the same way as a normal node one - when res.end() is called on a ServerResponse, the current value of statusCode is committed immediately, while with the OpenNextNodeResponse flushHeaders is only called once the stream "finish" event has been emitted asynchronously, by which time the status code has been reset back to 200 (or whatever other value it was previously). The status then returned to the lambda caller is incorrectly a 200 with an empty body, which may then get cached by CloudFront or similar, exacerbating the issue.

Not entirely sure what the right fix is here - the Next behaviour is a bit suspect, but does ultimately work with their Node server, and presumably it is necessary to allow the status code to be reset from 304 on the response to make sure it doesn't itself get cached. Maybe the OpenNextNodeResponse can internally snapshot statusCode at the time the response should be committed and use that for the lambda status, but still allow it to be mutated?

Reproduction

• Deploy a Next.js app router application with generateEtags: true config and a SSG route to lambda.
• Make a request to that route to get an etag header value. The body should be returned as expected.
• Make a second request with if-none-match set to that etag value. This request will return 200 with no body.

In case its relevant, I've been testing with the aws-lambda-streaming wrapper enabled and the nodejs20.x lambda runtime.

[conico974]

Are you not deploying behind cloudfront ?
On sst for example the default caching policy will remove the if-none-match header thus totally removing this issue

[mdlawson]

Interesting - I was seeing this behaviour using cloudfront but fronting another reverse proxy layer with its own caching that was adding the if-none-match headers. This does seem easy enough to work around though by setting generateEtags: false in Next, especially if typical setups wouldn't be seeing any benefit from conditional gets anyway.

[conico974]

@mdlawson Could you test the fix ? #632 (comment)

[mdlawson]

Seems to work great and return 304's for matching etags as expected.

Noticed a slight oddity when using a custom OpenNextHandler wrapper like this you still see the "200" status logged:

const handler: WrapperHandler = async (handler, converter) => {
  const loggingHandler: OpenNextHandler = async (event, responseStream) => {
    const result = await handler(event, responseStream);
    console.log(result);
    return result;
  };

  return await streamingWrapper.wrapper(loggingHandler, converter);
};

export default {
  wrapper: handler,
  name: "custom-aws-lambda",
  supportStreaming: true,
};

But thats super minor and I think just caused again by reading the status after Next has already reset it, which really just seems like unfortunate behaviour on the Next side.

Thanks for the quick fix!

[conico974]

Yeah unfortunately the way next does thing cause a race condition. There is no good way to do this.
The fix works well with streaming wrapper, but not with non streaming one because by the time the request is done and ready to be sent, next has already reset it.
And not applying the status code Next sets will cause the initial issue in Next appear again.
I'll try to open an issue in the Next repo when I got some time, the fix should be done upstream.