[JENKINS-49073] Parallel step should propagate the worst result when not using failFast

pom.xml

@@ -167,6 +167,12 @@
             <version>1.11</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>pipeline-build-step</artifactId>
+            <version>2.10-rc366.8d263648aa27</version> <!-- TODO: Pending release of https://github.com/jenkinsci/pipeline-build-step-plugin/pull/24 -->
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>pipeline-input-step</artifactId>

src/main/java/org/jenkinsci/plugins/workflow/cps/steps/ParallelStep.java

@@ -3,23 +3,29 @@
 import com.cloudbees.groovy.cps.Outcome;
 
 import groovy.lang.Closure;
+import hudson.AbortException;
 import hudson.Extension;
+import hudson.model.Result;
 import hudson.model.TaskListener;
 import java.io.IOException;
 
 import org.jenkinsci.plugins.workflow.cps.CpsVmThreadOnly;
 import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
 import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
+import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 import org.jenkinsci.plugins.workflow.steps.Step;
 import org.jenkinsci.plugins.workflow.steps.StepContext;
 import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
 import org.jenkinsci.plugins.workflow.steps.StepExecution;
 
 import java.io.Serializable;
-import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -63,16 +69,20 @@ public StepExecution start(StepContext context) throws Exception {
         return failFast;
     }
 
-
     @PersistIn(PROGRAM)
     static class ResultHandler implements Serializable {
         private final StepContext context;
         private final ParallelStepExecution stepExecution;
         private final boolean failFast;
         /** Have we called stop on the StepExecution? */
         private boolean stopSent = false;
-        /** if we failFast we need to record the first failure */
-        private SimpleEntry<String,Throwable> originalFailure = null;
+        /**
+         * If we fail fast, we need to record the first failure.
+         *
+         * <p>We use a set because we may be encountering the same abort being delivered across
+         * branches. We use a linked hash set to maintain insertion order.
+         */
+        private final LinkedHashSet<Throwable> failures = new LinkedHashSet<>();
 
         /**
          * Collect the results of sub-workflows as they complete.
@@ -123,14 +133,7 @@ public void onFailure(StepContext context, Throwable t) {
                 } catch (IOException | InterruptedException x) {
                     LOGGER.log(Level.WARNING, null, x);
                 }
-                if (handler.originalFailure == null) {
-                    handler.originalFailure = new SimpleEntry<>(name, t);
-                } else {
-                    Throwable originalT = handler.originalFailure.getValue();
-                    if (t != originalT) { // could be the same abort being delivered across branches
-                        originalT.addSuppressed(t);
-                    }
-                }
+                handler.failures.add(t);
                 checkAllDone(true);
             }
 
@@ -153,18 +156,26 @@ private void checkAllDone(boolean stepFailed) {
                         return;
                     }
                     if (o.isFailure()) {
-                        if (handler.originalFailure == null) {
+                        if (handler.failures.isEmpty()) {
                             // in case the plugin is upgraded whilst a parallel step is running
-                            handler.originalFailure = new SimpleEntry<>(e.getKey(), e.getValue().getAbnormal());
+                            handler.failures.add(e.getValue().getAbnormal());
                         }
                         // recorded in the onFailure
                     } else {
                         success.put(e.getKey(), o.getNormal());
                     }
                 }
                 // all done
-                if (handler.originalFailure!=null) {
-                    handler.context.onFailure(handler.originalFailure.getValue());
+                List<Throwable> toAttach = new ArrayList<>(handler.failures);
+                if (!handler.failFast) {
+                    Collections.sort(toAttach, new ThrowableComparator(new ArrayList<>(handler.failures)));
+                }
+                if (!toAttach.isEmpty()) {
+                    Throwable head = toAttach.get(0);
+                    for (int i = 1; i < toAttach.size(); i++) {
+                        head.addSuppressed(toAttach.get(i));
+                    }
+                    handler.context.onFailure(head);
                 } else {
                     handler.context.onSuccess(success);
                 }
@@ -173,6 +184,62 @@ private void checkAllDone(boolean stepFailed) {
             private static final long serialVersionUID = 1L;
         }
 
+        static final class ThrowableComparator implements Comparator<Throwable>, Serializable {
+
+            private final List<Throwable> insertionOrder;
+
+            ThrowableComparator() {
+                this.insertionOrder = new ArrayList<>();
+            }
+
+            ThrowableComparator(List<Throwable> insertionOrder) {
+                this.insertionOrder = insertionOrder;
+            }
+
+            @Override
+            public int compare(Throwable t1, Throwable t2) {
+                if (!(t1 instanceof FlowInterruptedException)
+                        && t2 instanceof FlowInterruptedException) {
+                    // FlowInterruptedException is always higher than any other exception.
+                    return -1;
+                } else if (t1 instanceof FlowInterruptedException
+                        && !(t2 instanceof FlowInterruptedException)) {
+                    // FlowInterruptedException is always higher than any other exception.
+                    return 1;
+                } else if (!(t1 instanceof AbortException) && t2 instanceof AbortException) {
+                    // AbortException is always higher than any exception other than
+                    // FlowInterruptedException.
+                    return -1;
+                } else if (t1 instanceof AbortException && !(t2 instanceof AbortException)) {
+                    // AbortException is always higher than any exception other than
+                    // FlowInterruptedException.
+                    return 1;
+                } else if (t1 instanceof FlowInterruptedException
+                        && t2 instanceof FlowInterruptedException) {
+                    // Two FlowInterruptedExceptions are compared by their results.
+                    FlowInterruptedException fie1 = (FlowInterruptedException) t1;
+                    FlowInterruptedException fie2 = (FlowInterruptedException) t2;
+                    Result r1 = fie1.getResult();
+                    Result r2 = fie2.getResult();
+                    if (r1.isWorseThan(r2)) {
+                        return -1;
+                    } else if (r1.isBetterThan(r2)) {
+                        return 1;
+                    }
+                } else if (insertionOrder.contains(t1) && insertionOrder.contains(t2)) {
+                    // Break ties by insertion order. Earlier errors are worse.
+                    int index1 = insertionOrder.indexOf(t1);
+                    int index2 = insertionOrder.indexOf(t2);
+                    if (index1 < index2) {
+                        return -1;
+                    } else if (index1 > index2) {
+                        return 1;
+                    }
+                }
+                return 0;
+            }
+        }
+
         private static final long serialVersionUID = 1L;
     }