Delay resolving patterns to existing versions until all final version…

[blexrob]

…s are known. Fixes #3466

Summary

When PackageRequester has retrieved the version information for a pattern, it queries the package resolver for the current best match for its pattern. If it finds one, it immediately resolves to that version.

This can cause indeterminism. If we have requests for graceful-fs@4.1.10, graceful-fs@^4.1.2 and graceful-fs@^4.1.11, the following can happen.

1. graceful-fs@4.1.10 resolves first, to 4.1.10
2. graceful-fs@^4.1.2 resolves next, finds 4.1.10, resolves to that.
3. graceful-fs@^4.1.11 resolves last to 4.1.11.

In a different order, the following can happen:

1. graceful-fs@4.1.10 resolves first, to 4.1.10
2. graceful-fs@^4.1.11 resolves next to 4.1.11.
3. graceful-fs@^4.1.2 resolves last, finds 4.1.11, resolves to that.

This PR reworks the package-requester to store requests that match an existing version with the resolver. When the resolver has processed all other patterns, it then resolves those stored requests. These requests will then always find the best version.

Test plan
Added a test that installs and checks the package.json from #3466. This test isn't deterministic, but if the error is present it should find it now and then. I have run the test a few time without the fix and seen it fail. The whole testsuite runs without error.

Risks
I have adjusted the default package resolver and the resolver from import.js, a risk is that I have overlooked other (less well-tested) paths.

[voxsim]

Mmh for what I understand from your pr we ordered patterns in a different way, am I correct?

Do you think we can code something that is deterministic and indipendent from the order? (maybe we need to add a phase after, I am just guessing).

Btw thanks for your work, you are awesome!!!

[blexrob]

As far as I can tell from the code, the intention is that the intent is to resolve patterns to already found versions, and only introduce new versions if none of the current versions are a match. So, the logic is:

• look up existing versions. A match? use that one
• go to the registry to get the best version match, add that to the set of current versions

(the current code always does the registry lookup part, but that is only needed for yarn import. Could be a perf improvement to skip it for yarn add/install, but that's something for another issue).

The indeterminism comes from immediately using matching versions - because resolutions without matches can introduce extra versions, which could be better matches.

This PR makes the packagerequest report the packagerequest that it found a match to the resolver (with the function reportPackageWithExistingVersion). The resolver then records that package request in the delayedResolveQueue.

When the resolver has run all packagerequests, it then runs resolvePackagesWithExistingVersions (this is the phase after you mentioned). This will call resolveToExistingVersion in the package requests that found an existing version. This function will re-run the search for existing versions. A match was found previously, so it will be found again. At this time, all packagerequests have found their version info, so all versions that are needed to satisfy all requests are know to the resolver. So, the packagerequest will find the best matching version every time.

As far as I understand the code, the final set of resolved versions should be the same every run (when the input and the registry responses are the same, of course). And therefore, the best version match for a pattern will also be the same, and therefore deterministic.

[bestander]

Huge thanks, that looks great and should remove randomness from the package resolution.

[raido]

When will this ship? It seems this might the issue for me, because yarn.lock seems to "fluctuate" if new packages are added from different branches, even if taking the master branch yarn.lock as base file just before a merge of another branch.

[bestander]

In 0.26, I think we can release RC on Friday

[blexrob]

@bestander @voxsim I just found an oversight in my analysis: with 2 patterns, a@1.0.1 (A) and a@^1.0.0 (B), current version 1.0.2, the following can happen:
Order 1

1. A resolves, adds version 1.0.1
2. B resolves, finds matching version 1.0.1, delays
3. final resolve for B, to version 1.0.1
   Outcome: a@1.0.1 -> 1.0.1, a@^1.0.0 -> 1.0.1

Order 2

1. B resolves, adds version 1.0.2
2. A resolves, adds version 1.0.1
   Outcome: a@1.0.1 -> 1.0.1, a@^1.0.0 -> 1.0.2

The committed code will still mitigate problems, but it won't completely solve them...

I'm afraid a better algorithm is needed to get fully deterministic resolves.

[voxsim]

yep :) I think to be deterministic we need to put in place some rules that are invariant respect the order otherwise it will be always not deterministic. For example from your last message we should pick always version 1.0.2 for B. I don't know if it is possible, i will dig further in the topic in the next weeks.

[voxsim]

Btw i want to thank you so much, one of our task is to make package-* files stateless and this analysis helps a lot :D

[bestander]

Ok, I'll file a separate issue

[bestander]

Done #3489