fix: Improve error handling in restoreCache (fixes #120)#122
fix: Improve error handling in restoreCache (fixes #120)#122p0deje merged 1 commit intobazel-contrib:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect usage of the promise-based setTimeout API in the restoreCache function and improves error handling by ensuring proper cleanup of logging groups.
Key Changes:
- Corrects the misuse of
setTimeoutfromtimers/promisesby removing the invalid callback parameter - Adds try-finally block to ensure
core.endGroup()is called even when errors occur during cache restoration
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| index.js | Fixed setTimeout API usage and added try-finally block for proper error handling in restoreCache function |
| dist/main/index.js | Mirror of index.js changes in the compiled distribution file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -177,9 +178,9 @@ async function restoreCache(cacheConfig) { | |||
| } else { | |||
| core.info(`Failed to restore ${name} cache`) | |||
| } | |||
|
|
|||
| } finally { | |||
| core.endGroup() | |||
| }()) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
this reads correctly to me, but I'm not a maintainer
the previous broken timeout flow appears to be:
- call
setTimeout(<time>, <thenable>) - immediately generate
<thenable>via the iife (async function() { ... } ()) <thenable>crashes internally but isn't caught
this PR fixes the crash but should also ensure that the setTimeout delay is actually introduced before the cache restoration occurs then 🤞
also, from a quick scan, it doesn't appear like the setTimeout from timers/promises is used elsewhere in the repo
There was a problem hiding this comment.
we may want to log some core.info with a catch if @p0deje thinks that appropriate:
try {
...
} catch (err) {
core.info(`Failed to restore ${name} cache with error: ${err}`)
} finally {
core.endGroup()
}
Fixes #120 (supposedly).
This PR was generated by https://jules.google.com (on my request).
My TypeScript / Node Foo is not strong enough to be able to personally vouch for whether this is a valid fix.
Maintainers of this project should please carefully review if they think this makes sense.