Merge pull request #355 from jglick/Callback.finished

`PlaceholderTask.Callback.finished` did not correctly suppress `CancelledItemListener`, and `willContinue` was unreliable

Jenkinsfile

@@ -1,13 +1,9 @@
-/*
- See the documentation for more options:
-
-https://github.com/jenkins-infra/pipeline-library/
-
-*/
+configurations = [[platform: 'linux', jdk: 21]]
+if (env.CHANGE_ID == null) { // TODO https://github.com/jenkins-infra/helpdesk/issues/3931 workaround
+  configurations += [platform: 'windows', jdk: 17]
+}
 buildPlugin(
   useContainerAgent: true, // Set to `false` if you need to use Docker for containerized tests
   forkCount: '1C', 
-  configurations: [
-    [platform: 'linux', jdk: 21],
-    [platform: 'windows', jdk: 17],
-])
+  configurations: configurations
+)

src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java

@@ -889,7 +889,7 @@ public boolean equals(Object obj) {
         }
 
         private static void finish(StepContext context, @CheckForNull final String cookie) {
-            RunningTask runningTask = RunningTasks.get(context, t -> t, () -> null);
+            RunningTask runningTask = RunningTasks.get(context);
             if (runningTask == null) {
                 LOGGER.fine(() -> "no known running task for " + context);
                 return;
@@ -900,6 +900,7 @@ private static void finish(StepContext context, @CheckForNull final String cooki
                 return;
             }
             assert runningTask.launcher != null;
+            runningTask.execution = null;
             Timer.get().submit(() -> execution.completed(null)); // JENKINS-31614
             if (cookie == null) {
                 LOGGER.fine(() -> "no cookie to kill from " + context);
@@ -952,7 +953,7 @@ private static final class Callback extends BodyExecutionCallback.TailCall {
                     finish(execution.getContext(), cookie);
                 }
                 execution.body = null;
-                RunningTask t = RunningTasks.remove(execution.getContext());
+                RunningTask t = RunningTasks.get(execution.getContext());
                 if (t != null) {
                     LOGGER.fine(() -> "cancelling any leftover task from " + execution.getContext());
                     boolean _stopping = t.stopping;
@@ -967,6 +968,7 @@ private static final class Callback extends BodyExecutionCallback.TailCall {
                 }
                 execution.state = null;
                 bodyContext.saveState();
+                RunningTasks.remove(execution.getContext());
             }
 
         }
@@ -1239,6 +1241,10 @@ static <T> T get(StepContext context, Function<RunningTask, T> fn, Supplier<T> f
             }
         }
 
+        static @CheckForNull RunningTask get(StepContext context) {
+            return get(context, t -> t, () -> null);
+        }
+
         static void run(StepContext context, Consumer<RunningTask> fn) {
             RunningTask t = find(context);
             if (t != null) {
@@ -1248,10 +1254,12 @@ static void run(StepContext context, Consumer<RunningTask> fn) {
             }
         }
 
-        static @CheckForNull RunningTask remove(StepContext context) {
+        static void remove(StepContext context) {
             RunningTasks holder = ExtensionList.lookupSingleton(RunningTasks.class);
             synchronized (holder) {
-                return holder.runningTasks.remove(context);
+                if (holder.runningTasks.remove(context) == null) {
+                    LOGGER.fine(() -> "no RunningTask to remove associated with " + context);
+                }
             }
         }
 

src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStep2Test.java

@@ -42,7 +42,10 @@
 import java.io.File;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.logging.Level;
 import jenkins.model.Jenkins;
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.Matchers.empty;
 import org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
@@ -55,6 +58,7 @@
 import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsSessionRule;
+import org.jvnet.hudson.test.LoggerRule;
 import org.jvnet.hudson.test.SimpleCommandLauncher;
 import org.jvnet.hudson.test.TestExtension;
 
@@ -65,6 +69,7 @@ public final class ExecutorStep2Test {
 
     @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public JenkinsSessionRule rr = new JenkinsSessionRule();
+    @Rule public LoggerRule logging = new LoggerRule();
 
     @Issue("JENKINS-53837")
     @Test public void queueTaskOwnerCorrectWhenRestarting() throws Throwable {
@@ -117,8 +122,11 @@ public String getShortDescription() {
             r.jenkins.clouds.add(new TestCloud());
             var p = r.createProject(WorkflowJob.class, "p");
             p.setDefinition(new CpsFlowDefinition("node('test') {}", true));
+            logging.record(OnceRetentionStrategy.class, Level.FINE).record(ExecutorStepExecution.class, Level.FINE);
             r.assertLogContains("Running on test-1", r.buildAndAssertSuccess(p));
+            await("waiting for test-1 to be removed").until(r.jenkins::getNodes, empty());
             r.assertLogContains("Running on test-2", r.buildAndAssertSuccess(p));
+            await("waiting for test-2 to be removed").until(r.jenkins::getNodes, empty());
         });
     }
     // adapted from org.jenkinci.plugins.mock_slave.MockCloud