Do not serve stale values from the assets when there is no KV

[dario-piotrowicz]

This change makes sure that stale cache values from assets are not served when there is no KV binding.

This makes the issue in #328 no longer flaky (it makes the test always fail) since before it was passing because stale cache values were being served from assets (so the fetch cached result would always stay the same, making this check pass). The reason why KV is not being used is still being investigated.

No other tests besides the 328 one is affected as far as I know, since the 328 one was the only flaky we had, regarding all the other tests, the ones that were passing before as still passing after my change, and the ones that were failing before (and we skip) are failing after my change.

[changeset-bot]

🦋 Changeset detected

Latest commit: fffd2ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@opennextjs/cloudflare	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@344

commit: fffd2ec

[vicb]

Could you add a comment in the file about what the code is doing an why?

no longer flaky (it makes the test always fail)

Is that an improvement?

[dario-piotrowicz]

Could you add a comment in the file about what the code is doing an why?

ok

Is that an improvement?

Yes, this is fixing a false positive we currently have, locally things are not working as intended, we are serving state cached data which make the test app-router fetch cache test pass, with this change the state data is no longer served so the underlying broken catching logic gets exposed

[dario-piotrowicz]

Is that an improvement?

I thought we discussed this and agreed that not keep always serving the same stale build time cache data was the correct behavior, if you disagree I'm fine closing the PR

[vicb]

not keep always serving the same stale build time cache data was the correct behavior, if you disagree I'm fine closing the PR

I agree with that 100%.

My question was if going from flaky to failing is an improvement.

I think the outcome of that PR should be that the tests are passing?

[dario-piotrowicz]

My question was if going from flaky to failing is an improvement.

Well I see it as a temporary improvement, my plan here is to go from flaky-difficult-to-test bug -> consistently failing (easier to work on) bug -> fixed bug

I think the outcome of that PR should be that the tests are passing?

The outcome of this PR is to fix the fact that now we can serve static stale assets forever, as a side effect this makes working on the fetch cache bug easier, that will be addressed in a separate PR (probably #340)

[vicb]

My question was if going from flaky to failing is an improvement.

Well I see it as a temporary improvement, my plan here is to go from flaky-difficult-to-test bug -> consistently failing (easier to work on) bug -> fixed bug

I think the outcome of that PR should be that the tests are passing?

The outcome of this PR is to fix the fact that now we can serve static stale assets forever, as a side effect this makes working on the fetch cache bug easier, that will be addressed in a separate PR (probably #340)

The PR is about not serving stale cache values from assets when there is no KV binding. So I think the title and description of the PR must be updated as well as the changeset.

You could also expand the description of the PR to tell more about why/which of the (skipped) tests always fail after this PR:

• maybe we don't really understand and it's fine to say that we will investigate
• if we have some clue, it would be good to update the linked issue

The kind of information that I am looking for is why this PR would make the tests always fail - it is supposed to change the behavior only when there is no kv binding but we have a kv binding in the tests.

I hope this helps.

[dario-piotrowicz]

The PR is about not serving stale cache values from assets when there is no KV binding. So I think the title and description of the PR must be updated as well as the changeset.

Updated 👍

You could also expand the description of the PR to tell more about why/which of the (skipped) tests always fail after this PR:

• maybe we don't really understand and it's fine to say that we will investigate
• if we have some clue, it would be good to update the linked issue

The kind of information that I am looking for is why this PR would make the tests always fail - it is supposed to change the behavior only when there is no kv binding but we have a kv binding in the tests.

I've added the information, I hope it helps

[vicb]

Thanks a lot for the updates, super clear 🙏

[dario-piotrowicz]

Thanks for the approval @vicb 👍

We can merge this PR, or if you think it would be safer we can be merged when issue #328 is fully understood, either is totally fine by me

[vicb]

Let's merge it