Improve reliablity of finding 'after' nodes. (#364)

timbrown5 · web-flow · commit c7ca8d934725 · 2024-03-15T22:46:59.000Z
diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationship.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationship.java
@@ -5,15 +5,10 @@
 import io.jenkins.plugins.pipelinegraphview.utils.BlueRun;
 import io.jenkins.plugins.pipelinegraphview.utils.NodeRunStatus;
 import io.jenkins.plugins.pipelinegraphview.utils.PipelineNodeUtil;
-import org.jenkinsci.plugins.workflow.actions.ErrorAction;
-import org.jenkinsci.plugins.workflow.actions.WarningAction;
-import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
-import org.jenkinsci.plugins.workflow.pipelinegraphanalysis.GenericStatus;
 import org.jenkinsci.plugins.workflow.pipelinegraphanalysis.StatusAndTiming;
 import org.jenkinsci.plugins.workflow.pipelinegraphanalysis.TimingInfo;
-import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 import org.jenkinsci.plugins.workflow.support.actions.PauseAction;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -117,60 +112,11 @@ public FlowNode getEnd() {
         } else if (PipelineNodeUtil.isPaused(this.end)) {
             return new NodeRunStatus(BlueRun.BlueRunResult.UNKNOWN, BlueRun.BlueRunState.PAUSED);
         }
-        // StatusAndTiming.computeChunkStatus2 assumes that a Stage is running if there
-        // is no after node. There are instances where this can happen (might be a bug
-        // in this code). To work around this we catch this case and explicitly generate
-        // the status ourselves.
-        if (this.after == null && !isRunning(run)) {
-            return new NodeRunStatus(getFinishNodeStatus());
-        }
         dump(
                 "Calculating Chunk Status start: %s, end: %s after: %s",
                 this.start.getId(), this.end.getId(), (this.after != null) ? this.after.getId() : "null");
         // Catch-all if none of the above are applicable.
         return new NodeRunStatus(
                 StatusAndTiming.computeChunkStatus2(run, this.before, this.start, this.end, this.after));
     }
-
-    /*
-     * Determine if the current block is still executing.
-     * Note: This doesn't seem efficient, but I couldn't see another way.
-     */
-    private boolean isRunning(WorkflowRun run) {
-        FlowExecution exec = run.getExecution();
-        if (exec != null) {
-            for (FlowNode head : exec.getCurrentHeads()) {
-                if (head.getAllEnclosingIds().contains(this.start.getId())) {
-                    return true;
-                }
-            }
-        }
-        return false;
-    }
-
-    /*
-     * Generate status for finished node.
-     * Source:
-     * https://github.com/jenkinsci/pipeline-graph-analysis-plugin/blob/master/src/
-     * main/java/org/jenkinsci/plugins/workflow/pipelinegraphanalysis/
-     * StatusAndTiming.java#L295
-     */
-    private GenericStatus getFinishNodeStatus() {
-        ErrorAction err = this.end.getError();
-        if (err != null) {
-            Throwable rootCause = err.getError();
-            if (rootCause instanceof FlowInterruptedException) {
-                return GenericStatus.fromResult(((FlowInterruptedException) rootCause).getResult());
-            } else {
-                return GenericStatus.FAILURE;
-            }
-        }
-        WarningAction warning = StatusAndTiming.findWorstWarningBetween(start, end);
-        if (warning != null) {
-            return GenericStatus.fromResult(warning.getResult());
-        }
-
-        // Previous chunk before end. If flow continued beyond this, it didn't fail.
-        return GenericStatus.SUCCESS;
-    }
 }
diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java
@@ -9,8 +9,10 @@
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode;
 import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
+import org.jenkinsci.plugins.workflow.graph.BlockStartNode;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -20,16 +22,24 @@ public class NodeRelationshipFinder {
     private boolean isDebugEnabled = logger.isDebugEnabled();
 
     private LinkedHashMap<String, FlowNode> endNodes = new LinkedHashMap<>();
-    private ArrayDeque<FlowNode> pendingEndNodes = new ArrayDeque<>();
-    private ArrayDeque<FlowNode> pendingStartNodes = new ArrayDeque<>();
 
-    // Somewhere to temporarily store the parallel branches information whilst we
-    // are handing a parallel block.
-    // The StatusAndTiming API requires us to the structure of the parallel block to
-    // get the status of a branch.
+    /* Stack of stacks to store the last seen node for each nested block we have gone into.
+     * Used to assign the after node for relationships.
+     * This can be a different type, depending on situation:
+     *  - For the last Atom node in a block, will be the BlockEndNode (or null if the step is running).
+     *  - For nodes that have later siblings, this will be the previous sibling we found.
+     *    - This might be a AtomNode or a BlockStartNode (when a step is followed by a StepBlock).
+     */
+    private ArrayDeque<ArrayDeque<FlowNode>> lastSeenNodes = new ArrayDeque<>();
+    private Map<String, ArrayDeque<FlowNode>> seenChildNodes = new LinkedHashMap<>();
+
+    /*  Somewhere to temporarily store the parallel branches information whilst we
+     * are handing a parallel block.
+     * The StatusAndTiming API requires us to the structure of the parallel block to
+     * get the status of a branch.
+     */
     private ArrayDeque<NodeRelationship> pendingBranchRelationships = new ArrayDeque<>();
 
-    private NodeRelationship subsequentStepRelationship = null;
     private LinkedHashMap<String, NodeRelationship> relationships = new LinkedHashMap<>();
 
     // Print debug message if 'isDebugEnabled' is true.
@@ -57,6 +67,9 @@ public LinkedHashMap<String, NodeRelationship> getNodeRelationships(
         dump("Sorted Ids: %s", String.join(", ", sortedIds));
         for (String id : sortedIds) {
             getRelationshipForNode(nodeMap.get(id));
+            // Add this node to the parents's stack as the last of it's child nodes that
+            //  we have seen.
+            addSeenNodes(nodeMap.get(id));
         }
         return relationships;
     }
@@ -73,8 +86,6 @@ private void getRelationshipForNode(@NonNull FlowNode node) {
     }
 
     private void handleBlockStart(@NonNull FlowNode node) {
-        // Reset step relationship - shouldn't carry over between blocks.
-        subsequentStepRelationship = null;
         // Assign end node to start node.
         if (FlowNodeWrapper.isStart(node)) {
             addBlockRelationship(node);
@@ -83,32 +94,68 @@ private void handleBlockStart(@NonNull FlowNode node) {
         }
     }
 
-    private void addStepRelationship(@NonNull StepAtomNode step) {
-        dump("Generating relationship for step %s", step.getId());
+    private void addSeenNodes(FlowNode node) {
+        if (!seenChildNodes.keySet().contains(node.getEnclosingId())) {
+            seenChildNodes.put(node.getEnclosingId(), new ArrayDeque<FlowNode>());
+        }
+        dump("Adding %s to seenChildNodes %s", node.getId(), node.getEnclosingId());
+        seenChildNodes.get(node.getEnclosingId()).push(node);
+    }
+
+    @CheckForNull
+    private FlowNode getAfterNode(FlowNode node) {
         FlowNode after = null;
-        // If we are followed by a step, that will be our after node, otherwise use the
-        // end node of the block.
-        if (subsequentStepRelationship != null) {
-            after = subsequentStepRelationship.getStart();
-            dump("Getting after node (%s) from subsequentStepRelationship", after.getId());
+        // The after node is the last child of the enclosing node, except for the last node in
+        // a block, then it's the last node in the enclosing nodes list (likely, this blocks end node).
+        FlowNode parentStartNode = getFirstEnclosingNode(node);
+        ArrayDeque<FlowNode> laterSiblings = getProcessedChildren(parentStartNode);
+        if (parentStartNode != null && laterSiblings.isEmpty()) {
+            // If there are no later siblings, get the parents later sibling.
+            ArrayDeque<FlowNode> parentsLaterSiblings = getProcessedChildren(getFirstEnclosingNode(parentStartNode));
+            after = parentsLaterSiblings.isEmpty() ? null : parentsLaterSiblings.peek();
+            dump(parentsLaterSiblings.toString());
         } else {
-            after = pendingEndNodes.peek();
-            dump(
-                    "Getting after node (%s) from endNodes stack (size: %s)",
-                    (after != null) ? after.getId() : "null", endNodes.size());
+            dump(laterSiblings.toString());
+            after = laterSiblings.peek();
         }
+        return after;
+    }
+
+    @CheckForNull
+    private BlockStartNode getFirstEnclosingNode(FlowNode node) {
+        return node.getEnclosingBlocks().isEmpty()
+                ? null
+                : node.getEnclosingBlocks().get(0);
+    }
+
+    private ArrayDeque<FlowNode> getProcessedChildren(@CheckForNull FlowNode node) {
+        if (node != null && seenChildNodes.keySet().contains(node.getId())) {
+            return seenChildNodes.get(node.getId());
+        }
+        return new ArrayDeque<FlowNode>();
+    }
+
+    private void addStepRelationship(@NonNull StepAtomNode step) {
+        dump("Generating relationship for step %s", step.getId());
+        // FlowNode after = subsequentNode;
+        FlowNode after = getAfterNode(step);
+        dump(
+                "Adding step for %s(%s),%s(%s)",
+                step.getId(),
+                step.getClass().getName(),
+                after == null ? "null" : after.getId(),
+                after == null ? "null" : after.getClass().getName());
         NodeRelationship nodeRelationship = new NodeRelationship(step, step, after);
         relationships.put(step.getId(), nodeRelationship);
-        subsequentStepRelationship = nodeRelationship;
     }
 
     private void handleBlockEnd(@NonNull BlockEndNode<?> endNode) {
         // Blindly push a new start pending reliable way to check for parallel node.
         FlowNode startNode = endNode.getStartNode();
         endNodes.put(startNode.getId(), endNode);
-        pendingEndNodes.push(endNode);
-        dump("Adding %s to pendingEndNodes", endNode.getId());
-        pendingStartNodes.push(startNode);
+        // Create new stack for this block, add the end node and push it to stack of stacks.
+        ArrayDeque<FlowNode> nodeBlockStack = new ArrayDeque<>();
+        lastSeenNodes.push(nodeBlockStack);
     }
 
     private void addBlockRelationship(@NonNull FlowNode node) {
@@ -131,15 +178,11 @@ private void addBlockRelationship(@NonNull FlowNode node) {
             if (endNode != node) {
                 relationships.put(endNode.getId(), blockRelationship);
             }
-            // Remove end node from stack (if there is one).
-            if (!pendingEndNodes.isEmpty()) {
-                pendingEndNodes.pop();
-            }
         }
     }
 
     private void addParallelBranchRelationship(@NonNull FlowNode node, @NonNull FlowNode endNode) {
-        FlowNode after = getBlockAfterNode();
+        FlowNode after = getAfterNode(node);
         // Store a parallel branch relationship - these will be used to build up the
         // parent parallel block relationship.
         // Once generated, that relationship will be superseded this one.
@@ -156,7 +199,7 @@ private void addParallelBranchRelationship(@NonNull FlowNode node, @NonNull Flow
     }
 
     private NodeRelationship addParallelRelationship(@NonNull FlowNode node, @NonNull FlowNode endNode) {
-        FlowNode after = getBlockAfterNode();
+        FlowNode after = getAfterNode(node);
         dump(
                 "Generating relationship for parallel Block %s (with after %s)",
                 node.getId(), (after != null) ? after.getId() : "null");
@@ -176,20 +219,8 @@ private NodeRelationship addParallelRelationship(@NonNull FlowNode node, @NonNul
         return parallelRelationship;
     }
 
-    @CheckForNull
-    private FlowNode getBlockAfterNode() {
-        FlowNode after = null;
-        if (!pendingEndNodes.isEmpty()) {
-            after = pendingEndNodes.pop();
-        }
-        dump(
-                "Getting after node (%s) from pendingEndNodes stack (size: %s)",
-                (after != null) ? after.getId() : "null", pendingEndNodes.size());
-        return after;
-    }
-
     private NodeRelationship addStageRelationship(@NonNull FlowNode node, @NonNull FlowNode endNode) {
-        FlowNode after = getBlockAfterNode();
+        FlowNode after = getAfterNode(node);
         dump(
                 "Generating relationship for Block %s{%s}->%s{%s} (with after %s{%s})",
                 node.getId(),
diff --git a/src/test/java/io/jenkins/plugins/pipelinegraphview/utils/PipelineStepApiTest.java b/src/test/java/io/jenkins/plugins/pipelinegraphview/utils/PipelineStepApiTest.java
@@ -11,7 +11,9 @@
 
 import hudson.model.Result;
 import hudson.model.queue.QueueTaskFuture;
+import io.jenkins.plugins.pipelinegraphview.treescanner.NodeRelationship;
 import io.jenkins.plugins.pipelinegraphview.treescanner.NodeRelationshipFinder;
+import io.jenkins.plugins.pipelinegraphview.treescanner.ParallelBlockRelationship;
 import io.jenkins.plugins.pipelinegraphview.treescanner.PipelineNodeGraphAdapter;
 import io.jenkins.plugins.pipelinegraphview.treescanner.PipelineNodeTreeScanner;
 import java.util.Arrays;
@@ -22,6 +24,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.junit.Before;
 import org.junit.Rule;
@@ -46,6 +49,8 @@ public void enabledDebugLogs() {
         l.record(PipelineNodeTreeScanner.class, Level.FINEST);
         l.record(PipelineNodeGraphAdapter.class, Level.FINEST);
         l.record(NodeRelationshipFinder.class, Level.FINEST);
+        l.record(NodeRelationship.class, Level.FINEST);
+        l.record(ParallelBlockRelationship.class, Level.FINEST);
     }
 
     @Test
@@ -483,4 +488,75 @@ public void stepsGetValidTimings() throws Exception {
             TestUtils.assertTimesInRange(n, checks.get(n.getName()));
         }
     }
+
+    @Issue("GH#362")
+    @Test
+    public void gh362_pausedParallelBranches() throws Exception {
+        WorkflowJob job = TestUtils.createJob(
+                j, "gh233_multipleRunningParallelBranches", "gh233_multipleRunningParallelBranches.jenkinsfile");
+
+        QueueTaskFuture<WorkflowRun> futureRun = job.scheduleBuild2(0);
+        WorkflowRun run = futureRun.waitForStart();
+        j.waitForMessage("Starting sleep A...", run);
+        j.waitForMessage("Starting sleep B...", run);
+        // Sleep to allow Pipeline to reach sleep.
+        Thread.sleep(500L);
+        List<PipelineStep> steps = new PipelineStepApi(run).getAllSteps().getSteps();
+        String stepsString = TestUtils.collectStepsAsString(steps, (PipelineStep s) -> TestUtils.nodeNameAndStatus(s));
+
+        LOGGER.log(Level.INFO, stepsString);
+        // Wait for Pipeline to end (terminating it means end nodes might not be
+        // created).
+        j.waitForCompletion(run);
+        List<PipelineStep> finishedSteps =
+                new PipelineStepApi(run).getAllSteps().getSteps();
+        String stepsStringFinished =
+                TestUtils.collectStepsAsString(finishedSteps, (PipelineStep s) -> TestUtils.nodeNameAndStatus(s));
+        LOGGER.log(Level.INFO, stepsStringFinished);
+
+        assertThat(
+                stepsString,
+                equalTo("Starting sleep A...{success},1{running},Starting sleep B...{success},1{running}"));
+        assertThat(
+                stepsStringFinished,
+                equalTo("Starting sleep A...{success},1{success},Starting sleep B...{success},1{success}"));
+    }
+
+    @Issue("GH#362")
+    @Test
+    public void gh362_stepsBeforeStepBlockGetValidStatus() throws Exception {
+        WorkflowJob job = TestUtils.createJob(j, "stepBlockInSteps", "stepBlockInSteps.jenkinsfile");
+
+        QueueTaskFuture<WorkflowRun> futureRun = job.scheduleBuild2(0);
+        WorkflowRun run = futureRun.waitForStart();
+        j.waitForMessage("Hello World", run);
+        // Sleep to allow Pipeline to reach sleep.
+        Thread.sleep(500L);
+        List<PipelineStep> steps = new PipelineStepApi(run).getAllSteps().getSteps();
+        String stepsString = TestUtils.collectStepsAsString(steps, (PipelineStep s) -> TestUtils.nodeNameAndStatus(s));
+
+        LOGGER.log(Level.INFO, stepsString);
+        // Wait for Pipeline to end (terminating it means end nodes might not be
+        // created).
+        j.waitForCompletion(run);
+
+        List<PipelineStep> finishedSteps =
+                new PipelineStepApi(run).getAllSteps().getSteps();
+        String stepsStringFinished =
+                TestUtils.collectStepsAsString(finishedSteps, (PipelineStep s) -> TestUtils.nodeNameAndStatus(s));
+        LOGGER.log(Level.INFO, stepsStringFinished);
+
+        assertThat(stepsString, equalTo("Hello World{success},1{running}"));
+        assertThat(stepsStringFinished, equalTo("Hello World{success},1{success},Goodbye World{success}"));
+
+        Map<String, List<Long>> checks = new LinkedHashMap<>();
+        // Give large ranges - we are testing that the values are feasible, not that they are precise.
+        checks.put("Hello World", Arrays.asList(1000L, 0L, 0L, 5000L, 500L, 500L));
+        checks.put("1", Arrays.asList(1000L, 0L, 1000L, 5000L, 1500L, 1500L));
+        checks.put("Goodbye World", Arrays.asList(0L, 0L, 0L, 5000L, 500L, 500L));
+        for (AbstractPipelineNode n : finishedSteps) {
+            assertThat(checks, hasEntry(is(n.getName()), notNullValue()));
+            TestUtils.assertTimesInRange(n, checks.get(n.getName()));
+        }
+    }
 }
diff --git a/src/test/resources/io/jenkins/plugins/pipelinegraphview/utils/stepBlockInSteps.jenkinsfile b/src/test/resources/io/jenkins/plugins/pipelinegraphview/utils/stepBlockInSteps.jenkinsfile
@@ -0,0 +1,15 @@
+pipeline {
+    agent any
+
+    stages {
+        stage('Hello') {
+            steps {
+                echo 'Hello World'
+                retry(1) {
+                    sleep(1)
+                }
+                echo 'Goodbye World'
+            }
+        }
+    }
+}
