[JENKINS-75067] Unexport step bodies when completed in synchronous mode

Previously, the unexport of a step's body would only happen when
scheduling the next execution (async mode).  If the step was not run
async (due to having an outcome set prior to the step starting), the
body was not removed until the CpsThreadGroup was finishing.  At this
point the body's presence was treated as a resource leak and logged a
warning.  Doing an explicit unexport for the sync case at the proper
time eliminates the warnings from the fall-back cleanup.

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java

@@ -359,6 +359,12 @@ private static boolean refersTo(Throwable t1, Throwable t2) {
      */
     private void scheduleNextRun() {
         if (syncMode) {
+            // probably rare for a legit sync step to have a body, but it's possible for a (typically) async step
+            // to be *treated* as sync due to having an outcome set prematurely (e.g. from a StepListener)
+            if (threadGroup != null && body != null) {
+                threadGroup.unexport(body);
+                body = null;
+            }
             // if we get the result set before the start method returned, then DSL.invokeMethod() will
             // plan the next action.
             return;

plugin/src/test/java/org/jenkinsci/plugins/workflow/StepListenerTest.java

@@ -24,29 +24,49 @@
 
 package org.jenkinsci.plugins.workflow;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.AbortException;
+import hudson.ExtensionList;
+import hudson.model.Result;
 import hudson.model.TaskListener;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
+import org.jenkinsci.plugins.workflow.cps.CpsThreadGroup;
+import org.jenkinsci.plugins.workflow.flow.GraphListener;
 import org.jenkinsci.plugins.workflow.flow.StepListener;
+import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
+import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.steps.Step;
 import org.jenkinsci.plugins.workflow.steps.StepContext;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.LoggerRule;
 import org.jvnet.hudson.test.TestExtension;
 
-import edu.umd.cs.findbugs.annotations.NonNull;
+import java.util.logging.Level;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
 
 public class StepListenerTest {
-    @ClassRule
-    public static JenkinsRule r = new JenkinsRule();
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
 
     @ClassRule
     public static BuildWatcher buildWatcher = new BuildWatcher();
 
+    @Rule
+    public LoggerRule logger = new LoggerRule();
+
     @Issue("JENKINS-58084")
     @Test
     public void listener() throws Exception {
@@ -59,7 +79,7 @@ public void listener() throws Exception {
         r.assertLogContains("Step listener saw step echo", build);
     }
 
-    @TestExtension
+    @TestExtension("listener")
     public static class TestStepListener implements StepListener {
         @Override
         public void notifyOfNewStep(@NonNull Step s, @NonNull StepContext context) {
@@ -74,4 +94,48 @@ public void notifyOfNewStep(@NonNull Step s, @NonNull StepContext context) {
             }
         }
     }
+
+    @Issue("JENKINS-75067")
+    @Test
+    public void failingListener() throws Exception {
+        // Even before the fix there's only one warning logged.  Asserting zero records is probably over-stepping,
+        // but asserting just one record with our target message risks a false negative (some other unrelated message
+        // being first, and our being later).
+        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
+        WorkflowJob job = r.createProject(WorkflowJob.class, "failingListener");
+        job.setDefinition(new CpsFlowDefinition("node {}\n", true));
+
+        WorkflowRun build = r.buildAndAssertStatus(Result.FAILURE, job);
+        r.assertLogContains("oops", build);
+        assertThat(TestFailingStepListener.get().closureCount, equalTo(0));
+        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
+    }
+
+    @TestExtension("failingListener")
+    public static class TestFailingStepListener implements StepListener, GraphListener.Synchronous {
+        int closureCount = -1;
+
+        @Override
+        public void notifyOfNewStep(@NonNull Step s, @NonNull StepContext context) {
+            context.onFailure(new AbortException("oops"));
+        }
+
+        @Override
+        public void onNewHead(FlowNode node) {
+            // this only works using a Synchronous listener, otherwise the fall-back closure cleaning
+            // will have already executed prior to receiving this event
+            if (node instanceof FlowEndNode) {
+              try {
+                closureCount = ((CpsFlowExecution) node.getExecution()).programPromise.get().closures.size();
+              }
+              catch (Exception e) {
+                throw new RuntimeException(e);
+              }
+            }
+        }
+
+        static TestFailingStepListener get() {
+            return ExtensionList.lookupSingleton(TestFailingStepListener.class);
+        }
+    }
 }