Skip to content

retry npm install when it fails#4334

Merged
rochdev merged 1 commit intomainfrom
rochdev/npm-install-retry
Mar 30, 2025
Merged

retry npm install when it fails#4334
rochdev merged 1 commit intomainfrom
rochdev/npm-install-retry

Conversation

@rochdev
Copy link
Copy Markdown
Member

@rochdev rochdev commented Mar 18, 2025

Motivation

This is a common cause of flakiness, often with a random 500 error from npm or from an ECONNRESET. Rerunning it reduces the flakiness pretty much to 0 (or as close as we can realistically expect without rerunning many times).

Changes

Retry npm install when it fails.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@rochdev rochdev marked this pull request as ready for review March 18, 2025 23:44
@rochdev rochdev requested review from a team as code owners March 18, 2025 23:44
@cbeauchesne
Copy link
Copy Markdown
Collaborator

cbeauchesne commented Mar 19, 2025

Would it be more clean to use npm config options : https://docs.npmjs.com/cli/v8/using-npm/config#fetch-retries ?

chatgpt told it can be achieved by npm config set fetch-retries 2

@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Mar 21, 2025

chatgpt told it can be achieved by npm config set fetch-retries 2

Interestingly that is the default and it doesn't seem to work (or some other issue might be at play). We've been using a manual retry for a while in dd-trace and it seems to pretty much have resolved the issue entirely (short of a full-blown npm outage).

@cbeauchesne
Copy link
Copy Markdown
Collaborator

it doesn't seem to work (or some other issue might be at play).

Do you mean that it does not try a second time, or it may try, but it does not solve the issue?

Because if it's the default, and if it actually tries twice, then the equivalent of npm install || npm install would be npm config set fetch-retries 4.

I just checked, and from what I see in logs, it respects the config given (even +1) :

> npm config set registry https://httpstat.us/500
> npm config set fetch-retries 10
> npm install axios --verbose

... logs

npm http fetch GET 500 https://httpstat.us/500/axios 315458ms attempt #11 (cache skip)

See the #11

Well, whatever, if you're more confortable with the double call, I'm fine with that.

@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Mar 21, 2025

Do you mean that it does not try a second time, or it may try, but it does not solve the issue?

I think it does try a second time, but it may do so with a strategy that doesn't quite work for us. I agree that the new code is technically equivalent to 4 retries, but since it's done differently, it's not quite equivalent. For example, in dd-trace-js we use yarn which doesn't retry out of the box, yet retrying manually (so effectively 2 retries total) fixed the issue entirely. This is also true of npm view. Basically, because our use case uses so many different flavours and versions of package managers, it seems to be a safer bet to retry manually even if that's not pretty, instead of relying on potentially incorrect or variable implementations.

@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Mar 30, 2025

We had another occurrence of the issue, going to merge this for now to see if it makes a difference. We can re-evaluate this if it ends up not fixing the issue.

@rochdev rochdev merged commit bccdffd into main Mar 30, 2025
442 checks passed
@rochdev rochdev deleted the rochdev/npm-install-retry branch March 30, 2025 20:51
mtoffl01 pushed a commit that referenced this pull request Apr 3, 2025
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.

2 participants