From bc75582e19aa1d3ed09d2229aae2464edd47911e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 Feb 2025 13:44:54 -0500 Subject: [PATCH 1/4] `WorkflowRun.reload` must call `super.onLoad` --- .../plugins/workflow/job/WorkflowRun.java | 1 + .../plugins/workflow/job/WorkflowRunTest.java | 22 +++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) 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..f4922bd4 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -557,6 +557,7 @@ public boolean hasAllowKill() { if (_execution != null) { _execution.onLoad(new Owner(this)); } + super.onLoad(); } } } 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..03e0b388 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java @@ -80,6 +80,7 @@ import jenkins.model.CauseOfInterruption; import jenkins.model.InterruptedBuildAction; import jenkins.model.Jenkins; +import jenkins.model.RunAction2; import jenkins.plugins.git.GitSampleRepoRule; import jenkins.security.QueueItemAuthenticatorConfiguration; import net.sf.json.JSONArray; @@ -620,13 +621,30 @@ 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"); + @Test public void reloadOwnerAndActions() throws Exception { + WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition("", true)); WorkflowRun 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)); + } + 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++; + } } // This test is to ensure that the shortDescription on the CancelCause is escaped properly on summary.jelly From a5fb661cffd7a1d886c3a69e1bb222851356564e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 Feb 2025 14:15:22 -0500 Subject: [PATCH 2/4] In general we may need to call `super.onLoad` even if `!executionLoaded`, so long as `completed` --- .../jenkinsci/plugins/workflow/job/WorkflowRun.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) 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 f4922bd4..9aae7419 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -552,10 +552,13 @@ 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); + if (Boolean.TRUE.equals(completed)) { + if (executionLoaded) { + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } super.onLoad(); } From 05ba74866e5be174fbc53e2a1c3215aafc738f37 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 Feb 2025 14:27:06 -0500 Subject: [PATCH 3/4] Not right; now `onLoad` is called twice after loading from disk --- .../workflow/job/WorkflowRunRestartTest.java | 45 +++++++++++++++++++ .../plugins/workflow/job/WorkflowRunTest.java | 28 ------------ 2 files changed, 45 insertions(+), 28 deletions(-) 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..6b6ec7c2 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,41 @@ 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("not attached in this instance", a.attached, is(0)); + assertThat("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 03e0b388..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; @@ -80,7 +79,6 @@ import jenkins.model.CauseOfInterruption; import jenkins.model.InterruptedBuildAction; import jenkins.model.Jenkins; -import jenkins.model.RunAction2; import jenkins.plugins.git.GitSampleRepoRule; import jenkins.security.QueueItemAuthenticatorConfiguration; import net.sf.json.JSONArray; @@ -621,32 +619,6 @@ private static String checkoutString(GitSampleRepoRule repo, boolean changelog, ", userRemoteConfigs: [[url: $/" + repo.fileUrl() + "/$]]])\n"; } - @Test public void reloadOwnerAndActions() throws Exception { - WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); - p.setDefinition(new CpsFlowDefinition("", true)); - WorkflowRun 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)); - } - 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++; - } - } - // 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 { From d63795c4a7e8e5c13662f5e30e44e6fc0d7cd17f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 Feb 2025 14:52:19 -0500 Subject: [PATCH 4/4] I think I have it now --- .../plugins/workflow/job/WorkflowRun.java | 15 +++++++++++++-- .../workflow/job/WorkflowRunRestartTest.java | 9 +++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) 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 9aae7419..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,7 +560,7 @@ 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()) { - LOGGER.fine(() -> getExternalizableId() + " completed=" + completed + " executionLoaded=" + executionLoaded); + LOGGER.fine(() -> getExternalizableId() + " completed=" + completed + " executionLoaded=" + executionLoaded + " loaded=" + loaded); if (Boolean.TRUE.equals(completed)) { if (executionLoaded) { var _execution = execution; @@ -560,8 +568,10 @@ public boolean hasAllowKill() { _execution.onLoad(new Owner(this)); } } - super.onLoad(); } + if (loaded) { + super.onLoad(); + } // else from WorkflowRun(WorkflowJob, File), and RunMap.retrieve will call onLoad } } @@ -569,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 6b6ec7c2..9b6de6b6 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java @@ -468,8 +468,13 @@ public void onNewHead(FlowNode node) { var p = r.jenkins.getItemByFullName("p", WorkflowJob.class); var b = p.getBuildByNumber(1); var a = b.getAction(A.class); - assertThat("not attached in this instance", a.attached, is(0)); - assertThat("loaded", a.loaded, is(1)); + 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 {