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) {
@@ -543,8 +543,10 @@ private String key() {
                         }
                     } else {   // Execution nulled due to a critical failure, explicitly mark completed
                         completed = Boolean.TRUE;
+                        needsToPersist = true;  // Make sure we save toggled state
                     }
-                } else if (execution == null) {
+                } else if (execution == null && completed != Boolean.TRUE) {
+                    needsToPersist = true; // Make sure we save toggled state
                     completed = Boolean.TRUE;
                 }
                 if (needsToPersist && completed) {
@@ -572,6 +574,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 +596,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 +622,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 +632,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 +1128,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 +1141,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);
         }
     }
 }