From e6e6484951d42ede297682e0fb834405ee2bb027 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 30 Jun 2025 17:29:04 -0400 Subject: [PATCH 01/10] Run local listeners after listeners from extensions --- .../jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index dc2d65b50..3459c04d6 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1523,10 +1523,14 @@ private static void cleanUpClassHelperCache(@NonNull Class clazz) throws Exce List getListenersToRun() { List l = new ArrayList<>(); + // TODO: Really, we need some kind of `GraphListener.ordinal` method that allows local listeners to be sorted + // into the overall listener list while also respection `Extension.ordinal`. Then we'd need to split + // `WorkflowRun.GraphL` in 2 and make the part that adds `TimingAction` high priority, but the part that calls + // `WorkflowRun.finish` low priority. + l.addAll(ExtensionList.lookup(GraphListener.class)); if (listeners != null) { l.addAll(listeners); } - l.addAll(ExtensionList.lookup(GraphListener.class)); return l; } From c66bb9d3fc83ca556bcfc43983ca8f8333757c8e Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 1 Jul 2025 16:35:30 -0400 Subject: [PATCH 02/10] Notify listeners in order according to GraphListener.ordinal --- plugin/pom.xml | 6 ++++++ .../jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 9 +++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/plugin/pom.xml b/plugin/pom.xml index 38c409ad9..d4bef64ff 100644 --- a/plugin/pom.xml +++ b/plugin/pom.xml @@ -57,6 +57,12 @@ import pom + + + org.jenkins-ci.plugins.workflow + workflow-api + 1377.v6fdf65c6195c + diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 3459c04d6..387ca893b 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -144,6 +144,7 @@ import net.jcip.annotations.GuardedBy; import java.nio.charset.StandardCharsets; +import java.util.Comparator; import jenkins.util.SystemProperties; import org.codehaus.groovy.GroovyBugError; import org.jboss.marshalling.reflect.SerializableClassRegistry; @@ -1523,15 +1524,11 @@ private static void cleanUpClassHelperCache(@NonNull Class clazz) throws Exce List getListenersToRun() { List l = new ArrayList<>(); - // TODO: Really, we need some kind of `GraphListener.ordinal` method that allows local listeners to be sorted - // into the overall listener list while also respection `Extension.ordinal`. Then we'd need to split - // `WorkflowRun.GraphL` in 2 and make the part that adds `TimingAction` high priority, but the part that calls - // `WorkflowRun.finish` low priority. - l.addAll(ExtensionList.lookup(GraphListener.class)); if (listeners != null) { l.addAll(listeners); } - + l.addAll(ExtensionList.lookup(GraphListener.class)); + l.sort(Comparator.comparing(GraphListener::ordinal).reversed()); return l; } From 7ea217855aa487aa31b8e3d956260c53a72c92ab Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 1 Jul 2025 16:44:20 -0400 Subject: [PATCH 03/10] Link to upstream PR --- plugin/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/pom.xml b/plugin/pom.xml index d4bef64ff..242cb3063 100644 --- a/plugin/pom.xml +++ b/plugin/pom.xml @@ -58,7 +58,7 @@ pom - + org.jenkins-ci.plugins.workflow workflow-api 1377.v6fdf65c6195c From 4291e87d2eb0b8ea6d443749d2a1c49b10666cbe Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 1 Jul 2025 16:51:31 -0400 Subject: [PATCH 04/10] Update baseline and BOM as needed for workflow-api update --- plugin/pom.xml | 3 ++- pom.xml | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/plugin/pom.xml b/plugin/pom.xml index 242cb3063..9a07511d1 100644 --- a/plugin/pom.xml +++ b/plugin/pom.xml @@ -53,12 +53,13 @@ io.jenkins.tools.bom bom-${jenkins.baseline}.x - 4488.v7fe26526366e + 4710.v016f0a_07e34d import pom + org.jenkins-ci.plugins.workflow workflow-api 1377.v6fdf65c6195c diff --git a/pom.xml b/pom.xml index 240352642..25c509d9d 100644 --- a/pom.xml +++ b/pom.xml @@ -42,8 +42,8 @@ 999999-SNAPSHOT jenkinsci/${project.artifactId}-plugin - 2.479 - ${jenkins.baseline}.3 + 2.504 + ${jenkins.baseline}.1 2.4.21 From 42419079a10571f28e5387c546f00c7aa8c9892b Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 2 Jul 2025 16:04:52 -0400 Subject: [PATCH 05/10] Simplify changes by handling TimingAction for FlowStartNode directly in FlowHead --- plugin/pom.xml | 9 ++-- .../workflow/cps/CpsFlowExecution.java | 12 +++-- .../plugins/workflow/cps/FlowHead.java | 1 + .../plugins/workflow/GraphListenerTest.java | 50 +++++++++++++++++-- .../workflow/cps/CpsFlowExecutionTest.java | 15 ++++++ pom.xml | 4 +- 6 files changed, 77 insertions(+), 14 deletions(-) diff --git a/plugin/pom.xml b/plugin/pom.xml index 9a07511d1..0d7cd96a1 100644 --- a/plugin/pom.xml +++ b/plugin/pom.xml @@ -53,16 +53,15 @@ io.jenkins.tools.bom bom-${jenkins.baseline}.x - 4710.v016f0a_07e34d + 4488.v7fe26526366e import pom - - + org.jenkins-ci.plugins.workflow - workflow-api - 1377.v6fdf65c6195c + workflow-job + 999999-SNAPSHOT diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 387ca893b..796544e39 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/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 listeners; /** @@ -1224,7 +1227,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) { @@ -1524,11 +1527,14 @@ private static void cleanUpClassHelperCache(@NonNull Class clazz) throws Exce List getListenersToRun() { List 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. + l.addAll(ExtensionList.lookup(GraphListener.class)); if (listeners != null) { l.addAll(listeners); } - l.addAll(ExtensionList.lookup(GraphListener.class)); - l.sort(Comparator.comparing(GraphListener::ordinal).reversed()); + return l; } diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java index db4d2026b..3e0e00a4d 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java +++ b/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); } diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java index e2a1bf65a..50e9d847b 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java @@ -1,13 +1,18 @@ package org.jenkinsci.plugins.workflow; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.lessThanOrEqualTo; + +import hudson.ExtensionList; import hudson.model.Run; +import java.io.IOException; 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.FlowNode; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.junit.Assert; -import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; @@ -15,19 +20,27 @@ import org.jvnet.hudson.test.TestExtension; import java.io.Serializable; +import java.time.Instant; import java.util.List; import java.util.Random; import java.util.logging.Level; +import org.jenkinsci.plugins.workflow.graph.FlowEndNode; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.rules.ErrorCollector; import org.jvnet.hudson.test.Issue; +import static org.awaitility.Awaitility.await; 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 +60,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 +74,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; + } + } + } } diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java index 9a9ef4baf..3a5132258 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java @@ -88,6 +88,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; @@ -963,4 +964,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()); + }); + } + } diff --git a/pom.xml b/pom.xml index 25c509d9d..240352642 100644 --- a/pom.xml +++ b/pom.xml @@ -42,8 +42,8 @@ 999999-SNAPSHOT jenkinsci/${project.artifactId}-plugin - 2.504 - ${jenkins.baseline}.1 + 2.479 + ${jenkins.baseline}.3 2.4.21 From dc0ef616008fc1dc1ee1af9b4682579ca28d5a02 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 2 Jul 2025 16:17:53 -0400 Subject: [PATCH 06/10] Remove SNAPSHOT dependency on workflow-job, which was only used for testing --- plugin/pom.xml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/plugin/pom.xml b/plugin/pom.xml index 0d7cd96a1..38c409ad9 100644 --- a/plugin/pom.xml +++ b/plugin/pom.xml @@ -57,12 +57,6 @@ import pom - - - org.jenkins-ci.plugins.workflow - workflow-job - 999999-SNAPSHOT - From 11321e4af4ca96022ebe66dcb9bf3e39597d5fde Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 3 Jul 2025 10:35:27 -0400 Subject: [PATCH 07/10] Remove unused imports --- .../java/org/jenkinsci/plugins/workflow/GraphListenerTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java index 50e9d847b..07bee32df 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java @@ -1,8 +1,6 @@ package org.jenkinsci.plugins.workflow; -import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.lessThanOrEqualTo; import hudson.ExtensionList; import hudson.model.Run; @@ -20,7 +18,6 @@ import org.jvnet.hudson.test.TestExtension; import java.io.Serializable; -import java.time.Instant; import java.util.List; import java.util.Random; import java.util.logging.Level; From c518cf8231ae627ccdbe5e280f69119f3b6deecb Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 3 Jul 2025 10:37:05 -0400 Subject: [PATCH 08/10] Adjust new import ordering to minimize diff --- .../jenkinsci/plugins/workflow/GraphListenerTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java index 07bee32df..b72ddb711 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java @@ -1,31 +1,31 @@ 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 java.io.IOException; 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.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.LoggerRule; import org.jvnet.hudson.test.TestExtension; +import org.junit.rules.ErrorCollector; +import java.io.IOException; import java.io.Serializable; import java.util.List; import java.util.Random; import java.util.logging.Level; -import org.jenkinsci.plugins.workflow.graph.FlowEndNode; -import org.jenkinsci.plugins.workflow.job.WorkflowRun; -import org.junit.rules.ErrorCollector; import org.jvnet.hudson.test.Issue; -import static org.awaitility.Awaitility.await; public class GraphListenerTest { From 2f7ceece7284f20c4cdc8f927813d0266fd03f2a Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 3 Jul 2025 10:38:28 -0400 Subject: [PATCH 09/10] Remove another unused import and improve import ordering --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 1 - .../java/org/jenkinsci/plugins/workflow/GraphListenerTest.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 3cd804048..d5e04e0e0 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -144,7 +144,6 @@ import net.jcip.annotations.GuardedBy; import java.nio.charset.StandardCharsets; -import java.util.Comparator; import jenkins.util.SystemProperties; import org.codehaus.groovy.GroovyBugError; import org.jboss.marshalling.reflect.SerializableClassRegistry; diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java index b72ddb711..7a12bff11 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java @@ -15,10 +15,10 @@ import org.junit.Assert; 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 org.junit.rules.ErrorCollector; import java.io.IOException; import java.io.Serializable; From 49661efbdd989ba2d777275fbfd68dfc5dd2b14f Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 18 Jul 2025 11:20:32 -0400 Subject: [PATCH 10/10] Simplify based on review feedback Co-authored-by: Jesse Glick --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index d5e04e0e0..a3d68b1eb 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1529,12 +1529,10 @@ private static void cleanUpClassHelperCache(@NonNull Class clazz) throws Exce } List getListenersToRun() { - List 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. - l.addAll(ExtensionList.lookup(GraphListener.class)); + List l = new ArrayList<>(ExtensionList.lookup(GraphListener.class)); if (listeners != null) { l.addAll(listeners); }