diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index f86f6106..e1d9d352 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -190,6 +190,13 @@ public String url() { */ volatile Boolean completed; // Non-private for testing + /** + * Whether {@link #onLoad} might be needed from {@link #reload}. + * Unfortunately {@link #reload} can be called either directly or implicitly via {@link #WorkflowRun(WorkflowJob, File)} + * and this is the only way to tell which of those cases this is. + */ + private transient volatile boolean loaded; + /** * Protects access to {@link #completed} etc. * @see #getMetadataGuard @@ -245,6 +252,7 @@ private BuildListener getListener() { public WorkflowRun(WorkflowJob job) throws IOException { super(job); firstTime = true; + loaded = true; checkouts = new PersistedList<>(this); //System.err.printf("created %s @%h%n", this, this); } @@ -552,12 +560,18 @@ public boolean hasAllowKill() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { - if (Boolean.TRUE.equals(completed) && executionLoaded) { - var _execution = execution; - if (_execution != null) { - _execution.onLoad(new Owner(this)); + LOGGER.fine(() -> getExternalizableId() + " completed=" + completed + " executionLoaded=" + executionLoaded + " loaded=" + loaded); + if (Boolean.TRUE.equals(completed)) { + if (executionLoaded) { + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } } + if (loaded) { + super.onLoad(); + } // else from WorkflowRun(WorkflowJob, File), and RunMap.retrieve will call onLoad } } @@ -565,6 +579,7 @@ public boolean hasAllowKill() { super.onLoad(); try { synchronized (getMetadataGuard()) { + loaded = true; if (executionLoaded) { LOGGER.log(Level.WARNING, "Double onLoad of build " + this, new Throwable()); return; diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java index e235fde4..9b6de6b6 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java @@ -35,11 +35,17 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; import hudson.model.Executor; +import hudson.model.InvisibleAction; import hudson.model.Result; +import hudson.model.Run; import hudson.model.TaskListener; import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.logging.Level; +import jenkins.model.RunAction2; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.flow.FlowDurabilityHint; import org.jenkinsci.plugins.workflow.flow.FlowExecution; @@ -63,6 +69,7 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsSessionRule; +import org.jvnet.hudson.test.LoggerRule; import org.jvnet.hudson.test.TestExtension; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.DataBoundSetter; @@ -71,6 +78,7 @@ public class WorkflowRunRestartTest { @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); @Rule public JenkinsSessionRule story = new JenkinsSessionRule(); + @Rule public LoggerRule logging = new LoggerRule(); @Issue("JENKINS-27299") @Test public void disabled() throws Throwable { @@ -437,4 +445,46 @@ public void onNewHead(FlowNode node) { } } } + + @Test public void reloadOwnerAndActions() throws Throwable { + logging.record(WorkflowRun.class, Level.FINE); + story.then(r -> { + var p = r.jenkins.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("", true)); + var b = r.buildAndAssertSuccess(p); + var a = new A(); + b.addAction(a); + b.save(); + assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); + assertThat("attached once", a.attached, is(1)); + assertThat("not yet loaded", a.loaded, is(0)); + b.reload(); + assertThat("right owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); + a = b.getAction(A.class); + assertThat("not attached in this instance", a.attached, is(0)); + assertThat("loaded", a.loaded, is(1)); + }); + story.then(r -> { + var p = r.jenkins.getItemByFullName("p", WorkflowJob.class); + var b = p.getBuildByNumber(1); + var a = b.getAction(A.class); + assertThat("after restart, not attached in this instance", a.attached, is(0)); + assertThat("after restart, loaded", a.loaded, is(1)); + b.reload(); + assertThat("after restart, owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); + a = b.getAction(A.class); + assertThat("after restart, not attached in this instance", a.attached, is(0)); + assertThat("after restart, loaded", a.loaded, is(1)); + }); + } + private static final class A extends InvisibleAction implements RunAction2 { + transient volatile int attached, loaded; + @Override public void onAttached(Run r) { + attached++; + } + @Override public void onLoad(Run r) { + loaded++; + } + } + } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java index 86a4f407..85af8c27 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java @@ -29,7 +29,6 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; @@ -620,15 +619,6 @@ private static String checkoutString(GitSampleRepoRule repo, boolean changelog, ", userRemoteConfigs: [[url: $/" + repo.fileUrl() + "/$]]])\n"; } - @Test public void reloadOwner() throws Exception { - WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "reloadOwner"); - p.setDefinition(new CpsFlowDefinition("", true)); - WorkflowRun b = r.buildAndAssertSuccess(p); - assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); - b.reload(); - assertThat("right owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); - } - // This test is to ensure that the shortDescription on the CancelCause is escaped properly on summary.jelly @Issue("SECURITY-3042") @Test public void escapedDisplayNameAfterAbort() throws Exception {