opennextjs / opennextjs-netlify

Open Next.js adapter for Netlify
https://opennext.js.org/netlify
676 stars 86 forks source link

Cookies modified then read back again in an RSC return unmodified values #2669

Open serhalp opened 1 month ago

serhalp commented 1 month ago

next@v15.0.0-canary.160 unskipped some tests in this commit These tests hadn't been running for some time.

Many of these tests fail on Netlify. For example:

  ● app-dir with middleware › should be possible to modify cookies & read them in an RSC in a single request

    expect(received).toMatch(expected)

    Expected pattern: /Cookie 1: \d+\.\d+/
    Received string:  "Cookie 1:"

      132 |
      133 |     // cookies were set in middleware, assert they are present and match the Math.random() pattern
    > 134 |     expect(initialRandom1).toMatch(/Cookie 1: \d+\.\d+/)
          |                            ^
      135 |     expect(initialRandom2).toMatch(/Cookie 2: \d+\.\d+/)
      136 |     expect(totalCookies).toBe('Total Cookie Length: 2')
      137 |

      at Object.toMatch (e2e/app-dir/app-middleware/app-middleware.test.ts:134:28)

If you run the fixture locally, it appears to work as expected with next dev, ntl dev, and ntl serve, but it does not work as expected when deployed to Netlify (more detail below).

These tests and fix for the issue being tested were originally introduced in next.js in https://github.com/vercel/next.js/pull/65008. My guess is they wouldn't have passed at the time on Netlify. Deploying the fixture exhibits the same behaviour with older canary versions and with next@latest.

I believe this is possibly a confusing intersection of three unrelated things:

  1. there is an inherent flakiness in these tests causing cookie 1 to get unset: https://github.com/vercel/next.js/pull/71319
  2. the above is only triggering because of https://linear.app/netlify/issue/FRB-1359/nextjs-prefetch-priority-tests-are-failing (assuming that is a real issue as opposed to a simple test setup issue)
  3. what https://github.com/vercel/next.js/pull/65008 was intending to fix is not fixed on Netlify, presumably due to something in next-runtime

This is largely conjecture, but what I observe is that when reloading the fixture page repeatedly I sometimes see both cookies set on the page and on the response, sometimes one is missing on the page but not headers, and sometimes they are missing on both. It's nondeterministic and confusing. However, if I remove the <Link href="/rsc-cookies-delete"> (or add prefetch={false}), the behaviour is much simpler: the two cookies are always set on the response but the values on the page always reflect the previous values; e.g.

load 1:
  headers: 4 5
  page: [blank] [blank]
load 2:
  headers 6 7
  page: 4 5
load 3:
  headers 8 9
 page 6 7

which is what led me to the above conclusions. This is the same on next@latest and next@canary.

I have no idea why this cannot be reproduced locally. It might be timing related?

You can verify the above with this repo (which is mostly just a copy of the test fixture): https://github.com/serhalp/nextjs-canary-rsc-cookies-repro.

This has never been reported by users AFAIK.

Data

The following is parsed automatically by the Next.js repo e2e test report generator.

test: test/e2e/app-dir/app-middleware/app-middleware.test.ts reason: Cookies modified then read back again in an RSC return unmodified values

orinokai commented 4 weeks ago

I had a look at the repro and it seems the x-middleware-set-cookie header is getting set correctly, so I'd imagine the problem is in the part that reads that header and combines the contents with the actual cookies in the request, i.e. here: https://github.com/vercel/next.js/pull/65008/files#diff-d16b8f6eab3df537748d15ebbf5901a5b4700dff01fbf030c29c7a18d89c5ef6R108

Not sure why this is only a problem with the Next Runtime, perhaps something to do with its use of AsyncLocalStorage?