Merge pull request #287 from dwnusbaum/forkscanner-double-visit

Prevent ForkScanner from visiting nodes more than once in some cases for in-progress builds with nested parallelism
jenkinsci · Jun 26, 2023 · 05cd837 · 05cd837
2 parents 2bee3e1 + f1242e7
commit 05cd837
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 2 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java b/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java
@@ -48,6 +48,7 @@
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * Scanner that will scan down all forks when we hit parallel blocks before continuing, but generally runs in linear order
@@ -443,8 +444,15 @@ ArrayDeque<ParallelBlockStart> leastCommonAncestor(@NonNull final Set<FlowNode>
             throw new IllegalStateException("No least common ancestor found from " + heads);
         }
 
-        // If we hit issues with the ordering of blocks by depth, apply a sorting to the parallels by depth
-        return convertForksToBlockStarts(parallelForks);
+        // The result must be in reverse topological order, i.e. inner branches must be visited before outer branches.
+        return convertForksToBlockStarts(parallelForks).stream().sorted((pbs1, pbs2) -> {
+            if (pbs1.forkStart.getEnclosingBlocks().contains(pbs2.forkStart)) {
+                return -1;
+            } else if (pbs2.forkStart.getEnclosingBlocks().contains(pbs1.forkStart)) {
+                return 1;
+            }
+            return 0;
+        }).collect(Collectors.toCollection(ArrayDeque::new));
     }
 
     @Override

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java
@@ -992,4 +992,39 @@ public void testParallelCorrectEndNodeForVisitor() throws Exception {
         testParallelFindsLast(jobPauseSecond, "wait2");
         testParallelFindsLast(jobPauseMiddle, "wait3");
     }
+
+    @Test
+    public void inProgressParallelInParallel() throws Exception {
+        WorkflowJob p = r.createProject(WorkflowJob.class);
+        p.setDefinition(new CpsFlowDefinition(
+                "stage('MyStage') {\n" +            // 3, 4
+                "  parallel(\n" +                   // 5
+                "    outerA: {\n" +                 // 8
+                "      semaphore('outerA')\n" +     // 11
+                "    },\n" +
+                "    outerB: {\n" +                 // 9
+                "      echo('outerB')\n" +          // 12
+                "    },\n" +                        // 13
+                "    outerC: {\n" +                 // 10
+                "      parallel(\n" +               // 14
+                "        innerA: {\n" +             // 16
+                "          semaphore('innerA')\n" + // 18
+                "        },\n" +
+                "        innerB: {\n" +             // 17
+                "          echo('innerB')\n" +
+                "        }\n" +
+                "      )\n" +
+                "    }\n" +
+                "  )\n" +
+                "}\n", true));
+        WorkflowRun b = p.scheduleBuild2(0).waitForStart();
+        SemaphoreStep.waitForStart("outerA/1", b);
+        SemaphoreStep.waitForStart("innerA/1", b);
+        ForkScanner scanner = new ForkScanner();
+        sanityTestIterationAndVisiter(b.getExecution().getCurrentHeads());
+        SemaphoreStep.success("outerA/1", null);
+        SemaphoreStep.success("innerA/1", null);
+        r.assertBuildStatusSuccess(r.waitForCompletion(b));
+        sanityTestIterationAndVisiter(b.getExecution().getCurrentHeads());
+    }
 }