test(yarn2): Demonstrate an issue with circular dependencies

[mnonnenmacher]

This PR demonstrates an issue with the handling of circular dependencies in the DependencyGraphBuilder, found while working on fixing the Yarn2 dependency resolution.

The first commit creates a minimal test project for Yarn2 with two dependencies that depend on each other. This leads to a stack overflow in DependencyGraphBuilder.insertIntoGraph when it tries to add transitive dependencies. The root cause is that the Yarn2DependencyHandler.dependenciesFor implementation is based on the output of yarn info which contains the cycle:

{"value":"mlly@npm:1.8.0","children":{"Version":"1.8.0","Manifest":{"License":"MIT","Homepage":null},"Dependencies":[{"descriptor":"pkg-types@npm:^1.3.1","locator":"pkg-types@npm:1.3.1"}, ...]}}
{"value":"pkg-types@npm:1.3.1","children":{"Version":"1.3.1","Manifest":{"License":"MIT","Homepage":null},"Dependencies":[{"descriptor":"mlly@npm:^1.7.4","locator":"mlly@npm:1.8.0"}, ...]}}

This dependency cycle was found when trying to analyze this project:
https://github.com/traefik/traefik/tree/master/webui

The second commit tries to fix that issue by breaking cycles in transitive dependencies. With that fix, the first stack overflow does not happena anymore, but then there is a stack overflow in the default implementation of DependencyHandler.areDependenciesEqual when it tries to compare the dependency subtrees when adding the nodes.

The third commit adds a minimal reproducer for the Yarn2 scenario from the first commit.

@oheger-bosch I am not sure what would be the best approach to fix this issue and if the fix from the second commit is even correct. I was hoping that maybe you have an idea how to handle such a scenario. One possible fix for the second issue could be to add a custom implementation of areDependenciesEqual to Yarn2DependencyHandler that can handle the cycles, but I think it would be better if such scenarios would be correctly handled by the default implementations.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.41%. Comparing base (4cee34b) to head (45f136d).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
...el/src/main/kotlin/utils/DependencyGraphBuilder.kt	86.66%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11207      +/-   ##
============================================
+ Coverage     58.39%   58.41%   +0.02%     
- Complexity     1759     1764       +5     
============================================
  Files           355      355              
  Lines         13204    13219      +15     
  Branches       1307     1309       +2     
============================================
+ Hits           7710     7722      +12     
  Misses         5007     5007              
- Partials        487      490       +3     

Flag	Coverage Δ
test-ubuntu-24.04	42.34% <86.66%> (+0.07%)	⬆️
test-windows-2025	42.32% <86.66%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sschuberth]

As a side note, IMO breaking cycles should be an inherent feature of the dependency graph builder, so that not all package managers need to implement such logic separately. Also see #10083 in that regard.

[mnonnenmacher]

@oheger-bosch I am not sure what would be the best approach to fix this issue and if the fix from the second commit is even correct. I was hoping that maybe you have an idea how to handle such a scenario. One possible fix for the second issue could be to add a custom implementation of areDependenciesEqual to Yarn2DependencyHandler that can handle the cycles, but I think it would be better if such scenarios would be correctly handled by the default implementations.

@oheger-bosch I would appreciate your feedback, I think it would be great if the DependencyGraphBuilder could centrally solve the issue with circular dependencies to simplify the package manager implementations.

[oheger-bosch]

@oheger-bosch I would appreciate your feedback, I think it would be great if the DependencyGraphBuilder could centrally solve the issue with circular dependencies to simplify the package manager implementations.

Yes, agreed. If possible, this case should be handled by the base implementation.

[sschuberth]

While the test now passes, the current expected result is not what I would expected to be listed there. It now seems that the duplicated node (that creates the cycle) itself is not listed again, though I would expect it to be listed, but just without its dependencies. Otherwise the fact that there is a cycle would not be visible in the output.

What do you think @oheger-bosch?

[sschuberth]

I would expect these not to be listed here, but only for pky-types below, which is closer to the root.

[sschuberth]

I would expect to see mlly being listed here (without dependencies).

[oheger-bosch]

While the test now passes, the current expected result is not what I would expected to be listed there. It now seems that the duplicated node (that creates the cycle) itself is not listed again, though I would expect it to be listed, but just without its dependencies. Otherwise the fact that there is a cycle would not be visible in the output.

What do you think @oheger-bosch?

Would have to look deeper into this. But first I have to deal with some demanding customers.

[oheger-bosch]

In #11627 I have added functionality to create a marker node for cycles in the dependency tree. In the synthetic unit tests, the results look good. You might want to check this against the Yarn project added here.

[sschuberth]

You might want to check this against the Yarn project added here.

I've rebased this PR onto yours, but in the dependency tree serialization the issue is the same as before, the node that creates the loop is omitted completely (expected on the left, actual on the right):

[oheger-bosch]

Could be that during the serialization to the tree format something goes wrong. In the test for the graph builder, I have added some functionality to serialize results in the graph format. Would this be an option for this test as well? Because, if the problem lies in the conversion to the tree format, I don't know whether it is worth the effort to fix it.

[sschuberth]

Would this be an option for this test as well? Because, if the problem lies in the conversion to the tree format, I don't know whether it is worth the effort to fix it.

Esp. for tests I believe that the tree format is much more readable. So even if all package managers would use the graph format at some point, I guess for tests we'd still keep the tree serialization around for the time being. So I believe it's worth fixing.

[oheger-bosch]

I have run the version from #11627 against the Yarn2 project causing a stackoverflow. It creates the node indicating a cycle, and this node is correctly displayed in the tree view of the WebApp report (even multiple times, since it is referenced from different packages):

So, the approach is obviously working.

[sschuberth]

So, the approach is obviously working.

I'm not meaning to doubt that the approach is working for the dependency graph; I rather assume there's a bug / limitation in the graph -> tree mapping before serializing the tree.

[oheger-bosch]

So, the approach is obviously working.

I'm not meaning to doubt that the approach is working for the dependency graph; I rather assume there's a bug / limitation in the graph -> tree mapping before serializing the tree.

Yes, these are separate issues. But this means that #11627 can be reviewed/merged as is, since it brings actual value to end users. The limitation only affects tests that are based on the tree format.

[sschuberth]

The limitation only affects tests that are based on the tree format.

Which for me, as a reviewer, makes it harder to comprehend the changes in and the effect of #11627. It would be just so much more convenient to convince oneself about the validity of the changes if they were also reflected in the tree to see the changes to (existing) expected results.

[oheger-bosch]

I see. Unfortunately, I am currently unable to spend more time on this. So, feel free to either take it over or leave it.

[oheger-bosch]

I was looking again into #11627. I have rebased this PR on the current state. The cyclic test is still failing, but the diff I get is as follows:

Isn't this exactly what it should be? So, could we maybe resurrect #11627 ?

[sschuberth]

Isn't this exactly what it should be?

Almost. Except that I would expect to see an indication of a cycle below the "leaf" mlly packages. Something like

- id: "NPM::mlly:1.8.0"
  cycle: true

So, could we maybe resurrect #11627 ?

I guess "resurrect" is the wrong wording here. At least from my end, I did not even start a real review due to the sheer code complexity added, let alone bit-shift and -masking operations... we've also been discussing this with @oss-review-toolkit/core-devs. So while doing good, I currently do not see how that PR could get merged in its current state, unfortunately.

[oheger-bosch]

This is indeed unfortunate, I was not aware about any such discussions, so also had no chance to address them.
Well, in any case, it was at least an interesting challenge.

[sschuberth]

I was not aware about any such discussions

We indeed missed to get back to you until now due to other priorities. Apologies for that.