Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch #302

dwnusbaum · 2023-08-24T18:43:55Z

Looking into more issues along the lines of #287. Note that I did verify that this particular issue occurs both before and after that PR.

Testing done

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

… in some cases

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java

jglick

Fine to start by merging as tests with @Ignore.

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java

…e it has less than 2 branches

dwnusbaum · 2023-08-24T22:29:49Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java

-        Fork split(@NonNull HashMap<FlowNode, FlowPiece> nodeMapping, @NonNull BlockStartNode joinPoint, @NonNull FlowPiece joiningBranch) {
-            int index = visited.lastIndexOf(joinPoint);  // Fork will be closer to end, so this is better than indexOf
-            Fork newFork = new Fork(joinPoint);
-
-            if (index < 0) {
-                throw new IllegalStateException("Tried to split a segment where the node doesn't exist in this segment");
-            } else if (index == this.visited.size()-1) { // We forked just off the most recent node
-                newFork.following.add(this);
-                newFork.following.add(joiningBranch);
-                this.visited.remove(index);
-            } else if (index == 0) {
-                throw new IllegalStateException("We have a cyclic graph or heads that are not separate branches!");
-            } else { // Splitting at some midpoint within the segment, everything before becomes part of the following
-                // Execute the split: create a new fork at the fork point, and shuffle the part of the flow after it
-                //   to a new segment and add that to the fork.
-
-                FlowSegment newSegment = new FlowSegment();
-                newSegment.after = this.after;
-                newSegment.visited.addAll(this.visited.subList(0, index));
-                newFork.following.add(newSegment);
-                newFork.following.add(joiningBranch);
-                this.after = newFork;
-                this.isLeaf = false;
-
-                // Remove the part before the fork point
-                this.visited.subList(0, index+1).clear();
-                for (FlowNode n : newSegment.visited) {
-                    nodeMapping.put(n, newSegment);
-                }


This logic previously had branches to account for being called in various ways. I changed the way it is called making all but one of the branches unreachable.

dwnusbaum · 2023-08-24T22:30:26Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java

@@ -235,7 +235,6 @@ interface FlowPiece {  // Mostly a marker
    // TODO see if this can be replaced with a FlowChunk acting as a container class for a list of FlowNodes
    static class FlowSegment implements FlowPiece {
        ArrayList<FlowNode> visited = new ArrayList<>();
-        FlowPiece after;


Was not ever used apart from test assertions and my simplifications left it always null, making it completely pointless.

dwnusbaum · 2023-08-24T22:31:48Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java

+                    if (isParallelStart(nextBlockStart)) {
+                        // We make a fork for every parallel step preemptively in case it has less than 2 branches.


This is the fix, essentially instead of waiting until we have a piece that needs to be merged with an existing segment, we create the Fork objects right away as soon as we see a parallel step.

dwnusbaum · 2023-08-24T22:32:17Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java

+                    } else {
+                        throw new IllegalStateException("Unexpected join on " + existingPiece);


Should be unreachable now, since all merges should hit the above condition 🤞

Turns out this is still reachable in at least some cases, see com.cloudbees.workflow.rest.endpoints.ParallelStepTest.test_stages_order in pipeline-rest-api

java.lang.IllegalStateException: Unexpected join on org.jenkinsci.plugins.workflow.graphanalysis.ForkScanner$FlowSegment@428b6b34 at org.jenkinsci.plugins.workflow.graphanalysis.ForkScanner.leastCommonAncestor(ForkScanner.java:414) at org.jenkinsci.plugins.workflow.graphanalysis.ForkScanner.setHeads(ForkScanner.java:453) at org.jenkinsci.plugins.workflow.graphanalysis.AbstractFlowScanner.setup(AbstractFlowScanner.java:140) at org.jenkinsci.plugins.workflow.graphanalysis.AbstractFlowScanner.setup(AbstractFlowScanner.java:151) at org.jenkinsci.plugins.workflow.graphanalysis.ForkScanner.visitSimpleChunks(ForkScanner.java:630) at com.cloudbees.workflow.rest.external.RunExt.createNew(RunExt.java:323) at com.cloudbees.workflow.rest.external.RunExt.create(RunExt.java:311) at com.cloudbees.workflow.rest.external.JobExt.create(JobExt.java:143) at com.cloudbees.workflow.rest.endpoints.JobAPI.doRuns(JobAPI.java:69)

I haven't been able to reproduce the failure yet. The test in question starts a build that uses sleep in some parallel branches and just checks it as soon as it has a Run, so the flow execution is an an unknown and possibly changing state when ForkScanner runs.

(EDIT: In fact I think the test only passes because it normally checks the branch order before the Pipeline finishes the sleep in branch 2.)

Finally reproduced it, seems to be quite rare (fails about 1 in 40 times for me), so I'm glad it happened to fail in the PCT run!

The problematic case seems to be where the parallel step start node is itself a head. That means it gets left out of blockStartIterator for all heads, and we end up trying to join two heads on whatever segment has the first parent of the parallel step. It seems to be possible to get things working by restoring some of the old cases in FlowSegment.split, but that seems undesirable, because we end up with a Fork whose start node is not actually a parallel step, which is bizarre.

I have a fix for this, but as I add new test assertions I keep finding new things that don't work and need to compare against the previous behavior to understand if they ever worked, so it is slow going.

dwnusbaum · 2023-08-24T22:33:01Z

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java

-        ForkScanner.Fork forked = mainBranch.split(nodeMap, (BlockStartNode)exec.getNode("4"), sideBranch);
-        ForkScanner.FlowSegment splitSegment = (ForkScanner.FlowSegment)nodeMap.get(BRANCH1_END); // New branch
-        Assert.assertNull(splitSegment.after);
-        FlowTestUtils.assertNodeOrder("Branch 1 split after fork", splitSegment.visited, 9, 8, 6);
-
-        // Just the single node before the fork
-        Assert.assertEquals(forked, mainBranch.after);
-        FlowTestUtils.assertNodeOrder("Head of flow, pre-fork", mainBranch.visited, 3);
-


This test was using split directly in a way that is no longer supported and not done by production code, so I tried to adapt the tests.

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java

dwnusbaum · 2023-08-24T22:36:53Z

I am going to test this a bit more, and I want to look into whether we can make the iteration order for incomplete parallel steps match the order of the closures map in the Pipeline script (the order for complete parallel steps already matches the script order).

dwnusbaum · 2023-08-29T20:45:46Z

This fix has subtle problems in various cases that are difficult to fix comprehensively. For now I am closing this, see #306.

Add test demonstrating that ForkScanner skips outer parallel branches…

d74337a

… in some cases

dwnusbaum commented Aug 24, 2023

View reviewed changes

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java Show resolved Hide resolved

jglick reviewed Aug 24, 2023

View reviewed changes

src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java Show resolved Hide resolved

Create a ForkScanner.Fork for every parallel step preemptively in cas…

819d8a8

…e it has less than 2 branches

dwnusbaum changed the title ~~Add test demonstrating that ForkScanner skips outer parallel branches in some cases~~ Prevent ForkScanner from skipping nodes when scanning incomplete parallel branches that only have a single branch Aug 24, 2023

dwnusbaum commented Aug 24, 2023

View reviewed changes

dwnusbaum added the bug label Aug 25, 2023

dwnusbaum changed the title ~~Prevent ForkScanner from skipping nodes when scanning incomplete parallel branches that only have a single branch~~ Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch Aug 25, 2023

dwnusbaum closed this Aug 29, 2023

dwnusbaum deleted the more-forkscanner-problems branch August 29, 2023 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch #302

Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch #302

dwnusbaum commented Aug 24, 2023 •

edited

Loading

Submitter checklist

jglick left a comment

dwnusbaum Aug 24, 2023

dwnusbaum Aug 24, 2023

dwnusbaum Aug 24, 2023 •

edited

Loading

dwnusbaum Aug 24, 2023

dwnusbaum Aug 28, 2023 •

edited

Loading

dwnusbaum Aug 28, 2023 •

edited

Loading

dwnusbaum Aug 28, 2023

dwnusbaum Aug 28, 2023

dwnusbaum Aug 28, 2023

dwnusbaum Aug 24, 2023

dwnusbaum commented Aug 24, 2023

dwnusbaum commented Aug 29, 2023

		if (isParallelStart(nextBlockStart)) {
		// We make a fork for every parallel step preemptively in case it has less than 2 branches.

		} else {
		throw new IllegalStateException("Unexpected join on " + existingPiece);

Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch #302

Prevent ForkScanner from skipping nodes when scanning incomplete parallel steps that only have a single branch #302

Conversation

dwnusbaum commented Aug 24, 2023 • edited Loading

Testing done

Submitter checklist

jglick left a comment

Choose a reason for hiding this comment

dwnusbaum Aug 24, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 24, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 24, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 28, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 28, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 28, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 24, 2023

Choose a reason for hiding this comment

dwnusbaum commented Aug 24, 2023

dwnusbaum commented Aug 29, 2023

dwnusbaum commented Aug 24, 2023 •

edited

Loading

dwnusbaum Aug 24, 2023 •

edited

Loading

dwnusbaum Aug 28, 2023 •

edited

Loading

dwnusbaum Aug 28, 2023 •

edited

Loading