Store DownstreamBuildAction on Run instead of FlowNode for consistent persistence even on completed builds (#128)

* Make DownstreamBuildAction immutable to fix persistence across restarts in some cases

* Store DownstreamBuildAction on Run instead of FlowNode for consistent persistence even on completed builds

* Refactor DownstreamBuildAction to hold multiple downstream builds internally

* Change DownstreamBuildAction.downstreamBuilds from a Map to a List

Author: dwnusbaum

pom.xml

@@ -47,7 +47,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.387.x</artifactId>
-                <version>2143.ve4c3c9ec790a</version>
+                <version>2543.vfb_1a_5fb_9496d</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>
@@ -90,16 +90,12 @@
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-cps</artifactId>
             <scope>test</scope>
-            <!-- TODO: Remove version declaration when workflow-cps plugin 3691.v28b_14c465a_b_b_ or newer is in plugin BOM -->
-            <version>3691.v28b_14c465a_b_b_</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-cps</artifactId>
             <classifier>tests</classifier>
             <scope>test</scope>
-            <!-- TODO: Remove version declaration when workflow-cps plugin 3691.v28b_14c465a_b_b_ or newer is in plugin BOM -->
-            <version>3691.v28b_14c465a_b_b_</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
@@ -151,8 +147,6 @@
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>git</artifactId>
             <scope>test</scope>
-            <!-- TODO: Remove version declaration when git plugin 5.1.0 or newer is in plugin BOM -->
-            <version>5.1.0</version>
         </dependency>
         <dependency>
             <groupId>org.awaitility</groupId>

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java

@@ -13,7 +13,6 @@
 import java.util.logging.Logger;
 import jenkins.util.Timer;
 import org.jenkinsci.plugins.workflow.actions.WarningAction;
-import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
@@ -88,32 +87,16 @@ public void onDeleted(final Run<?,?> run) {
         }
     }
 
-    private void updateDownstreamBuildAction(Run<?, ?> run) {
-        for (Cause cause : run.getCauses()) {
+    private void updateDownstreamBuildAction(Run<?, ?> downstream) {
+        for (Cause cause : downstream.getCauses()) {
             if (cause instanceof BuildUpstreamCause) {
                 BuildUpstreamCause buildUpstreamCause = (BuildUpstreamCause) cause;
                 Run<?, ?> upstream = buildUpstreamCause.getUpstreamRun();
                 if (upstream instanceof FlowExecutionOwner.Executable) {
-                    FlowExecutionOwner owner = ((FlowExecutionOwner.Executable) upstream).asFlowExecutionOwner();
-                    if (owner == null) {
-                        LOGGER.log(Level.FINE, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
-                        continue;
-                    }
+                    String flowNodeId = buildUpstreamCause.getNodeId();
+                    DownstreamBuildAction.getOrCreate(upstream, flowNodeId, downstream.getParent()).setBuild(downstream);
                     try {
-                        FlowExecution execution = owner.get();
-                        FlowNode node = execution.getNode(buildUpstreamCause.getNodeId());
-                        if (node == null) {
-                            LOGGER.log(Level.FINE, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
-                            continue;
-                        }
-                        DownstreamBuildAction downstreamAction = node.getPersistentAction(DownstreamBuildAction.class);
-                        if (downstreamAction == null) {
-                            // Should only happen for builds already in the queue when this plugin is updated to include DownstreamBuildAction.
-                            downstreamAction = new DownstreamBuildAction(run.getParent());
-                            node.addAction(downstreamAction);
-                        }
-                        downstreamAction.setBuild(run);
-                        run.save();
+                        upstream.save();
                     } catch (IOException e) {
                         LOGGER.log(Level.FINE, e, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
                     }

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepExecution.java

@@ -66,7 +66,8 @@ public BuildTriggerStepExecution(BuildTriggerStep step, @NonNull StepContext con
     @Override
     public boolean start() throws Exception {
         String job = step.getJob();
-        Item item = Jenkins.get().getItem(job, getContext().get(Run.class).getParent(), Item.class);
+        Run<?, ?> upstream = getContext().get(Run.class);
+        Item item = Jenkins.get().getItem(job, upstream.getParent(), Item.class);
         if (item == null) {
             throw new AbortException("No item named " + job + " found");
         }
@@ -75,12 +76,13 @@ public boolean start() throws Exception {
             // TODO find some way of allowing ComputedFolders to hook into the listener code
             throw new AbortException("Waiting for non-job items is not supported");
         }
+
         FlowNode node = getContext().get(FlowNode.class);
-        node.addAction(new DownstreamBuildAction(item));
+        DownstreamBuildAction.getOrCreate(upstream, node.getId(), item);
 
         List<Action> actions = new ArrayList<>();
-        actions.add(new CauseAction(new BuildUpstreamCause(getContext().get(FlowNode.class), getContext().get(Run.class))));
-        actions.add(new BuildUpstreamNodeAction(node, getContext().get(Run.class)));
+        actions.add(new CauseAction(new BuildUpstreamCause(getContext().get(FlowNode.class), upstream)));
+        actions.add(new BuildUpstreamNodeAction(node, upstream));
 
         if (item instanceof ParameterizedJobMixIn.ParameterizedJob) {
             final ParameterizedJobMixIn.ParameterizedJob project = (ParameterizedJobMixIn.ParameterizedJob) item;

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/DownstreamBuildAction.java

@@ -6,47 +6,87 @@
 import hudson.model.Item;
 import hudson.model.ItemGroup;
 import hudson.model.Run;
-import org.jenkinsci.plugins.workflow.actions.PersistentAction;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.springframework.security.access.AccessDeniedException;
 
 /**
- * Tracks the downstream build triggered by the {@code build} step.
+ * Tracks downstream builds triggered by the {@code build} step, as well as the {@link FlowNode#getId} of the step.
  *
  * @see BuildUpstreamCause
  */
-public final class DownstreamBuildAction extends InvisibleAction implements PersistentAction {
-    private final String jobFullName;
-    private Integer buildNumber;
+public final class DownstreamBuildAction extends InvisibleAction {
+    private final List<DownstreamBuild> downstreamBuilds = new ArrayList<>();
 
-    DownstreamBuildAction(Item job) {
-        this.jobFullName = job.getFullName();
+    public static @NonNull DownstreamBuild getOrCreate(@NonNull Run<?, ?> run, @NonNull String flowNodeId, @NonNull Item job) {
+        DownstreamBuildAction downstreamBuildAction;
+        synchronized (DownstreamBuildAction.class) {
+            downstreamBuildAction = run.getAction(DownstreamBuildAction.class);
+            if (downstreamBuildAction == null) {
+                downstreamBuildAction = new DownstreamBuildAction();
+                run.addAction(downstreamBuildAction);
+            }
+        }
+        return downstreamBuildAction.getOrAddDownstreamBuild(flowNodeId, job);
     }
 
-    public @NonNull String getJobFullName() {
-        return jobFullName;
+    public synchronized @NonNull List<DownstreamBuild> getDownstreamBuilds() {
+        return Collections.unmodifiableList(new ArrayList<>(downstreamBuilds));
     }
 
-    /**
-     * Get the build number of the downstream build, or {@code null} if the downstream build has not yet started or the queue item was cancelled.
-     */
-    public @CheckForNull Integer getBuildNumber() {
-        return buildNumber;
+    private synchronized @NonNull DownstreamBuild getOrAddDownstreamBuild(@NonNull String flowNodeId, @NonNull Item job) {
+        for (DownstreamBuild build : downstreamBuilds) {
+            if (build.getFlowNodeId().equals(flowNodeId)) {
+                return build;
+            }
+        }
+        var build = new DownstreamBuild(flowNodeId, job);
+        downstreamBuilds.add(build);
+        return build;
     }
 
-    /**
-     * Load the downstream build, if it has started and still exists.
-     * <p>Loading builds indiscriminately will affect controller performance, so use this carefully. If you only need
-     * to know whether the build started at one point, use {@link #getBuildNumber}.
-     * @throws AccessDeniedException as per {@link ItemGroup#getItem}
-     */
-    public @CheckForNull Run<?, ?> getBuild() throws AccessDeniedException {
-        if (buildNumber == null) {
-            return null;
-        }
-        return Run.fromExternalizableId(jobFullName + '#' + buildNumber);
-    }
+    public static final class DownstreamBuild {
+        private final String flowNodeId;
+        private final String jobFullName;
+        private Integer buildNumber;
+
+        DownstreamBuild(String flowNodeId, @NonNull Item job) {
+            this.flowNodeId = flowNodeId;
+            this.jobFullName = job.getFullName();
+        }
+
+        public @NonNull String getFlowNodeId() {
+            return flowNodeId;
+        }
 
-    void setBuild(Run<?, ?> build) {
-        this.buildNumber = build.getNumber();
+        public @NonNull String getJobFullName() {
+            return jobFullName;
+        }
+
+        /**
+         * Get the build number of the downstream build, or {@code null} if the downstream build has not yet started or the queue item was cancelled.
+         */
+        public @CheckForNull Integer getBuildNumber() {
+            return buildNumber;
+        }
+
+        /**
+         * Load the downstream build, if it has started and still exists.
+         * <p>Loading builds indiscriminately will affect controller performance, so use this carefully. If you only need
+         * to know whether the build started at one point, use {@link #getBuildNumber}.
+         * @throws AccessDeniedException as per {@link ItemGroup#getItem}
+         */
+        public @CheckForNull Run<?, ?> getBuild() throws AccessDeniedException {
+            if (buildNumber == null) {
+                return null;
+            }
+            return Run.fromExternalizableId(jobFullName + '#' + buildNumber);
+        }
+
+        void setBuild(@NonNull Run<?, ?> run) {
+            this.buildNumber = run.getNumber();
+        }
     }
 }

src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java

@@ -2,25 +2,37 @@
 
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
+import hudson.model.Label;
 import hudson.model.Queue;
 import hudson.model.Result;
+import hudson.model.Run;
 import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
-import org.junit.Assert;
+import org.jenkinsci.plugins.workflow.support.steps.build.DownstreamBuildAction.DownstreamBuild;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.JenkinsSessionRule;
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 
 /**
  * @author Vivek Pandey
  */
-public class BuildTriggerStepRestartTest extends Assert {
+public class BuildTriggerStepRestartTest {
 
     @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public JenkinsSessionRule sessions = new JenkinsSessionRule();
@@ -64,6 +76,79 @@ public void restartBetweenJobs() throws Throwable {
         sessions.then(j -> j.jenkins.getItemByFullName("test1", FreeStyleProject.class).getBuildByNumber(1).delete());
     }
 
+    @Test
+    public void downstreamBuildActionUpstreamCompletesBeforeDownstreamStarts() throws Throwable {
+        sessions.then(j -> {
+            FreeStyleProject downstream = j.createFreeStyleProject("downstream");
+            downstream.setAssignedLabel(Label.parseExpression("agent"));
+            WorkflowJob upstream = j.jenkins.createProject(WorkflowJob.class, "upstream");
+            upstream.setDefinition(new CpsFlowDefinition("build job: 'downstream', wait: false", true));
+            WorkflowRun upstreamRun = j.buildAndAssertSuccess(upstream);
+            // Check action while the build is still in the queue.
+            String buildStepId = BuildTriggerStepTest.findFirstNodeWithDescriptor(upstreamRun.getExecution(), BuildTriggerStep.DescriptorImpl.class).getId();
+            var downstreamBuildAction = upstreamRun.getAction(DownstreamBuildAction.class);
+            var downstreamBuild = downstreamBuildAction.getDownstreamBuilds().get(0);
+            assertThat(downstreamBuild.getFlowNodeId(), equalTo(buildStepId));
+            assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
+            assertThat(downstreamBuild.getBuildNumber(), nullValue());
+            assertThat(downstreamBuild.getBuild(), nullValue());
+            // Check again once the build has started.
+            j.createOnlineSlave(Label.parseExpression("agent"));
+            await().atMost(10, TimeUnit.SECONDS).until(() -> downstreamBuild.getBuildNumber(), notNullValue());
+            Run<?, ?> downstreamRun = downstream.getLastBuild();
+            assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
+            assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
+            assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
+            j.waitForCompletion(downstreamRun);
+        });
+        sessions.then(j -> {
+            FreeStyleProject downstream = j.jenkins.getItemByFullName("downstream", FreeStyleProject.class);
+            WorkflowJob upstream = j.jenkins.getItemByFullName("upstream", WorkflowJob.class);
+            WorkflowRun upstreamRun = upstream.getLastBuild();
+            String buildStepId = BuildTriggerStepTest.findFirstNodeWithDescriptor(upstreamRun.getExecution(), BuildTriggerStep.DescriptorImpl.class).getId();
+            var downstreamBuildAction = upstreamRun.getAction(DownstreamBuildAction.class);
+            var downstreamBuild = downstreamBuildAction.getDownstreamBuilds().get(0);
+            assertThat(downstreamBuild.getFlowNodeId(), equalTo(buildStepId));
+            assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
+            assertThat(downstreamBuild.getBuildNumber(), equalTo(downstream.getLastBuild().getNumber()));
+            assertThat(downstreamBuild.getBuild(), equalTo(downstream.getLastBuild()));
+        });
+    }
+
+    @Test
+    public void downstreamBuildActionMultipleBuilds() throws Throwable {
+        sessions.then(j -> {
+            FreeStyleProject downstream = j.createFreeStyleProject("downstream");
+            WorkflowJob upstream = j.jenkins.createProject(WorkflowJob.class, "upstream");
+            upstream.setDefinition(new CpsFlowDefinition("build 'downstream'; build 'downstream'", true));
+            WorkflowRun upstreamRun = j.buildAndAssertSuccess(upstream);
+            List<DownstreamBuild> downstreamBuilds = upstreamRun.getAction(DownstreamBuildAction.class).getDownstreamBuilds();
+            await().atMost(10, TimeUnit.SECONDS).until(
+                    () -> downstreamBuilds.stream().map(DownstreamBuild::getBuildNumber).filter(Objects::nonNull).count(),
+                    equalTo(2L));
+            for (Run<?, ?> downstreamRun : downstream.getBuilds()) {
+                String nodeId = downstreamRun.getCause(BuildUpstreamCause.class).getNodeId();
+                var downstreamBuild = downstreamBuilds.stream().filter(db -> db.getFlowNodeId().equals(nodeId)).findFirst().get();
+                assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
+                assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
+                assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
+            }
+        });
+        sessions.then(j -> {
+            FreeStyleProject downstream = j.jenkins.getItemByFullName("downstream", FreeStyleProject.class);
+            WorkflowJob upstream = j.jenkins.getItemByFullName("upstream", WorkflowJob.class);
+            WorkflowRun upstreamRun = upstream.getLastBuild();
+            List<DownstreamBuild> downstreamBuilds = upstreamRun.getAction(DownstreamBuildAction.class).getDownstreamBuilds();
+            for (Run<?, ?> downstreamRun : downstream.getBuilds()) {
+                String nodeId = downstreamRun.getCause(BuildUpstreamCause.class).getNodeId();
+                var downstreamBuild = downstreamBuilds.stream().filter(db -> db.getFlowNodeId().equals(nodeId)).findFirst().get();
+                assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
+                assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
+                assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
+            }
+        });
+    }
+
     private static void assertFreeStyleProjectsInQueue(int count, JenkinsRule j) {
         Queue.Item[] items = j.jenkins.getQueue().getItems();
         int actual = 0;