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

Introducing timeout in segment download to avoid the download stuck problem #1140

Merged
merged 19 commits into from
Aug 9, 2022

Conversation

bishal-pdMSFT
Copy link
Contributor

@bishal-pdMSFT bishal-pdMSFT commented Jul 29, 2022

This PR focuses on fixing the download stuck problem that had been reported by several users in the past, especially when caches are of larger sizes (>= 2GB).

To avoid the problem we've introduced a timeout that aborts the stuck download if not completed within 1 hour.

We have kept the timeout as 1 hour because the cache sizes can be very huge and on self hosted runners, the downloads might not be as fast as GitHub hosted runners, hence to accommodate for everyone and avoid inconvenience for some users the timeout has been set to 1 hour.

PS - lerna has been upgraded to version 5.x because 4.x has some vulnerability that was being caught by the audit action. The upgrade has been made using npm's audit fix tool.

@kotewar kotewar marked this pull request as ready for review August 5, 2022 05:41
@kotewar kotewar requested review from a team as code owners August 5, 2022 05:41
@kotewar kotewar changed the title Hack for timeout for segment download Introducing timeout in segment download to avoid the download stuck problem Aug 5, 2022
@kotewar kotewar marked this pull request as draft August 5, 2022 06:09
@kotewar kotewar marked this pull request as ready for review August 5, 2022 07:19
@kotewar kotewar merged commit d714ea0 into main Aug 9, 2022
@smorimoto
Copy link
Contributor

The bad thing about this is that PR is implemented on a fairly strong assumption that the existence of the cache is always optional. Some software doesn't make it optional, and it's technically wrong for caching, but in that case this really doesn't work anyway. You don't need to do this for this fairly low-level package, and this work should definitely be done only on the actions/cache side. It's really enough to leave a reference to that implementation in the documentation of this package.

@smorimoto
Copy link
Contributor

In short, it's not a bad thing to abort, but it should throw an error anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants