Always call super from WorkflowRun.onLoad

[jglick]

As of #96 and specifically 9964f77 it seems possible for WorkflowRun.onLoad to not call super. I cannot think of any justification for that. The PR description mentioned only the vague

Lazy load changes removed protection against double onLoad calls, restoring that

Whatever that might have referred to, Run.onLoad would typically be idempotent, so compared to the overridden portions of this method it seems safe to unconditionally call it. Nor do I see any compelling reason to scope this within getMetadataGuard(), which is used for private fields in WorkflowRun.

This is my only hypothesis so far as to the cause of the errors (CloudBees-internal reference) worked around by jenkinsci/junit-plugin#651 & jenkinsci/parallel-test-executor-plugin#282. I do not have access to system logs to see if Double onLoad of build … was in fact logged in these cases. I hypothesize that executionLoaded might have been set by an earlier call to getExecution though I cannot think of how that could be, since RunMap.retrieve creates the build and calls onLoad before it is made available to other threads. Perhaps LOADING_RUNS is responsible; might some background thread with access to a WorkflowRun.Owner be calling getOrNull right after reload but before onLoad?

[dwnusbaum]

Perhaps LOADING_RUNS is responsible; might some background thread with access to a WorkflowRun.Owner be calling getOrNull right after reload but before onLoad?

Maybe? It seems like it could possibly be interesting to add a special case to the "Double onLoad of build" message to log a stack trace at Level.FINEST.

[jglick]

Tried without success to reproduce:

Patch which passes consistently in several variants

diff --git src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
index 4002dba..9736bb6 100644
--- src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
+++ src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
@@ -556,7 +556,11 @@ public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements F
     }
 
     @Override protected void onLoad() {
-        super.onLoad();
+        try {
+            Thread.sleep(100);
+        } catch (InterruptedException x) {
+            x.printStackTrace();
+        }
         try {
             synchronized (getMetadataGuard()) {
                 if (executionLoaded) {
@@ -564,6 +568,8 @@ public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements F
                     return;
                 }
                 boolean needsToPersist = completed == null;
+                super.onLoad();
+
                 if (Boolean.TRUE.equals(completed) && result == null) {
                     LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for "+this);
                     setResult(Result.FAILURE);
@@ -997,7 +1003,7 @@ public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements F
                     return run.getExecution();
                 }
             } catch (Exception x) {
-                LOGGER.log(/* not important */Level.FINE, null, x);
+                LOGGER.log(/* not important */Level.INFO, null, x);
             }
             return null;
         }
diff --git src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java
index e235fde..3821baa 100644
--- src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java
+++ src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java
@@ -35,16 +35,29 @@ import static org.junit.Assert.assertTrue;
 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.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.util.Base64;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Function;
+import jenkins.model.RunAction2;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.flow.FlowDurabilityHint;
 import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionList;
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionListener;
+import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
 import org.jenkinsci.plugins.workflow.flow.GraphListener;
 import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
@@ -54,6 +67,7 @@ import org.jenkinsci.plugins.workflow.steps.Step;
 import org.jenkinsci.plugins.workflow.steps.StepContext;
 import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
 import org.jenkinsci.plugins.workflow.steps.StepExecution;
+import org.jenkinsci.plugins.workflow.steps.StepExecutions;
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import org.junit.Assume;
 import org.junit.ClassRule;
@@ -437,4 +451,70 @@ public class WorkflowRunRestartTest {
             }
         }
     }
+
+    @Test public void onLoad() throws Throwable {
+        story.then(r -> {
+            var p = r.jenkins.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition("addAction()", true));
+            var b = r.buildAndAssertSuccess(p);
+            var owner = b.asFlowExecutionOwner();
+            var baos = new ByteArrayOutputStream();
+            try (var oos = new ObjectOutputStream(baos)) {
+                oos.writeObject(owner);
+            }
+            System.err.println(b + " owner: " + Base64.getEncoder().encodeToString(baos.toByteArray()));
+        });
+        story.then(r -> {
+            var ownerSer = Base64.getDecoder().decode("rO0ABXNyADRvcmcuamVua2luc2NpLnBsdWdpbnMud29ya2Zsb3cuam9iLldvcmtmbG93UnVuJE93bmVyAAAAAAAAAAECAAJMAAJpZHQAEkxqYXZhL2xhbmcvU3RyaW5nO0wAA2pvYnEAfgABeHIANm9yZy5qZW5raW5zY2kucGx1Z2lucy53b3JrZmxvdy5mbG93LkZsb3dFeGVjdXRpb25Pd25lchjsxbvoGh3aAgAAeHB0AAExdAABcA==");
+            FlowExecutionOwner owner;
+            try (var bais = new ByteArrayInputStream(ownerSer); var ois = new ObjectInputStream(bais)) {
+                owner = (FlowExecutionOwner) ois.readObject();
+            }
+            Function<Integer, Runnable> check = n -> () -> {
+                while (true) {
+                    System.err.println(n + " Looking at " + owner);
+                    System.err.println(n + " Loaded: " + owner.getOrNull());
+                    try {
+                        System.err.println(n + " Force loaded: " + owner.get());
+                    } catch (Exception x) {
+                        x.printStackTrace();
+                    }
+                }
+            };
+            new Thread(check.apply(1)).start();
+            new Thread(check.apply(2)).start();
+            Thread.sleep(500);
+            var b = r.jenkins.getItemByFullName("p", WorkflowJob.class).getLastBuild();
+            var a = b.getAction(AnAction.class);
+            assertThat(a, notNullValue());
+            assertThat(a.r, is(b));
+        });
+    }
+    static final class AnAction extends InvisibleAction implements RunAction2 {
+        transient Run<?, ?> r;
+        @Override public void onAttached(Run<?, ?> r) {
+            this.r = r;
+        }
+        @Override public void onLoad(Run<?, ?> r) {
+            this.r = r;
+        }
+    }
+    @TestExtension("onLoad") public static final class AddActionStep extends Step {
+        @DataBoundConstructor public AddActionStep() {}
+        @Override public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.synchronous(context, _context -> {
+                _context.get(Run.class).addAction(new AnAction());
+                return null;
+            });
+        }
+        @TestExtension public static final class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "addAction";
+            }
+            @Override public Set<? extends Class<?>> getRequiredContext() {
+                return Collections.singleton(Run.class);
+            }
+        }
+    }
+
 }

[jglick]

(Deployment blocked by jenkins-infra/helpdesk#4382.)