Notify GraphListener extensions prior to listeners added via FlowExecution.addListener (#1052)

* Run local listeners after listeners from extensions

* Notify listeners in order according to GraphListener.ordinal

* Link to upstream PR

* Update baseline and BOM as needed for workflow-api update

* Simplify changes by handling TimingAction for FlowStartNode directly in FlowHead

* Remove SNAPSHOT dependency on workflow-job, which was only used for testing

* Remove unused imports

* Adjust new import ordering to minimize diff

* Remove another unused import and improve import ordering

* Simplify based on review feedback

Co-authored-by: Jesse Glick <[email protected]>

---------

Co-authored-by: Jesse Glick <[email protected]>

Author: dwnusbaum

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java

@@ -323,6 +323,9 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume {
         }
     }
 
+    /**
+     * Holds listeners added via {@link #addListener} in the reverse order that they were added.
+     */
     private transient List<GraphListener> listeners;
 
     /**
@@ -1229,7 +1232,7 @@ public void addListener(GraphListener listener) {
         if (listeners == null) {
             listeners = new CopyOnWriteArrayList<>();
         }
-        listeners.add(listener);
+        listeners.add(0, listener);
     }
 
     @Override public void removeListener(GraphListener listener) {
@@ -1527,12 +1530,13 @@ private static void cleanUpClassHelperCache(@NonNull Class<?> clazz) throws Exce
     }
 
     List<GraphListener> getListenersToRun() {
-        List<GraphListener> l = new ArrayList<>();
-
+        // Listeners from extensions always come first, ordered by `Extension.ordinal`. Listeners added via
+        // `addListener` are then notified in the reverse order that they were added (see `addListener`) so that the
+        // `build-finalizing WorkflowRun$GraphL` always runs last.
+        List<GraphListener> l = new ArrayList<>(ExtensionList.lookup(GraphListener.class));
         if (listeners != null) {
             l.addAll(listeners);
         }
-        l.addAll(ExtensionList.lookup(GraphListener.class));
 
         return l;
     }

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java

@@ -111,6 +111,7 @@ void newStartNode(FlowStartNode n) throws IOException {
             }
             execution.flowStartNodeActions.clear();
         } // may be unset from loadProgramFailed
+        n.addAction(new TimingAction());
         synchronized (execution) {
             this.head = execution.startNodes.push(n);
         }

plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java

@@ -1,19 +1,26 @@
 package org.jenkinsci.plugins.workflow;
 
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.Matchers.is;
+
+import hudson.ExtensionList;
 import hudson.model.Run;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
 import org.jenkinsci.plugins.workflow.flow.GraphListener;
+import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.junit.Assert;
-import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ErrorCollector;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.LoggerRule;
 import org.jvnet.hudson.test.TestExtension;
 
+import java.io.IOException;
 import java.io.Serializable;
 import java.util.List;
 import java.util.Random;
@@ -22,12 +29,15 @@
 
 public class GraphListenerTest
 {
-    @ClassRule
-    public static JenkinsRule r = new JenkinsRule();
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
 
     @Rule
     public LoggerRule logging = new LoggerRule();
 
+    @Rule
+    public ErrorCollector errors = new ErrorCollector();
+
     private static final String LOG_MESSAGE = "some problem here";
 
     @Issue("JENKINS-54890")
@@ -47,7 +57,7 @@ public void listener()
         Assert.assertTrue( "cannot find listener exception message", found > 0 );
     }
 
-    @TestExtension
+    @TestExtension("listener")
     public static class TestGraphListener
         implements GraphListener, Serializable
     {
@@ -61,4 +71,33 @@ public void onNewHead( FlowNode flowNode )
             throw new NullPointerException( LOG_MESSAGE );
         }
     }
+
+    @Test
+    public void listenersRunBeforeBuildCompletion() throws Exception {
+        var listener = ExtensionList.lookupSingleton(CheckBuildCompletionListener.class);
+        listener.errors = errors;
+        var p = r.createProject(WorkflowJob.class);
+        p.setDefinition(new CpsFlowDefinition("echo 'test'", true));
+        var b = r.buildAndAssertSuccess(p);
+        await().until(() -> listener.done);
+    }
+
+    @TestExtension("listenersRunBeforeBuildCompletion")
+    public static class CheckBuildCompletionListener implements GraphListener {
+        private ErrorCollector errors;
+        private boolean done;
+
+        @Override
+        public void onNewHead(FlowNode node) {
+            if (node instanceof FlowEndNode) {
+                try {
+                    var b = (WorkflowRun) node.getExecution().getOwner().getExecutable();
+                    errors.checkThat("Listeners should always run before build completion", b.isLogUpdated(), is(true));
+                } catch (IOException e) {
+                    errors.addError(e);
+                }
+                done = true;
+            }
+        }
+    }
 }

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java

@@ -91,6 +91,7 @@
 import org.htmlunit.WebRequest;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted;
+import org.jenkinsci.plugins.workflow.actions.TimingAction;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.TimingFlowNodeStorage;
 import org.jenkinsci.plugins.workflow.cps.GroovySourceFileAllowlist.DefaultAllowlist;
 import org.jenkinsci.plugins.workflow.flow.FlowExecution;
@@ -1007,4 +1008,18 @@ public boolean takesImplicitBlockArgument() {
         }
     }
 
+    @Test public void timingActionAlwaysAdded() throws Throwable {
+        sessions.then(r -> {
+            WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition("parallel(one: { stage('1') { echo '1' } }, two: { echo '2' })", true));
+            WorkflowRun b = r.buildAndAssertSuccess(p);
+            var nodesWithoutTiming = new DepthFirstScanner()
+                    .allNodes(b.getExecution())
+                    .stream()
+                    .filter(n -> n.getPersistentAction(TimingAction.class) == null)
+                    .toList();
+            assertThat(nodesWithoutTiming, empty());
+        });
+    }
+
 }