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

Change timeout to apply to the overall build process, not per-build #2185

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Aug 7, 2023

fixes #1910

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Aug 7, 2023
@Nemo157
Copy link
Member Author

Nemo157 commented Aug 7, 2023

Mostly a draft because I want more discussion on the idea, this would drastically reduce the total amount of time crates can consume, from 5.5 hours (15 minutes per build, 2 builds per target (with/without --show-coverage), 10 targets, + default-target rebuild after deleting Cargo.lock) down to 15 minutes if we don't change the default limit.

@Nemo157
Copy link
Member Author

Nemo157 commented Aug 7, 2023

One other case this potentially impacts a lot is when a crate is published with an incorrect lockfile, to get any docs the failure with the old lockfile and then the successful build with the new lockfile will have to complete together within the timeout.

I want to add more detailed logging and more visible metrics around this to the /crate/.../builds pages, so authors may improve their crates configuration, but would prefer to delay that till after the work I'm trying to get completed soon on #1011.

@Nemo157 Nemo157 changed the title Change timeout to apply to the overall build process, not per-target Change timeout to apply to the overall build process, not per-build Aug 7, 2023
@syphar
Copy link
Member

syphar commented Aug 8, 2023

when we would deploy this, we wouln't have any idea how many crate builds we would break.

I assume #2186 would help a little, but only for the builds in the time observed. Same as #1011 when we record more information about the build.

Do we have other ways to optimize?
For example, do we try a broken lockfile for each target?

@Nemo157
Copy link
Member Author

Nemo157 commented Aug 8, 2023

For example, do we try a broken lockfile for each target?

Nope, only on the default target, when the default target succeeds we do all the extra-targets with whichever lockfile was successful.

@syphar syphar added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Aug 9, 2023
@syphar
Copy link
Member

syphar commented Aug 9, 2023

Without more data this change feels quite risky, what do you think about waiting for the results of #2186 and then deciding?

@Nemo157
Copy link
Member Author

Nemo157 commented Aug 9, 2023

Sounds good.

@Nemo157
Copy link
Member Author

Nemo157 commented Aug 27, 2023

Some stats from the last week:

image

The orange line is 15 minutes, the red line is 30 minutes. We did have the 99th percentile exceed 30 minutes briefly, but I think it's prettty safe as an overall timeout if it'll just cause some targets to be missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't spend an hour on crates that timeout during the build
2 participants