Improve cycle detection when constructing the dependency graph by oheger-bosch · Pull Request #11575 · oss-review-toolkit/ort

oheger-bosch · 2026-03-17T11:35:24Z

This PR contains two fixes for potential stack overflow errors that might occur during construction of the dependency graph when there are cycles in dependencies. The errors were observed in practice in a Yarn2 project. This is probably related to #11524.

codecov · 2026-03-17T11:53:48Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.41%. Comparing base (6886cfb) to head (c988bd9).
⚠️ Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
model/src/main/kotlin/utils/DependencyHandler.kt	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11575      +/-   ##
============================================
+ Coverage     58.38%   58.41%   +0.03%     
- Complexity     1754     1761       +7     
============================================
  Files           355      355              
  Lines         13211    13222      +11     
  Branches       1313     1314       +1     
============================================
+ Hits           7713     7724      +11     
  Misses         5008     5008              
  Partials        490      490

Flag	Coverage Δ
funTest-external-tools	`14.56% <0.00%> (-0.02%)`	⬇️
funTest-no-external-tools	`30.63% <0.00%> (+0.05%)`	⬆️
test-ubuntu-24.04	`42.45% <84.61%> (+0.05%)`	⬆️
test-windows-2025	`42.43% <84.61%> (+0.05%)`	⬆️

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.

oheger-bosch · 2026-03-18T06:39:08Z

This is also related to #11426.

I am not sure about the failing fun test for cocoapods; this may be related to the changes or not. Would need to test this locally, but would need to set up cocoapods first.

sschuberth · 2026-03-18T07:07:46Z

With this, would the Set<DependencyGraphEdge>.removeCycles() / breakCycles() logic still be needed, or could it be removed to simplify the code? Because with this we should not get cycles anymore in the first place, right?

oheger-bosch · 2026-03-18T08:03:41Z

With this, would the Set<DependencyGraphEdge>.removeCycles() / breakCycles() logic still be needed, or could it be removed to simplify the code? Because with this we should not get cycles anymore in the first place, right?

I am not sure about this. This PR prevents a stack overflow when constructing a dependency graph with cyclic dependencies. But obviously, this logic was introduced in the past to handle cycles in graphs that could be constructed successfully. So maybe there are constellations that still lead to such cycles.

oheger-bosch · 2026-03-18T08:27:51Z

I am not sure about the failing fun test for cocoapods; this may be related to the changes or not. Would need to test this locally, but would need to set up cocoapods first.

I could run CocoaPodsFunTest successfully on my local machine with my changes. So, the test failure should be unrelated.

sschuberth · 2026-03-18T14:55:34Z

I could run CocoaPodsFunTest successfully on my local machine with my changes. So, the test failure should be unrelated.

I've retriggered the test run to verify.

sschuberth · 2026-03-18T14:59:28Z

But obviously, this logic was introduced in the past to handle cycles in graphs that could be constructed successfully.

Well, my point is that the existing logic maybe only had to be introduced because we did not have the (more general?) checks from this PR.

So maybe there are constellations that still lead to such cycles.

We should try to find our what those "constellations" are, if any. Do we trust our existing test coverage in that area enough to say that if with Set<DependencyGraphEdge>.removeCycles() / breakCycles() removed all tests still pass, it's indeed safe to remove them?

oheger-bosch · 2026-03-18T15:16:44Z

I could run CocoaPodsFunTest successfully on my local machine with my changes. So, the test failure should be unrelated.

I've retriggered the test run to verify.

This time, there are other test failures :-/

oheger-bosch · 2026-03-18T15:24:23Z

But obviously, this logic was introduced in the past to handle cycles in graphs that could be constructed successfully.

Well, my point is that the existing logic maybe only had to be introduced because we did not have they (more general?) checks from this PR.

So maybe there are constellations that still lead to such cycles.

We should try to find our what those "constellations" are, if any. Do we trust our existing test coverage in that area enough to say that if with Set<DependencyGraphEdge>.removeCycles() / breakCycles() removed all tests still pass, it's indeed safe to remove them?

I would support this, but I do not why this logic was added initially; were there concrete issues or was this a more theoretical exercise? The code is from @fviernau - do you remember?

Regarding test coverage, I have my doubts, since it is very difficult to simulate such scenarios - in this PR I had to do some tricks with spies. And the functional tests are probably not complex enough.

sschuberth · 2026-03-19T08:08:45Z

I could run CocoaPodsFunTest successfully on my local machine with my changes. So, the test failure should be unrelated.

While CocoaPodsFunTest locally succeeded for me as well, I'm still concerned that we continuously start seeing it failing with this PR (due to a timeout). Maybe it still works, but for some reason takes much more time? Note that PodDependencyHandler actually overrides areDependenciesEqual(), so I really don't currently see how it could be related, unless this new code would actually create cycles.

oheger-bosch · 2026-03-19T08:19:58Z

This is indeed strange. Does the test really fail on each run? There were also runs showing other test failures.

My local execution of the test was quite fast.

sschuberth · 2026-03-19T08:38:33Z

Does the test really fail on each run?

Maybe not on every run, but I've retriggered the test at least 5 times yesterday, and no run was without (some) failure.

My local execution of the test was quite fast.

I guess that depends on your definition of "fast" 😅 On main, running the whole of CocoaPodsFunTest for me takes about 1m 30s. On your branch, it makes about 2m 10s for me. So quite clearly an increase in runtime.

oheger-bosch · 2026-03-19T12:27:42Z

The culprit seems to be the change in DependencyHandler.areDependenciesEqual(). This increases the runtime significantly. I am investigating.

oheger-bosch · 2026-03-20T06:46:21Z

The current version of this fix is working as expected now. Running CocoaPodsFunTest locally does not show any performance degradation any more, and the test was also successful on CI. It seems, however, to be impossible to get the fun tests on CI green; there is always a different failure :-(

sschuberth · 2026-03-20T07:15:19Z

The current version of this fix is working as expected now.

Could you please share more details about what caused the previous problem, and how it's being solved now? I'm curious, esp. about why PodDependencyHandler was affected although it comes with its own areDependenciesEqual() overload.

oheger-bosch · 2026-03-20T07:29:16Z

When checking for the equality of dependencies, the previous version of the check had this code to check the transitive dependencies:

compareDependencyGraphs(
                dependenciesFor(depA) - processedA,
                dependenciesFor(depB) - processedB,
                processedA + dependenciesA,
                processedB + dependenciesB
            )

This obviously caused numerous recursive calls, even if the currently processed nodes have already been encountered before. This has now been replaced with this code:

(depA in processedA && depB in processedB) || compareDependencyGraphs(
                dependenciesFor(depA),
                dependenciesFor(depB),
                processedA + dependenciesA,
                processedB + dependenciesB
            )

Here no recursion is started if both nodes have already been processed before; so, the cycle is broken early.

The problems with CocoaPodsFunTest were not caused by PodDependencyHandler. In fact, two dependency graphs are constructed in this test, one for NPM and one for Pod. It was the NPM dependency graph that took so long.

sschuberth · 2026-03-20T07:43:58Z

model/src/main/kotlin/utils/DependencyGraphBuilder.kt

-        val transitiveDependencies = dependencyHandler.dependenciesFor(dependency).mapNotNullTo(mutableSetOf()) {
-            addDependencyToGraph(scopeName, it, transitive = true, processed)
-        }
+        val nextProcessed = processed + dependency


Commit message:

its own set of transitive dependencies.

I'm wondering, shouldn't this say "direct dependencies"? I though the special case of a cycle that's being fixed here is when the cycle is "as tight as it can be", with a dependency pointing directly to itself.

If the above is not correct, could you make more clear in the commit message how this type of cycle differs from the type of cycles that were already handled before this change?

Actually, this fix does not handle new types of cyclic dependencies. It just executes the check to detect cycles at a different time. The original problem was that this check came too late and therefore did not prevent the stack overflow.

Could you add the above to the first commit's message then, removing most of its current content? To me, this explanation is much clearer.

Changed the commit message.

The original problem was that this check came too late and therefore did not prevent the stack overflow.

For reference, this confirms what I had assumed already back here.

Improve the detection of cycles in the dependency graph when adding dependencies. Move the already existing check for cycles, so that it gets executed earlier before entering the recursive handling of transitive dependencies. In the original code, this check happened too late and did not prevent a stack overflow error, which was reported in a Yarn2 project. Signed-off-by: Oliver Heger <[email protected]>

The `areDependenciesEqual()` function could run in a stack overflow if the graphs to be compared contained cycles. Fix this by keeping track of the nodes that have been processed. Signed-off-by: Oliver Heger <[email protected]>

oheger-bosch force-pushed the oheger-bosch/yarn_stack_overflow branch from 73735ab to 69f5230 Compare March 17, 2026 14:07

oheger-bosch marked this pull request as ready for review March 18, 2026 06:16

oheger-bosch requested a review from a team as a code owner March 18, 2026 06:16

oheger-bosch mentioned this pull request Mar 18, 2026

Yarn 2+ failed to resolve dependencies for path ...: StackOverflowError: null; Exception in thread "main" java.lang.IllegalArgumentException: Unable to create the AnalyzerResult #11524

Closed

sschuberth previously approved these changes Mar 18, 2026

View reviewed changes

oheger-bosch dismissed sschuberth’s stale review via e008ffd March 19, 2026 15:07

oheger-bosch force-pushed the oheger-bosch/yarn_stack_overflow branch from 69f5230 to e008ffd Compare March 19, 2026 15:07

oheger-bosch requested a review from sschuberth March 20, 2026 06:46

sschuberth reviewed Mar 20, 2026

View reviewed changes

oheger-bosch added 2 commits March 20, 2026 10:33

oheger-bosch force-pushed the oheger-bosch/yarn_stack_overflow branch from e008ffd to c988bd9 Compare March 20, 2026 09:33

oheger-bosch requested a review from sschuberth March 20, 2026 09:33

sschuberth approved these changes Mar 20, 2026

View reviewed changes

sschuberth enabled auto-merge (rebase) March 20, 2026 11:04

sschuberth merged commit fbdf7cb into oss-review-toolkit:main Mar 20, 2026
36 of 39 checks passed

sschuberth mentioned this pull request Mar 20, 2026

test(model): Fix the test for cyclic dependencies #11426

Open

Conversation

oheger-bosch commented Mar 17, 2026

Uh oh!

codecov bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oheger-bosch commented Mar 18, 2026

Uh oh!

sschuberth commented Mar 18, 2026

Uh oh!

oheger-bosch commented Mar 18, 2026

Uh oh!

oheger-bosch commented Mar 18, 2026

Uh oh!

sschuberth commented Mar 18, 2026

Uh oh!

sschuberth commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oheger-bosch commented Mar 18, 2026

Uh oh!

oheger-bosch commented Mar 18, 2026

Uh oh!

sschuberth commented Mar 19, 2026

Uh oh!

oheger-bosch commented Mar 19, 2026

Uh oh!

sschuberth commented Mar 19, 2026

Uh oh!

oheger-bosch commented Mar 19, 2026

Uh oh!

oheger-bosch commented Mar 20, 2026

Uh oh!

sschuberth commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oheger-bosch commented Mar 20, 2026

Uh oh!

sschuberth Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

oheger-bosch Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

sschuberth Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

oheger-bosch Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

sschuberth Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 17, 2026 •

edited

Loading

sschuberth commented Mar 18, 2026 •

edited

Loading

sschuberth commented Mar 20, 2026 •

edited

Loading