Skip to content
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.

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

Fix cache with @actions/cache #485

Merged
merged 2 commits into from May 20, 2023
Merged

Fix cache with @actions/cache #485

merged 2 commits into from May 20, 2023

Conversation

MSP-Greg
Copy link
Collaborator

@MSP-Greg MSP-Greg commented Mar 21, 2023

See also actions/toolkit#1378.

If the above PR is accepted, the change to bundler.js isn't needed, but it's a good idea, as we don't want the value to change when passed to @actions/cache's code...

Currently, we use:

cache.restoreCache(paths, key, restoreKeys)
// then later the following may run
cache.saveCache(paths, key)

Current code in @actions/cache modifies paths during the call to cache.restoreCache. Since the verification hash is based on that array, cache.saveCache will associate a verification key with the saved cache that cache.restoreCache will never match...

@MSP-Greg
Copy link
Collaborator Author

I hacked my fork's branch into a Puma workflow, and all works as expected...

@MSP-Greg
Copy link
Collaborator Author

Added a 2nd commit to change the baseKey, as I tested with essentially a flushed cache. Or, I removed any matching caches before testing with the Puma workflow.

Please 'Squash & Merge' if you want one commit...

@eregon
Copy link
Member

eregon commented Mar 22, 2023

Thanks, I would prefer to wait for the fix upstream. AFAIK there is no rush to use the latest actions/toolkit cache, so we can wait they fix it probably.
Thank you for the investigation and the fix there! Surprising that such a bug was only noticed now.

@MSP-Greg
Copy link
Collaborator Author

Regardless, passing an object (or a non-primitive) is not a good thing to do, especially when we depend on its value later in the code...

@liunaijia
Copy link

liunaijia commented Mar 23, 2023

@MSP-Greg thanks for the fix. I was tracking down another issue and found the same bug.

I think @actions/cache has fixed some known issues in its past releases. We're still using v3.0.6 which was released in August last year. Any plan to update the dependency? @eregon

@MSP-Greg MSP-Greg mentioned this pull request Mar 24, 2023
@eregon eregon merged commit 8a45918 into ruby:master May 20, 2023
149 checks passed
@MSP-Greg MSP-Greg deleted the 00-fix-cache branch December 27, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants