Overlay: use restoreCache() timeout - #3125
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the overlay-base database download mechanism to use the native timeout capabilities of the restoreCache() function instead of a custom timeout wrapper. The change replaces waitForResultWithTimeLimit() with direct usage of actionsCache.restoreCache() and its segmentTimeoutInMs option to provide more effective timeout control for download operations.
Key changes:
- Remove custom timeout wrapper in favor of native cache API timeout option
- Configure segment-level timeout for more granular download control
- Clean up unused timeout constant while preserving it for other cache operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/overlay-database-utils.ts | Replace waitForResultWithTimeLimit() wrapper with direct restoreCache() call using segmentTimeoutInMs option |
| lib/init-action.js | Generated JavaScript code reflecting the TypeScript changes, including variable renaming to avoid conflicts |
Sorry, something went wrong.
esbena
left a comment
There was a problem hiding this comment.
LGTM with one minor concern about download speeds outside the cloud.
Sorry, something went wrong.
| { | ||
| // Azure SDK download (which is the default) uses 128MB segments. | ||
| // Setting segmentTimeoutInMs to 3000 translates to segment download | ||
| // speed of about 40 MB/s, which should be achievable unless the |
There was a problem hiding this comment.
isn't 40MB/s in the high end for more residential connections?
What if this is somehow run from outside Actions? For instance for local test?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think you would generally run this locally. For tests, we'd mock the behaviour. Beyond that, I haven't checked, but I'd imagine that the API endpoints used by the Actions cache also are only accessible from hosted runners or at least require some suitable auth token. Setting that up locally is unlikely to ever be worth it. + even if we wanted to, it would be easy to adjust this value for local testing if it's too high.
Sorry, something went wrong.
There was a problem hiding this comment.
40MB/s is indeed very high for residential connections. I don't think I could reach this download speed from my connection at home.
That said, I am still inclined to keep this setting, at least for now.
- I agree with Michael that we are not very likely to run this code locally.
- Using overlay analysis is a performance optimization, and there is a point where download would take up so much time that the entire workflow ends up being slower end-to-end. That break-even point will probably differ from one repository to another, but 40MB/s seems like a good figure to start with.
As we gain more experience, I am definitely open to adjusting this limit as appropriate.
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
This seems reasonable to me, if you're happy with it.
In terms of the motivating issue, I suppose this is a change in objective? Rather than an overall timeout (which didn't work), there's now a timeout for individual segments (which does work, but there's no overall timeout)? So you might spend more time than MAX_CACHE_OPERATION_MS downloading the overlay cache if it's large, but no more than 3000ms per segment.
Sorry, something went wrong.
| actionsCache.restoreCache([dbLocation], cacheRestoreKeyPrefix), | ||
| () => { | ||
| logger.info("Timed out downloading overlay-base database from cache"); | ||
| const foundKey = await actionsCache.restoreCache( |
There was a problem hiding this comment.
Is there any harm in keeping this wrapped in waitForResultWithTimeLimit with a suitable time limit?
Sorry, something went wrong.
There was a problem hiding this comment.
No, there is no harm, and there would be benefits (such as protecting against hangs outside segment downloads).
I put the waitForResultWithTimeLimit() wrapper back, with a (longer) 10-minute time limit.
Thanks for the suggestion!
Sorry, something went wrong.
This commit changes overlay-base database download to pass the segmentTimeoutInMs option to restoreCache(), so that restoreCache() itself can properly abort slow downloads. The waitForResultWithTimeLimit() wrapper around restoreCache() remains as a second line of defense, but with a higher 10-minute time limit, to guard against cache restore hangs outside segment downloads.
This PR changes overlay-base database download to pass the segmentTimeoutInMs option to restoreCache(), so that restoreCache() itself can properly abort slow downloads.
The waitForResultWithTimeLimit() wrapper around restoreCache() remains as a second line of defense, but with a higher 10-minute time limit, to guard against cache restore hangs outside segment downloads.
See linked issue for rationale and alternatives considered.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist