Skip to content

[BUG] R2 cache population #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
vicb opened this issue Apr 7, 2025 · 7 comments · Fixed by #536
Closed

[BUG] R2 cache population #535

vicb opened this issue Apr 7, 2025 · 7 comments · Fixed by #536
Assignees
@vicb

Comments

@vicb
Copy link
Contributor

vicb commented Apr 7, 2025 •
edited
Loading

populate cache

The command opennextjs-cloudflate populateCache populates a bucket named NEXT_INC_CACHE_R2_BUCKET.

NEXT_INC_CACHE_R2_BUCKET is actually the name of the binding rather than the bucket.

We probably should use getPlatformProxy() to retrieve the binding and call .put() on it. The command should take an env a directory to persist data.

cache handler

getR2Key returns ${directory}/${process.env.NEXT_BUILD_ID ?? "no-build-id"}/${key}.${isFetch ? "fetch" : "cache"}

The key is typically /path/to/file.cache so we will end up with BUILD_ID//key which will not match the uploaded file.

@james-elicx thoughts?

Edit: found while tackling #528
@james-elicx
Copy link
Collaborator

I don't think getPlatformProxy supports remote does it? If we can get the parsed wrangler config we could just grab the name from that to use as well

@james-elicx
Copy link
Collaborator

The key problem likely applies to the other cache adapters as well

@vicb
Copy link
Contributor Author

vicb commented Apr 7, 2025

I have a fix PR, will upload early afternoon!

@vicb vicb self-assigned this Apr 7, 2025
vicb added a commit that referenced this issue Apr 7, 2025
@vicb
Fix R2 bucket population
2863582 
Fixes #535
@vicb vicb mentioned this issue Apr 7, 2025
Merged
@vicb
Copy link
Contributor Author

vicb commented Apr 7, 2025 •
edited
Loading

@james-elicx what was the rationale not to fallback to assets? as of today the cache assets are populated in R2 and uploaded in the assets - if we do not fallback they we do not need to have the assets.

@james-elicx
Copy link
Collaborator

james-elicx commented Apr 7, 2025 •
edited
Loading

@james-elicx what was the rationale not to fallback to assets? as of today the cache assets are populated in R2 and uploaded in the assets - if we do not fallback they we do not need to have the assets.

To use the data store as the single source-of-truth instead of mixing it with assets and having to do extra work to figure out when something in assets can be used or not.

I was planning to change KV to do auto-population this week so that we can remove the need for storing them in assets completely.

@vicb
Copy link
Contributor Author

vicb commented Apr 7, 2025

Thanks @james-elicx, makes sense.

Do you have a tracking issue for that? I'd like to add a few minor fixes to both the handler, could be tackled at the same time if you have time.

@james-elicx
Copy link
Collaborator

Do you have a tracking issue for that? I'd like to add a few minor fixes to both the handler, could be tackled at the same time if you have time.

Just made #538 for it.

@vicb vicb closed this as completed in 8abea36 Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vicb vicb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix R2 bucket population opennextjs/opennextjs-cloudflare
2 participants
@vicb @james-elicx