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

Update npm config with fetch-retry-mintimeout and fetch-retry-maxtimeout #6698

Closed
wants to merge 7 commits into from

Conversation

geriux
Copy link
Contributor

@geriux geriux commented Mar 5, 2024

This PR attempts to solve the current errors when installing npm dependencies in CI, mostly happening in the Android jobs with the error:

npm ERR! code ETIMEDOUT
npm ERR! network
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly.  See: 'npm help config'

It increases the following config:

  • fetch-retry-mintimeout to 120000
  • fetch-retry-maxtimeout to 300000

For comparison in my local setup, the default values are 10000 for fetch-retry-mintimeout and 60000 for fetch-retry-maxtimeout

To test all CI checks should pass.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@geriux geriux changed the title Update npm config with fetch-retry-mintimeout and fetch-retry-maxtimeout Update npm config with fetch-retry-mintimeout and fetch-retry-maxtimeout Mar 5, 2024
@geriux geriux marked this pull request as ready for review March 5, 2024 13:36
@geriux geriux added the Tooling label Mar 5, 2024
@geriux geriux added this to the 1.115.0 (24.5) milestone Mar 5, 2024
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geriux I'm curious about how the timeout error would be addressed by increasing fetch-retry-mintimeout and fetch-retry-maxtimeout. As far as I checked the documentation, these parameters are used to wait for retrying fetching a package. The increase implies more retries based on the default factor (10). Per the retry module documentation , the timeout is calculated with the following formula.

Math.min(random * minTimeout * Math.pow(factor, attempt), maxTimeout)

That said, I wonder if instead of increasing the timeouts of retries, we could simply increase the request timeout (fetch-timeout). WDYT?

@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

From your comment, I tried that in #6699 and it doesn't work for our issues. When looking for solutions, the most mentioned approaches were to increase fetch-retry-mintimeout and fetch-retry-maxtimeout. For example in this issue from the npm client repo. I also saw other posts like this one.

It appears that fetch-timeout specifies the maximum time npm will wait for a single network request to complete before timing out. Meanwhile, the fetch-retry-mintimeout option sets the minimum timeout value for retry attempts when npm encounters a failed network request, as seems to be the case in our CI environment due to limited network speed/stability.

@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

I'm going to retry all jobs again just to make sure they pass the npm installation step. 🤞

@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

Some of the jobs will fail due to the existing artifacts in CI. I think we could try this approach and see how it performs. What do you think @fluiddot ?

@fluiddot
Copy link
Contributor

fluiddot commented Mar 5, 2024

Some of the jobs will fail due to the existing artifacts in CI. I think we could try this approach and see how it performs. What do you think @fluiddot ?

Sounds good, the changes won't break the CI jobs so let's try it.

@geriux geriux force-pushed the try/increase-npm-timeout branch from c9405d4 to 39ed993 Compare March 5, 2024 17:11
@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

Ok it is still failing 😅 . We should probably fix the CI issues by adding cache to see if it solves this npm failures.

I'll close this for now.

@fluiddot
Copy link
Contributor

fluiddot commented Mar 5, 2024

@geriux Another option we could try is to reduce the number of parallel connections with maxsockets. Maybe npm is initiating too many requests and the Buildkite instance has some kind of limitation.

@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

@geriux Another option we could try is to reduce the number of parallel connections with maxsockets. Maybe npm is initiating too many requests and the Buildkite instance has some kind of limitation.

Interesting, let's try that!

@fluiddot
Copy link
Contributor

fluiddot commented Mar 5, 2024

Seems changing maxsockets didn't address the issue 😞. At this point, I think we should investigate the Buildkite instance to avoid connection timeouts.

@geriux
Copy link
Contributor Author

geriux commented Mar 5, 2024

Seems changing maxsockets didn't address the issue 😞. At this point, I think we should investigate the Buildkite instance to avoid connection timeouts.

Yup, I'm trying one more thing just out of curiosity which is to remove the --prefer-offline flag, since there's no cache at the moment. But yeah we should investigate what's happening with Buildkite instead.

@geriux geriux closed this Mar 5, 2024
@geriux geriux deleted the try/increase-npm-timeout branch March 5, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants