Merge pull request #252 from jglick/JENKINS-25889

[JENKINS-25889] InputStepExecution.node null after restart
jenkinsci · Nov 19, 2015 · e175d3c · e175d3c
2 parents 00e5108 + c39ba56
commit e175d3c
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 28 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -2,6 +2,10 @@
 
 Only noting significant user changes, not internal code cleanups and minor bug fixes.
 
+## 1.12 (upcoming)
+
+* [JENKINS-25889](https://issues.jenkins-ci.org/browse/JENKINS-25889): error when submitting to an `input` step after a Jenkins restart.
+
 ## 1.11 (Nov 12 2015)
 
 * _Workflow: Multibranch_ plugin now released as nonbeta and available from the regular update center. (Currently not included in _Workflow: Aggregator_.)

diff --git a/...egator/src/test/java/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest.java b/...egator/src/test/java/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest.java
@@ -24,43 +24,78 @@
 
 package org.jenkinsci.plugins.workflow.steps.input;
 
-import com.google.common.base.Function;
-import java.io.IOException;
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.io.FileUtils;
 import org.jenkinsci.plugins.workflow.SingleJobTestBase;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.graph.FlowGraphWalker;
+import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
-import org.jenkinsci.plugins.workflow.steps.StepExecution;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+import org.jenkinsci.plugins.workflow.support.actions.PauseAction;
+import org.jenkinsci.plugins.workflow.support.steps.input.InputAction;
 import org.jenkinsci.plugins.workflow.support.steps.input.InputStepExecution;
+import static org.junit.Assert.*;
 import org.junit.Test;
 import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.recipes.LocalData;
 
 public class InputStepRestartTest extends SingleJobTestBase {
 
+    @Issue("JENKINS-25889")
     @Test public void restart() {
         story.addStep(new Statement() {
             @Override public void evaluate() throws Throwable {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition("input 'paused'"));
                 startBuilding();
-                waitForWorkflowToSuspend();
-                assertTrue(b.isBuilding());
+                story.j.waitForMessage("paused", b);
             }
         });
         story.addStep(new Statement() {
             @Override public void evaluate() throws Throwable {
                 rebuildContext(story.j);
-                assertThatWorkflowIsSuspended();
-                StepExecution.applyAll(InputStepExecution.class, new Function<InputStepExecution,Void>() {
-                    @Override public Void apply(InputStepExecution exec) {
-                        try {
-                            exec.doProceedEmpty();
-                        } catch (IOException x) {
-                            assert false : x;
-                        }
-                        return null;
-                    }
-                }).get();
+                proceed(b);
                 story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b));
+                sanity(b);
+            }
+        });
+    }
+
+    private void proceed(WorkflowRun b) throws Exception {
+        InputAction a = b.getAction(InputAction.class);
+        assertNotNull(a);
+        assertEquals(1, a.getExecutions().size());
+        story.j.submit(story.j.createWebClient().getPage(b, a.getUrlName()).getFormByName(a.getExecutions().get(0).getId()), "proceed");
+    }
+
+    private void sanity(WorkflowRun b) throws Exception {
+        List<PauseAction> pauses = new ArrayList<PauseAction>();
+        for (FlowNode n : new FlowGraphWalker(b.getExecution())) {
+            pauses.addAll(PauseAction.getPauseActions(n));
+        }
+        assertEquals(1, pauses.size());
+        assertFalse(pauses.get(0).isPaused());
+        String xml = FileUtils.readFileToString(new File(b.getRootDir(), "build.xml"));
+        assertFalse(xml, xml.contains(InputStepExecution.class.getName()));
+    }
+
+    @Issue("JENKINS-25889")
+    @LocalData // from 1.4.2
+    @Test public void oldFlow() {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                WorkflowJob p = jenkins().getItemByFullName("p", WorkflowJob.class);
+                assertNotNull(p);
+                WorkflowRun b = p.getLastBuild();
+                assertNotNull(b);
+                assertEquals(1, b.getNumber());
+                proceed(b);
+                story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b));
+                sanity(b);
             }
         });
     }

diff --git a/...est/resources/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest/oldFlow.zip b/...est/resources/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest/oldFlow.zip
diff --git a/support/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputAction.java b/support/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputAction.java
@@ -6,16 +6,22 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nonnull;
+import org.jenkinsci.plugins.workflow.flow.FlowExecution;
+import org.jenkinsci.plugins.workflow.flow.FlowExecutionList;
+import org.jenkinsci.plugins.workflow.steps.StepExecution;
 
 /**
  * Records the pending inputs required.
- *
- * @author Kohsuke Kawaguchi
  */
 public class InputAction implements RunAction2 {
 
-    private final List<InputStepExecution> executions = new ArrayList<InputStepExecution>();
+    private static final Logger LOGGER = Logger.getLogger(InputAction.class.getName());
+
+    private transient List<InputStepExecution> executions = new ArrayList<InputStepExecution>();
+    private List<String> ids = new ArrayList<String>();
 
     private transient Run<?,?> run;
 
@@ -27,9 +33,51 @@ public void onAttached(Run<?, ?> r) {
     @Override
     public void onLoad(Run<?, ?> r) {
         this.run = r;
-        assert executions != null && !executions.contains(null) : executions;
-        for (InputStepExecution step : executions) {
-            step.run = run;
+        synchronized (this) {
+            if (ids == null) {
+                // Loading from before JENKINS-25889 fix. Load the IDs and discard the executions, which lack state anyway.
+                assert executions != null && !executions.contains(null) : executions;
+                ids = new ArrayList<String>();
+                for (InputStepExecution execution : executions) {
+                    ids.add(execution.getId());
+                }
+                executions = null;
+            }
+        }
+    }
+
+    private synchronized void loadExecutions() {
+        if (executions == null) {
+            executions = new ArrayList<InputStepExecution>();
+            try {
+            FlowExecution execution = null;
+            for (FlowExecution _execution : FlowExecutionList.get()) {
+                if (_execution.getOwner().getExecutable() == run) {
+                    execution = _execution;
+                    break;
+                }
+            }
+            if (execution != null) {
+                // Futures.addCallback is the safer way to iterate, but we need to know that the result is in.
+                // And we cannot start the calculation during onLoad because we are still inside WorkflowRun.onLoad and the CpsFlowExecution is not yet initialized.
+                // WorkflowRun has getExecutionPromise but that is not accessible via API.
+                for (StepExecution se : execution.getCurrentExecutions(true).get()) {
+                    if (se instanceof InputStepExecution) {
+                        InputStepExecution ise = (InputStepExecution) se;
+                        if (ids.contains(ise.getId())) {
+                            executions.add(ise);
+                        }
+                    }
+                }
+                if (executions.size() < ids.size()) {
+                    LOGGER.log(Level.WARNING, "some input IDs not restored from {0}", run);
+                }
+            } else {
+                LOGGER.log(Level.WARNING, "no flow execution found for {0}", run);
+            }
+            } catch (Exception x) {
+                LOGGER.log(Level.WARNING, null, x);
+            }
         }
     }
 
@@ -39,12 +87,14 @@ public void onLoad(Run<?, ?> r) {
 
     @Override
     public String getIconFileName() {
+        loadExecutions();
         if (executions.isEmpty())    return null;
         else                    return "help.png";
     }
 
     @Override
     public String getDisplayName() {
+        loadExecutions();
         if (executions.isEmpty())    return null;
         else                    return "Paused for Input";
     }
@@ -55,11 +105,14 @@ public String getUrlName() {
     }
 
     public synchronized void add(@Nonnull InputStepExecution step) throws IOException {
+        loadExecutions();
         this.executions.add(step);
+        ids.add(step.getId());
         run.save();
     }
 
     public synchronized InputStepExecution getExecution(String id) {
+        loadExecutions();
         for (InputStepExecution e : executions) {
             if (e.input.getId().equals(id))
                 return e;
@@ -68,14 +121,17 @@ public synchronized InputStepExecution getExecution(String id) {
     }
 
     public synchronized List<InputStepExecution> getExecutions() {
+        loadExecutions();
         return new ArrayList<InputStepExecution>(executions);
     }
 
     /**
      * Called when {@link InputStepExecution} is completed to remove it from the active input list.
      */
     public synchronized void remove(InputStepExecution exec) throws IOException {
+        loadExecutions();
         executions.remove(exec);
+        ids.remove(exec.getId());
         run.save();
     }
 

diff --git a/.../src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java b/.../src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
@@ -39,11 +39,8 @@
  * @author Kohsuke Kawaguchi
  */
 public class InputStepExecution extends AbstractStepExecutionImpl implements ModelObject {
-    /**
-     * Pause gets added here.
-     */
-    @StepContextParameter
-    /*package*/ transient Run run;
+
+    @StepContextParameter private transient Run run;
 
     @StepContextParameter private transient TaskListener listener;
 
@@ -225,7 +222,11 @@ private void postSettlement() throws IOException {
             getPauseAction().remove(this);
             run.save();
         } finally {
-            PauseAction.endCurrentPause(node);
+            if (node != null) {
+                PauseAction.endCurrentPause(node);
+            } else {
+                assert false : "cannot set pause end time for " + getId() + " in " + run;
+            }
         }
     }