Address how and where we end the AsynchronousExecution

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -181,7 +181,7 @@ public String url() {
      * Flag for whether or not the build has completed somehow.
      * This was previously a transient field, so we may need to recompute in {@link #onLoad} based on {@link FlowExecution#isComplete}.
      */
-    Boolean completed;  // Non-private for testing
+    volatile Boolean completed;  // Non-private for testing
 
     /** Protects the access to logsToCopy, completed, and branchNameCache that are used in the logCopy process */
     private transient Object logCopyGuard = new Object();
@@ -508,7 +508,7 @@ private String key() {
         new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this);
     }
 
-    @Override protected void onLoad() {
+    @Override protected void onLoad() {  // Here there be DRAGONS: due to lazy-loading and subtle threading risks, be very cautious!
         try {
             synchronized (getLogCopyGuard()) {
                 if (executionLoaded) {
@@ -572,6 +572,20 @@ private String key() {
         }
     }
 
+    private void endExecutionTask() {
+        try {
+            Executor executor = getExecutor();
+            if (executor != null) {
+                AsynchronousExecution asynchronousExecution = executor.getAsynchronousExecution();
+                if (asynchronousExecution != null) {
+                    asynchronousExecution.completed(null);
+                }
+            }
+        } catch (Exception ex) {
+            LOGGER.log(Level.WARNING, "Error when trying to end the FlyWeightTask for run "+this, ex);
+        }
+    }
+
     /** Handles normal build completion (including errors) but also handles the case that the flow did not even start correctly, for example due to an error in {@link FlowExecution#start}. */
     private void finish(@Nonnull Result r, @CheckForNull Throwable t) {
         boolean nullListener = false;
@@ -580,6 +594,7 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) {
             setResult(r);
             completed = Boolean.TRUE;
             duration = Math.max(0, System.currentTimeMillis() - getStartTimeInMillis());
+
         }
         try {
             LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[]{toString(), getResult()});
@@ -605,7 +620,8 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) {
                 }
                 listener = null;
             }
-            saveWithoutFailing(); // TODO useless if we are inside a BulkChange
+            saveWithoutFailing();
+            endExecutionTask();
             Timer.get().submit(() -> {
                 try {
                     getParent().logRotate();
@@ -614,7 +630,8 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) {
                 }
             });
             onEndBuilding();
-        } finally {  // Ensure this is ALWAYS removed from FlowExecutionList
+        } finally {  // Ensure this is ALWAYS removed from FlowExecutionList and finished
+            endExecutionTask();  // Just in case an exception was thrown above
             FlowExecutionList.get().unregister(new Owner(this));
         }
 
@@ -1109,8 +1126,9 @@ private void saveWithoutFailing() {
 
     @Override
     public void save() throws IOException {
-
-        if(BulkChange.contains(this))   return;
+        /* Checking for completion ensures the final save can complete.
+         */
+        if(BulkChange.contains(this) && completed != Boolean.TRUE)   return;
         File loc = new File(getRootDir(),"build.xml");
         XmlFile file = new XmlFile(XSTREAM,loc);
 
@@ -1121,23 +1139,9 @@ public void save() throws IOException {
             isAtomic = hint.isAtomicWrite();
         }
 
-        boolean completeAsynchronousExecution = false;
-        try {
-            synchronized (this) {
-                completeAsynchronousExecution = Boolean.TRUE.equals(completed);
-                PipelineIOUtils.writeByXStream(this, loc, XSTREAM2, isAtomic);
-                SaveableListener.fireOnChange(this, file);
-            }
-        } finally {
-            if (completeAsynchronousExecution) {
-                Executor executor = getExecutor();
-                if (executor != null) {
-                    AsynchronousExecution asynchronousExecution = executor.getAsynchronousExecution();
-                    if (asynchronousExecution != null) {
-                        asynchronousExecution.completed(null);
-                    }
-                }
-            }
+        synchronized (this) {
+            PipelineIOUtils.writeByXStream(this, loc, XSTREAM2, isAtomic);
+            SaveableListener.fireOnChange(this, file);
         }
     }
 }

src/test/java/org/jenkinsci/plugins/workflow/job/CpsPersistenceTest.java

@@ -79,9 +79,8 @@ private static Stack<BlockStartNode> getCpsBlockStartNodes(CpsFlowExecution exec
 
     /** Verifies all the assumptions about a cleanly finished build. */
     static void assertCompletedCleanly(WorkflowRun run) throws Exception {
-        if (run.isBuilding()) {
-            System.out.println("Run initially building, going to wait a second to see if it finishes, run="+run);
-            Thread.sleep(1000);
+        while (run.isBuilding()) {
+            Thread.sleep(100); // Somewhat variable speeds
         }
         Assert.assertFalse(run.isBuilding());
         Assert.assertNotNull(run.getResult());
@@ -103,7 +102,9 @@ static void assertCompletedCleanly(WorkflowRun run) throws Exception {
             Stack<BlockStartNode> starts = getCpsBlockStartNodes(cpsExec);
             Assert.assertTrue(starts == null || starts.isEmpty());
             Thread.sleep(1000); // TODO seems to be flaky
-            Assert.assertFalse(cpsExec.blocksRestart());
+            while (cpsExec.blocksRestart()) {
+                Thread.sleep(100);
+            }
         } else {
             System.out.println("WARNING: no FlowExecutionForBuild");
         }

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java

@@ -129,7 +129,6 @@ public class WorkflowRunRestartTest {
             WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class);
             WorkflowRun b = p.getBuildByNumber(1);
             assertNotNull(b.asFlowExecutionOwner());
-            assertNull(b.execution.getOwner());
             assertFalse(b.executionLoaded);
             assertTrue(b.completed);
             assertFalse(b.isBuilding());